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

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