[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/4] vl: Simplify machine_parse()
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 4/4] vl: Simplify machine_parse() |
Date: |
Fri, 05 Apr 2019 07:39:25 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
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.
Re: [Qemu-devel] [PATCH 3/4] vl: Clean up after previous commit, Wei Yang, 2019/04/03