Re: [Qemu-devel] [PATCH v7] vl.c: Output error on invalid machine type

[Paolo Bonzini <pbonzini@redhat.com>]

Il 17/03/2014 09:04, Markus Armbruster ha scritto:
> mrezanin@redhat.com writes:
>
>> From: Miroslav Rezanina <mrezanin@redhat.com>
>>
>> Output error message using qemu's error_report() function when user
>> provides the invalid machine type on the command line. This also saves
>> time to find what issue is when you downgrade from one version of qemu
>> to another that doesn't support required machine type yet (the version
>> user downgraded to have to have this patch applied too, of course).
>>
>> Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
>> ---
>> v7:
>>  - use -machine instead of -M in error help message
>>  - rebase to commit 0056ae2
>>
>> v6:
>>  - print help instead of list supported machines on error
>> ---
>>  vl.c | 21 +++++++++++++--------
>>  1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 862cf20..cbd1381 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2649,15 +2649,20 @@ static MachineClass *machine_parse(const char *name)
>>      if (mc) {
>>          return mc;
>>      }
>> -    printf("Supported machines are:\n");
>> -    for (el = machines; el; el = el->next) {
>> -        MachineClass *mc = el->data;
>> -        QEMUMachine *m = mc->qemu_machine;
>> -        if (m->alias) {
>> -            printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
>> +    if (name && !is_help_option(name)) {
>> +        error_report("Unsupported machine type");
>> +        printf("\nUse -machine help to list supported machines!\n");
>
> Why the newline before 'Use'?
>
> Recommend to write the hint to the same fd as the error message.  An
> obvious way to do that is error_printf(), and it's what we do elsewhere.
>
> Both missed this in my review of v1, sorry.

I'm collecting a few patches for 2.0-rc1, so I fixed this printf and 
applied it.

Paolo

>> +    } else {
>> +        printf("Supported machines are:\n");
>> +        for (el = machines; el; el = el->next) {
>> +            MachineClass *mc = el->data;
>> +            QEMUMachine *m = mc->qemu_machine;
>> +            if (m->alias) {
>> +                printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
>> +            }
>> +            printf("%-20s %s%s\n", m->name, m->desc,
>> +                   m->is_default ? " (default)" : "");
>>          }
>> -        printf("%-20s %s%s\n", m->name, m->desc,
>> -               m->is_default ? " (default)" : "");
>>      }
>>
>>      g_slist_free(machines);
>
> The functions logic is a bit tortured (not your fault).  Here's how I'd
> clean it up:
>
> static MachineClass *machine_parse(const char *name)
> {
>     MachineClass *mc;
>     GSList *el, *machines;
>
>     if (is_help_option(name)) {
>         machines = object_class_get_list(TYPE_MACHINE, false);
>
>         printf("Supported machines are:\n");
>         for (el = machines; el; el = el->next) {
>             MachineClass *mc = el->data;
>             QEMUMachine *m = mc->qemu_machine;
>             if (m->alias) {
>                 printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
>             }
>             printf("%-20s %s%s\n", m->name, m->desc,
>                    m->is_default ? " (default)" : "");
>         }
>
>         g_slist_free(machines);
>         exit(0);
>     }
>
>     mc = find_machine(name);
>     if (!mc) {
>         error_report("Unsupported machine type");
>         error_printf("Use '-machine help' to list supported machines\n");
>         exit(1);
>     }
>     return mc;
> }
>
>