Ever since their introduction (commit 1509b80 in v0.5.0 for virConnectDomainEventRegister, commit 4445723 in v0.8.0 for virConnectDomainEventDeregisterAny), the event deregistration functions have been documented as returning 0 on success; likewise for older registration (only the newer RegisterAny must return a non-zero callbackID). And now that we are adding virConnectNetworkEventDeregisterAny for v1.2.1, it should have the same semantics. Fortunately, all of the stateful drivers have been obeying the docs and returning 0, thanks to the way the remote_driver tracks things (in fact, the RPC wire protocol is unable to send a return value for DomainEventRegisterAny, at least not without adding a new RPC number). Well, except for vbox, which was always failing, due to failure to set the return value to anything besides its initial -1 value. But for local drivers, such as test:///default, we've been returning non-zero numbers; worse, the non-zero numbers have differed over time. For example, in Fedora 12 (libvirt 0.8.2), calling Register twice would return 0 and 1 [the callbackID generated under the hood]; while in Fedora 20 (libvirt 1.1.3), it returns 1 and 2 [the number of callbacks registered for that event type]. Since we have changed the behavior over time, and since it differs by local vs. remote, we can safely argue that no one could have been reasonably relying on any particular behavior, so we might as well obey the docs, as well as prepare callers that might deal with older clients to not be surprised if the docs are not strictly followed. For consistency, this patch fixes the code for all drivers, even though it only makes an impact for vbox and for local drivers. By fixing all drivers, future copy and paste from a remote driver to a local driver is less likely to reintroduce the bug. * src/libvirt.c (virConnectDomainEventRegister) (virConnectDomainEventDeregister) (virConnectDomainEventDeregisterAny): Clarify docs. * src/libxl/libxl_driver.c (libxlConnectDomainEventRegister) (libxlConnectDomainEventDeregister) (libxlConnectDomainEventDeregisterAny): Match documentation. * src/lxc/lxc_driver.c (lxcConnectDomainEventRegister) (lxcConnectDomainEventDeregister) (lxcConnectDomainEventDeregisterAny): Likewise. * src/test/test_driver.c (testConnectDomainEventRegister) (testConnectDomainEventDeregister) (testConnectDomainEventDeregisterAny) (testConnectNetworkEventDeregisterAny): Likewise. * src/uml/uml_driver.c (umlConnectDomainEventRegister) (umlConnectDomainEventDeregister) (umlConnectDomainEventDeregisterAny): Likewise. * src/vbox/vbox_tmpl.c (vboxConnectDomainEventRegister) (vboxConnectDomainEventDeregister) (vboxConnectDomainEventDeregisterAny): Likewise. * src/xen/xen_driver.c (xenUnifiedConnectDomainEventRegister) (xenUnifiedConnectDomainEventDeregister) (xenUnifiedConnectDomainEventDeregisterAny): Likewise. * src/network/bridge_driver.c (networkConnectNetworkEventDeregisterAny): Likewise. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/libvirt.c | 17 +++++++++++------ src/libxl/libxl_driver.c | 35 ++++++++++++++++++----------------- src/lxc/lxc_driver.c | 30 +++++++++++++++--------------- src/network/bridge_driver.c | 11 +++++++---- src/test/test_driver.c | 38 +++++++++++++++++++++----------------- src/uml/uml_driver.c | 29 ++++++++++++++++------------- src/vbox/vbox_tmpl.c | 32 ++++++++++++++++++++++---------- src/xen/xen_driver.c | 25 ++++++++++++++----------- 8 files changed, 124 insertions(+), 93 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index bae504c..5b9e725 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -15975,7 +15975,9 @@ error: * The reference can be released once the object is no longer required * by calling virDomainFree. * - * Returns 0 on success, -1 on failure. + * Returns 0 on success, -1 on failure. Older versions of some hypervisors + * sometimes returned a positive number on success, but without any reliable + * semantics on what that number represents. */ int virConnectDomainEventRegister(virConnectPtr conn, @@ -16013,14 +16015,16 @@ error: * @conn: pointer to the connection * @cb: callback to the function handling domain events * - * Removes a callback previously registered with the virConnectDomainEventRegister - * function. + * Removes a callback previously registered with the + * virConnectDomainEventRegister() function. * * Use of this method is no longer recommended. Instead applications * should try virConnectDomainEventDeregisterAny() which has a more flexible * API contract * - * Returns 0 on success, -1 on failure + * Returns 0 on success, -1 on failure. Older versions of some hypervisors + * sometimes returned a positive number on success, but without any reliable + * semantics on what that number represents. */ int virConnectDomainEventDeregister(virConnectPtr conn, @@ -19014,8 +19018,9 @@ error: * Removes an event callback. The callbackID parameter should be the * value obtained from a previous virConnectDomainEventRegisterAny() method. * - * Returns 0 on success, -1 on failure - */ + * Returns 0 on success, -1 on failure. Older versions of some hypervisors + * sometimes returned a positive number on success, but without any reliable + * semantics on what that number represents. */ int virConnectDomainEventDeregisterAny(virConnectPtr conn, int callbackID) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index b24c195..61e3516 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1,7 +1,7 @@ /* * libxl_driver.c: core driver methods for managing libxenlight domains * - * Copyright (C) 2006-2013 Red Hat, Inc. + * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2011-2013 SUSE LINUX Products GmbH, Nuernberg, Germany. * Copyright (C) 2011 Univention GmbH. * @@ -3643,20 +3643,21 @@ cleanup: static int libxlConnectDomainEventRegister(virConnectPtr conn, - virConnectDomainEventCallback callback, void *opaque, + virConnectDomainEventCallback callback, + void *opaque, virFreeCallback freecb) { libxlDriverPrivatePtr driver = conn->privateData; - int ret; if (virConnectDomainEventRegisterEnsureACL(conn) < 0) return -1; - ret = virDomainEventStateRegister(conn, - driver->domainEventState, - callback, opaque, freecb); + if (virDomainEventStateRegister(conn, + driver->domainEventState, + callback, opaque, freecb) < 0) + return -1; - return ret; + return 0; } @@ -3665,16 +3666,16 @@ libxlConnectDomainEventDeregister(virConnectPtr conn, virConnectDomainEventCallback callback) { libxlDriverPrivatePtr driver = conn->privateData; - int ret; if (virConnectDomainEventDeregisterEnsureACL(conn) < 0) return -1; - ret = virDomainEventStateDeregister(conn, - driver->domainEventState, - callback); + if (virDomainEventStateDeregister(conn, + driver->domainEventState, + callback) < 0) + return -1; - return ret; + return 0; } static int @@ -4270,16 +4271,16 @@ static int libxlConnectDomainEventDeregisterAny(virConnectPtr conn, int callbackID) { libxlDriverPrivatePtr driver = conn->privateData; - int ret; if (virConnectDomainEventDeregisterAnyEnsureACL(conn) < 0) return -1; - ret = virObjectEventStateDeregisterID(conn, - driver->domainEventState, - callbackID); + if (virObjectEventStateDeregisterID(conn, + driver->domainEventState, + callbackID) < 0) + return -1; - return ret; + return 0; } diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index e598474..7e56a59 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1287,16 +1287,16 @@ lxcConnectDomainEventRegister(virConnectPtr conn, virFreeCallback freecb) { virLXCDriverPtr driver = conn->privateData; - int ret; if (virConnectDomainEventRegisterEnsureACL(conn) < 0) return -1; - ret = virDomainEventStateRegister(conn, - driver->domainEventState, - callback, opaque, freecb); + if (virDomainEventStateRegister(conn, + driver->domainEventState, + callback, opaque, freecb) < 0) + return -1; - return ret; + return 0; } @@ -1305,16 +1305,16 @@ lxcConnectDomainEventDeregister(virConnectPtr conn, virConnectDomainEventCallback callback) { virLXCDriverPtr driver = conn->privateData; - int ret; if (virConnectDomainEventDeregisterEnsureACL(conn) < 0) return -1; - ret = virDomainEventStateDeregister(conn, - driver->domainEventState, - callback); + if (virDomainEventStateDeregister(conn, + driver->domainEventState, + callback) < 0) + return -1; - return ret; + return 0; } @@ -1347,16 +1347,16 @@ lxcConnectDomainEventDeregisterAny(virConnectPtr conn, int callbackID) { virLXCDriverPtr driver = conn->privateData; - int ret; if (virConnectDomainEventDeregisterAnyEnsureACL(conn) < 0) return -1; - ret = virObjectEventStateDeregisterID(conn, - driver->domainEventState, - callbackID); + if (virObjectEventStateDeregisterID(conn, + driver->domainEventState, + callbackID) < 0) + return -1; - return ret; + return 0; } diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3e10758..9dc1f7e 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1,7 +1,7 @@ /* * bridge_driver.c: core driver methods for managing network * - * Copyright (C) 2006-2013 Red Hat, Inc. + * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -2329,9 +2329,12 @@ networkConnectNetworkEventDeregisterAny(virConnectPtr conn, if (virConnectNetworkEventDeregisterAnyEnsureACL(conn) < 0) goto cleanup; - ret = virObjectEventStateDeregisterID(conn, - driver->networkEventState, - callbackID); + if (virObjectEventStateDeregisterID(conn, + driver->networkEventState, + callbackID) < 0) + goto cleanup; + + ret = 0; cleanup: return ret; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index a48404a..d0f3fa8 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1,7 +1,7 @@ /* * test.c: A "mock" hypervisor for use by application unit tests * - * Copyright (C) 2006-2013 Red Hat, Inc. + * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -5993,12 +5993,13 @@ testConnectDomainEventRegister(virConnectPtr conn, virFreeCallback freecb) { testConnPtr driver = conn->privateData; - int ret; + int ret = 0; testDriverLock(driver); - ret = virDomainEventStateRegister(conn, - driver->domainEventState, - callback, opaque, freecb); + if (virDomainEventStateRegister(conn, + driver->domainEventState, + callback, opaque, freecb) < 0) + ret = -1; testDriverUnlock(driver); return ret; @@ -6010,12 +6011,13 @@ testConnectDomainEventDeregister(virConnectPtr conn, virConnectDomainEventCallback callback) { testConnPtr driver = conn->privateData; - int ret; + int ret = 0; testDriverLock(driver); - ret = virDomainEventStateDeregister(conn, - driver->domainEventState, - callback); + if (virDomainEventStateDeregister(conn, + driver->domainEventState, + callback) < 0) + ret = -1; testDriverUnlock(driver); return ret; @@ -6049,12 +6051,13 @@ testConnectDomainEventDeregisterAny(virConnectPtr conn, int callbackID) { testConnPtr driver = conn->privateData; - int ret; + int ret = 0; testDriverLock(driver); - ret = virObjectEventStateDeregisterID(conn, - driver->domainEventState, - callbackID); + if (virObjectEventStateDeregisterID(conn, + driver->domainEventState, + callbackID) < 0) + ret = -1; testDriverUnlock(driver); return ret; @@ -6089,12 +6092,13 @@ testConnectNetworkEventDeregisterAny(virConnectPtr conn, int callbackID) { testConnPtr driver = conn->privateData; - int ret; + int ret = 0; testDriverLock(driver); - ret = virObjectEventStateDeregisterID(conn, - driver->domainEventState, - callbackID); + if (virObjectEventStateDeregisterID(conn, + driver->domainEventState, + callbackID) < 0) + ret = -1; testDriverUnlock(driver); return ret; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 1784eb5..ad29ebf 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1,7 +1,7 @@ /* * uml_driver.c: core driver methods for managing UML guests * - * Copyright (C) 2006-2013 Red Hat, Inc. + * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -2610,15 +2610,16 @@ umlConnectDomainEventRegister(virConnectPtr conn, virFreeCallback freecb) { struct uml_driver *driver = conn->privateData; - int ret; + int ret = 0; if (virConnectDomainEventRegisterEnsureACL(conn) < 0) return -1; umlDriverLock(driver); - ret = virDomainEventStateRegister(conn, - driver->domainEventState, - callback, opaque, freecb); + if (virDomainEventStateRegister(conn, + driver->domainEventState, + callback, opaque, freecb) < 0) + ret = -1; umlDriverUnlock(driver); return ret; @@ -2629,15 +2630,16 @@ umlConnectDomainEventDeregister(virConnectPtr conn, virConnectDomainEventCallback callback) { struct uml_driver *driver = conn->privateData; - int ret; + int ret = 0; if (virConnectDomainEventDeregisterEnsureACL(conn) < 0) return -1; umlDriverLock(driver); - ret = virDomainEventStateDeregister(conn, - driver->domainEventState, - callback); + if (virDomainEventStateDeregister(conn, + driver->domainEventState, + callback) < 0) + ret = -1; umlDriverUnlock(driver); return ret; @@ -2674,15 +2676,16 @@ umlConnectDomainEventDeregisterAny(virConnectPtr conn, int callbackID) { struct uml_driver *driver = conn->privateData; - int ret; + int ret = 0; if (virConnectDomainEventDeregisterAnyEnsureACL(conn) < 0) return -1; umlDriverLock(driver); - ret = virObjectEventStateDeregisterID(conn, - driver->domainEventState, - callbackID); + if (virObjectEventStateDeregisterID(conn, + driver->domainEventState, + callbackID) < 0) + ret = -1; umlDriverUnlock(driver); return ret; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 385ee54..0fcaf8e 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -8,7 +8,7 @@ */ /* - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * Copyright (C) 2008-2009 Sun Microsystems, Inc. * * This file is part of a free software library; you can redistribute @@ -7284,10 +7284,12 @@ static void vboxReadCallback(int watch ATTRIBUTE_UNUSED, } } -static int vboxConnectDomainEventRegister(virConnectPtr conn, - virConnectDomainEventCallback callback, - void *opaque, - virFreeCallback freecb) { +static int +vboxConnectDomainEventRegister(virConnectPtr conn, + virConnectDomainEventCallback callback, + void *opaque, + virFreeCallback freecb) +{ VBOX_OBJECT_CHECK(conn, int, -1); int vboxRet = -1; nsresult rc; @@ -7338,7 +7340,7 @@ static int vboxConnectDomainEventRegister(virConnectPtr conn, vboxDriverUnlock(data); if (ret >= 0) { - return ret; + return 0; } else { if (data->vboxObj && data->vboxCallback) { data->vboxObj->vtbl->UnregisterCallback(data->vboxObj, data->vboxCallback); @@ -7347,8 +7349,10 @@ static int vboxConnectDomainEventRegister(virConnectPtr conn, } } -static int vboxConnectDomainEventDeregister(virConnectPtr conn, - virConnectDomainEventCallback callback) { +static int +vboxConnectDomainEventDeregister(virConnectPtr conn, + virConnectDomainEventCallback callback) +{ VBOX_OBJECT_CHECK(conn, int, -1); int cnt; @@ -7371,6 +7375,9 @@ static int vboxConnectDomainEventDeregister(virConnectPtr conn, vboxDriverUnlock(data); + if (cnt >= 0) + ret = 0; + return ret; } @@ -7441,8 +7448,10 @@ static int vboxConnectDomainEventRegisterAny(virConnectPtr conn, } } -static int vboxConnectDomainEventDeregisterAny(virConnectPtr conn, - int callbackID) { +static int +vboxConnectDomainEventDeregisterAny(virConnectPtr conn, + int callbackID) +{ VBOX_OBJECT_CHECK(conn, int, -1); int cnt; @@ -7465,6 +7474,9 @@ static int vboxConnectDomainEventDeregisterAny(virConnectPtr conn, vboxDriverUnlock(data); + if (cnt >= 0) + ret = 0; + return ret; } diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index b5d6738..2c7aeaa 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -2312,7 +2312,7 @@ xenUnifiedConnectDomainEventRegister(virConnectPtr conn, virFreeCallback freefunc) { xenUnifiedPrivatePtr priv = conn->privateData; - int ret; + int ret = 0; if (virConnectDomainEventRegisterEnsureACL(conn) < 0) return -1; @@ -2325,8 +2325,9 @@ xenUnifiedConnectDomainEventRegister(virConnectPtr conn, return -1; } - ret = virDomainEventStateRegister(conn, priv->domainEvents, - callback, opaque, freefunc); + if (virDomainEventStateRegister(conn, priv->domainEvents, + callback, opaque, freefunc) < 0) + ret = -1; xenUnifiedUnlock(priv); return ret; @@ -2337,7 +2338,7 @@ static int xenUnifiedConnectDomainEventDeregister(virConnectPtr conn, virConnectDomainEventCallback callback) { - int ret; + int ret = 0; xenUnifiedPrivatePtr priv = conn->privateData; if (virConnectDomainEventDeregisterEnsureACL(conn) < 0) @@ -2351,9 +2352,10 @@ xenUnifiedConnectDomainEventDeregister(virConnectPtr conn, return -1; } - ret = virDomainEventStateDeregister(conn, - priv->domainEvents, - callback); + if (virDomainEventStateDeregister(conn, + priv->domainEvents, + callback) < 0) + ret = -1; xenUnifiedUnlock(priv); return ret; @@ -2395,7 +2397,7 @@ static int xenUnifiedConnectDomainEventDeregisterAny(virConnectPtr conn, int callbackID) { - int ret; + int ret = 0; xenUnifiedPrivatePtr priv = conn->privateData; if (virConnectDomainEventDeregisterAnyEnsureACL(conn) < 0) @@ -2409,9 +2411,10 @@ xenUnifiedConnectDomainEventDeregisterAny(virConnectPtr conn, return -1; } - ret = virObjectEventStateDeregisterID(conn, - priv->domainEvents, - callbackID); + if (virObjectEventStateDeregisterID(conn, + priv->domainEvents, + callbackID) < 0) + ret = -1; xenUnifiedUnlock(priv); return ret; -- 1.8.4.2

