On Thu, Apr 04, 2019 at 06:05:25PM +0200, Markus Armbruster wrote:
>Wei Yang <richardw.y...@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 <arm...@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
Help you, Help me
