* [Qemu-devel] [PATCH] qemu-option: Include name of invalid parameter in error message @ 2010-08-01 10:23 Stefan Weil 2010-08-02 8:40 ` Markus Armbruster 0 siblings, 1 reply; 4+ messages in thread From: Stefan Weil @ 2010-08-01 10:23 UTC (permalink / raw) To: QEMU Developers; +Cc: Anthony Liguori, Markus Armbruster All other error messages in qemu-option.c display the name of the invalid parameter. This seems to be reasonable for invalid identifiers, too. Without it, a debugger is needed to find the name. Cc: Markus Armbruster <armbru@redhat.com> Cc: Anthony Liguori <aliguori@us.ibm.com> Signed-off-by: Stefan Weil <weil@mail.berlios.de> --- qemu-option.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/qemu-option.c b/qemu-option.c index 1f8f41a..ccea267 100644 --- a/qemu-option.c +++ b/qemu-option.c @@ -694,7 +694,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id, int fail_if_exist if (id) { if (!id_wellformed(id)) { - qerror_report(QERR_INVALID_PARAMETER_VALUE, "id", "an identifier"); + qerror_report(QERR_INVALID_PARAMETER_VALUE, id, "an identifier"); error_printf_unless_qmp("Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.\n"); return NULL; } -- 1.7.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-option: Include name of invalid parameter in error message 2010-08-01 10:23 [Qemu-devel] [PATCH] qemu-option: Include name of invalid parameter in error message Stefan Weil @ 2010-08-02 8:40 ` Markus Armbruster 2010-08-02 19:46 ` Stefan Weil 0 siblings, 1 reply; 4+ messages in thread From: Markus Armbruster @ 2010-08-02 8:40 UTC (permalink / raw) To: Stefan Weil; +Cc: Anthony Liguori, QEMU Developers Stefan Weil <weil@mail.berlios.de> writes: > All other error messages in qemu-option.c display the name > of the invalid parameter. This seems to be reasonable for > invalid identifiers, too. Without it, a debugger is needed > to find the name. > > Cc: Markus Armbruster <armbru@redhat.com> > Cc: Anthony Liguori <aliguori@us.ibm.com> > Signed-off-by: Stefan Weil <weil@mail.berlios.de> > --- > qemu-option.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/qemu-option.c b/qemu-option.c > index 1f8f41a..ccea267 100644 > --- a/qemu-option.c > +++ b/qemu-option.c > @@ -694,7 +694,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id, int fail_if_exist > > if (id) { > if (!id_wellformed(id)) { > - qerror_report(QERR_INVALID_PARAMETER_VALUE, "id", "an identifier"); > + qerror_report(QERR_INVALID_PARAMETER_VALUE, id, "an identifier"); > error_printf_unless_qmp("Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.\n"); > return NULL; > } No. QERR_INVALID_PARAMETER_VALUE's first argument is the parameter *name*, not the parameter *value*. In this case, the parameter name is "id". The variable id contains the parameter value. If you need a debugger to find the offending id=, then location information is lacking. Could you give an example where it's hard to find the offending parameter? Here's an example where it's easy to find: $ qemu-system-x86_64 -device e1000,id=. qemu-system-x86_64: -device e1000,id=.: Parameter 'id' expects an identifier Identifiers consist of letters, digits, '-', '.', '_', starting with a letter. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-option: Include name of invalid parameter in error message 2010-08-02 8:40 ` Markus Armbruster @ 2010-08-02 19:46 ` Stefan Weil 2010-08-20 8:57 ` Markus Armbruster 0 siblings, 1 reply; 4+ messages in thread From: Stefan Weil @ 2010-08-02 19:46 UTC (permalink / raw) To: Markus Armbruster; +Cc: Anthony Liguori, QEMU Developers Am 02.08.2010 10:40, schrieb Markus Armbruster: > Stefan Weil <weil@mail.berlios.de> writes: > >> All other error messages in qemu-option.c display the name >> of the invalid parameter. This seems to be reasonable for >> invalid identifiers, too. Without it, a debugger is needed >> to find the name. >> >> Cc: Markus Armbruster <armbru@redhat.com> >> Cc: Anthony Liguori <aliguori@us.ibm.com> >> Signed-off-by: Stefan Weil <weil@mail.berlios.de> >> --- >> qemu-option.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/qemu-option.c b/qemu-option.c >> index 1f8f41a..ccea267 100644 >> --- a/qemu-option.c >> +++ b/qemu-option.c >> @@ -694,7 +694,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, >> const char *id, int fail_if_exist >> >> if (id) { >> if (!id_wellformed(id)) { >> - qerror_report(QERR_INVALID_PARAMETER_VALUE, "id", "an identifier"); >> + qerror_report(QERR_INVALID_PARAMETER_VALUE, id, "an identifier"); >> error_printf_unless_qmp("Identifiers consist of letters, digits, '-', >> '.', '_', starting with a letter.\n"); >> return NULL; >> } > > No. > > QERR_INVALID_PARAMETER_VALUE's first argument is the parameter *name*, > not the parameter *value*. > > In this case, the parameter name is "id". The variable id contains the > parameter value. > > If you need a debugger to find the offending id=, then location > information is lacking. Could you give an example where it's hard to > find the offending parameter? > > Here's an example where it's easy to find: > > $ qemu-system-x86_64 -device e1000,id=. > qemu-system-x86_64: -device e1000,id=.: Parameter 'id' expects an > identifier > Identifiers consist of letters, digits, '-', '.', '_', starting with a > letter. I had a call of qemu_chr_open with an id containing a space (which looks better in the console display where the id is shown) in my ar7 emulation. You can test this scenario using this patch: --- a/vl.c +++ b/vl.c @@ -1700,7 +1700,7 @@ static int serial_parse(const char *devname) fprintf(stderr, "qemu: too many serial ports\n"); exit(1); } - snprintf(label, sizeof(label), "serial%d", index); + snprintf(label, sizeof(label), "serial %d", index); serial_hds[index] = qemu_chr_open(label, devname, NULL); if (!serial_hds[index]) { fprintf(stderr, "qemu: could not open serial device '%s': %s\n", It results in these error messages: qemu: Parameter 'id' expects an identifier Identifiers consist of letters, digits, '-', '.', '_', starting with a letter. qemu: could not open serial device 'vc:80Cx24C': No such file or directory The "location information" is indeed missing. For this kind of error, an assertion would be better than an error message. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-option: Include name of invalid parameter in error message 2010-08-02 19:46 ` Stefan Weil @ 2010-08-20 8:57 ` Markus Armbruster 0 siblings, 0 replies; 4+ messages in thread From: Markus Armbruster @ 2010-08-20 8:57 UTC (permalink / raw) To: Stefan Weil; +Cc: Anthony Liguori, QEMU Developers Stefan Weil <weil@mail.berlios.de> writes: > Am 02.08.2010 10:40, schrieb Markus Armbruster: >> Stefan Weil <weil@mail.berlios.de> writes: >> >>> All other error messages in qemu-option.c display the name >>> of the invalid parameter. This seems to be reasonable for >>> invalid identifiers, too. Without it, a debugger is needed >>> to find the name. >>> >>> Cc: Markus Armbruster <armbru@redhat.com> >>> Cc: Anthony Liguori <aliguori@us.ibm.com> >>> Signed-off-by: Stefan Weil <weil@mail.berlios.de> >>> --- >>> qemu-option.c | 2 +- >>> 1 files changed, 1 insertions(+), 1 deletions(-) >>> >>> diff --git a/qemu-option.c b/qemu-option.c >>> index 1f8f41a..ccea267 100644 >>> --- a/qemu-option.c >>> +++ b/qemu-option.c >>> @@ -694,7 +694,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, >>> const char *id, int fail_if_exist >>> >>> if (id) { >>> if (!id_wellformed(id)) { >>> - qerror_report(QERR_INVALID_PARAMETER_VALUE, "id", "an identifier"); >>> + qerror_report(QERR_INVALID_PARAMETER_VALUE, id, "an identifier"); >>> error_printf_unless_qmp("Identifiers consist of letters, digits, >>> -', '.', '_', starting with a letter.\n"); >>> return NULL; >>> } >> >> No. >> >> QERR_INVALID_PARAMETER_VALUE's first argument is the parameter *name*, >> not the parameter *value*. >> >> In this case, the parameter name is "id". The variable id contains the >> parameter value. >> >> If you need a debugger to find the offending id=, then location >> information is lacking. Could you give an example where it's hard to >> find the offending parameter? >> >> Here's an example where it's easy to find: >> >> $ qemu-system-x86_64 -device e1000,id=. >> qemu-system-x86_64: -device e1000,id=.: Parameter 'id' expects an >> identifier >> Identifiers consist of letters, digits, '-', '.', '_', starting with >> a letter. > > I had a call of qemu_chr_open with an id containing a space > (which looks better in the console display where the id is shown) > in my ar7 emulation. You can test this scenario using this patch: > > --- a/vl.c > +++ b/vl.c > @@ -1700,7 +1700,7 @@ static int serial_parse(const char *devname) > fprintf(stderr, "qemu: too many serial ports\n"); > exit(1); > } > - snprintf(label, sizeof(label), "serial%d", index); > + snprintf(label, sizeof(label), "serial %d", index); > serial_hds[index] = qemu_chr_open(label, devname, NULL); > if (!serial_hds[index]) { > fprintf(stderr, "qemu: could not open serial device '%s': %s\n", > > It results in these error messages: > > qemu: Parameter 'id' expects an identifier > Identifiers consist of letters, digits, '-', '.', '_', starting with a > letter. > qemu: could not open serial device 'vc:80Cx24C': No such file or directory > > The "location information" is indeed missing. For this kind of error, > an assertion would be better than an error message. I agree that programming errors should not be reported as user errors. Down where the error is detected, the code doesn't know whether it's processing hardcoded arguments (programming error), or whether it's processing user arguments (user error). The best we can do with relative ease is report the error assuming it's a user error, and return failure up the call chain. When we reach hardcoded-land, report the programming error. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-08-20 8:57 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-08-01 10:23 [Qemu-devel] [PATCH] qemu-option: Include name of invalid parameter in error message Stefan Weil 2010-08-02 8:40 ` Markus Armbruster 2010-08-02 19:46 ` Stefan Weil 2010-08-20 8:57 ` Markus Armbruster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).