[libvirt] [PATCH 9/2] event: properly filter count of remaining events

[Eric Blake]

On the surface, this sequence of API calls should succeed:

id1 = virConnectDomainEventRegisterAny(..., VIR_DOMAIN_EVENT_ID_LIFECYCLE,...);
id2 = virConnectDomainEventRegisterAny(..., VIR_DOMAIN_EVENT_ID_RTC_CHANGE,...);
virConnectDomainEventDeregisterAny(id1);
id1 = virConnectDomainEventRegisterAny(..., VIR_DOMAIN_EVENT_ID_LIFECYCLE,...);

And for test:///default, it does.  But for qemu:///system, it fails:
libvirt: XML-RPC error : internal error: domain event 0 already registered

Looking closer, the bug is caused by miscommunication between
the object event engine and the client side of the remote driver.
In our implementation, we set up a single server-side event per
eventID, then the client side replicates that one event to all
callbacks that have been registered client side.  To know when
to turn the server side eventID on or off, the client side must
track how many events for the same eventID have been registered.
But while our code was filtering by eventID on event registration,
it did not filter on event deregistration.  So the above API calls
resulted in the deregister returning 1 instead of 0, so no RPC
deregister was issued, and the final register detects on the
server side that the server is already handling eventID 0.

Unfortunately, since the problem is only observable on remote
connections, it's not possible to enhance objecteventtest to
expose the semantics using only public API entry points.

* src/conf/object_event.c (virObjectEventCallbackListCount): New
function.
(virObjectEventCallbackListAddID)
(virObjectEventCallbackListRemoveID)
(virObjectEventCallbackListMarkDeleteID): Use it.

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

diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index 9293ab1..bb7f935 100644
--- a/src/conf/object_event.c
+++ b/src/conf/object_event.c
@@ -141,6 +141,39 @@ virObjectEventCallbackListFree(virObjectEventCallbackListPtr list)


 /**
+ * virObjectEventCallbackListCount:
+ * @conn: pointer to the connection
+ * @cbList: the list
+ * @klass: the base event class
+ * @eventID: the event ID
+ *
+ * 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.
+ */
+static int
+virObjectEventCallbackListCount(virConnectPtr conn,
+                                virObjectEventCallbackListPtr cbList,
+                                virClassPtr klass,
+                                int eventID)
+{
+    size_t i;
+    int ret = 0;
+
+    for (i = 0; i < cbList->count; i++) {
+        virObjectEventCallbackPtr cb = cbList->callbacks[i];
+
+        if (cb->klass == klass &&
+            cb->eventID == eventID &&
+            cb->conn == conn &&
+            !cb->deleted)
+            ret++;
+    }
+    return ret;
+}
+
+/**
  * virObjectEventCallbackListRemoveID:
  * @conn: pointer to the connection
  * @cbList: the list
@@ -153,13 +186,15 @@ virObjectEventCallbackListRemoveID(virConnectPtr conn,
                                    virObjectEventCallbackListPtr cbList,
                                    int callbackID)
 {
-    int ret = 0;
     size_t i;

     for (i = 0; i < cbList->count; i++) {
         virObjectEventCallbackPtr cb = cbList->callbacks[i];

         if (cb->callbackID == callbackID && cb->conn == conn) {
+            virClassPtr klass = cb->klass;
+            int eventID = cb->eventID;
+
             if (cb->freecb)
                 (*cb->freecb)(cb->opaque);
             virObjectUnref(cb->conn);
@@ -169,11 +204,8 @@ virObjectEventCallbackListRemoveID(virConnectPtr conn,
             VIR_FREE(cb);
             VIR_DELETE_ELEMENT(cbList->callbacks, i, cbList->count);

-            for (i = 0; i < cbList->count; i++) {
-                if (!cbList->callbacks[i]->deleted)
-                    ret++;
-            }
-            return ret;
+            return virObjectEventCallbackListCount(conn, cbList, klass,
+                                                   eventID);
         }
     }

@@ -188,18 +220,15 @@ virObjectEventCallbackListMarkDeleteID(virConnectPtr conn,
                                        virObjectEventCallbackListPtr cbList,
                                        int callbackID)
 {
-    int ret = 0;
     size_t i;

     for (i = 0; i < cbList->count; i++) {
-        if (cbList->callbacks[i]->callbackID == callbackID &&
-            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;
+        virObjectEventCallbackPtr cb = cbList->callbacks[i];
+
+        if (cb->callbackID == callbackID && cb->conn == conn) {
+            cb->deleted = true;
+            return virObjectEventCallbackListCount(conn, cbList, cb->klass,
+                                                   cb->eventID);
         }
     }

@@ -315,7 +344,6 @@ virObjectEventCallbackListAddID(virConnectPtr conn,
                                 int *callbackID)
 {
     virObjectEventCallbackPtr event;
-    size_t i;
     int ret = -1;

     VIR_DEBUG("conn=%p cblist=%p uuid=%p name=%s id=%d "
@@ -361,13 +389,7 @@ virObjectEventCallbackListAddID(virConnectPtr conn,
     if (VIR_APPEND_ELEMENT(cbList->callbacks, cbList->count, event) < 0)
         goto cleanup;

-    for (ret = 0, i = 0; i < cbList->count; i++) {
-        if (cbList->callbacks[i]->klass == klass &&
-            cbList->callbacks[i]->eventID == eventID &&
-            cbList->callbacks[i]->conn == conn &&
-            !cbList->callbacks[i]->deleted)
-            ret++;
-    }
+    ret = virObjectEventCallbackListCount(conn, cbList, klass, eventID);

 cleanup:
     if (event) {
-- 
1.8.4.2