[Libvir] Re: [PATCH] Re: Proposal: Check availability of driver calls (repost)

[Daniel P. Berrange]

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