[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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




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/

> avoid a NULL deref if the user passes NULL for the object.
> 
> * src/libvirt.c (virConnectOpen, virConnectOpenReadOnly)
> (virConnectOpenAuth, virConnectRef, virDomainRef)
> (virNetworkRef, virInterfaceRef, virStoragePoolRef)
> (virStorageVolRef, virNodeDeviceRef, virSecretRef, virStreamRef)
> (virNWFilterRef, virDomainSnapshotRef): Debug on function entry.
> * src/libvirt-lxc.c (virDomainLxcEnterNamespace)
> (virDomainLxcEnterSecurityLabel): Likewise.
> 
> Signed-off-by: Eric Blake <eblake redhat com>
> ---
>  src/libvirt-lxc.c |  7 +++++++
>  src/libvirt.c     | 53 +++++++++++++++++++++++++++++++++++------------------
>  2 files changed, 42 insertions(+), 18 deletions(-)
> 
> diff --git a/src/libvirt-lxc.c b/src/libvirt-lxc.c
> index 5cbe8bf..7ffed3e 100644
> --- a/src/libvirt-lxc.c
> +++ b/src/libvirt-lxc.c
> @@ -138,6 +138,10 @@ virDomainLxcEnterNamespace(virDomainPtr domain,
>  {
>      size_t i;
> 
> +    VIR_DOMAIN_DEBUG(domain, "nfdlist=%d, fdlist=%p, "
> +                     "noldfdlist=%p, oldfdlist=%p, flags=%x",
> +                     nfdlist, fdlist, noldfdlist, oldfdlist, flags);
> +
>      virCheckFlagsGoto(0, error);
> 
>      if (noldfdlist && oldfdlist) {
> @@ -196,6 +200,9 @@ virDomainLxcEnterSecurityLabel(virSecurityModelPtr model,
>                                 virSecurityLabelPtr oldlabel,
>                                 unsigned int flags)
>  {
> +    VIR_DEBUG("model=%p, label=%p, oldlabel=%p, flags=%x",
> +              model, label, oldlabel, flags);
> +
>      virCheckFlagsGoto(0, error);
> 
>      virCheckNonNullArgGoto(model, error);
> diff --git a/src/libvirt.c b/src/libvirt.c
> index f145d74..815dd64 100644
> --- a/src/libvirt.c
> +++ 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()...

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...

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".


>      virResetLastError();
>      ret = do_open(name, NULL, 0);
>      if (!ret)
> @@ -1403,10 +1404,11 @@ virConnectOpenReadOnly(const char *name)
>  {
>      virConnectPtr ret = NULL;
> 
> +    VIR_DEBUG("name=%s", NULLSTR(name));
> +
>      if (virInitialize() < 0)
>          goto error;
> 
> -    VIR_DEBUG("name=%s", NULLSTR(name));
>      virResetLastError();
>      ret = do_open(name, NULL, VIR_CONNECT_RO);
>      if (!ret)
> @@ -1443,10 +1445,11 @@ virConnectOpenAuth(const char *name,
>  {
>      virConnectPtr ret = NULL;
> 
> +    VIR_DEBUG("name=%s, auth=%p, flags=%x", NULLSTR(name), auth, flags);
> +
>      if (virInitialize() < 0)
>          goto error;
> 
> -    VIR_DEBUG("name=%s, auth=%p, flags=%x", NULLSTR(name), auth, flags);
>      virResetLastError();
>      ret = do_open(name, auth, flags);
>      if (!ret)
> @@ -1530,12 +1533,13 @@ error:
>  int
>  virConnectRef(virConnectPtr conn)
>  {
> +    VIR_DEBUG("conn=%p refs=%d", conn, conn ? conn->object.u.s.refs : 0);
> +
>      if ((!VIR_IS_CONNECT(conn))) {
>          virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("conn=%p refs=%d", conn, conn->object.u.s.refs);
>      virObjectRef(conn);
>      return 0;
>  }
> @@ -2457,13 +2461,14 @@ virDomainFree(virDomainPtr domain)
>  int
>  virDomainRef(virDomainPtr domain)
>  {
> +    VIR_DOMAIN_DEBUG(domain, "refs=%d", domain ? domain->object.u.s.refs : 0);
> +
>      if ((!VIR_IS_CONNECTED_DOMAIN(domain))) {
>          virLibConnError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
>          virDispatchError(NULL);
>          return -1;
>      }
> 
> -    VIR_DOMAIN_DEBUG(domain, "refs=%d", domain->object.u.s.refs);
>      virObjectRef(domain);
>      return 0;
>  }
> @@ -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).


>  {
>      VIR_DEBUG("conn=%p, stream=%p, cookiein=%p, cookieinlen=%d, cookieout=%p, "
>                "cookieoutlen=%p, flags=%lx, dname=%s, bandwidth=%lu, "
> @@ -7362,7 +7366,6 @@ virDomainMigratePrepareTunnel3Params(virConnectPtr conn,
>                                       char **cookieout,
>                                       int *cookieoutlen,
>                                       unsigned int flags)
> -

Same here w/r/t patch1

>  {
>      VIR_DEBUG("conn=%p, stream=%p, params=%p, nparams=%d, cookiein=%p, "
>                "cookieinlen=%d, cookieout=%p, cookieoutlen=%p, flags=%x",
> @@ -7898,7 +7901,6 @@ virNodeSuspendForDuration(virConnectPtr conn,
>                            unsigned long long duration,
>                            unsigned int flags)
>  {
> -

Same here w/r/t patch1




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

John


>      VIR_DEBUG("conn=%p, target=%d, duration=%lld, flags=%x",
>                conn, target, duration, flags);
> 
> @@ -12078,12 +12080,14 @@ virNetworkFree(virNetworkPtr network)
>  int
>  virNetworkRef(virNetworkPtr network)
>  {
> +    VIR_DEBUG("network=%p refs=%d", network,
> +              network ? network->object.u.s.refs : 0);
> +
>      if ((!VIR_IS_CONNECTED_NETWORK(network))) {
>          virLibConnError(VIR_ERR_INVALID_NETWORK, __FUNCTION__);
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("network=%p refs=%d", network, network->object.u.s.refs);
>      virObjectRef(network);
>      return 0;
>  }
> @@ -13046,12 +13050,13 @@ error:
>  int
>  virInterfaceRef(virInterfacePtr iface)
>  {
> +    VIR_DEBUG("iface=%p refs=%d", iface, iface ? iface->object.u.s.refs : 0);
> +
>      if ((!VIR_IS_CONNECTED_INTERFACE(iface))) {
>          virLibConnError(VIR_ERR_INVALID_INTERFACE, __FUNCTION__);
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("iface=%p refs=%d", iface, iface->object.u.s.refs);
>      virObjectRef(iface);
>      return 0;
>  }
> @@ -14116,12 +14121,13 @@ virStoragePoolFree(virStoragePoolPtr pool)
>  int
>  virStoragePoolRef(virStoragePoolPtr pool)
>  {
> +    VIR_DEBUG("pool=%p refs=%d", pool, pool ? pool->object.u.s.refs : 0);
> +
>      if ((!VIR_IS_CONNECTED_STORAGE_POOL(pool))) {
>          virLibConnError(VIR_ERR_INVALID_STORAGE_POOL, __FUNCTION__);
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("pool=%p refs=%d", pool, pool->object.u.s.refs);
>      virObjectRef(pool);
>      return 0;
>  }
> @@ -15242,12 +15248,13 @@ virStorageVolFree(virStorageVolPtr vol)
>  int
>  virStorageVolRef(virStorageVolPtr vol)
>  {
> +    VIR_DEBUG("vol=%p refs=%d", vol, vol ? vol->object.u.s.refs : 0);
> +
>      if ((!VIR_IS_CONNECTED_STORAGE_VOL(vol))) {
>          virLibConnError(VIR_ERR_INVALID_STORAGE_VOL, __FUNCTION__);
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("vol=%p refs=%d", vol, vol->object.u.s.refs);
>      virObjectRef(vol);
>      return 0;
>  }
> @@ -15947,12 +15954,13 @@ virNodeDeviceFree(virNodeDevicePtr dev)
>  int
>  virNodeDeviceRef(virNodeDevicePtr dev)
>  {
> +    VIR_DEBUG("dev=%p refs=%d", dev, dev ? dev->object.u.s.refs : 0);
> +
>      if ((!VIR_IS_CONNECTED_NODE_DEVICE(dev))) {
>          virLibConnError(VIR_ERR_INVALID_NODE_DEVICE, __FUNCTION__);
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("dev=%p refs=%d", dev, dev->object.u.s.refs);
>      virObjectRef(dev);
>      return 0;
>  }
> @@ -17074,12 +17082,14 @@ error:
>  int
>  virSecretRef(virSecretPtr secret)
>  {
> +    VIR_DEBUG("secret=%p refs=%d", secret,
> +              secret ? secret->object.u.s.refs : 0);
> +
>      if (!VIR_IS_CONNECTED_SECRET(secret)) {
>          virLibSecretError(VIR_ERR_INVALID_SECRET, __FUNCTION__);
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("secret=%p refs=%d", secret, secret->object.u.s.refs);
>      virObjectRef(secret);
>      return 0;
>  }
> @@ -17169,12 +17179,14 @@ virStreamNew(virConnectPtr conn,
>  int
>  virStreamRef(virStreamPtr stream)
>  {
> +    VIR_DEBUG("stream=%p refs=%d", stream,
> +              stream ? stream->object.u.s.refs : 0);
> +
>      if ((!VIR_IS_CONNECTED_STREAM(stream))) {
>          virLibConnError(VIR_ERR_INVALID_STREAM, __FUNCTION__);
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("stream=%p refs=%d", stream, stream->object.u.s.refs);
>      virObjectRef(stream);
>      return 0;
>  }
> @@ -17597,7 +17609,8 @@ virStreamEventAddCallback(virStreamPtr stream,
>                            void *opaque,
>                            virFreeCallback ff)
>  {
> -    VIR_DEBUG("stream=%p, events=%d, cb=%p, opaque=%p, ff=%p", stream, events, cb, opaque, ff);
> +    VIR_DEBUG("stream=%p, events=%d, cb=%p, opaque=%p, ff=%p",
> +              stream, events, cb, opaque, ff);
> 
>      virResetLastError();
> 
> @@ -18606,12 +18619,14 @@ error:
>  int
>  virNWFilterRef(virNWFilterPtr nwfilter)
>  {
> +    VIR_DEBUG("nwfilter=%p refs=%d", nwfilter,
> +              nwfilter ? nwfilter->object.u.s.refs : 0);
> +
>      if ((!VIR_IS_CONNECTED_NWFILTER(nwfilter))) {
>          virLibConnError(VIR_ERR_INVALID_NWFILTER, __FUNCTION__);
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("nwfilter=%p refs=%d", nwfilter, nwfilter->object.u.s.refs);
>      virObjectRef(nwfilter);
>      return 0;
>  }
> @@ -20944,13 +20959,15 @@ error:
>  int
>  virDomainSnapshotRef(virDomainSnapshotPtr snapshot)
>  {
> +    VIR_DEBUG("snapshot=%p, refs=%d", snapshot,
> +              snapshot ? snapshot->object.u.s.refs : 0);
> +
>      if ((!VIR_IS_DOMAIN_SNAPSHOT(snapshot))) {
>          virLibDomainSnapshotError(VIR_ERR_INVALID_DOMAIN_SNAPSHOT,
>                                    __FUNCTION__);
>          virDispatchError(NULL);
>          return -1;
>      }
> -    VIR_DEBUG("snapshot=%p, refs=%d", snapshot, snapshot->object.u.s.refs);
>      virObjectRef(snapshot);
>      return 0;
>  }
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]