[libvirt] [PATCH] Add virConnectGetLibvirtVersion API

[Cole Robinson]

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 at redhat.com>
>> 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