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 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. --- 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 | 209 ++++++++++++++++++--- .../qemuxml2argv-hostdev-scsi-autogen-address.xml | 95 ++++++++++ ...qemuxml2xmlout-hostdev-scsi-autogen-address.xml | 106 +++++++++++ tests/qemuxml2xmltest.c | 2 + 4 files changed, 382 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 ad5550c..86d743b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3782,6 +3782,148 @@ 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; + + if (virDomainDriveAddressIsUsedByDisk(def, VIR_DOMAIN_DISK_BUS_SCSI, + controller, unit) || + virDomainDriveAddressIsUsedByHostdev(def, VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, + controller, unit)) + 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++) { + if (!virDomainSCSIDriveAddressIsUsed(def, controller, i)) + return i; + } + + return -1; +} + +#define SCSI_WIDE_BUS_MAX_CONT_UNIT 16 +#define SCSI_NARROW_BUS_MAX_CONT_UNIT 7 + +static int +virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, + virDomainDefPtr def, + virDomainHostdevDefPtr hostdev) +{ + int next_unit; + unsigned nscsi_controllers = 0; + bool found = false; + int i; + + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) + return -1; + + for (i = 0; i < def->ncontrollers; i++) {

+ if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) + continue; + + nscsi_controllers++; + next_unit = virDomainControllerSCSINextUnit(def, + xmlopt->config.hasWideScsiBus ? + SCSI_WIDE_BUS_MAX_CONT_UNIT : + SCSI_NARROW_BUS_MAX_CONT_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; + hostdev->info->addr.drive.bus = 0; + hostdev->info->addr.drive.target = 0; + hostdev->info->addr.drive.unit = 0; + }

