Re: [PATCH 03/21] Added basic implementation for virConnectGetCapabilities (required Win32_ComputerSystemProduct class)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



(1) Long subject line. I'd suggest
	hyperv: add implementation for virConnectGetCapabilities
	Done by adding the Win32_ComputerSystemProduct class.

"Done" is not the appropriate word;
 adding Win32_ComputerSystemProduct class was just a requirement to make the implementation.


(2) Dead 'if'; virObjectUnref(NULL) is safe

Stands also for patch #20 with xmlopt.


(3) Why do you need a forward declaration of a static function?  

Just because the new code was "grouped" near the end of the file.
It's not a problem to move the block of functions hypervLookupHostSystemBiosUuid & hypervCapsInit just before function hypervConnectOpen and remove this forward declaration.


(4) Is VIR_ERR_NO_DOMAIN really the right error to be using here?

No; it's a mistake; VIR_ERR_INTERNAL_ERROR error should be used instead.


(5) Libvirt style is two blank lines between functions, and function name starting in column 1 of a line separate from the return type:

In the current version of the hyperv libvirt driver, functions are actually separated by three blank lines.


(6) Might be nice to explain why this is commented out.

/* virCapabilitiesSetMacPrefix(caps, (unsigned char[]){ 0x00, 0x0c, 0x29 }); */

Has been commented because virCapabilitiesSetMacPrefix does not exist.
Is that something that a generic function that has been removed from the libvirt?

BTY. the Mac Prefix for hyperv is 00-15-0D


(7) Only need two blank lines

In the current version of the hyperv libvirt driver, functions are actually separated by three blank lines.
All the other functions must be reviewed then...


-----Message d'origine-----
De : Eric Blake [mailto:eblake@xxxxxxxxxx] 
Envoyé : jeudi 9 octobre 2014 00:34
À : Yves Vinter; libvir-list@xxxxxxxxxx
Objet : Re:  [PATCH 03/21] Added basic implementation for virConnectGetCapabilities (required Win32_ComputerSystemProduct class)

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

Long subject line. I'd suggest:

hyperv: add implementation for virConnectGetCapabilities

Done by adding the Win32_ComputerSystemProduct class.

> ---
>  src/hyperv/hyperv_driver.c            | 112 ++++++++++++++++++++++++++++++++++
>  src/hyperv/hyperv_private.h           |   2 +
>  src/hyperv/hyperv_wmi_generator.input |  12 ++++
>  3 files changed, 126 insertions(+)
> 
> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c 
> index f2017c3..dd56fb0 100644
> --- a/src/hyperv/hyperv_driver.c
> +++ b/src/hyperv/hyperv_driver.c
> @@ -58,12 +58,19 @@ hypervFreePrivate(hypervPrivate **priv)
>          wsmc_release((*priv)->client);
>      }
>  
> +    if ((*priv)->caps != NULL)
> +        virObjectUnref((*priv)->caps);

Dead 'if'; virObjectUnref(NULL) is safe.


>  
> +/* Forward declaration of hypervCapsInit */ static virCapsPtr 
> +hypervCapsInit(hypervPrivate *priv);

Why do you need a forward declaration of a static function?  If it's not recursive, just put the whole function body here (that may mean moving more than one function, to keep things topologically sorted; if you need to do code motion of existing functions, do it in a separate patch from where you add new code, to ease review).

>  
> +/* Retrieves host system UUID  */
> +static int
> +hypervLookupHostSystemBiosUuid(hypervPrivate *priv, unsigned char 
> +*uuid) {
> +    Win32_ComputerSystemProduct *computerSystem = NULL;
> +    virBuffer query = VIR_BUFFER_INITIALIZER;
> +    int result = -1;
> +    
> +    virBufferAddLit(&query, WIN32_COMPUTERSYSTEMPRODUCT_WQL_SELECT);
> +    
> +    if (hypervGetWin32ComputerSystemProductList(priv, &query, &computerSystem) < 0) {
> +        goto cleanup;
> +    }
> +    
> +    if (computerSystem == NULL) {
> +        virReportError(VIR_ERR_NO_DOMAIN,

Is VIR_ERR_NO_DOMAIN really the right error to be using here?

> +                       _("Unable to get Win32_ComputerSystemProduct"));
> +        goto cleanup;
> +    }
> +    
> +    if (virUUIDParse(computerSystem->data->UUID, uuid) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Could not parse UUID from string '%s'"),
> +                       computerSystem->data->UUID);
> +        goto cleanup;
> +    }
> +    
> +    result = 0;
> +    
> + cleanup:
> +    hypervFreeObject(priv, (hypervObject *)computerSystem);
> +    virBufferFreeAndReset(&query);

This virBufferFreeAndReset is not needed if you take my alternate patch for 1/21.

> +
> +    return result;
> +}
> +
> +
> +
> +static virCapsPtr hypervCapsInit(hypervPrivate *priv)

Libvirt style is two blank lines between functions, and function name starting in column 1 of a line separate from the return type:

static virCapsPtr
hypervCapsInit(hypervPrivate *priv)

> +{
> +    virCapsPtr caps = NULL;
> +    virCapsGuestPtr guest = NULL;
> +    
> +    caps = virCapabilitiesNew(VIR_ARCH_X86_64, 1, 1);
> +    
> +    if (caps == NULL) {
> +        virReportOOMError();
> +        return NULL;
> +    }
> +
> +    /* virCapabilitiesSetMacPrefix(caps, (unsigned char[]){ 0x00, 
> + 0x0c, 0x29 }); */

Might be nice to explain why this is commented out.


> +}
> +
> +
> +
> +static char*

Only need two blank lines.

> +hypervConnectGetCapabilities(virConnectPtr conn) {
> +    hypervPrivate *priv = conn->privateData;
> +    char *xml = virCapabilitiesFormatXML(priv->caps);

virCapabilitiesFormatXML can only return NULL on error, and it already outputs a decent error message; also, error is not always due to OOM...

> +    
> +    if (xml == NULL) {
> +        virReportOOMError();

...so this virReportOOMError() is bogus, because it overwrites the decent message.

> +        return NULL;
> +    }

Once you realize that, you can simplify this function to:

static char*
hypervConnectGetCapabilities(virConnectPtr conn) {
    hypervPrivate *priv = conn->privateData;
    return virCapabilitiesFormatXML(priv->caps);
}


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


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]