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

[Wei Yang]

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. 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