Prior to this patch, every test:/// URI has its own event manager, which means that registering for an event can only ever receive events from the connection where it issued the API that triggered the event. But the whole idea of events is to be able to learn about something where an API call did NOT trigger the action. In order to actually test asynchronous events, I wanted to be able to tie multiple test connections to the same state. Use of a file in a test URI is still per-connection state, but now parallel connections to test:///default (from the same binary, of course) now share common state and can affect one another. * src/test/test_driver.c (testConnectOpen): Move per-connection state initialization... (testOpenFromFile): ...here. (defaultConn, defaultConnections, defaultLock, testOnceInit): New shared state. (testOpenDefault): Only initialize on first connection. (testConnectClose): Don't clobber state if still shared. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/test/test_driver.c | 86 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 63 insertions(+), 23 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index d0f3fa8..b8bd6f4 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -105,6 +105,10 @@ struct _testConn { typedef struct _testConn testConn; typedef struct _testConn *testConnPtr; +static testConn defaultConn; +static int defaultConnections; +static virMutex defaultLock; + #define TEST_MODEL "i686" #define TEST_MODEL_WORDSIZE 32 #define TEST_EMULATOR "/usr/bin/test-hv" @@ -125,6 +129,14 @@ static int testConnectClose(virConnectPtr conn); static void testObjectEventQueue(testConnPtr driver, virObjectEventPtr event); +static int +testOnceInit(void) +{ + return virMutexInit(&defaultLock); +} + +VIR_ONCE_GLOBAL_INIT(test) + static void testDriverLock(testConnPtr driver) { @@ -665,9 +677,15 @@ cleanup: return ret; } -static int testOpenDefault(virConnectPtr conn) { + +/* Simultaneous test:///default connections should share the same + * common state (among other things, this allows testing event + * detection in one connection for an action caused in another). */ +static int +testOpenDefault(virConnectPtr conn) +{ int u; - testConnPtr privconn; + testConnPtr privconn = &defaultConn; virDomainDefPtr domdef = NULL; virDomainObjPtr domobj = NULL; virNetworkDefPtr netdef = NULL; @@ -679,18 +697,26 @@ static int testOpenDefault(virConnectPtr conn) { virNodeDeviceDefPtr nodedef = NULL; virNodeDeviceObjPtr nodeobj = NULL; - if (VIR_ALLOC(privconn) < 0) - return VIR_DRV_OPEN_ERROR; + virMutexLock(&defaultLock); + if (defaultConnections++) { + conn->privateData = &defaultConn; + virMutexUnlock(&defaultLock); + return VIR_DRV_OPEN_SUCCESS; + } + if (virMutexInit(&privconn->lock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize mutex")); - VIR_FREE(privconn); + defaultConnections--; + virMutexUnlock(&defaultLock); return VIR_DRV_OPEN_ERROR; } - testDriverLock(privconn); conn->privateData = privconn; + if (!(privconn->domainEventState = virObjectEventStateNew())) + goto error; + if (!(privconn->domains = virDomainObjListNew())) goto error; @@ -791,7 +817,7 @@ static int testOpenDefault(virConnectPtr conn) { } virNodeDeviceObjUnlock(nodeobj); - testDriverUnlock(privconn); + virMutexUnlock(&defaultLock); return VIR_DRV_OPEN_SUCCESS; @@ -802,10 +828,12 @@ error: virStoragePoolObjListFree(&privconn->pools); virNodeDeviceObjListFree(&privconn->devs); virObjectUnref(privconn->caps); - testDriverUnlock(privconn); + virObjectEventStateFree(privconn->domainEventState); + virMutexDestroy(&privconn->lock); conn->privateData = NULL; - VIR_FREE(privconn); virDomainDefFree(domdef); + defaultConnections--; + virMutexUnlock(&defaultLock); return VIR_DRV_OPEN_ERROR; } @@ -1327,6 +1355,9 @@ error: return ret; } + +/* No shared state between simultaneous test connections initialized + * from a file. */ static int testOpenFromFile(virConnectPtr conn, const char *file) { @@ -1355,6 +1386,9 @@ testOpenFromFile(virConnectPtr conn, const char *file) if (!(privconn->xmlopt = testBuildXMLConfig())) goto error; + if (!(privconn->domainEventState = virObjectEventStateNew())) + goto error; + if (!(doc = virXMLParseFileCtxt(file, &ctxt))) { goto error; } @@ -1398,6 +1432,7 @@ testOpenFromFile(virConnectPtr conn, const char *file) virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); VIR_FREE(privconn->path); + virObjectEventStateFree(privconn->domainEventState); testDriverUnlock(privconn); VIR_FREE(privconn); conn->privateData = NULL; @@ -1410,10 +1445,12 @@ static virDrvOpenStatus testConnectOpen(virConnectPtr conn, unsigned int flags) { int ret; - testConnPtr privconn; virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + if (testInitialize() < 0) + return VIR_DRV_OPEN_ERROR; + if (!conn->uri) return VIR_DRV_OPEN_DECLINED; @@ -1442,24 +1479,24 @@ static virDrvOpenStatus testConnectOpen(virConnectPtr conn, if (ret != VIR_DRV_OPEN_SUCCESS) return ret; - privconn = conn->privateData; - testDriverLock(privconn); - - privconn->domainEventState = virObjectEventStateNew(); - if (!privconn->domainEventState) { - testDriverUnlock(privconn); - testConnectClose(conn); - return VIR_DRV_OPEN_ERROR; - } - - testDriverUnlock(privconn); - return VIR_DRV_OPEN_SUCCESS; } static int testConnectClose(virConnectPtr conn) { testConnPtr privconn = conn->privateData; + + if (testInitialize() < 0) + return -1; + + if (privconn == &defaultConn) { + virMutexLock(&defaultLock); + if (--defaultConnections) { + virMutexUnlock(&defaultLock); + return 0; + } + } + testDriverLock(privconn); virObjectUnref(privconn->caps); virObjectUnref(privconn->xmlopt); @@ -1474,7 +1511,10 @@ static int testConnectClose(virConnectPtr conn) testDriverUnlock(privconn); virMutexDestroy(&privconn->lock); - VIR_FREE(privconn); + if (privconn == &defaultConn) + virMutexUnlock(&defaultLock); + else + VIR_FREE(privconn); conn->privateData = NULL; return 0; } -- 1.8.4.2

