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

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

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