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

diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index e08c6a95607acc12ba80fada0050e5f13c6633bf..59f375909e6fc67e7ebbefe03d93a75d7b056f92 100644 (file)
--- 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 b491ba814e0d06219acf10bd1fdc52d0b476f843..cc39dcf715c9aef2d18d51cbfe61e1abddedb7ff 100644 (file)
--- 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) {