[PATCH v2] hw/core/qdev-properties-system: Add missing return in set_drive_helper()

[PATCH v2] hw/core/qdev-properties-system: Add missing return in set_drive_helper()

[Fiona Ebner]

Currently, changing the 'drive' property of e.g. a scsi-hd object will
result in an assertion failure if the aio context of the block node
it's replaced with doesn't match the current aio context:

> bdrv_replace_child_noperm: Assertion `bdrv_get_aio_context(old_bs) ==
> bdrv_get_aio_context(new_bs)' failed.

The problematic scenario is already detected, but a 'return' statement
was missing.

Cc: qemu-stable@nongnu.org
Fixes: d1a58c176a ("qdev: allow setting drive property for realized device")
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v2:
* Add missing condition in commit message, sorry for the noise!

Reproducer:

#!/bin/bash
rm /tmp/disk0.raw
rm /tmp/disk1.raw
./qemu-img create -f raw /tmp/disk0.raw 1G
./qemu-img create -f raw /tmp/disk1.raw 1G
./qemu-system-x86_64 --qmp stdio \
--blockdev file,node-name=node0,filename=/tmp/disk0.raw \
--blockdev file,node-name=node1,filename=/tmp/disk1.raw \
--nodefaults \
--object 'iothread,id=iothread0' \
--device 'virtio-scsi-pci,id=virtioscsi0,bus=pci.0,addr=0x3,iothread=iothread0' \
--device 'scsi-hd,bus=virtioscsi0.0,scsi-id=1,drive=node0,id=scsi0' \
<<EOF
{"execute": "qmp_capabilities"}
{"execute": "qom-set", "arguments": { "path": "/machine/peripheral/scsi0", "property": "drive", "value": "node1" } }
{"execute": "quit"}
EOF

 hw/core/qdev-properties-system.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 8e11e6388b..24e145d870 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -145,6 +145,7 @@ static void set_drive_helper(Object *obj, Visitor *v, const char *name,
         if (ctx != bdrv_get_aio_context(bs)) {
             error_setg(errp, "Different aio context is not supported for new "
                        "node");
+            return;
         }
 
         blk_replace_bs(blk, bs, errp);
-- 
2.39.5

Re: [PATCH v2] hw/core/qdev-properties-system: Add missing return in set_drive_helper()

[Daniel P. Berrangé]

Copying block maintainers, since while this is in QOM source files, this 
method is defacto block layer code.

On Fri, May 23, 2025 at 09:02:11AM +0200, Fiona Ebner wrote:
> Currently, changing the 'drive' property of e.g. a scsi-hd object will
> result in an assertion failure if the aio context of the block node
> it's replaced with doesn't match the current aio context:
> 
> > bdrv_replace_child_noperm: Assertion `bdrv_get_aio_context(old_bs) ==
> > bdrv_get_aio_context(new_bs)' failed.
> 
> The problematic scenario is already detected, but a 'return' statement
> was missing.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: d1a58c176a ("qdev: allow setting drive property for realized device")
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Changes in v2:
> * Add missing condition in commit message, sorry for the noise!
> 
> Reproducer:
> 
> #!/bin/bash
> rm /tmp/disk0.raw
> rm /tmp/disk1.raw
> ./qemu-img create -f raw /tmp/disk0.raw 1G
> ./qemu-img create -f raw /tmp/disk1.raw 1G
> ./qemu-system-x86_64 --qmp stdio \
> --blockdev file,node-name=node0,filename=/tmp/disk0.raw \
> --blockdev file,node-name=node1,filename=/tmp/disk1.raw \
> --nodefaults \
> --object 'iothread,id=iothread0' \
> --device 'virtio-scsi-pci,id=virtioscsi0,bus=pci.0,addr=0x3,iothread=iothread0' \
> --device 'scsi-hd,bus=virtioscsi0.0,scsi-id=1,drive=node0,id=scsi0' \
> <<EOF
> {"execute": "qmp_capabilities"}
> {"execute": "qom-set", "arguments": { "path": "/machine/peripheral/scsi0", "property": "drive", "value": "node1" } }
> {"execute": "quit"}
> EOF
> 
>  hw/core/qdev-properties-system.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 8e11e6388b..24e145d870 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -145,6 +145,7 @@ static void set_drive_helper(Object *obj, Visitor *v, const char *name,
>          if (ctx != bdrv_get_aio_context(bs)) {
>              error_setg(errp, "Different aio context is not supported for new "
>                         "node");
> +            return;
>          }
>  
>          blk_replace_bs(blk, bs, errp);

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2] hw/core/qdev-properties-system: Add missing return in set_drive_helper()

[Kevin Wolf]

Am 23.05.2025 um 09:02 hat Fiona Ebner geschrieben:
> Currently, changing the 'drive' property of e.g. a scsi-hd object will
> result in an assertion failure if the aio context of the block node
> it's replaced with doesn't match the current aio context:
> 
> > bdrv_replace_child_noperm: Assertion `bdrv_get_aio_context(old_bs) ==
> > bdrv_get_aio_context(new_bs)' failed.
> 
> The problematic scenario is already detected, but a 'return' statement
> was missing.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: d1a58c176a ("qdev: allow setting drive property for realized device")
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Changes in v2:
> * Add missing condition in commit message, sorry for the noise!

Thanks, applied to the block branch.

But while we're here...

> Reproducer:
> 
> #!/bin/bash
> rm /tmp/disk0.raw
> rm /tmp/disk1.raw
> ./qemu-img create -f raw /tmp/disk0.raw 1G
> ./qemu-img create -f raw /tmp/disk1.raw 1G
> ./qemu-system-x86_64 --qmp stdio \
> --blockdev file,node-name=node0,filename=/tmp/disk0.raw \
> --blockdev file,node-name=node1,filename=/tmp/disk1.raw \
> --nodefaults \
> --object 'iothread,id=iothread0' \
> --device 'virtio-scsi-pci,id=virtioscsi0,bus=pci.0,addr=0x3,iothread=iothread0' \
> --device 'scsi-hd,bus=virtioscsi0.0,scsi-id=1,drive=node0,id=scsi0' \
> <<EOF
> {"execute": "qmp_capabilities"}
> {"execute": "qom-set", "arguments": { "path": "/machine/peripheral/scsi0", "property": "drive", "value": "node1" } }
> {"execute": "quit"}
> EOF

...could you put this in a qemu-iotests case as a follow-up? It looks
like we don't have a proper test for qom-set on drive properties yet.

>  hw/core/qdev-properties-system.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 8e11e6388b..24e145d870 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -145,6 +145,7 @@ static void set_drive_helper(Object *obj, Visitor *v, const char *name,
>          if (ctx != bdrv_get_aio_context(bs)) {
>              error_setg(errp, "Different aio context is not supported for new "
>                         "node");
> +            return;
>          }
>  
>          blk_replace_bs(blk, bs, errp);

While this is the trivial fix for what was originally meant, I'm not
sure if that's the best way to handle the situation. I would have
expected that we do something similar as in bdrv_attach_child_common()
and try to move @bs into @ctx before failing.

Nowadays, even on failure to move @bs, we don't strictly have to error
out any more since the backends are properly multithreaded. But we
haven't made that change even to bdrv_attach_child_common() yet. I
wonder if we should only do that with a force option.

We don't have to address this now, but it probably doesn't hurt to think
a bit about what we want it to look like in the long run.

Kevin