Re: [PATCH 5/5] conf: Check for hostdev conflicts when assign default disk address

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]