13
\$\begingroup\$

I recently made a tool called mkfh to create a FHS compliant filesystem structure.
I aimed to make it as portable as possible, so I wrote it in C89 and also tried to catch the vibe of old-school tools!

Did I achieve that? Is it really that portable? Anything I could improve?

It consists of 2 files, mkfh.c and usr.c, where the later defines umkdir and utouch to create a directory and file respectively. As these could potentially be the least portable, I put them in a separate file so someone porting it can easily modify the source code.

Note that usr.c is inherently not very portable and takes some liberties.

*portable as in "works on very old and very new *nix systems", not potentially windows NT and such.

mkfh.c:

/* Source: MKFH, License: MIT */ 

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

#ifndef EXIT_SUCCESS 
# define EXIT_SUCCESS 0 
#endif
#ifndef EXIT_FAILURE
# define EXIT_FAILURE 1 
#endif

#define OPTIONAL 1 
#define REQUIRED 0
#define USR_SUCCESS 1 
#define USR_FAIL 0

#define SEPERATOR "/"

/* user functions */
extern int umkdir(const char*);
extern int utouch(const char*);

/* options */ 
int ask_optional = 0; /* 0 ask ; 1 no ; 2 yes */
int silent       = 0;
int verbose      = 0;
int nouser       = 0;
int quiterr      = 0;
char* progname   = 0;
char* root       = 0;

/* NOTE: old C compilers have global struct 
   member mangling, so added a prefix */
struct dir {
    char*       dir_idf;
    struct dir* dir_sub;
    int         dir_opt;
};

int usage(void);
int help(void);
int ask(char*);
int vout(char*);
int trav(struct dir*, char*);
struct dir tree(void);

int main(int argc, char** argv) {
    struct dir tr;
    char* options = 0;

    progname = *argv;
    if(argc == 2) root = argv[1];
    else if(argc == 3) options = argv[1],
                        root = argv[2];
    else usage();

    if(!options && ( !strcmp(root, "-h") || !strcmp(root, "-u") ))
        options = root;

    if(options != 0)
        while(*options)
            switch(*options++) {
                case 'a': ask_optional = 1; 
                          break;
                case 'A': ask_optional = 2;
                          break;
                case 'h': help();
                          break;
                case 'q': quiterr = !quiterr;
                          break;
                case 's': silent = !silent;
                          break;
                case 'u': usage();
                          break;
                case 'U': nouser = !nouser;
                          break;
                case 'v': verbose = !verbose;
                          break;
                case '-':
                          break;
                default:
                          fprintf(stderr, "Unknown option %c\n",
                                  *--options);
                          exit(EXIT_FAILURE);
            }

    tr = tree();
    trav(&tr, "");

    /* tree freeing itself upon exit */

    return EXIT_SUCCESS;
}

int help(void) {
    fprintf(stderr,
        "%s (make filesystem hierarchy)\n"
        "LabRicecat (License: MIT) (FHS: 3.0)\n\n"
        "Usage: %s [-aAhsvuU] ROOT\n"
        "Creates a file hierarchy at ROOT\n\n"
        "Options:\n"
        "    a : NO to all optionals\n"
        "    A : YES to all optionals\n"
        "    q : dont quit on errors\n"
        "    h : this\n"
        "    u : show usage\n"
        "    U : dont execute user functions\n"
        "    v : verbose output\n"
        "    - : nothing\n"
    , progname, progname);
    exit(EXIT_SUCCESS);
}

int usage(void) {
    fprintf(stderr, "Usage: %s [-aAhqsvuU] ROOT\n", progname);
    exit(EXIT_FAILURE);
}

/* gets one char and eats the rest */
int cgetc(void) {
    int c, r;
    c = r = getchar();
    while(c != '\n' && c != EOF)
        c = getchar();

    return r;
}

int acc(char c) {
    return c == 'y'
        || c == 'Y';
}

int ask(char* c) {
    if(ask_optional == 1) return USR_FAIL;
    if(ask_optional == 2) return USR_SUCCESS;

    fprintf(stderr,"Create %s [y/N]:",c);
    return acc(cgetc());
}

