1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] qemu: make attaching disk partition to VM illegal

Pavel Mores posted 1 patch 4 years, 7 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190927151145.17083-1-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
[libvirt] [PATCH] qemu: make attaching disk partition to VM illegal
Posted by Pavel Mores 4 years, 7 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: make attaching disk partition to VM illegal
Posted by Daniel Henrique Barboza 4 years, 7 months ago 

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);

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: make attaching disk partition to VM illegal
Posted by Peter Krempa 4 years, 6 months ago 
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;

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: make attaching disk partition to VM illegal
Posted by Pavel Mores 4 years, 6 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.