[libvirt] [PATCH 03/16] hyperv: add get capabilities

John Ferlan jferlan at redhat.com
Wed Sep 14 21:33:14 UTC 2016



On 08/09/2016 08:39 AM, Jason Miesionczek wrote:
> ---
>  src/hyperv/hyperv_driver.c  | 106 +++++++++++++++++++++++++++++++++++++++++++-
>  src/hyperv/hyperv_private.h |   2 +
>  2 files changed, 107 insertions(+), 1 deletion(-)
> 
> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
> index b642a02..4a5e80d 100644
> --- a/src/hyperv/hyperv_driver.c
> +++ b/src/hyperv/hyperv_driver.c
> @@ -35,6 +35,7 @@
>  #include "hyperv_wmi.h"
>  #include "openwsman.h"
>  #include "virstring.h"
> +#include "virtypedparam.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_HYPERV
>  
> @@ -51,11 +52,15 @@ hypervFreePrivate(hypervPrivate **priv)
>          wsmc_release((*priv)->client);
>      }
>  
> +    if ((*priv)->caps != NULL)
> +        virObjectUnref((*priv)->caps);
> +
>      hypervFreeParsedUri(&(*priv)->parsedUri);
>      VIR_FREE(*priv);
>  }
>  
> -
> +/* Forward declaration of hypervCapsInit */
> +static virCapsPtr hypervCapsInit(hypervPrivate *priv);

Ewww.... Why not define the function right here then?

>  
>  static virDrvOpenStatus
>  hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth,
> @@ -183,6 +188,12 @@ hypervConnectOpen(virConnectPtr conn, virConnectAuthPtr auth,
>          goto cleanup;
>      }
>  
> +    /* Setup capabilities */
> +    priv->caps = hypervCapsInit(priv);
> +    if (priv->caps == NULL) {
> +        goto cleanup;
> +    }
> +

If there's only 1 line, then no need for { }

This occurs multiple times in this file.

You need to run "make check syntax-check"

I would change to :

    if (!(priv->caps = hypervCapsInit(priv)))
        goto cleanup;

>      conn->privateData = priv;
>      priv = NULL;
>      result = VIR_DRV_OPEN_SUCCESS;
> @@ -1324,7 +1335,19 @@ hypervConnectListAllDomains(virConnectPtr conn,
>  }
>  #undef MATCH
>  

Try to go with 2 blank lines between functions - it's the new normal for
our reviews...

> +static char*

static char *

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

This should be unnecessary. If there was a failure to memory allocation
it would already be messaged. If there was some other failure, then
you're overwriting that error.

> +        return NULL;
> +    }

So this simply becomes:

    return virCapabilitiesFormatXML(priv->caps);

> +
> +    return xml;
> +}
>  
>  
>  static virHypervisorDriver hypervHypervisorDriver = {
> @@ -1361,8 +1384,89 @@ static virHypervisorDriver hypervHypervisorDriver = {
>      .domainHasManagedSaveImage = hypervDomainHasManagedSaveImage, /* 0.9.5 */
>      .domainManagedSaveRemove = hypervDomainManagedSaveRemove, /* 0.9.5 */
>      .connectIsAlive = hypervConnectIsAlive, /* 0.9.8 */
> +    .connectGetCapabilities = hypervConnectGetCapabilities, /* 1.2.10 */

s/1.2.10/2.3.0/

(at the very earliest)

>  };
>  

blank line

> +/* 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;
> +    }

Unnecessary {}

> +
> +    if (computerSystem == NULL) {
> +        virReportError(VIR_ERR_NO_DOMAIN,

You need to a "%s, " e.g.:

virReportError(VIR_ERR_NO_DOMAIN, "%s",

(something syntax-check would complain about)

> +                       _("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);
> +
> +    return result;
> +}
> +
> +

Only 2 blank lines necessary

> +
> +static virCapsPtr hypervCapsInit(hypervPrivate *priv)

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;
> +    }

Unnecessary OOM

    if (!(caps = virCapabilitiesNew(VIR_ARCH_X86_64, 1, 1)))
        return NULL;
> +
> +    /* virCapabilitiesSetMacPrefix(caps, (unsigned char[]){ 0x00, 0x0c, 0x29 }); */
> +
> +    if (hypervLookupHostSystemBiosUuid(priv,caps->host.host_uuid) < 0) {

s/priv,caps/priv, caps/

Need a space between args - syntax-check complaint.

> +        goto failure;
> +    }

Unnecessary {}
s/failure/error/


> +
> +    /* i686 */
> +    guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_I686, NULL, NULL, 0, NULL);
> +    if (guest == NULL) {
> +        goto failure;
> +    }
> +    if (virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_HYPERV, NULL, NULL, 0, NULL) == NULL) {
> +        goto failure;
> +    }
> +
> +    /* x86_64 */
> +    guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_X86_64, NULL, NULL, 0, NULL);
> +    if (guest == NULL) {
> +        goto failure;
> +    }
> +    if (virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_HYPERV, NULL, NULL, 0, NULL) == NULL) {
> +        goto failure;
> +    }

These are all really long lines... They all fail syntax-check due to {}
issues... Try to have each line be 80 columns, consider

   if (!(guest = virCapabilitiesAddGuest(...)))
      goto error;
   if (!

> +
> +    return caps;
> +
> + failure:

We use error for error labels not failure

John

> +    virObjectUnref(caps);
> +    return NULL;
> +}
>  
>  
>  static void
> diff --git a/src/hyperv/hyperv_private.h b/src/hyperv/hyperv_private.h
> index 574bb5f..d9aa0bd 100644
> --- a/src/hyperv/hyperv_private.h
> +++ b/src/hyperv/hyperv_private.h
> @@ -26,6 +26,7 @@
>  # include "internal.h"
>  # include "virerror.h"
>  # include "hyperv_util.h"
> +# include "capabilities.h"
>  # include "openwsman.h"
>  
>  typedef struct _hypervPrivate hypervPrivate;
> @@ -33,6 +34,7 @@ typedef struct _hypervPrivate hypervPrivate;
>  struct _hypervPrivate {
>      hypervParsedUri *parsedUri;
>      WsManClient *client;
> +    virCapsPtr caps;
>  };
>  
>  #endif /* __HYPERV_PRIVATE_H__ */
> 




More information about the libvir-list mailing list