Argthtjtr

This might be one case for using gotos.

Sure, let's try that. Here's a possible implementation:

#include "stdio.h"

int main(int argc, char **argv) {

    int errorCode = 0;

    if (Do1()) { errorCode = 1; goto undo_0; }

    if (Do2()) { errorCode = 2; goto undo_1; }

    if (Do3()) { errorCode = 3; goto undo_2; }

    if (Do4()) { errorCode = 4; goto undo_3; }

    if (Do5()) { errorCode = 5; goto undo_4; }



undo_5: Undo5();    /* deliberate fallthrough */

undo_4: Undo4();

undo_3: Undo3();

undo_2: Undo2();

undo_1: Undo1();

undo_0: /* nothing to undo in this case */



    if (errorCode != 0) {

        printf("Failed %dn", errorCode);

    }

    return errorCode;

}

If you have the same signature for your function you can do something like this:

bool Do1(void) { printf("function %sn", __func__); return true;}

bool Do2(void) { printf("function %sn", __func__); return true;}

bool Do3(void) { printf("function %sn", __func__); return false;}

bool Do4(void) { printf("function %sn", __func__); return true;}

bool Do5(void) { printf("function %sn", __func__); return true;}



void Undo1(void) { printf("function %sn", __func__);}

void Undo2(void) { printf("function %sn", __func__);}

void Undo3(void) { printf("function %sn", __func__);}

void Undo4(void) { printf("function %sn", __func__);}

void Undo5(void) { printf("function %sn", __func__);}





typedef struct action {

    bool (*Do)(void);

    void (*Undo)(void);

} action_s;





int main(void)

{

    action_s actions = {{Do1, Undo1},

                          {Do2, Undo2},

                          {Do3, Undo3},

                          {Do4, Undo4},

                          {Do5, Undo5},

                          {NULL, NULL}};



    for (size_t i = 0; actions[i].Do; ++i) {

        if (!actions[i].Do()) {

            printf("Failed %zu.n", i + 1);

            for (int j = i - 1; j >= 0; --j) {

                actions[j].Undo();

            }

            return (i);

        }

    }



    return (0);

}

You can change the return of one of Do functions to see how it react :)

For completeness a bit of obfuscation:

int foo(void)

{

  int rc;



  if (0

    || (rc = 1, do1()) 

    || (rc = 2, do2()) 

    || (rc = 3, do3()) 

    || (rc = 4, do4()) 

    || (rc = 5, do5())

    || (rc = 0)

  ) 

  {

    /* More or less stolen from Chris' answer: 

       https://stackoverflow.com/a/53444967/694576) */

    switch(rc - 1)

    {

      case 5: /* Not needed for this example, but left in in case we'd add do6() ... */

        undo5();



      case 4:

        undo4();



      case 3:

        undo3();



      case 2:

        undo2();



      case 1:

        undo1();



      default:

        break;

    }

  }



  return rc;

}

Use goto to manage cleanup in C.

For instance, check the Linux kernel coding style:

The rationale for using gotos is:

• unconditional statements are easier to understand and follow nesting is reduced


• errors by not updating individual exit points when making modifications are prevented


• saves the compiler work to optimize redundant code away ;)

Example:

int fun(int a)

{

    int result = 0;

    char *buffer;



    buffer = kmalloc(SIZE, GFP_KERNEL);

    if (!buffer)

        return -ENOMEM;



    if (condition1) {

        while (loop1) {

            ...

        }

        result = 1;

        goto out_free_buffer;

    }



    ...



out_free_buffer:

    kfree(buffer);

    return result;

}

In your particular case, it could look like:

int f(...)

{

    int ret;



    if (Do1()) {

        printf("Failed 1");

        ret = 1;

        goto undo1;

    }



    ...



    if (Do5()) {

        printf("Failed 5");

        ret = 5;

        goto undo5;

    }



    // all good, return here if you need to keep the resources

    // (or not, if you want them deallocated; in that case initialize `ret`)

    return 0;



undo5:

    Undo4();

...

undo1:

    return ret;

}

There are probably many ways to do this, but one idea is since you won't call one function unless the preceeding one succeeded, you could chain your function calls using else if like this. And using a variable to track where it fails you can use a switch statement to roll back easily too.

int ret=0;

if(Do1()) {

    ret=1;

} else if(Do2()) {

    ret=2;

} else if(Do3()) {

    ret=3;

} else if(Do4()) {

    ret=4;

} else if(Do5()) {

    ret=5;

}



