[libvirt] [PATCH] don't test "res == NULL" after we've already dereferenced it

[Daniel Veillard]

On Thu, Jan 07, 2010 at 09:37:57AM +0100, Jim Meyering wrote:
> Daniel P. Berrange wrote:
> >> Considering that this is in the daemon and that "bad things"
> >> have been known to happen via NULL derefs, some would
> >> argue that an assertion failure is preferable.
> >
> > No, this code is the client side of the Xen proxy implementation, that is
> > never used within daemon context.
> 
> Oh, right.  Thanks.
> 
> However, the point is still valid, so I'll wait for confirmation.
> This is still about defensive coding, i.e., ensuring that
> maintenance doesn't violate invariants in harder-to-diagnose ways.
> If you get a bug report, which would you rather hear?
> "libvirt sometimes segfaults" or
> "I get an assertion failure on file.c:999"

  I guess it's mostly a matter of coding style, I'm not sure it makes
such a difference in practice, libvirt output will likely be burried
in a log file, unless virsh or similar CLI tool is used.
  We have only 4 asserts in the code currently, I think it shows that
so far we are not relying on it. On one hand this mean calling exit()
and I prefer a good old core dump for debugging than a one line message,
on the other hand if you managed to catch that message, well this can
help pinpoint if the person reporting has no debugging skills.

  I think there is pros and cons and that the current status quo is
that we don't use asserts but more defensing coding by erroring out
when this happen. The best way is not to assert() when we may
dereference a NULL pointer but check for NULL at the point where
we get that pointer, that's closer to the current code practice of
libvirt (or well I expect so :-) and allow useful reporting (we
failed to do a given operation) and a graceful error handling without
exit'ing. So in general if we think we need an assert somewhere that
mean that we failed to do the check at the right place, and I would
rather not start to get into the practice of just asserting in a zillion
place instead of checking at the correct place.

  So in my opinion, I'm still not fond of assert(), and I would prefer
to catch up problem earlier, like parameter checking on function entry
points or checking return value for functions producing pointers.

  In that case, res being NULL means that both answer and request
parameters are null  after the retry: label, since the two pointers
are not modified in the function this should be tested when entering
the function,

    if ((answer == NULL) && (request == NULL)) {
        virProxyError (NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
        return -1;
    }

you get the error location, as in the assert but propagate the error
back in the stack cleanly instead of exit'ing

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/