From 0cd02bca6e71c4ee9a659469486d16475c213a86 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Sat, 4 Jan 2014 05:55:55 -0700 Subject: [PATCH] event: don't allow mix of old- and new-style registration 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. Adding another event registration in the testsuite is sufficient to cover this, where the test fails without the rest of the patch. * src/conf/object_event.c (_virObjectEventCallback): Add field. (virObjectEventCallbackLookup): Add argument. (virObjectEventCallbackListAddID, virObjectEventStateCallbackID): Adjust callers. * tests/objecteventtest.c (testDomainCreateXMLMixed): Enhance test. Signed-off-by: Eric Blake --- src/conf/object_event.c | 21 ++++++++++++++++----- tests/objecteventtest.c | 40 +++++++++++++++++++++++++++------------- 2 files changed, 43 insertions(+), 18 deletions(-) diff --git a/src/conf/object_event.c b/src/conf/object_event.c index e08c6a9560..59f375909e 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, @@ -276,6 +279,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. @@ -287,7 +291,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; @@ -301,6 +306,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))) { @@ -356,7 +362,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; @@ -383,6 +390,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; @@ -885,7 +894,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, @@ -898,7 +909,7 @@ virObjectEventStateCallbackID(virConnectPtr conn, virObjectEventStateLock(state); ret = virObjectEventCallbackLookup(conn, state->callbacks, NULL, - klass, eventID, callback); + klass, eventID, callback, true); virObjectEventStateUnlock(state); if (ret < 0) diff --git a/tests/objecteventtest.c b/tests/objecteventtest.c index b491ba814e..cc39dcf715 100644 --- a/tests/objecteventtest.c +++ b/tests/objecteventtest.c @@ -214,21 +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. */ - if (!(dom = virDomainCreateXML(test->conn, domainDef, 0))) + /* 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; @@ -237,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) { -- GitLab