Jan - ping on the question from my response to your review... On 12/20/2017 01:33 PM, John Ferlan wrote: > > > 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@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; >> >> 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 > Is changing to use next_unit enough? Or did you have some other easier to read loop that actually works that you'd prefer to see? Tks - > > 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@xxxxxxxxxx >>> https://www.redhat.com/mailman/listinfo/libvir-list > > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
