[ros-dev] Error handling

[Phillip Susi]

I've been doing a bit of work on the usetup code and came to realize 
that we have a bit of a widespread problem in a lot of code.  There is a 
great deal of code that looks basically like this pseudo code:

NSTATUS DoSomething()
{
    NSTATUS Status;
    ...

    Status = NtXXX();
    if( !NT_SUCCESS( Status ) )
       return Status;
    Status = NtYYY();
    if( !NT_SUCCESS( Status ) )
    {
       cleanupXXX();
       return Status;
    }
    Status = NtZZZ();
    if( !NT_SUCCESS( Status ) )
    {
       cleanupYYY();
       cleanupXXX();
       return Status;
    }
    // yay, everything worked
    cleanupZZZ();
    cleanupYYY();
    cleanupXXX();
    return STATUS_SUCCESS;
}


This type of error handling is absolutely horrible.  Just look at all of 
the duplicate code!  There are 3 different return points and each one 
duplicates all of the code from the previous, and adds one more line. 
Maintaining code like this becomes very hard, and it produces a larger, 
slower executing binary.

Now ideally all of those NtXXX functions would be throwing a SEH 
exception on failure instead of returning the failure code, and we could 
just write that code like so:

void do_something()
{
__try {
    NtXXX();
    NtYYY();
    NtZZZ();
    // manipulate the stuff you allocated
    }
__finally {
    cleanupZZZ();
    cleanupYYY();
    cleanupXXX();
    }
}

But sadly, I don't think that is ever going to happen.  Instead, I 
propose that we use goto to eliminate the duplicate points of return, 
and wrap the goto in a nice macro.  So far I've come up with this macro:

#define NTCALL(target, function) if( Status = function ) goto target;

And you use it like this:

NTSTATUS do_something()
{
    NTSTATUS Status;

    NTCALL( done, NtXXX(...) );
    NTCALL( cleanXXX, NtYYY(...) );
    NTCALL( cleanYYY, NtZZZ(...) );

    // manipulate x, y, and z here

cleanZZZ:
    cleanupZZZ();
cleanYYY:
    cleanupYYY();
cleanXXX:
    cleanupXXX();
done:
    return Status;
}

Now that code is more compact and maintainable than the original, and 
will produce a leaner binary.  I'm still trying to improve on it a bit 
but I like this basic idea, so I'm throwing it out for comments.  Last 
night I came up with this macro and in about 10 minutes I rewrote the 
file copy routine in usetup to use memory mapping instead of allocating 
a huge buffer, reading in the source file, then writing it out to the 
destination.  The new code involves making several more system calls 
than the old code, but since the error handling has been greatly cleaned 
up, the new code is more compact and easier to maintain.