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

[Markus Armbruster]

address@hidden writes:

> From: Miroslav Rezanina <address@hidden>
>
> 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 <address@hidden>
> ---
> 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;
}