On 06/22/2015 05:05 PM, John Ferlan wrote: s/is/must be/ ?

On 07/15/2015 11:06 AM, Ján Tomko wrote: Would you like to a see a patch that removes : if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) return -1; from virDomainHostdevAssignAddress since it's superfluous? I'm still "sitting on" this series since there was some overlap with your patch: http://www.redhat.com/archives/libvir-list/2015-June/msg00887.html Although your patch moved the decision making into runtime rather than during parse, which is mostly why I'm hesitant for at least this particular patch. Of course the decision here builds on what I did with patch 3 using the else clause for handling the situation where someone does provide a drive address to make sure there's no conflict. John

Modify virDomainDriveAddressIsUsedBy{Disk|Hostdev} and virDomainSCSIDriveAddressIsUsed to take 'bus' and 'target' parameters. Will be used by future patches for more complete address conflict checks Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/domain_conf.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ce5093d..3205c43 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5183,6 +5183,8 @@ static bool virDomainDriveAddressIsUsedByDisk(const virDomainDef *def, virDomainDiskBus type, unsigned int controller, + unsigned int bus, + unsigned int target, unsigned int unit) { virDomainDiskDefPtr disk; @@ -5197,8 +5199,8 @@ virDomainDriveAddressIsUsedByDisk(const virDomainDef *def, 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) + disk->info.addr.drive.bus == bus && + disk->info.addr.drive.target == target) return true; } @@ -5212,6 +5214,8 @@ static bool virDomainDriveAddressIsUsedByHostdev(const virDomainDef *def, virDomainHostdevSubsysType type, unsigned int controller, + unsigned int bus, + unsigned int target, unsigned int unit) { virDomainHostdevDefPtr hostdev; @@ -5225,8 +5229,8 @@ virDomainDriveAddressIsUsedByHostdev(const virDomainDef *def, 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) + hostdev->info->addr.drive.bus == bus && + hostdev->info->addr.drive.target == target) return true; } @@ -5236,6 +5240,8 @@ virDomainDriveAddressIsUsedByHostdev(const virDomainDef *def, static bool virDomainSCSIDriveAddressIsUsed(const virDomainDef *def, unsigned int controller, + unsigned int bus, + unsigned int target, unsigned int unit) { /* In current implementation, the maximum unit number of a controller @@ -5245,9 +5251,10 @@ virDomainSCSIDriveAddressIsUsed(const virDomainDef *def, return true; if (virDomainDriveAddressIsUsedByDisk(def, VIR_DOMAIN_DISK_BUS_SCSI, - controller, unit) || - virDomainDriveAddressIsUsedByHostdev(def, VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, - controller, unit)) + controller, bus, target, unit) || + virDomainDriveAddressIsUsedByHostdev(def, + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, + controller, bus, target, unit)) return true; return false; @@ -5262,7 +5269,8 @@ virDomainControllerSCSINextUnit(const virDomainDef *def, size_t i; for (i = 0; i < max_unit; i++) { - if (!virDomainSCSIDriveAddressIsUsed(def, controller, i)) + /* Default to assigning addresses using bus = target = 0 */ + if (!virDomainSCSIDriveAddressIsUsed(def, controller, 0, 0, i)) return i; } -- 2.1.0

Rather than passing the def->seclabels and def->nseclabels, refactor the API to pass the entire domain definition. This will be used in a future patch as well. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/domain_conf.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e02cd49..6259d4a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6390,8 +6390,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, xmlNodePtr node, xmlXPathContextPtr ctxt, virHashTablePtr bootHash, - virSecurityLabelDefPtr* vmSeclabels, - int nvmSeclabels, + const virDomainDef *vmdef, unsigned int flags) { virDomainDiskDefPtr def; @@ -6930,8 +6929,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, ctxt->node = sourceNode; if (virSecurityDeviceLabelDefParseXML(&def->src->seclabels, &def->src->nseclabels, - vmSeclabels, - nvmSeclabels, + vmdef->seclabels, + vmdef->nseclabels, ctxt, flags) < 0) goto error; @@ -12256,9 +12255,7 @@ virDomainDeviceDefParse(const char *xmlStr, switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: if (!(dev->data.disk = virDomainDiskDefParseXML(xmlopt, node, ctxt, - NULL, def->seclabels, - def->nseclabels, - flags))) + NULL, def, flags))) goto error; break; case VIR_DOMAIN_DEVICE_LEASE: @@ -12400,9 +12397,7 @@ virDomainDiskDefSourceParse(const char *xmlStr, flags |= VIR_DOMAIN_DEF_PARSE_DISK_SOURCE; if (!(disk = virDomainDiskDefParseXML(xmlopt, node, ctxt, - NULL, def->seclabels, - def->nseclabels, - flags))) + NULL, def, flags))) goto cleanup; ret = disk->src; @@ -15418,8 +15413,7 @@ virDomainDefParseXML(xmlDocPtr xml, nodes[i], ctxt, bootHash, - def->seclabels, - def->nseclabels, + def, flags); if (!disk) goto error; -- 2.1.0

