[libvirt] [PATCH v2 0/3] driver level connection close event
Notify of connection close event from parallels driver (possibly) wrapped in the remote driver. Changes from v1: 1. fix comment style issues 2. remove spurious whitespaces 3. move rpc related part from vz patch to second(rpc) patch 4. remove unnecessary locks for immutable closeCallback in first patch. Discussion. In 1 and 2 patch we forced to some decisions because we don't have a weak reference mechanics. 1 patch. ----------- virConnectCloseCallback is introduced because we can not reference the connection object itself when setting a network layer callback because of how connection close works. A connection close procedure is next: 1. client closes connection 2. a this point nobody else referencing a connection and it is disposed 3. connection dispose unreferencing network connection 4. network connection disposes Thus if we referece a connection in network close callback we never get step 2. virConnectCloseCallback broke this cycle but at cost that clients MUST unregister explicitly before closing connection. This is not good as this unregistration is not really neaded. Client is not telling that it does not want to receive events anymore but rather forced to obey some implementation-driven rules. 2 patch. ----------- We impose requirements on driver implementations which is fragile. Moreover we again need to make explicit unregistrations. Implementation of domain events illustrates this point. remoteDispatchConnectDomainEventRegister does not reference NetClient and makes unregistration before NetClient is disposed but drivers do not meet the formulated requirements. Object event system release lock before delivering event for re-entrance purposes. Shortly we have 2 undesired consequences here. 1. Mandatory unregistration. 2. Imposing multi-threading requirements. Introduction of weak pointers could free us from these artifacts. Next weak reference workflow illustrates this. 1. Take weak reference on object of interest before passing to party. This doesn't break disposing mechanics as weak eference does not prevent from disposing object. Object is disposed but memory is not freed yet if there are weak references. 2. When callback is called we are safe to check if pointer dangling as we make a weak reference before. 3. Release weak reference and this trigger memory freeing if there are no more weak references. daemon/libvirtd.h | 1 + daemon/remote.c | 86 +++++++++++++++++++++++++++++++ src/datatypes.c | 115 +++++++++++++++++++++++++++++++---------- src/datatypes.h | 21 ++++++-- src/driver-hypervisor.h | 12 ++++ src/libvirt-host.c | 77 +++++++++------------------- src/remote/remote_driver.c | 106 +++++++++++++++++++++++++++++--------- src/remote/remote_protocol.x | 24 ++++++++- src/remote_protocol-structs | 6 ++ src/vz/vz_driver.c | 26 +++++++++ src/vz/vz_sdk.c | 29 +++++++++++ src/vz/vz_utils.h | 3 +
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> 1. Introduce connect(Un)RegisterCloseCallback driver functions. 2. virConnect(Un)RegisterCloseCallback now works through driver. 3. virConnectCloseCallback is factored from virConnect but mostly stay the same. Notice however that virConnect object is not referenced in virConnectCloseCallback anymore. It is safe. Explanation. Previous version of callback object keeps reference to connection. This leads to undocumented rule that all clients must exlicitly unregister close callback before closing connection or connection will never be disposed. As callback unregistering and close event delivering are serialized thru callback object lock and unregistering zeros connection object we will never get dangling pointer on delivering. 4. callback object doesn't check callback on unregistering. The reason is that it will helps us write registering/unregistering with atomic behaviour for remote driver as it can be seen in next patch. Moreover it is not really meaningful to check callback on unregistering. 5. virNetClientSetCloseCallback call is removed from doRemoteClose as it is excessive for the same reasons as in point 3. Unregistering MUST be called and this prevents from firing event on close initiated by client. I'm not sure where callback object should be so it stays in datatype.c Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/datatypes.c | 115 +++++++++++++++++++++++++++++++++----------- src/datatypes.h | 21 ++++++-- src/driver-hypervisor.h | 12 +++++ src/libvirt-host.c | 77 ++++++++++-------------------- src/remote/remote_driver.c | 57 ++++++++++++---------- 5 files changed, 171 insertions(+), 111 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index 12bcfc1..87a42cb 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -34,7 +34,7 @@ VIR_LOG_INIT("datatypes"); virClassPtr virConnectClass; -virClassPtr virConnectCloseCallbackDataClass; +virClassPtr virConnectCloseCallbackClass; virClassPtr virDomainClass; virClassPtr virDomainSnapshotClass; virClassPtr virInterfaceClass; @@ -47,7 +47,7 @@ virClassPtr virStorageVolClass; virClassPtr virStoragePoolClass; static void virConnectDispose(void *obj); -static void virConnectCloseCallbackDataDispose(void *obj); +static void virConnectCloseCallbackDispose(void *obj); static void virDomainDispose(void *obj); static void virDomainSnapshotDispose(void *obj); static void virInterfaceDispose(void *obj); @@ -78,7 +78,7 @@ virDataTypesOnceInit(void) DECLARE_CLASS_COMMON(basename, virClassForObjectLockable()) DECLARE_CLASS_LOCKABLE(virConnect); - DECLARE_CLASS_LOCKABLE(virConnectCloseCallbackData); + DECLARE_CLASS_LOCKABLE(virConnectCloseCallback); DECLARE_CLASS(virDomain); DECLARE_CLASS(virDomainSnapshot); DECLARE_CLASS(virInterface); @@ -119,14 +119,7 @@ virGetConnect(void) if (!(ret = virObjectLockableNew(virConnectClass))) return NULL; - if (!(ret->closeCallback = virObjectLockableNew(virConnectCloseCallbackDataClass))) - goto error; - return ret; - - error: - virObjectUnref(ret); - return NULL; } /** @@ -147,36 +140,102 @@ virConnectDispose(void *obj) virResetError(&conn->err); virURIFree(conn->uri); +} - if (conn->closeCallback) { - virObjectLock(conn->closeCallback); - conn->closeCallback->callback = NULL; - virObjectUnlock(conn->closeCallback); +virConnectCloseCallbackPtr +virGetConnectCloseCallback(void) +{ + virConnectCloseCallbackPtr ret; - virObjectUnref(conn->closeCallback); - } + if (virDataTypesInitialize() < 0) + return NULL; + + if (!(ret = virObjectLockableNew(virConnectCloseCallbackClass))) + return NULL; + + return ret; } +static void +virConnectCloseCallbackClean(virConnectCloseCallbackPtr obj) +{ + if (obj->freeCallback) + obj->freeCallback(obj->opaque); + + obj->callback = NULL; + obj->freeCallback = NULL; + obj->opaque = NULL; + obj->conn = NULL; +} -/** - * virConnectCloseCallbackDataDispose: - * @obj: the close callback data to release - * - * Release resources bound to the connection close callback. - */ static void -virConnectCloseCallbackDataDispose(void *obj) +virConnectCloseCallbackDispose(void *obj ATTRIBUTE_UNUSED) +{ + /* nothing really to do here */ +} + +int +virConnectCloseCallbackRegister(virConnectCloseCallbackPtr obj, + virConnectPtr conn, + virConnectCloseFunc cb, + void *opaque, + virFreeCallback freecb) { - virConnectCloseCallbackDataPtr cb = obj; + int ret = -1; - virObjectLock(cb); + virObjectLock(obj); - if (cb->freeCallback) - cb->freeCallback(cb->opaque); + if (obj->callback) { + /* Temporarily remove reporting to fix syntax-check. + Proper resolve of this will be to decide a new file location + for connection object */ - virObjectUnlock(cb); + /* report "A close callback is already registered" */ + goto finish; + } + + /* connection is not referenced, thus clients MUST call + unregister before closing connection and + as delivering event and unregistering are serialized + this is safe */ + obj->conn = conn; + obj->callback = cb; + obj->opaque = opaque; + obj->freeCallback = freecb; + + ret = 0; + + finish: + virObjectUnlock(obj); + return ret; } +void +virConnectCloseCallbackUnregister(virConnectCloseCallbackPtr obj) +{ + virObjectLock(obj); + virConnectCloseCallbackClean(obj); + virObjectUnlock(obj); +} + +void +virConnectCloseCallbackCall(virConnectCloseCallbackPtr obj, int reason) +{ + virObjectLock(obj); + + /* in triggered on unregistered state */ + if (!obj->callback) + goto finish; + + VIR_DEBUG("Triggering connection close callback %p reason=%d, opaque=%p", + obj->callback, reason, obj->opaque); + + obj->callback(obj->conn, reason, obj->opaque); + virConnectCloseCallbackClean(obj); + + finish: + virObjectUnlock(obj); +} /** * virGetDomain: diff --git a/src/datatypes.h b/src/datatypes.h index c498cb0..86230e7 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -329,13 +329,13 @@ extern virClassPtr virAdmConnectClass; __VA_ARGS__) -typedef struct _virConnectCloseCallbackData virConnectCloseCallbackData; -typedef virConnectCloseCallbackData *virConnectCloseCallbackDataPtr; +typedef struct _virConnectCloseCallback virConnectCloseCallback; +typedef virConnectCloseCallback *virConnectCloseCallbackPtr; /** * Internal structure holding data related to connection close callbacks. */ -struct _virConnectCloseCallbackData { +struct _virConnectCloseCallback { virObjectLockable parent; virConnectPtr conn; @@ -385,9 +385,6 @@ struct _virConnect { virError err; /* the last error */ virErrorFunc handler; /* associated handlet */ void *userData; /* the user data */ - - /* Per-connection close callback */ - virConnectCloseCallbackDataPtr closeCallback; }; /** @@ -586,4 +583,16 @@ virDomainSnapshotPtr virGetDomainSnapshot(virDomainPtr domain, virAdmConnectPtr virAdmConnectNew(void); +virConnectCloseCallbackPtr virGetConnectCloseCallback(void); +int +virConnectCloseCallbackRegister(virConnectCloseCallbackPtr obj, + virConnectPtr conn, + virConnectCloseFunc cb, + void *opaque, + virFreeCallback freecb); +void +virConnectCloseCallbackUnregister(virConnectCloseCallbackPtr obj); +void +virConnectCloseCallbackCall(virConnectCloseCallbackPtr obj, int reason); + #endif /* __VIR_DATATYPES_H__ */ diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index 3275343..795ab79 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -1207,6 +1207,16 @@ typedef int const char *password, unsigned int flags); +typedef int +(*virDrvConnectRegisterCloseCallback)(virConnectPtr conn, + virConnectCloseFunc cb, + void *opaque, + virFreeCallback freecb); + +typedef int +(*virDrvConnectUnregisterCloseCallback)(virConnectPtr conn, + virConnectCloseFunc cb); + typedef struct _virHypervisorDriver virHypervisorDriver; typedef virHypervisorDriver *virHypervisorDriverPtr; @@ -1437,6 +1447,8 @@ struct _virHypervisorDriver { virDrvDomainGetFSInfo domainGetFSInfo; virDrvDomainInterfaceAddresses domainInterfaceAddresses; virDrvDomainSetUserPassword domainSetUserPassword; + virDrvConnectRegisterCloseCallback connectRegisterCloseCallback; + virDrvConnectUnregisterCloseCallback connectUnregisterCloseCallback; }; diff --git a/src/libvirt-host.c b/src/libvirt-host.c index 9c88426..7fcdab3 100644 --- a/src/libvirt-host.c +++ b/src/libvirt-host.c @@ -1211,44 +1211,30 @@ virConnectRegisterCloseCallback(virConnectPtr conn, void *opaque, virFreeCallback freecb) { - VIR_DEBUG("conn=%p", conn); + int ret; + + VIR_DEBUG("conn=%p cb=%p opaque=%p freecb=%p", + conn, cb, opaque, freecb); virResetLastError(); virCheckConnectReturn(conn, -1); - - virObjectRef(conn); + virCheckNonNullArgGoto(cb, finish); virObjectLock(conn); - virObjectLock(conn->closeCallback); - - virCheckNonNullArgGoto(cb, error); - - if (conn->closeCallback->callback) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("A close callback is already registered")); - goto error; - } - - conn->closeCallback->conn = conn; - conn->closeCallback->callback = cb; - conn->closeCallback->opaque = opaque; - conn->closeCallback->freeCallback = freecb; - - virObjectUnlock(conn->closeCallback); + if (conn->driver && conn->driver->connectRegisterCloseCallback) + ret = conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb); + else + ret = 0; virObjectUnlock(conn); - return 0; + finish: + if (ret < 0) + virDispatchError(conn); - error: - virObjectUnlock(conn->closeCallback); - virObjectUnlock(conn); - virDispatchError(conn); - virObjectUnref(conn); - return -1; + return ret; } - /** * virConnectUnregisterCloseCallback: * @conn: pointer to connection object @@ -1266,41 +1252,28 @@ int virConnectUnregisterCloseCallback(virConnectPtr conn, virConnectCloseFunc cb) { - VIR_DEBUG("conn=%p", conn); + int ret; + + VIR_DEBUG("conn=%p, cb=%p", conn, cb); virResetLastError(); virCheckConnectReturn(conn, -1); - - virObjectLock(conn); - virObjectLock(conn->closeCallback); - virCheckNonNullArgGoto(cb, error); - if (conn->closeCallback->callback != cb) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("A different callback was requested")); - goto error; - } - - conn->closeCallback->callback = NULL; - if (conn->closeCallback->freeCallback) - conn->closeCallback->freeCallback(conn->closeCallback->opaque); - conn->closeCallback->freeCallback = NULL; - - virObjectUnlock(conn->closeCallback); + virObjectLock(conn); + if (conn->driver && conn->driver->connectRegisterCloseCallback) + ret = conn->driver->connectUnregisterCloseCallback(conn, cb); + else + ret = 0; virObjectUnlock(conn); - virObjectUnref(conn); - - return 0; error: - virObjectUnlock(conn->closeCallback); - virObjectUnlock(conn); - virDispatchError(conn); - return -1; -} + if (ret < 0) + virDispatchError(conn); + return ret; +} /** * virNodeGetCPUMap: diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 273799b..e1138a8 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -101,6 +101,7 @@ struct private_data { bool serverEventFilter; /* Does server support modern event filtering */ virObjectEventStatePtr eventState; + virConnectCloseCallbackPtr closeCallback; }; enum { @@ -531,25 +532,7 @@ remoteClientCloseFunc(virNetClientPtr client ATTRIBUTE_UNUSED, int reason, void *opaque) { - virConnectCloseCallbackDataPtr cbdata = opaque; - - virObjectLock(cbdata); - - if (cbdata->callback) { - VIR_DEBUG("Triggering connection close callback %p reason=%d, opaque=%p", - cbdata->callback, reason, cbdata->opaque); - cbdata->callback(cbdata->conn, reason, cbdata->opaque); - - if (cbdata->freeCallback) - cbdata->freeCallback(cbdata->opaque); - cbdata->callback = NULL; - cbdata->freeCallback = NULL; - } - virObjectUnlock(cbdata); - - /* free the connection reference that comes along with the callback - * registration */ - virObjectUnref(cbdata->conn); + virConnectCloseCallbackCall(opaque, reason); } /* helper macro to ease extraction of arguments from the URI */ @@ -976,11 +959,14 @@ doRemoteOpen(virConnectPtr conn, goto failed; } - virObjectRef(conn->closeCallback); + if (!(priv->closeCallback = virGetConnectCloseCallback())) + goto failed; + /* ref on behalf of netclient */ + virObjectRef(priv->closeCallback); virNetClientSetCloseCallback(priv->client, remoteClientCloseFunc, - conn->closeCallback, virObjectFreeCallback); + priv->closeCallback, virObjectFreeCallback); if (!(priv->remoteProgram = virNetClientProgramNew(REMOTE_PROGRAM, REMOTE_PROTOCOL_VERSION, @@ -1107,6 +1093,8 @@ doRemoteOpen(virConnectPtr conn, virObjectUnref(priv->remoteProgram); virObjectUnref(priv->lxcProgram); virObjectUnref(priv->qemuProgram); + virObjectUnref(priv->closeCallback); + priv->closeCallback = NULL; virNetClientClose(priv->client); virObjectUnref(priv->client); priv->client = NULL; @@ -1240,17 +1228,15 @@ doRemoteClose(virConnectPtr conn, struct private_data *priv) priv->tls = NULL; #endif - virNetClientSetCloseCallback(priv->client, - NULL, - conn->closeCallback, virObjectFreeCallback); - virNetClientClose(priv->client); virObjectUnref(priv->client); priv->client = NULL; virObjectUnref(priv->remoteProgram); virObjectUnref(priv->lxcProgram); virObjectUnref(priv->qemuProgram); + virObjectUnref(priv->closeCallback); priv->remoteProgram = priv->qemuProgram = priv->lxcProgram = NULL; + priv->closeCallback = NULL; /* Free hostname copy */ VIR_FREE(priv->hostname); @@ -8041,6 +8027,25 @@ remoteDomainInterfaceAddresses(virDomainPtr dom, return rv; } +static int +remoteConnectRegisterCloseCallback(virConnectPtr conn, + virConnectCloseFunc cb, + void *opaque, + virFreeCallback freecb) +{ + struct private_data *priv = conn->privateData; + + return virConnectCloseCallbackRegister(priv->closeCallback, conn, cb, opaque, freecb); +} + +static int +remoteConnectUnregisterCloseCallback(virConnectPtr conn, virConnectCloseFunc cb ATTRIBUTE_UNUSED) +{ + struct private_data *priv = conn->privateData; + + virConnectCloseCallbackUnregister(priv->closeCallback); + return 0; +} /* get_nonnull_domain and get_nonnull_network turn an on-wire * (name, uuid) pair into virDomainPtr or virNetworkPtr object. @@ -8391,6 +8396,8 @@ static virHypervisorDriver hypervisor_driver = { .domainGetFSInfo = remoteDomainGetFSInfo, /* 1.2.11 */ .domainInterfaceAddresses = remoteDomainInterfaceAddresses, /* 1.2.14 */ .domainSetUserPassword = remoteDomainSetUserPassword, /* 1.2.16 */ + .connectRegisterCloseCallback = remoteConnectRegisterCloseCallback, /* 1.3.0 */ + .connectUnregisterCloseCallback = remoteConnectUnregisterCloseCallback, /* 1.3.0 */ }; static virNetworkDriver network_driver = { -- 1.7.1
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> Add conneciton close subscription/unsubscription and event rpc. Now remote driver firing connection close event in 2 cases. 1. connection to daemon closed, as previously 2. as a relay of connection close event from wrapped driver As it commented out in remoteDispatchConnectCloseCallbackRegister we impose some multi-thread requirements on drivers implementations. This is the same approach as in for example remoteDispatchConnectDomainEventRegister. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- daemon/libvirtd.h | 1 + daemon/remote.c | 86 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 53 +++++++++++++++++++++++++- src/remote/remote_protocol.x | 24 +++++++++++- src/remote_protocol-structs | 6 +++ 5 files changed, 167 insertions(+), 3 deletions(-) diff --git a/daemon/libvirtd.h b/daemon/libvirtd.h index 8c1a904..5b2d2ca 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 e9e2dca..366ecd5 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1189,6 +1189,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 @@ -1251,6 +1265,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); @@ -3489,6 +3509,72 @@ 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; + } + + /* NetClient is passed to driver but not referenced. + This imposes next requirements on drivers implementation. + Driver must serialize unregistering and event delivering operations. + Thus as we unregister callback before unreferencing NetClient + remoteRelayConnectionClosedEvent is safe to use NetClient. */ + if (virConnectRegisterCloseCallback(priv->conn, remoteRelayConnectionClosedEvent, client, NULL) < 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 e1138a8..b2cd1ea 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -347,6 +347,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, @@ -505,8 +510,23 @@ static virNetClientProgramEvent remoteEvents[] = { remoteDomainBuildEventCallbackDeviceAdded, sizeof(remote_domain_event_callback_device_added_msg), (xdrproc_t)xdr_remote_domain_event_callback_device_added_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; + + virConnectCloseCallbackCall(priv->closeCallback, msg->reason); +} static void remoteDomainBuildQemuMonitorEvent(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, @@ -8034,17 +8054,46 @@ remoteConnectRegisterCloseCallback(virConnectPtr conn, virFreeCallback freecb) { struct private_data *priv = conn->privateData; + int ret = -1; + + remoteDriverLock(priv); + + if (virConnectCloseCallbackRegister(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) { + virConnectCloseCallbackUnregister(priv->closeCallback); + goto cleanup; + } - return virConnectCloseCallbackRegister(priv->closeCallback, conn, cb, opaque, freecb); + ret = 0; + cleanup: + remoteDriverUnlock(priv); + return ret; } static int remoteConnectUnregisterCloseCallback(virConnectPtr conn, virConnectCloseFunc cb ATTRIBUTE_UNUSED) { struct private_data *priv = conn->privateData; + int ret = -1; + + remoteDriverLock(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; + } virConnectCloseCallbackUnregister(priv->closeCallback); - return 0; + + ret = 0; + cleanup: + remoteDriverUnlock(priv); + return ret; } /* get_nonnull_domain and get_nonnull_network turn an on-wire diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 9f1be6b..3d54a69 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -3063,6 +3063,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; @@ -5696,5 +5700,23 @@ enum remote_procedure { * @generate:both * @acl: domain:set_password */ - REMOTE_PROC_DOMAIN_SET_USER_PASSWORD = 357 + REMOTE_PROC_DOMAIN_SET_USER_PASSWORD = 357, + + /** + * @generate: none + * @acl: none + */ + REMOTE_PROC_CONNECT_CLOSE_CALLBACK_REGISTER = 358, + + /** + * @generate: none + * @acl: none + */ + REMOTE_PROC_CONNECT_CLOSE_CALLBACK_UNREGISTER = 359, + + /** + * @generate: none + * @acl: none + */ + REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED = 360 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 48c3bd8..7a8e1c4 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; @@ -3042,4 +3045,7 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_ADD_IOTHREAD = 355, REMOTE_PROC_DOMAIN_DEL_IOTHREAD = 356, REMOTE_PROC_DOMAIN_SET_USER_PASSWORD = 357, + REMOTE_PROC_CONNECT_CLOSE_CALLBACK_REGISTER = 358, + REMOTE_PROC_CONNECT_CLOSE_CALLBACK_UNREGISTER = 359, + REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED = 360, }; -- 1.7.1
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> Reuse virConnectCloseCallback to implement connection close event functions. This way we automatically meet multi-thread requirements on unregistering/notification. --- src/vz/vz_driver.c | 26 ++++++++++++++++++++++++++ src/vz/vz_sdk.c | 29 +++++++++++++++++++++++++++++ src/vz/vz_utils.h | 3 +++ 3 files changed, 58 insertions(+), 0 deletions(-) diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index d9ddd4f..e3d0fdc 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -268,6 +268,9 @@ vzOpenDefault(virConnectPtr conn) if (prlsdkSubscribeToPCSEvents(privconn)) goto error; + if (!(privconn->closeCallback = virGetConnectCloseCallback())) + goto error; + conn->privateData = privconn; if (prlsdkLoadDomains(privconn)) @@ -276,6 +279,8 @@ vzOpenDefault(virConnectPtr conn) return VIR_DRV_OPEN_SUCCESS; error: + virObjectUnref(privconn->closeCallback); + privconn->closeCallback = NULL; virObjectUnref(privconn->domains); virObjectUnref(privconn->caps); virStoragePoolObjListFree(&privconn->pools); @@ -350,6 +355,8 @@ vzConnectClose(virConnectPtr conn) virObjectUnref(privconn->caps); virObjectUnref(privconn->xmlopt); virObjectUnref(privconn->domains); + virObjectUnref(privconn->closeCallback); + privconn->closeCallback = NULL; virObjectEventStateFree(privconn->domainEventState); prlsdkDisconnect(privconn); conn->privateData = NULL; @@ -1337,6 +1344,23 @@ vzDomainBlockStatsFlags(virDomainPtr domain, return ret; } +static int +vzConnectRegisterCloseCallback(virConnectPtr conn, + virConnectCloseFunc cb, + void *opaque, + virFreeCallback freecb) +{ + vzConnPtr privconn = conn->privateData; + return virConnectCloseCallbackRegister(privconn->closeCallback, conn, cb, opaque, freecb); +} + +static int +vzConnectUnregisterCloseCallback(virConnectPtr conn, virConnectCloseFunc cb ATTRIBUTE_UNUSED) +{ + vzConnPtr privconn = conn->privateData; + virConnectCloseCallbackUnregister(privconn->closeCallback); + return 0; +} static virHypervisorDriver vzDriver = { .name = "vz", @@ -1389,6 +1413,8 @@ static virHypervisorDriver vzDriver = { .domainGetMaxMemory = vzDomainGetMaxMemory, /* 1.2.15 */ .domainBlockStats = vzDomainBlockStats, /* 1.3.0 */ .domainBlockStatsFlags = vzDomainBlockStatsFlags, /* 1.3.0 */ + .connectRegisterCloseCallback = vzConnectRegisterCloseCallback, /* 1.3.0 */ + .connectUnregisterCloseCallback = vzConnectUnregisterCloseCallback, /* 1.3.0 */ }; static virConnectDriver vzConnectDriver = { diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 388ea19..5f38709 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -1745,6 +1745,32 @@ prlsdkHandleVmEvent(vzConnPtr privconn, PRL_HANDLE prlEvent) return; } +static +void prlsdkHandleDispatcherConnectionClosed(vzConnPtr privconn) +{ + virConnectCloseCallbackCall(privconn->closeCallback, VIR_CONNECT_CLOSE_REASON_EOF); +} + +static void +prlsdkHandleDispatcherEvent(vzConnPtr privconn, PRL_HANDLE prlEvent) +{ + PRL_RESULT pret = PRL_ERR_FAILURE; + PRL_EVENT_TYPE prlEventType; + + pret = PrlEvent_GetType(prlEvent, &prlEventType); + prlsdkCheckRetGoto(pret, error); + + switch (prlEventType) { + case PET_DSP_EVT_DISP_CONNECTION_CLOSED: + prlsdkHandleDispatcherConnectionClosed(privconn); + break; + default: + VIR_DEBUG("Skipping dispatcher event of type %d", prlEventType); + } + error: + return; +} + static PRL_RESULT prlsdkEventsHandler(PRL_HANDLE prlEvent, PRL_VOID_PTR opaque) { @@ -1772,6 +1798,9 @@ prlsdkEventsHandler(PRL_HANDLE prlEvent, PRL_VOID_PTR opaque) // above function takes own of event prlEvent = PRL_INVALID_HANDLE; break; + case PIE_DISPATCHER: + prlsdkHandleDispatcherEvent(privconn, prlEvent); + break; default: VIR_DEBUG("Skipping event of issuer type %d", prlIssuerType); } diff --git a/src/vz/vz_utils.h b/src/vz/vz_utils.h index 9b46bf9..b0dc3d8 100644 --- a/src/vz/vz_utils.h +++ b/src/vz/vz_utils.h @@ -32,6 +32,7 @@ # include "conf/network_conf.h" # include "virthread.h" # include "virjson.h" +# include "datatypes.h" # define vzParseError() \ virReportErrorHelper(VIR_FROM_TEST, VIR_ERR_OPERATION_FAILED, __FILE__, \ @@ -69,6 +70,8 @@ struct _vzConn { virObjectEventStatePtr domainEventState; virStorageDriverStatePtr storageState; const char *drivername; + /* Immutable pointer, self-locking APIs */ + virConnectCloseCallbackPtr closeCallback; }; typedef struct _vzConn vzConn; -- 1.7.1
Guys, please take a look. On 25.06.2015 14:31, nshirokovskiy@virtuozzo.com wrote:
Notify of connection close event from parallels driver (possibly) wrapped in the remote driver.
Changes from v1: 1. fix comment style issues 2. remove spurious whitespaces 3. move rpc related part from vz patch to second(rpc) patch 4. remove unnecessary locks for immutable closeCallback in first patch.
Discussion.
In 1 and 2 patch we forced to some decisions because we don't have a weak reference mechanics.
1 patch. ----------- virConnectCloseCallback is introduced because we can not reference the connection object itself when setting a network layer callback because of how connection close works.
A connection close procedure is next: 1. client closes connection 2. a this point nobody else referencing a connection and it is disposed 3. connection dispose unreferencing network connection 4. network connection disposes
Thus if we referece a connection in network close callback we never get step 2. virConnectCloseCallback broke this cycle but at cost that clients MUST unregister explicitly before closing connection. This is not good as this unregistration is not really neaded. Client is not telling that it does not want to receive events anymore but rather forced to obey some implementation-driven rules.
2 patch. ----------- We impose requirements on driver implementations which is fragile. Moreover we again need to make explicit unregistrations. Implementation of domain events illustrates this point. remoteDispatchConnectDomainEventRegister does not reference NetClient and makes unregistration before NetClient is disposed but drivers do not meet the formulated requirements. Object event system release lock before delivering event for re-entrance purposes.
Shortly we have 2 undesired consequences here. 1. Mandatory unregistration. 2. Imposing multi-threading requirements.
Introduction of weak pointers could free us from these artifacts. Next weak reference workflow illustrates this.
1. Take weak reference on object of interest before passing to party. This doesn't break disposing mechanics as weak eference does not prevent from disposing object. Object is disposed but memory is not freed yet if there are weak references.
2. When callback is called we are safe to check if pointer dangling as we make a weak reference before.
3. Release weak reference and this trigger memory freeing if there are no more weak references.
daemon/libvirtd.h | 1 + daemon/remote.c | 86 +++++++++++++++++++++++++++++++ src/datatypes.c | 115 +++++++++++++++++++++++++++++++---------- src/datatypes.h | 21 ++++++-- src/driver-hypervisor.h | 12 ++++ src/libvirt-host.c | 77 +++++++++------------------- src/remote/remote_driver.c | 106 +++++++++++++++++++++++++++++--------- src/remote/remote_protocol.x | 24 ++++++++- src/remote_protocol-structs | 6 ++ src/vz/vz_driver.c | 26 +++++++++ src/vz/vz_sdk.c | 29 +++++++++++ src/vz/vz_utils.h | 3 +
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Nikolay Shirokovskiy -
nshirokovskiy＠virtuozzo.com