Parsing a configuration file in C

Question

I've written the following function to parse a configuration file. I did my best to make it simple, but it's still over 30 lines and has too much indentation caused by a series of if statements.

Any suggestions make it easier to read are welcome.

int
parse_config (FILE * conf_file, module_initializer_t init)
{
  char *line; /* Current line */
  char *type; /* Type of current module */
  char *name; /* Name of current module */
  char *envz;
  size_t envz_len;
  size_t lineno = 0;

  line = type = name = envz = NULL;
  while (getline (&line, NULL, conf_file) != EOF) 
    {
      lineno++ ;
      if (!is_comment(line))
        if (line[0] == '[')
          if (update_header(line, &type, &name)) {
            if (type)
              if ((*init)(type, name, envz, envz_len)) {
                fprintf("failed to initialize module %s with name %s", type, name);
                return 1;
              }
            clear_envz(&envz, &envz_len);
            }
          else {
            fprint("syntax error on line %d", lineno);
            return 1;
          }
      else
        argz_add(&envz, &envz_len, line);
    }
  if ((*init)(type, name, envz, envz_len)) {
    fprintf("failed to initialize module %s with name %s", type, name);
    return 1;
  }
}

It parses a configuration file with the following syntax:

   #comment and empty lines ignored
   [module-type]{module-name}
   var1=val1
   #comment
   var2=val2
   [another-type]{another-name}
   var3=val3

Answer 1

I would do some re-arranging:
Use the quick exit pattern.

This is basically if it fails the useful criteria exit (or in this case start the next loop). Basically you test for conditions that don't satisfy your working conditions. If the data does not match then you move on.

int
parse_config (FILE * conf_file, module_initializer_t init)
{
  char *line; /* Current line */
  char *type; /* Type of current module */
  char *name; /* Name of current module */
  char *envz;
  size_t envz_len;
  size_t lineno = 0;

  line = type = name = envz = NULL;
  while (getline (&line, NULL, conf_file) != EOF) 
  {
      lineno++ ;
      if (is_comment(line))
      {    continue;        // Ignore comments;
      }

      if (line[0] != '[')
      {
          argz_add(&envz, &envz_len, line);
          continue;         // Processes arguments and continue
      }

      if (!update_header(line, &type, &name))
      {
          fprint("syntax error on line %d", lineno);
          return 1;         // Not an argument and not a header
                            // must be an error.
      }

      // Processes a header
      if (type)
      {
          if ((*init)(type, name, envz, envz_len))
          {
            fprintf("failed to initialize module %s with name %s", type, name);
            return 1;
          }
          clear_envz(&envz, &envz_len);
      }     
  }

  if ((*init)(type, name, envz, envz_len)) {
    fprintf("failed to initialize module %s with name %s", type, name);
    return 1;
  }

  // You should also return a value here.
  return 0;
}

Answer 2

Rather than 'removing' ifs, it may be better to move them around!

But please, put {} for every if, even if it has only one statement (another if). It makes it a lot easier to read.

1. Treat the !is_comment as a precondition for the loop contents. It's right at the start, so it's obvious that you're doing something 'special' with it. Maybe even change the while to a for to push it further up.

   for ( lineno = 1; getline(&line, NULL, conf_file) != EOF; lineno++ ) 
   {
       /* skip over comments */
       if ( is_comment( line ) ) { continue; }
   

2. Put your "header processing" part into another function, since it performs a single task. It's a little awkward with the number of parameters even so, but you're removing three levels of indentation while reading it.

You'll be left with the following (if I've got it right); it's longer, but may be more readable.

#define PARSE_FAILURE 1
#define SUCCESS 0

int parse_module_header( module_initializer_t init, char* line, int lineno, char** penvz, size_t* penvz_len )
{
    char *type; /* Type of current module */
    char *name; /* Name of current module */

    type = name = NULL;
    if ( update_header( line, &type, &name ) ) {

        if ( type ) {

            if ( (*init)( type, name, *penvz, *penvz_len ) ) {

                fprintf("failed to initialize module %s with name %s", type, name);
                return PARSE_FAILURE;
            }
        }
        clear_envz( penvz, penvz_len );
    }
    else {

        fprint( "syntax error on line %d", lineno );
        return PARSE_FAILURE;
    }
    return SUCCESS;
}

int
parse_config (FILE * conf_file, module_initializer_t init)
{
    char *line; /* Current line */
    char *envz;
    size_t envz_len;
    size_t lineno = 0;

    line = envz = NULL;
    for ( lineno = 1; getline( &line, NULL, conf_file ) != EOF; lineno++ ) 
    {
        /* skip over comments */
        if ( is_comment( line ) ) { continue; }

        if (line[0] == '[') {

            /* begin a new module section */
            int err;
            err = parse_module_header( init, line, lineno, &envz, &envz_len );
            if ( err != SUCCESS ) {

                return err;
            }
        }
        else {

            /* set a config variable for the current module */
            argz_add( &envz, &envz_len, line );
        }
    }
    return SUCCESS;
}

Answer 3

You can pack together single line if sentences, but remember to put an empty line after the last one:

if (cond1) continue;
if (cond2) {printMessage(); continue;}

// Put empty line after last if block!
doRealWork();