On Thu, Jul 16, 2015 at 11:57:52AM -0400, John Ferlan wrote:
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?
Both. I did not give it an 'ACK with that split out' because I expected another series with the address generation done after all is parsed.
+ 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);
The disks are parsed before hostdevs, so this will only catch conflicts on hotplug.
'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
Oh, so the targets are not really guaranteed. I guess that could be considered an XML error, even if it is caused by our interpretation of the (mandatory) target attribute. Jan