[PATCH 2/2] qemu: Don't prepare an empty disk source on media change

Mitsuru Kariya via Devel posted 2 patches 1 month, 2 weeks ago
[PATCH 2/2] qemu: Don't prepare an empty disk source on media change
Posted by Mitsuru Kariya via Devel 1 month, 2 weeks ago
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
Re: [PATCH 2/2] qemu: Don't prepare an empty disk source on media change
Posted by Peter Krempa via Devel 3 weeks, 4 days ago
On Mon, Jul 20, 2026 at 16:06:24 +0900, Mitsuru Kariya via Devel wrote:
> 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

This by itself is not a problem. In addition some of the functions don't
do anything at all for empty source.

But I agree that setting up all of this is pointless.


> that a later detach would then try to blockdev-del.

This is not really fixed by this, you'd get a different error because
in qemuBlockStorageSourceDetachPrepare

data->storageNodeName would be NULL, but data->storageAttached is still
set to true.

qemuBlockStorageSourceChainDetach then would call qemuMonitorBlockdevDel
with NULL argument, which would report an JSON formatting error as
'node-name' is mandatory.

Thus the code was doing pointless stuff but this commit isn't fixing
anything regarding to the previous error.


> 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);

I'll tweak the commit message slightly.

Reviewed-by: Peter Krempa <pkrempa@redhat.com>