[libvirt] [PATCH] vmx: do not treat controllers as implicit devices

[John Ferlan]

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 at 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 at 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) {
>