4:49 p.m.
Hi,
Jim Fehlig reported a failure in a libvirt-tck test after the
patch:
commit f1d6585300001c7b23b8796a0faa4411c3531996
Author: Daniel Henrique Barboza <danielhb413(a)gmail.com>
Date: Fri Mar 15 18:06:45 2019 -0300
domain_conf: check device address before
This patch is blocking the use of 'virsh attach-device' for changing
and ejecting CDROM/Floppy drives. More info and some discussion
can be found at [1].
This small patch set makes changes in the device address
verification to allow this usage back. First patch is just an
extra helper to make the code a bit easier to read. Patch 2
amends commit f1d658530.
[1] https://www.redhat.com/archives/libvir-list/2019-April/msg00733.html
Daniel Henrique Barboza (2):
domain_conf: adding virDomainDiskIsCdromOrFloppy helper
domain_conf: allow CDROM/Floppy media change with attach-device
src/conf/domain_conf.c | 9 +++++++++
src/conf/domain_conf.h | 2 ++
src/libvirt_private.syms | 1 +
src/qemu/qemu_command.c | 3 +--
src/qemu/qemu_driver.c | 3 +--
src/qemu/qemu_hotplug.c | 6 ++----
6 files changed, 16 insertions(+), 8 deletions(-)
--
2.20.1
4:49 p.m.
New subject: [libvirt] [PATCH 1/2] domain_conf: adding virDomainDiskIsCdromOrFloppy helper
Checking if a device is a CDROM or floppy disk is a common
verification that is made a few times in the code, and next
patch is going to add one more.
Let's put it into a helper to enhance readability and spare some
lines of code.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/conf/domain_conf.c | 8 ++++++++
src/conf/domain_conf.h | 2 ++
src/libvirt_private.syms | 1 +
src/qemu/qemu_command.c | 3 +--
src/qemu/qemu_driver.c | 3 +--
src/qemu/qemu_hotplug.c | 6 ++----
6 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bbde3788a6..b6be1e730d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16708,6 +16708,14 @@ void virDomainDiskInsertPreAlloced(virDomainDefPtr def,
}
+bool
+virDomainDiskIsCdromOrFloppy(virDomainDiskDefPtr disk)
+{
+ return disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ||
+ disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY;
+}
+
+
virDomainDiskDefPtr
virDomainDiskRemove(virDomainDefPtr def, size_t i)
{
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 12eb71c197..2a1c6fd18b 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3579,4 +3579,6 @@ virDomainGraphicsGetRenderNode(const virDomainGraphicsDef
*graphics);
bool
virDomainGraphicsNeedsAutoRenderNode(const virDomainGraphicsDef *graphics);
+bool
+virDomainDiskIsCdromOrFloppy(virDomainDiskDefPtr disk);
#endif /* LIBVIRT_DOMAIN_CONF_H */
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6e64e77839..817d9bc28d 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -342,6 +342,7 @@ virDomainDiskInsert;
virDomainDiskInsertPreAlloced;
virDomainDiskIoTypeFromString;
virDomainDiskIoTypeToString;
+virDomainDiskIsCdromOrFloppy;
virDomainDiskMirrorStateTypeFromString;
virDomainDiskMirrorStateTypeToString;
virDomainDiskModelTypeFromString;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a3e845a848..2c663c9009 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1666,8 +1666,7 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
/* nothing to format if the drive is empty */
if (!(source || srcprops) ||
- ((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY ||
- disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) &&
+ (virDomainDiskIsCdromOrFloppy(disk) &&
disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
ret = 0;
goto cleanup;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fe2c586274..599c0f175f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8021,8 +8021,7 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm,
if (!virStorageSourceIsSameLocation(disk->src, orig_disk->src)) {
/* Disk source can be changed only for removable devices */
- if (disk->device != VIR_DOMAIN_DISK_DEVICE_CDROM &&
- disk->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
+ if (!virDomainDiskIsCdromOrFloppy(disk)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("disk source can be changed only in removable "
"drives"));
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index ef14b1977c..0ba1cc48c9 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1230,8 +1230,7 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriverPtr driver,
virDomainDiskDefPtr disk = dev->data.disk;
int ret = -1;
- if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ||
- disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
+ if (virDomainDiskIsCdromOrFloppy(disk)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("cdrom/floppy device hotplug isn't supported"));
return -1;
@@ -1314,8 +1313,7 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver,
/* this API overloads media change semantics on disk hotplug
* for devices supporting media changes */
- if ((disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ||
- disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) &&
+ if (virDomainDiskIsCdromOrFloppy(disk) &&
(orig_disk = virDomainDiskFindByBusAndDst(vm->def, disk->bus,
disk->dst))) {
if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk,
disk->src, false) < 0)
--
2.20.1
4:49 p.m.
New subject: [libvirt] [PATCH 2/2] domain_conf: allow CDROM/Floppy media change with attach-device
Commit f1d6585300 ("domain_conf: check device address before attach")
added an address verification for all devices, avoiding calling
the driver attach() for a case in which we know that the device
would have duplicated address.
That commit failed to contemplate the case of CDROM/Floppy devices
that can change and eject media using attach-device, an usage
that is also covered by this command. This patch adds an
extra condition for the added address verification code,
allowing CDROM/Floppy devices to bypass it.
Fixes: f1d6585300001c7b23b8796a0faa4411c3531996
Reported-by: Jim Fehlig <jfehlig(a)suse.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/conf/domain_conf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b6be1e730d..f65a335d86 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -28597,6 +28597,7 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
if (action == VIR_DOMAIN_DEVICE_ACTION_ATTACH &&
data.newInfo &&
data.newInfo->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+ !virDomainDiskIsCdromOrFloppy(dev->data.disk) &&
virDomainDefHasDeviceAddress(def, data.newInfo)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("Domain already contains a device with the same
address"));
--
2.20.1
8:40 a.m.
New subject: [libvirt] [PATCH 2/2] domain_conf: allow CDROM/Floppy media change with attach-device
On 4/10/19 11:49 PM, Daniel Henrique Barboza wrote:
Commit f1d6585300 ("domain_conf: check device address before
attach")
added an address verification for all devices, avoiding calling
the driver attach() for a case in which we know that the device
would have duplicated address.
That commit failed to contemplate the case of CDROM/Floppy devices
that can change and eject media using attach-device, an usage
that is also covered by this command. This patch adds an
extra condition for the added address verification code,
allowing CDROM/Floppy devices to bypass it.
Fixes: f1d6585300001c7b23b8796a0faa4411c3531996
Reported-by: Jim Fehlig <jfehlig(a)suse.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/conf/domain_conf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b6be1e730d..f65a335d86 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -28597,6 +28597,7 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
if (action == VIR_DOMAIN_DEVICE_ACTION_ATTACH &&
data.newInfo &&
data.newInfo->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+ !virDomainDiskIsCdromOrFloppy(dev->data.disk) &&
This is unsafe. You can't access dev->data.* unless you check dev->type
because @data is a union.
I'm testing a different approach. Will post patches shortly.
Michal
2043
days inactive
2044
days old
3 comments
2 participants
participants (2)
-
Daniel Henrique Barboza -
Michal Privoznik