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@xxxxxxxxxx> >> --- >> 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; >> } >> > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
