System Utility to Create Random Files

Question

The following program makes random files. By default, they are of the form /tmp/rfile-XXX. Certain options allow for the user to specify where these files are placed(using the -d dir flag) or what name is used to generate (using the -n name flag, which generates name-XXX) and then uses mkstemp(3) followed by fopen(3) In context of this review I am interested in the following:

1. How can I make this code more efficient? I have to repeat a lot of checks (like the if(print_flag == USER) to test if the user wants a more verbose output or having to test dir_flag multiple times to determine what directory name and length thereof. Would it be better in the 2nd implementation to use pthreads to create each file, and just generate a list of /dir/name-XXX for each thread to act on?

2. Error handling and security, what needs to be changed or added to handle other failures or problems? Are there other functions that I need to check for error? In version 2 I want to implement a check of fullname_len against the maximum pathname(I don't recall the variable/value). What else could potentially error out, or otherwise cause problems?

3. I tried to keep the formatting consistent throughout but I would like some input on that. I know my code (intentionally) looks more like the BSD style(9) guide, and I don't want to hear if other style standards/guides are better. I just want to know if it's consistent and conducive to code reviewing/reading.

/*  FILE: mkrfiles.c
    AUTHOR: Jordan Effinger
    DATE: 1 May 2023
    VERSION: 0
    
    LICENSE: BSD-3 Clause

    COPYRIGHT<2023><Jordan Effinger>
    Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following conditions are met:

    1. Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.

    2. Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in the documentation and/or other materials provided with the     distribution.

    3. Neither the name of the copyright holder nor the names of its contributors may be used to endorse or promote products derived from this software without specific prior written permission.

    THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS “AS IS” AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND     FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL        DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,        WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

    COMMENT: This program was written as an exercise in argument parsing, error handling, user i/o and in printing verbose error messages. It was written on a MacOS 10.15.7, with      Apple clang 12.0.0 on an x86_64 system.
*/

#include <string.h>
#include <unistd.h>

#include <stdio.h>
#include <stdlib.h>

#include <sys/errno.h>

#define DFLT 0
#define USER 1

void
print_error(char *, pid_t, char *, unsigned, unsigned);

unsigned uerrno = 0;

int
main(int argc, char **argv)
{
    //  Debugging information:
    pid_t main_pid = getpid();  
    char *exec_name = calloc( strlen(argv[0]), sizeof(char) );
    if(exec_name == NULL)
    {
        print_error(argv[0], main_pid, __FILE__, __LINE__, errno);
        return(errno);
    }

    //  Default settings:
    const char *dir_dflt = "/tmp";
    const char *name_dflt = "rfile";
    const char *fullname_dflt = "/tmp/rfile-XXX";

    const size_t dir_len_dflt = strlen(dir_dflt);
    const size_t name_len_dflt = strlen(name_dflt);
    const size_t fullname_len_dflt = strlen(fullname_dflt)+1;

    //  User set settings:
    char *dir_user, *name_user;
    size_t dir_len, name_len, fullname_len = 0;
    unsigned long count_user;
    
    int count_flag =0;
    int dir_flag = 0;
    int name_flag = 0;
    int print_flag = 0;

    int ch;
    while( (ch = getopt(argc, argv, "c:d:hn:p")) != -1)
    {
        switch(ch)
        {
            case 'c':
                count_flag = USER;
                count_user = strtoul(optarg,NULL,0);    
                if(errno != 0)
                {
                    print_error(exec_name, main_pid, __FILE__, __LINE__, errno);
                    free(exec_name);
                    return(errno);
                    break;
                }
                break;

            case 'd':
                dir_flag = USER;
                dir_len = strlen(optarg);
                dir_user = calloc(dir_len, sizeof(char) );
                if(dir_user == NULL)
                {
                    print_error(exec_name, main_pid, __FILE__, __LINE__, errno);
                    free(exec_name);
                    return(errno);
                    break;
                }
                stpncpy(dir_user, optarg, dir_len);
                break;

            case 'h':
                printf("%s -cdhnp\n", exec_name);
                printf("-c file count\n");
                printf("-d dir name\n");
                printf("-h help\n");
                printf("-n file name template\n");
                printf("-p print (verbose)\n");
                return(-1);
                break;

            case 'n':
                name_flag = USER;
                name_len = strlen(optarg);
                name_user = calloc(name_len, sizeof(char) );
                if(name_user == NULL)
                {
                    print_error(exec_name, main_pid, __FILE__, __LINE__, errno);
                    free(exec_name);
                    return(errno);
                    break;
                }
                stpncpy(name_user, optarg, name_len);
                break;

            case 'p':
                print_flag = USER;
                break;

            default:
                print_error(exec_name, main_pid, __FILE__, __LINE__, EINVAL);
                printf("Execute with -h for a usage menu.\n");
                free(exec_name);
                return(EINVAL);
                break;
        }
    }

    if(print_flag == USER)
    {
        printf("Determining filename length.\n");
    }
    if( dir_flag == USER && name_flag == USER)
    {
        fullname_len = dir_len + strlen("/") + name_len + strlen("-XXX") + 1;
    }
    else if(dir_flag == USER && name_flag == DFLT)
    {
        fullname_len = dir_len + strlen("/") + name_len_dflt + strlen("-XXX") + 1;
    }
    else if(dir_flag == DFLT && name_flag == USER)
    {
        fullname_len = dir_len_dflt + strlen("/") + name_len + strlen("-XXX") + 1;
    }
    else if(dir_flag == DFLT && name_flag == DFLT)
    {
        fullname_len = fullname_len_dflt;
    }

    if(print_flag == USER)
    {
        printf("Allocating space for filename: %zu = %zu(strlen) & %zu(char size)\n", fullname_len * sizeof(char), fullname_len, sizeof(char) );
    }
    char *fullname = calloc(fullname_len, sizeof(char));
    if(fullname == NULL)
    {
        print_error(exec_name, main_pid, __FILE__, __LINE__, errno);    
        free(exec_name);
        free(dir_user);
        free(name_user);
        return(uerrno);
    }

    if(print_flag == USER)
    {
        printf("Building filename:\n");
    }

    if(dir_flag == USER)
    {
        stpncpy(fullname, dir_user, dir_len);
    }
    else
    {
        stpncpy(fullname, dir_dflt, dir_len_dflt);
    }

    strcat(fullname,"/");

    if(name_flag == USER)
    {
        strncat(fullname, name_user, name_len);
    }
    else
    {
        strncat(fullname, name_dflt, name_len_dflt);
    }

    strncat(fullname, "-XXX", strlen("-XXX") );

    FILE *fd;
    if(count_flag == USER)
    {
        if(print_flag == USER)
        {
            printf("Creating %zu random files.\n", count_user);
        }   
        for( unsigned long idx = 0; idx < count_user; idx++ ) 
        {
            mkstemp(fullname);
            fd = fopen(fullname, "w");
            if(fd == NULL)
            {
                print_error(exec_name, main_pid, __FILE__, __LINE__, errno);
                free(exec_name);
                free(dir_user);
                free(name_user);
                free(fullname);
                return(errno);      
            }
            fclose(fd);

            fullname[fullname_len-2] = 'X';
            fullname[fullname_len-3] = 'X';
            fullname[fullname_len-4] = 'X';
        }
    }
    else
    {
        if(print_flag == USER)
        {
            printf("Creating random file.\n");
        }
        
        mkstemp(fullname);
        fd = fopen(fullname, "w");
        if(fd == NULL)
        {
            print_error(exec_name, main_pid, __FILE__, __LINE__, errno);
            free(exec_name);
            free(dir_user);
            free(name_user);
            free(fullname);
            return(errno);
        }
        fclose(fd);
    }

    free(exec_name);
    free(dir_user);
    free(name_user);
    free(fullname);
    return(uerrno); 
}
void
print_error(char *progname, pid_t main_pid, char *file, unsigned line, unsigned errnum)
{
    fprintf(stderr, "[%s, %u] [%s, line:%u], Errno: %u, Error: %s\n",
            progname, main_pid, file, line, errnum, strerror(errnum)
           );
}
```

Answer 1

Write modular code:

Short-term memory and the field of vision are small. The function main() is too large, does too much, and hence is hard to read. Explain small tasks as pure functions whenever possible.

An example follows:

static inline void usage (const char *exec_name)
{
    printf ("%s -cdhnp\n"
            "-c file count\n"
            "-d dir name\n"
            "-h help\n"
            "-n file name template\n"
            "-p print (verbose)\n", 
            exec_name);
}

Now we can replace:

case 'h':
    printf("%s -cdhnp\n", exec_name);
    printf("-c file count\n");
    printf("-d dir name\n");
    printf("-h help\n");
    printf("-n file name template\n");
    printf("-p print (verbose)\n");
    return(-1);
    break;

with:

case 'h':
    usage();
    return EXIT_FAILURE;

---

Code risks invoking undefined behaviour:

argv[0] is passed to print_error() in main(), yet it was never sanitized:

int 
main (int argc, char **argv) 
{
    /* POSIX requires the calling program to pass in a non-null argv[0]. */
    if (!argv) { // Add
       complain();
    }
}

---

Simplify code:

/* 
if( dir_flag == USER && name_flag == USER)
    {
        fullname_len = dir_len + strlen("/") + name_len + strlen("-XXX") + 1;
    }
    else if(dir_flag == USER && name_flag == DFLT)
    {
        fullname_len = dir_len + strlen("/") + name_len_dflt + strlen("-XXX") + 1;
    }
    else if(dir_flag == DFLT && name_flag == USER)
    {
        fullname_len = dir_len_dflt + strlen("/") + name_len + strlen("-XXX") + 1;
    }
    else if(dir_flag == DFLT && name_flag == DFLT)
    {
        fullname_len = fullname_len_dflt;
    }
*/

if (dir_flag == USER) {
    size_t len = name_flag == USER ? name_len : name_len_dflt;
    fullname_len = dir_len + strlen("/") + size + strlen("-XXX") + 1;
} else {
    fullname_len = name_flag == USER ?
                   dir_len_dflt + strlen("/") + name_len + strlen("-XXX") + 1 :
                   fullname_len_dflt;
}

/*
fullname[fullname_len-2] = 'X';
fullname[fullname_len-3] = 'X';
fullname[fullname_len-4] = 'X';
        
*/

memcpy (fullname + fullname_len - 4, "XXX", 3);

---

Reduce duplication:

I dislike such redundancy:

free(exec_name);
free(dir_user);
free(name_user);
free(fullname);

But we can clean it up with some macro-trickery:

#define Fn_apply(type, fn, ...) {                                      \
    void *stopper_for_apply = (int[]){0};                              \
    type **list_for_apply = (type*[]){__VA_ARGS__, stopper_for_apply}; \
    for (size_t i = 0; list_for_apply[i] != stopper_for_apply; i++)         \
         fn(list_for_apply[i]);                                        \
}

#define Free_all(...) Fn_apply(void, free, __VA_ARGS__);

/* @Credit: Ben Klemens */
/* Could the above have a non-standard practice? Perhaps, but it's one option. */

Now we can simply write:

Free_all (exec_name, dir_user, name_user, fullname);

Or if that's not a feasible solution for you, we can use goto:

  out_free_all:
    free(exec_name);
    free(dir_user);
    free(name_user);
    free(fullname);
    return(errno);

Now we can simply write:

goto out_free_all;

Instead of repeating all those calls to free() at every error.

A similar label can be used for:

print_error(exec_name, main_pid, __FILE__, __LINE__, errno);
free(exec_name);
return(errno);
break;

which is repeated in almost all the switch() cases.

---

Write error messages to stderr:

// printf("Execute with -h for a usage menu.\n");
   fputs ("Execute with -h for a usage menu.\n", stderr);

stderr is unbuffered, stdout is not.

---

Potential bug:

Code tests errno after calling strtoul, yet doesn't clear errno before it.

#if 0
   count_user = strtoul(optarg,NULL,0);                  
   if(errno != 0)
#else
   errno = 0; // Add
   count_user = strtoul(optarg,NULL,0);                  
   if(errno != 0)
#endif

See also: Correct usage of strtol().

---

Adjacent string literals are concatenated into one:

So calling printf() once suffices:

#if 0
   printf("%s -cdhnp\n", exec_name);
   printf("-c file count\n");
   printf("-d dir name\n");
   printf("-h help\n");
   printf("-n file name template\n");
   printf("-p print (verbose)\n");             
#else
   printf ("%s -cdhnp\n"
           "-c file count\n"
           "-d dir name\n"
           "-h help\n"
           "-n file name template\n"
           "-p print (verbose)\n", exec_name);
#endif

---

File-scope variables are frowned upon:

unsigned uerrno = 0;

There's no need for uerrno to be declared outside of main().

---

Arguments that are not supposed to be modified should be declared with the const qualifier:

An object of const-qualified is read-only.

void
print_error(const char *progname, pid_t main_pid, const char *file, unsigned line, unsigned errnum)

restrict could allow for select optimizations:

void
print_error(const char *restrict progname, pid_t main_pid, const char *restrict file, unsigned line, unsigned errnum)

And as the function is only used in this translation unit, it should be declared with having internal linkage:

static void print_error (print_error(const char *restrict progname, pid_t main_pid, const char *restrict file, unsigned line, unsigned errnum) 

/* Yes, it could be marked inline too. But the compiler can take care of that. */

---

Use more const:

#if 0
    //  Default settings:
    const char *dir_dflt = "/tmp";
    const char *name_dflt = "rfile";
    const char *fullname_dflt = "/tmp/rfile-XXX";
#else    
    //  Default settings:
    const char *const dir_dflt = "/tmp";
    const char *const name_dflt = "rfile";
    const char *const fullname_dflt = "/tmp/rfile-XXX";
#endif

---

Is returning the errno value a BSD convention?

return(errno); // ??

I suggest:

return EXIT_FAILURE;

---

Use the standard bool type:

With the inclusion of:

#include <stdbool.h>

We can declare variables of type bool and use true and false instead of USER and DFLT, which aren't very unambiguous.

#if 0
    int count_flag =0;
    int dir_flag = 0;
    int name_flag = 0;
    int print_flag = 0;
#else 
    bool count_flag = dir_flag = name_flag = print_flag = false;
#endif

---

Check the return values of library/system calls for failure:

#if 0
    mkstemp (fullname);
#else
    if (mkstemp (fullname) == -1) {
        complain ();
    }
#endif

---

Minor:

• We won't require the forward declaration for print_error() if we simply define it before main().
• We don't require the inclusion of <sys/errno.h>, the standard <errno.h> suffices.
• In the original C specification, parentheses were required around the return value. That's no longer the case, and it makes return look like a function call. Simply:

   return a;

instead of:

   return(a);

• stpncpy() is a non-standard function. Consider using strcpy() / strncpy() instead.
• sizeof(char) is defined by the standard to be 1, so you can leave it out.
• We can group together the directory names and their lengths by defining a struct like so:

    struct dir_info {
        const char *const dir_name;
        const size_t len;
    };

• fd has always conventionally been used to hold a file descriptor, not a file pointer. Consider naming it stream or fp.
• stpncpy() returns a pointer to the terminating null byte in dest, or, if dest is not null-terminated, dest + n. Save that and use it for further concatenations instead of calling strcat().