31

Getting back in to some C work.

Many of my functions look like this:

int err = do_something(arg1, arg2, arg3, &result);

With the intent the result gets populated by the function, and the return value is the status of the call.

The darkside is you get something naive like this:

int err = func1(...);
if (!err) {
    err = func2(...);
    if (!err) {
        err = func3(...);
    }
}
return err;

I could macro it I suppose:

#define ERR(x) if (!err) { err = (x) }
int err = 0;
ERR(func1(...));
ERR(func2(...));
ERR(func3(...));
return err;

But that only works if I'm chaining function calls, vs doing other work.

Obviously Java, C#, C++ have exceptions that work very well for these kinds of things.

I'm just curious what other folks do and how other folks do error handling in their C programs nowadays.

Will Hartung
  • 115,893
  • 19
  • 128
  • 203
  • You could avoid such nesting by checking if `err` is non-zero and cleaning up / returning if it is. I think that would look better. – IVlad May 07 '10 at 16:16
  • I gave the solution I use for a different question: http://stackoverflow.com/questions/1073397/while1-break-instead-of-goto/1073490#1073490 – Makis May 07 '10 at 16:25
  • @Makis - that technique works fine if you only care about the existence of error, rather than the actual result code. I'll be interesting in the result code in my case. Thanks though! – Will Hartung May 07 '10 at 16:41
  • There is a "style" out there that uses C++ as if it was "C with exceptions". I have seen that before. The trouble I have is not so much with linear cascades of error checks, but the fact that my error checks are within several layers of functions. Exception handling can not be beat for the "make sure you get out of here now" thing. – Warren P May 07 '10 at 19:38
  • @Warren P: I have considered C++ because of issues like this, but I'm going to wait. As a corollary, I have seen exception mechanisms in C, using longjmp (I've actually used them, but that was year and years ago). – Will Hartung May 07 '10 at 21:03

12 Answers12

38

If you have resources that need to be released at the end, then sometimes the old trusty goto can be handy!

int
major_func(size_t len)
{
    int err;
    char *buf;

    buf = malloc(len);

    if (err = minor_func1(buf))
        goto major_func_end;
    if (err = minor_func2(buf))
        goto major_func_end;
    if (err = minor_func3(buf))
        goto major_func_end;

major_func_end:
    free(buf);
    return err;
}
Judge Maygarden
  • 26,961
  • 9
  • 82
  • 99
  • 6
    +1 for dealing with the real world reality of resource cleanup – Alan Jackson May 07 '10 at 18:54
  • 9
    Many of the open source code bases I have looked through used the idiom of "goto to resource collection on err". It is one place the otherwise maligned instruction shines. – Michael Speer May 07 '10 at 19:19
  • 2
    This can be done without a goto by using return(CleanupOnError(err, buf)); (where that method contains the code you have in your goto). As well as avoiding goto, you can sometimes use this same cleanup function from multiple processing functions (as long as they need the same cleanup of course) – Jason Williams May 07 '10 at 19:41
  • 2
    @Jason Williams: and you can write an entire program without control flow, but it's not a good idea either. Using return in this case isn't nearly as bad as that, but there's no reason not to use `goto` here. – RCIX May 08 '10 at 04:25
  • 1
    @RCIX: I was simply offering an alternative. An exit function-call is a much nicer idiom in many cases - especially to allow the code to be shared by other functions (e.g. "file parsing failed" error in a parser made up of 20 different functions that all operate on the same file). The advantages of using goto in this situation are that the cleanup code is embedded within the same function, and that it is executed by default, so it's harder to forget to call it. – Jason Williams May 08 '10 at 05:20
  • 4
    +1 for positive use of `goto`. What about giving another example with multiple goto labels depending on how much needs to be cleaned up at that point (in reverse order)? I use that idiom all the time. – R.. GitHub STOP HELPING ICE Jul 10 '10 at 09:31
17

Two typical patterns:

int major_func()
{
    int err = 0;

    if (err = minor_func1()) return err;
    if (err = minor_func2()) return err;
    if (err = minor_func3()) return err;

    return 0;
}

int other_idea()
{
    int err = minor_func1();
    if (!err)
        err = minor_func2();
    if (!err)
        err = minor_func3();
    return err;            
}

