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?

Answer 1

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.

Answer 2

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

Answer 3

Seems to me that cleanup_3 should do its cleanup, then call cleanup_2. Similarly, cleanup_2 should do it's cleanup, then call cleanup_1. It appears that anytime you do cleanup_[n], that cleanup_[n-1] is required, thus it should be the responsibility of the method (so that, for instance, cleanup_3 can never be called without calling cleanup_2 and possibly causing a leak.)

Given that approach, instead of gotos, you would simply call the cleanup routine, then return.

The goto approach isn't wrong or bad, though, it's just worth noting that it's not necessarily the "cleanest" approach (IMHO).

If you are looking for the optimal performance, then I suppose that the goto solution is best. I only expect it to be relevant, however, in a select few, performance critical, applications (e.g., device drivers, embedded devices, etc). Otherwise, it's a micro-optimization that has lower priority than code clarity.

Answer 4

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.

Answer 5

GOTO is useful. It's something your processor can do and this is why you should have access to it.

Sometimes you want to add a little something to your function and single goto let's you do that easily. It can save time..

Answer 6

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.

Answer 7

I personally am a follower of the "The Power of Ten - 10 Rules for Writing Safety Critical Code".

I will include a small snippet from that text that illustrates what I believe to be a good idea about goto.

---

Rule: Restrict all code to very simple control flow constructs – do not use goto statements, setjmp or longjmp constructs, and direct or indirect recursion.

Rationale: Simpler control flow translates into stronger capabilities for verification and often results in improved code clarity. The banishment of recursion is perhaps the biggest surprise here. Without recursion, though, we are guaranteed to have an acyclic function call graph, which can be exploited by code analyzers, and can directly help to prove that all executions that should be bounded are in fact bounded. (Note that this rule does not require that all functions have a single point of return – although this often also simplifies control flow. There are enough cases, though, where an early error return is the simpler solution.)

---

Banishing the use of goto seems bad but:

If the rules seem Draconian at first, bear in mind that they are meant to make it possible to check code where very literally your life may depend on its correctness: code that is used to control the airplane that you fly on, the nuclear power plant a few miles from where you live, or the spacecraft that carries astronauts into orbit. The rules act like the seat-belt in your car: initially they are perhaps a little uncomfortable, but after a while their use becomes second-nature and not using them becomes unimaginable.

Answer 8

I think that the question here is fallacious with respect to the given code.

Consider:

1. do_something(), init_stuff() and prepare_stuff() appear to know if they have failed, since they return either false or nil in that case.
2. Responsibility for setting up state appears to be the responsibility of those functions, since there is no state being set up directly in foo().

Therefore: do_something(), init_stuff() and prepare_stuff() should be doing their own cleanup. Having a separate cleanup_1() function that cleans up after do_something() breaks the philosophy of encapsulation. It's bad design.

If they did their own cleanup, then foo() becomes quite simple.

On the other hand. If foo() actually created its own state that needed to be torn down, then goto would be appropriate.