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

[John Ferlan]

On 12/15/2017 11:32 AM, Eric Farman wrote:
> 
> 
> 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?
> 

This API is called from virDomainHostdevDefPostParse only when the
<hostdev> 'type' is VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI when the
<address> 'type' is VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE.

So we're already limited to a very specific set. This would have been
needed if we were running through all the controllers because we
couldn't add to a non SCSI controller.

>> -
>> -        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.
> 

We're not really walking the controller list, we're in a function that
is being called for every hostdev in the 'nhostdevs' list.


John

>> 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;
>>   }
>>
>