int vout(char* c) {
    if(verbose)
        fputs(c, stdout);
    return verbose;
}

struct dir newdir(char* name, int opt, struct dir sub[]) {
    struct dir d;

    d.dir_idf = name;
    d.dir_sub = sub;
    d.dir_opt = opt;

    return d;
}

struct dir* dirlst(int vlz, ...) {
    struct dir* ds;
    int i;
    va_list vl;

    ds = malloc(sizeof(struct dir) * (vlz + 1));
    va_start(vl, vlz);
    
    for(i = 0; i < vlz; ++i)
        ds[i] = va_arg(vl,struct dir);

    ds[vlz] = newdir(NULL, REQUIRED, NULL);

    va_end(vl);
    return ds;
}

char* strsum(char* s1, char* s2) {
    int sz, i;    
    char* r;

    sz = strlen(s1) + strlen(s2);
    r = malloc(sz + 1);
    r[sz] = 0;

    for(i = 0; i < sz; ++i)
        r[i] = *s1 ? *(s1)++ : *(s2++);

    return r;
}

/* joins directories */
char* djoin(char* parent, char* child) {
    char* r, *c, *sp;

    sp = SEPERATOR;
    if(!strcmp(parent, SEPERATOR) || !strcmp(parent,""))
        sp = "";

    c = strsum(parent, sp);
    r = strsum(c, child);
    free(c);

    return r;
}

/* traverses the tree */
int trav(struct dir* d, char* parent) {
    char* nm, *c;

    nm = djoin(parent, d->dir_idf);    
    if(!d || ( d->dir_opt && ask(nm) )) {
        free(nm);
        return EXIT_FAILURE;
    }

    vout(nm);
    vout("\n");

    while(d->dir_sub && d->dir_sub->dir_idf) {
        c = djoin(nm, d->dir_sub->dir_idf);

        if(!nouser && umkdir(c) == USR_FAIL) {
            free(c); 
            fputs("umkdir() failed", stderr);
            if(!quiterr)
                break;
        }
        else 
            free(c);
        
        trav(d->dir_sub, nm);
        ++d->dir_sub;
    }

    free(nm);
    return EXIT_SUCCESS;
}

