* [Qemu-devel] [PATCH v2] numa: clarify error message when node index is out of range in -numa dist, ... @ 2018-05-15 14:48 Igor Mammedov 2018-05-15 15:26 ` Andrew Jones 0 siblings, 1 reply; 7+ messages in thread From: Igor Mammedov @ 2018-05-15 14:48 UTC (permalink / raw) To: qemu-devel; +Cc: ehabkost, drjones 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 <imammedo@redhat.com> Reviewed-by: Andrew Jones <drjones@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; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] numa: clarify error message when node index is out of range in -numa dist, ... 2018-05-15 14:48 [Qemu-devel] [PATCH v2] numa: clarify error message when node index is out of range in -numa dist, Igor Mammedov @ 2018-05-15 15:26 ` Andrew Jones 2018-05-15 15:50 ` [Qemu-devel] [PATCH v3] " Igor Mammedov 2018-05-15 16:47 ` [Qemu-devel] [PATCH v2] " Eric Blake 0 siblings, 2 replies; 7+ messages in thread From: Andrew Jones @ 2018-05-15 15:26 UTC (permalink / raw) To: Igor Mammedov; +Cc: qemu-devel, ehabkost On Tue, May 15, 2018 at 04:48:33PM +0200, Igor Mammedov wrote: > 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 <imammedo@redhat.com> > Reviewed-by: Andrew Jones <drjones@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]", ^ 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. > + MAX(src, dst), MAX_NODES - 1); > return; > } > > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH v3] numa: clarify error message when node index is out of range in -numa dist, ... 2018-05-15 15:26 ` Andrew Jones @ 2018-05-15 15:50 ` Igor Mammedov 2018-05-15 16:47 ` [Qemu-devel] [PATCH v2] " Eric Blake 1 sibling, 0 replies; 7+ messages in thread From: Igor Mammedov @ 2018-05-15 15:50 UTC (permalink / raw) To: qemu-devel; +Cc: drjones, ehabkost 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 <imammedo@redhat.com> Reviewed-by: Andrew Jones <drjones@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; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] numa: clarify error message when node index is out of range in -numa dist, ... 2018-05-15 15:26 ` Andrew Jones 2018-05-15 15:50 ` [Qemu-devel] [PATCH v3] " Igor Mammedov @ 2018-05-15 16:47 ` Eric Blake 2018-05-15 17:37 ` Markus Armbruster 1 sibling, 1 reply; 7+ messages in thread From: Eric Blake @ 2018-05-15 16:47 UTC (permalink / raw) To: Andrew Jones, Igor Mammedov; +Cc: qemu-devel, ehabkost, Markus Armbruster On 05/15/2018 10:26 AM, Andrew Jones wrote: > On Tue, May 15, 2018 at 04:48:33PM +0200, Igor Mammedov wrote: >> 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 <imammedo@redhat.com> >> Reviewed-by: Andrew Jones <drjones@redhat.com> >> --- >> 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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] numa: clarify error message when node index is out of range in -numa dist, ... 2018-05-15 16:47 ` [Qemu-devel] [PATCH v2] " Eric Blake @ 2018-05-15 17:37 ` Markus Armbruster 2018-05-16 14:32 ` Igor Mammedov 0 siblings, 1 reply; 7+ messages in thread From: Markus Armbruster @ 2018-05-15 17:37 UTC (permalink / raw) To: Eric Blake; +Cc: Andrew Jones, Igor Mammedov, qemu-devel, ehabkost Eric Blake <eblake@redhat.com> writes: > On 05/15/2018 10:26 AM, Andrew Jones wrote: >> On Tue, May 15, 2018 at 04:48:33PM +0200, Igor Mammedov wrote: >>> 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 <imammedo@redhat.com> >>> Reviewed-by: Andrew Jones <drjones@redhat.com> >>> --- > >>> 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. >> >>> + 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. > 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] numa: clarify error message when node index is out of range in -numa dist, ... 2018-05-15 17:37 ` Markus Armbruster @ 2018-05-16 14:32 ` Igor Mammedov 2018-05-16 14:46 ` Eric Blake 0 siblings, 1 reply; 7+ messages in thread From: Igor Mammedov @ 2018-05-16 14:32 UTC (permalink / raw) To: Markus Armbruster; +Cc: Eric Blake, Andrew Jones, qemu-devel, ehabkost On Tue, 15 May 2018 19:37:02 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Eric Blake <eblake@redhat.com> writes: > > > On 05/15/2018 10:26 AM, Andrew Jones wrote: > >> On Tue, May 15, 2018 at 04:48:33PM +0200, Igor Mammedov wrote: > >>> 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 <imammedo@redhat.com> > >>> Reviewed-by: Andrew Jones <drjones@redhat.com> > >>> --- > > > >>> 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. > >> > >>> + 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 > Correct. Providing help on valid values is exactly what > error_append_hint() is for. > > > 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v2] numa: clarify error message when node index is out of range in -numa dist, ... 2018-05-16 14:32 ` Igor Mammedov @ 2018-05-16 14:46 ` Eric Blake 0 siblings, 0 replies; 7+ messages in thread From: Eric Blake @ 2018-05-16 14:46 UTC (permalink / raw) To: Igor Mammedov, Markus Armbruster; +Cc: Andrew Jones, qemu-devel, ehabkost On 05/16/2018 09:32 AM, Igor Mammedov wrote: >>> 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 > Use of error_append_hint() requires you to provide a newline (unlike error_setg(). But as Markus pointed out, you don't need to use it: > >> Correct. Providing help on valid values is exactly what >> error_append_hint() is for. >> >>> 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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-05-16 14:46 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-15 14:48 [Qemu-devel] [PATCH v2] numa: clarify error message when node index is out of range in -numa dist, Igor Mammedov 2018-05-15 15:26 ` Andrew Jones 2018-05-15 15:50 ` [Qemu-devel] [PATCH v3] " Igor Mammedov 2018-05-15 16:47 ` [Qemu-devel] [PATCH v2] " Eric Blake 2018-05-15 17:37 ` Markus Armbruster 2018-05-16 14:32 ` Igor Mammedov 2018-05-16 14:46 ` Eric Blake
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.