From 4221d64fcb83f73be6cfacdeb26bc23f342075cd Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 3 Jan 2014 16:50:14 -0700 Subject: [PATCH] event: don't let old-style events clobber per-domain events 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. And like all good bug fix patches, I enhanced the testsuite, validating that the changes in tests/ expose the failure without the rest of the patch. * 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. * tests/objecteventtest.c (testDomainCreateXMLMixed): New test. Signed-off-by: Eric Blake --- src/conf/domain_event.c | 171 ++++---------------------------- src/conf/object_event.c | 94 +++++++++++++++--- src/conf/object_event_private.h | 9 ++ tests/objecteventtest.c | 68 ++++++++++++- 4 files changed, 173 insertions(+), 169 deletions(-) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index df370f60a0..4f8ede552f 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 7c264f5ffb..babefb5102 100644 --- a/src/conf/object_event.c +++ b/src/conf/object_event.c @@ -207,6 +207,50 @@ virObjectEventCallbackListPurgeMarked(virObjectEventCallbackListPtr cbList) } +/** + * virObjectEventCallbackLookup: + * @conn: pointer to the connection + * @cbList: the list + * @uuid: the uuid 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 @@ -251,19 +295,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 +822,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 59fb2b3342..7c6ec59c75 100644 --- a/src/conf/object_event_private.h +++ b/src/conf/object_event_private.h @@ -86,6 +86,15 @@ struct _virObjectEvent { 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, diff --git a/tests/objecteventtest.c b/tests/objecteventtest.c index 6f657d25b1..22b0bfef51 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; @@ -207,6 +207,67 @@ cleanup: return ret; } +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. */ + if (!(dom = virDomainCreateXML(test->conn, domainDef, 0)) + 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) { @@ -471,7 +532,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; -- GitLab