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

[Eric Blake]

On 10/08/2014 06:33 AM, Yves Vinter wrote:
> From: yvinter <yves.vinter at bull.net>

Looks like you had identity problems throughout the series. I don't mind
touching up first-time contributions, but it would be nice to get it
right in the future so I don't have to spend time on it.

> 
> ---
>  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(-)
> 
> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
> index 372ff39..f2017c3 100644
> --- a/src/hyperv/hyperv_driver.c
> +++ b/src/hyperv/hyperv_driver.c
> @@ -1232,6 +1232,7 @@ hypervDomainManagedSaveRemove(virDomainPtr domain, unsigned int flags)
>  }
>  
>  
> +
>  #define MATCH(FLAG) (flags & (FLAG))

Why the added blank line?

>  static int
>  hypervConnectListAllDomains(virConnectPtr conn,
> @@ -1366,6 +1367,59 @@ hypervConnectListAllDomains(virConnectPtr conn,
>  
>  
>  
> +static int

Wow, that's a lot of blank lines already there.

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

Why the double space?

> +    virBuffer query = VIR_BUFFER_INITIALIZER;
> +    char *p;
> +    
> +    virBufferAddLit(&query, " Select * from CIM_DataFile where Name='c:\\\\windows\\\\system32\\\\vmms.exe' ");

Is the leading and trailing space really necessary in the string literal?

> +    if (hypervGetCIMDataFileList(priv, &query, &datafile) < 0) {
> +        goto cleanup;
> +    }

Libvirt style says {} are not necessary for one-line bodies (qemu style
would require it, but we're a bit more lenient).

> +    
> +    /* Check the result of convertion */
> +    if (datafile == NULL) {

It's sufficient to write 'if (datafile)' (comparison to NULL is just
extra verbosity); but as that appears to be the prevailing style already
in this file, I'm not going to worry about shortening it.

> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Could not lookup %s for domain %s"),
> +                       "Msvm_VirtualSystemSettingData",
> +                       datafile->data->Version);
> +        goto cleanup;
> +    }
> +    
> +    /* Delete release number and last digit of build number 1.1.111x.xxxx */
> +    p = strrchr(datafile->data->Version,'.');

This failed 'make syntax-check' due to a missing space:

  GEN      bracket-spacing-check
src/hyperv/hyperv_driver.c:1381:     p =
strrchr(datafile->data->Version,'.');
maint.mk: incorrect whitespace, see HACKING for rules
make: *** [bracket-spacing-check] Error 1

> +    if (p == NULL) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Could not parse version number from '%s'"),
> +                       datafile->data->Version);
> +        goto cleanup;
> +    }
> +    p--;
> +    *p = '\0';
> +    
> +    /* Parse Version String to Long */
> +    if (virParseVersionString(datafile->data->Version,
> +                              version, true) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Could not parse version number from '%s'"),
> +                       datafile->data->Version);
> +        goto cleanup;
> +    }
> +    
> +    result = 0;
> +    
> + cleanup:
> +    hypervFreeObject(priv, (hypervObject *)datafile);
> +    virBufferFreeAndReset(&query);

This line is not necessary if you ACK my v2 of 1/21.

> +
> +    return result;
> +}
> +
> +
>  
>  static virDriver hypervDriver = {
>      .no = VIR_DRV_HYPERV,
> @@ -1402,6 +1456,7 @@ static virDriver hypervDriver = {
>      .domainHasManagedSaveImage = hypervDomainHasManagedSaveImage, /* 0.9.5 */
>      .domainManagedSaveRemove = hypervDomainManagedSaveRemove, /* 0.9.5 */
>      .connectIsAlive = hypervConnectIsAlive, /* 0.9.8 */
> +    .connectGetVersion = hypervConnectGetVersion, /* 1.2.10 */
>  };

I got through here in my review for now; mostly looks good other than
style.  When I finish the rest of the patch, I may end up just fixing it
myself and pushing.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141008/5d414263/attachment-0001.sig>