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/

On Tue, Jul 31, 2007 at 02:09:20PM +0100, Richard W.M. Jones wrote: Then feature detection + versionning in the protocol. I'm +100 on this. Daniel -- 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/

On Wed, Aug 01, 2007 at 02:45:57PM +0100, Richard W.M. Jones wrote: 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 ? 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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

Daniel Veillard wrote: Examples being ...? I want to know if there's a real example where a coordinating function would like to query > 1 feature of the underlying driver, and if doing so save signicant numbers of round trips in the remote case. I don't claim that it can do this. In the original design I looked at how one might do this and concluded it was pretty difficult. You can't just use indexes into the driver structure because they change depending on architecture and packing rules[1]. and Dan Berrange wrote: That is another thing though. The patch I gave is an internal libvirt.c -> driver API to replace occasions when we might do "if (drv->some_func != NULL) { ... }". What you're talking about is already supported through virConnectGetCapabilities -- a public function which allows libvirt users to query specific capabilities of the underlying hypervisor, eg. do we support save/restore? Rich. [1] See discussion of "is_available" macro here: https://www.redhat.com/archives/libvir-list/2007-July/msg00437.html -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903