+ + return 0; +} + static int virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, xmlXPathContextPtr ctxt, @@ -8802,7 +8944,9 @@ error: } static virDomainHostdevDefPtr -virDomainHostdevDefParseXML(const xmlNodePtr node, +virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, + virDomainDefPtr vmdef, + const xmlNodePtr node, xmlXPathContextPtr ctxt, virHashTablePtr bootHash, unsigned int flags) @@ -8862,7 +9006,9 @@ 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; @@ -9271,8 +9417,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; @@ -10255,6 +10401,30 @@ error: return NULL; } +static int +virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) +{ + /* Look for any hostdev scsi dev */ + int i; + int maxController = -1; + virDomainHostdevDefPtr hostdev; + + for (i = 0; i < def->nhostdevs; i++) { + hostdev = def->hostdevs[i]; + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && + (int)hostdev->info->addr.drive.controller > maxController) { + maxController = hostdev->info->addr.drive.controller; + } + } + + for (i = 0; i <= maxController; i++) { + if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, -1) < 0) + return -1; + } + + return 0; +} static virDomainDefPtr virDomainDefParseXML(xmlDocPtr xml, @@ -11618,7 +11788,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; @@ -11631,6 +11802,9 @@ virDomainDefParseXML(xmlDocPtr xml, } def->hostdevs[def->nhostdevs++] = hostdev; + + if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0) + goto error; } VIR_FREE(nodes); @@ -13232,31 +13406,6 @@ virDomainDefMaybeAddSmartcardController(virDomainDefPtr def) return 0; } -static int -virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) -{ - /* Look for any hostdev scsi dev */ - int i; - int maxController = -1; - virDomainHostdevDefPtr hostdev; - - for (i = 0; i < def->nhostdevs; i++) { - hostdev = def->hostdevs[i]; - if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && - (int)hostdev->info->addr.drive.controller > maxController) { - maxController = hostdev->info->addr.drive.controller; - } - } - - for (i = 0; i <= maxController; i++) { - if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, -1) < 0) - return -1; - } - - return 0; -} - /* * Based on the declared <address/> info for any devices, * add necessary drive controllers which are not already present diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-autogen-address.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-autogen-address.xml new file mode 100644 index 0000000..21f112b --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-autogen-address.xml @@ -0,0 +1,95 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host0'/> + <address bus='0' target='0' unit='0'/> + </source> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host1'/> + <address bus='0' target='0' unit='1'/> + </source> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host2'/> + <address bus='0' target='0' unit='2'/> + </source> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host3'/> + <address bus='0' target='0' unit='3'/> + </source> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host4'/> + <address bus='0' target='0' unit='4'/> + </source> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host5'/> + <address bus='0' target='0' unit='5'/> + </source> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host6'/> + <address bus='0' target='0' unit='6'/> + </source> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host7'/> + <address bus='0' target='0' unit='7'/> + </source> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host8'/> + <address bus='0' target='0' unit='8'/> + </source> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host9'/> + <address bus='0' target='0' unit='9'/> + </source> + <address type='drive' controller='1' bus='0' target='0' unit='5'/>

+ </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host10'/> + <address bus='0' target='0' unit='10'/> + </source> + </hostdev> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-hostdev-scsi-autogen-address.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-hostdev-scsi-autogen-address.xml new file mode 100644 index 0000000..e416654 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-hostdev-scsi-autogen-address.xml @@ -0,0 +1,106 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <controller type='scsi' index='1'/> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host0'/> + <address bus='0' target='0' unit='0'/> + </source> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host1'/> + <address bus='0' target='0' unit='1'/> + </source> + <address type='drive' controller='0' bus='0' target='0' unit='1'/> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host2'/> + <address bus='0' target='0' unit='2'/> + </source> + <address type='drive' controller='0' bus='0' target='0' unit='2'/> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host3'/> + <address bus='0' target='0' unit='3'/> + </source> + <address type='drive' controller='0' bus='0' target='0' unit='3'/> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host4'/> + <address bus='0' target='0' unit='4'/> + </source> + <address type='drive' controller='0' bus='0' target='0' unit='4'/> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host5'/> + <address bus='0' target='0' unit='5'/> + </source> + <address type='drive' controller='0' bus='0' target='0' unit='5'/> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host6'/> + <address bus='0' target='0' unit='6'/> + </source> + <address type='drive' controller='0' bus='0' target='0' unit='6'/> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host7'/> + <address bus='0' target='0' unit='7'/> + </source> + <address type='drive' controller='1' bus='0' target='0' unit='0'/> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host8'/> + <address bus='0' target='0' unit='8'/> + </source> + <address type='drive' controller='1' bus='0' target='0' unit='1'/> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host9'/> + <address bus='0' target='0' unit='9'/> + </source> + <address type='drive' controller='1' bus='0' target='0' unit='5'/> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source> + <adapter name='scsi_host10'/> + <address bus='0' target='0' unit='10'/> + </source> + <address type='drive' controller='1' bus='0' target='0' unit='2'/> + </hostdev> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 64a271c..06e4206 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -298,6 +298,8 @@ mymain(void) DO_TEST("hostdev-scsi-shareable"); DO_TEST("hostdev-scsi-sgio"); + DO_TEST_DIFFERENT("hostdev-scsi-autogen-address"); +

virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt);

On 28/05/13 23:09, John Ferlan wrote: > On 05/23/2013 12:48 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 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 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. >> >> --- >> 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. > First pass of code is found at: > > https://www.redhat.com/archives/libvir-list/2013-May/msg00340.html > > The previous/initial review and reply is found at: > > https://www.redhat.com/archives/libvir-list/2013-May/msg00506.html > > https://www.redhat.com/archives/libvir-list/2013-May/msg00526.html > >> --- >> src/conf/domain_conf.c | 209 >> ++++++++++++++++++--- >> .../qemuxml2argv-hostdev-scsi-autogen-address.xml | 95 ++++++++++ >> ...qemuxml2xmlout-hostdev-scsi-autogen-address.xml | 106 +++++++++++ >> tests/qemuxml2xmltest.c | 2 + >> 4 files changed, 382 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 ad5550c..86d743b 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -3782,6 +3782,148 @@ 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) > > FYI: The previous code cared that a wide address was being used when > returning true - this code doesn't. Just making sure that change is > intentional. Essentially, 7 cannot be used. The comments above the > code talk about wide/narrow which it seems becomes irrelevant. The > reality seems to be that you are avoiding using 7 altogether because > it's reserved for the controller itself, correct? Regardless of whether > a wide or narrow. The controller itself uses the unit number of the > maximum value of the narrow band. At least that's how I'm reading it. You are right, will update the comment.

> >> + return true; >> + >> + if (virDomainDriveAddressIsUsedByDisk(def, >> VIR_DOMAIN_DISK_BUS_SCSI, >> + controller, unit) || >> + virDomainDriveAddressIsUsedByHostdev(def, >> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, >> + controller, unit)) >> +} >> + >> +/* 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++) { >> + if (!virDomainSCSIDriveAddressIsUsed(def, controller, i)) >> + return i; >> + } >> + >> + return -1; >> +} >> + >> +#define SCSI_WIDE_BUS_MAX_CONT_UNIT 16 >> +#define SCSI_NARROW_BUS_MAX_CONT_UNIT 7 >> + >> +static int >> +virDomainHostdevAssignAddress(virDomainXMLOptionPtr xmlopt, >> + virDomainDefPtr def, >> + virDomainHostdevDefPtr hostdev) >> +{ >> + int next_unit; >> + unsigned nscsi_controllers = 0; >> + bool found = false; >> + int i; >> + >> + if (hostdev->source.subsys.type != >> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) >> + return -1; >> + >> + for (i = 0; i < def->ncontrollers; i++) { > As a for loop exit condition, consider either using > > && !found > or > > && next_unit == -1 > > as long as you initialize next_unit = -1 above... > >> + if (def->controllers[i]->type != >> VIR_DOMAIN_CONTROLLER_TYPE_SCSI) >> + continue; >> + >> + nscsi_controllers++; >> + next_unit = virDomainControllerSCSINextUnit(def, >> + xmlopt->config.hasWideScsiBus ? >> + SCSI_WIDE_BUS_MAX_CONT_UNIT : >> + SCSI_NARROW_BUS_MAX_CONT_UNIT, >> + def->controllers[i]->idx); >> + if (next_unit >= 0) { >> + found = true; >> + break; > break; is irrelevant using !found in loop exit or the whole statement is > irrelevent if using next_unit == -1 for loop exit... > >> + } >> + } >> + >> + 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; >> + hostdev->info->addr.drive.bus = 0; >> + hostdev->info->addr.drive.target = 0; >> + hostdev->info->addr.drive.unit = 0; >> + } > > Although it looked odd initially, I think I convinced myself the logic > is OK. Initially, nscsi_controllers = 0; > > If none exist, then 'controller = 0' and 'units' are assigned until > full, right? Right. > If one exists and is full, then 'controller = 1' and is > placed at unit = 0. Right. > > NOTE: > Using next_unit to break the loop means this code could be rewritten as: > > hostdev->info->addr.drive.controller = (next_unit >= 0 ? > def->controllers[i]->idx : > nscsi_controllers) > hostdev->info->addr.drive.bus = 0; > hostdev->info->addr.drive.target = 0; > hostdev->info->addr.drive.unit = (next_unit >= 0 ? next_unit : 0); > > Although one could argue setting "= 0;" is redundant due to allocation. Compact. I will consider your suggestion for the loop in v3. > > > >> + >> + return 0; >> +} >> + >> static int >> virDomainHostdevDefParseXMLSubsys(xmlNodePtr node, >> xmlXPathContextPtr ctxt, >> @@ -8802,7 +8944,9 @@ error: >> } >> static virDomainHostdevDefPtr >> -virDomainHostdevDefParseXML(const xmlNodePtr node, >> +virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, >> + virDomainDefPtr vmdef, >> + const xmlNodePtr node, >> xmlXPathContextPtr ctxt, >> virHashTablePtr bootHash, >> unsigned int flags) >> @@ -8862,7 +9006,9 @@ 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; >> @@ -9271,8 +9417,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; >> @@ -10255,6 +10401,30 @@ error: >> return NULL; >> } >> +static int >> +virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) >> +{ >> + /* Look for any hostdev scsi dev */ >> + int i; >> + int maxController = -1; >> + virDomainHostdevDefPtr hostdev; >> + >> + for (i = 0; i < def->nhostdevs; i++) { >> + hostdev = def->hostdevs[i]; >> + if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && >> + hostdev->source.subsys.type == >> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && >> + (int)hostdev->info->addr.drive.controller > >> maxController) { >> + maxController = hostdev->info->addr.drive.controller; >> + } >> + } >> + >> + for (i = 0; i <= maxController; i++) { >> + if (virDomainDefMaybeAddController(def, >> VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, -1) < 0) >> + return -1; >> + } >> + >> + return 0; >> +} >> static virDomainDefPtr >> virDomainDefParseXML(xmlDocPtr xml, >> @@ -11618,7 +11788,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; >> @@ -11631,6 +11802,9 @@ virDomainDefParseXML(xmlDocPtr xml, >> } >> def->hostdevs[def->nhostdevs++] = hostdev; >> + >> + if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0) >> + goto error; >> } >> VIR_FREE(nodes); >> @@ -13232,31 +13406,6 @@ >> virDomainDefMaybeAddSmartcardController(virDomainDefPtr def) >> return 0; >> } >> -static int >> -virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def) >> -{ >> - /* Look for any hostdev scsi dev */ >> - int i; >> - int maxController = -1; >> - virDomainHostdevDefPtr hostdev; >> - >> - for (i = 0; i < def->nhostdevs; i++) { >> - hostdev = def->hostdevs[i]; >> - if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && >> - hostdev->source.subsys.type == >> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && >> - (int)hostdev->info->addr.drive.controller > >> maxController) { >> - maxController = hostdev->info->addr.drive.controller; >> - } >> - } >> - >> - for (i = 0; i <= maxController; i++) { >> - if (virDomainDefMaybeAddController(def, >> VIR_DOMAIN_CONTROLLER_TYPE_SCSI, i, -1) < 0) >> - return -1; >> - } >> - >> - return 0; >> -} >> - >> /* >> * Based on the declared <address/> info for any devices, >> * add necessary drive controllers which are not already present >> diff --git >> a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-autogen-address.xml >> b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-autogen-address.xml >> new file mode 100644 >> index 0000000..21f112b >> --- /dev/null >> +++ >> b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-autogen-address.xml >> @@ -0,0 +1,95 @@ >> +<domain type='qemu'> >> + <name>QEMUGuest2</name> >> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> >> + <memory unit='KiB'>219100</memory> >> + <currentMemory unit='KiB'>219100</currentMemory> >> + <vcpu placement='static'>1</vcpu> >> + <os> >> + <type arch='i686' machine='pc'>hvm</type> >> + <boot dev='hd'/> >> + </os> >> + <clock offset='utc'/> >> + <on_poweroff>destroy</on_poweroff> >> + <on_reboot>restart</on_reboot> >> + <on_crash>destroy</on_crash> >> + <devices> >> + <emulator>/usr/bin/qemu</emulator> >> + <disk type='block' device='disk'> >> + <source dev='/dev/HostVG/QEMUGuest2'/> >> + <target dev='hda' bus='ide'/> >> + <address type='drive' controller='0' bus='0' target='0' >> unit='0'/> >> + </disk> >> + <controller type='scsi' index='0' model='virtio-scsi'/> >> + <controller type='usb' index='0'/> >> + <controller type='ide' index='0'/> >> + <controller type='pci' index='0' model='pci-root'/> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host0'/> >> + <address bus='0' target='0' unit='0'/> >> + </source> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host1'/> >> + <address bus='0' target='0' unit='1'/> >> + </source> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host2'/> >> + <address bus='0' target='0' unit='2'/> >> + </source> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host3'/> >> + <address bus='0' target='0' unit='3'/> >> + </source> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host4'/> >> + <address bus='0' target='0' unit='4'/> >> + </source> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host5'/> >> + <address bus='0' target='0' unit='5'/> >> + </source> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host6'/> >> + <address bus='0' target='0' unit='6'/> >> + </source> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host7'/> >> + <address bus='0' target='0' unit='7'/> >> + </source> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host8'/> >> + <address bus='0' target='0' unit='8'/> >> + </source> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host9'/> >> + <address bus='0' target='0' unit='9'/> >> + </source> >> + <address type='drive' controller='1' bus='0' target='0' >> unit='5'/> > [1] see below > >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host10'/> >> + <address bus='0' target='0' unit='10'/> >> + </source> >> + </hostdev> >> + <memballoon model='virtio'/> >> + </devices> >> +</domain> >> diff --git >> a/tests/qemuxml2xmloutdata/qemuxml2xmlout-hostdev-scsi-autogen-address.xml >> b/tests/qemuxml2xmloutdata/qemuxml2xmlout-hostdev-scsi-autogen-address.xml >> >> new file mode 100644 >> index 0000000..e416654 >> --- /dev/null >> +++ >> b/tests/qemuxml2xmloutdata/qemuxml2xmlout-hostdev-scsi-autogen-address.xml >> @@ -0,0 +1,106 @@ >> +<domain type='qemu'> >> + <name>QEMUGuest2</name> >> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> >> + <memory unit='KiB'>219100</memory> >> + <currentMemory unit='KiB'>219100</currentMemory> >> + <vcpu placement='static'>1</vcpu> >> + <os> >> + <type arch='i686' machine='pc'>hvm</type> >> + <boot dev='hd'/> >> + </os> >> + <clock offset='utc'/> >> + <on_poweroff>destroy</on_poweroff> >> + <on_reboot>restart</on_reboot> >> + <on_crash>destroy</on_crash> >> + <devices> >> + <emulator>/usr/bin/qemu</emulator> >> + <disk type='block' device='disk'> >> + <source dev='/dev/HostVG/QEMUGuest2'/> >> + <target dev='hda' bus='ide'/> >> + <address type='drive' controller='0' bus='0' target='0' >> unit='0'/> >> + </disk> >> + <controller type='scsi' index='0' model='virtio-scsi'/> >> + <controller type='usb' index='0'/> >> + <controller type='ide' index='0'/> >> + <controller type='pci' index='0' model='pci-root'/> >> + <controller type='scsi' index='1'/> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host0'/> >> + <address bus='0' target='0' unit='0'/> >> + </source> >> + <address type='drive' controller='0' bus='0' target='0' >> unit='0'/> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host1'/> >> + <address bus='0' target='0' unit='1'/> >> + </source> >> + <address type='drive' controller='0' bus='0' target='0' >> unit='1'/> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host2'/> >> + <address bus='0' target='0' unit='2'/> >> + </source> >> + <address type='drive' controller='0' bus='0' target='0' >> unit='2'/> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host3'/> >> + <address bus='0' target='0' unit='3'/> >> + </source> >> + <address type='drive' controller='0' bus='0' target='0' >> unit='3'/> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host4'/> >> + <address bus='0' target='0' unit='4'/> >> + </source> >> + <address type='drive' controller='0' bus='0' target='0' >> unit='4'/> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host5'/> >> + <address bus='0' target='0' unit='5'/> >> + </source> >> + <address type='drive' controller='0' bus='0' target='0' >> unit='5'/> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host6'/> >> + <address bus='0' target='0' unit='6'/> >> + </source> >> + <address type='drive' controller='0' bus='0' target='0' >> unit='6'/> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host7'/> >> + <address bus='0' target='0' unit='7'/> >> + </source> >> + <address type='drive' controller='1' bus='0' target='0' >> unit='0'/> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host8'/> >> + <address bus='0' target='0' unit='8'/> >> + </source> >> + <address type='drive' controller='1' bus='0' target='0' >> unit='1'/> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host9'/> >> + <address bus='0' target='0' unit='9'/> >> + </source> >> + <address type='drive' controller='1' bus='0' target='0' >> unit='5'/> >> + </hostdev> >> + <hostdev mode='subsystem' type='scsi' managed='yes'> >> + <source> >> + <adapter name='scsi_host10'/> >> + <address bus='0' target='0' unit='10'/> >> + </source> >> + <address type='drive' controller='1' bus='0' target='0' >> unit='2'/> >> + </hostdev> >> + <memballoon model='virtio'/> >> + </devices> >> +</domain> >> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c >> index 64a271c..06e4206 100644 >> --- a/tests/qemuxml2xmltest.c >> +++ b/tests/qemuxml2xmltest.c >> @@ -298,6 +298,8 @@ mymain(void) >> DO_TEST("hostdev-scsi-shareable"); >> DO_TEST("hostdev-scsi-sgio"); >> + DO_TEST_DIFFERENT("hostdev-scsi-autogen-address"); >> + > [1] > Still don't understand the test setup 100%, but I think for the purposes > of what you're trying to prove that the defined unit=5 above should be 1 > or 2 and then ensure that '3' gets created to prove the algorithm will > find one at '1' or '2'. As it stands all that seems to be provided is > the scsi10 will use '3' even though '5' exists. Yes, I want to prove it will allocate the smallest available unit number, instead of increasing on top of the current largest unit number. /me should document this in the commit log. > > Curious to understand why the 2nd controller gets created without the > "model='virtio-scsi'"? Does it matter? Does creating a controller on > the fly like this would seem to imply that the controllers would be > alike. It's a problem actually, I intended to write a RFC for it, but not posted yet. Assuming that one wants to hotplug a virtio-scsi disk with XML: <disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/f18_sdb.img'/> <target dev='sda' bus='scsi'/> <address type='drive' controller='1' bus='0' target='0' unit='0'/> </disk> If there is no scsi controller with index "1", the implicitly added scsi controller will use the default scsi controller model "lsilogic" (true for qemu/kvm). That's obviously not what the user wants. Though we provide the way to hotplug a controller, one can hotplug a controller first, and then hotplug the disk. But it's just a bit better, as it still needs some manual work. There are many other problems, I'm going to write them out when posting the RFC to solve the problem (I don't have a good thought yet though). > > Also what is the delineation between a narrow vs. wide bus in the xml? > It seems you'd want to have a test that ensures that a wide scsi bus > skips unit=7 when adding 10 devices, right? > I wanted to add test for the wide scsi bus too, but the test qemu only supports narrow scsi bus, I didn't dig it more, as I thought it may not be deserved to do. The 10 devices in this test is to prove a new controller can be added, and the address can be allocated as expected.