3
\$\begingroup\$

As part of a project where I will be rewriting the most common GNU coreutils for practice purposes, I wrote a simple command line option parser in C. The parser works by categorizing argv entries into either flag or argument by comparing them to a list of possible flags specified by the user and grouping them together in pairs where applicable. Currently, only the standard encoding is supported (i.e. ASCII in most moderncases, no UTF-8 or other extended charsets).

Nearly all of the work is done in a function named parse_cmd_args.

The Files:

options.h

#pragma once

#include <stddef.h>

typedef struct {
    const char* flag;
    unsigned char has_argument;
} option;


typedef struct {
    option opt;
    const char* argument;
} option_entry;

typedef struct {
    option_entry* opts;
    size_t opts_length;
} option_list;

typedef void (*callback_t)(const char*);

/*
 * Parses the command line arguments passed to the program into a list of
 * option flags and their corresponding values. 
 *
 * Command line arguments which require no parameter or to which no parameter
 * was passed have NULL for the value of option_entry.argument. Similarly,
 * command line parameters which do not follow a flag that requires an argument
 * have NULL for the value of option_entry.opt.flag.
 *
 * argc and argv are supposed to be passed as-is from main.
 *
 * warning_callback and error_callback are two function pointers of type
 * void (*)(const char*) which are invoked in case a recoverable error or an
 * unrecoverable error occurs, respectively. After error_callback is invoked,
 * the program will terminate.
 */
option_list parse_cmd_args(int argc, const char** argv,
        callback_t warning_callback, callback_t error_callback);

void delete_option_list(option_list list);

#ifdef DEBUG
void print_option_list(option_list l);
#endif

options.c

#include "options.h"

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

#ifdef DEBUG
#include <stdio.h>
#endif

option_list parse_cmd_args(int argc, const char** argv,
        const option* accepted_options, size_t accepted_options_length,
        callback_t warning_callback, callback_t error_callback) {
    const char* allocation_failure_msg = "Allocating memory failed. Aborting.";
    const option_entry null_entry = {.opt = {.flag = NULL, .has_argument = 0},
            .argument = 0};

    if (argc < 2) {
        option_list ret = {.opts = NULL, .opts_length = 0};
        return ret;
    }

    //argv[0] contains the program name
    const char** arg_start = argv + 1;
    size_t real_argc = argc - 1;

    size_t allocated_options = 8;
    option_list list = {.opts = malloc(allocated_options * sizeof *list.opts)};

    //calloc is not possible here: NULL can have some bits set depending on
    //architecture, so we do instead copy a null_entry with correct
    //NULL-pointers
    for (size_t i = 0; i < allocated_options; ++i) {
        list.opts[i] = null_entry;
    }

    if (!list.opts) {
        error_callback(allocation_failure_msg);
        exit(EXIT_FAILURE);
    }

    size_t current_option = 0;

    for (size_t i = 0; i < real_argc; ++i) {
        const char* current_string = arg_start[i];
        if (!current_string) {
            continue;
        }

        if (current_string[0] == '-') {
            for (size_t j = 0; j < accepted_options_length; ++j) {
                if (!strcmp(current_string, accepted_options[j].flag)) {
                    list.opts[current_option].opt = accepted_options[j];
                    if (!accepted_options[j].has_argument) {
                        current_option++;
                    }
                    break;
                }
            }
        } else {
            list.opts[current_option].argument = current_string;
            current_option++;
        }

        if (current_option >= allocated_options) {
            allocated_options *= 2;
            option_entry* tmp = realloc(list.opts,
                    allocated_options * sizeof *tmp);
            if (!tmp) {
                error_callback(allocation_failure_msg);
                exit(EXIT_FAILURE);
            }

            list.opts = tmp;
            for (size_t j = 0; j < allocated_options / 2; ++j) {
                list.opts[allocated_options / 2 + j] = null_entry;
            }
        }
    }

    list.opts_length = current_option;
    if (allocated_options != list.opts_length) {
        option_entry* tmp = realloc(list.opts, list.opts_length * sizeof *tmp);
        if (!tmp) {
            warning_callback("Unable to shrink list to fit.");
        } else {
            list.opts = tmp;
        }
    }

    return list;
}

void delete_option_list(option_list l) {
    free(l.opts);
}

#ifdef DEBUG
void print_option_list(option_list l) {
    printf("%c", '{');
    for (size_t i = 0; i < l.opts_length; ++i) {
        const char* flag_str = "NULL";
        const char* argument_str = "NULL";
        if (l.opts[i].opt.flag) {
            flag_str = l.opts[i].opt.flag;
        }
        if (l.opts[i].argument) {
            argument_str = l.opts[i].argument;
        }

        printf("{\"%s\", \"%s\"}", flag_str, argument_str);

        if (i < l.opts_length - 1){
            printf("%s", ", ");
        }
    }
    printf("%c\n", '}');
}
#endif

