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

[Daniel Veillard]

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 at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/