[PATCH 0/2] qemu: Fix detach of a CD-ROM device with no media
Detaching a CD-ROM device that has no media inserted currently logs an internal error: internal error: unable to execute QEMU command 'blockdev-del': Failed to find node with node-name='libvirt-N-storage' or, when no stale node name is present in the source: internal error: argument key 'node-name' must not have null value The root cause is that an empty disk source (path == NULL) satisfies virStorageSourceIsBacking() while no corresponding blockdev node ever exists in QEMU, so the detach path prepares a blockdev-del for a node that was never created. Patch 1 fixes the detach path itself: qemuDomainRemoveDiskDevice() now skips the blockdev detach preparation for an empty source, mirroring what the old-media detach in qemuDomainChangeMediaBlockdev() already does. Patch 2 fixes the way the stale node name gets there in the first place: qemuDomainChangeEjectableMedia() ran the full image setup (qemuDomainPrepareDiskSource(), backing chain detection, storage access, managed PR) even when ejecting to no media, assigning node names to an empty source that are never realized in QEMU. The setup is now skipped for an empty source, mirroring qemuDomainAttachDeviceDiskLiveInternal(), and rollback only revokes storage access when it was actually granted. To reproduce the 1st message: 1. Start a domain with a CD-ROM device with media attached 2. virsh change-media $dom $target --eject --live 3. virsh detach-disk $dom $target --live To reproduce the 2nd message: 1. Start a domain with a CD-ROM device without media attached 2. virsh detach-disk $dom $target --live No regression tests are added. Neither failure mode is reachable from qemuhotplugtest: the "null value" variant fails while constructing the blockdev-del arguments, before any QMP command is sent, leaving nothing for the test monitor to observe; the stale-node-name variant does emit a blockdev-del and would be detectable, but requires a preceding media eject to set up, which qemuhotplugtest cannot drive. Mitsuru Kariya (2): qemu: Skip empty disk source when detaching a disk device qemu: Don't prepare an empty disk source on media change src/qemu/qemu_hotplug.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) base-commit: 674fcf87bdd479302032fbeb331fea2aa51606ac -- 2.43.0
qemuDomainChangeEjectableMedia() ran qemuDomainPrepareDiskSource() and the surrounding image setup (backing chain detection, storage access, managed PR) unconditionally. When ejecting to no media the new source is empty, and qemuDomainPrepareDiskSource() still assigned it node names that are never realized in QEMU, leaving a stale name in the disk source that a later detach would then try to blockdev-del. Wrap the image setup in a virStorageSourceIsEmpty() check, mirroring qemuDomainAttachDeviceDiskLiveInternal(), and only revoke storage access on rollback when it was actually granted, using a releaseSeclabel flag as the attach path already does. Signed-off-by: Mitsuru Kariya <Mitsuru.Kariya@oss.nttdata.com> --- src/qemu/qemu_hotplug.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c2cd5496e0..b96ff24cbf 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -612,6 +612,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriver *driver, qemuDomainObjPrivate *priv = vm->privateData; virStorageSource *oldsrc = disk->src; qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + bool releaseSeclabel = false; int rc; if (diskPriv->blockjob && qemuBlockJobIsRunning(diskPriv->blockjob)) { @@ -625,17 +626,21 @@ qemuDomainChangeEjectableMedia(virQEMUDriver *driver, if (virDomainDiskTranslateSourcePool(disk) < 0) goto rollback; - if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL) < 0) - goto rollback; + if (!virStorageSourceIsEmpty(newsrc)) { + if (qemuDomainDetermineDiskChain(driver, vm, disk, NULL) < 0) + goto rollback; - if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) - goto rollback; + if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) + goto rollback; - if (qemuDomainStorageSourceChainAccessAllow(driver, vm, newsrc) < 0) - goto rollback; + if (qemuDomainStorageSourceChainAccessAllow(driver, vm, newsrc) < 0) + goto rollback; - if (qemuHotplugAttachManagedPR(vm, newsrc, VIR_ASYNC_JOB_NONE) < 0) - goto rollback; + releaseSeclabel = true; + + if (qemuHotplugAttachManagedPR(vm, newsrc, VIR_ASYNC_JOB_NONE) < 0) + goto rollback; + } rc = qemuDomainChangeMediaBlockdev(vm, disk, oldsrc, newsrc, force); @@ -653,7 +658,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriver *driver, return 0; rollback: - ignore_value(qemuDomainStorageSourceChainAccessRevoke(driver, vm, newsrc)); + if (releaseSeclabel) + ignore_value(qemuDomainStorageSourceChainAccessRevoke(driver, vm, newsrc)); qemuHotplugRemoveManagedPR(vm, newsrc, VIR_ASYNC_JOB_NONE); -- 2.43.0
qemuDomainRemoveDiskDevice() called qemuBlockStorageSourceChainDetachPrepareBlockdev() unconditionally. A CD-ROM with no media has an empty source (path == NULL), for which virStorageSourceIsEmpty() returns true while virStorageSourceIsBacking() still returns true, so the chain walk prepared a blockdev-del for a node that was never created in QEMU. Detaching such a device produced an internal error, either "Failed to find node with node-name='libvirt-N-storage'" (when a prior eject had left a stale node name in the source) or "argument key 'node-name' must not have null value". Guard the detach preparation with virStorageSourceIsEmpty(), as the old-media detach in qemuDomainChangeMediaBlockdev() already does. The following qemuBlockStorageSourceChainDetach() and access revoke are already guarded by a non-NULL diskBackend, so an empty source is left untouched. Signed-off-by: Mitsuru Kariya <Mitsuru.Kariya@oss.nttdata.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 5be567b510..c2cd5496e0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4861,7 +4861,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriver *driver, diskPriv->blockjob->disk = NULL; g_clear_pointer(&diskPriv->blockjob, virObjectUnref); } else { - if (!(diskBackend = qemuBlockStorageSourceChainDetachPrepareBlockdev(disk->src))) + if (!virStorageSourceIsEmpty(disk->src) && + !(diskBackend = qemuBlockStorageSourceChainDetachPrepareBlockdev(disk->src))) goto cleanup; } -- 2.43.0
participants (1)
-
Mitsuru Kariya