On 07/05/13 21:42, John Ferlan wrote:
On 05/03/2013 02:07 PM, 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 algrithom to honor "bus"
s/algrithom/algorithm
and "target" too, it's another story. The address generator
s/it's another story/that's a different issue
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 found
one, it sits on it, otherwise, it creates a new controller
(actually the controller is created implicitly by someone
else), and sits on $new_controller:0:0:0 instead.
Implicit creation
---
Since it needs to add the controllers for "drive" type address
implicitly, I add the generator in domain_conf.c instead of
the specific the driver, e.g. qemu.
---
src/conf/domain_conf.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 139 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 31a8c46..cff2b46 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8658,8 +8658,140 @@ error:
return NULL;
}
+/* 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;
+}
+
+/* Find out the next usable "unit" of a specific controller */
+static int
+virDomainControllerSCSINextUnit(virDomainDefPtr def,
+ unsigned int max_unit,
+ unsigned int controller)
+{
+ int i;
+
+ for (i = 0; i < max_unit; i++) {
+ /* The controller itself is on unit 7 */
+ if (max_unit == 16 && i == 7)
I would expect 16 and 7 to be constants
They are used in old code, I intended to make a later patch to get
rid of the new and old ones together, with defining macros.
The 'max_unit' check is irrelevant if max_unit=7, then we're not getting
here anyway...
Will send a v2 to cleanup this.
+ continue;
+
+ if (!virDomainDriveAddressIsUsedByDisk(def,
+ VIR_DOMAIN_DISK_BUS_SCSI, controller, i) &&
+ !virDomainDriveAddressIsUsedByHostdev(def,
+ VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, controller, i))
+ return i;
+ }
+
+ return -1;
+}
+
+static int
+virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt,
+ virDomainDefPtr def,
+ virDomainHostdevDefPtr hostdev)
This ends up being Scsi specific right? E.G. AssignSCSIAddress instead
of generic AssignAddress (or wherever the "best" place is to put SCSI)
I intended to make it general enough to be used for other type
hostdev in future. There is checking below.
+{
+ unsigned int max_unit;
+ int next_unit;
+ unsigned nscsi_controllers;
s/;/ = 0;
+ bool found;
+ int i;
+
+ if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)
+ return -1;
+
+ /* See comments in virDomainDiskDefAssignAddress */
+ if (xmlopt->config.hasWideScsiBus)
+ max_unit = 16;
+ else
+ max_unit = 7;
Guess I would have figured these would be constants somewhere... Not
that I expect the values to change...
Units are numbered 0 -> 15 and 0 -> 6 then?
Yeah, with s/and/or/,
+
+ for (i = 0; i < def->ncontrollers; i++) {
+ if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI)
+ continue;
+
+ nscsi_controllers++;
If we're "implicitly" creating a new controller, then is it's number
based on the total number of controllers or the next scsi controller?
s/number/index/, Based on the number of specific type controller.
+ next_unit = virDomainControllerSCSINextUnit(def, max_unit,
+ def->controllers[i]->idx);
+ if (next_unit >= 0) {
+ found = true;
+ break;
+ }
+ }
+
+ hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE;
+
+ if (found) {
+ hostdev->info->addr.drive.controller = def->controllers[i]->idx;
+ hostdev->info->addr.drive.bus = 0;
+ hostdev->info->addr.drive.target = 0;
+ hostdev->info->addr.drive.unit = next_unit;
+ } else {
+ hostdev->info->addr.drive.controller = nscsi_controllers + 1;
To be fair, I'm not familiar (yet) with how things are laid out, but it
would seem to me that this might not be returning what you want/expect.
I'm missing a nuance here - the 'controller' field is generally based
off the def->controllers[i]->idx value, except when we're "implicitly
creating" one. In this case it's just a 'random' value based on some
count of existing nscsi_controllers.
If there are zero existing controllers, then we start at 1
s/1/0/,
If there is one existing/full controller, then we start at 2
s/2/1/,
How does the 'idx' value get populated? Is it possible that this
setting conflicts with a different and non scsi controller?
No, the controller index is classified by the controller type. E.g.
<controller type='usb' index='0'>
<address type='pci' domain='0x0000' bus='0x00' slot='0x01'
function='0x2'/>
</controller>
<controller type='ide' index='0'>
<address type='pci' domain='0x0000' bus='0x00' slot='0x01'
function='0x1'/>
</controller>
<controller type='scsi' index='0' model='virtio-scsi'>
<address type='pci' domain='0x0000' bus='0x00' slot='0x03'
function='0x0'/>
</controller>
They have same index (idx in the internal struct).
Is there a max controller number we shouldn't go past?
I'm not clear about the what the max controller number is, but it depends on
the hypervisor.
Osier
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list