Clean ways to do multiple undos in C

Question

Someone will probably say something about exceptions... but in C, what are other ways to do the following cleanly/clearly and without repeating so much code?

if (Do1()) { printf("Failed 1"); return 1; }
if (Do2()) { Undo1(); printf("Failed 2"); return 2; }
if (Do3()) { Undo2(); Undo1(); printf("Failed 3"); return 3; }
if (Do4()) { Undo3(); Undo2(); Undo1(); printf("Failed 4"); return 4; }
if (Do5()) { Undo4(); Undo3(); Undo2(); Undo1(); printf("Failed 5"); return 5; }
Etc...

This might be one case for using gotos. Or maybe multiple inner functions...

Answer 1

Yes, it's quite common to use goto in such cases to avoid repeating yourself.

An example:

int hello() {
  int result;

  if (Do1()) { result = 1; goto err_one; }
  if (Do2()) { result = 2; goto err_two; }
  if (Do3()) { result = 3; goto err_three; }
  if (Do4()) { result = 4; goto err_four; }
  if (Do5()) { result = 5; goto err_five; }

  // Assuming you'd like to return 0 on success.
  return 0;

err_five:
  Undo4();
err_four:
  Undo3();
err_three:
  Undo2();
err_two:
  Undo1();
err_one:
  printf("Failed %i", result); 
  return result;
}

As always you probably also want to keep your functions small and batch together the operations in a meaningful manner to avoid a large "undo-code".

Answer 2

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 %d\n", errorCode);
    }
    return errorCode;
}

Answer 3

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

bool Do1(void) { printf("function %s\n", __func__); return true;}
bool Do2(void) { printf("function %s\n", __func__); return true;}
bool Do3(void) { printf("function %s\n", __func__); return false;}
bool Do4(void) { printf("function %s\n", __func__); return true;}
bool Do5(void) { printf("function %s\n", __func__); return true;}

void Undo1(void) { printf("function %s\n", __func__);}
void Undo2(void) { printf("function %s\n", __func__);}
void Undo3(void) { printf("function %s\n", __func__);}
void Undo4(void) { printf("function %s\n", __func__);}
void Undo5(void) { printf("function %s\n", __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 :)

Answer 4

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

Answer 5

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

Answer 6

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 %d\n",ret);
    break; 
}
return ret;

Answer 7

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 bar() saved references to p and p2, but we don't need
    // p3 anymore.
    free(p3);    

    return 0;

err3:    
    free(p2);
err2:
    free(p);
err1:
    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(p);
    }
    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.

Answer 8

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.

Answer 9

This question is already overburdened with answers, but I would ike to point out that some codebases actually have wrapper code to deal with -what basically are- exceptions, in clean ways. For example, MuPdf implemented some trickery using longjmp's that emulate exception handling. In my opinion, if it comes to that, they should just be using C++ already, but that's just me.

You could try to do such wrappers yourself. As an exercise, let's think about your requirements and try to come up with a (very) crude design that tries to satisfy them:

• We have a set of operations that need to be undone if subsequent operations fail;
• Multiple operations must be undone in the reverse order that they were done;
• The operation that failed must not be undone. It failed, after all;
• Operations that were never reached must also not be undone, since they were never done in the first place.
• Ideally, allow the programmer to be explicit: He knows what operations need to be undone, and when they do.

I've come up with some macros to solve this problem:

#include <stdio.h>

// Define some variables to keep track of when an error happened, and how many operations should be undone.
// Names are "mangled" by prefixing them with try_. You probably should come up with a better mangling scheme than this.
#define BEGIN_TRY int try_done = 0, try_error = 0, try_count = 0

// Here's how this works:
// - First, count the expression as an operation that may need to be undone;
// - If no error occured yet, do the operation;
// - If it succeeds, count it as a "done" operation;
// - If it fails, signal the error
#define TRY(expression) try_count++; if(!try_error && !(expression)) try_done++; else try_error = 1

// Here we take advantage of the fact that the operations behave like a queue.
// This means that, no matter what, operations need to be undone in the same 
// order everytime, and if an operation needs to be undone when there 
// are N operations, it also needs to be undone when there are N+1 operations.
// So, we don't really need to maintain the queue, if the programmer puts the operations in the correct order already. We just
// need to know how many operations to undo, and how much total operations are there (because we need to start at the end)
#define ON_ERROR(expression) do { if(try_error && try_done >= try_count--) {try_done--; (expression);} } while(0)

// To simplify the test, the "jobs" that we will try to do just pass or fail depending on the argument passed.
int job(int result) {return result;}
void undo(int i) {printf("Undone %d.\n", i);}

#define PASS 0
#define FAIL 1

// Let's test this
int main() {
    BEGIN_TRY;

    // try toying with the order (and quantity) of these.
    // just remember that for each "TRY" there must be one "ON_ERROR"!
    TRY(job(PASS));
    TRY(job(PASS));
    TRY(job(FAIL));
    TRY(job(PASS));

    // Because only the first two operations succeeded, we should only see the effects of undo(2) and undo(1).
    ON_ERROR(undo(4));
    ON_ERROR(undo(3));
    ON_ERROR(undo(2));
    ON_ERROR(undo(1));
}

See it live!

I'm no C expert, so there's probably some bugs in this (writing safe macros is hard), but my point is: If you think about your requirements in detail, all you will have to do is to come up with a solution which satisfies all of them. Another point that can be made is: Much like goto, many people see macros as evil. Don't be one of them: If a macro will make your code clearer, easier to read, then, by all means, use it.

Answer 10

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();

Answer 11

TL;DR:

I believe it should be written as:

int main (void)
{
  int result = do_func();
  printf("Failed %d\n", 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 %d\n", 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 %d\n", result);
}

Output:

Do1
Do2
Do3
Do4
Do5
Undo4
Undo3
Undo2
Undo1
Failed 5

Answer 12

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

Answer 13

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.

Answer 14

Let's try for something with zero curly braces:

int result;
result =                   Do1() ? 1 : 0;
result = result ? result : Do2() ? 2 : 0;
result = result ? result : Do3() ? 3 : 0;
result = result ? result : Do4() ? 4 : 0;
result = result ? result : Do5() ? 5 : 0;

result > 4 ? (Undo5(),0) : 0;
result > 3 ? Undo4() : 0;
result > 2 ? Undo3() : 0;
result > 1 ? Undo2() : 0;
result > 0 ? Undo1() : 0;

result ? printf("Failed %d\r\n", result) : 0;

Yes. 0; is a valid statement in C (and C++). In the case that some of the functions return something that is incompatible with this syntax (e.g. void perhaps) then the Undo5() style can be used.

Answer 15

A sane (no gotos, no nested or chained ifs) approach would be

int bar(void)
{
  int rc = 0;

  do
  { 
    if (do1())
    {
      rc = 1;
      break;        
    }

    if (do2())
    {
      rc = 2;
      break;        
    }

    ...

    if (do5())
    {
      rc = 5;
      break;        
    }
  } while (0);

  if (rc)
  {
    /* 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;
}