[libvirt] [PATCHv2 00/14] event cleanups
I'm sitting on quite a few event fixes, prior to being able to reach my goal of adding a new api to libvirt-qemu.so for querying arbitrary monitor events. For testing purposes, I split the tests from the code changes, to make it easier (at least for me) to prove what the bugs are, and how the code fixes the bug. Each patch in this series should pass 'make', but those labeled '[squash with next]' will fail 'make check' until you also apply the next patch in the series. Before pushing, I will merge the patches as documented. That is, although there are 14 mails, there are only 9 commits to push when I finally get acks. If you exclude tests/, this series removes more lines than it adds (always a desirable goal when refactoring to fix bugs). Eric Blake (14): event [squash with next]: add test of old-style events event: make deregister return value match docs event [squash with next]: test shared state driver in test:///default event: share state driver between test:///default connections event: rename confusing variable in test, remote drivers event [squash with next]: test that old-style can't clobber new-style event: don't let old-style events clobber per-domain events event: tighten scope of object_event event: properly filter count of remaining events event [squash with next]: test that old-style is distinct from new-style event: don't allow mix of old- and new-style registration event [squash with next]: test per-domain events of offline domain event: don't turn offline domain into global event event: make network events easier to use without casts src/conf/domain_event.c | 185 ++++------------------------- src/conf/network_event.c | 16 +-- src/conf/network_event.h | 3 +- src/conf/object_event.c | 255 +++++++++++++++++++++++++++++----------- src/conf/object_event.h | 5 +- src/conf/object_event_private.h | 63 ++-------- src/libvirt.c | 17 ++- src/libxl/libxl_driver.c | 35 +++--- src/lxc/lxc_driver.c | 30 ++--- src/network/bridge_driver.c | 14 ++- src/remote/remote_driver.c | 86 ++++++-------- src/test/test_driver.c | 135 +++++++++++++-------- src/uml/uml_driver.c | 29 +++-- src/vbox/vbox_tmpl.c | 32 +++-- src/xen/xen_driver.c | 25 ++-- tests/objecteventtest.c | 196 +++++++++++++++++++++++++++--- 16 files changed, 631 insertions(+), 495 deletions(-) -- 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> --- tests/objecteventtest.c | 73 +++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 61 insertions(+), 12 deletions(-) diff --git a/tests/objecteventtest.c b/tests/objecteventtest.c index ae29792..833c0fc 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,35 +127,79 @@ networkLifecycleCb(virConnectPtr conn ATTRIBUTE_UNUSED, static int +testDomainCreateXMLOld(const void *data) +{ + const objecteventTest *test = data; + lifecycleEventCounter counter; + virDomainPtr dom = NULL; + 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) { + virDomainDestroy(dom); + virDomainFree(dom); + } + + return ret; +} + +static int testDomainCreateXML(const void *data) { const objecteventTest *test = data; lifecycleEventCounter counter; int eventId = VIR_DOMAIN_EVENT_ID_LIFECYCLE; - virDomainPtr dom; + virDomainPtr dom = NULL; 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 (dom != NULL) { + if (id >= 0) + virConnectDomainEventDeregisterAny(test->conn, id); + if (dom) { virDomainDestroy(dom); virDomainFree(dom); } @@ -168,7 +213,7 @@ testDomainDefine(const void *data) const objecteventTest *test = data; lifecycleEventCounter counter; int eventId = VIR_DOMAIN_EVENT_ID_LIFECYCLE; - virDomainPtr dom; + virDomainPtr dom = NULL; int id; int ret = 0; @@ -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
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@redhat.com> --- 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
On 01/06/2014 05:27 PM, Eric Blake wrote:
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@redhat.com> --- 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(-)
Since they're a match set... I'll ACK 1 & 2 together. I was going to comment about vbox_tmpl.c, but some things are just better left alone :-) John
On 01/07/2014 06:09 AM, John Ferlan wrote:
On 01/06/2014 05:27 PM, Eric Blake wrote:
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.
Since they're a match set... I'll ACK 1 & 2 together.
Thanks; pushed.
I was going to comment about vbox_tmpl.c, but some things are just better left alone :-)
If more people were trying to backport vbox bug fixes to stable versions, then splitting the patch to fix the vbox bug in its own patch may have made more sense - but I don't get the sense that anyone will be bothered by me pushing this as one patch :) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The updated testsuite fails without the rest of this patch. Valgrind didn't report any leaks. * tests/objecteventtest.c (testDomainStartStopEvent): Enhance to cover this. (timeout, mymain): Ensure test fails rather than blocks. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/objecteventtest.c | 49 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 7 deletions(-) diff --git a/tests/objecteventtest.c b/tests/objecteventtest.c index 833c0fc..6f657d2 100644 --- a/tests/objecteventtest.c +++ b/tests/objecteventtest.c @@ -265,8 +265,10 @@ testDomainStartStopEvent(const void *data) lifecycleEventCounter counter; int eventId = VIR_DOMAIN_EVENT_ID_LIFECYCLE; int id; - int ret = 0; + int ret = -1; virDomainPtr dom; + virConnectPtr conn2 = NULL; + virDomainPtr dom2 = NULL; lifecycleEventCounter_reset(&counter); @@ -282,20 +284,40 @@ testDomainStartStopEvent(const void *data) virDomainDestroy(dom); virDomainCreate(dom); - if (virEventRunDefaultImpl() < 0) { - ret = -1; + if (virEventRunDefaultImpl() < 0) goto cleanup; - } if (counter.startEvents != 1 || counter.stopEvents != 1 || - counter.unexpectedEvents > 0) { - ret = -1; + counter.unexpectedEvents > 0) + goto cleanup; + + /* Repeat the test, but this time, trigger the events via an + * alternate connection. */ + if (!(conn2 = virConnectOpen("test:///default"))) + goto cleanup; + if (!(dom2 = virDomainLookupByName(conn2, "test"))) goto cleanup; - } + if (virDomainDestroy(dom2) < 0) + goto cleanup; + if (virDomainCreate(dom2) < 0) + goto cleanup; + + if (virEventRunDefaultImpl() < 0) + goto cleanup; + + if (counter.startEvents != 2 || counter.stopEvents != 2 || + counter.unexpectedEvents > 0) + goto cleanup; + + ret = 0; cleanup: virConnectDomainEventDeregisterAny(test->conn, id); virDomainFree(dom); + if (dom2) + virDomainFree(dom2); + if (conn2) + virConnectClose(conn2); return ret; } @@ -419,14 +441,26 @@ cleanup: return ret; } +static void +timeout(int id ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) +{ + fputs("test taking too long; giving up", stderr); + _exit(EXIT_FAILURE); +} + static int mymain(void) { objecteventTest test; int ret = EXIT_SUCCESS; + int timer; virEventRegisterDefaultImpl(); + /* Set up a timer to abort this test if it takes 10 seconds. */ + if ((timer = virEventAddTimeout(10 * 1000, timeout, NULL, NULL)) < 0) + return EXIT_FAILURE; + if (!(test.conn = virConnectOpen("test:///default"))) return EXIT_FAILURE; @@ -460,6 +494,7 @@ mymain(void) virNetworkUndefine(test.net); virNetworkFree(test.net); virConnectClose(test.conn); + virEventRemoveTimeout(timer); 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@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
On 01/06/2014 05:27 PM, Eric Blake wrote:
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(-)
Continuing with the matched set theme.... Does every compiler/architecture guarantee that 'defaultConnections' initializes to zero? It's a nit, but better safe than sorry I suppose. ACK John
On 01/07/2014 07:16 AM, John Ferlan wrote:
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.
Continuing with the matched set theme....
Does every compiler/architecture guarantee that 'defaultConnections' initializes to zero?
Yes. The C89, C99, and C11 language standards guarantee 0 initialization of all file-scope and static variables (only automatic scope variables inside function bodies are indeterminate).
It's a nit, but better safe than sorry I suppose.
No problem; not everyone reads the C standard for fun :)
ACK
Thanks; pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Since the introduction of network events, any driver that uses a single event state object to track both domain and network events should not include 'domain' in the name of that object. * src/test/test_driver.c (_testConn): s/domainEventState/eventState/, and fix all callers. * src/remote/remote_driver.c (private_data): Likewise. (remoteDomainEventQueue): Rename to remoteEventQueue. (remoteDomainEvents): Rename to remoteEvents. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/remote/remote_driver.c | 83 +++++++++++++++++++++------------------------- src/test/test_driver.c | 32 ++++++++---------- 2 files changed, 50 insertions(+), 65 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b952e57..7e48c59 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -93,7 +93,7 @@ struct private_data { char *hostname; /* Original hostname */ bool serverKeepAlive; /* Does server support keepalive protocol? */ - virObjectEventStatePtr domainEventState; + virObjectEventStatePtr eventState; }; enum { @@ -151,7 +151,7 @@ static void make_nonnull_storage_vol(remote_nonnull_storage_vol *vol_dst, virSto static void make_nonnull_secret(remote_nonnull_secret *secret_dst, virSecretPtr secret_src); static void make_nonnull_nwfilter(remote_nonnull_nwfilter *nwfilter_dst, virNWFilterPtr nwfilter_src); static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDomainSnapshotPtr snapshot_src); -static void remoteDomainEventQueue(struct private_data *priv, virObjectEventPtr event); +static void remoteEventQueue(struct private_data *priv, virObjectEventPtr event); /*----------------------------------------------------------------------*/ /* Helper functions for remoteOpen. */ @@ -279,7 +279,7 @@ remoteNetworkBuildEventLifecycle(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, virNetClientPtr client ATTRIBUTE_UNUSED, void *evdata, void *opaque); -static virNetClientProgramEvent remoteDomainEvents[] = { +static virNetClientProgramEvent remoteEvents[] = { { REMOTE_PROC_DOMAIN_EVENT_RTC_CHANGE, remoteDomainBuildEventRTCChange, sizeof(remote_domain_event_rtc_change_msg), @@ -815,8 +815,8 @@ doRemoteOpen(virConnectPtr conn, if (!(priv->remoteProgram = virNetClientProgramNew(REMOTE_PROGRAM, REMOTE_PROTOCOL_VERSION, - remoteDomainEvents, - ARRAY_CARDINALITY(remoteDomainEvents), + remoteEvents, + ARRAY_CARDINALITY(remoteEvents), conn))) goto failed; if (!(priv->lxcProgram = virNetClientProgramNew(LXC_PROGRAM, @@ -890,7 +890,7 @@ doRemoteOpen(virConnectPtr conn, goto failed; } - if (!(priv->domainEventState = virObjectEventStateNew())) + if (!(priv->eventState = virObjectEventStateNew())) goto failed; /* Successful. */ @@ -1095,8 +1095,8 @@ doRemoteClose(virConnectPtr conn, struct private_data *priv) /* See comment for remoteType. */ VIR_FREE(priv->type); - virObjectEventStateFree(priv->domainEventState); - priv->domainEventState = NULL; + virObjectEventStateFree(priv->eventState); + priv->eventState = NULL; return ret; } @@ -2927,8 +2927,7 @@ remoteConnectNetworkEventRegisterAny(virConnectPtr conn, remoteDriverLock(priv); - if ((count = virNetworkEventStateRegisterID(conn, - priv->domainEventState, + if ((count = virNetworkEventStateRegisterID(conn, priv->eventState, net, eventID, VIR_OBJECT_EVENT_CALLBACK(callback), opaque, freecb, @@ -2943,8 +2942,7 @@ remoteConnectNetworkEventRegisterAny(virConnectPtr conn, if (call(conn, priv, 0, REMOTE_PROC_CONNECT_NETWORK_EVENT_REGISTER_ANY, (xdrproc_t) xdr_remote_connect_network_event_register_any_args, (char *) &args, (xdrproc_t) xdr_void, (char *)NULL) == -1) { - virObjectEventStateDeregisterID(conn, - priv->domainEventState, + virObjectEventStateDeregisterID(conn, priv->eventState, callbackID); goto done; } @@ -2970,15 +2968,13 @@ remoteConnectNetworkEventDeregisterAny(virConnectPtr conn, remoteDriverLock(priv); - if ((eventID = virObjectEventStateEventID(conn, - priv->domainEventState, + if ((eventID = virObjectEventStateEventID(conn, priv->eventState, callbackID)) < 0) { virReportError(VIR_ERR_RPC, _("unable to find callback ID %d"), callbackID); goto done; } - if ((count = virObjectEventStateDeregisterID(conn, - priv->domainEventState, + if ((count = virObjectEventStateDeregisterID(conn, priv->eventState, callbackID)) < 0) { virReportError(VIR_ERR_RPC, _("unable to find callback ID %d"), callbackID); goto done; @@ -4421,7 +4417,7 @@ static int remoteConnectDomainEventRegister(virConnectPtr conn, remoteDriverLock(priv); - if ((count = virDomainEventStateRegister(conn, priv->domainEventState, + if ((count = virDomainEventStateRegister(conn, priv->eventState, callback, opaque, freecb)) < 0) goto done; @@ -4449,8 +4445,7 @@ static int remoteConnectDomainEventDeregister(virConnectPtr conn, remoteDriverLock(priv); - if ((count = virDomainEventStateDeregister(conn, - priv->domainEventState, + if ((count = virDomainEventStateDeregister(conn, priv->eventState, callback)) < 0) goto done; @@ -4488,7 +4483,7 @@ remoteDomainBuildEventLifecycle(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, event = virDomainEventLifecycleNewFromDom(dom, msg->event, msg->detail); virDomainFree(dom); - remoteDomainEventQueue(priv, event); + remoteEventQueue(priv, event); } @@ -4510,7 +4505,7 @@ remoteDomainBuildEventReboot(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, event = virDomainEventRebootNewFromDom(dom); virDomainFree(dom); - remoteDomainEventQueue(priv, event); + remoteEventQueue(priv, event); } @@ -4532,7 +4527,7 @@ remoteDomainBuildEventRTCChange(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, event = virDomainEventRTCChangeNewFromDom(dom, msg->offset); virDomainFree(dom); - remoteDomainEventQueue(priv, event); + remoteEventQueue(priv, event); } @@ -4554,7 +4549,7 @@ remoteDomainBuildEventWatchdog(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, event = virDomainEventWatchdogNewFromDom(dom, msg->action); virDomainFree(dom); - remoteDomainEventQueue(priv, event); + remoteEventQueue(priv, event); } @@ -4579,7 +4574,7 @@ remoteDomainBuildEventIOError(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, msg->action); virDomainFree(dom); - remoteDomainEventQueue(priv, event); + remoteEventQueue(priv, event); } @@ -4606,7 +4601,7 @@ remoteDomainBuildEventIOErrorReason(virNetClientProgramPtr prog ATTRIBUTE_UNUSED virDomainFree(dom); - remoteDomainEventQueue(priv, event); + remoteEventQueue(priv, event); } static void @@ -4629,7 +4624,7 @@ remoteDomainBuildEventBlockJob(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, virDomainFree(dom); - remoteDomainEventQueue(priv, event); + remoteEventQueue(priv, event); } static void @@ -4685,7 +4680,7 @@ remoteDomainBuildEventGraphics(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, virDomainFree(dom); - remoteDomainEventQueue(priv, event); + remoteEventQueue(priv, event); return; error: @@ -4731,7 +4726,7 @@ remoteDomainBuildEventControlError(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, virDomainFree(dom); - remoteDomainEventQueue(priv, event); + remoteEventQueue(priv, event); } @@ -4758,7 +4753,7 @@ remoteDomainBuildEventDiskChange(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, virDomainFree(dom); - remoteDomainEventQueue(priv, event); + remoteEventQueue(priv, event); } @@ -4783,7 +4778,7 @@ remoteDomainBuildEventTrayChange(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, virDomainFree(dom); - remoteDomainEventQueue(priv, event); + remoteEventQueue(priv, event); } static void @@ -4805,7 +4800,7 @@ remoteDomainBuildEventPMWakeup(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, virDomainFree(dom); - remoteDomainEventQueue(priv, event); + remoteEventQueue(priv, event); } static void @@ -4827,7 +4822,7 @@ remoteDomainBuildEventPMSuspend(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, virDomainFree(dom); - remoteDomainEventQueue(priv, event); + remoteEventQueue(priv, event); } @@ -4849,7 +4844,7 @@ remoteDomainBuildEventBalloonChange(virNetClientProgramPtr prog ATTRIBUTE_UNUSED event = virDomainEventBalloonChangeNewFromDom(dom, msg->actual); virDomainFree(dom); - remoteDomainEventQueue(priv, event); + remoteEventQueue(priv, event); } @@ -4872,7 +4867,7 @@ remoteDomainBuildEventPMSuspendDisk(virNetClientProgramPtr prog ATTRIBUTE_UNUSED virDomainFree(dom); - remoteDomainEventQueue(priv, event); + remoteEventQueue(priv, event); } @@ -4895,7 +4890,7 @@ remoteDomainBuildEventDeviceRemoved(virNetClientProgramPtr prog ATTRIBUTE_UNUSED virDomainFree(dom); - remoteDomainEventQueue(priv, event); + remoteEventQueue(priv, event); } @@ -4917,7 +4912,7 @@ remoteNetworkBuildEventLifecycle(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, event = virNetworkEventLifecycleNew(net->name, net->uuid, msg->event, msg->detail); virNetworkFree(net); - remoteDomainEventQueue(priv, event); + remoteEventQueue(priv, event); } @@ -5226,8 +5221,7 @@ static int remoteConnectDomainEventRegisterAny(virConnectPtr conn, remoteDriverLock(priv); - if ((count = virDomainEventStateRegisterID(conn, - priv->domainEventState, + if ((count = virDomainEventStateRegisterID(conn, priv->eventState, dom, eventID, callback, opaque, freecb, &callbackID)) < 0) @@ -5241,8 +5235,7 @@ static int remoteConnectDomainEventRegisterAny(virConnectPtr conn, if (call(conn, priv, 0, REMOTE_PROC_CONNECT_DOMAIN_EVENT_REGISTER_ANY, (xdrproc_t) xdr_remote_connect_domain_event_register_any_args, (char *) &args, (xdrproc_t) xdr_void, (char *)NULL) == -1) { - virObjectEventStateDeregisterID(conn, - priv->domainEventState, + virObjectEventStateDeregisterID(conn, priv->eventState, callbackID); goto done; } @@ -5267,15 +5260,13 @@ static int remoteConnectDomainEventDeregisterAny(virConnectPtr conn, remoteDriverLock(priv); - if ((eventID = virObjectEventStateEventID(conn, - priv->domainEventState, + if ((eventID = virObjectEventStateEventID(conn, priv->eventState, callbackID)) < 0) { virReportError(VIR_ERR_RPC, _("unable to find callback ID %d"), callbackID); goto done; } - if ((count = virObjectEventStateDeregisterID(conn, - priv->domainEventState, + if ((count = virObjectEventStateDeregisterID(conn, priv->eventState, callbackID)) < 0) { virReportError(VIR_ERR_RPC, _("unable to find callback ID %d"), callbackID); goto done; @@ -6809,9 +6800,9 @@ done: } static void -remoteDomainEventQueue(struct private_data *priv, virObjectEventPtr event) +remoteEventQueue(struct private_data *priv, virObjectEventPtr event) { - virObjectEventStateQueue(priv->domainEventState, event); + virObjectEventStateQueue(priv->eventState, event); } /* get_nonnull_domain and get_nonnull_network turn an on-wire diff --git a/src/test/test_driver.c b/src/test/test_driver.c index b8bd6f4..da76666 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -100,7 +100,7 @@ struct _testConn { int numCells; testCell cells[MAX_CELLS]; - virObjectEventStatePtr domainEventState; + virObjectEventStatePtr eventState; }; typedef struct _testConn testConn; typedef struct _testConn *testConnPtr; @@ -714,7 +714,7 @@ testOpenDefault(virConnectPtr conn) conn->privateData = privconn; - if (!(privconn->domainEventState = virObjectEventStateNew())) + if (!(privconn->eventState = virObjectEventStateNew())) goto error; if (!(privconn->domains = virDomainObjListNew())) @@ -828,7 +828,7 @@ error: virStoragePoolObjListFree(&privconn->pools); virNodeDeviceObjListFree(&privconn->devs); virObjectUnref(privconn->caps); - virObjectEventStateFree(privconn->domainEventState); + virObjectEventStateFree(privconn->eventState); virMutexDestroy(&privconn->lock); conn->privateData = NULL; virDomainDefFree(domdef); @@ -1386,7 +1386,7 @@ testOpenFromFile(virConnectPtr conn, const char *file) if (!(privconn->xmlopt = testBuildXMLConfig())) goto error; - if (!(privconn->domainEventState = virObjectEventStateNew())) + if (!(privconn->eventState = virObjectEventStateNew())) goto error; if (!(doc = virXMLParseFileCtxt(file, &ctxt))) { @@ -1432,7 +1432,7 @@ testOpenFromFile(virConnectPtr conn, const char *file) virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); VIR_FREE(privconn->path); - virObjectEventStateFree(privconn->domainEventState); + virObjectEventStateFree(privconn->eventState); testDriverUnlock(privconn); VIR_FREE(privconn); conn->privateData = NULL; @@ -1505,7 +1505,7 @@ static int testConnectClose(virConnectPtr conn) virNetworkObjListFree(&privconn->networks); virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); - virObjectEventStateFree(privconn->domainEventState); + virObjectEventStateFree(privconn->eventState); VIR_FREE(privconn->path); testDriverUnlock(privconn); @@ -6036,8 +6036,7 @@ testConnectDomainEventRegister(virConnectPtr conn, int ret = 0; testDriverLock(driver); - if (virDomainEventStateRegister(conn, - driver->domainEventState, + if (virDomainEventStateRegister(conn, driver->eventState, callback, opaque, freecb) < 0) ret = -1; testDriverUnlock(driver); @@ -6054,8 +6053,7 @@ testConnectDomainEventDeregister(virConnectPtr conn, int ret = 0; testDriverLock(driver); - if (virDomainEventStateDeregister(conn, - driver->domainEventState, + if (virDomainEventStateDeregister(conn, driver->eventState, callback) < 0) ret = -1; testDriverUnlock(driver); @@ -6076,8 +6074,7 @@ testConnectDomainEventRegisterAny(virConnectPtr conn, int ret; testDriverLock(driver); - if (virDomainEventStateRegisterID(conn, - driver->domainEventState, + if (virDomainEventStateRegisterID(conn, driver->eventState, dom, eventID, callback, opaque, freecb, &ret) < 0) ret = -1; @@ -6094,8 +6091,7 @@ testConnectDomainEventDeregisterAny(virConnectPtr conn, int ret = 0; testDriverLock(driver); - if (virObjectEventStateDeregisterID(conn, - driver->domainEventState, + if (virObjectEventStateDeregisterID(conn, driver->eventState, callbackID) < 0) ret = -1; testDriverUnlock(driver); @@ -6116,8 +6112,7 @@ testConnectNetworkEventRegisterAny(virConnectPtr conn, int ret; testDriverLock(driver); - if (virNetworkEventStateRegisterID(conn, - driver->domainEventState, + if (virNetworkEventStateRegisterID(conn, driver->eventState, net, eventID, VIR_OBJECT_EVENT_CALLBACK(callback), opaque, freecb, &ret) < 0) @@ -6135,8 +6130,7 @@ testConnectNetworkEventDeregisterAny(virConnectPtr conn, int ret = 0; testDriverLock(driver); - if (virObjectEventStateDeregisterID(conn, - driver->domainEventState, + if (virObjectEventStateDeregisterID(conn, driver->eventState, callbackID) < 0) ret = -1; testDriverUnlock(driver); @@ -6149,7 +6143,7 @@ testConnectNetworkEventDeregisterAny(virConnectPtr conn, static void testObjectEventQueue(testConnPtr driver, virObjectEventPtr event) { - virObjectEventStateQueue(driver->domainEventState, event); + virObjectEventStateQueue(driver->eventState, event); } static virDrvOpenStatus testSecretOpen(virConnectPtr conn, -- 1.8.4.2
On 01/06/2014 05:27 PM, Eric Blake wrote:
Since the introduction of network events, any driver that uses a single event state object to track both domain and network events should not include 'domain' in the name of that object.
* src/test/test_driver.c (_testConn): s/domainEventState/eventState/, and fix all callers. * src/remote/remote_driver.c (private_data): Likewise. (remoteDomainEventQueue): Rename to remoteEventQueue. (remoteDomainEvents): Rename to remoteEvents.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/remote/remote_driver.c | 83 +++++++++++++++++++++------------------------- src/test/test_driver.c | 32 ++++++++---------- 2 files changed, 50 insertions(+), 65 deletions(-)
ACK (mechanical) John
On 01/07/2014 07:20 AM, John Ferlan wrote:
On 01/06/2014 05:27 PM, Eric Blake wrote:
Since the introduction of network events, any driver that uses a single event state object to track both domain and network events should not include 'domain' in the name of that object.
* src/test/test_driver.c (_testConn): s/domainEventState/eventState/, and fix all callers. * src/remote/remote_driver.c (private_data): Likewise. (remoteDomainEventQueue): Rename to remoteEventQueue. (remoteDomainEvents): Rename to remoteEvents.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/remote/remote_driver.c | 83 +++++++++++++++++++++------------------------- src/test/test_driver.c | 32 ++++++++---------- 2 files changed, 50 insertions(+), 65 deletions(-)
ACK (mechanical)
Pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Test the previous patch. * tests/objecteventtest.c (testDomainCreateXMLMixed): New test. Signed-off-by: Eric Blake <eblake@redhat.com> --- 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
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 | 9 +++ 3 files changed, 109 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..7c6ec59 100644 --- a/src/conf/object_event_private.h +++ b/src/conf/object_event_private.h @@ -87,6 +87,15 @@ virClassPtr virClassForObjectEvent(void); int +virObjectEventStateCallbackID(virConnectPtr conn, + virObjectEventStatePtr state, + virClassPtr klass, + int eventID, + virConnectObjectEventGenericCallback callback) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_NONNULL(5); + +int virObjectEventCallbackListAddID(virConnectPtr conn, virObjectEventCallbackListPtr cbList, unsigned char *uuid, -- 1.8.4.2
On 01/06/2014 05:27 PM, Eric Blake wrote:
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 | 9 +++ 3 files changed, 109 insertions(+), 167 deletions(-)
ACK the pair (6 & 7)... The intro comment to 6 had me do a double take though thinking I messed up the pair! John
On 01/07/2014 08:14 AM, John Ferlan wrote:
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 | 9 +++ 3 files changed, 109 insertions(+), 167 deletions(-)
ACK the pair (6 & 7)... The intro comment to 6 had me do a double take though thinking I messed up the pair!
Oops :) - I forgot to touch that up when submitting the series (you can tell which order I wrote the series, and then I rebased before posting it, and rebased again to get my merged patches that I'm actually pushing). At any rate, I'll be pushing this soon. Thanks for the review.
John
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/07/2014 08:24 AM, Eric Blake wrote:
On 01/07/2014 08:14 AM, John Ferlan wrote:
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 | 9 +++ 3 files changed, 109 insertions(+), 167 deletions(-)
ACK the pair (6 & 7)... The intro comment to 6 had me do a double take though thinking I messed up the pair!
Oops :) - I forgot to touch that up when submitting the series (you can tell which order I wrote the series, and then I rebased before posting it, and rebased again to get my merged patches that I'm actually pushing). At any rate, I'll be pushing this soon. Thanks for the review.
Then I promptly broke the patch while merging the two together, and had to commit a one-line fixup patch. Oh well, bisection is broken if you hit my mistake (serves me right for not waiting for 'make check' to complete...) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Tighten up scope after the previous patch avoided using internals. This will also make it easier to change internal implementation without having to chase down quite as many impacted callers or worrying about two files getting implementations out of sync. * src/conf/object_event_private.h (virObjectEventCallbackListAddID, virObjectEventQueueClear) (virObjectEventStateLock, virObjectEventStateUnlock) (virObjectEventTimer): Drop prototype. (_virObjectEventCallbackList, _virObjectEventState) (_virObjectEventCallback): Move... * src/conf/object_event.c: ...here. (virObjectEventCallbackListAddID, virObjectEventQueueClear) (virObjectEventStateLock, virObjectEventStateUnlock) (virObjectEventTimer): Mark private. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/object_event.c | 42 +++++++++++++++++++++++++---- src/conf/object_event_private.h | 58 ----------------------------------------- 2 files changed, 37 insertions(+), 63 deletions(-) diff --git a/src/conf/object_event.c b/src/conf/object_event.c index 6c4d8b4..9293ab1 100644 --- a/src/conf/object_event.c +++ b/src/conf/object_event.c @@ -36,10 +36,42 @@ #define VIR_FROM_THIS VIR_FROM_NONE +struct _virObjectEventCallbackList { + unsigned int nextID; + size_t count; + virObjectEventCallbackPtr *callbacks; +}; + struct _virObjectEventQueue { size_t count; virObjectEventPtr *events; }; +typedef struct _virObjectEventQueue virObjectEventQueue; +typedef virObjectEventQueue *virObjectEventQueuePtr; + +struct _virObjectEventState { + /* The list of domain event callbacks */ + virObjectEventCallbackListPtr callbacks; + /* The queue of object events */ + virObjectEventQueuePtr queue; + /* Timer for flushing events queue */ + int timer; + /* Flag if we're in process of dispatching */ + bool isDispatching; + virMutex lock; +}; + +struct _virObjectEventCallback { + int callbackID; + virClassPtr klass; + int eventID; + virConnectPtr conn; + virObjectMetaPtr meta; + virConnectObjectEventGenericCallback cb; + void *opaque; + virFreeCallback freecb; + bool deleted; +}; static virClassPtr virObjectEventClass; @@ -269,7 +301,7 @@ virObjectEventCallbackLookup(virConnectPtr conn, * * Internal function to add a callback from a virObjectEventCallbackListPtr */ -int +static int virObjectEventCallbackListAddID(virConnectPtr conn, virObjectEventCallbackListPtr cbList, unsigned char uuid[VIR_UUID_BUFLEN], @@ -376,7 +408,7 @@ virObjectEventCallbackListEventID(virConnectPtr conn, * * Removes all elements from the queue */ -void +static void virObjectEventQueueClear(virObjectEventQueuePtr queue) { size_t i; @@ -422,7 +454,7 @@ virObjectEventQueueNew(void) * * Lock event state before calling functions from object_event_private.h. */ -void +static void virObjectEventStateLock(virObjectEventStatePtr state) { virMutexLock(&state->lock); @@ -435,7 +467,7 @@ virObjectEventStateLock(virObjectEventStatePtr state) * * Unlock event state after calling functions from object_event_private.h. */ -void +static void virObjectEventStateUnlock(virObjectEventStatePtr state) { virMutexUnlock(&state->lock); @@ -477,7 +509,7 @@ static void virObjectEventStateFlush(virObjectEventStatePtr state); * the callback of a periodic timer on the event loop, in order to * flush the callback queue. */ -void +static void virObjectEventTimer(int timer ATTRIBUTE_UNUSED, void *opaque) { virObjectEventStatePtr state = opaque; diff --git a/src/conf/object_event_private.h b/src/conf/object_event_private.h index 7c6ec59..76bd6d1 100644 --- a/src/conf/object_event_private.h +++ b/src/conf/object_event_private.h @@ -35,41 +35,9 @@ struct _virObjectMeta { typedef struct _virObjectMeta virObjectMeta; typedef virObjectMeta *virObjectMetaPtr; -struct _virObjectEventCallbackList { - unsigned int nextID; - size_t count; - virObjectEventCallbackPtr *callbacks; -}; typedef struct _virObjectEventCallbackList virObjectEventCallbackList; typedef virObjectEventCallbackList *virObjectEventCallbackListPtr; -typedef struct _virObjectEventQueue virObjectEventQueue; -typedef virObjectEventQueue *virObjectEventQueuePtr; - -struct _virObjectEventState { - /* The list of domain event callbacks */ - virObjectEventCallbackListPtr callbacks; - /* The queue of object events */ - virObjectEventQueuePtr queue; - /* Timer for flushing events queue */ - int timer; - /* Flag if we're in process of dispatching */ - bool isDispatching; - virMutex lock; -}; - -struct _virObjectEventCallback { - int callbackID; - virClassPtr klass; - int eventID; - virConnectPtr conn; - virObjectMetaPtr meta; - virConnectObjectEventGenericCallback cb; - void *opaque; - virFreeCallback freecb; - bool deleted; -}; - typedef void (*virObjectEventDispatchFunc)(virConnectPtr conn, virObjectEventPtr event, @@ -95,32 +63,6 @@ virObjectEventStateCallbackID(virConnectPtr conn, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5); -int -virObjectEventCallbackListAddID(virConnectPtr conn, - virObjectEventCallbackListPtr cbList, - unsigned char *uuid, - const char *name, - int id, - virClassPtr klass, - int eventID, - virConnectObjectEventGenericCallback callback, - void *opaque, - virFreeCallback freecb, - int *callbackID); - -void -virObjectEventQueueClear(virObjectEventQueuePtr queue); - -void -virObjectEventStateLock(virObjectEventStatePtr state); - -void -virObjectEventStateUnlock(virObjectEventStatePtr state); - -void -virObjectEventTimer(int timer, - void *opaque); - void * virObjectEventNew(virClassPtr klass, virObjectEventDispatchFunc dispatcher, -- 1.8.4.2
On 01/06/2014 05:27 PM, Eric Blake wrote:
Tighten up scope after the previous patch avoided using internals. This will also make it easier to change internal implementation without having to chase down quite as many impacted callers or worrying about two files getting implementations out of sync.
* src/conf/object_event_private.h (virObjectEventCallbackListAddID, virObjectEventQueueClear) (virObjectEventStateLock, virObjectEventStateUnlock) (virObjectEventTimer): Drop prototype. (_virObjectEventCallbackList, _virObjectEventState) (_virObjectEventCallback): Move... * src/conf/object_event.c: ...here. (virObjectEventCallbackListAddID, virObjectEventQueueClear) (virObjectEventStateLock, virObjectEventStateUnlock) (virObjectEventTimer): Mark private.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/object_event.c | 42 +++++++++++++++++++++++++---- src/conf/object_event_private.h | 58 ----------------------------------------- 2 files changed, 37 insertions(+), 63 deletions(-)
ACK John
On 01/07/2014 08:18 AM, John Ferlan wrote:
On 01/06/2014 05:27 PM, Eric Blake wrote:
Tighten up scope after the previous patch avoided using internals. This will also make it easier to change internal implementation without having to chase down quite as many impacted callers or worrying about two files getting implementations out of sync.
* src/conf/object_event_private.h (virObjectEventCallbackListAddID, virObjectEventQueueClear) (virObjectEventStateLock, virObjectEventStateUnlock) (virObjectEventTimer): Drop prototype. (_virObjectEventCallbackList, _virObjectEventState) (_virObjectEventCallback): Move... * src/conf/object_event.c: ...here. (virObjectEventCallbackListAddID, virObjectEventQueueClear) (virObjectEventStateLock, virObjectEventStateUnlock) (virObjectEventTimer): Mark private.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/object_event.c | 42 +++++++++++++++++++++++++---- src/conf/object_event_private.h | 58 ----------------------------------------- 2 files changed, 37 insertions(+), 63 deletions(-)
ACK
Pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 01/06/2014 05:27 PM, Eric Blake wrote:
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(-)
Strangely enough - I was going to remark about the usage of 'deleted' in the previous patch deleted code as being a bit unusual.. ACK John
On 01/07/2014 08:48 AM, John Ferlan wrote:
On 01/06/2014 05:27 PM, Eric Blake wrote:
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
Strangely enough - I was going to remark about the usage of 'deleted' in the previous patch deleted code as being a bit unusual..
Yep - part of the inconsistency was whether the various open-coded iterations considered or skipped deleted callbacks; having all the code go through a common function sure helps in understanding what is going on.
ACK
Pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
* tests/objecteventtest.c (testDomainCreateXMLMixed): Enhance test. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/objecteventtest.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/tests/objecteventtest.c b/tests/objecteventtest.c index 649fc25..cc39dcf 100644 --- a/tests/objecteventtest.c +++ b/tests/objecteventtest.c @@ -214,22 +214,24 @@ testDomainCreateXMLMixed(const void *data) lifecycleEventCounter counter; virDomainPtr dom; int ret = -1; - int id = -1; + int id1 = -1; + int id2 = -1; bool registered = false; lifecycleEventCounter_reset(&counter); - /* Fun with mixing old and new API. Handler should be fired twice, - * once for each registration. */ + /* Fun with mixing old and new API, also with global and + * per-domain. Handler should be fired three times, 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, + id1 = virConnectDomainEventRegisterAny(test->conn, dom, + VIR_DOMAIN_EVENT_ID_LIFECYCLE, VIR_DOMAIN_EVENT_CALLBACK(&domainLifecycleCb), - &counter, NULL); - if (id < 0) + &counter, NULL); + if (id1 < 0) goto cleanup; if (virDomainDestroy(dom) < 0) goto cleanup; @@ -238,25 +240,36 @@ testDomainCreateXMLMixed(const void *data) &counter, NULL) != 0) goto cleanup; registered = true; + id2 = virConnectDomainEventRegisterAny(test->conn, NULL, + VIR_DOMAIN_EVENT_ID_LIFECYCLE, + VIR_DOMAIN_EVENT_CALLBACK(&domainLifecycleCb), + &counter, NULL); + if (id2 < 0) + goto cleanup; dom = virDomainCreateXML(test->conn, domainDef, 0); if (dom == NULL || virEventRunDefaultImpl() < 0) goto cleanup; - if (counter.startEvents != 2 || counter.unexpectedEvents > 0) + if (counter.startEvents != 3 || counter.unexpectedEvents > 0) goto cleanup; if (virConnectDomainEventDeregister(test->conn, domainLifecycleCb) != 0) goto cleanup; registered = false; - if (virConnectDomainEventDeregisterAny(test->conn, id) != 0) + if (virConnectDomainEventDeregisterAny(test->conn, id1) != 0) goto cleanup; - id = -1; + id1 = -1; + if (virConnectDomainEventDeregisterAny(test->conn, id2) != 0) + goto cleanup; + id2 = -1; ret = 0; cleanup: - if (id >= 0) - virConnectDomainEventDeregisterAny(test->conn, id); + if (id1 >= 0) + virConnectDomainEventDeregisterAny(test->conn, id1); + if (id2 >= 0) + virConnectDomainEventDeregisterAny(test->conn, id2); if (registered) virConnectDomainEventDeregister(test->conn, domainLifecycleCb); if (dom != NULL) { -- 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
On 01/06/2014 05:27 PM, Eric Blake wrote:
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(-)
ACK the pair (10 & 11) John It's a tangled web you weave...
On 01/07/2014 09:23 AM, John Ferlan wrote:
On 01/06/2014 05:27 PM, Eric Blake wrote:
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);
ACK the pair (10 & 11)
John
It's a tangled web you weave...
Indeed; thanks for putting up with it, and reviewing the series. This one is now pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
* tests/objecteventtest.c (testDomainCreateXMLMixed): Enhance test. --- tests/objecteventtest.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/objecteventtest.c b/tests/objecteventtest.c index cc39dcf..65642a2 100644 --- a/tests/objecteventtest.c +++ b/tests/objecteventtest.c @@ -223,7 +223,7 @@ testDomainCreateXMLMixed(const void *data) /* Fun with mixing old and new API, also with global and * per-domain. Handler should be fired three times, once for each * registration. */ - dom = virDomainCreateXML(test->conn, domainDef, 0); + dom = virDomainDefineXML(test->conn, domainDef); if (dom == NULL) goto cleanup; @@ -233,8 +233,6 @@ testDomainCreateXMLMixed(const void *data) &counter, NULL); if (id1 < 0) goto cleanup; - if (virDomainDestroy(dom) < 0) - goto cleanup; if (virConnectDomainEventRegister(test->conn, domainLifecycleCb, &counter, NULL) != 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 | 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/06/2014 05:27 PM, Eric Blake wrote:
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 | 5 ++--- 4 files changed, 27 insertions(+), 55 deletions(-)
The intro comments to 'virObjectEventStateRegisterID()' need to be adjusted to remove '@name' and '@id'. Same for 'virObjectEventCallbackLookup()' - sadly missed in patch 7, but I was looking for @name and @id now... Leaving only 'virObjectEventNew()' as the lone API that cares about 'id' and 'name', but doesn't utilize. Should that be noted somewhere? Since 'meta.name' and 'meta.id' aren't used anywhere, do they even need to be saved... Would save an alloc/free for name. Should the existing 'testDomainCreateXMLMixed()' be kept as is? And then add a 'testDomainDefineXMLMixed()'? At the very least the name should probably be changed since other test functions distinguish Define & Create in their names. ACK in general though. John
On 01/07/2014 10:07 AM, John Ferlan wrote:
The intro comments to 'virObjectEventStateRegisterID()' need to be adjusted to remove '@name' and '@id'.
Will do.
Same for 'virObjectEventCallbackLookup()' - sadly missed in patch 7, but I was looking for @name and @id now...
I actually caught that in patch 7 in time (but managed to botch the testsuite in the process of amending that patch).
Leaving only 'virObjectEventNew()' as the lone API that cares about 'id' and 'name', but doesn't utilize. Should that be noted somewhere? Since 'meta.name' and 'meta.id' aren't used anywhere, do they even need to be saved... Would save an alloc/free for name.
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 :)
Should the existing 'testDomainCreateXMLMixed()' be kept as is? And then add a 'testDomainDefineXMLMixed()'? At the very least the name should probably be changed since other test functions distinguish Define & Create in their names.
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 :)
ACK in general though.
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
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 7e48c59..fecb9b2 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
On 01/06/2014 05:27 PM, Eric Blake wrote:
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(-)
ACK No worries about back compat since added after 1.2... John
On 01/07/2014 10:15 AM, John Ferlan wrote:
On 01/06/2014 05:27 PM, Eric Blake wrote:
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.
ACK
Pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
John Ferlan