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
The 'max_unit' check is irrelevant if max_unit=7, then we're not getting
here anyway...
+ 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)
+{
+ 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?
+
+ 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?
+ 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
If there is one existing/full controller, then we start at 2
How does the 'idx' value get populated? Is it possible that this
setting conflicts with a different and non scsi controller?
Is there a max controller number we shouldn't go past?
John
+ hostdev->info->addr.drive.bus = 0;
+ hostdev->info->addr.drive.target = 0;
+ hostdev->info->addr.drive.unit = 0;
+ }
+
+ return 0;
+}
+
static virDomainHostdevDefPtr
-virDomainHostdevDefParseXML(const xmlNodePtr node,
+virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt,
+ virDomainDefPtr vmdef,
+ const xmlNodePtr node,
xmlXPathContextPtr ctxt,
virHashTablePtr bootHash,
unsigned int flags)
@@ -8719,7 +8851,8 @@ virDomainHostdevDefParseXML(const xmlNodePtr node,
}
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
- if (def->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+ if (def->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+ virDomainHostdevAssignAddress(xmlopt, vmdef, def) < 0) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("SCSI host devices must have address
specified"));
goto error;
@@ -9128,8 +9261,8 @@ virDomainDeviceDefParse(const char *xmlStr,
goto error;
} else if (xmlStrEqual(node->name, BAD_CAST "hostdev")) {
dev->type = VIR_DOMAIN_DEVICE_HOSTDEV;
- if (!(dev->data.hostdev = virDomainHostdevDefParseXML(node, ctxt, NULL,
- flags)))
+ if (!(dev->data.hostdev = virDomainHostdevDefParseXML(xmlopt, def, node,
+ ctxt, NULL, flags)))
goto error;
} else if (xmlStrEqual(node->name, BAD_CAST "controller")) {
dev->type = VIR_DOMAIN_DEVICE_CONTROLLER;
@@ -11477,7 +11610,8 @@ virDomainDefParseXML(xmlDocPtr xml,
for (i = 0 ; i < n ; i++) {
virDomainHostdevDefPtr hostdev;
- hostdev = virDomainHostdevDefParseXML(nodes[i], ctxt, bootHash, flags);
+ hostdev = virDomainHostdevDefParseXML(xmlopt, def, nodes[i],
+ ctxt, bootHash, flags);
if (!hostdev)
goto error;