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

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