On 07/16/2015 08:03 AM, Ján Tomko wrote: Are you advocating removing virDomainDiskDefAssignAddress() from virDomainDiskDefParseXML() and relying on qemuParseCommandLineDisk() and qemuParseCommandLine()? Following around all the existing "assumptions" especially as they relate to hotplug and even perhaps blockcopy could be an adventure. Not to say that I don't disagree with the concept separating parse from autofill; however, that seems outside the scope of this particular issue John

https://bugzilla.redhat.com/show_bug.cgi?id=1210587 (completed) When generating the default drive address for a SCSI <disk> device, check the generated address to ensure it doesn't conflict with a SCSI <hostdev> address. The <disk> address generation algorithm uses the <target> "dev" name in order to determine which controller and unit in order to place the device. Since a SCSI <hostdev> device doesn't require a target device name, its placement on the guest SCSI address "could" conflict. For instance, if a SCSI <hostdev> exists at controller=0 unit=0 and an attempt to hotplug 'sda' into the guest made, there would be a conflict if the <hostdev> is already using /dev/sda. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/domain_conf.c | 40 +++++++++++++++++++++++++++++----------- src/conf/domain_conf.h | 3 ++- src/qemu/qemu_command.c | 4 ++-- src/vmx/vmx.c | 22 ++++++++++++---------- src/vmx/vmx.h | 3 ++- 5 files changed, 47 insertions(+), 25 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6259d4a..0999e86 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5672,7 +5672,8 @@ virDomainDiskFindByBusAndDst(virDomainDefPtr def, int virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, - virDomainDiskDefPtr def) + virDomainDiskDefPtr def, + const virDomainDef *vmdef) { int idx = virDiskNameToIndex(def->dst); if (idx < 0) { @@ -5683,7 +5684,10 @@ virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, } switch (def->bus) { - case VIR_DOMAIN_DISK_BUS_SCSI: + case VIR_DOMAIN_DISK_BUS_SCSI: { + unsigned int controller; + unsigned int unit; + def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; if (xmlopt->config.hasWideSCSIBus) { @@ -5692,22 +5696,36 @@ virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, * Unit 7 is the SCSI controller itself. Therefore unit 7 * cannot be assigned to disks and is skipped. */ - def->info.addr.drive.controller = idx / 15; - def->info.addr.drive.bus = 0; - def->info.addr.drive.unit = idx % 15; + controller = idx / 15; + unit = idx % 15; /* Skip the SCSI controller at unit 7 */ - if (def->info.addr.drive.unit >= 7) - ++def->info.addr.drive.unit; + if (unit >= 7) + ++unit; } else { /* For a narrow SCSI bus we define the default mapping to be * 7 units per bus, 1 bus per controller, many controllers */ - def->info.addr.drive.controller = idx / 7; - def->info.addr.drive.bus = 0; - def->info.addr.drive.unit = idx % 7; + controller = idx / 7; + unit = idx % 7; + } + + if (virDomainDriveAddressIsUsedByHostdev(vmdef, + VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, + controller, 0, 0, unit)) { + virReportError(VIR_ERR_XML_ERROR, + _("address generated using disk target name '%s' " + "may conflict with SCSI host device address " + "controller='%u' bus='%u' target='%u' unit='%u"), + def->dst, controller, 0, 0, unit); + return -1; } + def->info.addr.drive.controller = controller; + def->info.addr.drive.bus = 0; + def->info.addr.drive.target = 0; + def->info.addr.drive.unit = unit; break; + } case VIR_DOMAIN_DISK_BUS_IDE: /* For IDE we define the default mapping to be 2 units @@ -7261,7 +7279,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, if (!(flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) { if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE - && virDomainDiskDefAssignAddress(xmlopt, def) < 0) + && virDomainDiskDefAssignAddress(xmlopt, def, vmdef) < 0) goto error; if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c96a6e4..5b1c838 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2717,7 +2717,8 @@ int virDomainDiskInsert(virDomainDefPtr def, void virDomainDiskInsertPreAlloced(virDomainDefPtr def, virDomainDiskDefPtr disk); int virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, - virDomainDiskDefPtr def); + virDomainDiskDefPtr def, + const virDomainDef *vmdef); virDomainDiskDefPtr virDomainDiskRemove(virDomainDefPtr def, size_t i); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5444638..11e8e16 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -11784,7 +11784,7 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, else def->dst[2] = 'a' + idx; - if (virDomainDiskDefAssignAddress(xmlopt, def) < 0) { + if (virDomainDiskDefAssignAddress(xmlopt, def, dom) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid device name '%s'"), def->dst); virDomainDiskDefFree(def); @@ -12927,7 +12927,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, } } - if (virDomainDiskDefAssignAddress(xmlopt, disk) < 0) { + if (virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot assign address for device name '%s'"), disk->dst); diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index aede2ad..d031398 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -960,7 +960,9 @@ virVMXFloppyDiskNameToUnit(const char *name, int *unit) static int -virVMXVerifyDiskAddress(virDomainXMLOptionPtr xmlopt, virDomainDiskDefPtr disk) +virVMXVerifyDiskAddress(virDomainXMLOptionPtr xmlopt, + virDomainDiskDefPtr disk, + virDomainDefPtr vmdef) { virDomainDiskDef def; virDomainDeviceDriveAddressPtr drive; @@ -979,7 +981,7 @@ virVMXVerifyDiskAddress(virDomainXMLOptionPtr xmlopt, virDomainDiskDefPtr disk) def.dst = disk->dst; def.bus = disk->bus; - if (virDomainDiskDefAssignAddress(xmlopt, &def) < 0) { + if (virDomainDiskDefAssignAddress(xmlopt, &def, vmdef) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not verify disk address")); return -1; @@ -1588,7 +1590,7 @@ virVMXParseConfig(virVMXContext *ctx, if (virVMXParseDisk(ctx, xmlopt, conf, VIR_DOMAIN_DISK_DEVICE_DISK, VIR_DOMAIN_DISK_BUS_SCSI, controller, unit, - &def->disks[def->ndisks]) < 0) { + &def->disks[def->ndisks], def) < 0) { goto cleanup; } @@ -1599,7 +1601,7 @@ virVMXParseConfig(virVMXContext *ctx, if (virVMXParseDisk(ctx, xmlopt, conf, VIR_DOMAIN_DISK_DEVICE_CDROM, VIR_DOMAIN_DISK_BUS_SCSI, controller, unit, - &def->disks[def->ndisks]) < 0) { + &def->disks[def->ndisks], def) < 0) { goto cleanup; } @@ -1613,7 +1615,7 @@ virVMXParseConfig(virVMXContext *ctx, for (unit = 0; unit < 2; ++unit) { if (virVMXParseDisk(ctx, xmlopt, conf, VIR_DOMAIN_DISK_DEVICE_DISK, VIR_DOMAIN_DISK_BUS_IDE, bus, unit, - &def->disks[def->ndisks]) < 0) { + &def->disks[def->ndisks], def) < 0) { goto cleanup; } @@ -1624,7 +1626,7 @@ virVMXParseConfig(virVMXContext *ctx, if (virVMXParseDisk(ctx, xmlopt, conf, VIR_DOMAIN_DISK_DEVICE_CDROM, VIR_DOMAIN_DISK_BUS_IDE, bus, unit, - &def->disks[def->ndisks]) < 0) { + &def->disks[def->ndisks], def) < 0) { goto cleanup; } @@ -1637,7 +1639,7 @@ virVMXParseConfig(virVMXContext *ctx, for (unit = 0; unit < 2; ++unit) { if (virVMXParseDisk(ctx, xmlopt, conf, VIR_DOMAIN_DISK_DEVICE_FLOPPY, VIR_DOMAIN_DISK_BUS_FDC, 0, unit, - &def->disks[def->ndisks]) < 0) { + &def->disks[def->ndisks], def) < 0) { goto cleanup; } @@ -1925,7 +1927,7 @@ virVMXParseSCSIController(virConfPtr conf, int controller, bool *present, int virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr conf, int device, int busType, int controllerOrBus, int unit, - virDomainDiskDefPtr *def) + virDomainDiskDefPtr *def, virDomainDefPtr vmdef) { /* * device = {VIR_DOMAIN_DISK_DEVICE_DISK, @@ -2280,7 +2282,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con goto cleanup; } - if (virDomainDiskDefAssignAddress(xmlopt, *def) < 0) { + if (virDomainDiskDefAssignAddress(xmlopt, *def, vmdef) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not assign address to disk '%s'"), virDomainDiskGetSource(*def)); @@ -3189,7 +3191,7 @@ virVMXFormatConfig(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virDomainDe /* def:disks */ for (i = 0; i < def->ndisks; ++i) { - if (virVMXVerifyDiskAddress(xmlopt, def->disks[i]) < 0 || + if (virVMXVerifyDiskAddress(xmlopt, def->disks[i], def) < 0 || virVMXHandleLegacySCSIDiskDriverName(def, def->disks[i]) < 0) { goto cleanup; } diff --git a/src/vmx/vmx.h b/src/vmx/vmx.h index 6a68c8b..e986124 100644 --- a/src/vmx/vmx.h +++ b/src/vmx/vmx.h @@ -89,7 +89,8 @@ int virVMXParseSCSIController(virConfPtr conf, int controller, bool *present, int virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr conf, int device, int busType, - int controllerOrBus, int unit, virDomainDiskDefPtr *def); + int controllerOrBus, int unit, virDomainDiskDefPtr *def, + virDomainDefPtr vmdef); int virVMXParseFileSystem(virConfPtr conf, int number, virDomainFSDefPtr *def); -- 2.1.0

On 07/16/2015 08:23 AM, Ján Tomko wrote: OK - I can separate out out just the non-functional into its own patch. Do you want me to send it out again or trust that I know what I'm doing? Right - it does conflict... The issue is that the 'idx' is generated as a result of the target device name ("dst"). The assumption being "sda" would be controller = unit = 0, 'sdb' would be ctlr = 0, unit = 1, 'sdc' would be ctlr = 0, unit = 2, etc. However, for hostdev's there is no such "dst" logic - it's a straight assignment. When the guest boots, whatever is assigned to "c = 0, u = 0" is assigned to 'sda' (regardless of whether it's a hostdev or disk). So if the hostdev which had it's address provided in the XML, but the disk which didn't *and* is using "<target dev='sda'.../>", then there's a possible conflict because "sda" resolves to "c = 0, b = 0". The error message I guess should thus state : "using disk target name '%s' conflicts with SCSI host device address ..." Would you still call that an XML error or perhaps CONFIG_UNSUPPORTED John