[libvirt] [PATCH 0/4] qemu: hotplug: Clean up and fix disk device unplug (blockdev-add saga)
Refactor checking of disk bus and type for disk unplug and fix checking whether a virtio disk has a running block job by deleting the almost-identical implementation specific to virtio disks. Peter Krempa (4): qemu: hotplug: Remove 'ret' variable in qemuDomainDetachDeviceDiskLive qemu: hotplug: Use typecasted enum in qemuDomainDetachDeviceDiskLive qemu: hotplug: Use switch statement for selecting disk bus function qemu: hotplug: Merge virtio and non-virtio disk unplug code src/qemu/qemu_hotplug.c | 87 ++++++++++++++++++----------------------- 1 file changed, 39 insertions(+), 48 deletions(-) -- 2.20.1
We don't have any cleanup section, we can return the value directly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 811fdf6c3a..0825b03a0d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5366,7 +5366,6 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, bool async) { virDomainDiskDefPtr disk; - int ret = -1; int idx; if ((idx = qemuFindDisk(vm->def, dev->data.disk->dst)) < 0) { @@ -5380,10 +5379,10 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, case VIR_DOMAIN_DISK_DEVICE_DISK: case VIR_DOMAIN_DISK_DEVICE_LUN: if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) - ret = qemuDomainDetachVirtioDiskDevice(driver, vm, disk, async); + return qemuDomainDetachVirtioDiskDevice(driver, vm, disk, async); else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI || disk->bus == VIR_DOMAIN_DISK_BUS_USB) - ret = qemuDomainDetachDiskDevice(driver, vm, disk, async); + return qemuDomainDetachDiskDevice(driver, vm, disk, async); else virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("This type of disk cannot be hot unplugged")); @@ -5395,7 +5394,7 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, break; } - return ret; + return -1; } -- 2.20.1
On Fri, Mar 15, 2019 at 03:44:55PM +0100, Peter Krempa wrote:
We don't have any cleanup section, we can return the value directly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Use the correct type in switch and populate the missing cases. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0825b03a0d..92af85c3d3 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5375,7 +5375,7 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, } disk = vm->def->disks[idx]; - switch (disk->device) { + switch ((virDomainDiskDevice) disk->device) { case VIR_DOMAIN_DISK_DEVICE_DISK: case VIR_DOMAIN_DISK_DEVICE_LUN: if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) @@ -5387,11 +5387,18 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("This type of disk cannot be hot unplugged")); break; - default: + + case VIR_DOMAIN_DISK_DEVICE_CDROM: + case VIR_DOMAIN_DISK_DEVICE_FLOPPY: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("disk device type '%s' cannot be detached"), virDomainDiskDeviceTypeToString(disk->device)); break; + + case VIR_DOMAIN_DISK_DEVICE_LAST: + default: + virReportEnumRangeError(virDomainDiskDevice, disk->device); + break; } return -1; -- 2.20.1
On Fri, Mar 15, 2019 at 03:44:56PM +0100, Peter Krempa wrote:
Use the correct type in switch and populate the missing cases.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 92af85c3d3..ffe0031362 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5378,14 +5378,30 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, switch ((virDomainDiskDevice) disk->device) { case VIR_DOMAIN_DISK_DEVICE_DISK: case VIR_DOMAIN_DISK_DEVICE_LUN: - if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) + + switch ((virDomainDiskBus) disk->bus) { + case VIR_DOMAIN_DISK_BUS_VIRTIO: return qemuDomainDetachVirtioDiskDevice(driver, vm, disk, async); - else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI || - disk->bus == VIR_DOMAIN_DISK_BUS_USB) + + case VIR_DOMAIN_DISK_BUS_USB: + case VIR_DOMAIN_DISK_BUS_SCSI: return qemuDomainDetachDiskDevice(driver, vm, disk, async); - else + + case VIR_DOMAIN_DISK_BUS_IDE: + case VIR_DOMAIN_DISK_BUS_FDC: + case VIR_DOMAIN_DISK_BUS_XEN: + case VIR_DOMAIN_DISK_BUS_UML: + case VIR_DOMAIN_DISK_BUS_SATA: + case VIR_DOMAIN_DISK_BUS_SD: virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("This type of disk cannot be hot unplugged")); + break; + + case VIR_DOMAIN_DISK_BUS_LAST: + default: + virReportEnumRangeError(virDomainDiskBus, disk->bus); + break; + } break; case VIR_DOMAIN_DISK_DEVICE_CDROM: -- 2.20.1
On Fri, Mar 15, 2019 at 03:44:57PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The functions do basically exactly the same thing modulo few checks. In case of virtio disks we check that the device is not multifunction as that can't be unplugged at once. In case of USB and SCSI disks we checked that no active block job is running. The check for running blockjobs should have also been done for virtio disks. By moving the multifunction check into the common function we fix this case and also simplify the code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 49 ++++++++--------------------------------- 1 file changed, 9 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ffe0031362..41d60277d1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5276,43 +5276,6 @@ qemuDomainSignalDeviceRemoval(virDomainObjPtr vm, } -static int -qemuDomainDetachVirtioDiskDevice(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr detach, - bool async) -{ - int ret = -1; - - if (qemuIsMultiFunctionDevice(vm->def, &detach->info)) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("cannot hot unplug multifunction PCI device: %s"), - detach->dst); - goto cleanup; - } - - if (!async) - qemuDomainMarkDeviceForRemoval(vm, &detach->info); - - if (qemuDomainDeleteDevice(vm, detach->info.alias) < 0) { - if (virDomainObjIsActive(vm)) - virDomainAuditDisk(vm, detach->src, NULL, "detach", false); - goto cleanup; - } - - if (async) { - ret = 0; - } else { - if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveDiskDevice(driver, vm, detach); - } - - cleanup: - if (!async) - qemuDomainResetDeviceRemoval(vm); - return ret; -} - static int qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -5322,7 +5285,15 @@ qemuDomainDetachDiskDevice(virQEMUDriverPtr driver, int ret = -1; if (qemuDomainDiskBlockJobIsActive(detach)) - goto cleanup; + return -1; + + if (detach->bus == VIR_DOMAIN_DISK_BUS_VIRTIO && + qemuIsMultiFunctionDevice(vm->def, &detach->info)) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("cannot hot unplug multifunction PCI device: %s"), + detach->dst); + return -1; + } if (!async) qemuDomainMarkDeviceForRemoval(vm, &detach->info); @@ -5381,8 +5352,6 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, switch ((virDomainDiskBus) disk->bus) { case VIR_DOMAIN_DISK_BUS_VIRTIO: - return qemuDomainDetachVirtioDiskDevice(driver, vm, disk, async); - case VIR_DOMAIN_DISK_BUS_USB: case VIR_DOMAIN_DISK_BUS_SCSI: return qemuDomainDetachDiskDevice(driver, vm, disk, async); -- 2.20.1
On Fri, Mar 15, 2019 at 03:44:58PM +0100, Peter Krempa wrote:
The functions do basically exactly the same thing modulo few checks. In case of virtio disks we check that the device is not multifunction as that can't be unplugged at once. In case of USB and SCSI disks we checked that no active block job is running.
The check for running blockjobs should have also been done for virtio disks. By moving the multifunction check into the common function we fix this case and also simplify the code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 49 ++++++++--------------------------------- 1 file changed, 9 insertions(+), 40 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa