[libvirt] [PATCHv2 11/14] event: don't allow mix of old- and new-style registration

[Eric Blake]

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.

* src/conf/object_event.c (_virObjectEventCallback): Add field.
(virObjectEventCallbackLookup): Add argument.
(virObjectEventCallbackListAddID, virObjectEventStateCallbackID):
Adjust callers.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 src/conf/object_event.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index bb7f935..87e10e0 100644
--- 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,
@@ -278,6 +281,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.
@@ -289,7 +293,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;
@@ -303,6 +308,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))) {
@@ -358,7 +364,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;
@@ -385,6 +392,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;
@@ -887,7 +896,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,
@@ -900,7 +911,7 @@ virObjectEventStateCallbackID(virConnectPtr conn,

     virObjectEventStateLock(state);
     ret = virObjectEventCallbackLookup(conn, state->callbacks, NULL,
-                                       klass, eventID, callback);
+                                       klass, eventID, callback, true);
     virObjectEventStateUnlock(state);

     if (ret < 0)
-- 
1.8.4.2