Test the previous patch. * tests/objecteventtest.c (testDomainCreateXMLMixed): New test. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tests/objecteventtest.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 67 insertions(+), 2 deletions(-) diff --git a/tests/objecteventtest.c b/tests/objecteventtest.c index 6f657d2..649fc25 100644 --- a/tests/objecteventtest.c +++ b/tests/objecteventtest.c @@ -167,7 +167,7 @@ cleanup: } static int -testDomainCreateXML(const void *data) +testDomainCreateXMLNew(const void *data) { const objecteventTest *test = data; lifecycleEventCounter counter; @@ -208,6 +208,68 @@ cleanup: } static int +testDomainCreateXMLMixed(const void *data) +{ + const objecteventTest *test = data; + lifecycleEventCounter counter; + virDomainPtr dom; + int ret = -1; + int id = -1; + bool registered = false; + + lifecycleEventCounter_reset(&counter); + + /* Fun with mixing old and new API. Handler should be fired twice, + * once for each registration. */ + dom = virDomainCreateXML(test->conn, domainDef, 0); + if (dom == NULL) + goto cleanup; + + id = virConnectDomainEventRegisterAny(test->conn, dom, + VIR_DOMAIN_EVENT_ID_LIFECYCLE, + VIR_DOMAIN_EVENT_CALLBACK(&domainLifecycleCb), + &counter, NULL); + if (id < 0) + goto cleanup; + if (virDomainDestroy(dom) < 0) + goto cleanup; + if (virConnectDomainEventRegister(test->conn, + domainLifecycleCb, + &counter, NULL) != 0) + goto cleanup; + registered = true; + + dom = virDomainCreateXML(test->conn, domainDef, 0); + if (dom == NULL || virEventRunDefaultImpl() < 0) + goto cleanup; + + if (counter.startEvents != 2 || counter.unexpectedEvents > 0) + goto cleanup; + + if (virConnectDomainEventDeregister(test->conn, domainLifecycleCb) != 0) + goto cleanup; + registered = false; + if (virConnectDomainEventDeregisterAny(test->conn, id) != 0) + goto cleanup; + id = -1; + ret = 0; + +cleanup: + if (id >= 0) + virConnectDomainEventDeregisterAny(test->conn, id); + if (registered) + virConnectDomainEventDeregister(test->conn, domainLifecycleCb); + if (dom != NULL) { + virDomainUndefine(dom); + virDomainDestroy(dom); + virDomainFree(dom); + } + + return ret; +} + + +static int testDomainDefine(const void *data) { const objecteventTest *test = data; @@ -471,7 +533,10 @@ mymain(void) testDomainCreateXMLOld, &test) < 0) ret = EXIT_FAILURE; if (virtTestRun("Domain createXML start event (new API)", - testDomainCreateXML, &test) < 0) + testDomainCreateXMLNew, &test) < 0) + ret = EXIT_FAILURE; + if (virtTestRun("Domain createXML start event (both API)", + testDomainCreateXMLMixed, &test) < 0) ret = EXIT_FAILURE; if (virtTestRun("Domain (un)define events", testDomainDefine, &test) < 0) ret = EXIT_FAILURE; -- 1.8.4.2

On the surface, this sequence of API calls should succeed: id1 = virConnectDomainEventRegisterAny(..., VIR_DOMAIN_EVENT_ID_LIFECYCLE,...); id2 = virConnectDomainEventRegisterAny(..., VIR_DOMAIN_EVENT_ID_RTC_CHANGE,...); virConnectDomainEventDeregisterAny(id1); id1 = virConnectDomainEventRegisterAny(..., VIR_DOMAIN_EVENT_ID_LIFECYCLE,...); And for test:///default, it does. But for qemu:///system, it fails: libvirt: XML-RPC error : internal error: domain event 0 already registered Looking closer, the bug is caused by miscommunication between the object event engine and the client side of the remote driver. In our implementation, we set up a single server-side event per eventID, then the client side replicates that one event to all callbacks that have been registered client side. To know when to turn the server side eventID on or off, the client side must track how many events for the same eventID have been registered. But while our code was filtering by eventID on event registration, it did not filter on event deregistration. So the above API calls resulted in the deregister returning 1 instead of 0, so no RPC deregister was issued, and the final register detects on the server side that the server is already handling eventID 0. Unfortunately, since the problem is only observable on remote connections, it's not possible to enhance objecteventtest to expose the semantics using only public API entry points. * src/conf/object_event.c (virObjectEventCallbackListCount): New function. (virObjectEventCallbackListAddID) (virObjectEventCallbackListRemoveID) (virObjectEventCallbackListMarkDeleteID): Use it. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/conf/object_event.c | 68 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 45 insertions(+), 23 deletions(-) diff --git a/src/conf/object_event.c b/src/conf/object_event.c index 9293ab1..bb7f935 100644 --- a/src/conf/object_event.c +++ b/src/conf/object_event.c @@ -141,6 +141,39 @@ virObjectEventCallbackListFree(virObjectEventCallbackListPtr list) /** + * virObjectEventCallbackListCount: + * @conn: pointer to the connection + * @cbList: the list + * @klass: the base event class + * @eventID: the event ID + * + * Internal function to count how many callbacks remain registered for + * the given @eventID; knowing this allows the client side of the + * remote driver know when it must send an RPC to adjust the callbacks + * on the server. + */ +static int +virObjectEventCallbackListCount(virConnectPtr conn, + virObjectEventCallbackListPtr cbList, + virClassPtr klass, + int eventID) +{ + size_t i; + int ret = 0; + + for (i = 0; i < cbList->count; i++) { + virObjectEventCallbackPtr cb = cbList->callbacks[i]; + + if (cb->klass == klass && + cb->eventID == eventID && + cb->conn == conn && + !cb->deleted) + ret++; + } + return ret; +} + +/** * virObjectEventCallbackListRemoveID: * @conn: pointer to the connection * @cbList: the list @@ -153,13 +186,15 @@ virObjectEventCallbackListRemoveID(virConnectPtr conn, virObjectEventCallbackListPtr cbList, int callbackID) { - int ret = 0; size_t i; for (i = 0; i < cbList->count; i++) { virObjectEventCallbackPtr cb = cbList->callbacks[i]; if (cb->callbackID == callbackID && cb->conn == conn) { + virClassPtr klass = cb->klass; + int eventID = cb->eventID; + if (cb->freecb) (*cb->freecb)(cb->opaque); virObjectUnref(cb->conn); @@ -169,11 +204,8 @@ virObjectEventCallbackListRemoveID(virConnectPtr conn, VIR_FREE(cb); VIR_DELETE_ELEMENT(cbList->callbacks, i, cbList->count); - for (i = 0; i < cbList->count; i++) { - if (!cbList->callbacks[i]->deleted) - ret++; - } - return ret; + return virObjectEventCallbackListCount(conn, cbList, klass, + eventID); } } @@ -188,18 +220,15 @@ virObjectEventCallbackListMarkDeleteID(virConnectPtr conn, virObjectEventCallbackListPtr cbList, int callbackID) { - int ret = 0; size_t i; for (i = 0; i < cbList->count; i++) { - if (cbList->callbacks[i]->callbackID == callbackID && - cbList->callbacks[i]->conn == conn) { - cbList->callbacks[i]->deleted = true; - for (i = 0; i < cbList->count; i++) { - if (!cbList->callbacks[i]->deleted) - ret++; - } - return ret; + virObjectEventCallbackPtr cb = cbList->callbacks[i]; + + if (cb->callbackID == callbackID && cb->conn == conn) { + cb->deleted = true; + return virObjectEventCallbackListCount(conn, cbList, cb->klass, + cb->eventID); } } @@ -315,7 +344,6 @@ virObjectEventCallbackListAddID(virConnectPtr conn, int *callbackID) { virObjectEventCallbackPtr event; - size_t i; int ret = -1; VIR_DEBUG("conn=%p cblist=%p uuid=%p name=%s id=%d " @@ -361,13 +389,7 @@ virObjectEventCallbackListAddID(virConnectPtr conn, if (VIR_APPEND_ELEMENT(cbList->callbacks, cbList->count, event) < 0) goto cleanup; - for (ret = 0, i = 0; i < cbList->count; i++) { - if (cbList->callbacks[i]->klass == klass && - cbList->callbacks[i]->eventID == eventID && - cbList->callbacks[i]->conn == conn && - !cbList->callbacks[i]->deleted) - ret++; - } + ret = virObjectEventCallbackListCount(conn, cbList, klass, eventID); cleanup: if (event) { -- 1.8.4.2

