[libvirt] [PATCH 4/4] conf: Fix generating addresses for SCSI hostdev

[Eric Farman]

On 12/06/2017 08:08 AM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1519130

> 
> Commit id 'dc692438' reverted the automagic addition of a SCSI
> controller attempt during virDomainHostdevAssignAddress; however,
> the logic to determine where to place the next_unit depended upon
> the "new" controller being added.  Without the new controller the
> the next time through the call for the next SCSI hostdev found
> would result in the "next_unit" never changing from 0 (zero) and
> as a result the addition of the device will fail due to being a
> duplicate unit number of the first with the error message:
> 
>    virDomainDefCheckDuplicateDriveAddresses:$line : unsupported
>        configuration: SCSI host address controller='0' bus='1'
>        target='0' unit='0' in use by another SCSI host device
> 
> So instead of walking the controller list looking for SCSI
> controllers, all we can do is "pretend" that they exist and
> allow other code to create them later as necessary.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>   src/conf/domain_conf.c | 31 +++++++++++++------------------
>   1 file changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 61b4a0075..73c6708cf 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -4322,10 +4322,8 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt,
>                                 const virDomainDef *def,
>                                 virDomainHostdevDefPtr hostdev)
>   {
> -    int next_unit = 0;
> -    unsigned controller = 0;
> +    int controller = 0;
>       unsigned int max_unit;
> -    size_t i;
>       int ret;
> 
>       if (xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_WIDE_SCSI)
> @@ -4333,33 +4331,30 @@ virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt,
>       else
>           max_unit = SCSI_NARROW_BUS_MAX_CONT_UNIT;
> 
> -    for (i = 0; i < def->ncontrollers; i++) {
> -        if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
> -            continue;

Don't we still need this check in the case of non-SCSI controllers 
intermixed with SCSI ones?

> -
> -        controller++;
> -        ret = virDomainControllerSCSINextUnit(def, max_unit,
> -                                              def->controllers[i]->idx);
> -        if (ret >= 0) {
> -            next_unit = ret;
> -            controller = def->controllers[i]->idx;
> -            break;
> -        }
> -    }
> -
>       /* NB: Do not attempt calling virDomainDefMaybeAddController to
>        * automagically add a "new" controller. Doing so will result in
>        * qemuDomainFindOrCreateSCSIDiskController "finding" the controller
>        * in the domain def list and thus not hotplugging the controller as
>        * well as the hostdev in the event that there are either no SCSI
>        * controllers defined or there was no space on an existing one.
> +     *
> +     * Because we cannot add a controller, then we should not walk the
> +     * defined controllers list in order to find empty space. 

But we do walk the list, we just don't use a for loop to do it.

> Doing
> +     * so fails to return the valid next unit number for the 2nd
> +     * hostdev being added to the as yet to be created controller.
>        */
> +    do {
> +        ret = virDomainControllerSCSINextUnit(def, max_unit, controller);
> +        if (ret < 0)
> +            controller++;
> +    } while (ret < 0);
> +

I do like the simplification of the loop though!

  - Eric

> 
>       hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
>       hostdev->info->addr.drive.controller = controller;
>       hostdev->info->addr.drive.bus = 0;
>       hostdev->info->addr.drive.target = 0;
> -    hostdev->info->addr.drive.unit = next_unit;
> +    hostdev->info->addr.drive.unit = ret;
> 
>       return 0;
>   }
>