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