[PATCH 0/2] blockdev: Fix block_resize error reporting for op blockers

[PATCH 0/2] blockdev: Fix block_resize error reporting for op blockers

[Markus Armbruster]

PATCH 2 requires my "[PATCH] char: Slightly better error reporting
when chardev is in use" posted earlier today, PATCH 1 does not.

Based-on: <20240306081505.2258405-1-armbru@redhat.com>

Markus Armbruster (2):
  blockdev: Fix block_resize error reporting for op blockers
  qerror: QERR_DEVICE_IN_USE is no longer used, drop

 include/qapi/qmp/qerror.h | 3 ---
 blockdev.c                | 3 +--
 2 files changed, 1 insertion(+), 5 deletions(-)

-- 
2.44.0

[PATCH 1/2] blockdev: Fix block_resize error reporting for op blockers

[Markus Armbruster]

When block_resize() runs into an op blocker, it creates an error like
this:

        error_setg(errp, "Device '%s' is in use", device);

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

1. Create two block devices

    -> {"execute": "blockdev-add", "arguments": {"driver": "file", "node-name": "blk0", "filename": "64k.img"}}
    <- {"return": {}}
    -> {"execute": "blockdev-add", "arguments": {"driver": "file", "node-name": "blk1", "filename": "m.img"}}
    <- {"return": {}}

2. Put a blocker on one them

    -> {"execute": "blockdev-mirror", "arguments": {"job-id": "job0", "device": "blk0", "target": "blk1", "sync": "full"}}
    {"return": {}}
    -> {"execute": "job-pause", "arguments": {"id": "job0"}}
    {"return": {}}
    -> {"execute": "job-complete", "arguments": {"id": "job0"}}
    {"return": {}}

   Note: job events elided for brevity.

3. Attempt to resize

    -> {"execute": "block_resize", "arguments": {"node-name": "blk1", "size":32768}}
    <- {"error": {"class": "GenericError", "desc": "Device '(null)' is in use"}}

Broken when commit 3b1dbd11a60 made @device optional.  Fixed in commit
ed3d2ec98a3 (block: Add errp to b{lk,drv}_truncate()), except for this
one instance.

Fix it by using the error message provided by the op blocker instead,
so it fails like this:

    <- {"error": {"class": "GenericError", "desc": "Node 'blk1' is busy: block device is in use by block job: mirror"}}

Fixes: 3b1dbd11a60d (qmp: Allow block_resize to manipulate bs graph nodes.)
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index f8bb0932f8..d8fb3399f5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2252,8 +2252,7 @@ void coroutine_fn qmp_block_resize(const char *device, const char *node_name,
     }
 
     bdrv_graph_co_rdlock();
-    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, NULL)) {
-        error_setg(errp, QERR_DEVICE_IN_USE, device);
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, errp)) {
         bdrv_graph_co_rdunlock();
         return;
     }
-- 
2.44.0

Re: [PATCH 1/2] blockdev: Fix block_resize error reporting for op blockers

[Philippe Mathieu-Daudé]

On 6/3/24 14:10, Markus Armbruster wrote:
> When block_resize() runs into an op blocker, it creates an error like
> this:
> 
>          error_setg(errp, "Device '%s' is in use", device);
> 
> Trouble is @device can be null.  My system formats null as "(null)",
> but other systems might crash.  Reproducer:
> 
> 1. Create two block devices
> 
>      -> {"execute": "blockdev-add", "arguments": {"driver": "file", "node-name": "blk0", "filename": "64k.img"}}
>      <- {"return": {}}
>      -> {"execute": "blockdev-add", "arguments": {"driver": "file", "node-name": "blk1", "filename": "m.img"}}
>      <- {"return": {}}
> 
> 2. Put a blocker on one them
> 
>      -> {"execute": "blockdev-mirror", "arguments": {"job-id": "job0", "device": "blk0", "target": "blk1", "sync": "full"}}
>      {"return": {}}
>      -> {"execute": "job-pause", "arguments": {"id": "job0"}}
>      {"return": {}}
>      -> {"execute": "job-complete", "arguments": {"id": "job0"}}
>      {"return": {}}
> 
>     Note: job events elided for brevity.
> 
> 3. Attempt to resize
> 
>      -> {"execute": "block_resize", "arguments": {"node-name": "blk1", "size":32768}}
>      <- {"error": {"class": "GenericError", "desc": "Device '(null)' is in use"}}
> 
> Broken when commit 3b1dbd11a60 made @device optional.  Fixed in commit
> ed3d2ec98a3 (block: Add errp to b{lk,drv}_truncate()), except for this
> one instance.
> 
> Fix it by using the error message provided by the op blocker instead,
> so it fails like this:
> 
>      <- {"error": {"class": "GenericError", "desc": "Node 'blk1' is busy: block device is in use by block job: mirror"}}
> 
> Fixes: 3b1dbd11a60d (qmp: Allow block_resize to manipulate bs graph nodes.)
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   blockdev.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 2/2] qerror: QERR_DEVICE_IN_USE is no longer used, drop

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/qmp/qerror.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 8dd9fcb071..0c2689cf8a 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -23,9 +23,6 @@
 #define QERR_DEVICE_HAS_NO_MEDIUM \
     "Device '%s' has no medium"
 
-#define QERR_DEVICE_IN_USE \
-    "Device '%s' is in use"
-
 #define QERR_DEVICE_NO_HOTPLUG \
     "Device '%s' does not support hotplugging"
 
-- 
2.44.0

Re: [PATCH 2/2] qerror: QERR_DEVICE_IN_USE is no longer used, drop

[Philippe Mathieu-Daudé]

On 6/3/24 14:10, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   include/qapi/qmp/qerror.h | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 8dd9fcb071..0c2689cf8a 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -23,9 +23,6 @@
>   #define QERR_DEVICE_HAS_NO_MEDIUM \
>       "Device '%s' has no medium"
>   
> -#define QERR_DEVICE_IN_USE \
> -    "Device '%s' is in use"

\o/

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>