[libvirt] [PATCH 4/4] conf: Fix generating addresses for SCSI hostdev
John Ferlan
jferlan at redhat.com
Thu Jan 4 12:24:30 UTC 2018
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 at redhat.com>
>>> ---
>>> 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 at redhat.com
>>> https://www.redhat.com/mailman/listinfo/libvir-list
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
More information about the libvir-list
mailing list