When parsing the config, we look for the SCSI controllers one by one,
remembering their models, then let virDomainDefAddImplicitDevices
add them if any SCSI disk is using them.

Since these controllers are not really implicit (they are present
in the source config), add them explicitly.

This patch maintains the behavior of not adding a controller
if it was present in the config, but no disk was using it.

This also resolves the memory leak of virVMXParseConfig overwriting
the video device added by calling virDomainDefAddImplicitDevices
before the parsing is finished.

Reported-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/vmx/vmx.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 3e2f4c3e1..849cfc6b1 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -1650,6 +1650,18 @@ virVMXParseConfig(virVMXContext *ctx,
             if (def->disks[def->ndisks] != NULL)
                 ++def->ndisks;
         }
+
+    }
+
+    /* add all the SCSI controllers we've seen, up until the last one that is
+     * currently used by a disk */
+    if (def->ndisks != 0) {
+        virDomainDeviceInfoPtr info = &def->disks[def->ndisks - 1]->info;
+        for (controller = 0; controller <= info->addr.drive.controller; controller++) {
+            if (virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI,
+                                          controller, scsi_virtualDev[controller]) < 0)
+                goto cleanup;
+        }
     }
 
     /* def:disks (ide) */
@@ -1689,26 +1701,6 @@ virVMXParseConfig(virVMXContext *ctx,
             ++def->ndisks;
     }
 
-    /* def:controllers */
-    if (virDomainDefAddImplicitDevices(def) < 0) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not add controllers"));
-        goto cleanup;
-    }
-
-    for (controller = 0; controller < def->ncontrollers; ++controller) {
-        if (def->controllers[controller]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
-            if (def->controllers[controller]->idx > 3) {
-                virReportError(VIR_ERR_INTERNAL_ERROR,
-                               _("SCSI controller index %d out of [0..3] range"),
-                               def->controllers[controller]->idx);
-                goto cleanup;
-            }
-
-            def->controllers[controller]->model =
-              scsi_virtualDev[def->controllers[controller]->idx];
-        }
-    }
-
     /* def:fss */
     if (virVMXGetConfigBoolean(conf, "isolation.tools.hgfs.disable",
                                &hgfs_disabled, true, true) < 0) {
-- 
2.13.0

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

On 08/21/2017 11:41 AM, Ján Tomko wrote:
> When parsing the config, we look for the SCSI controllers one by one,
> remembering their models, then let virDomainDefAddImplicitDevices
> add them if any SCSI disk is using them.
> 
> Since these controllers are not really implicit (they are present
> in the source config), add them explicitly.
> 
> This patch maintains the behavior of not adding a controller
> if it was present in the config, but no disk was using it.
> 
> This also resolves the memory leak of virVMXParseConfig overwriting
> the video device added by calling virDomainDefAddImplicitDevices
> before the parsing is finished.
> 
> Reported-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/vmx/vmx.c | 32 ++++++++++++--------------------
>  1 file changed, 12 insertions(+), 20 deletions(-)
> 

My compiler isn't happy with this

  CC       remote/libvirt_driver_remote_la-remote_driver.lo
vmx/vmx.c: In function 'virVMXParseConfig':
vmx/vmx.c:1662:84: error: ordered comparison of pointer with integer
zero [-Werror=extra]
                                           controller,
scsi_virtualDev[controller]) < 0)

           ^
cc1: all warnings being treated as errors

> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index 3e2f4c3e1..849cfc6b1 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -1650,6 +1650,18 @@ virVMXParseConfig(virVMXContext *ctx,
>              if (def->disks[def->ndisks] != NULL)
>                  ++def->ndisks;
>          }
> +
> +    }
> +
> +    /* add all the SCSI controllers we've seen, up until the last one that is
> +     * currently used by a disk */
> +    if (def->ndisks != 0) {
> +        virDomainDeviceInfoPtr info = &def->disks[def->ndisks - 1]->info;
> +        for (controller = 0; controller <= info->addr.drive.controller; controller++) {

I dunno - perhaps feels 'safer' to use 4 instead of
info->addr.drive.controller, but I also see the value of keeping things
as they are... Guess it always looks strange to go from 0 .. <= #...

IDC either way, your call.


> +            if (virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI,
> +                                          controller, scsi_virtualDev[controller]) < 0)

The < 0 is the incorrect comparison, should be:

    if (!virDomainDefAddController

Don't forget to properly indent the second line too.

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

> +                goto cleanup;
> +        }
>      }
>  
>      /* def:disks (ide) */
> @@ -1689,26 +1701,6 @@ virVMXParseConfig(virVMXContext *ctx,
>              ++def->ndisks;
>      }
>  
> -    /* def:controllers */
> -    if (virDomainDefAddImplicitDevices(def) < 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not add controllers"));
> -        goto cleanup;
> -    }
> -
> -    for (controller = 0; controller < def->ncontrollers; ++controller) {
> -        if (def->controllers[controller]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
> -            if (def->controllers[controller]->idx > 3) {
> -                virReportError(VIR_ERR_INTERNAL_ERROR,
> -                               _("SCSI controller index %d out of [0..3] range"),
> -                               def->controllers[controller]->idx);
> -                goto cleanup;
> -            }
> -
> -            def->controllers[controller]->model =
> -              scsi_virtualDev[def->controllers[controller]->idx];
> -        }
> -    }
> -
>      /* def:fss */
>      if (virVMXGetConfigBoolean(conf, "isolation.tools.hgfs.disable",
>                                 &hgfs_disabled, true, true) < 0) {
> 

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