Consider these two calls, in either order: id1 = virConnectDomainEventRegisterAny(conn, NULL, VIR_DOMAIN_EVENT_ID_LIFECYCLE, VIR_DOMAIN_EVENT_CALLBACK(callback), NULL, NULL); virConnectDomainEventRegister(conn, callback, NULL, NULL); Right now, the second call fails, because under the hood, the old-style function registration is tightly coupled to the new style lifecycle eventID, and the two calls both try to register the same global eventID callback representation. We've alreay documented that users should avoid old-style registration and deregistration, so anyone heeding the advice won't run into this situation. But it would be even nicer if we pretend the two interfaces are completely separate, and disallow any cross-linking. That is, a call to old-style deregister should never remove a new-style callback even if it is the same function pointer, and a call to new-style callback using only callbackIDs obtained legitimately should never remove an old-style callback (of course, since our callback IDs are sequential, and there is still coupling under the hood, you can easily guess the callbackID of an old style registration and use new-style deregistration to nuke it - but that starts to be blatantly bad coding on your part rather than a surprising result on what looks like reasonable stand-alone API). With this patch, you can now register a global lifecycle event handler twice, by using both old and new APIs; if such an event occurs, your callback will be entered twice. But that is not a problem in practice, since it is already possible to use the new API to register both a global and per-domain event handler using the same function, which will likewise fire your callback twice for that domain. Duplicates are still prevented when using the same API with same parameters twice (old-style twice, new-style global twice, or new-style per-domain with same domain twice), and things are still bounded (it is not possible to register a single function pointer more than N+2 times per event id, where N is the number of domains available on the connection). Besides, it has always been possible to register as many separate function pointers on the same event id as desired, through either old or new style API, where the bound there is the physical limitation of writing a program with enough distinct function pointers. * src/conf/object_event.c (_virObjectEventCallback): Add field. (virObjectEventCallbackLookup): Add argument. (virObjectEventCallbackListAddID, virObjectEventStateCallbackID): Adjust callers. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/conf/object_event.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/conf/object_event.c b/src/conf/object_event.c index bb7f935..87e10e0 100644 --- a/src/conf/object_event.c +++ b/src/conf/object_event.c @@ -71,6 +71,7 @@ struct _virObjectEventCallback { void *opaque; virFreeCallback freecb; bool deleted; + bool legacy; /* true if end user does not know callbackID */ }; static virClassPtr virObjectEventClass; @@ -150,7 +151,9 @@ virObjectEventCallbackListFree(virObjectEventCallbackListPtr list) * Internal function to count how many callbacks remain registered for * the given @eventID; knowing this allows the client side of the * remote driver know when it must send an RPC to adjust the callbacks - * on the server. + * on the server. Note that this function intentionally ignores + * the legacy field, since RPC calls use only a single callback on + * the server to manage both legacy and modern domain lifecycle events. */ static int virObjectEventCallbackListCount(virConnectPtr conn, @@ -278,6 +281,7 @@ virObjectEventCallbackListPurgeMarked(virObjectEventCallbackListPtr cbList) * @klass: the base event class * @eventID: the event ID * @callback: the callback to locate + * @legacy: true if callback is tracked by function instead of callbackID * * Internal function to determine if @callback already has a * callbackID in @cbList for the given @conn and other filters. @@ -289,7 +293,8 @@ virObjectEventCallbackLookup(virConnectPtr conn, unsigned char uuid[VIR_UUID_BUFLEN], virClassPtr klass, int eventID, - virConnectObjectEventGenericCallback callback) + virConnectObjectEventGenericCallback callback, + bool legacy) { int ret = -1; size_t i; @@ -303,6 +308,7 @@ virObjectEventCallbackLookup(virConnectPtr conn, cb->klass == klass && cb->eventID == eventID && cb->conn == conn && + cb->legacy == legacy && ((uuid && cb->meta && memcmp(cb->meta->uuid, uuid, VIR_UUID_BUFLEN) == 0) || (!uuid && !cb->meta))) { @@ -358,7 +364,8 @@ virObjectEventCallbackListAddID(virConnectPtr conn, /* check if we already have this callback on our list */ if (virObjectEventCallbackLookup(conn, cbList, uuid, - klass, eventID, callback) != -1) { + klass, eventID, callback, + !callbackID) != -1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("event callback already tracked")); return -1; @@ -385,6 +392,8 @@ virObjectEventCallbackListAddID(virConnectPtr conn, if (callbackID) *callbackID = event->callbackID; + else + event->legacy = true; if (VIR_APPEND_ELEMENT(cbList->callbacks, cbList->count, event) < 0) goto cleanup; @@ -887,7 +896,9 @@ virObjectEventStateDeregisterID(virConnectPtr conn, * @callback: function registered as a callback * * Returns the callbackID of @callback, or -1 with an error issued if the - * function is not currently registered. + * function is not currently registered. This only finds functions + * registered via virConnectDomainEventRegister, even if modern style + * virConnectDomainEventRegisterAny also registers the same callback. */ int virObjectEventStateCallbackID(virConnectPtr conn, @@ -900,7 +911,7 @@ virObjectEventStateCallbackID(virConnectPtr conn, virObjectEventStateLock(state); ret = virObjectEventCallbackLookup(conn, state->callbacks, NULL, - klass, eventID, callback); + klass, eventID, callback, true); virObjectEventStateUnlock(state); if (ret < 0) -- 1.8.4.2

If a user registers for a domain event filtered to a particular domain, but the persistent domain is offline at the time, then the code silently failed to set up the filter. As a result, the event fires for all domains, rather than being filtered. Network events were immune, since they always passed an id 0 argument. The key to this patch is realizing that virObjectEventDispatchMatchCallback() only cared about uuid; so refusing to create a meta for a negative id is pointless, and in fact, malloc'ing meta at all was overkill; instead, just directly store a uuid and a flag of whether to filter. * src/conf/object_event_private.h (_virObjectEventCallback): Replace meta with uuid and flag. (virObjectEventCallbackListAddID): Update signature. * src/conf/object_event.h (virObjectEventStateRegisterID): Likewise. * src/conf/object_event.c (virObjectEventCallbackListAddID): Drop arguments that don't affect filtering. (virObjectEventCallbackListRemoveID) (virObjectEventDispatchMatchCallback) (virObjectEventStateRegisterID): Update clients. * src/conf/domain_event.c (virDomainEventCallbackListAdd) (virDomainEventStateRegisterID): Likewise. * src/conf/network_event.c (virNetworkEventStateRegisterID): Likewise. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/conf/domain_event.c | 16 +++++----------- src/conf/network_event.c | 13 +++---------- src/conf/object_event.c | 48 +++++++++++++++++------------------------------- src/conf/object_event.h | 5 ++--- 4 files changed, 27 insertions(+), 55 deletions(-) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 4f8ede5..35212ef 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -1284,7 +1284,7 @@ virDomainEventStateRegister(virConnectPtr conn, if (virDomainEventsInitialize() < 0) return -1; - return virObjectEventStateRegisterID(conn, state, NULL, NULL, 0, + return virObjectEventStateRegisterID(conn, state, NULL, virDomainEventClass, VIR_DOMAIN_EVENT_ID_LIFECYCLE, VIR_OBJECT_EVENT_CALLBACK(callback), @@ -1322,16 +1322,10 @@ virDomainEventStateRegisterID(virConnectPtr conn, if (virDomainEventsInitialize() < 0) return -1; - if (dom) - return virObjectEventStateRegisterID(conn, state, dom->uuid, dom->name, - dom->id, virDomainEventClass, eventID, - VIR_OBJECT_EVENT_CALLBACK(cb), - opaque, freecb, callbackID); - else - return virObjectEventStateRegisterID(conn, state, NULL, NULL, 0, - virDomainEventClass, eventID, - VIR_OBJECT_EVENT_CALLBACK(cb), - opaque, freecb, callbackID); + return virObjectEventStateRegisterID(conn, state, dom ? dom->uuid : NULL, + virDomainEventClass, eventID, + VIR_OBJECT_EVENT_CALLBACK(cb), + opaque, freecb, callbackID); } diff --git a/src/conf/network_event.c b/src/conf/network_event.c index a192ffe..38c74d1 100644 --- a/src/conf/network_event.c +++ b/src/conf/network_event.c @@ -151,16 +151,9 @@ virNetworkEventStateRegisterID(virConnectPtr conn, if (virNetworkEventsInitialize() < 0) return -1; - if (net) - return virObjectEventStateRegisterID(conn, state, - net->uuid, net->name, 0, - virNetworkEventClass, eventID, - cb, opaque, freecb, callbackID); - else - return virObjectEventStateRegisterID(conn, state, - NULL, NULL, 0, - virNetworkEventClass, eventID, - cb, opaque, freecb, callbackID); + return virObjectEventStateRegisterID(conn, state, net ? net->uuid : NULL, + virNetworkEventClass, eventID, + cb, opaque, freecb, callbackID); } diff --git a/src/conf/object_event.c b/src/conf/object_event.c index 87e10e0..320a422 100644 --- a/src/conf/object_event.c +++ b/src/conf/object_event.c @@ -66,7 +66,8 @@ struct _virObjectEventCallback { virClassPtr klass; int eventID; virConnectPtr conn; - virObjectMetaPtr meta; + bool uuid_filter; + unsigned char uuid[VIR_UUID_BUFLEN]; virConnectObjectEventGenericCallback cb; void *opaque; virFreeCallback freecb; @@ -201,9 +202,6 @@ virObjectEventCallbackListRemoveID(virConnectPtr conn, if (cb->freecb) (*cb->freecb)(cb->opaque); virObjectUnref(cb->conn); - if (cb->meta) - VIR_FREE(cb->meta->name); - VIR_FREE(cb->meta); VIR_FREE(cb); VIR_DELETE_ELEMENT(cbList->callbacks, i, cbList->count); @@ -309,9 +307,9 @@ virObjectEventCallbackLookup(virConnectPtr conn, cb->eventID == eventID && cb->conn == conn && cb->legacy == legacy && - ((uuid && cb->meta && - memcmp(cb->meta->uuid, uuid, VIR_UUID_BUFLEN) == 0) || - (!uuid && !cb->meta))) { + ((uuid && cb->uuid_filter && + memcmp(cb->uuid, uuid, VIR_UUID_BUFLEN) == 0) || + (!uuid && !cb->uuid_filter))) { ret = cb->callbackID; break; } @@ -325,8 +323,6 @@ virObjectEventCallbackLookup(virConnectPtr conn, * @conn: pointer to the connection * @cbList: the list * @uuid: the uuid of the object to filter on - * @name: the name of the object to filter on - * @id: the ID of the object to filter on * @klass: the base event class * @eventID: the event ID * @callback: the callback to add @@ -340,8 +336,6 @@ static int virObjectEventCallbackListAddID(virConnectPtr conn, virObjectEventCallbackListPtr cbList, unsigned char uuid[VIR_UUID_BUFLEN], - const char *name, - int id, virClassPtr klass, int eventID, virConnectObjectEventGenericCallback callback, @@ -352,10 +346,9 @@ virObjectEventCallbackListAddID(virConnectPtr conn, virObjectEventCallbackPtr event; int ret = -1; - VIR_DEBUG("conn=%p cblist=%p uuid=%p name=%s id=%d " + VIR_DEBUG("conn=%p cblist=%p uuid=%p " "klass=%p eventID=%d callback=%p opaque=%p", - conn, cbList, uuid, name, id, klass, eventID, - callback, opaque); + conn, cbList, uuid, klass, eventID, callback, opaque); /* Check incoming */ if (!cbList) { @@ -381,13 +374,12 @@ virObjectEventCallbackListAddID(virConnectPtr conn, event->opaque = opaque; event->freecb = freecb; - if (name && uuid && id > 0) { - if (VIR_ALLOC(event->meta) < 0) - goto cleanup; - if (VIR_STRDUP(event->meta->name, name) < 0) - goto cleanup; - memcpy(event->meta->uuid, uuid, VIR_UUID_BUFLEN); - event->meta->id = id; + /* Only need 'uuid' for matching; 'id' can change as domain + * switches between running and shutoff, and 'name' can change in + * Xen migration. */ + if (uuid) { + event->uuid_filter = true; + memcpy(event->uuid, uuid, VIR_UUID_BUFLEN); } if (callbackID) @@ -401,12 +393,8 @@ virObjectEventCallbackListAddID(virConnectPtr conn, ret = virObjectEventCallbackListCount(conn, cbList, klass, eventID); cleanup: - if (event) { + if (event) virObjectUnref(event->conn); - if (event->meta) - VIR_FREE(event->meta->name); - VIR_FREE(event->meta); - } VIR_FREE(event); return ret; } @@ -669,14 +657,14 @@ virObjectEventDispatchMatchCallback(virObjectEventPtr event, if (cb->eventID != event->eventID) return false; - if (cb->meta) { + if (cb->uuid_filter) { /* Deliberately ignoring 'id' for matching, since that * will cause problems when a domain switches between * running & shutoff states & ignoring 'name' since * Xen sometimes renames guests during migration, thus * leaving 'uuid' as the only truly reliable ID we can use. */ - return memcmp(event->meta.uuid, cb->meta->uuid, VIR_UUID_BUFLEN) == 0; + return memcmp(event->meta.uuid, cb->uuid, VIR_UUID_BUFLEN) == 0; } return true; } @@ -807,8 +795,6 @@ int virObjectEventStateRegisterID(virConnectPtr conn, virObjectEventStatePtr state, unsigned char *uuid, - const char *name, - int id, virClassPtr klass, int eventID, virConnectObjectEventGenericCallback cb, @@ -832,7 +818,7 @@ virObjectEventStateRegisterID(virConnectPtr conn, } ret = virObjectEventCallbackListAddID(conn, state->callbacks, - uuid, name, id, klass, eventID, + uuid, klass, eventID, cb, opaque, freecb, callbackID); diff --git a/src/conf/object_event.h b/src/conf/object_event.h index fa1281c..39e92d5 100644 --- a/src/conf/object_event.h +++ b/src/conf/object_event.h @@ -71,15 +71,14 @@ int virObjectEventStateRegisterID(virConnectPtr conn, virObjectEventStatePtr state, unsigned char *uuid, - const char *name, - int id, virClassPtr klass, int eventID, virConnectObjectEventGenericCallback cb, void *opaque, virFreeCallback freecb, int *callbackID) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(8); + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) + ATTRIBUTE_NONNULL(6); int virObjectEventStateDeregisterID(virConnectPtr conn, virObjectEventStatePtr state, -- 1.8.4.2

On 01/07/2014 10:07 AM, John Ferlan wrote: Will do. I actually caught that in patch 7 in time (but managed to botch the testsuite in the process of amending that patch). virObjectEvent DOES care about id and name - the dispatcher function has to reconstruct the virDomainPtr for handing to the callback function (see virDomainEventDispatchDefaultFunc in domain_event.c). So meta is still important for events, just not for callbacks. But if you found it confusing, then it can't hurt for me to enhance the commit message :) The test is covering whether an event happens when a domain is created (whether created as transient, or changed from offline to online for persistent); but to register an event at all, the domain already has to exist. When I first wrote the entire function for this patch, I used 'define' to make the domain exist, then register events, then 'create' to see the creation event; but it turned up other bugs, which I then rebased and fixed first, using the portions of the test that worked to expose those problems. But when rebasing, I had to first test create as transient, register, then destroy, then re-create, to avoid tripping the bug fixed in this patch. I added more commentary in my commit message :) Okay, here's what I squashed in when pushing. diff --git i/src/conf/object_event.c w/src/conf/object_event.c index c4aedd9..b01ffe5 100644 --- i/src/conf/object_event.c +++ w/src/conf/object_event.c @@ -774,8 +774,6 @@ virObjectEventStateFlush(virObjectEventStatePtr state) * @conn: connection to associate with callback * @state: domain event state * @uuid: uuid of the object for event filtering - * @name: name of the object for event filtering - * @id: id of the object for event filtering, or 0 * @klass: the base event class * @eventID: ID of the event type to register for * @cb: function to invoke when event occurs diff --git i/src/conf/object_event_private.h w/src/conf/object_event_private.h index 76bd6d1..571fc40 100644 --- i/src/conf/object_event_private.h +++ w/src/conf/object_event_private.h @@ -69,6 +69,8 @@ virObjectEventNew(virClassPtr klass, int eventID, int id, const char *name, - const unsigned char *uuid); + const unsigned char *uuid) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5) + ATTRIBUTE_NONNULL(6); #endif -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org