blockdev: Fix blockdev-snapshot-sync error reporting for no medium

[PATCH] blockdev: Fix blockdev-snapshot-sync error reporting for no medium

Posted by Markus Armbruster 8 months, 3 weeks ago

When external_snapshot_abort() rejects a BlockDriverState without a
medium, it creates an error like this:

        error_setg(errp, "Device '%s' has no medium", device);

Trouble is @device can be null.  My system formats null as "(null)",
but other systems might crash.  Reproducer:

1. Create a block device without a medium

    -> {"execute": "blockdev-add", "arguments": {"driver": "host_cdrom", "node-name": "blk0", "filename": "/dev/sr0"}}
    <- {"return": {}}

3. Attempt to snapshot it

    -> {"execute":"blockdev-snapshot-sync", "arguments": { "node-name": "blk0", "snapshot-file":"/tmp/foo.qcow2","format":"qcow2"}}
    <- {"error": {"class": "GenericError", "desc": "Device '(null)' has no medium"}}

Broken when commit 0901f67ecdb made @device optional.

Use bdrv_get_device_or_node_name() instead.  Now it fails as it
should:

    <- {"error": {"class": "GenericError", "desc": "Device 'blk0' has no medium"}}

Fixes: 0901f67ecdb7 (qmp: Allow to take external snapshots on bs graphs node.)
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index d8fb3399f5..057601dcf0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1395,7 +1395,8 @@ static void external_snapshot_action(TransactionAction *action,
     bdrv_drained_begin(state->old_bs);
 
     if (!bdrv_is_inserted(state->old_bs)) {
-        error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+        error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM,
+                   bdrv_get_device_or_node_name(state->old_bs));
         return;
     }
 
-- 
2.44.0

Re: [PATCH] blockdev: Fix blockdev-snapshot-sync error reporting for no medium

Posted by Kevin Wolf 8 months, 1 week ago

Am 06.03.2024 um 15:28 hat Markus Armbruster geschrieben:
> When external_snapshot_abort() rejects a BlockDriverState without a
> medium, it creates an error like this:
> 
>         error_setg(errp, "Device '%s' has no medium", device);
> 
> Trouble is @device can be null.  My system formats null as "(null)",
> but other systems might crash.  Reproducer:
> 
> 1. Create a block device without a medium
> 
>     -> {"execute": "blockdev-add", "arguments": {"driver": "host_cdrom", "node-name": "blk0", "filename": "/dev/sr0"}}
>     <- {"return": {}}
> 
> 3. Attempt to snapshot it
> 
>     -> {"execute":"blockdev-snapshot-sync", "arguments": { "node-name": "blk0", "snapshot-file":"/tmp/foo.qcow2","format":"qcow2"}}
>     <- {"error": {"class": "GenericError", "desc": "Device '(null)' has no medium"}}
> 
> Broken when commit 0901f67ecdb made @device optional.
> 
> Use bdrv_get_device_or_node_name() instead.  Now it fails as it
> should:
> 
>     <- {"error": {"class": "GenericError", "desc": "Device 'blk0' has no medium"}}
> 
> Fixes: 0901f67ecdb7 (qmp: Allow to take external snapshots on bs graphs node.)
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Thanks, applied to the block branch.

Kevin