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). 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 local drivers; that way, 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@redhat.com> --- src/libvirt.c | 17 +++++++++++------ src/libxl/libxl_driver.c | 35 ++++++++++++++++++----------------- src/lxc/lxc_driver.c | 32 ++++++++++++++++---------------- 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 | 27 +++++++++++++++------------ 8 files changed, 126 insertions(+), 95 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index a0a26e5..f527dcc 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -16363,7 +16363,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, @@ -16401,14 +16403,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, @@ -19443,8 +19447,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 77b25d4..e0e3fd4 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * Copyright IBM Corp. 2008 * * lxc_driver.c: linux container driver functions @@ -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 4103dc9..f563837 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1,7 +1,7 @@ /* * xen_driver.c: Unified Xen driver. * - * Copyright (C) 2007-2013 Red Hat, Inc. + * Copyright (C) 2007-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -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

Ensure our testsuite has some coverage of old-style domain event registration. * tests/objecteventtest.c (testDomainCreateXMLOld): New test. (mymain): Run it. (testDomainCreateXML): Check return values. Signed-off-by: Eric Blake <eblake@redhat.com> --- I verified that this test fails until you also apply patch 2/2 "event: make deregister return value match docs". Should I squash them together or keep it as separate commits? tests/objecteventtest.c | 67 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 58 insertions(+), 9 deletions(-) diff --git a/tests/objecteventtest.c b/tests/objecteventtest.c index ae29792..f7b60cd 100644 --- a/tests/objecteventtest.c +++ b/tests/objecteventtest.c @@ -1,4 +1,5 @@ /* + * Copyright (C) 2014 Red Hat, Inc. * Copyright (C) 2013 SUSE LINUX Products GmbH, Nuernberg, Germany. * * This library is free software; you can redistribute it and/or @@ -126,6 +127,46 @@ networkLifecycleCb(virConnectPtr conn ATTRIBUTE_UNUSED, static int +testDomainCreateXMLOld(const void *data) +{ + const objecteventTest *test = data; + lifecycleEventCounter counter; + virDomainPtr dom; + int ret = -1; + bool registered = false; + + lifecycleEventCounter_reset(&counter); + + 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 != 1 || counter.unexpectedEvents > 0) + goto cleanup; + + if (virConnectDomainEventDeregister(test->conn, domainLifecycleCb) != 0) + goto cleanup; + registered = false; + ret = 0; + +cleanup: + if (registered) + virConnectDomainEventDeregister(test->conn, domainLifecycleCb); + if (dom != NULL) { + virDomainDestroy(dom); + virDomainFree(dom); + } + + return ret; +} + +static int testDomainCreateXML(const void *data) { const objecteventTest *test = data; @@ -133,27 +174,31 @@ testDomainCreateXML(const void *data) int eventId = VIR_DOMAIN_EVENT_ID_LIFECYCLE; virDomainPtr dom; int id; - int ret = 0; + int ret = -1; lifecycleEventCounter_reset(&counter); id = virConnectDomainEventRegisterAny(test->conn, NULL, eventId, VIR_DOMAIN_EVENT_CALLBACK(&domainLifecycleCb), &counter, NULL); + if (id < 0) + goto cleanup; dom = virDomainCreateXML(test->conn, domainDef, 0); - if (dom == NULL || virEventRunDefaultImpl() < 0) { - ret = -1; + if (dom == NULL || virEventRunDefaultImpl() < 0) goto cleanup; - } - if (counter.startEvents != 1 || counter.unexpectedEvents > 0) { - ret = -1; + if (counter.startEvents != 1 || counter.unexpectedEvents > 0) goto cleanup; - } + + if (virConnectDomainEventDeregisterAny(test->conn, id) != 0) + goto cleanup; + id = -1; + ret = 0; cleanup: - virConnectDomainEventDeregisterAny(test->conn, id); + if (id >= 0) + virConnectDomainEventDeregisterAny(test->conn, id); if (dom != NULL) { virDomainDestroy(dom); virDomainFree(dom); @@ -388,7 +433,11 @@ mymain(void) virtTestQuiesceLibvirtErrors(false); /* Domain event tests */ - if (virtTestRun("Domain createXML start event ", testDomainCreateXML, &test) < 0) + if (virtTestRun("Domain createXML start event (old API)", + testDomainCreateXMLOld, &test) < 0) + ret = EXIT_FAILURE; + if (virtTestRun("Domain createXML start event (new API)", + testDomainCreateXML, &test) < 0) ret = EXIT_FAILURE; if (virtTestRun("Domain (un)define events", testDomainDefine, &test) < 0) ret = EXIT_FAILURE; -- 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@redhat.com> --- 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

