[Libvir] Re: [PATCH] Re: Proposal: Check availability of driver calls (repost)
Daniel Veillard
veillard at redhat.com
Tue Jul 31 21:26:23 UTC 2007
On Tue, Jul 31, 2007 at 03:23:40PM +0100, Richard W.M. Jones wrote:
> Daniel Veillard wrote:
> >On Tue, Jul 31, 2007 at 02:09:20PM +0100, Richard W.M. Jones wrote:
> >>Daniel Veillard wrote:
> >>>I'm not 100% sure we want to
> >>>expose this as a library API though, at least not in that way.
> >>I should just say in case I wasn't 100% clear that this is just a driver
> >>API. Nothing should be exposed from libvirt.
> >
> > Then feature detection + versionning in the protocol. I'm
>
> Here's a patch.
>
> [I'm going to start a separate thread on the issue of exporting calls to
> libvirtd.]
>
> Rich.
>
> --
> Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
> Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
> Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in
> England and Wales under Company Registration No. 03798903
> Index: src/driver.h
> ===================================================================
> RCS file: /data/cvs/libvirt/src/driver.h,v
> retrieving revision 1.32
> diff -u -p -r1.32 driver.h
> --- src/driver.h 27 Jul 2007 23:23:00 -0000 1.32
> +++ src/driver.h 31 Jul 2007 14:20:02 -0000
> @@ -44,12 +44,44 @@ typedef enum {
> VIR_DRV_OPEN_ERROR = -2,
> } virDrvOpenStatus;
>
> +/* Feature detection. This is a libvirt-private interface for determining
> + * what features are supported by the driver.
> + *
> + * The remote driver passes features through to the real driver at the
> + * remote end unmodified, except if you query a VIR_DRV_FEATURE_REMOTE*
> + * feature.
> + */
> +typedef enum {
> + /* Driver supports V1-style virDomainMigrate, ie. domainMigratePrepare/
> + * domainMigratePerform/domainMigrateFinish.
> + */
> + VIR_DRV_FEATURE_MIGRATION_V1 = 1,
> +
> + /* Driver is not local. */
> + VIR_DRV_FEATURE_REMOTE = 2,
> +} virDrvFeature;
Probably best done with defines than enums, as you want to be able to compose
them the fact it comes from a type doesn't help, and I have learnt the hard way
that enums sucks in C in general.
> +/* Internal feature-detection macro. Don't call drv->supports_feature
> + * directly, because it may be NULL, use this macro instead.
> + *
> + * Note that you must check for errors.
> + *
> + * Returns:
> + * >= 1 Feature is supported.
> + * 0 Feature is not supported.
> + * -1 Error.
> + */
> +#define VIR_DRV_SUPPORTS_FEATURE(drv,conn,feature) \
> + ((drv)->supports_feature ? (drv)->supports_feature((conn),(feature)) : 0)
> +
> typedef virDrvOpenStatus
> (*virDrvOpen) (virConnectPtr conn,
> const char *name,
> int flags);
> typedef int
> (*virDrvClose) (virConnectPtr conn);
> +typedef int
> + (*virDrvSupportsFeature) (virConnectPtr conn, virDrvFeature feature);
I would rather use , int features) where you OR the features, allows
to know in one call if you get what you want or not.
> typedef const char *
> (*virDrvGetType) (virConnectPtr conn);
> typedef int
> @@ -202,6 +234,7 @@ struct _virDriver {
> unsigned long ver; /* the version of the backend */
> virDrvOpen open;
> virDrvClose close;
> + virDrvSupportsFeature supports_feature;
Actually you probably want a combination of those not just one of them
so again an int with OR'ed values there, I think.
> virDrvGetType type;
> virDrvGetVersion version;
> virDrvGetHostname getHostname;
> Index: src/internal.h
> ===================================================================
> RCS file: /data/cvs/libvirt/src/internal.h,v
> retrieving revision 1.47
> diff -u -p -r1.47 internal.h
> --- src/internal.h 25 Jul 2007 23:16:30 -0000 1.47
> +++ src/internal.h 31 Jul 2007 14:20:03 -0000
> @@ -229,6 +229,8 @@ int __virStateActive(void);
> #define virStateReload() __virStateReload()
> #define virStateActive() __virStateActive()
>
> +int __virDrvSupportsFeature (virConnectPtr conn, int feature);
> +
features and will be perfect :-)
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
> Index: src/libvirt.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/libvirt.c,v
> retrieving revision 1.92
> diff -u -p -r1.92 libvirt.c
> --- src/libvirt.c 27 Jul 2007 23:23:00 -0000 1.92
> +++ src/libvirt.c 31 Jul 2007 14:20:04 -0000
> @@ -540,6 +540,20 @@ virConnectClose(virConnectPtr conn)
> return (0);
> }
>
> +/* Not for public use. This function is part of the internal
> + * implementation of driver features in the remote case.
> + */
> +int
> +__virDrvSupportsFeature (virConnectPtr conn, int feature)
> +{
> + DEBUG("conn=%p, feature=%d", conn, feature);
> +
> + if (!VIR_IS_CONNECT(conn))
> + return (-1);
> +
> + return VIR_DRV_SUPPORTS_FEATURE (conn->driver, conn, feature);
> +}
> +
I would plural and use the macro to hide an OR. You could return
the set of features actually supported in the set, or the opposite
which could be easier but less readable
ret = __virDrvSupportsFeature(conn, A | B | C);
if (ret != 0) {
detail for each bit the missing problem.
}
> /**
> * virConnectGetType:
> * @conn: pointer to the hypervisor connection
> Index: src/libvirt_sym.version
> ===================================================================
> RCS file: /data/cvs/libvirt/src/libvirt_sym.version,v
> retrieving revision 1.25
> diff -u -p -r1.25 libvirt_sym.version
> --- src/libvirt_sym.version 26 Jun 2007 22:56:14 -0000 1.25
> +++ src/libvirt_sym.version 31 Jul 2007 14:20:05 -0000
> @@ -116,5 +116,7 @@
> __virStateReload;
> __virStateActive;
>
> + __virDrvSupportsFeature;
> +
> local: *;
> };
> Index: src/qemu_driver.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/qemu_driver.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 qemu_driver.c
> --- src/qemu_driver.c 30 Jul 2007 09:59:06 -0000 1.14
> +++ src/qemu_driver.c 31 Jul 2007 14:20:07 -0000
> @@ -2343,6 +2343,7 @@ static virDriver qemuDriver = {
> LIBVIR_VERSION_NUMBER,
> qemudOpen, /* open */
> qemudClose, /* close */
> + NULL, /* supports_feature */
> qemudGetType, /* type */
> qemudGetVersion, /* version */
> NULL, /* hostname */
> Index: src/remote_internal.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/remote_internal.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 remote_internal.c
> --- src/remote_internal.c 27 Jul 2007 23:23:00 -0000 1.16
> +++ src/remote_internal.c 31 Jul 2007 14:20:10 -0000
> @@ -1190,6 +1190,30 @@ remoteClose (virConnectPtr conn)
> return ret;
> }
>
> +static int
> +remoteSupportsFeature (virConnectPtr conn, virDrvFeature feature)
> +{
> + remote_supports_feature_args args;
> + remote_supports_feature_ret ret;
> + GET_PRIVATE (conn, -1);
> +
> + /* VIR_DRV_FEATURE_REMOTE* features are handled directly. */
> + switch (feature) {
> + case VIR_DRV_FEATURE_REMOTE: return 1;
> + default: /*FALLTHROUGH*/;
> + }
> +
> + args.feature = feature;
> +
> + memset (&ret, 0, sizeof ret);
> + if (call (conn, priv, 0, REMOTE_PROC_SUPPORTS_FEATURE,
> + (xdrproc_t) xdr_remote_supports_feature_args, (char *) &args,
> + (xdrproc_t) xdr_remote_supports_feature_ret, (char *) &ret) == -1)
> + return -1;
> +
> + return ret.supported;
> +}
> +
> /* Unfortunately this function is defined to return a static string.
> * Since the remote end always answers with the same type (for a
> * single connection anyway) we cache the type in the connection's
> @@ -2893,6 +2917,7 @@ static virDriver driver = {
> .ver = REMOTE_PROTOCOL_VERSION,
> .open = remoteOpen,
> .close = remoteClose,
> + .supports_feature = remoteSupportsFeature,
> .type = remoteType,
> .version = remoteVersion,
> .getHostname = remoteGetHostname,
> Index: src/test.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/test.c,v
> retrieving revision 1.43
> diff -u -p -r1.43 test.c
> --- src/test.c 27 Jul 2007 23:23:00 -0000 1.43
> +++ src/test.c 31 Jul 2007 14:20:12 -0000
> @@ -1932,6 +1932,7 @@ static virDriver testDriver = {
> LIBVIR_VERSION_NUMBER,
> testOpen, /* open */
> testClose, /* close */
> + NULL, /* supports_feature */
> NULL, /* type */
> testGetVersion, /* version */
> testGetHostname, /* hostname */
> Index: qemud/remote.c
> ===================================================================
> RCS file: /data/cvs/libvirt/qemud/remote.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 remote.c
> --- qemud/remote.c 24 Jul 2007 14:21:03 -0000 1.6
> +++ qemud/remote.c 31 Jul 2007 14:20:13 -0000
> @@ -418,6 +418,18 @@ remoteDispatchClose (struct qemud_client
> }
>
> static int
> +remoteDispatchSupportsFeature (struct qemud_client *client, remote_message_header *req,
> + remote_supports_feature_args *args, remote_supports_feature_ret *ret)
> +{
> + CHECK_CONN(client);
> +
> + ret->supported = __virDrvSupportsFeature (client->conn, args->feature);
> + if (ret->supported == -1) return -1;
> +
> + return 0;
> +}
> +
> +static int
> remoteDispatchGetType (struct qemud_client *client, remote_message_header *req,
> void *args ATTRIBUTE_UNUSED, remote_get_type_ret *ret)
> {
> Index: qemud/remote_protocol.x
> ===================================================================
> RCS file: /data/cvs/libvirt/qemud/remote_protocol.x,v
> retrieving revision 1.3
> diff -u -p -r1.3 remote_protocol.x
> --- qemud/remote_protocol.x 26 Jun 2007 11:42:46 -0000 1.3
> +++ qemud/remote_protocol.x 31 Jul 2007 14:20:13 -0000
> @@ -170,6 +170,14 @@ struct remote_open_args {
> int flags;
> };
>
> +struct remote_supports_feature_args {
> + int feature;
> +};
> +
> +struct remote_supports_feature_ret {
> + int supported;
> +};
> +
> struct remote_get_type_ret {
> remote_nonnull_string type;
> };
> @@ -610,7 +618,8 @@ enum remote_procedure {
> REMOTE_PROC_DOMAIN_GET_SCHEDULER_TYPE = 56,
> REMOTE_PROC_DOMAIN_GET_SCHEDULER_PARAMETERS = 57,
> REMOTE_PROC_DOMAIN_SET_SCHEDULER_PARAMETERS = 58,
> - REMOTE_PROC_GET_HOSTNAME = 59
> + REMOTE_PROC_GET_HOSTNAME = 59,
> + REMOTE_PROC_SUPPORTS_FEATURE = 60
> };
>
> /* Custom RPC structure. */
Doing multiple features check remotely would potentially be faster but
changes a bit those files.
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard at redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
More information about the libvir-list
mailing list