[libvirt] [PATCH v2 1/3] remote: move connection close callback to driver level

From: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; 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(a)virtuozzo.com&gt; --- 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