void main_func()
{
    int err = major_func();
    if (err)
    {
        show_err();
        return;
    }
    happy_happy_joy_joy();

    err = other_idea();
    if (err)
    {
        show_err();
        return;
    }
    happy_happy_joy_joy();
}
Bobby
  • 11,419
  • 5
  • 44
  • 69
egrunin
  • 24,650
  • 8
  • 50
  • 93
  • 6
    +1: Personally I'd go with the implementation shown in major_func(). Many people have an aversion to writing code with multiple return statements, but this is one case where the principle of locality should override those concerns. PS. Where's the implementation of happy_happy_joy_joy() ? – torak May 07 '10 at 16:32
  • 5
    @torak I think it's generally accepted that implementation of happy_happy_joy_joy() is left as an exercise for the reader. – Will Hartung May 07 '10 at 17:13
  • This and the other post are pretty similar, so I'll go with the community on this one barring some dramatic, unforeseen, exciting development. – Will Hartung May 07 '10 at 17:14
  • 3
    `else` before `happy_...` seems superficial. – jfs May 07 '10 at 17:18
  • 4
    `other_idea` is best when resources must be released before returning (e.g. malloc/free or mutex lock/unlock). Otherwise, I prefer `major_func ` as well. However, I must admit that I sometimes resort to `major_func` plus a `goto`. ;) – Judge Maygarden May 07 '10 at 18:19
  • @J.F. Sebastian: yes, it's vestigial. I inserted the return later and forgot to remove the else. I'll fix that now... – egrunin May 07 '10 at 19:24
  • 1
    @Judge Maygarden- I often do the same thing. It's one of those rare times when a `goto` statment is appropriate. – bta May 07 '10 at 20:04
8

What are you doing in the else statements? If nothing, try this:

int err = func1(...);
if (err) {
    return err;
}

err = func2(...);
if (err) {
    return err;
}

err = func3(...);

return err;

This way you're short-circuiting the entire function, not even bothering with the following function calls.

EDIT

Going back and reading again, I realize that it doesn't matter what you do in your else statements. That sort of code can easily go immediately after the if blocks.

Aaron
  • 6,988
  • 4
  • 31
  • 48
  • Putting `return` inside a macro is going to *kill* the next person who has to debug your code...not that I've never done it myself :) – egrunin May 07 '10 at 16:23
  • 1
    True. I thought about it and removed that part of my answer, because I decided that I think it's a bad idea too :) – Aaron May 07 '10 at 16:28
6

If error codes are boolean, then try the simpler code below:

return func1() && func2() && func3()
lhf
  • 70,581
  • 9
  • 108
  • 149
5

One approach which has been taken by OpenGL is to not return errors from functions at all but rather present an error state which can be examined after the function call. One nice thing about this approach is that when you have a function which you actually want to return something other than an error code, you can handle errors in the same way. Another thing which is nice about this is that if a user wants to call a number of functions and only succeed if all of them were successful, you can check for errors after the x amount of calls.

/* call a number of functions which may error.. */
glMatrixMode(GL_MODELVIEW);
glEnableClientState(GL_VERTEX_ARRAY);
glEnableClientState(GL_TEXTURE_COORD_ARRAY);
glEnable(GL_TEXTURE_2D);

