event: clean up client side RPC code

Commit cfd62c1 was incomplete; I found more cases where error
messages were being overwritten, and where the code between
the three registration/deregistration APIs was not consistent.

Since it is fairly easy to trigger an attempt to deregister an
unregistered object through public API, I also changed the error
message from VIR_ERR_INTERNAL_ERROR to VIR_ERR_INVALID_ARG.

* src/conf/object_event.c (virObjectEventCallbackListEventID):
Inline...
(virObjectEventStateEventID): ...into lone caller, and report
error on failure.
(virObjectEventCallbackListAddID, virObjectEventStateCallbackID)
(virObjectEventCallbackListRemoveID)
(virObjectEventCallbackListMarkDeleteID): Tweak error category.
* src/remote/remote_driver.c (remoteConnectDomainEventRegister):
Don't leak registration on failure.
(remoteConnectDomainEventDeregisterAny)
(remoteConnectNetworkEventDeregisterAny): Don't overwrite error.

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

diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index b01ffe554c0aa21510824cd47f51f3c4687e4478..b69387ad1f34102d66ef9362ec84ea35fd739a02 100644 (file)
--- a/src/conf/object_event.c
+++ b/src/conf/object_event.c
@@ -210,8 +210,9 @@ virObjectEventCallbackListRemoveID(virConnectPtr conn,
         }
     }
 
-    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                   _("could not find event callback for removal"));
+    virReportError(VIR_ERR_INVALID_ARG,
+                   _("could not find event callback %d for deletion"),
+                   callbackID);
     return -1;
 }
 
@@ -233,8 +234,9 @@ virObjectEventCallbackListMarkDeleteID(virConnectPtr conn,
         }
     }
 
-    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                   _("could not find event callback for deletion"));
+    virReportError(VIR_ERR_INVALID_ARG,
+                   _("could not find event callback %d for deletion"),
+                   callbackID);
     return -1;
 }
 
@@ -357,7 +359,7 @@ virObjectEventCallbackListAddID(virConnectPtr conn,
     if (virObjectEventCallbackLookup(conn, cbList, uuid,
                                      klass, eventID, callback,
                                      !callbackID) != -1) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+        virReportError(VIR_ERR_INVALID_ARG, "%s",
                        _("event callback already tracked"));
         return -1;
     }
@@ -398,27 +400,6 @@ cleanup:
 }
 
 
-static int
-virObjectEventCallbackListEventID(virConnectPtr conn,
-                                  virObjectEventCallbackListPtr cbList,
-                                  int callbackID)
-{
-    size_t i;
-
-    for (i = 0; i < cbList->count; i++) {
-        virObjectEventCallbackPtr cb = cbList->callbacks[i];
-
-        if (cb->deleted)
-            continue;
-
-        if (cb->callbackID == callbackID && cb->conn == conn)
-            return cb->eventID;
-    }
-
-    return -1;
-}
-
-
 /**
  * virObjectEventQueueClear:
  * @queue: pointer to the queue
@@ -897,7 +878,7 @@ virObjectEventStateCallbackID(virConnectPtr conn,
     virObjectEventStateUnlock(state);
 
     if (ret < 0)
-        virReportError(VIR_ERR_INTERNAL_ERROR,
+        virReportError(VIR_ERR_INVALID_ARG,
                        _("event callback function %p not registered"),
                        callback);
     return ret;
@@ -920,11 +901,27 @@ virObjectEventStateEventID(virConnectPtr conn,
                            virObjectEventStatePtr state,
                            int callbackID)
 {
-    int ret;
+    int ret = -1;
+    size_t i;
+    virObjectEventCallbackListPtr cbList = state->callbacks;
 
     virObjectEventStateLock(state);
-    ret = virObjectEventCallbackListEventID(conn,
-                                            state->callbacks, callbackID);
+    for (i = 0; i < cbList->count; i++) {
+        virObjectEventCallbackPtr cb = cbList->callbacks[i];
+
+        if (cb->deleted)
+            continue;
+
+        if (cb->callbackID == callbackID && cb->conn == conn) {
+            ret = cb->eventID;
+            break;
+        }
+    }
     virObjectEventStateUnlock(state);
+
+    if (ret < 0)
+        virReportError(VIR_ERR_INVALID_ARG,
+                       _("event callback id %d not registered"),
+                       callbackID);
     return ret;
 }

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index fecb9b2e6a1a76a4f98437c776ccf115a7ae91bf..64d9d92334157e3d7f824a333fa0719401a0895a 100644 (file)
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -2911,6 +2911,7 @@ done:
     return rv;
 }
 
+
 static int
 remoteConnectNetworkEventRegisterAny(virConnectPtr conn,
                                      virNetworkPtr net,
@@ -2968,16 +2969,12 @@ remoteConnectNetworkEventDeregisterAny(virConnectPtr conn,
     remoteDriverLock(priv);
 
     if ((eventID = virObjectEventStateEventID(conn, priv->eventState,
-                                              callbackID)) < 0) {
-        virReportError(VIR_ERR_RPC, _("unable to find callback ID %d"), callbackID);
+                                              callbackID)) < 0)
         goto done;
-    }
 
     if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
-                                                 callbackID)) < 0) {
-        virReportError(VIR_ERR_RPC, _("unable to find callback ID %d"), callbackID);
+                                                 callbackID)) < 0)
         goto done;
-    }
 
     /* If that was the last callback for this eventID, we need to disable
      * events on the server */
