On 07/16/2015 08:23 AM, Ján Tomko wrote:
On Mon, Jun 22, 2015 at 05:05:07PM -0400, John Ferlan wrote:
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@redhat.com> --- 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; + } +
This mixes non-functional and functional changes.
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?
+ 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);
'may conflict' sounds ambiguous. Either it does conflict and we should reject it, or it does not and there is no error.
And this seems like a problem of libvirt's address generation algorithm, not an XML error.
Jan
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