[Qemu-devel] [PATCH] qemu-option: Include name of invalid parameter in error message

[Qemu-devel] [PATCH] qemu-option: Include name of invalid parameter in error message

[Stefan Weil]

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

Re: [Qemu-devel] [PATCH] qemu-option: Include name of invalid parameter in error message

[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.

Re: [Qemu-devel] [PATCH] qemu-option: Include name of invalid parameter in error message

[Stefan Weil]

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.

Re: [Qemu-devel] [PATCH] qemu-option: Include name of invalid parameter in error message

[Markus Armbruster]

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.