5
\$\begingroup\$

I'm starting a new, small C project and would like to have the safest, cleanest and shortest error handling code possible.

Here is a comparison of different ways to handle errors in the same C function. Which one should I use and why ?

The function, called db_create, creates a new ImageDatabase instance.

Table Of Content

  • Snippet 1 uses nested if
  • Snippet 2 uses nested else
  • Snippet 3 uses goto
  • Snippet 4 improves on snippet 3 with two macros

Snippet 1

This version (30 lines of code) uses nested if to achieve error handling without duplication. The code uses the following structure multiple times:

if (allocate resource and check if success) { // push on the (imaginary) stack
  <further code using the resource>
  free resource // pop the (imaginary) stack
} else {
  signal error // unable to push further
}
  • Upside: the nesting provides a sort of stack unwinding that ensures correct resources releasing
  • Downside: [locality cost] sometimes the else is far from the condition and this makes code hard to track down, see also arrow anti-pattern.

ImageDatabase*
db_create_if(char const* filename,
             uint32_t max_images,
             struct resolution resolutions[NB_RESOLUTIONS - 1])
{
    ImageDatabase* db = allocate_empty_db();
    if (db) {
        memcpy(db->resolutions, resolutions, (NB_RESOLUTIONS - 1) * sizeof(struct resolution));
        db->max_nb_images = max_images;
        if ((db->file = fopen(filename, "r+b"))) {
            if ((db->metadatas = calloc(max_images, sizeof(struct metadata)))) {
                if (fwrite(db, 1, sizeof(ImageDatabase), db->file) == 1) {
                    return db;
                    //
                    //error handling below:
                } else {
                    fprintf(stderr, "unable to write newly created database to %s", filename);
                }
                free(db->metadatas);
            } else {
                fprintf(stderr, "unable to allocate metadatas");
            }
            fclose(db->file);
        } else {
            fprintf(stderr, "unable to open %s", filename);
        }
        free(db);
    } else {
        fprintf(stderr, "unable to allocate database");
    }
    return NULL;
}

Snippet 2

This version (30 lines of code) uses nested else to achieve error handling without duplication. The code uses the following structure multiple times:

 if (allocate resource and check if error) {
     signal error
 } else {
     <further code using the resource>
     free resource
 }
  • Upsides: the else clause is short so there is no problem with code locality the nesting provides a sort of stack unwinding that ensures correct resources releasing
  • Downsides: error handling code mixed with logic, lots of nesting, hard to read

ImageDatabase*
db_create_else(char const* filename,
           uint32_t max_images,
           struct resolution resolutions[NB_RESOLUTIONS - 1])
{
    ImageDatabase* db = allocate_empty_db();
    if (db == NULL) {
        fprintf(stderr, "unable to allocate database");
    } else {
        memcpy(db->resolutions, resolutions, (NB_RESOLUTIONS - 1) * sizeof(struct resolution));
        db->max_nb_images = max_images;
        if ((db->file = fopen(filename, "r+b")) == NULL) {
            fprintf(stderr, "unable to open %s", filename);
        } else {
            if ((db->metadatas = calloc(max_images, sizeof(struct metadata))) == NULL) {
                fprintf(stderr, "unable to allocate metadatas");
            } else {
                if (fwrite(db, 1, sizeof(ImageDatabase), db->file) != 1) {
                    fprintf(stderr, "unable to write newly created database to %s", filename);
                } else {
                    return db;
                    //cleanup code below :
                }
                free(db->metadatas);
            }
            fclose(db->file);
        }
        free(db);
    }
    return NULL;
}

Snippet 3

This version (34 lines of code) uses goto to achieve error handling without duplication and without deep nesting. The code has the following structure:

if (allocate resource and check for error) { // push on the (imaginary) stack
     <signal error>
     goto pop_label;
 }
 <further code using the resource>

 pop_label: <cleanup code> // pop the (imaginary) stack
  • Upsides: the stack unwinding pattern with goto is common in C and ensures correct resources releasing, link1, link2.
  • Downside: some error handling code mixed with logic