Right now, the older virConnectDomainEventRegister (takes a function pointer, returns 0 on success) and the newer virConnectDomainEventRegisterID (takes an eventID, returns a callbackID) share the underlying implementation (the older API ends up consuming a callbackID for eventID 0 under the hood). We implemented that by a lot of copy and pasted code between object_event.c and domain_event.c, according to whether we are dealing with a function pointer or an eventID. However, our copy and paste is not symmetric. Consider this sequence: id1 = virConnectDomainEventRegisterAny(conn, dom, VIR_DOMAIN_EVENT_ID_LIFECYCLE, VIR_DOMAIN_EVENT_CALLBACK(callback), NULL, NULL); virConnectDomainEventRegister(conn, callback, NULL, NULL); virConnectDomainEventDeregister(conn, callback); virConnectDomainEventDeregsiterAny(conn, id1); the first three calls would succeed, but the third call ended up nuking the id1 callbackID (the per-domain new-style handler), then the fourth call failed with an error about an unknown callbackID, leaving us with the global handler (old-style) still live and receiving events. It required another old-style deregister to clean up the mess. Root cause was that virDomainEventCallbackList{Remove,MarkDelete} were only checking for function pointer match, rather than also checking for whether the registration was global. Rather than playing with the guts of object_event ourselves in domain_event, it is nicer to add a mapping function for the internal callback id, then share common code for event removal. For now, the function-to-id mapping is used only internally; I thought about whether a new public API to let a user learn the callback would be useful, but decided exposing this to the user is probably a disservice, since we already publicly document that they should avoid the old style, and since this patch already demonstrates that older libvirt versions have weird behavior when mixing old and new styles. * src/conf/object_event.c (virObjectEventCallbackLookup) (virObjectEventStateCallbackID): New functions. (virObjectEventCallbackLookup): Use helper function. * src/conf/object_event_private.h (virObjectEventStateCallbackID): Declare new function. * src/conf/domain_event.c (virDomainEventStateRegister) (virDomainEventStateDeregister): Let common code handle the complexity. (virDomainEventCallbackListRemove) (virDomainEventCallbackListMarkDelete) (virDomainEventCallbackListAdd): Drop unused functions. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_event.c | 171 ++++------------------------------------ src/conf/object_event.c | 96 +++++++++++++++++++--- src/conf/object_event_private.h | 7 ++ 3 files changed, 107 insertions(+), 167 deletions(-) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index df370f6..4f8ede5 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -360,111 +360,6 @@ virDomainEventDeviceRemovedDispose(void *obj) } -/** - * virDomainEventCallbackListRemove: - * @conn: pointer to the connection - * @cbList: the list - * @callback: the callback to remove - * - * Internal function to remove a callback from a virObjectEventCallbackListPtr, - * when registered via the older virConnectDomainEventRegister with no - * callbackID - */ -static int -virDomainEventCallbackListRemove(virConnectPtr conn, - virObjectEventCallbackListPtr cbList, - virConnectDomainEventCallback callback) -{ - int ret = 0; - size_t i; - for (i = 0; i < cbList->count; i++) { - if (cbList->callbacks[i]->cb == VIR_OBJECT_EVENT_CALLBACK(callback) && - cbList->callbacks[i]->eventID == VIR_DOMAIN_EVENT_ID_LIFECYCLE && - cbList->callbacks[i]->conn == conn) { - virFreeCallback freecb = cbList->callbacks[i]->freecb; - if (freecb) - (*freecb)(cbList->callbacks[i]->opaque); - virObjectUnref(cbList->callbacks[i]->conn); - VIR_FREE(cbList->callbacks[i]); - - if (i < (cbList->count - 1)) - memmove(cbList->callbacks + i, - cbList->callbacks + i + 1, - sizeof(*(cbList->callbacks)) * - (cbList->count - (i + 1))); - - if (VIR_REALLOC_N(cbList->callbacks, - cbList->count - 1) < 0) { - ; /* Failure to reduce memory allocation isn't fatal */ - } - cbList->count--; - - for (i = 0; i < cbList->count; i++) { - if (!cbList->callbacks[i]->deleted) - ret++; - } - return ret; - } - } - - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("could not find event callback for removal")); - return -1; -} - - -static int -virDomainEventCallbackListMarkDelete(virConnectPtr conn, - virObjectEventCallbackListPtr cbList, - virConnectDomainEventCallback callback) -{ - int ret = 0; - size_t i; - for (i = 0; i < cbList->count; i++) { - if (cbList->callbacks[i]->cb == VIR_OBJECT_EVENT_CALLBACK(callback) && - cbList->callbacks[i]->eventID == VIR_DOMAIN_EVENT_ID_LIFECYCLE && - 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; - } - } - - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("could not find event callback for deletion")); - return -1; -} - - -/** - * virDomainEventCallbackListAdd: - * @conn: pointer to the connection - * @cbList: the list - * @callback: the callback to add - * @opaque: opaque data to pass to @callback - * @freecb: callback to free @opaque - * - * Internal function to add a callback from a virObjectEventCallbackListPtr, - * when registered via the older virConnectDomainEventRegister. - */ -static int -virDomainEventCallbackListAdd(virConnectPtr conn, - virObjectEventCallbackListPtr cbList, - virConnectDomainEventCallback callback, - void *opaque, - virFreeCallback freecb) -{ - return virObjectEventCallbackListAddID(conn, cbList, NULL, NULL, 0, - virDomainEventClass, - VIR_DOMAIN_EVENT_ID_LIFECYCLE, - VIR_OBJECT_EVENT_CALLBACK(callback), - opaque, freecb, NULL); -} - - static void * virDomainEventNew(virClassPtr klass, int eventID, @@ -1386,37 +1281,14 @@ virDomainEventStateRegister(virConnectPtr conn, void *opaque, virFreeCallback freecb) { - int ret = -1; - if (virDomainEventsInitialize() < 0) return -1; - virObjectEventStateLock(state); - - if ((state->callbacks->count == 0) && - (state->timer == -1) && - (state->timer = virEventAddTimeout(-1, - virObjectEventTimer, - state, - NULL)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("could not initialize domain event timer")); - goto cleanup; - } - - ret = virDomainEventCallbackListAdd(conn, state->callbacks, - callback, opaque, freecb); - - if (ret == -1 && - state->callbacks->count == 0 && - state->timer != -1) { - virEventRemoveTimeout(state->timer); - state->timer = -1; - } - -cleanup: - virObjectEventStateUnlock(state); - return ret; + return virObjectEventStateRegisterID(conn, state, NULL, NULL, 0, + virDomainEventClass, + VIR_DOMAIN_EVENT_ID_LIFECYCLE, + VIR_OBJECT_EVENT_CALLBACK(callback), + opaque, freecb, NULL); } @@ -1467,34 +1339,25 @@ virDomainEventStateRegisterID(virConnectPtr conn, * virDomainEventStateDeregister: * @conn: connection to associate with callback * @state: object event state - * @callback: function to remove from event + * @cb: function to remove from event * - * Unregister the function @callback with connection @conn, - * from @state, for lifecycle events. + * Unregister the function @cb with connection @conn, from @state, for + * lifecycle events. * * Returns: the number of lifecycle callbacks still registered, or -1 on error */ int virDomainEventStateDeregister(virConnectPtr conn, virObjectEventStatePtr state, - virConnectDomainEventCallback callback) + virConnectDomainEventCallback cb) { - int ret; - - virObjectEventStateLock(state); - if (state->isDispatching) - ret = virDomainEventCallbackListMarkDelete(conn, - state->callbacks, callback); - else - ret = virDomainEventCallbackListRemove(conn, state->callbacks, callback); - - if (state->callbacks->count == 0 && - state->timer != -1) { - virEventRemoveTimeout(state->timer); - state->timer = -1; - virObjectEventQueueClear(state->queue); - } + int callbackID; - virObjectEventStateUnlock(state); - return ret; + callbackID = virObjectEventStateCallbackID(conn, state, + virDomainEventClass, + VIR_DOMAIN_EVENT_ID_LIFECYCLE, + VIR_OBJECT_EVENT_CALLBACK(cb)); + if (callbackID < 0) + return -1; + return virObjectEventStateDeregisterID(conn, state, callbackID); } diff --git a/src/conf/object_event.c b/src/conf/object_event.c index 7c264f5..6c4d8b4 100644 --- a/src/conf/object_event.c +++ b/src/conf/object_event.c @@ -208,6 +208,52 @@ virObjectEventCallbackListPurgeMarked(virObjectEventCallbackListPtr cbList) /** + * virObjectEventCallbackLookup: + * @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 locate + * + * Internal function to determine if @callback already has a + * callbackID in @cbList for the given @conn and other filters. + * Return the id if found, or -1 with no error issued if not present. + */ +static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) +virObjectEventCallbackLookup(virConnectPtr conn, + virObjectEventCallbackListPtr cbList, + unsigned char uuid[VIR_UUID_BUFLEN], + virClassPtr klass, + int eventID, + virConnectObjectEventGenericCallback callback) +{ + int ret = -1; + size_t i; + + for (i = 0; i < cbList->count; i++) { + virObjectEventCallbackPtr cb = cbList->callbacks[i]; + + if (cb->deleted) + continue; + if (cb->cb == callback && + cb->klass == klass && + cb->eventID == eventID && + cb->conn == conn && + ((uuid && cb->meta && + memcmp(cb->meta->uuid, uuid, VIR_UUID_BUFLEN) == 0) || + (!uuid && !cb->meta))) { + ret = cb->callbackID; + break; + } + } + return ret; +} + + +/** * virObjectEventCallbackListAddID: * @conn: pointer to the connection * @cbList: the list @@ -251,19 +297,11 @@ virObjectEventCallbackListAddID(virConnectPtr conn, } /* check if we already have this callback on our list */ - for (i = 0; i < cbList->count; i++) { - if (cbList->callbacks[i]->cb == VIR_OBJECT_EVENT_CALLBACK(callback) && - cbList->callbacks[i]->klass == klass && - cbList->callbacks[i]->eventID == eventID && - cbList->callbacks[i]->conn == conn && - ((uuid && cbList->callbacks[i]->meta && - memcmp(cbList->callbacks[i]->meta->uuid, - uuid, VIR_UUID_BUFLEN) == 0) || - (!uuid && !cbList->callbacks[i]->meta))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("event callback already tracked")); - return -1; - } + if (virObjectEventCallbackLookup(conn, cbList, uuid, + klass, eventID, callback) != -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("event callback already tracked")); + return -1; } /* Allocate new event */ if (VIR_ALLOC(event) < 0) @@ -786,6 +824,38 @@ virObjectEventStateDeregisterID(virConnectPtr conn, return ret; } +/** + * virObjectEventStateCallbackID: + * @conn: connection associated with callback + * @state: object event state + * @klass: the base event class + * @eventID: the event ID + * @callback: function registered as a callback + * + * Returns the callbackID of @callback, or -1 with an error issued if the + * function is not currently registered. + */ +int +virObjectEventStateCallbackID(virConnectPtr conn, + virObjectEventStatePtr state, + virClassPtr klass, + int eventID, + virConnectObjectEventGenericCallback callback) +{ + int ret = -1; + + virObjectEventStateLock(state); + ret = virObjectEventCallbackLookup(conn, state->callbacks, NULL, + klass, eventID, callback); + virObjectEventStateUnlock(state); + + if (ret < 0) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("event callback function %p not registered"), + callback); + return ret; +} + /** * virObjectEventStateEventID: diff --git a/src/conf/object_event_private.h b/src/conf/object_event_private.h index 59fb2b3..f8063ff 100644 --- a/src/conf/object_event_private.h +++ b/src/conf/object_event_private.h @@ -87,6 +87,13 @@ virClassPtr virClassForObjectEvent(void); int +virObjectEventStateCallbackID(virConnectPtr conn, + virObjectEventStatePtr state, + virClassPtr klass, + int eventID, + virConnectObjectEventGenericCallback callback); + +int virObjectEventCallbackListAddID(virConnectPtr conn, virObjectEventCallbackListPtr cbList, unsigned char *uuid, -- 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@redhat.com> --- 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@redhat.com> --- 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@redhat.com> --- src/conf/domain_event.c | 16 +++++----------- src/conf/network_event.c | 13 +++---------- src/conf/object_event.c | 48 +++++++++++++++++------------------------------- src/conf/object_event.h | 2 -- 4 files changed, 25 insertions(+), 54 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..e44d4f2 100644 --- a/src/conf/object_event.h +++ b/src/conf/object_event.h @@ -71,8 +71,6 @@ int virObjectEventStateRegisterID(virConnectPtr conn, virObjectEventStatePtr state, unsigned char *uuid, - const char *name, - int id, virClassPtr klass, int eventID, virConnectObjectEventGenericCallback cb, -- 1.8.4.2

While comparing network and domain events, I noticed that the test driver had to do a cast in one place and not the other. For consistency, we should hide the necessary casting as low as possible in the stack, with everything else using saner types. * src/conf/network_event.h (virNetworkEventStateRegisterID): Alter type. * src/conf/network_event.c (virNetworkEventStateRegisterID): Hoist cast here. * src/test/test_driver.c (testConnectNetworkEventRegisterAny): Simplify callers. * src/remote/remote_driver.c (remoteConnectNetworkEventRegisterAny): Likewise. * src/network/bridge_driver.c (networkConnectNetworkEventRegisterAny): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/network_event.c | 5 +++-- src/conf/network_event.h | 3 ++- src/network/bridge_driver.c | 3 +-- src/remote/remote_driver.c | 3 +-- src/test/test_driver.c | 3 +-- 5 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/conf/network_event.c b/src/conf/network_event.c index 38c74d1..3cd884d 100644 --- a/src/conf/network_event.c +++ b/src/conf/network_event.c @@ -143,7 +143,7 @@ virNetworkEventStateRegisterID(virConnectPtr conn, virObjectEventStatePtr state, virNetworkPtr net, int eventID, - virConnectObjectEventGenericCallback cb, + virConnectNetworkEventGenericCallback cb, void *opaque, virFreeCallback freecb, int *callbackID) @@ -153,7 +153,8 @@ virNetworkEventStateRegisterID(virConnectPtr conn, return virObjectEventStateRegisterID(conn, state, net ? net->uuid : NULL, virNetworkEventClass, eventID, - cb, opaque, freecb, callbackID); + VIR_OBJECT_EVENT_CALLBACK(cb), + opaque, freecb, callbackID); } diff --git a/src/conf/network_event.h b/src/conf/network_event.h index a1afbc5..843faf9 100644 --- a/src/conf/network_event.h +++ b/src/conf/network_event.h @@ -1,6 +1,7 @@ /* * network_event.h: network event queue processing helpers * + * Copyright (C) 2014 Red Hat, Inc. * Copyright (C) 2013 SUSE LINUX Products GmbH, Nuernberg, Germany. * * This library is free software; you can redistribute it and/or @@ -32,7 +33,7 @@ virNetworkEventStateRegisterID(virConnectPtr conn, virObjectEventStatePtr state, virNetworkPtr net, int eventID, - virConnectObjectEventGenericCallback cb, + virConnectNetworkEventGenericCallback cb, void *opaque, virFreeCallback freecb, int *callbackID); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 9dc1f7e..95e4b65 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2310,8 +2310,7 @@ networkConnectNetworkEventRegisterAny(virConnectPtr conn, goto cleanup; if (virNetworkEventStateRegisterID(conn, driver->networkEventState, - net, eventID, - VIR_OBJECT_EVENT_CALLBACK(callback), + net, eventID, callback, opaque, freecb, &ret) < 0) ret = -1; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index f311544..74da726 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -2928,8 +2928,7 @@ remoteConnectNetworkEventRegisterAny(virConnectPtr conn, remoteDriverLock(priv); if ((count = virNetworkEventStateRegisterID(conn, priv->eventState, - net, eventID, - VIR_OBJECT_EVENT_CALLBACK(callback), + net, eventID, callback, opaque, freecb, &callbackID)) < 0) goto done; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index da76666..cde82a1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -6113,8 +6113,7 @@ testConnectNetworkEventRegisterAny(virConnectPtr conn, testDriverLock(driver); if (virNetworkEventStateRegisterID(conn, driver->eventState, - net, eventID, - VIR_OBJECT_EVENT_CALLBACK(callback), + net, eventID, callback, opaque, freecb, &ret) < 0) ret = -1; testDriverUnlock(driver); -- 1.8.4.2