[PATCH 0/3] qemuDomainAttachDeviceDiskLiveInternal: Fix error handling and properly hotplug empty cdroms
10:22 a.m.
Patch 3 is best viewed with '-w'
Peter Krempa (3):
qemuDomainAttachDeviceDiskLiveInternal: Fix jumps on error
qemuDomainAttachDeviceDiskLiveInternal: Add missing jump to 'cleanup'
on error
qemu: hotplug: Don't try to setup disk image when hotplugging empty
cdrom drive
src/qemu/qemu_hotplug.c | 100 +++++++++++++++++++++-------------------
1 file changed, 53 insertions(+), 47 deletions(-)
--
2.42.0
10:22 a.m.
New subject: [PATCH 1/3] qemuDomainAttachDeviceDiskLiveInternal: Fix jumps on error
When I've originally refactored the function in commit 0d981bcefcb5defa2
the logic was still correct, but then later in commit 52f865543920b0
I've moved most of the image setup logic into the function neglecting to
add the 'goto cleanup;' needed to skip over the setup of the disk
images.
Fixes: 52f865543920b0cc5ba93f4407c1b2efdffb8ddc
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index fec7c4be4e..2d55d1d21e 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -931,7 +931,7 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver,
if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("disk device='lun' is not supported for usb
bus"));
- break;
+ goto cleanup;
}
if (virDomainUSBAddressEnsure(priv->usbaddrs, &disk->info) < 0)
@@ -991,6 +991,7 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver,
virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
_("disk bus '%1$s' cannot be hotplugged."),
virDomainDiskBusTypeToString(disk->bus));
+ goto cleanup;
}
if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) < 0)
--
2.42.0
10:22 a.m.
New subject: [PATCH 2/3] qemuDomainAttachDeviceDiskLiveInternal: Add missing jump to 'cleanup' on error
Commit allowing hotplug of CDROMs moved the logic forbidding the hotplug
to the appropriate blocks based on the disk frontend but forgot to
actually bail out on such error.
Fixes: 3078799fef82d45ac10624e3bacded7a285d8a4f
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 2d55d1d21e..80a39c159f 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -944,6 +944,7 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver,
if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("cdrom device with virtio bus isn't
supported"));
+ goto cleanup;
}
if (qemuDomainEnsureVirtioAddress(&releaseVirtio, vm, dev) < 0)
goto cleanup;
--
2.42.0
10:22 a.m.
New subject: [PATCH 3/3] qemu: hotplug: Don't try to setup disk image when hotplugging empty cdrom drive
Originally the disk hotplug code didn't know how to attach a CD-ROM
drive, thus didn't have the necessary logic to handle empty cdroms.
Other disks can't be empty which is enforced by the parser validation
logic.
When support for hotplugging cdroms was added the code was not adjusted
to deal with empty drives thus attempted to setup the blockdev backend
for it.
Fixes: 3078799fef82d45ac10624e3bacded7a285d8a4f
Resolves: https://issues.redhat.com/browse/RHEL-16870
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_hotplug.c | 96 +++++++++++++++++++++--------------------
1 file changed, 50 insertions(+), 46 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 80a39c159f..f197a9d5ff 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -662,53 +662,55 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
g_autoptr(qemuSnapshotDiskContext) transientDiskSnapshotCtxt = NULL;
bool origReadonly = disk->src->readonly;
- if (disk->transient)
- disk->src->readonly = true;
+ if (!virStorageSourceIsEmpty(disk->src)) {
+ if (disk->transient)
+ disk->src->readonly = true;
- if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_VHOST_USER) {
- if (!(data = qemuBuildStorageSourceChainAttachPrepareChardev(disk)))
- return -1;
- } else {
- if (!(data = qemuBuildStorageSourceChainAttachPrepareBlockdev(disk->src)))
- return -1;
-
- if (disk->copy_on_read == VIR_TRISTATE_SWITCH_ON) {
- if (!(data->copyOnReadProps = qemuBlockStorageGetCopyOnReadProps(disk)))
+ if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_VHOST_USER)
{
+ if (!(data = qemuBuildStorageSourceChainAttachPrepareChardev(disk)))
return -1;
+ } else {
+ if (!(data =
qemuBuildStorageSourceChainAttachPrepareBlockdev(disk->src)))
+ return -1;
+
+ if (disk->copy_on_read == VIR_TRISTATE_SWITCH_ON) {
+ if (!(data->copyOnReadProps =
qemuBlockStorageGetCopyOnReadProps(disk)))
+ return -1;
- data->copyOnReadNodename =
g_strdup(QEMU_DOMAIN_DISK_PRIVATE(disk)->nodeCopyOnRead);
+ data->copyOnReadNodename =
g_strdup(QEMU_DOMAIN_DISK_PRIVATE(disk)->nodeCopyOnRead);
+ }
}
- }
- disk->src->readonly = origReadonly;
+ disk->src->readonly = origReadonly;
- if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0)
- return -1;
+ if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0)
+ return -1;
- rc = qemuBlockStorageSourceChainAttach(priv->mon, data);
+ rc = qemuBlockStorageSourceChainAttach(priv->mon, data);
- qemuDomainObjExitMonitor(vm);
+ qemuDomainObjExitMonitor(vm);
- if (rc < 0)
- goto rollback;
+ if (rc < 0)
+ goto rollback;
- if (disk->transient) {
- g_autoptr(qemuBlockStorageSourceAttachData) backend = NULL;
- g_autoptr(GHashTable) blockNamedNodeData = NULL;
+ if (disk->transient) {
+ g_autoptr(qemuBlockStorageSourceAttachData) backend = NULL;
+ g_autoptr(GHashTable) blockNamedNodeData = NULL;
- if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob)))
- goto rollback;
+ if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob)))
+ goto rollback;
- if (!(transientDiskSnapshotCtxt = qemuDomainAttachDiskGenericTransient(vm, disk,
blockNamedNodeData, asyncJob)))
- goto rollback;
+ if (!(transientDiskSnapshotCtxt = qemuDomainAttachDiskGenericTransient(vm,
disk, blockNamedNodeData, asyncJob)))
+ goto rollback;
- if (qemuSnapshotDiskCreate(transientDiskSnapshotCtxt) < 0)
- goto rollback;
+ if (qemuSnapshotDiskCreate(transientDiskSnapshotCtxt) < 0)
+ goto rollback;
- QEMU_DOMAIN_DISK_PRIVATE(disk)->transientOverlayCreated = true;
- backend = qemuBlockStorageSourceDetachPrepare(disk->src);
- ignore_value(VIR_INSERT_ELEMENT(data->srcdata, 0, data->nsrcdata,
backend));
+ QEMU_DOMAIN_DISK_PRIVATE(disk)->transientOverlayCreated = true;
+ backend = qemuBlockStorageSourceDetachPrepare(disk->src);
+ ignore_value(VIR_INSERT_ELEMENT(data->srcdata, 0, data->nsrcdata,
backend));
+ }
}
if (!(devprops = qemuBuildDiskDeviceProps(vm->def, disk, priv->qemuCaps)))
@@ -995,28 +997,30 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver,
goto cleanup;
}
- if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) < 0)
+ if (qemuAssignDeviceDiskAlias(vm->def, disk) < 0)
goto cleanup;
- releaseSeclabel = true;
+ if (!virStorageSourceIsEmpty(disk->src)) {
+ if (qemuDomainStorageSourceChainAccessAllow(driver, vm, disk->src) < 0)
+ goto cleanup;
- if (qemuAssignDeviceDiskAlias(vm->def, disk) < 0)
- goto cleanup;
+ releaseSeclabel = true;
- if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0)
- goto cleanup;
+ if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0)
+ goto cleanup;
- if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL) < 0)
- goto cleanup;
+ if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL) < 0)
+ goto cleanup;
- if (qemuProcessPrepareHostStorageDisk(vm, disk) < 0)
- goto cleanup;
+ if (qemuProcessPrepareHostStorageDisk(vm, disk) < 0)
+ goto cleanup;
- if (qemuHotplugAttachManagedPR(vm, disk->src, VIR_ASYNC_JOB_NONE) < 0)
- goto cleanup;
+ if (qemuHotplugAttachManagedPR(vm, disk->src, VIR_ASYNC_JOB_NONE) < 0)
+ goto cleanup;
- if (qemuNbdkitStartStorageSource(driver, vm, disk->src) < 0)
- goto cleanup;
+ if (qemuNbdkitStartStorageSource(driver, vm, disk->src) < 0)
+ goto cleanup;
+ }
ret = qemuDomainAttachDiskGeneric(vm, disk, VIR_ASYNC_JOB_NONE);
--
2.42.0
6:37 p.m.
New subject: [PATCH 0/3] qemuDomainAttachDeviceDiskLiveInternal: Fix error
handling and properly hotplug empty cdroms
On a Tuesday in 2023, Peter Krempa wrote:
Patch 3 is best viewed with '-w'
Peter Krempa (3):
qemuDomainAttachDeviceDiskLiveInternal: Fix jumps on error
qemuDomainAttachDeviceDiskLiveInternal: Add missing jump to 'cleanup'
on error
qemu: hotplug: Don't try to setup disk image when hotplugging empty
cdrom drive
src/qemu/qemu_hotplug.c | 100 +++++++++++++++++++++-------------------
1 file changed, 53 insertions(+), 47 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
343
days inactive
345
days old
4 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa