[libvirt] [PATCH v2 1/2] qemu: make attaching disk partition to VM illegal

[Daniel P. Berrangé]

On Wed, Oct 02, 2019 at 10:45:28AM +0200, Michal Privoznik wrote:
> On 9/30/19 3:41 PM, Pavel Mores wrote:
> > The way in which the qemu driver generates aliases for disks involves
> > ignoring the partition number part of a target dev name.  This means that
> > all partitions of a block device and the device itself all end up with the
> > same alias.  If multiple such disks are specified in XML, the resulting
> > name clash makes qemu invocation fail.
> > 
> > Since attaching partitions to qemu VMs doesn't seem to make much sense
> > anyway, disallow partitions in target specifications altogether.
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1346265
> > 
> > Signed-off-by: Pavel Mores <pmores at redhat.com>
> > ---
> >   src/qemu/qemu_domain.c                        | 15 +++++++++++
> >   .../disk-attaching-partition-nosupport.xml    | 27 +++++++++++++++++++
> >   tests/qemuxml2argvtest.c                      |  1 +
> >   3 files changed, 43 insertions(+)
> >   create mode 100644 tests/qemuxml2argvdata/disk-attaching-partition-nosupport.xml
> > 
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index e8e895d9aa..545c985f40 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -5880,6 +5880,8 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk,
> >   {
> >       const char *driverName = virDomainDiskGetDriver(disk);
> >       virStorageSourcePtr n;
> > +    int idx;
> > +    int partition;
> >       if (disk->src->shared && !disk->src->readonly &&
> >           !qemuBlockStorageSourceSupportsConcurrentAccess(disk->src)) {
> > @@ -5948,6 +5950,19 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk,
> >           return -1;
> >       }
> > +    if (virDiskNameParse(disk->dst, &idx, &partition) < 0) {
> > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                       _("invalid disk target '%s'"), disk->dst);
> > +        return -1;
> > +    }
> > +
> > +    if (partition != 0) {
> > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                       _("can't attach disk partition '%s', please attach whole disk instead"),
> > +                       disk->dst);
> 
> Hopefully it's not too late, but this error message and the commit title
> neither don't look okay. You can attach /dev/sda1 to a domain, but we don't
> want you to put "sda1" into the disk target, we want you to name it just
> "sda". So perhaps "Refuse partitions in disk targets" as commit tile and
> "invalid disk target '%s', partitions can't appear in disk targets" for the
> error message looks better?
> 
> The patch looks goot otherwise. Currently we are in a freeze, but since this
> is a bug fix it can go in. Objections anybody? I volunteer to merge it.

I think it is fine for freeze, so go ahead with your proposed fix.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|