struct dir tree(void) {
    return
        newdir(root, REQUIRED, dirlst(14, 
            newdir("bin", REQUIRED, NULL),
            newdir("boot", REQUIRED, NULL),
            newdir("dev", REQUIRED, NULL),
            newdir("etc", REQUIRED,
                dirlst(4,
                    newdir("opt", REQUIRED, NULL),
                    newdir("X11", OPTIONAL, NULL),
                    newdir("sgml", OPTIONAL, NULL),
                    newdir("xml", OPTIONAL, NULL)
                )),           
            newdir("lib", REQUIRED,
                dirlst(1,
                    newdir("modules", OPTIONAL, NULL)
                )),       
            newdir("media", REQUIRED, 
                dirlst(4,
                    newdir("floppy", OPTIONAL, NULL),
                    newdir("cdrom", OPTIONAL, NULL),
                    newdir("cdrecorder", OPTIONAL, NULL),
                    newdir("zip", OPTIONAL, NULL)
                )),
            newdir("mnt", REQUIRED, NULL),
            newdir("opt", REQUIRED, NULL),
            newdir("run", REQUIRED, NULL),
            newdir("sbin", REQUIRED, NULL),
            newdir("srv", REQUIRED, NULL),
            newdir("tmp", REQUIRED, NULL),
            newdir("usr", REQUIRED, 
                dirlst(10,
                    newdir("bin", REQUIRED, NULL),
                    newdir("lib", REQUIRED, NULL),
                    newdir("local", REQUIRED, 
                        dirlst(10,
                            newdir("bin", REQUIRED, NULL),
                            newdir("etc", REQUIRED, NULL),
                            newdir("games", REQUIRED, NULL),
                            newdir("include", REQUIRED, NULL),
                            newdir("lib", REQUIRED, NULL),
                            newdir("man", REQUIRED, NULL),
                            newdir("sbin", REQUIRED, NULL),
                            newdir("share", REQUIRED,
                                dirlst(15,
                                    newdir("man", REQUIRED, 
                                        dirlst(8,
                                            newdir("man1", OPTIONAL, NULL),
                                            newdir("man2", OPTIONAL, NULL),
                                            newdir("man3", OPTIONAL, NULL),
                                            newdir("man4", OPTIONAL, NULL),
                                            newdir("man5", OPTIONAL, NULL),
                                            newdir("man6", OPTIONAL, NULL),
                                            newdir("man7", OPTIONAL, NULL),
                                            newdir("man8", OPTIONAL, NULL)
                                        )),
                                    newdir("misc", REQUIRED, NULL),
                                    newdir("color", OPTIONAL, 
                                        dirlst(1,
                                            newdir("icc", OPTIONAL, NULL)
                                        )),
                                    newdir("dict", OPTIONAL, NULL),
                                    newdir("doc", OPTIONAL, NULL),
                                    newdir("games", OPTIONAL, NULL),
                                    newdir("info", OPTIONAL, NULL),
                                    newdir("locale", OPTIONAL, NULL),
                                    newdir("nls", OPTIONAL, NULL),
                                    newdir("ppd", OPTIONAL, NULL),
                                    newdir("sgml", OPTIONAL, 
                                        dirlst(4,
                                            newdir("docbook", OPTIONAL, NULL),
                                            newdir("tei", OPTIONAL, NULL),
                                            newdir("html", OPTIONAL, NULL),
                                            newdir("mathml", OPTIONAL, NULL)
                                        )),
                                    newdir("terminfo", OPTIONAL, NULL),
                                    newdir("tmac", OPTIONAL, NULL),
                                    newdir("xml", OPTIONAL, 
                                        dirlst(3,
                                            newdir("docbook", OPTIONAL, NULL),
                                            newdir("xhtml", OPTIONAL, NULL),
                                            newdir("mathml", OPTIONAL, NULL)
                                        )),
                                    newdir("zoneinfo", OPTIONAL, NULL)
                                )),
                            newdir("src", REQUIRED, NULL),
                            newdir("lib64", OPTIONAL, NULL)
                        )),
                    newdir("sbin", REQUIRED, NULL),
                    newdir("share", REQUIRED, 
                        dirlst(15,
                            newdir("man", REQUIRED, 
                                dirlst(8,
                                    newdir("man1", OPTIONAL, NULL),
                                    newdir("man2", OPTIONAL, NULL),
                                    newdir("man3", OPTIONAL, NULL),
                                    newdir("man4", OPTIONAL, NULL),
                                    newdir("man5", OPTIONAL, NULL),
                                    newdir("man6", OPTIONAL, NULL),
                                    newdir("man7", OPTIONAL, NULL),
                                    newdir("man8", OPTIONAL, NULL)
                                )),
                            newdir("misc", REQUIRED, NULL),
                            newdir("color", OPTIONAL, 
                                dirlst(1,
                                    newdir("icc", OPTIONAL, NULL)
                                )),
                            newdir("dict", OPTIONAL, NULL),
                            newdir("doc", OPTIONAL, NULL),
                            newdir("games", OPTIONAL, NULL),
                            newdir("info", OPTIONAL, NULL),
                            newdir("locale", OPTIONAL, NULL),
                            newdir("nls", OPTIONAL, NULL),
                            newdir("ppd", OPTIONAL, NULL),
                            newdir("sgml", OPTIONAL, 
                                dirlst(4,
                                    newdir("docbook", OPTIONAL, NULL),
                                    newdir("tei", OPTIONAL, NULL),
                                    newdir("html", OPTIONAL, NULL),
                                    newdir("mathml", OPTIONAL, NULL)
                                )),
                            newdir("terminfo", OPTIONAL, NULL),
                            newdir("tmac", OPTIONAL, NULL),
                            newdir("xml", OPTIONAL, 
                                dirlst(3,
                                    newdir("docbook", OPTIONAL, NULL),
                                    newdir("xhtml", OPTIONAL, NULL),
                                    newdir("mathml", OPTIONAL, NULL)
                                )),
                            newdir("zoneinfo", OPTIONAL, NULL)
                        )),
                    newdir("games", OPTIONAL, NULL),
                    newdir("include", OPTIONAL, 
                        dirlst(1,
                            newdir("bsd", OPTIONAL, NULL)
                        )),
                    newdir("libexec", OPTIONAL, NULL),
                    newdir("lib64", OPTIONAL, NULL),
                    newdir("src", OPTIONAL, NULL)
                )),
            newdir("var", REQUIRED, 
                dirlst(14,
                    newdir("cache", REQUIRED, 
                        dirlst(3,
                            newdir("fonts", OPTIONAL, NULL),
                            newdir("man", OPTIONAL, NULL),
                            newdir("www", OPTIONAL, NULL)
                        )),
                    newdir("lib", REQUIRED, 
                        dirlst(4,
                            newdir("misc", REQUIRED, NULL),
                            newdir("color", OPTIONAL, NULL),
                            newdir("hwclock", OPTIONAL, NULL),
                            newdir("xdm", OPTIONAL, NULL)
                        )),
                    newdir("local", REQUIRED, NULL),
                    newdir("lock", REQUIRED, NULL),
                    newdir("log", REQUIRED, NULL),
                    newdir("opt", REQUIRED, NULL),
                    newdir("run", REQUIRED, NULL),
                    newdir("spool", REQUIRED, 
                        dirlst(5,
                            newdir("lpd", OPTIONAL, 
                                dirlst(1,
                                    newdir("printer", OPTIONAL, NULL)
                                )),
                            newdir("mqueue", OPTIONAL, NULL),
                            newdir("news", OPTIONAL, NULL),
                            newdir("rwho", OPTIONAL, NULL),
                            newdir("uucp", OPTIONAL, NULL)
                        )),
                    newdir("tmp", REQUIRED, NULL),
                    newdir("account", OPTIONAL, NULL),
                    newdir("crash", OPTIONAL, NULL),
                    newdir("games", OPTIONAL, NULL),
                    newdir("mail", OPTIONAL, NULL),
                    newdir("yp", OPTIONAL, NULL)
                )),
            newdir("home", OPTIONAL, NULL),
            newdir("root", OPTIONAL, NULL),
            newdir("lib64", OPTIONAL, NULL)
        ));
}

