| Message ID | 1394798814-21279-1-git-send-email-mrezanin@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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; }
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; > } > >
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);