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

[Jim Meyering]

Daniel P. Berrange wrote:

> On Thu, Jan 07, 2010 at 11:03:23AM +0100, Daniel Veillard wrote:
>> On Thu, Jan 07, 2010 at 09:37:57AM +0100, Jim Meyering wrote:
>> >
>> > 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.
>
> The other reason for adding assert(), is if the code leading upto a
> particular point is too complex to reliably understand whether a
> variable is NULL. I think that applies in this case. I don't think
> adding an assert() is the right way to deal with that complexity
> though. I think it is better to make the code clearer/easier to
> follow & understand

I agree and have looked at the code in question (the somewhat
duplicated if/else blocks in proxy_internal.c lines 385-443)
http://libvirt.org/git/?p=libvirt.git;a=blob;f=src/xen/proxy_internal.c#l385
and don't see off hand why these if-blocks differ:

        res = request;
        if (res->len != sizeof(virProxyPacket)) {
            virProxyError(conn, VIR_ERR_INTERNAL_ERROR,
                          _("Communication error with proxy: expected %d bytes got %d\n"),
                          (int) sizeof(virProxyPacket), res->len);
            goto error;
        }


        res = (virProxyPacketPtr) answer;
        if ((res->len < sizeof(virProxyPacket)) ||
            (res->len > sizeof(virProxyFullPacket))) {
            virProxyError(conn, VIR_ERR_INTERNAL_ERROR,
                          _("Communication error with proxy: got %d bytes packet\n"),
                          res->len);
            goto error;
        }