usr.c

#include <sys/stat.h>
#include <stdio.h>

#define USR_SUCCESS 1
#define USR_FAIL 0

/* define user functions here */

/* create a directory at relative path PATH */ 
int umkdir(const char* path) {
    struct stat st = {0};
    
    if(stat(path, &st) == -1)
        return mkdir(path, 0700) == 0 ? USR_SUCCESS : USR_FAIL;

    return USR_SUCCESS;
}

/* create a file at relative path PATH */
/* currently unused */
int utouch(const char* path) {
    FILE* f;

    f = fopen(path, "r");
    if(!f)
        return USR_FAIL;

    fclose(f);
    return USR_SUCCESS;
}
\$\endgroup\$
9
  • 1
    \$\begingroup\$ To small for an answer, but I'd omit the counter from dirlst and end each listing with NULL instead. The way you're doing it right now is way too susceptible to errors if you miscount (and adding/removing a directory won't require you to find the corresponding counter and adjust it). \$\endgroup\$ Commented Jan 9 at 19:10
  • 2
    \$\begingroup\$ What is “the vibe of old-school tools”? Command-line tools haven’t changed much since the 1980’s, how can you tell the difference? \$\endgroup\$ Commented Jan 9 at 23:14
  • 1
    \$\begingroup\$ Depending on how "very old" you're interested in, you might consider using getopt() to parse your options (and then deal with the excitement of figuring out which header provides it on the system in question). \$\endgroup\$
    – Mark
    Commented Jan 10 at 3:57
  • \$\begingroup\$ Cris Luengo What I mean by that, is it does one thing and that well. Nothing else, depends on no config files, does not put any files onto your system to save data and can be run anytime anywhere. I agree there is no strict line, and highly subjective. \$\endgroup\$
    – Labricecat
    Commented Jan 10 at 11:27
  • \$\begingroup\$ Mark Interesting! I will have a look at that, thanks for the suggestion! \$\endgroup\$
    – Labricecat
    Commented Jan 10 at 11:27

5 Answers 5

10
\$\begingroup\$

EXIT_SUCCESS and EXIT_FAILURE

The language spec promises that EXIT_SUCCESS and EXIT_FAILURE are defined by stdlib.h. Having included that header, you do not need to check whether they are defined. Moreover, the point of these is that the specific values (other than 0) that convey success and failure to the exit() function are not necessarily portable, so if you could not rely on these macros in fact being defined, then defining them yourself would conflict with your goal of maximum portability.

Define headers

Your mkfh.c and usr.c both declare some of the same functions and define some of the same macros, each independent of the other. The definitions of these macros and (forward) declarations of these functions should go in a header file, which both source files #include.

"global struct member wrangling"

I guess the comment about struct member wrangling refers to ancient versions of C in which the identifiers of struct members were not scoped to the struct definitions in which they appeared. This is the reason for some of the very old standard struct types having member names with distinctive prefixes. But you have chosen C89 for portability, and that can provide portability only for C implementations that in fact conform to C89 or, with some limitations, to one of its successors. There is nothing that could be characterized as struct member wrangling in C89.

It's not wrong, per se, to prefix the names of your structure members, but it's superfluous. And if it means you limit yourself to fewer characters for expressing meaningful member names, then it is counterproductive.

Null pointer constants

It is valid and portable to use 0 as a null pointer constant, but you shouldn't. Use the macro NULL when a null pointer constant is what you mean. This is much clearer.

Compound statements (blocks)

As a matter of good code style, use compound statements (brace-enclosed code blocks) in conjunction with conditional and looping statements, even when they contain only one simple statement. Especially do not use the comma operator to squeeze what would ordinarily be two separate expression statements into a single one in order to avoid braces.

For example, this ...

    if(argc == 2) root = argv[1];
    else if(argc == 3) options = argv[1],
                        root = argv[2];
    else usage();

should be

    if (argc == 2) {
        root = argv[1];
    } else if (argc == 3) {
        options = argv[1];
        root = argv[2];
    } else {
        usage();
    }

Option processing

It is conventional to recognize options by the leading '-' character, but your program recognizes them only positionally. Moreover, it requires all the options to be presented in the same argument, which is not normal.

Variadic functions

You appear to have implemented variadic function dirlst() correctly (congratulations!), and I even think I understand why you chose that implementation approach. Too bad I hate what you did with it, but don't take that personally.

The target directory tree structure would be much better expressed as data instead of as code. You do get half a pass on this, though, for you need C99 compound literals for a clean way to express the structure as data, and those are not available because of your choice to limit yourself to C89. That doesn't mean you couldn't have structured it as data, though, only that the data would been messier.

Or reading the directory structure from a file would have been an option, too. That would make your program easier to customize. In fact, it would not be too big a step from there to it being a more general-purpose directory-tree-creation tool.

Miscellaneous

  • the correct spelling is "separator" (not "seperator")
  • function cgetc() would be more efficiently written in terms of the fgets() function.
  • the return value of function vout() is not actually computed by the function, and it is never used anyway. This function probably should not return anything.
  • for clarity and efficiency, function strsum() should be written in terms of strcpy(). Or maybe strcat(). Or even sprintf().
  • for clarity, function djoin() should probably be written in terms of one call to sprintf() instead of two to strsum().
\$\endgroup\$
3
  • \$\begingroup\$ Thank you very much for your long answer! I agree on most of them and see what I can improve. I really need to remove or at least rethink all of the mallocs, and some unnecessary can be gotten rid of. The idea of putting it onto a file seems interesting, but I dont want my program to depend on some config files somewhere. I need to think of a solution for that. Thanks! \$\endgroup\$
    – Labricecat
    Commented Jan 10 at 11:31
  • 2
    \$\begingroup\$ @Labricecat, you could think of it as an input file, as opposed to a config file. The program coming packaged with one that builds out an FHS directory structure. Allowing that other input files might do different structures altogether is what would shift this to a general-purpose tool. Or not. \$\endgroup\$ Commented Jan 10 at 13:32
  • \$\begingroup\$ thats a good idea, i think I also got a minimalistic file format in mind. Thanks for the idea, even such a small program can always be improved, i love it. \$\endgroup\$
    – Labricecat
    Commented Jan 10 at 17:41
8
\$\begingroup\$

Check the return value of library functions:

In dirlst() and strsum(), the return value of malloc() goes unchecked. malloc() and family returns a null pointer to signify failure. Failing to check for it risks invoking undefined behavior by a subsequent null pointer dereference.

Consider:

/* Allocate to the referenced object, not the type. 
 * It is easier to maintain.
 */
ds = malloc(sizeof *ds * (vlz + 1));

if (!ds) {
    complain();
    /* Handle error here. */
}
\$\endgroup\$
3
  • \$\begingroup\$ Oh you're right, I missed that! I should aswell make the program less memory intense. \$\endgroup\$
    – Labricecat
    Commented Jan 9 at 9:07
  • 2
    \$\begingroup\$ Note that on Linux systems, the fact that malloc() returns a non-null value doesn't mean that the pointer can be used without crashing the program. The real semantics of malloc are not that it will return null in case of failure, but rather that implementations for platforms that can detect allocation failures and process failed requests in side-effect-free fashion should indicate failure that by returning null, while those for platforms that can't may be limited to the broken semantics of the underlying platform. \$\endgroup\$
    – supercat
    Commented Jan 9 at 23:31
  • \$\begingroup\$ Although it may be possible to detect whether over-allocation is enabled on these platforms, that would add a substantial amount of platform-specific code (still portable, but likely against the spirit of simplicity the program currently has). \$\endgroup\$ Commented Jan 11 at 8:22
4
\$\begingroup\$

General Observations

There are some really good answers here, I will only comment on what I think was not covered.

The reason I suggested that you add usr.c was that we really needed to see the definitions of the 2 functions there. The question was borderline off-topic before that.

Code Organization

Function prototypes are very useful in large programs that contain multiple source files, and that in case they will be in header files. In a single file program like this it is better to put the main() function at the bottom of the file and all the functions that get used in the proper order above main(). Keep in mind that every line of code written is another line of code where a bug can crawl into the code.

Avoid Global Variables

It is very difficult to read, write, debug and maintain programs that use global variables. Global variables can be modified by any function within the program and therefore require each function to be examined before making changes in the code. In C and C++ global variables impact the namespace and they can cause linking errors if they are defined in multiple files. The answers in this stackoverflow question provide a fuller explanation.

The option variables could be implemented in an option struct, rather than having them all as separate variables. The struct would be easier to pass between functions that all of those variables separately.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks! Personally I like having the main function at the start, as one who opens the file can easily see at first glance what this program even does. I recently figured out that you can declate extern variables inside of functions to make them able to use one storage together, this also helps with understanding what function uses what in the first place, but might get verbose. An option struct is generally a good idea, as I had a small program in mind at the start, i figured having everywhere an &option additionally might get verbose. I need to rethink that, thanks for the input! \$\endgroup\$
    – Labricecat
    Commented Jan 10 at 17:45
  • \$\begingroup\$ @Labricecat You could move the main function into a file called main.c where it is the only function and provide headers for all the other files. The point is not to write any more code than you need to. \$\endgroup\$
    – pacmaninbw
    Commented Jan 10 at 17:49
3
\$\begingroup\$

The functions in usr.c should be forward-declared in usr.h. Both .c files will then #include this file.

This will allow the compiler to catch if the definition doesn't match the forward-declaration.

#pragma once
#ifndef _USR_H_
#define _USR_H_

#define USR_SUCCESS 1 
#define USR_FAIL 0

extern int umkdir(const char*);
extern int utouch(const char*);

#endif /* _USR_H_ */

\$\endgroup\$
2
  • 6
    \$\begingroup\$ Either #pragma once or #ifndef _USR_H, you don't need both. If the user is using an old C compiler, #pragma once may not be implemented. #pragma once is not part of the standard and may not work with all compilers. \$\endgroup\$
    – pacmaninbw
    Commented Jan 10 at 0:11
  • \$\begingroup\$ That seems a bit unneeded, but I suppose I can do that. Or get rid of the macros, hm.. \$\endgroup\$
    – Labricecat
    Commented Jan 10 at 11:32
3
\$\begingroup\$

If you wanted an old-school feel, you succeeded. I was getting LISP flashbacks reading the tree() function :)

