Re: [libvirt] [PATCH 03/24] maint: move debug statements first in public API
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
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)