From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57470) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPSXh-0006E9-Cn for qemu-devel@nongnu.org; Mon, 17 Mar 2014 04:05:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WPSXa-00070e-NX for qemu-devel@nongnu.org; Mon, 17 Mar 2014 04:05:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9508) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPSXa-00070L-FL for qemu-devel@nongnu.org; Mon, 17 Mar 2014 04:04:58 -0400 From: Markus Armbruster References: <1394798814-21279-1-git-send-email-mrezanin@redhat.com> Date: Mon, 17 Mar 2014 09:04:52 +0100 In-Reply-To: <1394798814-21279-1-git-send-email-mrezanin@redhat.com> (mrezanin@redhat.com's message of "Fri, 14 Mar 2014 13:06:54 +0100") Message-ID: <87fvmhgsij.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v7] vl.c: Output error on invalid machine type List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: mrezanin@redhat.com Cc: peter.maydell@linaro.org, qemu-devel@nongnu.org, afaerber@suse.de mrezanin@redhat.com writes: > From: Miroslav Rezanina > > 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 > --- > 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; }