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: NEric Blake <eblake@redhat.com>

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);
 }

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:

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,

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;