[libvirt] [PATCH 3/4] conf: Use existing SCSI hostdev model to create new

[Eric Farman]

On 12/06/2017 08:08 AM, John Ferlan wrote:
> In virDomainDefMaybeAddHostdevSCSIcontroller when we add a new
> controller because someone neglected to add one or we're adding
> one because the existing one is full, we should copy over the
> model number from the existing controller since whatever we
> create should at least have the same characteristics as the one
> we cannot use because it's full.
> 
> NB: This affects the existing hostdev-scsi-autogen-address test
> which would add a default ('lsi') SCSI controller for the various
> scsi_host's that would create a controller for the hostdev.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>   src/conf/domain_conf.c                                    | 13 ++++++++++++-
>   tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml |  2 +-
>   2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 66e21c4bd..61b4a0075 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -17689,12 +17689,22 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
>       size_t i;
>       int maxController = -1;
>       virDomainHostdevDefPtr hostdev;
> +    virDomainControllerModelSCSI model = -1;
> +    virDomainControllerModelSCSI newModel = -1;
> 
>       for (i = 0; i < def->nhostdevs; i++) {
>           hostdev = def->hostdevs[i];
>           if (virHostdevIsSCSIDevice(hostdev) &&
>               (int)hostdev->info->addr.drive.controller > maxController) {
>               maxController = hostdev->info->addr.drive.controller;
> +            /* We may be creating a new controller because this one is full.
> +             * So let's grab the model from it and update the model we're
> +             * going to add as long as this one isn't undefined. The premise
> +             * being keeping the same controller model for all SCSI hostdevs. */
> +            model = virDomainDeviceFindControllerModel(def, hostdev->info,
> +                                                       VIR_DOMAIN_CONTROLLER_TYPE_SCSI);
> +            if (model != -1)
> +                newModel = model;

What's the harm if the last controller were undefined?  Wouldn't it get 
populated down the road anyway if one were not set at this point (we 
send -1 today, after all).  That could eliminate one of the two 
virDomainControllerModelSCSI variables here, I would think.

But, either way I think it's fine.

Reviewed-by: Eric Farman <farman at linux.vnet.ibm.com>

>           }
>       }
> 
> @@ -17702,7 +17712,8 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
>           return 0;
> 
>       for (i = 0; i <= maxController; i++) {
> -        if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, -1) < 0)
> +        if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI,
> +                                           i, newModel) < 0)
>               return -1;
>       }
> 
> diff --git a/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml b/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml
> index 8e93056ee..cea212b64 100644
> --- a/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml
> +++ b/tests/qemuxml2xmloutdata/hostdev-scsi-autogen-address.xml
> @@ -29,7 +29,7 @@
>         <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
>       </controller>
>       <controller type='pci' index='0' model='pci-root'/>
> -    <controller type='scsi' index='1'>
> +    <controller type='scsi' index='1' model='virtio-scsi'>
>         <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
>       </controller>
>       <input type='mouse' bus='ps2'/>
>