Valid use of goto for error management in C?

FWIF, I find the error handling idiom you gave in the question's example to be more readable and easier to understand than any of the alternatives given in the answers so far. While goto is a bad idea in general, it can be useful for error handling when done in a simple and uniform manner. In this situation, even though it's a goto, it's being used in well-defined and more or less structured manner.

As a general rule, avoiding goto is a good idea, but the abuses that were prevalent when Dijkstra first wrote 'GOTO Considered Harmful' don't even cross most people's minds as an option these days.

What you outline is a generalizable solution to the error handling problem - it is fine with me as long as it is carefully used.

Your particular example can be simplified as follows (step 1):

int foo(int bar){    int return_value = 0;    if (!do_something(bar)) {        goto error_1;    }    if (!init_stuff(bar)) {        goto error_2;    }    if (prepare_stuff(bar))    {        return_value = do_the_thing(bar);        cleanup_3();    }error_2:    cleanup_2();error_1:    cleanup_1();    return return_value;}

Continuing the process:

int foo(int bar){    int return_value = 0;    if (do_something(bar))    {           if (init_stuff(bar))        {            if (prepare_stuff(bar))            {                return_value = do_the_thing(bar);                cleanup_3();            }            cleanup_2();        }        cleanup_1();    }    return return_value;}

This is, I believe, equivalent to the original code. This looks particularly clean since the original code was itself very clean and well organized. Often, the code fragments are not as tidy as that (though I'd accept an argument that they should be); for example, there is frequently more state to pass to the initialization (setup) routines than shown, and therefore more state to pass to the cleanup routines too.

I'm surprised nobody has suggested this alternative, so even though the question has been around a while I'll add it in: one good way of addressing this issue is to use variables to keep track of the current state. This is a technique that can be used whether or not goto is used for arriving at the cleanup code. Like any coding technique, it has pros and cons, and won't be suitable for every situation, but if you're choosing a style it's worth considering - especially if you want to avoid goto without ending up with deeply nested ifs.

The basic idea is that, for every cleanup action that might need to be taken, there is a variable from whose value we can tell whether the cleanup needs doing or not.

I'll show the goto version first, because it is closer to the code in the original question.

int foo(int bar){    int return_value = 0;    int something_done = 0;    int stuff_inited = 0;    int stuff_prepared = 0;    /*     * Prepare     */    if (do_something(bar)) {        something_done = 1;    } else {        goto cleanup;    }    if (init_stuff(bar)) {        stuff_inited = 1;    } else {        goto cleanup;    }    if (prepare_stuff(bar)) {        stufF_prepared = 1;    } else {        goto cleanup;    }    /*     * Do the thing     */    return_value = do_the_thing(bar);    /*     * Clean up     */cleanup:    if (stuff_prepared) {        unprepare_stuff();    }    if (stuff_inited) {        uninit_stuff();    }    if (something_done) {        undo_something();    }    return return_value;}

One advantage of this over some of the other techniques is that, if the order of the initialisation functions is changed, the correct cleanup will still happen - for instance, using the switch method described in another answer, if the order of initialisation changes, then the switch has to be very carefully edited to avoid trying to clean up something wasn't actually initialised in the first place.

Now, some might argue that this method adds a whole lot of extra variables - and indeed in this case that's true - but in practice often an existing variable already tracks, or can be made to track, the required state. For example, if the prepare_stuff() is actually a call to malloc(), or to open(), then the variable holding the returned pointer or file descriptor can be used - for example:

int fd = -1;....fd = open(...);if (fd == -1) {    goto cleanup;}...cleanup:if (fd != -1) {    close(fd);}

Now, if we additionally track the error status with a variable, we can avoid goto entirely, and still clean up correctly, without having indentation that gets deeper and deeper the more initialisation we need:

int foo(int bar){    int return_value = 0;    int something_done = 0;    int stuff_inited = 0;    int stuff_prepared = 0;    int oksofar = 1;    /*     * Prepare     */    if (oksofar) {  /* NB This "if" statement is optional (it always executes) but included for consistency */        if (do_something(bar)) {            something_done = 1;        } else {            oksofar = 0;        }    }    if (oksofar) {        if (init_stuff(bar)) {            stuff_inited = 1;        } else {            oksofar = 0;        }    }    if (oksofar) {        if (prepare_stuff(bar)) {            stuff_prepared = 1;        } else {            oksofar = 0;        }    }    /*     * Do the thing     */    if (oksofar) {        return_value = do_the_thing(bar);    }    /*     * Clean up     */    if (stuff_prepared) {        unprepare_stuff();    }    if (stuff_inited) {        uninit_stuff();    }    if (something_done) {        undo_something();    }    return return_value;}

Again, there are potential criticisms of this:

• Don't all those "if"s hurt performance? No - because in the success case, you have to do all of the checks anyway (otherwise you're not checking all the error cases); and in the failure case most compilers will optimise the sequence of failing if (oksofar) checks to a single jump to the cleanup code (GCC certainly does) - and in any case, the error case is usually less critical for performance.

• Isn't this adding yet another variable? In this case yes, but often the return_value variable can be used to play the role that oksofar is playing here. If you structure your functions to return errors in a consistent way, you can even avoid the second if in each case:

  int return_value = 0;if (!return_value) {    return_value = do_something(bar);}if (!return_value) {    return_value = init_stuff(bar);}if (!return_value) {    return_value = prepare_stuff(bar);}

  One of the advantages of coding like that is that the consistency means that any place where the original programmer has forgotten to check the return value sticks out like a sore thumb, making it much easier to find (that one class of) bugs.

So - this is (yet) one more style that can be used to solve this problem. Used correctly it allows for very clean, consistent code - and like any technique, in the wrong hands it can end up producing code that is long-winded and confusing :-)