Re: [libvirt] [PATCH v3 7/8] daemon: add connection close rpc

remoteConnectUnregisterCloseCallback is not quite good. if it is given a callback function different from that was registered before then local part will fail silently. On the other hand we can not gracefully handle this fail as the remote part is already unregistered.

There are a lot of options to fix it. I think of totally removing the callback argument from unregistering. What's the use of it?

--- daemon/libvirtd.h | 1 + daemon/remote.c | 84 ++++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 52 ++++++++++++++++++++++++--- src/remote/remote_protocol.x | 24 ++++++++++++- src/remote_protocol-structs | 6 ++++ 5 files changed, 161 insertions(+), 6 deletions(-) diff --git a/daemon/libvirtd.h b/daemon/libvirtd.h index efd4823..7271b0f 100644 --- a/daemon/libvirtd.h +++ b/daemon/libvirtd.h @@ -60,6 +60,7 @@ struct daemonClientPrivate { size_t nnetworkEventCallbacks; daemonClientEventCallbackPtr *qemuEventCallbacks; size_t nqemuEventCallbacks; + bool closeRegistered; # if WITH_SASL virNetSASLSessionPtr sasl; diff --git a/daemon/remote.c b/daemon/remote.c index 370f442..bea1996 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1221,6 +1221,20 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn, VIR_FREE(details_p); } +static +void remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, int reason, void *opaque) +{ + virNetServerClientPtr client = opaque; + + VIR_DEBUG("Relaying connection closed event, reason %d", reason); + + remote_connect_event_connection_closed_msg msg = { reason }; + remoteDispatchObjectEventSend(client, remoteProgram, + REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED, + (xdrproc_t)xdr_remote_connect_event_connection_closed_msg, + &msg); +} + /* * You must hold lock for at least the client * We don't free stuff here, merely disconnect the client's @@ -1283,6 +1297,12 @@ void remoteClientFreeFunc(void *data) } VIR_FREE(priv->qemuEventCallbacks); + if (priv->closeRegistered) { + if (virConnectUnregisterCloseCallback(priv->conn, + remoteRelayConnectionClosedEvent) < 0) + VIR_WARN("unexpected close callback event deregister failure"); + } + virConnectClose(priv->conn); virIdentitySetCurrent(NULL); @@ -3515,6 +3535,70 @@ remoteDispatchNodeDeviceGetParent(virNetServerPtr server ATTRIBUTE_UNUSED, return rv; } +static int +remoteDispatchConnectCloseCallbackRegister(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr) +{ + int rv = -1; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + virMutexLock(&priv->lock); + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + // on behalf of close callback + virObjectRef(client); + if (virConnectRegisterCloseCallback(priv->conn, + remoteRelayConnectionClosedEvent, + client, virObjectFreeCallback) < 0) + goto cleanup; + + priv->closeRegistered = true; + rv = 0; + + cleanup: + virMutexUnlock(&priv->lock); + if (rv < 0) + virNetMessageSaveError(rerr); + return rv; +} + +static int +remoteDispatchConnectCloseCallbackUnregister(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr) +{ + int rv = -1; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + virMutexLock(&priv->lock); + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (virConnectUnregisterCloseCallback(priv->conn, + remoteRelayConnectionClosedEvent) < 0) + goto cleanup; + + priv->closeRegistered = false; + rv = 0; + + cleanup: + virMutexUnlock(&priv->lock); + if (rv < 0) + virNetMessageSaveError(rerr); + return rv; +} /*************************** * Register / deregister events diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index f64ce4d..a9b451b 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -352,6 +352,11 @@ remoteNetworkBuildEventLifecycle(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, virNetClientPtr client ATTRIBUTE_UNUSED, void *evdata, void *opaque); +static void +remoteConnectNotifyEventConnectionClosed(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, + virNetClientPtr client ATTRIBUTE_UNUSED, + void *evdata, void *opaque); + static virNetClientProgramEvent remoteEvents[] = { { REMOTE_PROC_DOMAIN_EVENT_LIFECYCLE, remoteDomainBuildEventLifecycle, @@ -514,8 +519,23 @@ static virNetClientProgramEvent remoteEvents[] = { remoteDomainBuildEventCallbackMigrationIteration, sizeof(remote_domain_event_callback_migration_iteration_msg), (xdrproc_t)xdr_remote_domain_event_callback_migration_iteration_msg }, + { REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED, + remoteConnectNotifyEventConnectionClosed, + sizeof(remote_connect_event_connection_closed_msg), + (xdrproc_t)xdr_remote_connect_event_connection_closed_msg }, }; +static void +remoteConnectNotifyEventConnectionClosed(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, + virNetClientPtr client ATTRIBUTE_UNUSED, + void *evdata, void *opaque) +{ + virConnectPtr conn = opaque; + struct private_data *priv = conn->privateData; + remote_connect_event_connection_closed_msg *msg = evdata; + + virConnectCloseCallbackDataCall(priv->closeCallback, msg->reason); +} static void remoteDomainBuildQemuMonitorEvent(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, @@ -8074,10 +8094,21 @@ remoteConnectRegisterCloseCallback(virConnectPtr conn, int ret = -1; remoteDriverLock(priv); - ret = virConnectCloseCallbackDataRegister(priv->closeCallback, conn, cb, - opaque, freecb); - remoteDriverUnlock(priv); + if (virConnectCloseCallbackDataRegister(priv->closeCallback, conn, cb, + opaque, freecb) < 0) + goto cleanup; + + if (call(conn, priv, 0, REMOTE_PROC_CONNECT_CLOSE_CALLBACK_REGISTER, + (xdrproc_t) xdr_void, (char *) NULL, + (xdrproc_t) xdr_void, (char *) NULL) == -1) { + virConnectCloseCallbackDataUnregister(priv->closeCallback, cb); + goto cleanup; + } + + ret = 0; + cleanup: + remoteDriverUnlock(priv); return ret; } @@ -8089,9 +8120,20 @@ remoteConnectUnregisterCloseCallback(virConnectPtr conn, int ret = -1; remoteDriverLock(priv); - ret = virConnectCloseCallbackDataUnregister(priv->closeCallback, cb); - remoteDriverUnlock(priv); + if (call(conn, priv, 0, REMOTE_PROC_CONNECT_CLOSE_CALLBACK_UNREGISTER, + (xdrproc_t) xdr_void, (char *) NULL, + (xdrproc_t) xdr_void, (char *) NULL) == -1) { + goto cleanup; + } + + // hopefully nobody will call us with different callback + // or we will fail here + virConnectCloseCallbackDataUnregister(priv->closeCallback, cb); + + ret = 0; + cleanup: + remoteDriverUnlock(priv); return ret; } diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index bfdbce7..c6dd51e 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -3045,6 +3045,10 @@ struct remote_domain_event_callback_device_added_msg { remote_nonnull_string devAlias; }; +struct remote_connect_event_connection_closed_msg { + int reason; +}; + struct remote_connect_get_cpu_model_names_args { remote_nonnull_string arch; int need_results; @@ -5706,5 +5710,23 @@ enum remote_procedure { * @generate: both * @acl: none */ - REMOTE_PROC_DOMAIN_EVENT_CALLBACK_MIGRATION_ITERATION = 359 + REMOTE_PROC_DOMAIN_EVENT_CALLBACK_MIGRATION_ITERATION = 359, + + /** + * @generate: none + * @acl: none + */ + REMOTE_PROC_CONNECT_CLOSE_CALLBACK_REGISTER = 360, + + /** + * @generate: none + * @acl: none + */ + REMOTE_PROC_CONNECT_CLOSE_CALLBACK_UNREGISTER = 361, + + /** + * @generate: none + * @acl: none + */ + REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED = 362 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index dff54e8..11048b7 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -2502,6 +2502,9 @@ struct remote_domain_event_callback_device_added_msg { remote_nonnull_domain dom; remote_nonnull_string devAlias; }; +struct remote_connect_event_connection_closed_msg { + int reason; +}; struct remote_connect_get_cpu_model_names_args { remote_nonnull_string arch; int need_results; @@ -3057,4 +3060,7 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SET_USER_PASSWORD = 357, REMOTE_PROC_DOMAIN_RENAME = 358, REMOTE_PROC_DOMAIN_EVENT_CALLBACK_MIGRATION_ITERATION = 359, + REMOTE_PROC_CONNECT_CLOSE_CALLBACK_REGISTER = 360, + REMOTE_PROC_CONNECT_CLOSE_CALLBACK_UNREGISTER = 361, + REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED = 362, }; -- 1.8.3.1 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list