[PATCH 0/2] Clarify error messages pertaining to 'node-name'

[PATCH 0/2] Clarify error messages pertaining to 'node-name'

[Connor Kuehl]

Some error messages contain ambiguous representations of the 'node-name'
parameter. This can be particularly confusing when exchanging QMP
messages (C = client, S = server):

C: {"execute": "block_resize", "arguments": { "device": "my_file", "size": 26843545600 }}
S: {"error": {"class": "GenericError", "desc": "Cannot find device=my_file nor node_name="}}
                                                                               ^^^^^^^^^

This error message suggests one could send a message with a key called
'node_name':

C: {"execute": "block_resize", "arguments": { "node_name": "my_file", "size": 26843545600 }}
                                               ^^^^^^^^^

but using the underscore is actually incorrect, the parameter should be
'node-name':

S: {"error": {"class": "GenericError", "desc": "Parameter 'node_name' is unexpected"}}

This behavior was uncovered in bz1651437[1], but I ended up going down a
rabbit hole looking for other areas where this miscommunication might
occur and changing those accordingly as well.

[1] https://bugzilla.redhat.com/1651437

Connor Kuehl (2):
  block: Clarify error messages pertaining to 'node-name'
  blockdev: Clarify error messages pertaining to 'node-name'

 block.c                    |  8 ++++----
 blockdev.c                 | 13 +++++++------
 tests/qemu-iotests/040     |  4 ++--
 tests/qemu-iotests/249.out |  2 +-
 4 files changed, 14 insertions(+), 13 deletions(-)

-- 
2.29.2

[PATCH 1/2] block: Clarify error messages pertaining to 'node-name'

[Connor Kuehl]

Reported-by: Tingting Mao <timao@redhat.com>
Fixes: https://bugzilla.redhat.com/1651437
Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
---
 block.c                    | 8 ++++----
 tests/qemu-iotests/040     | 4 ++--
 tests/qemu-iotests/249.out | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index a1f3cecd75..2daff6d29a 100644
--- a/block.c
+++ b/block.c
@@ -1440,7 +1440,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
          * Check for empty string or invalid characters, but not if it is
          * generated (generated names use characters not available to the user)
          */
-        error_setg(errp, "Invalid node name");
+        error_setg(errp, "Invalid node-name: '%s'", node_name);
         return;
     }
 
@@ -1453,7 +1453,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
 
     /* takes care of avoiding duplicates node names */
     if (bdrv_find_node(node_name)) {
-        error_setg(errp, "Duplicate node name");
+        error_setg(errp, "Duplicate nodes with node-name='%s'", node_name);
         goto out;
     }
 
@@ -5432,7 +5432,7 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
         }
     }
 
-    error_setg(errp, "Cannot find device=%s nor node_name=%s",
+    error_setg(errp, "Cannot find device=\'%s\' nor node-name=\'%s\'",
                      device ? device : "",
                      node_name ? node_name : "");
     return NULL;
@@ -6752,7 +6752,7 @@ BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
     AioContext *aio_context;
 
     if (!to_replace_bs) {
-        error_setg(errp, "Node name '%s' not found", node_name);
+        error_setg(errp, "Failed to find node with node-name='%s'", node_name);
         return NULL;
     }
 
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 7ebc9ed825..336ff7c4f2 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -175,13 +175,13 @@ class TestSingleDrive(ImageCommitTestCase):
         self.assert_no_active_block_jobs()
         result = self.vm.qmp('block-commit', device='drive0', top_node='badfile', base_node='base')
         self.assert_qmp(result, 'error/class', 'GenericError')
-        self.assert_qmp(result, 'error/desc', "Cannot find device= nor node_name=badfile")
+        self.assert_qmp(result, 'error/desc', "Cannot find device='' nor node-name='badfile'")
 
     def test_base_node_invalid(self):
         self.assert_no_active_block_jobs()
         result = self.vm.qmp('block-commit', device='drive0', top_node='mid', base_node='badfile')
         self.assert_qmp(result, 'error/class', 'GenericError')
-        self.assert_qmp(result, 'error/desc', "Cannot find device= nor node_name=badfile")
+        self.assert_qmp(result, 'error/desc', "Cannot find device='' nor node-name='badfile'")
 
     def test_top_path_and_node(self):
         self.assert_no_active_block_jobs()
diff --git a/tests/qemu-iotests/249.out b/tests/qemu-iotests/249.out
index 92ec81db03..d2bf9be85e 100644
--- a/tests/qemu-iotests/249.out
+++ b/tests/qemu-iotests/249.out
@@ -18,7 +18,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.
                      'filter-node-name': '1234'}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}}
-{"error": {"class": "GenericError", "desc": "Invalid node name"}}
+{"error": {"class": "GenericError", "desc": "Invalid node-name: '1234'"}}
 
 === Send a write command to a drive opened in read-only mode (2)
 
-- 
2.29.2

[PATCH 2/2] blockdev: Clarify error messages pertaining to 'node-name'