switch(ret) {   

    case 5:  

        Undo4();

    case 4:  

        Undo3();

    case 3:  

        Undo2();

    case 2:  

        Undo1();

    case 1:

        printf("Failed %dn",ret);

    break; 

}

return ret;

Yes, as explained by other answers, using goto for error-handling is often appropriate in C.

That said, if possible, you probably should make your cleanup code safe to call even if the corresponding action was never performed. For example, instead of:

void foo()

{

    int result;

    int* p = malloc(...);

    if (p == NULL) { result = 1; goto err1; }



    int* p2 = malloc(...);

    if (p2 == NULL) { result = 2; goto err2; }



    int* p3 = malloc(...);

    if (p3 == NULL) { result = 3; goto err3; }



    // Do something with p, p2, and p3.

    bar(p, p2, p3);



    // Maybe we don't need p3 anymore.

    free(p3);    



    return 0;



err3:

    free(p3);

err2:

    free(p2);

err1:

    free(p1);

    return result;

}

I'd advocate:

void foo()

{

    int result = -1; // Or some generic error code for unknown errors.



    int* p = NULL;

    int* p2 = NULL;

    int* p3 = NULL;



    p = malloc(...);

    if (p == NULL) { result = 1; goto exit; }



    p2 = malloc(...);

    if (p2 == NULL) { result = 2; goto exit; }



    p3 = malloc(...);

    if (p3 == NULL) { result = 3; goto exit; }



    // Do something with p, p2, and p3.

    bar(p, p2, p3);



    // Set success *only* on the successful path.

    result = 0;



exit:

    // free(NULL) is a no-op, so this is safe even if p3 was never allocated.

    free(p3);



    if (result != 0)

    {

        free(p2);

        free(p1);

    }

    return result;

}

It's slightly less efficient since it requires initializing variables to NULL, but it's more maintainable since you don't need extra labels. There's less stuff to get wrong when making changes to the code. Also, if there's cleanup code that you need on both success and failure paths, you can avoid code duplication.

I typically approach this kind of problem by nesting the conditionals:

int rval = 1;

if (!Do1()) {

    if (!Do2()) {

        if (!Do3()) {

            if (!Do4()) {

                if (!Do5()) {

                    return 0;

                    // or "goto succeeded", or ...;

                } else {

                    printf("Failed 5");

                    rval = 5;

                }

                Undo4();

            } else {

                printf("Failed 4");

                rval = 4;

            }

            Undo3();

        } else {

            printf("Failed 3");

            rval = 3;

        }

        Undo2();

    } else {

        printf("Failed 2");

        rval = 2;

    }

    Undo1();

} else {

    printf("Failed 1");

    rval = 1;

}

return rval;

Usually, for me, the DoX() are some kind of resource acquisition, such as malloc(), and the UndoX() are corresponding resource releases that should be performed only in the event of failure. The nesting clearly shows the association between corresponding acquisitions and releases, and avoids the need for repetition of the code for undo operations. It's also very easy to write -- you don't need to create or maintain labels, and it's easy to put the resource release in the right place as soon as you write the acquisition.

This approach does sometimes produce deeply nested code. That doesn't bother me much, but you might consider it an issue.

Here is an answer that I have found resilient to bugs.