/* ...check for errors */
if ((error = glGetError()) != GL_NO_ERROR) {
    if (error == GL_INVALID_VALUE)
        printf("error: invalid value creating view");
    else if (error == GL_INVALID_OPERATION)
        printf("error: invalid operation creating view");
    else if (error == GL_OUT_OF_MEMORY)
        printf("error: out of memory creating view");
}
Nick Van Brunt
  • 15,244
  • 11
  • 66
  • 92
  • This is nice. You still have the burden of checking the error code, but you can do it as much (or as little) as you like. I suppose the only discipline is to not reset the error state, except perhaps in the glGetError function. – Will Hartung May 07 '10 at 19:08
  • 1
    btw, there should be `glStrError(error)` (similar to `strerror(errno)`) instead of if/else/if/else print. Or `glPrintError("additional message")` (similar to `perror()`). – jfs May 07 '10 at 19:23
  • @Will, This is exactly what openGl does - "When an error occurs, the error flag is set to the appropriate error code value. No other errors are recorded until glGetError is called, the error code is returned, and the flag is reset to GL_NO_ERROR." – Nick Van Brunt May 07 '10 at 19:48
  • @J.F. Sebastian +1 but for simplicity's sake one may not go to the trouble with error strings and all the alloc which could end up in this lovely error code LUA_ERRERR: error while running the error handler function. – Nick Van Brunt May 08 '10 at 02:56
  • 2
    -1 for recommending global state/global variables. If your API has an object/context structure to keep the error state, this approach is great. But if you have to resort to a global variable, you have all sorts of nightmares to deal with involving: (1) thread-safety, (2) performance/portability when you try to make it thread-safe, (3) issues with dynamic-loaded modules, global variables, and possibly associated memory leaks, .... – R.. GitHub STOP HELPING ICE Jul 10 '10 at 09:34
  • No where did I say the error state has to be saved in a global. I'm not sure where you got this from. In this case I have no idea where opengl keeps its error state only that I can retrieve it with glGetError(). – Nick Van Brunt Jul 12 '10 at 14:51
4

Others have suggested good ideas. Here're the idioms I've seen

int err;
...
err = foo(...);
if (err)
    return err;
...

You could macro this out to something like

