[libvirt] [PATCH] qemu: make attaching disk partition to VM illegal
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@redhat.com> --- src/qemu/qemu_domain.c | 10 +++++++ .../disk-attaching-partition-invalid.xml | 27 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 38 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-attaching-partition-invalid.xml diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e8e895d9aa..d03f3bed5f 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,14 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk, return -1; } + int result = virDiskNameParse(disk->dst, &idx, &partition); + if (result != 0 || partition != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("can't attach disk partition '%s', please attach whole disk instead"), + disk->dst); + return -1; + } + for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { if (qemuDomainValidateStorageSource(n, qemuCaps) < 0) return -1; diff --git a/tests/qemuxml2argvdata/disk-attaching-partition-invalid.xml b/tests/qemuxml2argvdata/disk-attaching-partition-invalid.xml new file mode 100644 index 0000000000..591819fbb6 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-attaching-partition-invalid.xml @@ -0,0 +1,27 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/export/vmimages/1.raw'/> + <target dev='vdb1' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 5bbac1c8b8..b54b4bbf35 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1097,6 +1097,7 @@ mymain(void) DO_TEST("disk-no-boot", NONE); DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid", QEMU_CAPS_VIRTIO_SCSI); + DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-attaching-partition-invalid"); DO_TEST_FAILURE("disk-usb-nosupport", NONE); DO_TEST("disk-usb-device", QEMU_CAPS_DEVICE_USB_STORAGE); -- 2.21.0
On 9/27/19 12:11 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@redhat.com> ---
I have a small nit below, but patch seems fine. Unfortunately it breaks 'make check' in my machine, in virschematest: 297 281) Checking chardev-reconnect-generated-path.xml against domain.rng ... OK 298 282) Checking disk-attaching-partition-invalid.xml against domain.rng ... FAILED 299 283) Checking iommu-smmuv3.xml against domain.rng ... OK
src/qemu/qemu_domain.c | 10 +++++++ .../disk-attaching-partition-invalid.xml | 27 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 38 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-attaching-partition-invalid.xml
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e8e895d9aa..d03f3bed5f 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,14 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk, return -1; }
+ int result = virDiskNameParse(disk->dst, &idx, &partition); + if (result != 0 || partition != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("can't attach disk partition '%s', please attach whole disk instead"), + disk->dst);
Break line to keep the line <= 80 chars plz.
+ return -1; + } + for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { if (qemuDomainValidateStorageSource(n, qemuCaps) < 0) return -1; diff --git a/tests/qemuxml2argvdata/disk-attaching-partition-invalid.xml b/tests/qemuxml2argvdata/disk-attaching-partition-invalid.xml new file mode 100644 index 0000000000..591819fbb6 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-attaching-partition-invalid.xml @@ -0,0 +1,27 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/export/vmimages/1.raw'/> + <target dev='vdb1' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 5bbac1c8b8..b54b4bbf35 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1097,6 +1097,7 @@ mymain(void) DO_TEST("disk-no-boot", NONE); DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid", QEMU_CAPS_VIRTIO_SCSI); + DO_TEST_CAPS_LATEST_PARSE_ERROR("disk-attaching-partition-invalid"); DO_TEST_FAILURE("disk-usb-nosupport", NONE); DO_TEST("disk-usb-device", QEMU_CAPS_DEVICE_USB_STORAGE);
On Fri, Sep 27, 2019 at 12:41:29 -0300, Daniel Henrique Barboza wrote:
On 9/27/19 12:11 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@redhat.com> ---
I have a small nit below, but patch seems fine.
Unfortunately it breaks 'make check' in my machine, in virschematest:
297 281) Checking chardev-reconnect-generated-path.xml against domain.rng ... OK 298 282) Checking disk-attaching-partition-invalid.xml against domain.rng ... FAILED
Ah this is a sneaky one. The virschematest was written so that it iterates all XML files in the tests to validate against the schema so that it doesn't require any action. There was just 1 bit of metadata it needs and that is if the given XML file is expected to fail schema validation. We chose to identify this by having the 'invalid.xml' suffix so that we don't have to keep an external database. This means that the new test data is annotated as if it should fail schema validation but it passes, thus the test fails. Thus you must rename the test.
299 283) Checking iommu-smmuv3.xml against domain.rng ... OK
src/qemu/qemu_domain.c | 10 +++++++ .../disk-attaching-partition-invalid.xml | 27 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 38 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-attaching-partition-invalid.xml
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e8e895d9aa..d03f3bed5f 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,14 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef *disk, return -1; } + int result = virDiskNameParse(disk->dst, &idx, &partition); + if (result != 0 || partition != 0) {
So virDiskNameParse returns -1 if it fails to parse the whole target string. This means for example also if it has the wrong prefix where the error message would be wrong. Please add a separate error message for that case. Also you don't really need to store the return value so you can check it directly. The error message can read something like VIR_ERR_CONFIG_UNSUPPORTED, "invalid disk target '%s'". Additionally once you add the check suggested above, you can in a separate patch remove the call to virDiskNameToIndex from qemuCheckDiskConfig which will become redundant. (with a nice bonus of improving the error message)
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("can't attach disk partition '%s', please attach whole disk instead"), + disk->dst);
Break line to keep the line <= 80 chars plz.
Note that 80 columns is no longer a strict requirement especially if it would make code harder to read. In this case it can be broken.
+ return -1; + } + for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { if (qemuDomainValidateStorageSource(n, qemuCaps) < 0) return -1;
On Mon, Sep 30, 2019 at 08:38:20AM +0200, Peter Krempa wrote:
On Fri, Sep 27, 2019 at 12:41:29 -0300, Daniel Henrique Barboza wrote:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("can't attach disk partition '%s', please attach whole disk instead"), + disk->dst);
Break line to keep the line <= 80 chars plz.
Note that 80 columns is no longer a strict requirement especially if it would make code harder to read. In this case it can be broken.
I had the line broken originally but fixed it later as per https://libvirt.org/hacking.html#errors which looked authoritative (and also seemed to make sense). I'll gladly fix the patch however I'm not clear at the moment as to which guidelines actually apply. Thanks, pvl
participants (3)
-
Daniel Henrique Barboza -
Pavel Mores -
Peter Krempa