[libvirt] [PATCH 0/2] Fix 'New libvirt-tck failure with libvirt 5.2.0'
Hi, Jim Fehlig reported a failure in a libvirt-tck test after the patch: commit f1d6585300001c7b23b8796a0faa4411c3531996 Author: Daniel Henrique Barboza <danielhb413@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
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@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
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@suse.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@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
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@suse.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@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
participants (2)
-
Daniel Henrique Barboza -
Michal Privoznik