[libvirt] [PATCH 02/21] Added implementation for virConnectGetVersion (required CIM_DataFile class)

[Yves Vinter]

(1) syntax-check:

I run make syntax-check and found some other "white space" or "TAB" issues (coming with subsequent patches)


(2) Is this error message correct?  That is, is datafile->data->Version the name of the domain you are referring to in the message?

More than that: this part of code is completely wrong!
datafile->data->Version shouldn't be evaluated if datafile == NULL!!!

> +    if (datafile == NULL) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Could not lookup %s for domain %s"),
> +                       "Msvm_VirtualSystemSettingData",
> +                       datafile->data->Version

Right code is:

If (datafile == NULL) {
        virReportError(VIR_ERR_INTERNAL_ERROR,
                       _("Unable to get the hypervisor version"));
}

[ Removed the comment:  /* Check the result of conversion */]



(3) This is modifying datafile->data in-place.  I hope that's safe (if not, you'd have to strdup a copy that you can safely manipulate locally).

It is safe; datafile is a temporary object that is released at the end of hypervConnectGetVersion call.



(4) The comment is a bit redundant with the name of the function you are calling.  I'd just omit the comment.

No problem...

-----Message d'origine-----
De : Eric Blake [mailto:eblake at redhat.com] 
Envoyé : jeudi 9 octobre 2014 00:15
À : Yves Vinter; libvir-list at redhat.com
Objet : Re: [libvirt] [PATCH 02/21] Added implementation for virConnectGetVersion (required CIM_DataFile class)

On 10/08/2014 06:33 AM, Yves Vinter wrote:
> From: yvinter <yves.vinter at bull.net>
> 
> ---
>  src/hyperv/hyperv_driver.c            | 55 +++++++++++++++++++++++++++++++++++
>  src/hyperv/hyperv_wmi_generator.input | 37 +++++++++++++++++++++++
>  src/hyperv/hyperv_wmi_generator.py    |  4 +--
>  3 files changed, 94 insertions(+), 2 deletions(-)
> 

Another round of reviews; the .input and .py file look okay to me, but I spotted more issues in the .c that I didn't point out the first time around.

> +static int
> +hypervConnectGetVersion(virConnectPtr conn, unsigned long *version) {
> +    int result = -1;
> +    hypervPrivate *priv = conn->privateData;
> +    CIM_DataFile  *datafile = NULL;
> +    virBuffer query = VIR_BUFFER_INITIALIZER;
> +    char *p;
> +    

Trailing whitespace. Another thing 'make syntax-check' will flag.

> +    virBufferAddLit(&query, " Select * from CIM_DataFile where Name='c:\\\\windows\\\\system32\\\\vmms.exe' ");
> +    if (hypervGetCIMDataFileList(priv, &query, &datafile) < 0) {
> +        goto cleanup;
> +    }
> +    
> +    /* Check the result of convertion */

s/convertion/conversion/

> +    if (datafile == NULL) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Could not lookup %s for domain %s"),
> +                       "Msvm_VirtualSystemSettingData",
> +                       datafile->data->Version);

Is this error message correct?  That is, is datafile->data->Version the name of the domain you are referring to in the message?

> +        goto cleanup;
> +    }
> +    
> +    /* Delete release number and last digit of build number 1.1.111x.xxxx */
> +    p = strrchr(datafile->data->Version,'.');
> +    if (p == NULL) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Could not parse version number from '%s'"),
> +                       datafile->data->Version);
> +        goto cleanup;
> +    }
> +    p--;
> +    *p = '\0';

This is modifying datafile->data in-place.  I hope that's safe (if not, you'd have to strdup a copy that you can safely manipulate locally).

> +    
> +    /* Parse Version String to Long */
> +    if (virParseVersionString(datafile->data->Version,

The comment is a bit redundant with the name of the function you are calling.  I'd just omit the comment.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org