From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:38968) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hC4rv-0004eq-LM for qemu-devel@nongnu.org; Thu, 04 Apr 2019 12:05:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hC4rt-0000UG-3P for qemu-devel@nongnu.org; Thu, 04 Apr 2019 12:05:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46788) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1hC4rs-0000Qz-On for qemu-devel@nongnu.org; Thu, 04 Apr 2019 12:05:33 -0400 From: Markus Armbruster References: <20190311060823.18360-1-richardw.yang@linux.intel.com> <20190402132650.23095-2-armbru@redhat.com> <20190403223255.GB18809@richard> Date: Thu, 04 Apr 2019 18:05:25 +0200 In-Reply-To: <20190403223255.GB18809@richard> (Wei Yang's message of "Thu, 4 Apr 2019 06:32:55 +0800") Message-ID: <875zrt69iy.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 4/4] vl: Simplify machine_parse() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wei Yang Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com Wei Yang writes: > On Tue, Apr 02, 2019 at 03:26:50PM +0200, Markus Armbruster wrote: >>Exploit that argument @name is nerver null. Check is_help_option() >>first, because that's what we do elsewhere. >> >>Signed-off-by: Markus Armbruster >>--- >> vl.c | 24 +++++++++++------------- >> 1 file changed, 11 insertions(+), 13 deletions(-) >> >>diff --git a/vl.c b/vl.c >>index 6a31e5bfac..da1af3e10d 100644 >>--- a/vl.c >>+++ b/vl.c >>@@ -2573,19 +2573,10 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b) >> >> static MachineClass *machine_parse(const char *name, GSList *machines) >> { >>- MachineClass *mc = NULL; >>+ MachineClass *mc; >> GSList *el; >> >>- if (name) { >>- mc = find_machine(name, machines); >>- } >>- if (mc) { >>- return mc; >>- } >>- if (name && !is_help_option(name)) { >>- error_report("unsupported machine type"); >>- error_printf("Use -machine help to list supported machines\n"); >>- } else { >>+ if (is_help_option(name)) { >> printf("Supported machines are:\n"); >> machines = g_slist_sort(machines, machine_class_cmp); >> for (el = machines; el; el = el->next) { >>@@ -2597,9 +2588,16 @@ static MachineClass *machine_parse(const char *name, GSList *machines) >> mc->is_default ? " (default)" : "", >> mc->deprecation_reason ? " (deprecated)" : ""); >> } >>+ exit(0); >> } >>- >>- exit(!name || !is_help_option(name)); >>+ >>+ mc = find_machine(name, machines); >>+ if (!mc) { >>+ error_report("unsupported machine type"); >>+ error_printf("Use -machine help to list supported machines\n"); >>+ exit(1); >>+ } >>+ return mc; > > This change looks changed the original behavior. > > In original logic, if mc is not NULL, there is no message printed. While now > it rely on is_help_option(). And no it exit when !is_help_option(), while > before this change it exit when is_help_option(). > > I don't understand the reason behind this. My suggestion is you may split this > patch into two: > > 1. remove check on name > 2. refine the logic with explanations. Cases: (1) User asks for help, i.e. is_help_option(name) (1a) and no machine named @name exists, i.e. is_help_option(name) && !find_machine(name, machines) (1b) and a machine named @name exists is_help_option(name) && find_machine(name, machines) (2) User asks for a machine that doesn't exist, i.e. !is_help_option(name) && !find_machine(name, machines) (3) User asks for a machine that exists, i.e. !is_help_option(name) && find_machine(name, machines) Since no machines are called "help" or "?", case (1b) is not actually possible. Old code: static MachineClass *machine_parse(const char *name, GSList *machines) { MachineClass *mc = NULL; GSList *el; if (name) { mc = find_machine(name, machines); } if (mc) { return mc; } if (name && !is_help_option(name)) { error_report("unsupported machine type"); error_printf("Use -machine help to list supported machines\n"); } else { printf("Supported machines are:\n"); machines = g_slist_sort(machines, machine_class_cmp); for (el = machines; el; el = el->next) { MachineClass *mc = el->data; if (mc->alias) { printf("%-20s %s (alias of %s)\n", mc->alias, mc->desc, mc->name); } printf("%-20s %s%s%s\n", mc->name, mc->desc, mc->is_default ? " (default)" : "", mc->deprecation_reason ? " (deprecated)" : ""); } } exit(!name || !is_help_option(name)); } Case (1a): print help, exit(0) Case (1b): return find_machine() Case (2): report error, exit(1) Case (3): return find_machine() New code: static MachineClass *machine_parse(const char *name, GSList *machines) { MachineClass *mc; GSList *el; if (is_help_option(name)) { printf("Supported machines are:\n"); machines = g_slist_sort(machines, machine_class_cmp); for (el = machines; el; el = el->next) { MachineClass *mc = el->data; if (mc->alias) { printf("%-20s %s (alias of %s)\n", mc->alias, mc->desc, mc->name); } printf("%-20s %s%s%s\n", mc->name, mc->desc, mc->is_default ? " (default)" : "", mc->deprecation_reason ? " (deprecated)" : ""); } exit(0); } mc = find_machine(name, machines); if (!mc) { error_report("unsupported machine type"); error_printf("Use -machine help to list supported machines\n"); exit(1); } return mc; } Case (1a): print help, exit(0) Case (1b): print help, exit(0) Case (2): report error, exit(1) Case (3): return find_machine() The patch changes "impossible" case (1b). That's intentional (but my commit message could explain it better).