| Message ID | 20190402132650.23095-2-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cleanup select_machine | expand |
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 <armbru@redhat.com> >--- > 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. > } > > void qemu_add_exit_notifier(Notifier *notify) >-- >2.17.2
Wei Yang <richardw.yang@linux.intel.com> 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 <armbru@redhat.com> >>--- >> 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).
On Thu, Apr 04, 2019 at 06:05:25PM +0200, Markus Armbruster wrote: >Wei Yang <richardw.yang@linux.intel.com> 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 <armbru@redhat.com> >>>--- >>> 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). This looks better. Would you mind refine it so that I could send all these patches in v2. Or you prefer send it out by our self?
Wei Yang <richardw.yang@linux.intel.com> writes: > On Thu, Apr 04, 2019 at 06:05:25PM +0200, Markus Armbruster wrote: >>Wei Yang <richardw.yang@linux.intel.com> 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 <armbru@redhat.com> >>>>--- >>>> 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). > > This looks better. Amending my commit message to explain things better: vl: Simplify machine_parse() Exploit that argument @name is nerver null. Check is_help_option() first, because that's what we do elsewhere. If we (foolishly!) defined a machine named "help", -machine help would now print help instead of selecting the machine named "help". > Would you mind refine it so that I could send all these > patches in v2. > > Or you prefer send it out by our self? Please send v2.
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; } void qemu_add_exit_notifier(Notifier *notify)
Exploit that argument @name is nerver null. Check is_help_option() first, because that's what we do elsewhere. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- vl.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)