Re: [libvirt] [PATCH] secret_driver.c: remove dead cleanup code
On Mon, Jan 18, 2010 at 10:53:31AM +0100, Jim Meyering wrote:
Here's another example of appropriate use of assert.
The while loop removed below is currently guaranteed never
to be executed, since "list" is always NULL at that point.
However, you can argue that simply removing the loop is a little
risky: what if someone changes things to "goto cleanup" before
"list" reaches the final NULL? That's why I added the assertion:
to catch the potential (albeit unlikely) future coding error.
I don't really consider that a net win. If we leave the current code in
there and someone adds a 'goto cleanup' in future enhancement, then
everything will work as design. If we replace the current code with an
assert, then it will abort(), or silently do nothing & thus leak if
compiled with -DNDEBUG.
So while this is technically dead code at this time I don't think we
should be removing it from the cleanup: block. The cleanup: blocks
should be pessimistic about considering what has been cleaned up already,
even if this results in possible dead code warnings. We've had many
actual bugs from cleanup: blocks not free'ing stuff they should have
done, many of those introduced after refactorings of original code.
I realize this means that some automated code checkers like Coverity
will always complain about certain things, but these are not actual
bugs.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|