On 11/12/2009 06:49 AM, Daniel P. Berrange wrote:
> 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@xxxxxxxxxx>
>> 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
>
I'll go with GetLibVersion
>> 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()
>
I did that at first, but thought it was kind of hacky. I figured it
would be easier for driver writers to plug the util.c function into
their driver table, rather than ask 'How do I implement this?' only to
end up at libvirt.c and realize it's implemented for them. I suppose a
comment would clear that up, so I'll reinstate that behavior.
Thanks,
Cole
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list