[Connor Kuehl]

Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
---
 blockdev.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index cd438e60e3..7c7ab2b386 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1515,13 +1515,13 @@ static void external_snapshot_prepare(BlkActionState *common,
             s->has_snapshot_node_name ? s->snapshot_node_name : NULL;
 
         if (node_name && !snapshot_node_name) {
-            error_setg(errp, "New overlay node name missing");
+            error_setg(errp, "New overlay node-name missing");
             goto out;
         }
 
         if (snapshot_node_name &&
             bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
-            error_setg(errp, "New overlay node name already in use");
+            error_setg(errp, "New overlay node-name already in use");
             goto out;
         }
 
@@ -3598,13 +3598,14 @@ void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp)
 
     /* Check for the selected node name */
     if (!options->has_node_name) {
-        error_setg(errp, "Node name not specified");
+        error_setg(errp, "node-name not specified");
         goto fail;
     }
 
     bs = bdrv_find_node(options->node_name);
     if (!bs) {
-        error_setg(errp, "Cannot find node named '%s'", options->node_name);
+        error_setg(errp, "Failed to find node with node-name='%s'",
+                   options->node_name);
         goto fail;
     }
 
@@ -3635,7 +3636,7 @@ void qmp_blockdev_del(const char *node_name, Error **errp)
 
     bs = bdrv_find_node(node_name);
     if (!bs) {
-        error_setg(errp, "Cannot find node %s", node_name);
+        error_setg(errp, "Failed to find node with node-name='%s'", node_name);
         return;
     }
     if (bdrv_has_blk(bs)) {
@@ -3758,7 +3759,7 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
 
     bs = bdrv_find_node(node_name);
     if (!bs) {
-        error_setg(errp, "Cannot find node %s", node_name);
+        error_setg(errp, "Failed to find node with node-name='%s'", node_name);
         return;
     }
 
-- 
2.29.2

Re: [PATCH 0/2] Clarify error messages pertaining to 'node-name'

[Kevin Wolf]

Am 02.03.2021 um 00:36 hat Connor Kuehl geschrieben:
> Some error messages contain ambiguous representations of the 'node-name'
> parameter. This can be particularly confusing when exchanging QMP
> messages (C = client, S = server):
> 
> C: {"execute": "block_resize", "arguments": { "device": "my_file", "size": 26843545600 }}
> S: {"error": {"class": "GenericError", "desc": "Cannot find device=my_file nor node_name="}}
>                                                                                ^^^^^^^^^

Arguably, this error message isn't great anyway because of the empty
string node name. We didn't even look for a node name, so why mention it
in the error message?

But your patches are certainly a good improvement already. I would have
merged them, but git grep 'nor node_name=' shows that you missed to
update a few tests, so they fail after the series. I suppose you only
caught the ones that are run by default in 'make check' and missed the
ones that require running the qemu-iotests 'check' script manually.

> This error message suggests one could send a message with a key called
> 'node_name':
> 
> C: {"execute": "block_resize", "arguments": { "node_name": "my_file", "size": 26843545600 }}
>                                                ^^^^^^^^^
> 
> but using the underscore is actually incorrect, the parameter should be
> 'node-name':
> 
> S: {"error": {"class": "GenericError", "desc": "Parameter 'node_name' is unexpected"}}
> 
> This behavior was uncovered in bz1651437[1], but I ended up going down a
> rabbit hole looking for other areas where this miscommunication might
> occur and changing those accordingly as well.
> 
> [1] https://bugzilla.redhat.com/1651437

This is a good explanation for the change you're making. I think it
deserves being committed to the repository in the commit message for
patch 1.

Kevin

Re: [PATCH 0/2] Clarify error messages pertaining to 'node-name'

[Connor Kuehl]

On 3/3/21 3:53 AM, Kevin Wolf wrote:
> Am 02.03.2021 um 00:36 hat Connor Kuehl geschrieben:
>> Some error messages contain ambiguous representations of the 'node-name'
>> parameter. This can be particularly confusing when exchanging QMP
>> messages (C = client, S = server):
>>
>> C: {"execute": "block_resize", "arguments": { "device": "my_file", "size": 26843545600 }}
>> S: {"error": {"class": "GenericError", "desc": "Cannot find device=my_file nor node_name="}}
>>                                                                                 ^^^^^^^^^
> 
> Arguably, this error message isn't great anyway because of the empty
> string node name. We didn't even look for a node name, so why mention it
> in the error message?
> 
> But your patches are certainly a good improvement already. I would have
> merged them, but git grep 'nor node_name=' shows that you missed to
> update a few tests, so they fail after the series. I suppose you only
> caught the ones that are run by default in 'make check' and missed the
> ones that require running the qemu-iotests 'check' script manually.

Ah, good catch! Yes, I was only using `make check`, I'll use the `check` 
script to uncover the other failures and fix them accordingly.

>> [..]
> 
> This is a good explanation for the change you're making. I think it
> deserves being committed to the repository in the commit message for
> patch 1.

I'll move this to patch #1 in the next revision of this series.

Thank you!

Connor