On 01/02/2014 10:32 AM, John Ferlan wrote:
>
>
> On 12/28/2013 11:11 AM, Eric Blake wrote:
>> Most of our public APIs emit a debug log on entry, prior to anything
>> else. There were a few exceptions where obvious failures were not
>> logged, so fix those. Many of the fixes are made more careful to
>
> s/careful/carefully/
I went with: Many of the changes include adding more caution to avoid a
NULL deref if the user passes NULL for the object.
>> +++ b/src/libvirt.c
>> @@ -1367,10 +1367,11 @@ virConnectOpen(const char *name)
>> {
>> virConnectPtr ret = NULL;
>>
>> + VIR_DEBUG("name=%s", NULLSTR(name));
>> +
>> if (virInitialize() < 0)
>> goto error;
>>
>> - VIR_DEBUG("name=%s", NULLSTR(name));
>
> For this and other virConnect*()'s being moved before virInitialize()...
Hmm. We document that for most APIs, you MUST call virInitialize() or
virConnectOpen*() first (and that virConnectOpen*() is one of the
exceptions that calls virInitialize() on your behalf).
>
> Something tells me there's some "historical reason" for the VIR_DEBUG()
> after virInitialize(). Something that perhaps some assumption that
> virLogMessage() or what it calls may assume has been initialized properly...
Indeed - VIR_DEBUG has the potential for different behavior before and
after virInitialize(); and if you are trying to log behavior of a
client, getting the connection open details logged is important.
>
> Although I suppose, since virGetVersion() already will VIR_DEBUG before
> virInitialize(), it's perhaps just an overly paranoid observation based
> on a previous job where debug/output logs got filled with messages
> because due to a similar change and some thread was waiting for
> initialization to complete and expected a failure from an entry routine
> such as this. Having 100's of entries in the log was "of concern".
I'm still thinking about whether to change the location in these
functions. But I think you're right, and I should drop those hunks.
>> @@ -7033,7 +7038,6 @@ virDomainMigratePrepareTunnel3(virConnectPtr conn,
>> const char *dname,
>> unsigned long bandwidth,
>> const char *dom_xml)
>> -
>
> Seems to be something more relevant to patch 1 (downside of peeking at
> finished product for me I guess).
And I moved it there accordingly.
>
> The rest is mechanical code movement and the extra checks for printing
> of "->object.u.s.refs"
>
> ACK - as long as you're satisfied that calling VIR_DEBUG before
> virInitialize is ok
I'll respin this one to keep virInitialize() first. The v2 series will
be shorter, though, once I push the non-controversial patches from this
review (thanks for tackling it, by the way).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list