[libvirt] [PATCHv2 07/14] event: don't let old-style events clobber per-domain events
Eric Blake
eblake at redhat.com
Mon Jan 6 22:27:22 UTC 2014
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 at 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
More information about the libvir-list
mailing list