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@xxxxxxxxxx> > >> --- > >> 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list