event: don't turn offline domain into global event

If a user registers for a domain event filtered to a particular
domain, but the persistent domain is offline at the time, then
the code silently failed to set up the filter.  As a result,
the event fires for all domains, rather than being filtered.
Network events were immune, since they always passed an id
0 argument.

The key to this patch is realizing that
virObjectEventDispatchMatchCallback() only cared about uuid;
so refusing to create a meta for a negative id is pointless,
and in fact, malloc'ing meta at all was overkill; instead,
just directly store a uuid and a flag of whether to filter.

Note that virObjectEventPtr still needs all fields of meta,
because this is how we reconstruct a virDomainPtr inside the
dispatch handler before calling the end user's callback
pointer with the correct object, even though only the uuid
portion of meta is used in deciding whether a callback
matches the given event.  So while uuid is optional for
callbacks, it is mandatory for events.

The change to testDomainCreateXMLMixed is merely on the setup
scenario (as you can't register for a domain unless it is either
running or persistent).  I actually first wrote that test for
this patch, then rebased it to also cover a prior patch (commit
4221d64), but had to adjust it for that patch to use Create
instead of Define for setting up the domain long enough to
register the event in order to work around this bug.  But while
the setup is changed, the main body of the test is still about
whether creation events fire as expected.

* src/conf/object_event_private.h (_virObjectEventCallback):
Replace meta with uuid and flag.
(virObjectEventCallbackListAddID): Update signature.
* src/conf/object_event.h (virObjectEventStateRegisterID):
Likewise.
* src/conf/object_event_private.h (virObjectEventNew): Document
use of name and uuid in events.
* src/conf/object_event.c (virObjectEventCallbackListAddID): Drop
arguments that don't affect filtering.
(virObjectEventCallbackListRemoveID)
(virObjectEventDispatchMatchCallback)
(virObjectEventStateRegisterID): Update clients.
* src/conf/domain_event.c (virDomainEventCallbackListAdd)
(virDomainEventStateRegisterID): Likewise.
* src/conf/network_event.c (virNetworkEventStateRegisterID):
Likewise.
* tests/objecteventtest.c (testDomainCreateXMLMixed): Enhance test.

Signed-off-by: Eric Blake <eblake@redhat.com>

diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
index 4f8ede552f5155f41bc2f62126a1386cad15b87d..35212ef45d246d54f99a73596220195226432fcc 100644 (file)
--- a/src/conf/domain_event.c
+++ b/src/conf/domain_event.c
@@ -1284,7 +1284,7 @@ virDomainEventStateRegister(virConnectPtr conn,
     if (virDomainEventsInitialize() < 0)
         return -1;
 
-    return virObjectEventStateRegisterID(conn, state, NULL, NULL, 0,
+    return virObjectEventStateRegisterID(conn, state, NULL,
                                          virDomainEventClass,
                                          VIR_DOMAIN_EVENT_ID_LIFECYCLE,
                                          VIR_OBJECT_EVENT_CALLBACK(callback),
@@ -1322,16 +1322,10 @@ virDomainEventStateRegisterID(virConnectPtr conn,
     if (virDomainEventsInitialize() < 0)
         return -1;
 
-    if (dom)
-        return virObjectEventStateRegisterID(conn, state, dom->uuid, dom->name,
-                                             dom->id, virDomainEventClass, eventID,
-                                             VIR_OBJECT_EVENT_CALLBACK(cb),
-                                             opaque, freecb, callbackID);
-     else
-        return virObjectEventStateRegisterID(conn, state, NULL, NULL, 0,
-                                             virDomainEventClass, eventID,
-                                             VIR_OBJECT_EVENT_CALLBACK(cb),
-                                             opaque, freecb, callbackID);
+    return virObjectEventStateRegisterID(conn, state, dom ? dom->uuid : NULL,
+                                         virDomainEventClass, eventID,
+                                         VIR_OBJECT_EVENT_CALLBACK(cb),
+                                         opaque, freecb, callbackID);
 }
 
 

diff --git a/src/conf/network_event.c b/src/conf/network_event.c
index a192ffe6b1a92b6c4fbd552703f383071f2b1272..38c74d1ab7e385bb9da604bf2b85b0f13c9d49a6 100644 (file)
--- a/src/conf/network_event.c
+++ b/src/conf/network_event.c
@@ -151,16 +151,9 @@ virNetworkEventStateRegisterID(virConnectPtr conn,
     if (virNetworkEventsInitialize() < 0)
         return -1;
 
-    if (net)
-        return virObjectEventStateRegisterID(conn, state,
-                                             net->uuid, net->name, 0,
-                                             virNetworkEventClass, eventID,
-                                             cb, opaque, freecb, callbackID);
-    else
-        return virObjectEventStateRegisterID(conn, state,
-                                             NULL, NULL, 0,
-                                             virNetworkEventClass, eventID,
-                                             cb, opaque, freecb, callbackID);
+    return virObjectEventStateRegisterID(conn, state, net ? net->uuid : NULL,
+                                         virNetworkEventClass, eventID,
+                                         cb, opaque, freecb, callbackID);
 }
 
 

diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index 59f375909e6fc67e7ebbefe03d93a75d7b056f92..b01ffe554c0aa21510824cd47f51f3c4687e4478 100644 (file)
--- a/src/conf/object_event.c
+++ b/src/conf/object_event.c
@@ -66,7 +66,8 @@ struct _virObjectEventCallback {
     virClassPtr klass;
     int eventID;
     virConnectPtr conn;
-    virObjectMetaPtr meta;
+    bool uuid_filter;
+    unsigned char uuid[VIR_UUID_BUFLEN];
     virConnectObjectEventGenericCallback cb;
     void *opaque;
     virFreeCallback freecb;
@@ -201,9 +202,6 @@ virObjectEventCallbackListRemoveID(virConnectPtr conn,
             if (cb->freecb)
                 (*cb->freecb)(cb->opaque);
             virObjectUnref(cb->conn);
-            if (cb->meta)
-                VIR_FREE(cb->meta->name);
-            VIR_FREE(cb->meta);
             VIR_FREE(cb);
             VIR_DELETE_ELEMENT(cbList->callbacks, i, cbList->count);
 
@@ -307,9 +305,9 @@ virObjectEventCallbackLookup(virConnectPtr conn,
             cb->eventID == eventID &&
             cb->conn == conn &&
             cb->legacy == legacy &&
-            ((uuid && cb->meta &&
-              memcmp(cb->meta->uuid, uuid, VIR_UUID_BUFLEN) == 0) ||
-             (!uuid && !cb->meta))) {
+            ((uuid && cb->uuid_filter &&
+              memcmp(cb->uuid, uuid, VIR_UUID_BUFLEN) == 0) ||
+             (!uuid && !cb->uuid_filter))) {
             ret = cb->callbackID;
             break;
         }
@@ -323,8 +321,6 @@ virObjectEventCallbackLookup(virConnectPtr conn,
  * @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 add
@@ -338,8 +334,6 @@ static int
 virObjectEventCallbackListAddID(virConnectPtr conn,
                                 virObjectEventCallbackListPtr cbList,
                                 unsigned char uuid[VIR_UUID_BUFLEN],
-                                const char *name,
-                                int id,
                                 virClassPtr klass,
                                 int eventID,
                                 virConnectObjectEventGenericCallback callback,
@@ -350,10 +344,9 @@ virObjectEventCallbackListAddID(virConnectPtr conn,
     virObjectEventCallbackPtr event;
     int ret = -1;
 
-    VIR_DEBUG("conn=%p cblist=%p uuid=%p name=%s id=%d "
+    VIR_DEBUG("conn=%p cblist=%p uuid=%p "
               "klass=%p eventID=%d callback=%p opaque=%p",
-              conn, cbList, uuid, name, id, klass, eventID,
-              callback, opaque);
+              conn, cbList, uuid, klass, eventID, callback, opaque);
 
     /* Check incoming */
     if (!cbList) {
@@ -379,13 +372,12 @@ virObjectEventCallbackListAddID(virConnectPtr conn,
     event->opaque = opaque;
     event->freecb = freecb;
 
-    if (name && uuid && id > 0) {
-        if (VIR_ALLOC(event->meta) < 0)
-            goto cleanup;
-        if (VIR_STRDUP(event->meta->name, name) < 0)
-            goto cleanup;
-        memcpy(event->meta->uuid, uuid, VIR_UUID_BUFLEN);
-        event->meta->id = id;
+    /* Only need 'uuid' for matching; 'id' can change as domain
+     * switches between running and shutoff, and 'name' can change in
+     * Xen migration.  */
+    if (uuid) {
+        event->uuid_filter = true;
+        memcpy(event->uuid, uuid, VIR_UUID_BUFLEN);
     }
 
     if (callbackID)
@@ -399,12 +391,8 @@ virObjectEventCallbackListAddID(virConnectPtr conn,
     ret = virObjectEventCallbackListCount(conn, cbList, klass, eventID);
 
 cleanup:
-    if (event) {
+    if (event)
         virObjectUnref(event->conn);
-        if (event->meta)
-            VIR_FREE(event->meta->name);
-        VIR_FREE(event->meta);
-    }
     VIR_FREE(event);
     return ret;
 }
@@ -667,14 +655,14 @@ virObjectEventDispatchMatchCallback(virObjectEventPtr event,
     if (cb->eventID != event->eventID)
         return false;
 
-    if (cb->meta) {
+    if (cb->uuid_filter) {
         /* Deliberately ignoring 'id' for matching, since that
          * will cause problems when a domain switches between
          * running & shutoff states & ignoring 'name' since
          * Xen sometimes renames guests during migration, thus
          * leaving 'uuid' as the only truly reliable ID we can use. */
 
-        return memcmp(event->meta.uuid, cb->meta->uuid, VIR_UUID_BUFLEN) == 0;
+        return memcmp(event->meta.uuid, cb->uuid, VIR_UUID_BUFLEN) == 0;
     }
     return true;
 }
@@ -786,8 +774,6 @@ virObjectEventStateFlush(virObjectEventStatePtr state)
  * @conn: connection to associate with callback
  * @state: domain event state
  * @uuid: uuid of the object for event filtering
- * @name: name of the object for event filtering
- * @id: id of the object for event filtering, or 0
  * @klass: the base event class
  * @eventID: ID of the event type to register for
  * @cb: function to invoke when event occurs
@@ -805,8 +791,6 @@ int
 virObjectEventStateRegisterID(virConnectPtr conn,
                               virObjectEventStatePtr state,
                               unsigned char *uuid,
-                              const char *name,
-                              int id,
                               virClassPtr klass,
                               int eventID,
                               virConnectObjectEventGenericCallback cb,
@@ -830,7 +814,7 @@ virObjectEventStateRegisterID(virConnectPtr conn,
     }
 
     ret = virObjectEventCallbackListAddID(conn, state->callbacks,
-                                          uuid, name, id, klass, eventID,
+                                          uuid, klass, eventID,
                                           cb, opaque, freecb,
                                           callbackID);
 

diff --git a/src/conf/object_event.h b/src/conf/object_event.h
index fa1281c6c1f2363bad93dee0f6f40935b0f6f738..39e92d50c97506b2ec53c62db3f3396fa2148808 100644 (file)
--- a/src/conf/object_event.h
+++ b/src/conf/object_event.h
@@ -71,15 +71,14 @@ int
 virObjectEventStateRegisterID(virConnectPtr conn,
                               virObjectEventStatePtr state,
                               unsigned char *uuid,
-                              const char *name,
-                              int id,
                               virClassPtr klass,
                               int eventID,
                               virConnectObjectEventGenericCallback cb,
                               void *opaque,
                               virFreeCallback freecb,
                               int *callbackID)
-    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(8);
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4)
+    ATTRIBUTE_NONNULL(6);
 int
 virObjectEventStateDeregisterID(virConnectPtr conn,
                                 virObjectEventStatePtr state,

diff --git a/src/conf/object_event_private.h b/src/conf/object_event_private.h
index 76bd6d154ef94521e6b129bb330524057b6f101e..571fc40f708247a39373c831c9832f32259cf1e8 100644 (file)
--- a/src/conf/object_event_private.h
+++ b/src/conf/object_event_private.h
@@ -69,6 +69,8 @@ virObjectEventNew(virClassPtr klass,
                   int eventID,
                   int id,
                   const char *name,
-                  const unsigned char *uuid);
+                  const unsigned char *uuid)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5)
+    ATTRIBUTE_NONNULL(6);
 
 #endif

diff --git a/tests/objecteventtest.c b/tests/objecteventtest.c
index cc39dcf715c9aef2d18d51cbfe61e1abddedb7ff..65642a24595954e1565663944b1044cc23fce0b6 100644 (file)
--- a/tests/objecteventtest.c
+++ b/tests/objecteventtest.c
@@ -223,7 +223,7 @@ testDomainCreateXMLMixed(const void *data)
     /* 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);
+    dom = virDomainDefineXML(test->conn, domainDef);
     if (dom == NULL)
         goto cleanup;
 
@@ -233,8 +233,6 @@ testDomainCreateXMLMixed(const void *data)
                                            &counter, NULL);
     if (id1 < 0)
         goto cleanup;
-    if (virDomainDestroy(dom) < 0)
-        goto cleanup;
     if (virConnectDomainEventRegister(test->conn,
                                       domainLifecycleCb,
                                       &counter, NULL) != 0)