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@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;
- 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);
+
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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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;
> - 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;
> }
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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;
>> - 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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);
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@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
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@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@redhat.com
>>> https://www.redhat.com/mailman/listinfo/libvir-list
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jan 04, 2018 at 07:24:30AM -0500, John Ferlan wrote: > >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: [...] >>> >>> 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? Yes. >Or did you have some other easier >to read loop that actually works that you'd prefer to see? > I do not. Jan -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.