[Libvirt-cim] [PATCH 6/6] VSMS: Determine if default controller exists for KVM

[John Ferlan]

On 03/18/2014 10:49 PM, John Ferlan wrote:
> If a default controller for 'pci' with model 'pci-root' doesn't exist,
> create one for any add/modify type event.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/Virt_VirtualSystemManagementService.c | 106 +++++++++++++++++++++++++++++-
>  1 file changed, 103 insertions(+), 3 deletions(-)
> 
> diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c
> index 3e7785e..4610374 100644
> --- a/src/Virt_VirtualSystemManagementService.c
> +++ b/src/Virt_VirtualSystemManagementService.c
> @@ -659,6 +659,82 @@ static bool default_input_device(struct domain *domain)
>          return true;
>  }
>  
> +static bool check_default_controller_device(struct domain *domain)
> +{
> +        int i;
> +        struct controller_device *cdev = NULL;
> +
> +        CU_DEBUG("Domain '%s' type=%d", domain->name, domain->type);
> +
> +        /* Only QEMU/KVM devices need controllers */
> +        if (domain->type != DOMAIN_KVM && domain->type != DOMAIN_QEMU)
> +                return true;
> +
> +        /* Do we find a 'pci' device with 'pci-root' model on the system?
> +         * If not, create one.
> +         */
> +        for (i = 0; i < domain->dev_controller_ct; i++) {
> +                struct virt_device *_dev = &domain->dev_controller[i];
> +                if (_dev->type == CIM_RES_TYPE_UNKNOWN) {
> +                        CU_DEBUG("controller _dev->type is unknown")
> +                        continue;
> +                }
> +
> +                cdev = &_dev->dev.controller;
> +
> +                CU_DEBUG("controller type=%d model=%s",
> +                         cdev->type, cdev->model ? cdev->model : "unknown");
> +
> +                if (cdev->type == CIM_CONTROLLER_PROTOCOL_TYPE_PCI &&
> +                        cdev->model != NULL &&
> +                        STREQC(cdev->model, "pci-root")) {
> +                        CU_DEBUG("Found 'pci-root' for %s", domain->name);
> +                        break;
> +                }
> +                cdev = NULL;
> +        }
> +
> +        /* Didn't find 'pci'/'pci-root', let's add it */
> +        if (cdev == NULL) {
> +                char *model;
> +                struct virt_device *_list;
> +                struct virt_device *_dev;
> +
> +                CU_DEBUG("Domain '%s' missing 'pci-root'", domain->name);
> +
> +                model = strdup("pci-root");
> +                if (model == NULL) {
> +                        CU_DEBUG("Fail to allocate default controller model");
> +                        return false;
> +                }
> +
> +                if (domain->dev_controller == NULL)
> +                        _list = calloc(1, sizeof(*domain->dev_controller));
> +                else
> +                        _list = realloc(domain->dev_controller,
> +                                        (domain->dev_controller_ct + 1) *
> +                                        sizeof(struct virt_device));
> +                if (_list == NULL) {
> +                        CU_DEBUG("Fail to allocate controller model");
> +                        free(model);
> +                        return false;
> +                }
> +                domain->dev_controller = _list;
> +
> +                _dev = &domain->dev_controller[domain->dev_controller_ct];
> +                _dev->type = CIM_RES_TYPE_CONTROLLER;
> +
> +                cdev = &_dev->dev.controller;
> +                memset(cdev, 0, sizeof(*cdev));
> +                cdev->type = CIM_CONTROLLER_PROTOCOL_TYPE_PCI;
> +                cdev->index = 0; /* By definition it must be 0 */
> +                cdev->model = model;
> +                domain->dev_controller_ct += 1;
> +        }
> +
> +        return true;
> +}
> +
>  static bool add_default_devs(struct domain *domain)
>  {
>          if (domain->dev_graphics_ct < 1 &&
> @@ -2545,8 +2621,16 @@ static CMPIInstance *create_system(const CMPIContext *context,
>                  goto out;
>          }
>  
> +        if (!check_default_controller_device(domain)) {
> +                CU_DEBUG("Failed to check default controller");
> +                cu_statusf(_BROKER, s,
> +                           CMPI_RC_ERR_FAILED,
> +                           "Failed to create default controller");
> +                goto out;
> +        }
> +
>          xml = system_to_xml(domain);
> -        CU_DEBUG("System XML:\n%s", xml);
> +        CU_DEBUG("create_system XML:\n%s", xml);
>  
>          inst = connect_and_create(xml, ref, s);
>          if (inst != NULL) {
> @@ -2815,9 +2899,17 @@ static CMPIStatus update_system_settings(const CMPIContext *context,
>                  goto out;
>          }
>  
> +        if (!check_default_controller_device(dominfo)) {
> +                CU_DEBUG("Failed to check default controller");
> +                cu_statusf(_BROKER, &s,
> +                           CMPI_RC_ERR_FAILED,
> +                           "Failed to create default controller");
> +                goto out;
> +        }
> +
>          xml = system_to_xml(dominfo);
>          if (xml != NULL) {
> -                CU_DEBUG("New XML is:\n%s", xml);
> +                CU_DEBUG("update_system_settings XML:\n%s", xml);
>                  connect_and_create(xml, ref, &s);
>          }
>  
> @@ -3310,9 +3402,17 @@ static CMPIStatus _update_resources_for(const CMPIContext *context,
>                  goto out;
>          }
>  
> +        if (!check_default_controller_device(dominfo)) {
> +                CU_DEBUG("Failed to check default controller");
> +                cu_statusf(_BROKER, &s,
> +                           CMPI_RC_ERR_FAILED,
> +                           "Failed to create default controller");
> +                goto out;
> +        }
> +
>          xml = system_to_xml(dominfo);
>          if (xml != NULL) {
> -                CU_DEBUG("New XML:\n%s", xml);
> +                CU_DEBUG("_update_resources_for XML:\n%s", xml);
>                  connect_and_create(xml, ref, &s);
>  
>                  if (inst_list_add(&list, rasd) == 0) {
> 


Running the changes through Coverity resulted in a simple change...

Although totally unnecessary - Coverity complained that comparing
 dev_controller to NULL in order to decide whether to calloc or
realloc, could result in a data overrun if dev_controller_ct was
greater than 0. Changing the comparison to check dev_controller_ct
kept Coverity quiet.

Consider the following diff squashed in

diff --git a/src/Virt_VirtualSystemManagementService.c b/src/Virt_VirtualSystemManagementService.c
index 4610374..7958537 100644
--- a/src/Virt_VirtualSystemManagementService.c
+++ b/src/Virt_VirtualSystemManagementService.c
@@ -708,7 +708,7 @@ static bool check_default_controller_device(struct domain *d
                         return false;
                 }
 
-                if (domain->dev_controller == NULL)
+                if (domain->dev_controller_ct == 0)
                         _list = calloc(1, sizeof(*domain->dev_controller));
                 else
                         _list = realloc(domain->dev_controller,