Scoping

Helper functions that are only used in the current file should be declared static. This doesn't matter so much in single-file programs, but once you have multiple source files it's helpful to be explicit about the intended scope of each function.

Global variables are typically frowned upon, but I find that they're usually fine for CLI flags in simple command-line programs like this. You're only writing them in one place, and the overhead required for a more robust solution can be significant compared to the size of the rest of the program.

Whitespace

On one hand, the indentation in your tree() function helps explain what it's doing, with the code reflecting the way the output will be nested. On the other hand, this will be really hard to keep consistent. Things like proportional vs. fixed-width fonts, or tabs vs. spaces will also impact how effective this is. In addition, some compilers report compile errors in multi-line statements using the line on which that statement started. That makes it hard to figure out exactly where the error is in mega-statements like this. Long-term, this will likely be more of a hindrance than a help.

Option Parsing

  • Consider using a standard/common option-parsing system like getopt_long(). That will simplify your code and help your program's syntax match the style that command-line programs typically use.
  • When an invalid option is given, your usage() function produces a message that does not explain what any of the options actually do. Even after reading it, I still don't know how to call your program correctly. You should display the same detailed output that you display in help().
  • Your usage() and help() functions terminate the program when called. This makes your code flow a bit hard to follow, especially when debugging from an error code. It would be better to have your function simply display the message, then have main() return if desired.
  • Usage information should be printed to stderr when the result of an error, but printed to stdout when the user gives the "show help" flag.
  • Some platforms are not case-sensitive, so it's risky to use flags that differ only by case (like 'a' and 'A').

