Valid use of goto for error management in C?

Question

This question is actually a result of an interesting discussion at programming.reddit.com a while ago. It basically boils down to the following code:

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 )) {
        goto error_3;
    }
    return_value = do_the_thing( bar );
error_3:
    cleanup_3();
error_2:
    cleanup_2();
error_1:
    cleanup_1();
    return return_value;
}

The usage of goto here appears to be the best way to go, resulting in the cleanest and most efficient code of all possibilities, or at least so it seems to me. Quoting Steve McConnell in Code Complete:

The goto is useful in a routine that allocates resources, performs operations on those resources, and then deallocates the resources. With a goto, you can clean up in one section of the code. The goto reduces the likelihood of your forgetting to deallocate the resources in each place you detect an error.

Another support for this approach comes from the Linux Device Drivers book, in this section.

What do you think? Is this case a valid use for goto in C? Would you prefer other methods, which produce more convoluted and/or less efficient code, but avoid goto?

Solution 1

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.

Solution 2

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.

Solution 3

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 :-)

Solution 4

The problem with the goto keyword is mostly misunderstood. It is not plain-evil. You just need to be aware of the extra control paths that you create with every goto. It becomes difficult to reason about your code and hence its validity.

FWIW, if you look up developer.apple.com tutorials, they take the goto approach to error handling.

We do not use gotos. A higher importance is laid on return values. Exception handling is done via setjmp/longjmp -- whatever little you can.

Solution 5

There's nothing morally wrong about the goto statement any more than there is something morally wrong with (void)* pointers.

It's all in how you use the tool. In the (trivial) case you presented, a case statement can achieve the same logic, albeit with more overhead. The real question is, "what's my speed requirement?"

goto is just plain fast, especially if you're careful to make sure that it compiles to a short jump. Perfect for applications where speed is a premium. For other applications, it probably makes sense to take the overhead hit with if/else + case for maintainability.

Remember: goto doesn't kill applications, developers kill applications.

UPDATE: Here's the case example

int foo(int bar) { 
     int return_value = 0 ; 
     int failure_value = 0 ;

     if (!do_something(bar)) { 
          failure_value = 1; 
      } else if (!init_stuff(bar)) { 
          failure_value = 2; 
      } else if (prepare_stuff(bar)) { 
          return_value = do_the_thing(bar); 
          cleanup_3(); 
      } 

      switch (failure_value) { 
          case 2: cleanup_2(); 
          case 1: cleanup_1(); 
          default: break ; 
      } 
} 

View more solutions

Comments

[karlphillip]

stackoverflow.com/questions/3339946/…

[Admin]

@Eli: Why don't you remove the tags and place the function(cleanup_3();) in the parenthesis of if?

[Eli Bendersky]

@Akito: what do you mean? Could you post your suggestion as an answer with a complete code sample?

[Admin]

@EliBendersky:Please see my answer.

[Manu343726]

One of the things I hated most from Visual Basic (VBS and VB6 included) was the on error goto error error handling system :)

[drakorg]

You are missing a key point in this example: access to the resources you are trying to cleanup. The most common case is having the resources defined in the same scope as where you are placing the gotos, making the use of goto even more necessary, since they wouldn't even be available at cleanup_x() scopes. The finalizer block (labels) having access to the scope where the resources are defined, is what actually makes it the one of the best solutions compared to anything else. Gotos in these cases are clean, fast, readable, and maintainable.

[drakorg]

... And for this reason cleanup is normally made right in place, and not delegated to a function.

[Iharob Al Asimi]

I am seriously surprised of the answers to this question. Anyone who thinks that goto is bad, should go and read the original articles that said so. When goto was bad, it was used in a way that made programs pretty confusing. It is no longer the case, so goto is no longer bad.

[Eli Bendersky]

Could you present the 'case' alternative? Also, I would argue that this is way different from void*, which is required for any serious data structure abstraction in C. I don't think there's anyone seriously opposing void*, and you won't find a single large code base without it.

[Eli Bendersky]

That won't cut - cleanups are specific to resources, which are allocated in this order only in this routine. In other places, they aren't related, so calling one from the other doesn't make sense.

[Eli Bendersky]

Yep, the nested solution is one of the viable alternatives. Unfortunately it becomes less viable as the nesting level deepens.

[Jonathan Leffler]

@eliben: Agreed - but the deeper nesting might be (probably is) an indication that you need to introduce more functions, or have the preparation steps do more, or otherwise refactor your code. I could argue that each of the prepare functions should do their setup, call the next in the chain, and do their own cleanup. It localizes that work - you might even save on the three cleanup functions. It also depends, in part, on whether any of the setup or cleanup functions are used (usable) in any other calling sequence.

[webmarc]