#define dERR int err=0
#define CALL err = 
#define CHECK do { if (err) return err } while(0)
...
void my_func(void) {
   dERR;
   ...
   CALL foo(...);
   CHECK;

or, if you're feeling really motivated, fiddle with CALL and CHECK so they can be used like

CALL foo(...) CHECK;

or

CALL( foo(...) );

--

Often, functions which need to do cleanup on exit (e.g. free memory) are written like this:

int do_something_complicated(...) {
    ...

    err = first_thing();
    if (err)
       goto err_out;

    buffer = malloc(...);
    if (buffer == NULL)
        goto err_out

    err = another_complicated(...);
    if (err)
        goto err_out_free;

    ...

   err_out_free:
    free(buffer);
   err_out:
    return err; /* err might be zero */
}

You could use that pattern, or try to simplify it with macros.

--

Finally, if you're feeling /really/ motivated, you can use setjmp/longjmp.

int main(int argc, char *argv[]) {
    jmp_buf on_error;
    int err;
    if (err = setjmp(on_error)) {
        /* error occurred, error code in err */
        return 1;
    } else {
        actual_code(..., on_error);
        return 0;
    }
}
void actual_code(..., jmp_buf on_error) {
    ...
    if (err)
        longjmp(on_error, err);
}

Essentially, a declaration of a new jmp_buf and a setjmp function as setting up a try block. The case where setjmp returns non-zero is your catch, and calling longjmp is your throw. I wrote this with passing the jmp_buf around in case you want nested handlers (e.g. if you need to free stuff before signaling an error); if you don't need that, feel free to declare err and the jmp_buf as globals.

Alternately, you could use macros to simply the argument passing around. I'd suggest the way Perl's implementation does it:

#define pERR jmp_buf _err_handler
#define aERR _err_handler
#define HANDLE_ERRORS do { jmp_buf _err_handler; int err = setjmp(_err_handler);
#define END_HANDLE while(0)
#define TRY if (! err)
#define CATCH else
#define THROW(e) longjmp(_err_handler, e)

void always_fails(pERR, int other_arg) {
    THROW(42);
}
void does_some_stuff(pERR) {
    normal_call(aERR);
    HANDLE_ERRORS
      TRY {
        always_fails(aERR, 23);
      } CATCH {
        /* err is 42 */
      }
    END_HANDLE;
}
int main(int argc, char *argv[]) {
    HANDLE_ERRORS
      TRY {
        does_some_stuff(aERR);
        return 0;
      } CATCH {
        return err;
      }
    DONE_ERRORS;
}

--

Phew. I'm done. (Crazy examples untested. Some details might be off.)

jade
  • 744
  • 5
  • 16
  • 1
    +1 for mentioning longjmp. This seems to be common in older libraries, libpng being an example. I've never found this method very intuitive but it works well when resources need to be freed on error. – Nick Van Brunt May 07 '10 at 19:57
4

And now for something completely different...

Another approach is to use a struct to contain your error information, e.g:

struct ErrorInfo
{
    int errorCode;
    char *errorMessage;
#if DEBUG
    char *functionName;
    int lineNumber;
#endif
}

The best way to use this is to return your method's results as the return code (e.g. "FALSE for failed", or "a file pointer or NULL if it fails", or "size of the buffer or 0 if it fails", etc) and pass in an ErrorInfo as a parameter that the called function will fill in if something fails.

This gives rich error reporting: if the method fails, you can fill in more than a simple error code (e.g. error message, code line and file of the failure, or whatever). The nice thing about it being a struct is that if you think of something, anything, useful later, you can just add it - for example, in my struct above I've allowed for a debug build to include the location of the error (file/line), but you could add a dump of the whole call stack in there at any time without having to change any of the client code.

You can use a global function to fill in an ErrorInfo so the error return can be managed cleanly, and you can update the struct to provide more info easily:

if (error)
{
    Error(pErrorInfo, 123, "It failed");
    return(FALSE);
}

...and you can have variants of this function that return FALSE, 0, or NULL, to allow most error returns to be phrased as a single line:

if (error)
    return(ErrorNull(pErrorInfo, 123, "It failed"));

This gives you a lot of the advantages of an Exception class in other languages (although the caller still needs to handle the errors - callers have to check for error codes and may have to return early, but they can do nothing or next-to-nothing and allow the error to propagate back up a chain of calling methods until one of them wishes to handle it, much like an exception.

In addition, you can go further, to create a chain of error reports (like "InnerException"s):

struct ErrorInfo
{
    int errorCode;
    char *errorMessage;
    ...
    ErrorInfo *pInnerError;    // Pointer to previous error that may have led to this one
}

Then, if you "catch" an error from a function that you call, you can create a new, higher-level error description, and return a chain of these errors. e.g. "Mouse speed will revert to the default value" (because) "Preference block 'MousePrefs' could not be located" (because) "XML reader failed" (because) "File not found".

i.e.

FILE *OpenFile(char *filename, ErrorInfo *pErrorInfo)
{
    FILE *fp = fopen(filename, "rb");
    if (fp == NULL)
        return(ChainedErrorNull(pErrorInfo, "Couldn't open file"));

    return(fp);
}

XmlElement *ReadPreferenceXml(ErrorInfo *pErrorInfo)
{
    if (OpenFile("prefs.xml", pErrorInfo) == NULL)
        return(ChainedErrorNull(pErrorInfo, "Couldn't read pref"));
    ...
}

char *ReadPreference(char *prefName, ErrorInfo *pErrorInfo)
{
    XmlElement *pXml = ReadPreferenceXml(pErrorInfo);
    if (pXml == NULL)
        return(ChainedErrorNull(pErrorInfo, "Couldn't read pref"));
    ...
}
Jason Williams
  • 56,972
  • 11
  • 108
  • 137
3

You should check out what DirectX has done with the HRESULT - it's basically this. There's a reason that the exception came into being. Alternatively, if you run on Win32, they have SEH which runs in C programs.

Puppy
  • 144,682
  • 38
  • 256
  • 465
2

You can get really silly and do continuations:

void step_1(int a, int b, int c, void (*step_2)(int), void (*err)(void *) ) {
     if (!c) {
         err("c was 0");
     } else {
         int r = a + b/c;
         step_2(r);
     }
}

This probably isn't actually what you want to do, but it is how many functional programming languages are used, and even more often how they model their code for optimization.

nategoose
  • 12,054
  • 27
  • 42
1

Something I've recently seen is this idom:

int err;
do 
{
  err = func1 (...);
  if (!err) break;

  err = func2 (...);
  if (!err) break;

  err = func3 (...);
  if (!err) break;

  /* add more calls here */

} while (0);

if (err)
{
  /* handle the error here */
  return E_ERROR; /* or something else */
}
 else 
{
  return E_SUCCESS;
}

Pro arguments:

It avoids the goto (abuses the while(0) / break combination for that). Why would you want to do this? It keeps the cyclomatic complexity down and will still pass most static code analyzer checks (MISRA anyone?). For projects that get tested against cyclomatic complexity this is a god sent because it keeps all the initialization stuff together.

Contra arguments:

The meaning of the do/while loop construct is not obvious because a loop-construct is used as a cheap goto replacement, and this can only be seen at the loop tail. I'm sure for the first time this construct will cause lots of "WTF"-moments.

At least a comment is necessary to explain why the code is written the way it is required.

Nils Pipenbrinck
  • 83,631
  • 31
  • 151
  • 221
  • "The meaning of the loop is not obvious": You could use macros (e.g. START_ERROR_HANDLING_BLOCK, CHECK_ERROR, END_ERROR_HANDLING_BLOCK, to use rather nasty wordy names) to make the error-handling purpose of the code more obvious. – Jason Williams May 08 '10 at 05:28
  • 3
    @Jason, I think the macros make the intention more obvious but make the actual flow and implementation much less obvious. I generally hate code with flow control hidden in macros. @Nils, abusing a do/while loop as a goto because of ideological nonsense about gotos is an abomination... – R.. GitHub STOP HELPING ICE Jul 10 '10 at 09:40
  • @R..: I agree, macros must be used with care. But if you use a mechanism like this commonly (throughout a whole library) and it is well named and documented, then a macro can help to hide the messy implementation details while actually clarifying the "meaning" of the code. I would personally tend towards using a SUCCESS() style of macro for every test (it's a common idiom, e.g. in COM code) rather than a loop-start and loop-end approach - it's safer and gives you more control over every specific case. – Jason Williams Jul 10 '10 at 20:03
  • 3
    @Jason, it still requires someone reading the code to be familiar with your specific library's idioms, whereas a simple `if (failed) goto failure;` is perfectly readable by anyone who knows C. – R.. GitHub STOP HELPING ICE Jul 11 '10 at 04:50
  • @R..: Indeed. I'm not arguing for using macros, just for applying best practices *if and when* you use them. – Jason Williams Jul 11 '10 at 10:00
  • If you look at MISRA 2004, you passed 14.4 but now fail at 14.6, which prohibits multiple breaks for loop termination. – Hugo Dec 05 '12 at 00:37
1

Here's a quite informative article & test file by IBM Unix article series:

Errors: errno in UNIX programs

Working with the standard error mechanism

https://www.ibm.com/developerworks/aix/library/au-errnovariable/

Another good example of how to implement exit codes is the source code of curl (man 1 curl).

pivz
  • 11
  • 1
  • One of the annoying things about errno, is that you can't use it yourself. So, you can't leverage the existing system, you have to not only make your own, but it has to live in parallel with theirs. You'll notice in that article that errno is a macro for (*__error()), which means you can't assign it. Nor is there a system primitive that lets you assign it. So, just...annoying. – Will Hartung May 08 '10 at 16:11
  • 3
    @Will, you're badly mistaken. `errno` is a writable lvalue, and in fact certain standard library functions **require** you to explicitly zero `errno` before calling them if you want to detect errors (since all possible return values are in the co-domain of the function). I think you confused `(*__error())` with `(__error())` or else you don't understand pointers... – R.. GitHub STOP HELPING ICE Jul 10 '10 at 09:38
0

Provided you are working with a specific context, I think the following pattern is very nice. The basic idea is that operations on an error-set state are no-ops, so error checking can be postponed to when it is convenient!

A concrete example: A deserialization context. Decoding of any element can fail, but the function may continue without error checking because all the decode_* functions are no-ops when the serialization record is in an error state. It's a matter of convenience or opportunity or optimization to insert decode_has_error. In the example below, there is no error check, the caller will take care of that.

void list_decode(struct serialization_record *rec,                       
                 struct list *list,                                     
                 void *(*child_decode)(struct serialization_record *)) {
    uint32_t length;                                                             
    decode_begin(rec, TAG);                                  
    decode_uint32(rec, &length);                                          
    for (uint32_t i = 0; i < length; i++) {                                
        list_append(list, child_decode(rec));
    }                                                                        
    decode_end(rec, TAG);
}
u0b34a0f6ae
  • 48,117
  • 14
  • 92
  • 101