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