[ros-dev] Error handling
Eric Kohl
eric.kohl at t-online.de
Mon Apr 11 22:43:07 CEST 2005
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
More information about the Ros-dev
mailing list