Best practices for non-local exit with cleanup in C?-CodePudding

What is considered best practice for aborting on errors in C?

In our code base we currently have a pattern using

#define CHECKERROR(code) if(code) { return code; }

but this leads to resources not being closed in code of the form

/* not actual code due to non-disclosure restrictions */
int somefunction() {
    handle_t res1, res2;
    int errorcode;

    res1 = getResource();
    res2 = getResource();

    errorcode = action1(res1, res2);
    CHECK(errorcode);
    errorcode = action2(res1, res2);
    CHECK(errorcode);

    freeResource(res1);
    freeResource(res2);
    return errorcode;
}

I came across the pattern

/* initialize resources */
do {
    /* ... */
    errorcode = action();
    if(errorcode) break;
    /* ... */
} while(0);
/* cleanup resources */
return errorcode;

before in articles, but couldn't find any source discussing it now.

What is a good practice, that would be considered idiomatic to C? Does the do { } while(0); pattern qualify? Is there an idiomatic way to make it more clear, that it is not intended to be a loop, but a block with non-local exit?

CodePudding user response：

What is considered best practice for aborting on errors in C?

What is a good practice, that would be considered idiomatic to C?

Really, nothing. There is no best-practice. Best is to tailor a specific solution to the specific case you are handling. For sure - concentrate on writing readable code.

Let's mention some documents. MISRA 2008 has the following. The rule is strict - single exit point. So you have to assign variables and jump to a single return statement

Rule 6–6–5 (Required) A function shall have a single point of exit at the end of the function.

Error handling is the only place where using goto is actually encouraged. Linux Kernel Coding style presents and encourages using goto to "keep all exit points close". The style is not enforced - not all kernel functions use this. See Linux kernel coding style # Centralized exiting of functions.

The kernel recommendation of goto was adopted by SEI-C: MEM12-C. Consider using a goto chain when leaving a function on error when using and releasing resources.

Does the do { } while(0); pattern qualify?

Sure, why not. If you do not allocate any more resources inside the do { .. here .. }while(0) block, you might as well write a separate function and then call return from it.

There are also expansions on the idea. Even implementations of exceptions in C using longjmp. I know of ThrowTheSwitch/CException.

Overall, error handling in C is not easy. Handling errors from multiple libraries becomes extremely hard and is an art of its own. See MBed OS error-handling, mbed_error.h, even a site that explains MBed OS error codes.

Strongly prefer single return point from your functions - as you found out, using your CHECK(errorcode); will leak resources. Multiple return places are confusing. Consider using gotos:

int somefunction() {
    int errorcode = 0;

    handle_t res1 = getResource();
    if (!res1) {
        errorcode = somethnig;
        goto res1_fail;
    }
    handle_t res2 = getResource();
    if (!res2) {
        errorcode = somethnig_else;
        goto res2_fail;
    }

    errorcode = action1(res1, res2);
    if (!errorcode) {
       goto actions_fail;
    }
    errorcode = action2(res1, res2);
    if (!errorcode) {
       goto actions_fail;
    }

actions_fail:
    freeResource(res2);
res2_fail:
    freeResource(res1);
res1_fail:
    return errorcode;
}

CodePudding user response：

First of all, mysterious macros such as your CHECKERROR which hide away flow control are widely considered very bad practice. Don't do that - creating secret macro languages that no other C programmer understands is a much more serious quality concern than code repetition. Code repetition isn't good but it shouldn't be solved by creating a much worse problem. Assume that the reader knows C well, but don't assume that they know or want to know your secret macro language local to this project.

In idiomatic C there are two acceptable ways to write this code. Either with explicit return or with the "on error goto" pattern à la BASIC. I would generally recommend the return version since it saves you from having that old tiresome "goto considered harmful" debate yet again. But goto to a clean-up at the end of the function is acceptable too, as long as you only jump downwards.

(Your do-while(0) with break is just a goto in disguise. It isn't better or worse.)

The single point of return from functions is also debated, especially in the context of MISRA-C (see this). Multiple returns from a function is however fine as long as it doesn't make the code harder to read. In practice this means that you should avoid return (or goto) from inside deeply nested loops or statements. Generally keep the "cyclomatic complexity" (the number of possible execution paths in a function) as low as possible.

In case you need to free up resources, I personally prefer return over goto. For return you need to make a wrapper function, which also serves the purpose of separating resource allocation from the algorithm. I would have rewritten your code like this:

typedef enum // use an actual enum not sloppy int
{
  OK, // keeping code 0 for no error is the most common practice
  ERR_THIS,
  ERR_THAT
} err_t;

static err_t the_actual_algorithm (handle_t res1, handle_t res2) // likely inlined
{
   err_t errorcode;

   errorcode = action1(res1, res2);
   if(errorcode != OK) { return errorcode; }

   errorcode = action2(res1, res2);
   if(errorcode != OK) { return errorcode; }

   return OK;
}

err_t somefunction (void)  // note void, not empty parenthesis which is obsolete style
{
    handle_t res1, res2;
    err_t errorcode;

    res1 = getResource();
    res2 = getResource();

    errorcode = the_actual_algorithm(res1, res2);
 
    freeResource(res1);
    freeResource(res2);
    return errorcode;
}