[Libvir] Proposal: Check availability of driver calls (repost)
-- 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
On Tue, Jul 31, 2007 at 12:54:03PM +0100, Richard W.M. Jones wrote:
In general I like the idea of a feature checking API, internally it used to be one just needed to check the pointer in the driver table, with remote that's not possible anymore. I'm not 100% sure we want to expose this as a library API though, at least not in that way. And yes I would really appreciate to have versionning in place before actually putting this in a release, this helps tremendously in the long term. So general +1 except we may need to revisit the technical way to do things. Daniel
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
Date: Tue, 24 Jul 2007 12:53:37 +0100 From: "Richard W.M. Jones" <rjones@redhat.com> Subject: [Libvir] Proposal: Check availability of driver calls To: "Daniel P. Berrange" <berrange@redhat.com> Cc: libvir-list@redhat.com
Daniel P. Berrange wrote:
On Mon, Jul 23, 2007 at 11:00:21AM +0100, Richard W.M. Jones wrote:
step virDrvDomainMigrateFinish() might be needed, it could for example resume on the target side and also verify the domain is actually okay. That could improve error handling and feels a lot more like a transactional system where you really want an atomic work/fail operation and nothing else. Yes. It's important to note that the internals may be changed later, although that may complicate things if we want to allow people to run incompatible versions of the daemons. [Another email on that subject is coming up after this one].
We need to treat the binary on-the-wire ABI for the client<->daemon the same way as our public ELF library ABI. Never break it, only add to it. So if we think the internals client<->server needs may change we should avoid including this code in a release until we're more satisfied with its longevity.
Yesterday I wrote: "It's important to note that the internals may be changed later, although that may complicate things if we want to allow people to run incompatible versions of the daemons. [Another email on that subject is coming up after this one]." When I actually looked into it, it turned out to be trickier and less useful than I thought, so the promised email never came.
At the moment the function src/libvirt.c: virDomainMigrate is a kind of "coordination function", in that it coordinates the steps necessary (ie. domainMigratePrepare -> domainMigratePerform -> etc.)
One of the things which this coordination function does first is to check that the drivers support migration at all. The code is:
/* Check that migration is supported. * Note that in the remote case these functions always exist. * But the remote driver will throw NO_SUPPORT errors later. */ if (!conn->driver->domainMigratePerform || !dconn->driver->domainMigratePrepare) { virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); return NULL; }
As noted in the comment, this test is only functional in the local case. The remote driver registers functions for every driver entrypoint.
So I began to think about having an "is_available" function. The test above would be rewritten:
if (!is_available (conn->driver->domainMigratePerform) || !is_available (dconn->driver->domainMigratePrepare)) { ... }
and the idea is that is_available expands to a straightforward non-NULL test for local drivers, but something more complicated if the driver is the remote driver.
Implementing is_available turns out to be less than simple however. I had some ideas along these lines:
#define is_available(drv,fn) \ (!(drv)->_available \ ? (drv)->(fn) \ : (drv)->_available((drv,offsetof(typeof(drv),(fn)))))
but this is wrong in several ways: (1) relies on GCC typeof extension, (2) not easy to write the _available function to do the right thing across different word sizes or structure packing (which might happen at the two ends of the remote connection).
What seems to be better is some sort of private driver capabilities function. We might use it like this:
if (!conn->driver->supports_feature (VIR_DRV_MIGRATION_V1) || !dconn->driver->supports_feature (VIR_DRV_MIGRATION_V1)) { ... }
(Note that this is a completely private interface). In the remote case, the supports_feature call is passed through to the end driver.
Now if we decide that the current internal implementation of virDomainMigrate is bogus, we can add the extra driver calls for a new migration implementation, then support both old and new by changing virDomainMigrate coordination function as so:
if (conn->driver->supports_feature (VIR_DRV_MIGRATION_V2) && dconn->driver->supports_feature (VIR_DRV_MIGRATION_V2)) {
// shiny new implementation of migration // ...
} else if (conn->driver->supports_feature (VIR_DRV_MIGRATION_V1) && dconn->driver->supports_feature (VIR_DRV_MIGRATION_V1)) {
// bogus old implementation of migration // ...
} else { // no compatible implementation virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); return NULL; }
The above allows us to do migrations from old libvirtd -> old libvirtd, or from new libvirtd -> new libvirtd. Plausibly we could add the other cases as well, depending upon how the new migration implementation worked.
So that is my proposal. No code, but obviously simple to implement.
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
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
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. 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
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 +100 on this. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
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
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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard wrote:
+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.
What in particular? I'm not particularly concerned either way since enums are basically just as unsafe as #defines in C, but it is nice for the library user to be able to connect the argument prototype ("virDrvFeature feature") to the list of permitted types.
+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.
In the case where you've got multiple different migration possibilities (VIR_DRV_FEATURE_MIGRATE_V1 & VIR_DRV_FEATURE_MIGRATE_V2) then this saves you one remote call in the legacy case (VIR_DRV_FEATURE_MIGRATE_V2 not supported so we have to do a second remote call to check VIR_DRV_FEATURE_MIGRATE_V1). On the other hand, it complicates the interface. You need to return an array rather than a single int. (OK, so you can return a bit array, but now the feature list had better always be <= 32 entries long). And in the case where someone queries VIR_DRV_FEATURE_MIGRATE_V1 & VIR_DRV_FEATURE_REMOTE you need to have two different drivers answering a single request. I think this complicates things unnecessarily ... 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
On Wed, Aug 01, 2007 at 02:45:57PM +0100, Richard W.M. Jones wrote:
Daniel Veillard wrote:
+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.
What in particular? I'm not particularly concerned either way since enums are basically just as unsafe as #defines in C, but it is nice for the library user to be able to connect the argument prototype ("virDrvFeature feature") to the list of permitted types.
+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.
In the case where you've got multiple different migration possibilities (VIR_DRV_FEATURE_MIGRATE_V1 & VIR_DRV_FEATURE_MIGRATE_V2) then this saves you one remote call in the legacy case (VIR_DRV_FEATURE_MIGRATE_V2 not supported so we have to do a second remote call to check VIR_DRV_FEATURE_MIGRATE_V1).
You seems to think it's useful only for migrate. I want that to be usable for a lot more. You talk to a remote server, you want to know if it supports any of the entry point in the driver table, how do you do this ?
On the other hand, it complicates the interface. You need to return an array rather than a single int. (OK, so you can return a bit array, but now the feature list had better always be <= 32 entries long). And in the case where someone queries VIR_DRV_FEATURE_MIGRATE_V1 & VIR_DRV_FEATURE_REMOTE you need to have two different drivers answering a single request.
I think this complicates things unnecessarily ...
I think this would be a waste to design it with a single narrowly focused usage in mind, when it can be far more generally useful. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Wed, Aug 01, 2007 at 10:19:42AM -0400, Daniel Veillard wrote:
On Wed, Aug 01, 2007 at 02:45:57PM +0100, Richard W.M. Jones wrote:
Daniel Veillard wrote:
+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.
What in particular? I'm not particularly concerned either way since enums are basically just as unsafe as #defines in C, but it is nice for the library user to be able to connect the argument prototype ("virDrvFeature feature") to the list of permitted types.
+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.
In the case where you've got multiple different migration possibilities (VIR_DRV_FEATURE_MIGRATE_V1 & VIR_DRV_FEATURE_MIGRATE_V2) then this saves you one remote call in the legacy case (VIR_DRV_FEATURE_MIGRATE_V2 not supported so we have to do a second remote call to check VIR_DRV_FEATURE_MIGRATE_V1).
You seems to think it's useful only for migrate. I want that to be usable for a lot more. You talk to a remote server, you want to know if it supports any of the entry point in the driver table, how do you do this ?
On the other hand, it complicates the interface. You need to return an array rather than a single int. (OK, so you can return a bit array, but now the feature list had better always be <= 32 entries long). And in the case where someone queries VIR_DRV_FEATURE_MIGRATE_V1 & VIR_DRV_FEATURE_REMOTE you need to have two different drivers answering a single request.
I think this complicates things unnecessarily ...
I think this would be a waste to design it with a single narrowly focused usage in mind, when it can be far more generally useful.
Yes - as a concrete example - QEMU driver doesn't support save/restore. We have no way to find this out in virt-manager without actually trying to run the API & wait for it to fail. If we could query libvirt to ask if the driver supported a particular feature, we could disable the save/restore buttons/menus in virt-manager. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Daniel Veillard wrote:
You seems to think it's useful only for migrate. I want that to be usable for a lot more.
Examples being ...? I want to know if there's a real example where a coordinating function would like to query > 1 feature of the underlying driver, and if doing so save signicant numbers of round trips in the remote case.
You talk to a remote server, you want to know if it supports any of the entry point in the driver table, how do you do this ?
I don't claim that it can do this. In the original design I looked at how one might do this and concluded it was pretty difficult. You can't just use indexes into the driver structure because they change depending on architecture and packing rules[1].
On the other hand, it complicates the interface. You need to return an array rather than a single int. (OK, so you can return a bit array, but now the feature list had better always be <= 32 entries long). And in the case where someone queries VIR_DRV_FEATURE_MIGRATE_V1 & VIR_DRV_FEATURE_REMOTE you need to have two different drivers answering a single request.
I think this complicates things unnecessarily ...
I think this would be a waste to design it with a single narrowly focused usage in mind, when it can be far more generally useful.
and Dan Berrange wrote:
Yes - as a concrete example - QEMU driver doesn't support save/restore. We have no way to find this out in virt-manager without actually trying to run the API & wait for it to fail. If we could query libvirt to ask if the driver supported a particular feature, we could disable the save/restore buttons/menus in virt-manager.
That is another thing though. The patch I gave is an internal libvirt.c -> driver API to replace occasions when we might do "if (drv->some_func != NULL) { ... }". What you're talking about is already supported through virConnectGetCapabilities -- a public function which allows libvirt users to query specific capabilities of the underlying hypervisor, eg. do we support save/restore? Rich. [1] See discussion of "is_available" macro here: https://www.redhat.com/archives/libvir-list/2007-July/msg00437.html -- 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
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Richard W.M. Jones