ImageDatabase*
db_create_goto(char const* filename,
           uint32_t max_images,
           struct resolution resolutions[NB_RESOLUTIONS - 1])
{
    ImageDatabase* db = allocate_empty_db();
    if (db == NULL) {
        fprintf(stderr, "unable to allocate database");
        goto db_fail;
    }
    memcpy(db->resolutions, resolutions, (NB_RESOLUTIONS - 1) * sizeof(struct resolution));
    db->max_nb_images = max_images;
    if ((db->file = fopen(filename, "r+b")) == NULL) {
        fprintf(stderr, "unable to open %s", filename);
        goto file_fail;
    }
    if ((db->metadatas = calloc(max_images, sizeof(struct metadata))) == NULL) {
        fprintf(stderr, "unable to allocate metadatas");
        goto metadata_fail;
    }
    if (fwrite(db, 1, sizeof(ImageDatabase), db->file) != 1) {
        fprintf(stderr, "unable to write newly created database to %s", filename);
        goto write_fail;
    }
    return db;

write_fail:
      free(db->metadatas);
metadata_fail:
      fclose(db->file);
file_fail:
      free(db);
db_fail:
      return NULL;
}

Snippet 4

This version (22 lines of code) uses goto to achieve error handling without duplication and without deep nesting and two macros to shorten the code and to avoid mixing the logic with the error handling. The code has the following structure:

try( allocate resource and check for error, label_for_error); // push on the (imaginary) stack
 <further code using the resource>

 LABEL(label_for_error, code_to_execute_once_if_try_failed); // pop the (imaginary) stack
  • Upside: the shortest version and doesn't mix error signaling/handling with logic
  • Downside: uses two (short) macros
  • Note: the team is very small (max 2/3 persons, probably less) and, if used, these macros will appear in almost every function so I expect that we will get used to it very quickly.

The macros:

#define try( stmt, label )           \
    do{                              \
        if(!(stmt)){ goto label; } } \
    while(false)                    

#define LABEL( label, execute_once_code )        \
    do{                                          \
        if(false){ label: execute_once_code; } } \
    while(false)

The code:

ImageDatabase*
db_create_try(char const* filename,
           uint32_t max_images,
           struct resolution resolutions[NB_RESOLUTIONS - 1])
{
    //logic is not mixed with error handling:

    ImageDatabase* db;
    try( db = allocate_empty_db(), db_fail);
    memcpy(db->resolutions, resolutions, (NB_RESOLUTIONS - 1) * sizeof(struct resolution));
    db->max_nb_images = max_images;
    try( db->file = fopen(filename, "r+b"), file_fail);
    try( db->metadatas = calloc(max_images, sizeof(struct metadata)), metadata_fail);
    if (fwrite(db, 1, sizeof(ImageDatabase), db->file) != 1) { goto fwrite_fail; }
    return db;

    //error handling:
    LABEL( fwrite_fail, fprintf(stderr, "unable to write newly created database to %s", filename));
    free(db->metadatas);

    LABEL( metadata_fail, fprintf(stderr, "unable to allocate metadatas"));
    fclose(db->file);

    LABEL( file_fail, fprintf(stderr, "unable to open %s", filename));
    free(db);

    LABEL( db_fail, fprintf(stderr, "unable to allocate database"));
    return NULL;
}
\$\endgroup\$
1
\$\begingroup\$

I spent a lot of time wondering about this sort of issue over the years and I don't think I ever came to a firm conclusion as to the best approach. I'm never very happy with using macros though as I have seen them badly misused. And goto, though neat, raises objections, in particular when there is a company coding standard in place.

If in doubt, I usually consider whether a function can help. It often can. For example your cleanup code can easily be db_free(db) in which the function checks each resource in db and frees it if it exists.