Here is also a minimal test file to demonstrate the use of parse_cmd_args:

test.c

#include "options.h"

#include <stdio.h>

void print_w(const char* msg) {
    puts(msg);
}

int main(int argc, const char** argv) {
    option options[5] = {
        {.flag = "-v", .has_argument = 0},
        {.flag = "-c", .has_argument = 1},
        {.flag = "--print", .has_argument = 0}, 
        {.flag = "--file", .has_argument = 1},
        {.flag = "-q", .has_argument = 0}};
    option_list l = parse_cmd_args(argc, argv, options, 5, print_w, print_w);
    print_option_list(l);
    delete_option_list(l);
}

My C is quite rusty, so I naturally have a few questions:

  1. Is it good practice to have a debug function that is dependent on preprocessor flags defined in the normal header and source file, or should such functions only be defined and implemented in test files?

  2. Are there better alternatives for error handling? I realize that passing two callbacks to a function is not the optimal pattern, but just logging to stderr cannot be the right way, and integrating an error code into the return value would mean adding another layer of structs which does not seem desirable either.

  3. In the same line of thought as 2., should a library-like function like this just be allowed to call exit when encountering an error that makes successful program continuation extremely difficult (such as an out-of-memory error), or should the task of terminating the program be left to the calling function? If yes, how should the information that such an error has occured be passed back?

Please feel free, of course, to also review my code generally without respect to the above questions.

\$\endgroup\$
5
\$\begingroup\$

The first thing is that most of the relevant code is squeezed into a single large method parse_cmd_args. That is code granularity is definitely insufficient. The reasonable solution would be to extract methods that form fancy inner loops.

"printf debugging" as error handling mechanism is no good at all. It may provide some kind of human-readable description of a problem but that part of error handling should be optional as one can not expect human to perform error handling all the time. Error handling should be suitable for the calling code. Particular error handling method can be selected based on error kind:

  • if it is an incorrect API usage (for example argc is negative) you can place an assert; assertions can be also used to check some invariants in inner function code, for example that string indexes aren't going out of bounds;
  • if it is a some kind of runtime problem that can not be resolved inside of current method (for example malloc failure) then most likely you may want to interrupt it and inform caller that method failed and delegate error handling to caller; typically in C return codes are used; you may want to mark your method with _Check_return_ SAL annotation or __attribute__ ((warn_unused_result)) gcc attribute to ensure that caller checks it; if you want to use callback to report errors then you should definitely allow passing some kind of opaque user-defined parameter there from caller;
  • if it is some kind of critical problem with which caller won't be able to deal with or which indicates an inconsistent program state (for example close call to close file descriptor that is not -1 fails with EBADF) then it may be ok to call exit; this is rare case;

Functionality of this parser could be improved by accepting both "long" and "shorter" option variants. And not requiring prepending them with - e.g.

{.flag = "print", .sflag = "p", .has_argument = 0}
\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer. Would you be so kind to explain what the difference between a critical problem and a runtime problem is? As I see it, failing to malloc is more of a critical failure than failing to close a file descriptor since it implies some kind of critical system state (such as out-of-memory) which is very likely unrecoverable. \$\endgroup\$ – Ben Steffan Oct 2 '17 at 9:49
  • \$\begingroup\$ @BenSteffan As I wrote above critical problem is a runtime problem that with which caller won't be able to deal with or which indicates an inconsistent program state. Dealing with malloc failure and other low-resource situations is definitely possible and 24/7 server-side applications are required to do so. But deciding how to deal with it is outside of this library responsibilities so it would be better to delegate it to caller. If caller is a small util then it may just exit, if it is a tough server application it may try to free some memory by stopping low priority / old tasks. \$\endgroup\$ – user7860670 Oct 2 '17 at 10:48
  • 1
    \$\begingroup\$ @BenSteffan The close failure indicates that we screwed up resource management as a result of some logic flaw or memory corruption or race condition or something else. That is our application is in inconsistent state and allowing it to run will only yield unpredictable results. So there is nothing left to do but to terminate it and to rely on some external error-reporting mechanism to report this failure to developers (e.g. produce core dump). \$\endgroup\$ – user7860670 Oct 2 '17 at 10:52
  • \$\begingroup\$ It's possible that you might want to compile the code before answering. \$\endgroup\$ – pacmaninbw Oct 3 '17 at 15:38
1
\$\begingroup\$

