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

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 > Date: Tue, 24 Jul 2007 12:53:37 +0100 > From: "Richard W.M. Jones" <rjones(a)redhat.com&gt; > Subject: [Libvir] Proposal: Check availability of driver calls > To: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > Cc: libvir-list(a)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. > -- -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/