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 -=|