Yes. It uses goto. I firmly believe you should use what gives you most clarity, rather than just blindly following the advice of those before you (goto as a construct can make spaghetti code, but in this instance every other error handling method ususally ends up more spaghetti-like than using this method of goto, so IMO it's superior).

Some people may not like the form of this code, but I contest that when used to the style it is cleaner, easier to read (when everything's lined up, of course), and much more resilient to errors. If you have the properly linter/static analysis setup, and you're working with POSIX, it pretty much requires you to code in this fashion to allow for good error handling.

static char *readbuf(char *path)

{

    struct stat st;

    char *s = NULL;

    size_t size = 0;

    int fd = -1;



    if (!path) { return NULL; }



    if ((stat(path, &st)) < 0) { perror(path); goto _throw; }



    size = st.st_size;

    if (size == 0) { printf("%s is empty!n", path); goto _throw; }



    if (!(s = calloc(size, 1))) { perror("calloc"); goto _throw; }



    fd = open(path, O_RDONLY);

    if (fd < -1) { perror(path); goto _throw; }

    if ((read(fd, s, size)) < 0) { perror("read"); goto _throw; }

    close(fd); /* There's really no point checking close for errors */



    return s;



_throw:

    if (fd > 0) close(fd);

    if (s) free(s);

    return NULL;

}

If the functions return some kind of state pointer or handle (like most allocation & initialization functions would), you can quite cleanly do this without goto by giving initial values to variables. Then you can have a single deallocation function that can handle the case where only part of the resources has been allocated.

For example:

void *mymemoryblock = NULL;

FILE *myfile = NULL;

int mysocket = -1;



bool allocate_everything()

{

    mymemoryblock = malloc(1000);

    if (!mymemoryblock)

    {

        return false;

    }



    myfile = fopen("/file", "r");   

    if (!myfile)

    {

        return false;

    }



    mysocket = socket(AF_INET, SOCK_STREAM, 0);

    if (mysocket < 0)

    {

        return false;

    }



    return true;

}



void deallocate_everything()

{

    if (mysocket >= 0)

    {

        close(mysocket);

        mysocket = -1;

    }



    if (myfile)

    {

        fclose(myfile);

        myfile = NULL;

    }



    if (mymemoryblock)

    {

        free(mymemoryblock);

        mymemoryblock = NULL;

    }

}

And then just do:

if (allocate_everything())

{

    do_the_deed();

}

deallocate_everything();

TL;DR:

I believe it should be written as:

int main (void)

{

  int result = do_func();

  printf("Failed %dn", result);

}

Detailed explanation:

If nothing can be assumed what-so-ever about the function types, we can't easily use an array of function pointers, which would otherwise be the correct answer.

Assuming all function types are incompatible, then we would have to wrap in the original obscure design containing all those non-compatible functions, inside something else.

We should make something that is readable, maintainable, fast. We should avoid tight coupling, so that the undo behavior of "Do_x" doesn't depend on the undo behavior of "Do_y".

int main (void)

{

  int result = do_func();

  printf("Failed %dn", result);

}

Where do_func is the function doing all the calls required by the algorithm, and the printf is the UI output, separated from the algorithm logic.

do_func would be implemented like a wrapper function around the actual function calls, handling the outcome depending on the result:

(With gcc -O3, do_func is inlined in the caller, so there is no overhead for having 2 separate functions)

int do_it (void)

{

  if(Do1()) { return 1; };

  if(Do2()) { return 2; };

  if(Do3()) { return 3; };

  if(Do4()) { return 4; };

  if(Do5()) { return 5; };

  return 0;

}



int do_func (void)

{

  int result = do_it();

  if(result != 0)

  {

    undo[result-1]();

  }

  return result;

}

Here the specific behavior is controlled by the array undo, which is a wrapper around the various non-compatible functions. Which functions to to call, in which order, is all part of the specific behavior tied to each result code.

We need to tidy it all up, so that we can couple a certain behavior to a certain result code. Then when needed, we only change the code in one single place if the behavior should be changed during maintenance:

void Undo_stuff1 (void) { }

void Undo_stuff2 (void) { Undo1(); }

void Undo_stuff3 (void) { Undo2(); Undo1(); }

void Undo_stuff4 (void) { Undo3(); Undo2(); Undo1(); }

void Undo_stuff5 (void) { Undo4(); Undo3(); Undo2(); Undo1(); }



typedef void Undo_stuff_t (void);

static Undo_stuff_t* undo[5] = 

{ 

  Undo_stuff1, 

  Undo_stuff2, 

  Undo_stuff3, 

  Undo_stuff4, 

  Undo_stuff5, 

};

MCVE:

#include <stdbool.h>

#include <stdio.h>



// some nonsense functions:

bool Do1 (void) { puts(__func__); return false; }

bool Do2 (void) { puts(__func__); return false; }

bool Do3 (void) { puts(__func__); return false; }

bool Do4 (void) { puts(__func__); return false; }

bool Do5 (void) { puts(__func__); return true; }

void Undo1 (void) { puts(__func__); }

void Undo2 (void) { puts(__func__); }

void Undo3 (void) { puts(__func__); }

void Undo4 (void) { puts(__func__); }

void Undo5 (void) { puts(__func__); }



// wrappers specifying undo behavior:

void Undo_stuff1 (void) { }

void Undo_stuff2 (void) { Undo1(); }

void Undo_stuff3 (void) { Undo2(); Undo1(); }

void Undo_stuff4 (void) { Undo3(); Undo2(); Undo1(); }

void Undo_stuff5 (void) { Undo4(); Undo3(); Undo2(); Undo1(); }



typedef void Undo_stuff_t (void);

static Undo_stuff_t* undo[5] = 

{ 

  Undo_stuff1, 

  Undo_stuff2, 

  Undo_stuff3, 

  Undo_stuff4, 

  Undo_stuff5, 

};



int do_it (void)

{

  if(Do1()) { return 1; };

  if(Do2()) { return 2; };

  if(Do3()) { return 3; };

  if(Do4()) { return 4; };

  if(Do5()) { return 5; };

  return 0;

}



int do_func (void)

{

  int result = do_it();

  if(result != 0)

  {

    undo[result-1]();

  }

  return result;

}



int main (void)

{

  int result = do_func();

  printf("Failed %dn", result);

}

Output:

Do1

Do2

Do3

Do4

Do5

Undo4

Undo3

Undo2

Undo1

Failed 5

typedef void(*undoer)();

int undo( undoer*const* list ) {

  while(*list) {

    (*list)();

    ++list;

  }

}

void undo_push( undoer** list, undoer* undo ) {

  if (!undo) return;

  // swap

  undoer* tmp = *list;

  *list = undo;

  undo = tmp;

  undo_push( list+1, undo );

}

int func() {

  undoer undoers[6]={0};



  if (Do1()) { printf("Failed 1"); return 1; }

  undo_push( undoers, Undo1 );

  if (Do2()) { undo(undoers); printf("Failed 2"); return 2; }

  undo_push( undoers, Undo2 );

  if (Do3()) { undo(undoers); printf("Failed 3"); return 3; }

  undo_push( undoers, Undo3 );

  if (Do4()) { undo(undoers); printf("Failed 4"); return 4; }

  undo_push( undoers, Undo4 );

  if (Do5()) { undo(undoers); printf("Failed 5"); return 5; }

  return 6;

}

I made undo_push do the O(n) work. This is less efficient than having undo do the O(n) work, as we expect more push's than undos. But this version was a touch simpler.

A more industrial strength version would have head and tail pointers and even capacity.

The basic idea is to keep a queue of undo actions in a stack, then execute them if you need to clean up.

Everything is local here, so we don't pollute global state.

struct undoer {

  void(*action)(void*);

  void(*cleanup)(void*);

  void* state;

};



struct undoers {

  undoer* top;

  undoer buff[5];

};

void undo( undoers u ) {

  while (u.top != buff) 

  {

    (u.top->action)(u.top->state);

    if (u.top->cleanup)

      (u.top->cleanup)(u.top->state);

    --u.top;

  }

}

void pundo(void* pu) {

  undo( *(undoers*)pu );

  free(pu);

}

void cleanup_undoers(undoers u) {

  while (u.top != buff) 

  {

    if (u.top->cleanup)

      (u.top->cleanup)(u.top->state);

    --u.top;

  }

}

void pcleanup_undoers(void* pu) {

  cleanup_undoers(*(undoers*)pu);

  free(pu);

}

void push_undoer( undoers* to_here, undoer u ) {

  if (to_here->top != (to_here->buff+5))

  {

    to_here->top = u;

    ++(to_here->top)

    return;

  }

  undoers* chain = (undoers*)malloc( sizeof(undoers) );

  memcpy(chain, to_here, sizeof(undoers));

  memset(to_here, 0, sizeof(undoers));

  undoer chainer;

  chainer.action = pundo;

  chainer.cleanup = pcleanup_undoers;

  chainer.data = chain;

  push_undoer( to_here, chainer );

  push_undoer( to_here, u );

}

void paction( void* p ) {

  (void)(*a)() = ((void)(*)());

  a();

}

void push_undo( undoers* to_here, void(*action)() ) {

  undor u;

  u.action = paction;

  u.cleanup = 0;

  u.data = (void*)action;

  push_undoer(to_here, u);

}

then you get:

int func() {

  undoers u={0};



  if (Do1()) { printf("Failed 1"); return 1; }

  push_undo( &u, Undo1 );

  if (Do2()) { undo(u); printf("Failed 2"); return 2; }

  push_undo( &u, Undo2 );

  if (Do3()) { undo(u); printf("Failed 3"); return 3; }

  push_undo( &u, Undo3 );

  if (Do4()) { undo(u); printf("Failed 4"); return 4; }

  push_undo( &u, Undo4 );

  if (Do5()) { undo(u); printf("Failed 5"); return 5; }

  cleanup_undoers(u);

  return 6;

}

but that is getting ridiculous.