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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list