[Qemu-devel] [PATCH v2] numa: clarify error message when node index is out of range in -numa dist, ...

[Igor Mammedov]

When using following CLI:
-numa dist,src=128,dst=1,val=20
user gets a rather confusing error message:
"Invalid node 128, max possible could be 128"

Where 128 is number of nodes that QEMU supports (MAX_NODES),
while src/dst is an index up to that limit, so it should be
MAX_NODES - 1 in error message.
Make error message to explicitly state valid range for node
index to be more clear.

Signed-off-by: Igor Mammedov <***@redhat.com>
Reviewed-by: Andrew Jones <***@redhat.com>
---
numa.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/numa.c b/numa.c
index a3637cc..9f0c49f 100644
--- a/numa.c
+++ b/numa.c
@@ -142,8 +142,8 @@ static void parse_numa_distance(NumaDistOptions *dist, Error **errp)

if (src >= MAX_NODES || dst >= MAX_NODES) {
error_setg(errp,
- "Invalid node %d, max possible could be %d",
- MAX(src, dst), MAX_NODES);
+ "Invalid node %d, The valid node range is [0 - %d]",
+ MAX(src, dst), MAX_NODES - 1);
return;
}

[Andrew Jones]

Post by Igor Mammedov
-numa dist,src=128,dst=1,val=20
"Invalid node 128, max possible could be 128"
Where 128 is number of nodes that QEMU supports (MAX_NODES),
while src/dst is an index up to that limit, so it should be
MAX_NODES - 1 in error message.
Make error message to explicitly state valid range for node
index to be more clear.
---
numa.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/numa.c b/numa.c
index a3637cc..9f0c49f 100644
--- a/numa.c
+++ b/numa.c
@@ -142,8 +142,8 @@ static void parse_numa_distance(NumaDistOptions *dist, Error **errp)
if (src >= MAX_NODES || dst >= MAX_NODES) {
error_setg(errp,
- "Invalid node %d, max possible could be %d",
- MAX(src, dst), MAX_NODES);
+ "Invalid node %d, The valid node range is [0 - %d]",

^ should be a '.'

And maybe need a '.' at the end of the second sentence too, as it's not
an error phrase, but a real sentence.

Post by Igor Mammedov
+ MAX(src, dst), MAX_NODES - 1);
return;
}
--
2.7.4

[Igor Mammedov]

When using following CLI:
-numa dist,src=128,dst=1,val=20
user gets a rather confusing error message:
"Invalid node 128, max possible could be 128"

Where 128 is number of nodes that QEMU supports (MAX_NODES),
while src/dst is an index up to that limit, so it should be
MAX_NODES - 1 in error message.
Make error message to explicitly state valid range for node
index to be more clear.

Signed-off-by: Igor Mammedov <***@redhat.com>
Reviewed-by: Andrew Jones <***@redhat.com>
---
v3:
- s/,/./ in error message
- add '.' at the end of sentence
---
numa.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/numa.c b/numa.c
index 188bfdf..d98c106 100644
--- a/numa.c
+++ b/numa.c
@@ -142,8 +142,8 @@ static void parse_numa_distance(NumaDistOptions *dist, Error **errp)

if (src >= MAX_NODES || dst >= MAX_NODES) {
error_setg(errp,
- "Invalid node %d, max possible could be %d",
- MAX(src, dst), MAX_NODES);
+ "Invalid node %d. The valid node range is [0 - %d].",
+ MAX(src, dst), MAX_NODES - 1);
return;
}

[Eric Blake]

Post by Andrew Jones

Post by Igor Mammedov
-numa dist,src=128,dst=1,val=20
"Invalid node 128, max possible could be 128"
Where 128 is number of nodes that QEMU supports (MAX_NODES),
while src/dst is an index up to that limit, so it should be
MAX_NODES - 1 in error message.
Make error message to explicitly state valid range for node
index to be more clear.
---
if (src >= MAX_NODES || dst >= MAX_NODES) {
error_setg(errp,
- "Invalid node %d, max possible could be %d",
- MAX(src, dst), MAX_NODES);
+ "Invalid node %d, The valid node range is [0 - %d]",

^ should be a '.'
And maybe need a '.' at the end of the second sentence too, as it's not
an error phrase, but a real sentence.

Actually, error_setg() is documented as taking a single phrase (no '.'
included), and that if you need a second sentence, it's better to use
error_append_hint(). Maybe Markus has an opinion on the best way to
word this error message.

[Markus Armbruster]

Post by Eric Blake

Post by Andrew Jones

Post by Igor Mammedov
-numa dist,src=128,dst=1,val=20
"Invalid node 128, max possible could be 128"
Where 128 is number of nodes that QEMU supports (MAX_NODES),
while src/dst is an index up to that limit, so it should be
MAX_NODES - 1 in error message.
Make error message to explicitly state valid range for node
index to be more clear.
---
if (src >= MAX_NODES || dst >= MAX_NODES) {
error_setg(errp,
- "Invalid node %d, max possible could be %d",
- MAX(src, dst), MAX_NODES);
+ "Invalid node %d, The valid node range is [0 - %d]",

^ should be a '.'
And maybe need a '.' at the end of the second sentence too, as it's not
an error phrase, but a real sentence.

Post by Igor Mammedov
+ MAX(src, dst), MAX_NODES - 1);
return;
}

