Re: [Qemu-devel] [PATCH 4/4] vl: Simplify machine_parse()

[Markus Armbruster]

Wei Yang <address@hidden> writes:

> On Thu, Apr 04, 2019 at 06:05:25PM +0200, Markus Armbruster wrote:
>>Wei Yang <address@hidden> 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 <address@hidden>
>>>>---
>>>> 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.