[libvirt] [PATCH 7/2] 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. * 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(a)redhat.com&gt; --- src/conf/domain_event.c | 171 ++++------------------------------------ src/conf/object_event.c | 96 +++++++++++++++++++--- src/conf/object_event_private.h | 7 ++ 3 files changed, 107 insertions(+), 167 deletions(-) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index df370f6..4f8ede5 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -360,111 +360,6 @@ virDomainEventDeviceRemovedDispose(void *obj) } -/** - * virDomainEventCallbackListRemove: - * @conn: pointer to the connection - * @cbList: the list - * @callback: the callback to remove - * - * Internal function to remove a callback from a virObjectEventCallbackListPtr, - * when registered via the older virConnectDomainEventRegister with no - * callbackID - */ -static int -virDomainEventCallbackListRemove(virConnectPtr conn, - virObjectEventCallbackListPtr cbList, - virConnectDomainEventCallback callback) -{ - int ret = 0; - size_t i; - for (i = 0; i < cbList->count; i++) { - if (cbList->callbacks[i]->cb == VIR_OBJECT_EVENT_CALLBACK(callback) && - cbList->callbacks[i]->eventID == VIR_DOMAIN_EVENT_ID_LIFECYCLE && - cbList->callbacks[i]->conn == conn) { - virFreeCallback freecb = cbList->callbacks[i]->freecb; - if (freecb) - (*freecb)(cbList->callbacks[i]->opaque); - virObjectUnref(cbList->callbacks[i]->conn); - VIR_FREE(cbList->callbacks[i]); - - if (i < (cbList->count - 1)) - memmove(cbList->callbacks + i, - cbList->callbacks + i + 1, - sizeof(*(cbList->callbacks)) * - (cbList->count - (i + 1))); - - if (VIR_REALLOC_N(cbList->callbacks, - cbList->count - 1) < 0) { - ; /* Failure to reduce memory allocation isn't fatal */ - } - cbList->count--; - - for (i = 0; i < cbList->count; i++) { - if (!cbList->callbacks[i]->deleted) - ret++; - } - return ret; - } - } - - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("could not find event callback for removal")); - return -1; -} - - -static int -virDomainEventCallbackListMarkDelete(virConnectPtr conn, - virObjectEventCallbackListPtr cbList, - virConnectDomainEventCallback callback) -{ - int ret = 0; - size_t i; - for (i = 0; i < cbList->count; i++) { - if (cbList->callbacks[i]->cb == VIR_OBJECT_EVENT_CALLBACK(callback) && - cbList->callbacks[i]->eventID == VIR_DOMAIN_EVENT_ID_LIFECYCLE && - cbList->callbacks[i]->conn == conn) { - cbList->callbacks[i]->deleted = true; - for (i = 0; i < cbList->count; i++) { - if (!cbList->callbacks[i]->deleted) - ret++; - } - return ret; - } - } - - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("could not find event callback for deletion")); - return -1; -} - - -/** - * virDomainEventCallbackListAdd: - * @conn: pointer to the connection - * @cbList: the list - * @callback: the callback to add - * @opaque: opaque data to pass to @callback - * @freecb: callback to free @opaque - * - * Internal function to add a callback from a virObjectEventCallbackListPtr, - * when registered via the older virConnectDomainEventRegister. - */ -static int -virDomainEventCallbackListAdd(virConnectPtr conn, - virObjectEventCallbackListPtr cbList, - virConnectDomainEventCallback callback, - void *opaque, - virFreeCallback freecb) -{ - return virObjectEventCallbackListAddID(conn, cbList, NULL, NULL, 0, - virDomainEventClass, - VIR_DOMAIN_EVENT_ID_LIFECYCLE, - VIR_OBJECT_EVENT_CALLBACK(callback), - opaque, freecb, NULL); -} - - static void * virDomainEventNew(virClassPtr klass, int eventID, @@ -1386,37 +1281,14 @@ virDomainEventStateRegister(virConnectPtr conn, void *opaque, virFreeCallback freecb) { - int ret = -1; - if (virDomainEventsInitialize() < 0) return -1; - virObjectEventStateLock(state); - - if ((state->callbacks->count == 0) && - (state->timer == -1) && - (state->timer = virEventAddTimeout(-1, - virObjectEventTimer, - state, - NULL)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("could not initialize domain event timer")); - goto cleanup; - } - - ret = virDomainEventCallbackListAdd(conn, state->callbacks, - callback, opaque, freecb); - - if (ret == -1 && - state->callbacks->count == 0 && - state->timer != -1) { - virEventRemoveTimeout(state->timer); - state->timer = -1; - } - -cleanup: - virObjectEventStateUnlock(state); - return ret; + return virObjectEventStateRegisterID(conn, state, NULL, NULL, 0, + virDomainEventClass, + VIR_DOMAIN_EVENT_ID_LIFECYCLE, + VIR_OBJECT_EVENT_CALLBACK(callback), + opaque, freecb, NULL); } @@ -1467,34 +1339,25 @@ virDomainEventStateRegisterID(virConnectPtr conn, * virDomainEventStateDeregister: * @conn: connection to associate with callback * @state: object event state - * @callback: function to remove from event + * @cb: function to remove from event * - * Unregister the function @callback with connection @conn, - * from @state, for lifecycle events. + * Unregister the function @cb with connection @conn, from @state, for + * lifecycle events. * * Returns: the number of lifecycle callbacks still registered, or -1 on error */ int virDomainEventStateDeregister(virConnectPtr conn, virObjectEventStatePtr state, - virConnectDomainEventCallback callback) + virConnectDomainEventCallback cb) { - int ret; - - virObjectEventStateLock(state); - if (state->isDispatching) - ret = virDomainEventCallbackListMarkDelete(conn, - state->callbacks, callback); - else - ret = virDomainEventCallbackListRemove(conn, state->callbacks, callback); - - if (state->callbacks->count == 0 && - state->timer != -1) { - virEventRemoveTimeout(state->timer); - state->timer = -1; - virObjectEventQueueClear(state->queue); - } + int callbackID; - virObjectEventStateUnlock(state); - return ret; + callbackID = virObjectEventStateCallbackID(conn, state, + virDomainEventClass, + VIR_DOMAIN_EVENT_ID_LIFECYCLE, + VIR_OBJECT_EVENT_CALLBACK(cb)); + if (callbackID < 0) + return -1; + return virObjectEventStateDeregisterID(conn, state, callbackID); } diff --git a/src/conf/object_event.c b/src/conf/object_event.c index 7c264f5..6c4d8b4 100644 --- a/src/conf/object_event.c +++ b/src/conf/object_event.c @@ -208,6 +208,52 @@ virObjectEventCallbackListPurgeMarked(virObjectEventCallbackListPtr cbList) /** + * virObjectEventCallbackLookup: + * @conn: pointer to the connection + * @cbList: the list + * @uuid: the uuid of the object to filter on + * @name: the name of the object to filter on + * @id: the ID of the object to filter on + * @klass: the base event class + * @eventID: the event ID + * @callback: the callback to locate + * + * Internal function to determine if @callback already has a + * callbackID in @cbList for the given @conn and other filters. + * Return the id if found, or -1 with no error issued if not present. + */ +static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) +virObjectEventCallbackLookup(virConnectPtr conn, + virObjectEventCallbackListPtr cbList, + unsigned char uuid[VIR_UUID_BUFLEN], + virClassPtr klass, + int eventID, + virConnectObjectEventGenericCallback callback) +{ + int ret = -1; + size_t i; + + for (i = 0; i < cbList->count; i++) { + virObjectEventCallbackPtr cb = cbList->callbacks[i]; + + if (cb->deleted) + continue; + if (cb->cb == callback && + cb->klass == klass && + cb->eventID == eventID && + cb->conn == conn && + ((uuid && cb->meta && + memcmp(cb->meta->uuid, uuid, VIR_UUID_BUFLEN) == 0) || + (!uuid && !cb->meta))) { + ret = cb->callbackID; + break; + } + } + return ret; +} + + +/** * virObjectEventCallbackListAddID: * @conn: pointer to the connection * @cbList: the list @@ -251,19 +297,11 @@ virObjectEventCallbackListAddID(virConnectPtr conn, } /* check if we already have this callback on our list */ - for (i = 0; i < cbList->count; i++) { - if (cbList->callbacks[i]->cb == VIR_OBJECT_EVENT_CALLBACK(callback) && - cbList->callbacks[i]->klass == klass && - cbList->callbacks[i]->eventID == eventID && - cbList->callbacks[i]->conn == conn && - ((uuid && cbList->callbacks[i]->meta && - memcmp(cbList->callbacks[i]->meta->uuid, - uuid, VIR_UUID_BUFLEN) == 0) || - (!uuid && !cbList->callbacks[i]->meta))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("event callback already tracked")); - return -1; - } + if (virObjectEventCallbackLookup(conn, cbList, uuid, + klass, eventID, callback) != -1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("event callback already tracked")); + return -1; } /* Allocate new event */ if (VIR_ALLOC(event) < 0) @@ -786,6 +824,38 @@ virObjectEventStateDeregisterID(virConnectPtr conn, return ret; } +/** + * virObjectEventStateCallbackID: + * @conn: connection associated with callback + * @state: object event state + * @klass: the base event class + * @eventID: the event ID + * @callback: function registered as a callback + * + * Returns the callbackID of @callback, or -1 with an error issued if the + * function is not currently registered. + */ +int +virObjectEventStateCallbackID(virConnectPtr conn, + virObjectEventStatePtr state, + virClassPtr klass, + int eventID, + virConnectObjectEventGenericCallback callback) +{ + int ret = -1; + + virObjectEventStateLock(state); + ret = virObjectEventCallbackLookup(conn, state->callbacks, NULL, + klass, eventID, callback); + virObjectEventStateUnlock(state); + + if (ret < 0) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("event callback function %p not registered"), + callback); + return ret; +} + /** * virObjectEventStateEventID: diff --git a/src/conf/object_event_private.h b/src/conf/object_event_private.h index 59fb2b3..f8063ff 100644 --- a/src/conf/object_event_private.h +++ b/src/conf/object_event_private.h @@ -87,6 +87,13 @@ virClassPtr virClassForObjectEvent(void); int +virObjectEventStateCallbackID(virConnectPtr conn, + virObjectEventStatePtr state, + virClassPtr klass, + int eventID, + virConnectObjectEventGenericCallback callback); + +int virObjectEventCallbackListAddID(virConnectPtr conn, virObjectEventCallbackListPtr cbList, unsigned char *uuid, -- 1.8.4.2