[libvirt] [PATCH v2 0/2] make attaching disk partition to VM illegal
This version addresses comments and objections to the original submission. Pavel Mores (2): qemu: make attaching disk partition to VM illegal remove a now redundant call to virDiskNameToIndex() src/qemu/qemu_command.c | 6 ----- src/qemu/qemu_domain.c | 15 +++++++++++ .../disk-attaching-partition-nosupport.xml | 27 +++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 4 files changed, 43 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-attaching-partition-nosupport.xml -- 2.21.0
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 | 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); + 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-nosupport.xml b/tests/qemuxml2argvdata/disk-attaching-partition-nosupport.xml new file mode 100644 index 0000000000..591819fbb6 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-attaching-partition-nosupport.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..058eee998f 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-nosupport"); DO_TEST_FAILURE("disk-usb-nosupport", NONE); DO_TEST("disk-usb-device", QEMU_CAPS_DEVICE_USB_STORAGE); -- 2.21.0
On Mon, Sep 30, 2019 at 03:41:00PM +0200, 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> --- 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
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
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@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. Michal
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@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 :|
On 10/2/19 1:11 PM, Daniel P. Berrangé wrote:
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@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
I think it is fine for freeze, so go ahead with your proposed fix.
Thanks, I've made the change and pushed these. Congratulations Pavel on your first libvirt contribution! Michal
On Wed, Oct 02, 2019 at 02:04:28PM +0200, Michal Privoznik wrote:
On 10/2/19 1:11 PM, Daniel P. Berrangé wrote:
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@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
I think it is fine for freeze, so go ahead with your proposed fix.
Thanks, I've made the change and pushed these.
Congratulations Pavel on your first libvirt contribution!
Cheers, and apologies for the additional work you had with it. :-) pvl
Parseability of disk name is now checked in qemuDomainDeviceDefValidateDisk(). Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_command.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 77470a6037..890d89b92c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1290,12 +1290,6 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk, if (qemuCheckDiskConfigBlkdeviotune(disk, qemuCaps) < 0) return -1; - if (virDiskNameToIndex(disk->dst) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unsupported disk type '%s'"), disk->dst); - return -1; - } - if (disk->wwn) { if ((disk->bus != VIR_DOMAIN_DISK_BUS_IDE) && (disk->bus != VIR_DOMAIN_DISK_BUS_SCSI)) { -- 2.21.0
On Mon, Sep 30, 2019 at 03:41:01PM +0200, Pavel Mores wrote:
Parseability of disk name is now checked in qemuDomainDeviceDefValidateDisk().
Signed-off-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_command.c | 6 ------ 1 file changed, 6 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
participants (3)
-
Daniel P. Berrangé -
Michal Privoznik -
Pavel Mores