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.

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.

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

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..

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.

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.

Here's what I've preferred:

bool do_something(void **ptr1, void **ptr2)
{
    if (!ptr1 || !ptr2) {
        err("Missing arguments");
        return false;
    }
    bool ret = false;

    //Pointers must be initialized as NULL
    void *some_pointer = NULL, *another_pointer = NULL;

    if (allocate_some_stuff(&some_pointer) != STUFF_OK) {
        err("allocate_some_stuff step1 failed, abort");
        goto out;
    }
    if (allocate_some_stuff(&another_pointer) != STUFF_OK) {
        err("allocate_some_stuff step 2 failed, abort");
        goto out;
    }

    void *some_temporary_malloc = malloc(1000);

    //Do something with the data here
    info("do_something OK");

    ret = true;

    // Assign outputs only on success so we don't end up with
    // dangling pointers
    *ptr1 = some_pointer;
    *ptr2 = another_pointer;
out:
    if (!ret) {
        //We are returning an error, clean up everything
        //deallocate_some_stuff is a NO-OP if pointer is NULL
        deallocate_some_stuff(some_pointer);
        deallocate_some_stuff(another_pointer);
    }
    //this needs to be freed every time
    free(some_temporary_malloc);
    return ret;
}

Old discussion, however.... what about using "arrow anti pattern" and encapsulate later every nested level in a static inline function? The code look clean, it is optimal (when optimisations are enabled), and no goto is used. In short, divide and conquer. Below an example:

static inline int foo_2(int bar)
{
    int return_value = 0;
    if ( prepare_stuff( bar ) ) {
        return_value = do_the_thing( bar );
    }
    cleanup_3();
    return return_value;
}

static inline int foo_1(int bar)
{
    int return_value = 0;
    if ( init_stuff( bar ) ) {
        return_value = foo_2(bar);
    }
    cleanup_2();
    return return_value;
}

int foo(int bar)
{
    int return_value = 0;
    if (do_something(bar)) {
        return_value = foo_1(bar);
    }
    cleanup_1();
    return return_value;
}

In terms of space, we are creating three times the variable in the stack, which is not good, but this disappear when compiling with -O2 removing the variable from the stack and using a register in this simple example. What I got from the above block with gcc -S -O2 test.c was the below:

    .section    __TEXT,__text,regular,pure_instructions
    .macosx_version_min 10, 13
    .globl  _foo                    ## -- Begin function foo
    .p2align    4, 0x90
_foo:                                   ## @foo
    .cfi_startproc
## %bb.0:
    pushq   %rbp
    .cfi_def_cfa_offset 16
    .cfi_offset %rbp, -16
    movq    %rsp, %rbp
    .cfi_def_cfa_register %rbp
    pushq   %r14
    pushq   %rbx
    .cfi_offset %rbx, -32
    .cfi_offset %r14, -24
    movl    %edi, %ebx
    xorl    %r14d, %r14d
    xorl    %eax, %eax
    callq   _do_something
    testl   %eax, %eax
    je  LBB0_6
## %bb.1:
    xorl    %r14d, %r14d
    xorl    %eax, %eax
    movl    %ebx, %edi
    callq   _init_stuff
    testl   %eax, %eax
    je  LBB0_5
## %bb.2:
    xorl    %r14d, %r14d
    xorl    %eax, %eax
    movl    %ebx, %edi
    callq   _prepare_stuff
    testl   %eax, %eax
    je  LBB0_4
## %bb.3:
    xorl    %eax, %eax
    movl    %ebx, %edi
    callq   _do_the_thing
    movl    %eax, %r14d
LBB0_4:
    xorl    %eax, %eax
    callq   _cleanup_3
LBB0_5:
    xorl    %eax, %eax
    callq   _cleanup_2
LBB0_6:
    xorl    %eax, %eax
    callq   _cleanup_1
    movl    %r14d, %eax
    popq    %rbx
    popq    %r14
    popq    %rbp
    retq
    .cfi_endproc
                                        ## -- End function

.subsections_via_symbols

Yes, its valid and the best practice for exceptions in C. All error handling mechanism of any language just jumps from error to handle like goto to label. But consider placing the label after goto in execution flow and in the same scope.