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

On 12/20/2017 07:38 AM, Ján Tomko wrote: > On Wed, Dec 06, 2017 at 08:08:06AM -0500, 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(a)redhat.com&gt; >> --- >> 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; > > Please keep the descriptive 'next_unit' variable name. > >> -    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; >> - >> -        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. 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); >> + > > Easier to read as: > for (next_unit = -1; next_unit < -1; controller++) >    next_unit = virDomainControllerSCSINextUnit(def, max_unit, controller); > Not functionally the same comparisons... Caused hostdev-scsi-autogen-address to fail... There's 11 hostdevs and only 1 controller. We never get controller==1 from that for loop. I can change @ret above to be @next_unit though

John > ACK > > Jan >> >>     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; >> } >> --  >> 2.13.6 >> >> -- >> libvir-list mailing list >> libvir-list(a)redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list