[libvirt] [PATCH 1/3] remote: move connection close callback to driver level
Nikolay Shirokovskiy
nshirokovskiy at virtuozzo.com
Wed Jun 24 14:33:51 UTC 2015
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 at virtuozzo.com>
---
src/datatypes.c | 112 +++++++++++++++++++++++++++++++++-----------
src/datatypes.h | 21 ++++++--
src/driver-hypervisor.h | 12 +++++
src/libvirt-host.c | 79 ++++++++++---------------------
src/remote/remote_driver.c | 67 ++++++++++++++++----------
5 files changed, 179 insertions(+), 112 deletions(-)
diff --git a/src/datatypes.c b/src/datatypes.c
index 12bcfc1..07212d2 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,99 @@ 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 to here
+}
+
+int
+virConnectCloseCallbackRegister(virConnectCloseCallbackPtr obj,
+ virConnectPtr conn,
+ virConnectCloseFunc cb,
+ void *opaque,
+ virFreeCallback freecb)
{
- virConnectCloseCallbackDataPtr cb = obj;
+ int ret = -1;
+
+ virObjectLock(obj);
- virObjectLock(cb);
+ if (obj->callback) {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("A close callback is already registered"));
+ goto finish;
+ }
- if (cb->freeCallback)
- cb->freeCallback(cb->opaque);
+ // 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;
- virObjectUnlock(cb);
+ 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..644e255 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..2608171 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -1181,7 +1181,6 @@ virConnectIsAlive(virConnectPtr conn)
return -1;
}
-
/**
* virConnectRegisterCloseCallback:
* @conn: pointer to connection object
@@ -1205,50 +1204,37 @@ virConnectIsAlive(virConnectPtr conn)
*
* Returns 0 on success, -1 on error
*/
+
int
virConnectRegisterCloseCallback(virConnectPtr conn,
virConnectCloseFunc cb,
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..9e05c8a 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,
@@ -1044,6 +1030,8 @@ doRemoteOpen(virConnectPtr conn,
if (conn->uri == NULL) {
remote_connect_get_uri_ret uriret;
+
+
VIR_DEBUG("Trying to query remote URI");
memset(&uriret, 0, sizeof(uriret));
if (call(conn, priv, 0,
@@ -1107,6 +1095,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 +1230,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 +8029,33 @@ remoteDomainInterfaceAddresses(virDomainPtr dom,
return rv;
}
+static int
+remoteConnectRegisterCloseCallback(virConnectPtr conn,
+ virConnectCloseFunc cb,
+ void *opaque,
+ virFreeCallback freecb)
+{
+ struct private_data *priv = conn->privateData;
+ int ret = 0;
+
+ remoteDriverLock(priv);
+ ret = virConnectCloseCallbackRegister(priv->closeCallback, conn, cb, opaque, freecb);
+ remoteDriverUnlock(priv);
+
+ return ret;
+}
+
+static int
+remoteConnectUnregisterCloseCallback(virConnectPtr conn, virConnectCloseFunc cb ATTRIBUTE_UNUSED)
+{
+ struct private_data *priv = conn->privateData;
+
+ remoteDriverLock(priv);
+ virConnectCloseCallbackUnregister(priv->closeCallback);
+ remoteDriverUnlock(priv);
+
+ return 0;
+}
/* get_nonnull_domain and get_nonnull_network turn an on-wire
* (name, uuid) pair into virDomainPtr or virNetworkPtr object.
@@ -8391,6 +8406,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
More information about the libvir-list
mailing list