[libvirt] [PATCH] 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 register an unregistered object, 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(a)redhat.com&gt; --- src/conf/object_event.c | 57 ++++++++++++++++++++++------------------------ src/remote/remote_driver.c | 56 ++++++++++++++++++++++----------------------- 2 files changed, 55 insertions(+), 58 deletions(-) diff --git a/src/conf/object_event.c b/src/conf/object_event.c index b01ffe5..b69387a 100644 --- 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 fecb9b2..64d9d92 100644 --- 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 */ -- 1.8.4.2