Re: void*, that's exactly my point, there's nothing morally wrong with either. Switch/case example below. int foo(int bar) { int return_value = 0 ; int failure_value = 0 ; if (!do_something(bar)) { failure_value = 1; } else if (!init_stuff(bar)) { failure_value = 2; } else if (prepare_stuff(bar)) { { return_value = do_the_thing(bar); cleanup_3(); } switch (failure_value) { case 2: cleanup_2(); break ; case 1: cleanup_1(); break ; default: break ; } }

[Eli Bendersky]

@webmarc, sorry but this is horrible! Yo've just wholly simulated goto with labels - inventing your own non-descriptive values for labels and implementing goto with switch/case. Failure_value = 1 is cleaner than "goto cleanup_something" ?

[webmarc]

I feel like you set me up here... your question is for an opinion, and what I would prefer. Yet when I offered my answer, it's horrible. :-( As for your complaint about the label names, they're just as descriptive as the rest of your example: cleanup_1, foo, bar. Why are you attacking label names when that's not even germane to the question?

[Eli Bendersky]

I had no intention of "setting up" and cause any sort of negative feelings, sorry for that! It just feels like your new approach is targeted solely at 'goto removal', without adding any more clarity. It's as if you've reimplemented what goto does, just using more code which is less clear. This IMHO is not a good thing to do - getting rid of the goto just for the sake of it.

[webmarc]

Encapsulating the cleanup logic in a switch block (or any other block for that matter) allows additional logic to be executed between it and the if block above. This is far more maintainable, IMO, while the goto method is far more efficient. Since there's no clear goal for this code (speed versus maintainability, or any other parameters), there's no right answer here.

[Adam Rosenfield]

Unfortunately this doesn't work with loops -- if an error occurs inside a loop, then a goto is much, much cleaner than the alternative of setting and checking flags and 'break' statements (which are just cleverly disguised gotos).

[Trevor Boyd Smith]

Yes but gotos ruin your static code analysis tools! Or maybe you aren't using static code analysis tools? Shame on you. Not using static code analysis tools is like driving without a seat belt!

[David Thornley]

You don't need access to every single thing your processor can do. Most of the time goto is more confusing than the alternative.

[strager]

@Smith, More like driving without a bullet-proof vest.

[Richard]

The introduction of additional functions would also address the loop issue; have the loop as the top-most item in the function and use a return statement to break out of it in case of error. Once everything is compiled, you'll end up with the same basic result, a branch when there's an error, but it's a bit more declarative and easier to reason about.

[Donal Fellows]

@Trevor: It's a poor static code analysis tool that is fazed by goto. Indeed, it is goto-heavy code that is likely to need analysis…

[Donal Fellows]

The problem with this is that the usual way of totally banishing the goto is to use some set of “clever” booleans in deeply nested ifs or loops. That really doesn't help. Maybe your tools will grok it better, but you won't and you're more important.

[Admin]

Can't he just remove those tags and put the functions directly into if?Won't it be more readable?

[supercat]

While I've certainly used setjmp/longjmp in some cases where it was called for, I'd consider it even "worse" than goto (which I also use, somewhat less reservedly, when it is called for). The only times I use setjmp/longjmp are when either (1) The target is going to shut everything down in a way which is won't be affected by its present state, or (2) The target is going to reinitialize everything controlled within the setjmp/longjmp-guarded block in a way which is independent of its present state.

[Admin]

Looks like you're late to the party, but I certainly like the answer!

[gbtimmon]

@StartupCrazy, I know this is years old, but for the sake of validity of posts on this site i will point out that no he can not. If he gets an error at goto error3 in his code he would run clean up 1 2 and 3, in your sugeested solution he would only run cleanup 3. He could nest things, but that would just be the arrow antipattern, something programmers should avoid.

[remmy]

@webmarc: Old answer, but his goto and your case example doesn't do the same thing. A goto will jump to the label and then just continue running from there. Your example runs the appropriate case branch and then runs whatever is after the case

[KingRadical]

I know I'm necromancing here, but I find this advice rather poor - the arrow anti-pattern should be avoided.

[KingRadical]

I'm a little surprised nobody has pointed out that flaggy code, while an alternative to the goto, is actually less clean for two reasons. 1) You now have an unnecessary variable to keep track of. 2) The resulting code is more difficult to follow and understand than the example using goto in the question.

[KingRadical]

macro abuse (as with CSteps) is far worse than judicious use of goto

[KingRadical]

flaggy code and the arrow anti-pattern (both showcased in your example) are both things that needlessly complicate code. There isn't any justification outside of "goto is evil" to use them.

[Fizz]

Linus would probably reject your code blogs.oracle.com/oswald/entry/is_goto_the_root_of

[psmears]

@user3588161: If he would, that's his prerogative - but I'm not sure, based on the article you linked to, that that's the case: note that in the style I'm describing, (1) the conditionals don't nest arbitrarily deeply, and (2) there are no additional "if" statements compared to what you need anyway (assuming you're going to check all the return codes).

[The Paramagnetic Croissant]

So much this instead of the horrible goto and even worse arrow-antipattern solution!

[Ron Maimon]

@DavidThornley: Yes, you do need access to every single thing your processor can do, otherwise, you are wasting your processor. Goto is the best instruction in programming.

[Dan Sheppard]

@Richard that would be my method of choice as well, to use returns in small functions to emulate exceptions and to prevent "code exits screen right" nesting but keep things clean (mainly because you get an opportunity to introduce a name, a test, a clearly-scoped, informative comment, a chance to reduce the set of values in-scope, etc). Sometimes, though, the result can be a mess, lots of things called "thing()" "try_do_thing()" "do_thing()" "real_do_thing()" etc. In that case, end-of-function goto resource unwinding can end up neater.