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

[Markus Armbruster]

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