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@xxxxxxxxxx>
> ---
> 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;
> }
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list