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;