On Mon, Feb 01, 2016 at 03:40:20PM +0300, Nikolay Shirokovskiy wrote:
> 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.
We could sanity check the callback before unregistering the
remote part. Or you could do the local unregister first since
if the remote part then fails, it is harmless - we'll see the
close event frm the server still, but we won't dispatch it.
> 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?
We can't remove it since that would change ABI.
> ---
> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list