[libvirt] [PATCH v3 09/11] admin: Add support for connection close callbacks
Martin Kletzander
mkletzan at redhat.com
Mon Nov 16 16:40:07 UTC 2015
On Fri, Nov 06, 2015 at 12:46:24PM +0100, Erik Skultety wrote:
>As we need a client disconnect handler, we also need a mechanism to register
>such handlers for a client. This patch introduced both the close callbacks and
>also the client vshAdmCatchDisconnect handler to be registered with it. By
>registering the handler we still need to make sure the client can react to
>daemon's events like disconnect or keepalive, so asynchronous I/O event polling
>is necessary to be enabled too.
>---
> cfg.mk | 2 +-
> include/libvirt/libvirt-admin.h | 21 ++++++++
> src/admin/admin_remote.c | 35 +++++++++++++
> src/datatypes.c | 24 +++++++++
> src/datatypes.h | 16 +++++-
> src/libvirt-admin.c | 112 ++++++++++++++++++++++++++++++++++++++++
> src/libvirt_admin_public.syms | 2 +
> tools/virt-admin.c | 50 ++++++++++++++++++
> 8 files changed, 260 insertions(+), 2 deletions(-)
>
>diff --git a/cfg.mk b/cfg.mk
>index a9bba38..43ee945 100644
>--- a/cfg.mk
>+++ b/cfg.mk
>@@ -1192,7 +1192,7 @@ exclude_file_name_regexp--sc_prohibit_include_public_headers_quote = \
> ^(src/internal\.h$$|tools/wireshark/src/packet-libvirt.h$$)
>
> exclude_file_name_regexp--sc_prohibit_include_public_headers_brackets = \
>- ^(tools/|examples/|include/libvirt/(virterror|libvirt-(qemu|lxc))\.h$$)
>+ ^(tools/|examples/|include/libvirt/(virterror|libvirt-(admin|qemu|lxc))\.h$$)
>
> exclude_file_name_regexp--sc_prohibit_int_ijk = \
> ^(src/remote_protocol-structs|src/remote/remote_protocol.x|cfg.mk|include/)$
>diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
>index 145720d..5f9841c 100644
>--- a/include/libvirt/libvirt-admin.h
>+++ b/include/libvirt/libvirt-admin.h
>@@ -26,6 +26,8 @@
> #ifndef __VIR_ADMIN_H__
> # define __VIR_ADMIN_H__
>
>+# include <libvirt/libvirt.h>
>+
Oh no you didn't... This means that including libvirt-admin.h will
allow seeing *everything* from libvirt. I see two things you need
this for. Enum values and a function. Enum values could be
defined somewhere else (e.g. datatypes.h) and that 'somewhere else'
could be included instead. The function should either be
re-implemented for both libraries or just moved outside and each
library should have a wrapper for it. Do anything to remove this
line, thanks.
> # ifdef __cplusplus
> extern "C" {
> # endif
>@@ -56,6 +58,25 @@ int virAdmConnectIsAlive(virAdmConnectPtr conn);
>
> char *virAdmConnectGetURI(virAdmConnectPtr conn);
>
>+/**
>+ * virAdmConnectCloseFunc:
>+ * @conn: virAdmConnect connection
>+ * @reason: reason why the connection was closed (see virConnectCloseReason)
>+ * @opaque: opaque client data
>+ *
>+ * A callback to be registered, in case a connection was closed.
>+ */
>+typedef void (*virAdmConnectCloseFunc)(virAdmConnectPtr conn,
>+ int reason,
>+ void *opaque);
>+
>+int virAdmConnectRegisterCloseCallback(virAdmConnectPtr conn,
>+ virAdmConnectCloseFunc cb,
>+ void *opaque,
>+ virFreeCallback freecb);
>+int virAdmConnectUnregisterCloseCallback(virAdmConnectPtr conn,
>+ virAdmConnectCloseFunc cb);
>+
> # ifdef __cplusplus
> }
> # endif
>diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c
>index 2c02ba6..7b40ea1 100644
>--- a/src/admin/admin_remote.c
>+++ b/src/admin/admin_remote.c
>@@ -101,6 +101,28 @@ call(virAdmConnectPtr conn,
>
> #include "admin_client.h"
>
>+static void
>+remoteAdminClientCloseFunc(virNetClientPtr client ATTRIBUTE_UNUSED,
>+ int reason,
>+ void *opaque)
>+{
>+ virAdmConnectCloseCallbackDataPtr 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);
>+}
>+
> static int
> remoteAdminConnectOpen(virAdmConnectPtr conn, unsigned int flags)
> {
>@@ -112,6 +134,17 @@ remoteAdminConnectOpen(virAdmConnectPtr conn, unsigned int flags)
>
> args.flags = flags;
>
>+ if (virNetClientRegisterAsyncIO(priv->client) < 0) {
>+ VIR_DEBUG("Failed to add event watch, disabling events and support for"
>+ " keepalive messages");
This doesn't do anything with keepalive. But when I'm thinking about
it I don't think it's worth removing just for the time until we add
those few keepalive calls here.
>+ virResetLastError();
>+ }
>+
>+ virObjectRef(conn->closeCallback);
>+ virNetClientSetCloseCallback(priv->client, remoteAdminClientCloseFunc,
>+ conn->closeCallback,
>+ virObjectFreeCallback);
>+
> if (call(conn, 0, ADMIN_PROC_CONNECT_OPEN,
> (xdrproc_t)xdr_admin_connect_open_args, (char *)&args,
> (xdrproc_t)xdr_void, (char *)NULL) == -1) {
>@@ -139,6 +172,8 @@ remoteAdminConnectClose(virAdmConnectPtr conn)
> goto done;
> }
>
>+ virNetClientSetCloseCallback(priv->client, NULL, NULL, NULL);
>+
> rv = 0;
>
> done:
>diff --git a/src/datatypes.c b/src/datatypes.c
>index aeac301..c832d80 100644
>--- a/src/datatypes.c
>+++ b/src/datatypes.c
>@@ -60,8 +60,10 @@ static void virStorageVolDispose(void *obj);
> static void virStoragePoolDispose(void *obj);
>
> virClassPtr virAdmConnectClass;
>+virClassPtr virAdmConnectCloseCallbackDataClass;
>
> static void virAdmConnectDispose(void *obj);
>+static void virAdmConnectCloseCallbackDataDispose(void *obj);
>
> static int
> virDataTypesOnceInit(void)
>@@ -91,6 +93,7 @@ virDataTypesOnceInit(void)
> DECLARE_CLASS(virStoragePool);
>
> DECLARE_CLASS_LOCKABLE(virAdmConnect);
>+ DECLARE_CLASS_LOCKABLE(virAdmConnectCloseCallbackData);
>
> #undef DECLARE_CLASS_COMMON
> #undef DECLARE_CLASS_LOCKABLE
>@@ -822,7 +825,14 @@ virAdmConnectNew(void)
> if (!(ret = virObjectLockableNew(virAdmConnectClass)))
> return NULL;
>
>+ if (!(ret->closeCallback = virObjectLockableNew(virAdmConnectCloseCallbackDataClass)))
>+ goto error;
>+
> return ret;
>+
>+ error:
>+ virObjectUnref(ret);
>+ return NULL;
> }
>
> static void
>@@ -834,4 +844,18 @@ virAdmConnectDispose(void *obj)
> conn->privateDataFreeFunc(conn);
>
> virURIFree(conn->uri);
>+ virObjectUnref(conn->closeCallback);
>+}
>+
>+static void
>+virAdmConnectCloseCallbackDataDispose(void *obj)
>+{
>+ virAdmConnectCloseCallbackDataPtr cb_data = obj;
>+
>+ virObjectLock(cb_data);
>+
>+ if (cb_data->freeCallback)
>+ cb_data->freeCallback(cb_data->opaque);
>+
>+ virObjectUnlock(cb_data);
> }
>diff --git a/src/datatypes.h b/src/datatypes.h
>index b16bdb1..1b1777d 100644
>--- a/src/datatypes.h
>+++ b/src/datatypes.h
>@@ -331,9 +331,11 @@ extern virClassPtr virAdmConnectClass;
>
> typedef struct _virConnectCloseCallbackData virConnectCloseCallbackData;
> typedef virConnectCloseCallbackData *virConnectCloseCallbackDataPtr;
>+typedef struct _virAdmConnectCloseCallbackData virAdmConnectCloseCallbackData;
>+typedef virAdmConnectCloseCallbackData *virAdmConnectCloseCallbackDataPtr;
>
> /**
>- * Internal structure holding data related to connection close callbacks.
>+ * Internal structures holding data related to connection close callbacks.
> */
> struct _virConnectCloseCallbackData {
> virObjectLockable parent;
>@@ -344,6 +346,15 @@ struct _virConnectCloseCallbackData {
> virFreeCallback freeCallback;
> };
>
>+struct _virAdmConnectCloseCallbackData {
>+ virObjectLockable parent;
>+
>+ virAdmConnectPtr conn;
>+ virAdmConnectCloseFunc callback;
>+ void *opaque;
>+ virFreeCallback freeCallback;
>+};
>+
> /**
> * _virConnect:
> *
>@@ -401,6 +412,9 @@ struct _virAdmConnect {
>
> void *privateData;
> virFreeCallback privateDataFreeFunc;
>+
>+ /* Per-connection close callback */
>+ virAdmConnectCloseCallbackDataPtr closeCallback;
> };
>
>
>diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c
>index d9a20fc..fdcf2c1 100644
>--- a/src/libvirt-admin.c
>+++ b/src/libvirt-admin.c
>@@ -374,3 +374,115 @@ virAdmConnectGetURI(virAdmConnectPtr conn)
>
> return uri;
> }
>+
>+/**
>+ * virAdmConnectRegisterCloseCallback:
>+ * @conn: connection to admin server
>+ * @cb: callback to be invoked upon connection close
>+ * @opaque: user data to pass to @cb
>+ * @freecb: callback to free @opaque
>+ *
>+ * Registers a callback to be invoked when the connection
>+ * is closed. This callback is invoked when there is any
>+ * condition that causes the socket connection to the
>+ * hypervisor to be closed.
>+ *
>+ * The @freecb must not invoke any other libvirt public
>+ * APIs, since it is not called from a re-entrant safe
>+ * context.
>+ *
>+ * Returns 0 on success, -1 on error
>+ */
>+int virAdmConnectRegisterCloseCallback(virAdmConnectPtr conn,
>+ virAdmConnectCloseFunc cb,
>+ void *opaque,
>+ virFreeCallback freecb)
>+{
>+ VIR_DEBUG("conn=%p", conn);
>+
>+ virResetLastError();
>+
>+ virCheckAdmConnectReturn(conn, -1);
>+
>+ virObjectRef(conn);
>+
>+ 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);
>+ virObjectUnlock(conn);
>+
>+ return 0;
>+
>+ error:
>+ virObjectUnlock(conn->closeCallback);
>+ virObjectUnlock(conn);
>+ virDispatchError(NULL);
>+ virObjectUnref(conn);
>+ return -1;
>+
>+}
>+
>+/**
>+ * virAdmConnectUnregisterCloseCallback:
>+ * @conn: pointer to connection object
>+ * @cb: pointer to the current registered callback
>+ *
>+ * Unregisters the callback previously set with the
>+ * virAdmConnectRegisterCloseCallback method. The callback
>+ * will no longer receive notifications when the connection
>+ * closes. If a virFreeCallback was provided at time of
>+ * registration, it will be invoked.
>+ *
>+ * Returns 0 on success, -1 on error
>+ */
>+int virAdmConnectUnregisterCloseCallback(virAdmConnectPtr conn,
>+ virAdmConnectCloseFunc cb)
>+{
>+ VIR_DEBUG("conn=%p", conn);
>+
>+ virResetLastError();
>+
>+ virCheckAdmConnectReturn(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);
>+ virObjectUnlock(conn);
>+ virObjectUnref(conn);
>+
>+ return 0;
>+
>+ error:
>+ virObjectUnlock(conn->closeCallback);
>+ virObjectUnlock(conn);
>+ virDispatchError(NULL);
>+ return -1;
>+}
>diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms
>index 069508c..df01837 100644
>--- a/src/libvirt_admin_public.syms
>+++ b/src/libvirt_admin_public.syms
>@@ -17,4 +17,6 @@ LIBVIRT_ADMIN_1.3.0 {
> virAdmConnectRef;
> virAdmConnectIsAlive;
> virAdmConnectGetURI;
>+ virAdmConnectRegisterCloseCallback;
>+ virAdmConnectUnregisterCloseCallback;
> };
>diff --git a/tools/virt-admin.c b/tools/virt-admin.c
>index ce36303..fc98964 100644
>--- a/tools/virt-admin.c
>+++ b/tools/virt-admin.c
>@@ -52,6 +52,51 @@ static char *progname;
> static const vshCmdGrp cmdGroups[];
> static const vshClientHooks hooks;
>
>+/*
>+ * vshAdmCatchDisconnect:
>+ *
>+ * We get here when the connection was closed. Unlike virsh, we do not save
>+ * the fact that the event was raised, sice there is virAdmConnectIsAlive to
>+ * check if the communication channel has not been closed by remote party.
>+ */
>+static void
>+vshAdmCatchDisconnect(virAdmConnectPtr conn ATTRIBUTE_UNUSED,
>+ int reason,
>+ void *opaque)
>+{
>+ if (reason != VIR_CONNECT_CLOSE_REASON_CLIENT) {
Please change this to
if (reason == VIR_CONNECT_CLOSE_REASON_CLIENT)
return;
and then continue with unintended block. It's more readable. Feel
free to fix it in virsh as well ;)
Although I'm not sure we need that since we do unregister the close
callback before disconnecting... But it's nice to be safe.
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151116/fcc93fc5/attachment-0001.sig>
More information about the libvir-list
mailing list