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

• William Morris: Does it work? What is it supposed to do? Note that you have init called with envz and envz_len uninitialized, and the function falls off the end without a return value. Also, which if is the else argz_add supposed to belong to?
• KAction: Why uninitialized? NULL and 0 are valid. According standard and indentation.
• miracle173: Does fprintf not need a file stream as first argument? cplusplus.com/reference/clibrary/cstdio/fprintf
• KAction: Yes, you are right. But that's not the point.
• William Morris: Sorry, I didn't notice envz is set to NULL. GCC will warn you of the ambiguous 'else'. According to the indentation, argz_add() is called for comments.

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;
}

• KAction: Your code leaks memory. If i do not need to free memory, things would be much easier.
• Loki Astari: Not sure how my code differs from yours (apart from looking nicer) :-)
• KAction: Yeah. My bad. I forgot free too. =)

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;
}

• KAction: You too missed memory leak. But idea of moving lineno to for is awesome. Thanks! And, also, I do not like this braces.

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();

• svick: Why is the empty line so important?
• Loki Astari: Not loving single line if statements.