[PATCH 1/1] block: improve error logging in bdrv_reopen_prepare()

[PATCH 1/1] block: improve error logging in bdrv_reopen_prepare()

[Denis V. Lunev]

The error generated when the option could not be changed inside
bdrv_reopen_prepare() does not give a clue about problematic
BlockDriverState as we could get very long tree of devices.

The patch adds node name to the error report in the same way as done
above.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block.c                    |  4 +++-
 tests/qemu-iotests/133     | 12 ++++++------
 tests/qemu-iotests/133.out | 12 ++++++------
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index b4a89207ad..0da38652c3 100644
--- a/block.c
+++ b/block.c
@@ -4828,7 +4828,9 @@ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
              * so they will stay unchanged.
              */
             if (!qobject_is_equal(new, old)) {
-                error_setg(errp, "Cannot change the option '%s'", entry->key);
+                error_setg(errp, "Cannot change the option '%s' on '%s'",
+                           entry->key,
+                           bdrv_get_device_or_node_name(reopen_state->bs));
                 ret = -EINVAL;
                 goto error;
             }
diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133
index d997db1685..63fd9886ad 100755
--- a/tests/qemu-iotests/133
+++ b/tests/qemu-iotests/133
@@ -47,9 +47,9 @@ echo
 echo "=== Check that node-name can't be changed ==="
 echo
 
-$QEMU_IO -c 'reopen -o node-name=foo' $TEST_IMG
-$QEMU_IO -c 'reopen -o file.node-name=foo' $TEST_IMG
-$QEMU_IO -c 'reopen -o backing.node-name=foo' $TEST_IMG
+$QEMU_IO -c 'reopen -o node-name=foo' $TEST_IMG 2>&1 | _filter_generated_node_ids
+$QEMU_IO -c 'reopen -o file.node-name=foo' $TEST_IMG 2>&1 | _filter_generated_node_ids
+$QEMU_IO -c 'reopen -o backing.node-name=foo' $TEST_IMG 2>&1 | _filter_generated_node_ids
 
 echo
 echo "=== Check that unchanged node-name is okay ==="
@@ -69,9 +69,9 @@ echo
 echo "=== Check that driver can't be changed ==="
 echo
 
-$QEMU_IO -c 'reopen -o driver=raw' $TEST_IMG
-$QEMU_IO -c 'reopen -o file.driver=qcow2' $TEST_IMG
-$QEMU_IO -c 'reopen -o backing.driver=file' $TEST_IMG
+$QEMU_IO -c 'reopen -o driver=raw' $TEST_IMG 2>&1 | _filter_generated_node_ids
+$QEMU_IO -c 'reopen -o file.driver=qcow2' $TEST_IMG 2>&1 | _filter_generated_node_ids
+$QEMU_IO -c 'reopen -o backing.driver=file' $TEST_IMG 2>&1 | _filter_generated_node_ids
 
 echo
 echo "=== Check that unchanged driver is okay ==="
diff --git a/tests/qemu-iotests/133.out b/tests/qemu-iotests/133.out
index d70c2e8041..26aad4a0fd 100644
--- a/tests/qemu-iotests/133.out
+++ b/tests/qemu-iotests/133.out
@@ -4,18 +4,18 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t
 
 === Check that node-name can't be changed ===
 
-qemu-io: Cannot change the option 'node-name'
-qemu-io: Cannot change the option 'node-name'
-qemu-io: Cannot change the option 'node-name'
+qemu-io: Cannot change the option 'node-name' on 'NODE_NAME'
+qemu-io: Cannot change the option 'node-name' on 'NODE_NAME'
+qemu-io: Cannot change the option 'node-name' on 'NODE_NAME'
 
 === Check that unchanged node-name is okay ===
 
 
 === Check that driver can't be changed ===
 
-qemu-io: Cannot change the option 'driver'
-qemu-io: Cannot change the option 'driver'
-qemu-io: Cannot change the option 'driver'
+qemu-io: Cannot change the option 'driver' on 'NODE_NAME'
+qemu-io: Cannot change the option 'driver' on 'NODE_NAME'
+qemu-io: Cannot change the option 'driver' on 'NODE_NAME'
 
 === Check that unchanged driver is okay ===
 
-- 
2.34.1

Re: [PATCH 1/1] block: improve error logging in bdrv_reopen_prepare()

[Vladimir Sementsov-Ogievskiy]

On 13.02.23 13:31, Denis V. Lunev wrote:
> The error generated when the option could not be changed inside
> bdrv_reopen_prepare() does not give a clue about problematic
> BlockDriverState as we could get very long tree of devices.
> 
> The patch adds node name to the error report in the same way as done
> above.
> 
> Signed-off-by: Denis V. Lunev<den@openvz.org>
> CC: Kevin Wolf<kwolf@redhat.com>
> CC: Hanna Reitz<hreitz@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

Re: [PATCH 1/1] block: improve error logging in bdrv_reopen_prepare()

[Kevin Wolf]

Am 13.02.2023 um 11:31 hat Denis V. Lunev geschrieben:
> The error generated when the option could not be changed inside
> bdrv_reopen_prepare() does not give a clue about problematic
> BlockDriverState as we could get very long tree of devices.
> 
> The patch adds node name to the error report in the same way as done
> above.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> CC: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Truly fascinating how inconsistent error reporting is in
bdrv_reopen_prepare(). Some places use the node name, some places device
or node name, some places filename and some places nothing. Your choice
is as good as any.

The only problem I can see with this patch is that qemu-iotests 245
needs an update, too.

Kevin