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