Actually, error_setg() is documented as taking a single phrase (no '.'
included), and that if you need a second sentence, it's better to use
error_append_hint().

Correct. Providing help on valid values is exactly what
error_append_hint() is for.

Post by Eric Blake
Maybe Markus has an opinion on the best way to
word this error message.

Yes: "Parameter 'src' expects an integer between 0 and 127"

Referring to an erroneous key=value by value is not nice. What if the
value occurs in multiple places, and is valid in at least one? key is
there, it's unique[*], so use it.


[*] Except in the few places that use repeated keys to form lists. Ugh.

[Igor Mammedov]

On Tue, 15 May 2018 19:37:02 +0200

Post by Markus Armbruster

Post by Eric Blake

Post by Andrew Jones

Post by Igor Mammedov
-numa dist,src=128,dst=1,val=20
"Invalid node 128, max possible could be 128"
Where 128 is number of nodes that QEMU supports (MAX_NODES),
while src/dst is an index up to that limit, so it should be
MAX_NODES - 1 in error message.
Make error message to explicitly state valid range for node
index to be more clear.
---
if (src >= MAX_NODES || dst >= MAX_NODES) {
error_setg(errp,
- "Invalid node %d, max possible could be %d",
- MAX(src, dst), MAX_NODES);
+ "Invalid node %d, The valid node range is [0 - %d]",

^ should be a '.'
And maybe need a '.' at the end of the second sentence too, as it's not
an error phrase, but a real sentence.

Post by Igor Mammedov
+ MAX(src, dst), MAX_NODES - 1);
return;
}

Actually, error_setg() is documented as taking a single phrase (no '.'
included), and that if you need a second sentence, it's better to use
error_append_hint().

well, using append_hint makes it less readable, before using it we get following error:

$ qemu-system-x86_64 -numa dist,src=128,dst=1,val=20
qemu-system-x86_64: -numa dist,src=128,dst=1,val=20: Invalid node 128, The valid node range is [0 - 127]
$

after using it we get:

$ qemu-system-x86_64 -numa dist,src=128,dst=1,val=20
qemu-system-x86_64: -numa dist,src=128,dst=1,val=20: Invalid node value 128
The valid node range is [0 - 127]$

i.e. an extra newline in the middle of error message and looses automatic
newline at the end so the shell prompt continues error message

Post by Markus Armbruster
Correct. Providing help on valid values is exactly what
error_append_hint() is for.

Post by Eric Blake
Maybe Markus has an opinion on the best way to
word this error message.

Yes: "Parameter 'src' expects an integer between 0 and 127"
Referring to an erroneous key=value by value is not nice. What if the
value occurs in multiple places, and is valid in at least one? key is
there, it's unique[*], so use it.
[*] Except in the few places that use repeated keys to form lists. Ugh.

[Eric Blake]

Post by Igor Mammedov

Post by Markus Armbruster

Post by Eric Blake
Actually, error_setg() is documented as taking a single phrase (no '.'
included), and that if you need a second sentence, it's better to use
error_append_hint().

$ qemu-system-x86_64 -numa dist,src=128,dst=1,val=20
qemu-system-x86_64: -numa dist,src=128,dst=1,val=20: Invalid node 128, The valid node range is [0 - 127]
$
$ qemu-system-x86_64 -numa dist,src=128,dst=1,val=20
qemu-system-x86_64: -numa dist,src=128,dst=1,val=20: Invalid node value 128
The valid node range is [0 - 127]$
i.e. an extra newline in the middle of error message and looses automatic
newline at the end so the shell prompt continues error message

Use of error_append_hint() requires you to provide a newline (unlike

Post by Igor Mammedov

Post by Markus Armbruster
Correct. Providing help on valid values is exactly what
error_append_hint() is for.

Post by Eric Blake
Maybe Markus has an opinion on the best way to
word this error message.

Yes: "Parameter 'src' expects an integer between 0 and 127"

I like this wording better, which avoids the shortfalls that
error_append_hint() would introduce.