Re: [libvirt] [PATCH] Add virConnectGetLibvirtVersion API

On Mon, Nov 02, 2009 at 03:52:28PM -0500, Cole Robinson wrote: > Hi all, > > The attached patch adds a new API call for retrieving the libvirt > version used by a connection: virConnectGetLibvirtVersion. Without this, > there is currently no way to determine the libvirt version of a remote > qemu connection for example. This info can be useful for feature > detection/enabling/disabling. > > As an example, virt-install may want to use the AC97 sound device as > the default sound model for new VMs. However, this was only added to > libvirt's white list in 0.6.0, while the other models have been > available since 0.4.3. If installing a remote guest, virt-install will > want to ensure that the remote libvirtd is >= 0.6.0. Granted, the remote > version could have backported the AC97 patch and virt-install would be > incorrect, but better to be overly restrictive than to blindly specify > AC97 and have guest creation bomb out. > > The 'correct' way to handle the above issue would be some combination of > dropping internal whitelists from libvirt and generating them from info > reported by the hypervisor, and advertising the available values in the > capabilities XML. However I think this API addition makes things more > manageable with little downside until a proper solution is implemented. Even as a simple debugging aid for bug reporting, it would be justified to add this extra API call to get the remote version. > commit 59871ddf8956a96a1148769c05ada6e763d91080 > Author: Cole Robinson <crobinso(a)redhat.com&gt; > Date: Mon Nov 2 15:34:46 2009 -0500 > > Add virConnectGetLibvirtVersion > > There is currently no way to determine the libvirt version of a remote libvirtd we > are connected to. This is a useful piece of data to enable feature detection. I think I'd prefer a name of either virConnectGetLibVersion virConnectGetAPIVersion

> diff --git a/src/libvirt.c b/src/libvirt.c > index 5ddf27a..85d7008 100644 > --- a/src/libvirt.c > +++ b/src/libvirt.c > @@ -1437,6 +1437,48 @@ error: > } > > /** > + * virConnectGetLibvirtVersion: > + * @conn: pointer to the hypervisor connection > + * @libVer: returns the libvirt library version used on the connection (OUT) > + * > + * Provides @libVer, which is the version of the libvirt on the @conn host. > + * > + * Returns -1 in case of failure, 0 otherwise, and values for @libVer have > + * the format major * 1,000,000 + minor * 1,000 + release. > + */ > +int > +virConnectGetLibvirtVersion(virConnectPtr conn, unsigned long *libVer) > +{ > + DEBUG("conn=%p, libVir=%p", conn, libVer); > + > + virResetLastError(); > + > + if (!VIR_IS_CONNECT(conn)) { > + virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__); > + return -1; > + } > + > + if (libVer == NULL) { > + virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__); > + goto error; > + } > + > + if (conn->driver->libvirtVersion) { > + int ret = conn->driver->libvirtVersion(conn, libVer); > + if (ret < 0) > + goto error; > + return ret; > + } > + > + virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); > + > +error: > + /* Copy to connection error object for back compatability */ > + virSetConnError(conn); > + return -1; > +} Wouldn't it be better here to fallback to *libVer = LIBVIR_VERSION_NUMBER; in the case of 'conn->driver->libvirtVersion' being NULL. That would mean you'd only need to implemnet this driver method for the remote driver, letting all the others fallback to this generic case as they do with virGetVersion()