[libvirt] [PATCH v3] conf: Generate address for scsi host device automatically

[Osier Yang]

On 31/05/13 22:11, John Ferlan wrote:
> On 05/31/2013 06:09 AM, Osier Yang wrote:
>> With unknown good reasons, the attribute "bus" of scsi device
>> address is always set to 0, same for attribute "target". (See
>> virDomainDiskDefAssignAddress).
>>
>> Though we might need to change the algorithm to honor "bus"
>> and "target" too, that's a different issue. The address generator
>> for scsi host device in this patch just follows the unknown
>> good reasons, only considering the "controller" and "unit".
>> It walks through all scsi controllers and their units, to see
>> if the address $controller:0:0:$unit can be used (if not used
>> by any disk or scsi host device yet), if found one, it sits on
>> it, otherwise, it creates a new controller (actually the controller
>> is implicitly created by someone else), and sits on
>> $new_controller:0:0:0 instead.
>>
>> ---
>> v2 - v3:
>>    * Improve the for loop a bit with John's suggestion
>>
>> v1 - v2:
>>    * A new helper virDomainSCSIDriveAddressIsUsed
>>    * Move virDomainDefMaybeAddHostdevSCSIcontroller
>>    * More comments
>>    * Add testing.
>>    * problems fixed with the testing:
>>      1) s/nscsi_controllers + 1/nscsi_controllers/,
>>      2) Add the controller implicitly after a scsi hostdev def
>>         parsing, as it can use a new scsi controller index
>>         (nscsi_controllers), which should be added implicitly.
>> ---
>>   src/conf/domain_conf.c                             | 202 ++++++++++++++++++---
>>   .../qemuxml2argv-hostdev-scsi-autogen-address.xml  |  95 ++++++++++
>>   ...qemuxml2xmlout-hostdev-scsi-autogen-address.xml | 106 +++++++++++
>>   tests/qemuxml2xmltest.c                            |   2 +
>>   4 files changed, 375 insertions(+), 30 deletions(-)
>>   create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-autogen-address.xml
>>   create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-hostdev-scsi-autogen-address.xml
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 2b4e160..46d49a2 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -3782,6 +3782,141 @@ cleanup:
>>       return ret;
>>   }
>>   
>> +/* Check if a drive type address $controller:0:0:$unit is already
>> + * taken by a disk or not.
>> + */
>> +static bool
>> +virDomainDriveAddressIsUsedByDisk(virDomainDefPtr def,
>> +                                  enum virDomainDiskBus type,
>> +                                  unsigned int controller,
>> +                                  unsigned int unit)
>> +{
>> +    virDomainDiskDefPtr disk;
>> +    int i;
>> +
>> +    for (i = 0; i < def->ndisks; i++) {
>> +        disk = def->disks[i];
>> +
>> +        if (disk->bus != type ||
>> +            disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)
>> +            continue;
>> +
>> +        if (disk->info.addr.drive.controller == controller &&
>> +            disk->info.addr.drive.unit == unit &&
>> +            disk->info.addr.drive.bus == 0 &&
>> +            disk->info.addr.drive.target == 0)
>> +            return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +/* Check if a drive type address $controller:0:0:$unit is already
>> + * taken by a host device or not.
>> + */
>> +static bool
>> +virDomainDriveAddressIsUsedByHostdev(virDomainDefPtr def,
>> +                                     enum virDomainHostdevSubsysType type,
>> +                                     unsigned int controller,
>> +                                     unsigned int unit)
>> +{
>> +    virDomainHostdevDefPtr hostdev;
>> +    int i;
>> +
>> +    for (i = 0; i < def->nhostdevs; i++) {
>> +        hostdev = def->hostdevs[i];
>> +
>> +        if (hostdev->source.subsys.type != type)
>> +            continue;
>> +
>> +        if (hostdev->info->addr.drive.controller == controller &&
>> +            hostdev->info->addr.drive.unit == unit &&
>> +            hostdev->info->addr.drive.bus == 0 &&
>> +            hostdev->info->addr.drive.target == 0)
>> +            return true;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>> +static bool
>> +virDomainSCSIDriveAddressIsUsed(virDomainDefPtr def,
>> +                                unsigned int controller,
>> +                                unsigned int unit)
>> +{
>> +    /* In current implementation, the maximum unit number of a controller
>> +     * is either 16 or 7 (narrow SCSI bus), and if the maximum unit number
>> +     * is 16, the controller itself is on unit 7 */
>> +    if (unit == 7)
>> +        return true;
> Given the comments in:
>
> https://www.redhat.com/archives/libvir-list/2013-May/msg02017.html
>
> The comment above is still confusing.
>
> As far as I understand things units are 0-7 with 8 being the max for
> narrow and 0-15 with 16 being the max for wide.
>
> It seems as though for narrow there are 8 units, but 7 is reserved for
> the controller, thus leaving a perception of only 7 max units.  Make
> sense so far?

Yes. But I need to confirm.

>
> What I'm not clear on with the above check is if we did have a wide bus
> (eg, 16 possible units), then is unit 7 still reserved for the
> controller?

Yes, even if we add the hasWideScsiBus for qemu driver, as domain_conf.c
is for all of the drivers, we need to keep it as 7. Unless there is critical
reasons for changing it.

> If so, indicate that and the code is fine, but the comment
> could be made less confusing. If not, then you need to have (readd) the
> check for max units again.

For narrow bus, we didn't declare anywhere to say the max unit "7" is
occupied by the controller. And actually in the code, it only uses "0-6"
for allocation. But regardless of whether it will use the 7 for the 
controller
itself or not.  Simple checking with

if (unit == 7)
     return true;

is fine. No need to have a redundant checking like:

if (max_unit == 16 && unit ==7)

Because even the 7 will be taken by the controller itself, it's still used,
and returning true is correct.

The comment like below

+    /* In current implementation, the maximum unit number of a controller
+     * is either 16 or 7 (narrow SCSI bus), and if the maximum unit number
+     * is 16, the controller itself is on unit 7 */

just follows what we do in the current code (it doesn't declare what will
the 7 will be used for).

I think what you are complaining is "and if the maximum unit number
is 16, the controller itself is on unit 7", what you want to see is
"the controller itself is on unit 7", right?

I agree with it, but before make the changing, I need to confirm whether
the controller is on the 7 indeed. And makes the changes if it is together
with changing on comment in virDomainDiskDefAssignAddress too. And it can
be a later patch.

Osier