views:

484

answers:

11

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.

+5  A: 

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
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
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
+1 - The cut and run method.
Tim Post
+10  A: 

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

    int err = other_idea();
    if (err)
    {
        show_err();
        return;
    }
    happy_happy_joy_joy();
}
egrunin
+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
@torak I think it's generally accepted that implementation of happy_happy_joy_joy() is left as an exercise for the reader.
Will Hartung
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
`else` before `happy_...` seems superficial.
J.F. Sebastian
`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
@J.F. Sebastian: yes, it's vestigial. I inserted the return later and forgot to remove the else. I'll fix that now...
egrunin
@Judge Maygarden- I often do the same thing. It's one of those rare times when a `goto` statment is appropriate.
bta
+2  A: 

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
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
btw, there should be `glStrError(error)` (similar to `strerror(errno)`) instead of if/else/if/else print. Or `glPrintError("additional message")` (similar to `perror()`).
J.F. Sebastian
@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
@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
-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..
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
+2  A: 

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

jon
+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
+2  A: 

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.

DeadMG
+2  A: 

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
+14  A: 

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
+1 for dealing with the real world reality of resource cleanup
Alan Jackson
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
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
@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
@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
+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..
+3  A: 

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

return func1() && func2() && func3()
lhf
R..
A: 

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
A: 

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
"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
@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..
@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
@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..
@R..: Indeed. I'm not arguing for using macros, just for applying best practices *if and when* you use them.
Jason Williams
A: 

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