Generally I would expect a command line parser to set variables that are used by the program to configure the way it works.

If the command line parser is used by programs such as ls, cp and other programs that are primarily run from the command line I would expect them to report errors to stderr. Graphic tools might report errors to the console, they would not report to stderr.

The use of exit() should be avoided in most cases, it will prevent a program from cleaning up after itself. When an error occurs it might be better to use setjmp() in main() and longjmp() where the error occurs.

More General Declarations
The code

option options[5] = {
    {.flag = "-v", .has_argument = 0},
    {.flag = "-c", .has_argument = 1},
    {.flag = "--print", .has_argument = 0},
    {.flag = "--file", .has_argument = 1},
    {.flag = "-q", .has_argument = 0}};

in main would be easier to extend if it didn't have the numeric constant 5 in it:

option options[] = {
    {.flag = "-v", .has_argument = 0},
    {.flag = "-c", .has_argument = 1},
    {.flag = "--print", .has_argument = 0},
    {.flag = "--file", .has_argument = 1},
    {.flag = "-q", .has_argument = 0}
};

The second brace on the last line of the original is problematic, it is not immediately clear when reading the code.

Keep it Simple
The function parse_cmd_args() contains the code

    if (argc < 2) {
        option_list ret = {.opts = NULL, .opts_length = 0};
        return ret;
    }

It might be better of the number of arguments was checked prior to calling parse_cmd_args() so that parse_cmd_args() could return NULL if there was an error. That way the call back would not be necessary.

The function also contains the lines:

    const char** arg_start = argv + 1;
    size_t real_argc = argc - 1;

Since the way argc and argv are passed into the function and used don't really allow argc and argv to be modified they can be treated as local variables.

    argv++;
    --argc;

Reduce Complexity, Follow SRP
@VTT is correct about the function parse_cmd_args() it should be broken up into multiple sub functions, it would have been easier to read, write, debug and maintain that way.

The Single Responsibility Principle states that every module or class should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by the class. All its services should be narrowly aligned with that responsibility.

Robert C. Martin expresses the principle as follows:
    `A class should have only one reason to change.`

While this is primarily targeted at classes in object oriented languages it applies to functions and subroutines in procedural languages like C as well.

Don't Ignore Compiler Warning and Error Messages

It is unclear whether the code was ever actually compiled and tested, and it is clear that it was never compiled without DEBUG defined. I have compiled this using Visual Studio 2013 on Windows 10 Pro and XCode running on El Capitan. In both cases it doesn't compile because the function declaration in the header file is wrong.

option_list parse_cmd_args(int argc, const char** argv,
     callback_t warning_callback, callback_t error_callback);

Should be

option_list parse_cmd_args(int argc, const char** argv,
    const option* accepted_options, size_t accepted_options_length,
    callback_t warning_callback, callback_t error_callback);

This could be a cut and paste error while creating the question.

The program will not link of DEBUG is not defined. The code either needs

#ifdef DEBUG 
    print_option_list(l);
#endif

or the the body of the function print_option_list() should be nested in the ifdef rather than the entire function

void print_option_list(option_list l) {
#ifdef DEBUG
    printf("%c", '{');
    for (size_t i = 0; i < l.opts_length; ++i) {
        const char* flag_str = "NULL";
        const char* argument_str = "NULL";
        if (l.opts[i].opt.flag) {
            flag_str = l.opts[i].opt.flag;
        }
        if (l.opts[i].argument) {
            argument_str = l.opts[i].argument;
        }

        printf("{\"%s\", \"%s\"}", flag_str, argument_str);

        if (i < l.opts_length - 1){
            printf("%s", ", ");
        }
    }
    printf("%c\n", '}');
#endif
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer. The faulty function declaration in the header does indeed seem to be a copy-and-paste error, as the definitions do match in my local files. I am aware of the fact that the code will not link if DEBUG is not defined, and I should have added the required #ifdef in test.c, but since test.c is just a very small example application, I simply deemed it unnecessary. About Keep it simple, I don't think I understand how checking the number of arguments beforehand would make the error callback superfluous. Would you be so kind to elaborate? \$\endgroup\$ – Ben Steffan Oct 3 '17 at 15:59
  • \$\begingroup\$ There is no reason to call the function if there are no command line arguments. The combination of reporting errors to stderr and returning NULL from the function if there is an error should provide enough error reporting capability. Here is my problem with callbacks, it is very difficult to add addition error messages as the code is maintained, each new kind of error will require a different callback. There is no checking whether a a flag should have an argument or not. The function creates a need for a second function to determine the validity of each flag. \$\endgroup\$ – pacmaninbw Oct 3 '17 at 16:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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