[Libvirt-cim] [PATCH] Improve support of nested KVM

[John Ferlan]

On 05/24/2013 03:34 AM, cngesaint at outlook.com wrote:
> From: Xu Wang <cngesaint at outlook.com>
> 
> Under nested KVM environment libvirt-cim could recognize kvm support
> correctly now.
> 
> Signed-off-by: Xu Wang <cngesaint at outlook.com>
> ---
>  libxkutil/device_parsing.c                |   19 +++++++++++++++++++
>  libxkutil/device_parsing.h                |    2 ++
>  src/Virt_VirtualSystemManagementService.c |   22 +++++++++++++++++++---
>  3 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/libxkutil/device_parsing.c b/libxkutil/device_parsing.c
> index 436415a..58d9094 100644
> --- a/libxkutil/device_parsing.c
> +++ b/libxkutil/device_parsing.c
> @@ -396,6 +396,25 @@ err:
>          return 0;
>  }
>  
> +int parse_domain_type(xmlNodePtr node, char **value)
> +{
> +        xmlNodePtr child = NULL;
> +
> +        child = node->children;
> +        while (child != NULL) {
> +                if (XSTREQ(child->name, "domain")) {
> +                        *value = get_attr_value(child, "type");

This returns either a NULL or strdup()'d value.  It seems by the makeup
of this loop that we will continue down a "child" forward link list
potentially overwriting a previous value and leaking memory.

Is there something that should cause us to exit the loop?

> +                }
> +                if (parse_domain_type(child, value) != 1)
> +                        goto err;

So a child could have a child.
> +                child = child->next;
> +        }
> +
> +        return 1;

We potentially could return a 1, but a NULL 'val' - is that expected.

> +err:
> +        return 0;

This function does not return CMPI_RC_OK or whatever else so the caller
shouldn't be comparing against that...

> +}
> +
>  static int parse_net_device(xmlNode *inode, struct virt_device **vdevs)
>  {
>          struct virt_device *vdev = NULL;
> diff --git a/libxkutil/device_parsing.h b/libxkutil/device_parsing.h
> index 6f6b0b4..733324f 100644
> --- a/libxkutil/device_parsing.h
> +++ b/libxkutil/device_parsing.h
> @@ -221,6 +221,8 @@ int attach_device(virDomainPtr dom, struct virt_device *dev);
>  int detach_device(virDomainPtr dom, struct virt_device *dev);
>  int change_device(virDomainPtr dom, struct virt_device *dev);
>  
> +int parse_domain_type(xmlNodePtr node, char **value);
> +
>  #define XSTREQ(x, y) (STREQ((char *)x, y))
>  #define STRPROP(d, p, n) (d->p = get_node_content(n))
>  
> diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c
> index 7b7261a..5210fdf 100644
> --- a/src/Virt_VirtualSystemManagementService.c
> +++ b/src/Virt_VirtualSystemManagementService.c
> @@ -393,8 +393,13 @@ static bool system_has_kvm(const char *pfx)
>          CMPIStatus s;
>          virConnectPtr conn;
>          char *caps = NULL;
> -        bool kvm = false;
>          bool disable_kvm = get_disable_kvm();
> +        char *val = NULL;
> +        int ret;
> +        xmlDocPtr doc;
> +        xmlNodePtr node;

I think both of these need to be initialized to NULL because of the
free() below.

> +        int len;
> +        bool kvm = false;
>  
>          /* sometimes disable KVM to avoid problem in nested KVM */
>          if (disable_kvm) {
> @@ -408,10 +413,21 @@ static bool system_has_kvm(const char *pfx)
>          }
>  
>          caps = virConnectGetCapabilities(conn);
> -        if (caps != NULL)
> -                kvm = (strstr(caps, "kvm") != NULL);
> +        if (caps != NULL) {
> +            len = strlen(caps) + 1;
> +            doc = xmlParseMemory(caps, len);

               if (doc == NULL) {
                   /* Error message ?  Do we care */
                   goto cleanup;
               }
> +            node = xmlDocGetRootElement(doc);
               if (node == NULL) {
                   /* Error message ? Do we care */
                   goto cleanup;
               }
> +            ret = parse_domain_type(node, &val);
> +            CU_DEBUG("domain type is %s.", val);
               ^^^^^
Move inside the if statement below, val could be NULL

> +            if (ret == CMPI_RC_OK) {

                       == 1) {

> +                    if(STREQC(val, "kvm"))
                         ^^
Add a space between "if" and "("

> +                        kvm = true;
> +            }
> +        }

           } else {
               /* Do we care what failure means here?? */
           }

Alternatively,

           if (parse_domain_type(node, &val) &&
               STREQC(val, "kvm")) {
               kvm = true;
           } else {
               if (val)
                   CU_DEBUG("domain type is %s.", val);
           }


cleanup:
>  
>          free(caps);
> +        free(doc);
> +        free(val);
>  
>          virConnectClose(conn);
>  
>