Clean error handling approach in C

Question

I'm writing a C program that needs good error handling. The code likes like this:

If(doWork("A")<0){
    return -1;   
}
If(doWork("B")<0){
    undoWork("A");
    return -1;
}
If(doWork("C")<0){
    undoWork("A");
    undoWork("B");
    return -1;
}
return 0;

This code works but looks very messy, especially I have a long list of doWork(X) to call. Is there a better and cleaner approach to handle error in this case?

possible duplicate of [How to avoid long chain of free's (or deletes) after every error check in C?](http://stackoverflow.com/questions/3339946/how-to-avoid-long-chain-of-frees-or-deletes-after-every-error-check-in-c) — karlphillip, Apr 07 '12 at 02:42
There are many good answers, but the best I can offer is a comment to go with them: whatever approach you take, you'll always minimize ugliness like this, and maximize robustness, by minimizing the number of steps that have a failure case. For example, precomputing the amount of memory you'll need and performing a single `malloc` before you get started instead of allocating small amounts at each step can make many tasks with complex error-out logic simple. — R.. GitHub STOP HELPING ICE, Apr 07 '12 at 03:56

George Skoptsov · Accepted Answer · 2012-04-07T03:14:19.553

Some people, especially beginner-to-intermediate programmers, have a very idiosyncratic reaction to seeing goto in production code, but the usual idiom for sequential acquiring of resources and their intelligent release upon error is the following:

if(doWork("A") < 0)
  goto errA;

if(doWork("B") < 0)
  goto errB;

if(doWork("C") < 0)
  goto errC;

/* success! */
return 0;

/* Error handling / releasing resources section */
errC:
  undoWork("B");
errB:
  undoWork("A");
errA:

return -1;

You will see plenty of examples in system code, e.g. in the linux kernel.

Flavius · Answer 2 · 2012-04-07T03:50:41.537

Being the same task doWork, you can probably define a linked list or vector of jobs and pass that as a parameter to doWork, append the corresponding information to this list inside the function, and only call undoWork once:

If(doWork("A", &jobs)<0){
    return -1;   
}
If(doWork("B", &jobs)<0){
    undoWork(jobs);
    return -1;
}
If(doWork("C", &jobs)<0){
    undoWork(jobs);
    return -1;
}
return 0;

This way, your logic will not become overly complicated, no matter the combination of jobs to be undone.

The advantage, compared to @twain249's solution, is that the function decides whether a job is added to the list or not, so you've got a nice isolation, modularity.

You can of course combine some form of an interable data structure with this, to further reduce the amount of repetitive code:

for(i=0; i < jobdata.size; i++) {
    If(doWork(jobdata[i], &jobs)<0){
        undowork(jobs);
        return -1;   
    }
}

As you can notice, data structure design plays an important role in algorithm design, usually a much more important one than one usually thinks.

There could be thousands of jobs, the code will remain a four-liner.

score 0 · Answer 3 · answered Apr 07 '12 at 02:31

0

Probably not. Newer languages like C++ and C# favor exceptions to help improve situations just like this.

Perhaps you could have a table that somehow indicated which tasks you've done and undo those. But I really think that would make your code more complex and not less.

Also note that, while there are some pretty strong feelings about using goto, there are in fact times when that can simplify structures like this.

answered Apr 07 '12 at 02:31

Jonathan Wood

65,341
71
269
466

Even newer languages like golang have even better constructs like `defer`. – Flavius Apr 07 '12 at 02:35

score 0 · Answer 4 · answered Apr 07 '12 at 02:40

if it's possible to store all things you have to call doWork on in an array then you could shorten the code significantly something like.

int i = 0;
int len = MAX_NUM; //set to the value of calls
int error = 0;

for(i = 0; i < len; i++) {
    if(doWork(a[i]) < 0) {
        error = 1;
        break;
    }
}

if(error) {
    for(int j = 0; j < i; i++) {
        undoWork(a[j]);
    }
    return -1;
}

pizza · Answer 5 · 2012-04-07T03:40:27.520

0

If you don't have a super long list, you can approach it this way.

if (dowork("A") >=0) {
if (dowork("B") >=0) {
if (dowork("C") >=0) {
if (dowork("D") >=0) return 0;
undowork("C"); }
undowork("B"); }
undowork("A"); }
return -1;

edited Apr 07 '12 at 03:40

answered Apr 07 '12 at 03:26

pizza

7,296
1
25
22

I don't understand why people were down voting this. When compiled it will be exactly the same as doing the goto/label technique. – EdH Apr 07 '12 at 03:45

score 0 · Answer 6 · answered Apr 07 '12 at 03:51

There is also another widely used approach based on a single pass loop that is clear and doesn't require goto. It implies though that Undo functions correctly handle both work that was done and that was not.

do
{
  if(doWork("A")<0)
    break;   

  if(doWork("B")<0)
    break;

  if(doWork("C")<0)
    break;

  return 0;
}
while(0);

undoWork("A");
undoWork("B");
undoWork("C");
return -1;

Clean error handling approach in C

6 Answers6