[libvirt] [PATCH 03/24] maint: move debug statements first in public API

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140102/517bc026/attachment-0001.sig>