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