@@ -2997,6 +2994,7 @@ done:
     return rv;
 }
 
+
 static int
 remoteConnectListAllInterfaces(virConnectPtr conn,
                                virInterfacePtr **ifaces,
@@ -4405,10 +4403,11 @@ out:
 #endif /* WITH_POLKIT */
 /*----------------------------------------------------------------------*/
 
-static int remoteConnectDomainEventRegister(virConnectPtr conn,
-                                            virConnectDomainEventCallback callback,
-                                            void *opaque,
-                                            virFreeCallback freecb)
+static int
+remoteConnectDomainEventRegister(virConnectPtr conn,
+                                 virConnectDomainEventCallback callback,
+                                 void *opaque,
+                                 virFreeCallback freecb)
 {
     int rv = -1;
     struct private_data *priv = conn->privateData;
@@ -4421,11 +4420,13 @@ static int remoteConnectDomainEventRegister(virConnectPtr conn,
          goto done;
 
     if (count == 1) {
-        /* Tell the server when we are the first callback deregistering */
+        /* Tell the server when we are the first callback registering */
         if (call(conn, priv, 0, REMOTE_PROC_CONNECT_DOMAIN_EVENT_REGISTER,
                  (xdrproc_t) xdr_void, (char *) NULL,
-                 (xdrproc_t) xdr_void, (char *) NULL) == -1)
+                 (xdrproc_t) xdr_void, (char *) NULL) == -1) {
+            virDomainEventStateDeregister(conn, priv->eventState, callback);
             goto done;
+        }
     }
 
     rv = 0;
@@ -4435,8 +4436,9 @@ done:
     return rv;
 }
 
-static int remoteConnectDomainEventDeregister(virConnectPtr conn,
-                                              virConnectDomainEventCallback callback)
+static int
+remoteConnectDomainEventDeregister(virConnectPtr conn,
+                                   virConnectDomainEventCallback callback)
 {
     struct private_data *priv = conn->privateData;
     int rv = -1;
@@ -5205,12 +5207,13 @@ static virStreamDriver remoteStreamDrv = {
 };
 
 
-static int remoteConnectDomainEventRegisterAny(virConnectPtr conn,
-                                               virDomainPtr dom,
-                                               int eventID,
-                                               virConnectDomainEventGenericCallback callback,
-                                               void *opaque,
-                                               virFreeCallback freecb)
+static int
+remoteConnectDomainEventRegisterAny(virConnectPtr conn,
+                                    virDomainPtr dom,
+                                    int eventID,
+                                    virConnectDomainEventGenericCallback callback,
+                                    void *opaque,
+                                    virFreeCallback freecb)
 {
     int rv = -1;
     struct private_data *priv = conn->privateData;
@@ -5248,8 +5251,9 @@ done:
 }
 
 
-static int remoteConnectDomainEventDeregisterAny(virConnectPtr conn,
-                                                 int callbackID)
+static int
+remoteConnectDomainEventDeregisterAny(virConnectPtr conn,
+                                      int callbackID)
 {
     struct private_data *priv = conn->privateData;
     int rv = -1;
@@ -5260,16 +5264,12 @@ static int remoteConnectDomainEventDeregisterAny(virConnectPtr conn,
     remoteDriverLock(priv);
 
     if ((eventID = virObjectEventStateEventID(conn, priv->eventState,
-                                              callbackID)) < 0) {
-        virReportError(VIR_ERR_RPC, _("unable to find callback ID %d"), callbackID);
+                                              callbackID)) < 0)
         goto done;
-    }
 
     if ((count = virObjectEventStateDeregisterID(conn, priv->eventState,
-                                                 callbackID)) < 0) {
-        virReportError(VIR_ERR_RPC, _("unable to find callback ID %d"), callbackID);
+                                                 callbackID)) < 0)
         goto done;
-    }
 
     /* If that was the last callback for this eventID, we need to disable
      * events on the server */