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

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

[mrezanin]

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");
+    } 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);
-- 
1.8.5.3

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1217 bytes --]

On 03/14/2014 06:06 AM, mrezanin@redhat.com wrote:
> 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(-)
> 

> +        error_report("Unsupported machine type");
> +        printf("\nUse -machine help to list supported machines!\n");

I might have used "Use '-machine help' to" to offset what the user is
supposed to type, but I can live with what you have.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Markus Armbruster]

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.

> +    } 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;
}

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

[Paolo Bonzini]

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;
> }
>
>