Re: [ros-dev] Error handling

[Eric Kohl]

Hello Philp!

Great to see you're back!
> 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.

At the time I wrote this code it was more important to make it work 
reliably than to make it look good. File I/O has been pretty instable at 
this time because of several bugs in the io manager and the cache 
manager. That's why I wrote it this way; it worked in most cases! ;-)
> 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.

I still prefer using the bare gotos because in my opinion it is a lot 
easier to read. But otherwise I agree with your improvements.

NTSTATUS DoSomething()
{
   NSTATUS Status;
   ...

Status = NtXXX();
   if (!NT_SUCCESS(Status))
     goto done;

Status = NtYYY();
   if (!NT_SUCCESS(Status))
     goto cleanXXX;

Status = NtZZZ();
   if (!NT_SUCCESS(Status))
     goto cleanYYY;

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

Regards,
Eric