From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54501) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPV5U-0005GP-QP for qemu-devel@nongnu.org; Mon, 17 Mar 2014 06:48:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WPV5M-0003OB-Cv for qemu-devel@nongnu.org; Mon, 17 Mar 2014 06:48:08 -0400 Received: from mail-ee0-x22a.google.com ([2a00:1450:4013:c00::22a]:44084) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPV5M-0003No-0Y for qemu-devel@nongnu.org; Mon, 17 Mar 2014 06:48:00 -0400 Received: by mail-ee0-f42.google.com with SMTP id d17so3945357eek.15 for ; Mon, 17 Mar 2014 03:47:59 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <5326D2DB.2050305@redhat.com> Date: Mon, 17 Mar 2014 11:47:55 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1394798814-21279-1-git-send-email-mrezanin@redhat.com> <87fvmhgsij.fsf@blackfin.pond.sub.org> In-Reply-To: <87fvmhgsij.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: Markus Armbruster , mrezanin@redhat.com Cc: peter.maydell@linaro.org, qemu-devel@nongnu.org, afaerber@suse.de Il 17/03/2014 09:04, Markus Armbruster ha scritto: > 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. 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; > } > >