void db_free(ImageDatabase *db) 
{
    if (db) {
        if (db->file) {
            fclose(db->file);
        }
        if (db->metadatas) {
            free(db->metadatas);
        }
        free(db);
    }
}

Similarly, opening and writing the file:

int db_save(ImageDatabase *db, char const* filename)
{
    if (!db->file) {
        if ((db->file = fopen(filename, "r+b")) == NULL) {
            return 1;
        }
    }
    if (fwrite(db, 1, sizeof(ImageDatabase), db->file) != 1) {
        return 1;
    }
    return 0;
}

By splitting the logic in this way, the original function becomes much simpler:

ImageDatabase*
db_create_else(char const* filename,
               uint32_t max_images,
               struct resolution resolutions[NB_RESOLUTIONS - 1])
{
    ImageDatabase* db = allocate_empty_db();
    if (db) {
        memcpy(db->resolutions, resolutions, (NB_RESOLUTIONS - 1) * sizeof(struct resolution));
        db->max_nb_images = max_images;
        db->metadatas = calloc(max_images, sizeof(struct metadata));
        if (db->metadatas && db_save(db, filename) == 0) {
            return db;
        }
    }
    perror("db_create_else");
    db_free(db);
    return NULL;
}

This isn't a generalized solution of course, but as a piecemeal approach I think it helps. Note that if you are going to print error messages from system or library calls, use perror (or at least reference errno and strerror) to tell the user why something failed, not just what failed.

\$\endgroup\$
1
\$\begingroup\$

chux's 5th version is basically snippet 3, but without the gotos. I'd also choose snippet 3, keeping the indentation at a basic level and retaining readability. Snippet 1 and 2 are Pascal-style and more like academical code, but not very practical in my opinion. Snippet 4 could look more handy, but it's extra load to understand and keep track of. For example, your proposal for it isn't intuitively followable.

As for snippet 3, you could also do it like this:

ImageDatabase*
db_create_goto(char const* filename,
           uint32_t max_images,
           struct resolution resolutions[NB_RESOLUTIONS - 1])
{
    ImageDatabase* db = allocate_empty_db();
    if (db == NULL) {
        fprintf(stderr, "unable to allocate database");
        goto fail;
    }

    db->file = NULL;
    db->metadatas = NULL;

    memcpy(db->resolutions, resolutions, (NB_RESOLUTIONS - 1) * sizeof(struct resolution));
    db->max_nb_images = max_images;
    if ((db->file = fopen(filename, "r+b")) == NULL) {
        fprintf(stderr, "unable to open %s", filename);
        goto fail;
    }
    if ((db->metadatas = calloc(max_images, sizeof(struct metadata))) == NULL) {
        fprintf(stderr, "unable to allocate metadatas");
        goto fail_with_open_file;
    }
    if (fwrite(db, 1, sizeof(ImageDatabase), db->file) != 1) {
        fprintf(stderr, "unable to write newly created database to %s", filename);
        goto fail_with_open_file;
    }
    /* .... */
    return db;

fail_with_open_file:
      free(db->metadatas);
      fclose(db->file);
fail:
      free(db);
      return NULL;
}

Because free(NULL) is guaranteed to do no harm, you can take away some of the clutter. Or similiar, also a bit like William Morris's db_free but within the goto:

ImageDatabase*
db_create_goto(char const* filename,
           uint32_t max_images,
           struct resolution resolutions[NB_RESOLUTIONS - 1])
{
    ImageDatabase* db = allocate_empty_db();
    if (db == NULL) {
        fprintf(stderr, "unable to allocate database");
        goto fail;
    }

    db->file = NULL;
    db->metadatas = NULL;