strsum

  • You aren't modifying the inputs, so they should be type const char*.
  • The inputs are not validated, so NULL arguments will get blindly dereferenced.
  • The for loop portion is condensed to the point where it's difficult to see exactly what the function is actually doing. It would be clearer if you did something like this instead:
char* r = malloc(strlen(s1) + strlen(s2) + 1);
strcpy(r, s1);
strcat(s1, s2);
return r;
  • You call this function twice in a row, and the output of the first call is an input into the second call. If you made the function take three string arguments instead of two, you could get the same result with a single call and avoid the extra memory allocations.

dirlst

  • Passing the number of variadic inputs as the first argument is a maintenance nightmare. Passing the wrong number can result in out-of-bounds memory reads. It would be far safer to rework this to not use a variable argument list.
  • Passing in a negative number for vlz causes bad things to happen. You probably want an unsigned type here.
  • ds = malloc(sizeof(struct dir) ... is correct, but can be hard to keep in sync when code gets refactored and data types change. Using ds = malloc(sizeof(*ds) ... instead is a bit more maintenance-friendly.

Simplification

  • You can use toupper(c) == 'Y' (from ctype.h) instead of a completely separate acc() function.
  • cgetc() seems to be overly-complicated. In what use case is there more than one character in the buffer? It might make sense to purge the input buffer before displaying the prompt, but I don't see how a human could enter multiple values before you have a chance to process them.
  • The vout() function returns a value that is never checked. If the return value is important, make sure you always check it. If it's not important, don't return anything.
  • The overall structure of this program is to run down a list of objects to create, build a struct for each object, arrange those in a big tree, then walk that tree and create each item. That's a lot of overhead, both CPU time and memory. It feels like you could greatly simplify things if you had your newdir function create the directory itself and then eschew the entire tree structure. To make this work you'd likely need a version of mkdir() that could create directories recursively, but that should be fairly straightforward.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.