[libvirt] [PATCH v2 2/2] domain_conf: Relax SCSI addr used check

[Daniel P. Berrangé]

On Wed, Sep 11, 2019 at 03:17:42PM +0200, Michal Privoznik wrote:
> In domain_conf.c we have virDomainSCSIDriveAddressIsUsed()
> function which returns true or false if given drive address is
> already in use for given domain config or not. However, it also
> takes a shortcut and returns true (meaning address in use) if the
> unit number equals 7. This is because for some controllers this
> is reserved address. The limitation comes mostly from vmware and
> applies to lsilogic, buslogic, spapr-vscsi and vmpvscsi models.
> On the other hand, we were not checking for the maximum unit
> number (aka LUN number) which is also relevant and differs from
> model to model.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/conf/domain_conf.c | 53 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 48 insertions(+), 5 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 69c486f382..3e7603891f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -4813,11 +4813,54 @@ bool
>  virDomainSCSIDriveAddressIsUsed(const virDomainDef *def,
>                                  const virDomainDeviceDriveAddress *addr)
>  {
> -    /* In current implementation, the maximum unit number of a controller
> -     * is either 16 or 7 (narrow SCSI bus), and if the maximum unit number
> -     * is 16, the controller itself is on unit 7 */
> -    if (addr->unit == 7)
> -        return true;
> +    const virDomainControllerDef *cont;
> +
> +    cont = virDomainDeviceFindSCSIController(def, addr);
> +    if (cont) {
> +        int max = -1;
> +        int reserved = -1;
> +
> +        /* Different controllers have different limits. These limits here are
> +         * taken from QEMU source code, but nevertheless they should apply to
> +         * other hypervisors too. */
> +        switch ((virDomainControllerModelSCSI) cont->model) {
> +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
> +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_TRANSITIONAL:
> +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_NON_TRANSITIONAL:
> +            max = 16383;
> +            break;
> +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
> +            max = 31;
> +            reserved = 7;
> +            break;
> +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068:
> +            max = 1;
> +            break;
> +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
> +            max = 255;
> +            break;
> +        case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
> +            max = 0;

        Surely this ^^^ means....

> +        if (max != -1 && addr->unit >= max)
> +            return true;

...this is always true and so we'll be unable to attach
a drive to any LUN at all.

> +        if (reserved != -1 && addr->unit == reserved)
> +            return true;
> +    }
>  
>      if (virDomainDriveAddressIsUsedByDisk(def, VIR_DOMAIN_DISK_BUS_SCSI,
>                                            addr) ||

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|