    memcpy(db->resolutions, resolutions, (NB_RESOLUTIONS - 1) * sizeof(struct resolution));
    db->max_nb_images = max_images;
    if ((db->file = fopen(filename, "r+b")) == NULL) {
        fprintf(stderr, "unable to open %s", filename);
        goto fail;
    }
    if ((db->metadatas = calloc(max_images, sizeof(struct metadata))) == NULL) {
        fprintf(stderr, "unable to allocate metadatas");
        goto fail;
    }
    if (fwrite(db, 1, sizeof(ImageDatabase), db->file) != 1) {
        fprintf(stderr, "unable to write newly created database to %s", filename);
        goto fail;
    }
    /* .... */
    return db;

fail:
      if (db) {
          free(db->metadatas);
          if (db->file)
              fclose(db->file);
      }
      free(db);
      return NULL;
}
\$\endgroup\$
1
\$\begingroup\$

I'd like to offer a 5th: detect error and exit.

Much like #1,2 but instead, handle the error and return. No need for an else.

if (!(allocate resource and check if success)) {
  signal error // unable to push further
  return NULL;  
}
<further code using the resource>
... many lines of code
free resource // pop the (imaginary) stack
}

Rather than jump via a long if or else or goto, just get those abnormal conditions detected and dealt with. Notice by doing so, there is no need for an indentation and code can get down to business.

Outline:

return_type foo(arguments, ...) {
  if (abnormal conditions) {
    return failure;
  }
  do_task();
  ....
  return reasonable_Value;
}

Snippet 1:

Any if else separated by many lines of code deserves re-consideration.

Snippet 2:

There should be no need for the else - just return. (which is much like this #5 approach)

Snippet 3:

The goto is a beacon for some as an evil. It will attract a disproportionate number of dis-likes. Although I find it not so objectionable, I see no advantage for it here. Only use goto when it has a clear advantage.

Snippet 4:

A macro to simplify many lines of code is useful when it has wide application and is not hiding branching (code flow). As the macros provide neither, suggest avoiding them.

[Edit]

Like to offer a generalization in pseudo code. Consider

void foo1(arg1, arg2) {
  r1 = get_resource1(arg1, arg2);
  r2 = get_resource2(arg1, arg2, r1);
  r3 = get_resource3(arg1, arg2, r1, r2);

  if (is_acceptable(r1) && is_acceptable(r2) && is_acceptable(r3)) {
    do_work(arg1, arg2, r1,r2,r3);
  }

  release_resource(r3);
  release_resource(r2);
  release_resource(r1);
}

Vs.

void foo2(arg1, arg2) {
  r1 = get_resource1(arg1, arg2);
  if (is_acceptable(r1)) {

    r2 = get_resource2(arg1, arg2, r1);
    if (is_acceptable(r2)) {

      r3 = get_resource3(arg1, arg2, r1, r2);
      if (is_acceptable(r3)) {

        do_work(arg1, arg2, r1,r2,r3);
        release_resource(r3);
      }

      release_resource(r2);
    }

    release_resource(r1);
  }
}

With 1 or 2 levels of resource allocation, it makes little difference. Yet as the complexities and contingencies of resource gathering increases the 2nd method become a indentation heavy and IMO: unclear. Other approaches exists like layers of functions with early return.

End results: I find the 2nd method acceptable for 1 or 2 levels of dependency. I recommend the 1st method for more complex situations.

\$\endgroup\$
  • \$\begingroup\$ Thank for answering. I think that your solution leads to duplication of cleanup code in case of error, which is not an option. Please give an example of your solution by re-writing the snippet do_create. \$\endgroup\$ – Julien__ Jul 1 '16 at 9:15
  • 1
    \$\begingroup\$ The problem is that you have to cleanup the other resources before returning \$\endgroup\$ – Julien__ Jul 1 '16 at 11:16
  • \$\begingroup\$ I'm sorry, this definitely doesn't answer the question. You example in pseudo code looks nice but has wrong behavior since you can't distinguish which resource failed. \$\endgroup\$ – Julien__ Dec 3 '16 at 15:49

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.