[Qemu-devel] [PATCH 0/2] cleanup select_machine

[Qemu-devel] [PATCH 0/2] cleanup select_machine

[Wei Yang]

Here is two simple change related to select_machine()

[1]: make find_default_machine() local since no one outside file use it
[2]: allocate TYPE_MACHINE list only once to select machine type

Wei Yang (2):
  vl.c: make find_default_machine() local
  vl.c: allocate TYPE_MACHINE list once during bootup

 include/hw/boards.h |  1 -
 vl.c                | 24 +++++++++++-------------
 2 files changed, 11 insertions(+), 14 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH 1/2] vl.c: make find_default_machine() local

[Wei Yang]

Function find_default_machine() is introduced by commit 2c8cffa599b7
"vl: make find_default_machine externally visible", while it seems no
one outside use it.

This patch make it local again.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 include/hw/boards.h | 1 -
 vl.c                | 4 ++--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 21212f0859..e911d56c28 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -57,7 +57,6 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
 #define MACHINE_CLASS(klass) \
     OBJECT_CLASS_CHECK(MachineClass, (klass), TYPE_MACHINE)
 
-MachineClass *find_default_machine(void);
 extern MachineState *current_machine;
 
 void machine_run_board_init(MachineState *machine);
diff --git a/vl.c b/vl.c
index 502857a176..3688e2bc98 100644
--- a/vl.c
+++ b/vl.c
@@ -1441,7 +1441,7 @@ static MachineClass *find_machine(const char *name)
     return mc;
 }
 
-MachineClass *find_default_machine(void)
+static MachineClass *find_default_machine(void)
 {
     GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
     MachineClass *mc = NULL;
@@ -2538,7 +2538,7 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b)
                   object_class_get_name(OBJECT_CLASS(mc1)));
 }
 
- static MachineClass *machine_parse(const char *name)
+static MachineClass *machine_parse(const char *name)
 {
     MachineClass *mc = NULL;
     GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
-- 
2.19.1

Re: [Qemu-devel] [PATCH 1/2] vl.c: make find_default_machine() local

[Markus Armbruster]

Wei Yang <richardw.yang@linux.intel.com> writes:

> Function find_default_machine() is introduced by commit 2c8cffa599b7
> "vl: make find_default_machine externally visible", while it seems no
> one outside use it.
It was used outside of vl.c until commit a904410af5f.

> This patch make it local again.

Suggest:

  Commit a904410af5f removed the only user of find_default_machine()
  outside vl.c, but neglected to make it static.  Do that now.

> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  include/hw/boards.h | 1 -
>  vl.c                | 4 ++--
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 21212f0859..e911d56c28 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -57,7 +57,6 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
>  #define MACHINE_CLASS(klass) \
>      OBJECT_CLASS_CHECK(MachineClass, (klass), TYPE_MACHINE)
>  
> -MachineClass *find_default_machine(void);
>  extern MachineState *current_machine;
>  
>  void machine_run_board_init(MachineState *machine);
> diff --git a/vl.c b/vl.c
> index 502857a176..3688e2bc98 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1441,7 +1441,7 @@ static MachineClass *find_machine(const char *name)
>      return mc;
>  }
>  
> -MachineClass *find_default_machine(void)
> +static MachineClass *find_default_machine(void)
>  {
>      GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
>      MachineClass *mc = NULL;
> @@ -2538,7 +2538,7 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b)
>                    object_class_get_name(OBJECT_CLASS(mc1)));
>  }
>  
> - static MachineClass *machine_parse(const char *name)
> +static MachineClass *machine_parse(const char *name)
>  {
>      MachineClass *mc = NULL;
>      GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);

Not related to this patch's stated purpose.  Should go into PATCH 2,
where you're changing this line anyway.

Re: [Qemu-devel] [PATCH 1/2] vl.c: make find_default_machine() local

[Wei Yang]

On Tue, Apr 02, 2019 at 08:33:08AM +0200, Markus Armbruster wrote:
>Wei Yang <richardw.yang@linux.intel.com> writes:
>
>> Function find_default_machine() is introduced by commit 2c8cffa599b7
>> "vl: make find_default_machine externally visible", while it seems no
>> one outside use it.
>It was used outside of vl.c until commit a904410af5f.
>
>> This patch make it local again.
>
>Suggest:
>
>  Commit a904410af5f removed the only user of find_default_machine()
>  outside vl.c, but neglected to make it static.  Do that now.
>

Markus

Thanks for your comments.

BTW, I think I need to spin a v2, right?

>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  include/hw/boards.h | 1 -
>>  vl.c                | 4 ++--
>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 21212f0859..e911d56c28 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -57,7 +57,6 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
>>  #define MACHINE_CLASS(klass) \
>>      OBJECT_CLASS_CHECK(MachineClass, (klass), TYPE_MACHINE)
>>  
>> -MachineClass *find_default_machine(void);
>>  extern MachineState *current_machine;
>>  
>>  void machine_run_board_init(MachineState *machine);
>> diff --git a/vl.c b/vl.c
>> index 502857a176..3688e2bc98 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1441,7 +1441,7 @@ static MachineClass *find_machine(const char *name)
>>      return mc;
>>  }
>>  
>> -MachineClass *find_default_machine(void)
>> +static MachineClass *find_default_machine(void)
>>  {
>>      GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
>>      MachineClass *mc = NULL;
>> @@ -2538,7 +2538,7 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b)
>>                    object_class_get_name(OBJECT_CLASS(mc1)));
>>  }
>>  
>> - static MachineClass *machine_parse(const char *name)
>> +static MachineClass *machine_parse(const char *name)
>>  {
>>      MachineClass *mc = NULL;
>>      GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
>
>Not related to this patch's stated purpose.  Should go into PATCH 2,
>where you're changing this line anyway.

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 1/2] vl.c: make find_default_machine() local

[Markus Armbruster]

Wei Yang <richard.weiyang@gmail.com> writes:

> On Tue, Apr 02, 2019 at 08:33:08AM +0200, Markus Armbruster wrote:
>>Wei Yang <richardw.yang@linux.intel.com> writes:
>>
>>> Function find_default_machine() is introduced by commit 2c8cffa599b7
>>> "vl: make find_default_machine externally visible", while it seems no
>>> one outside use it.
>>It was used outside of vl.c until commit a904410af5f.
>>
>>> This patch make it local again.
>>
>>Suggest:
>>
>>  Commit a904410af5f removed the only user of find_default_machine()
>>  outside vl.c, but neglected to make it static.  Do that now.
>>
>
> Markus
>
> Thanks for your comments.
>
> BTW, I think I need to spin a v2, right?

A v2 with the review comments addressed and my two patches included
would help.

[Qemu-devel] [PATCH 2/2] vl.c: allocate TYPE_MACHINE list once during bootup

[Wei Yang]

Now all the functions used to select machine is local and the call flow
looks like below:

    select_machine()
        find_default_machine()
        machine_parse()
            find_machine()

All these related function will need a GSList for TYPE_MACHINE.
Currently we allocate this list each time we use it, while this is not
necessary to do so because we don't need to modify this.

This patch make the TYPE_MACHINE list allocation in select_machine and
pass this to its child for use.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 vl.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/vl.c b/vl.c
index 3688e2bc98..cf08d96ce4 100644
--- a/vl.c
+++ b/vl.c
@@ -1418,9 +1418,9 @@ static int usb_parse(const char *cmdline)
 
 MachineState *current_machine;
 
-static MachineClass *find_machine(const char *name)
+static MachineClass *find_machine(const char *name, GSList *machines)
 {
-    GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
+    GSList *el;
     MachineClass *mc = NULL;
 
     for (el = machines; el; el = el->next) {
@@ -1437,13 +1437,12 @@ static MachineClass *find_machine(const char *name)
         }
     }
 
-    g_slist_free(machines);
     return mc;
 }
 
-static MachineClass *find_default_machine(void)
+static MachineClass *find_default_machine(GSList *machines)
 {
-    GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
+    GSList *el;
     MachineClass *mc = NULL;
 
     for (el = machines; el; el = el->next) {
@@ -1455,7 +1454,6 @@ static MachineClass *find_default_machine(void)
         }
     }
 
-    g_slist_free(machines);
     return mc;
 }
 
@@ -2538,16 +2536,15 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b)
                   object_class_get_name(OBJECT_CLASS(mc1)));
 }
 
-static MachineClass *machine_parse(const char *name)
+static MachineClass *machine_parse(const char *name, GSList *machines)
 {
     MachineClass *mc = NULL;
-    GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
+    GSList *el;
 
     if (name) {
-        mc = find_machine(name);
+        mc = find_machine(name, machines);
     }
     if (mc) {
-        g_slist_free(machines);
         return mc;
     }
     if (name && !is_help_option(name)) {
@@ -2567,7 +2564,6 @@ static MachineClass *machine_parse(const char *name)
         }
     }
 
-    g_slist_free(machines);
     exit(!name || !is_help_option(name));
 }
 
@@ -2659,7 +2655,8 @@ static const QEMUOption *lookup_opt(int argc, char **argv,
 
 static MachineClass *select_machine(void)
 {
-    MachineClass *machine_class = find_default_machine();
+    GSList *machines = object_class_get_list(TYPE_MACHINE, false);
+    MachineClass *machine_class = find_default_machine(machines);
     const char *optarg;
     QemuOpts *opts;
     Location loc;
@@ -2671,7 +2668,7 @@ static MachineClass *select_machine(void)
 
     optarg = qemu_opt_get(opts, "type");
     if (optarg) {
-        machine_class = machine_parse(optarg);
+        machine_class = machine_parse(optarg, machines);
     }
 
     if (!machine_class) {
@@ -2681,6 +2678,7 @@ static MachineClass *select_machine(void)
     }
 
     loc_pop(&loc);
+    g_slist_free(machines);
     return machine_class;
 }
 
-- 
2.19.1

Re: [Qemu-devel] [PATCH 2/2] vl.c: allocate TYPE_MACHINE list once during bootup

[Markus Armbruster]

Wei Yang <richardw.yang@linux.intel.com> writes:

> Now all the functions used to select machine is local and the call flow
> looks like below:
>
>     select_machine()
>         find_default_machine()
>         machine_parse()
>             find_machine()
>
> All these related function will need a GSList for TYPE_MACHINE.
> Currently we allocate this list each time we use it, while this is not
> necessary to do so because we don't need to modify this.
>
> This patch make the TYPE_MACHINE list allocation in select_machine and
> pass this to its child for use.
>
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  vl.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 3688e2bc98..cf08d96ce4 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1418,9 +1418,9 @@ static int usb_parse(const char *cmdline)
>  
>  MachineState *current_machine;
>  
> -static MachineClass *find_machine(const char *name)
> +static MachineClass *find_machine(const char *name, GSList *machines)
>  {
> -    GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
> +    GSList *el;
>      MachineClass *mc = NULL;
>  
>      for (el = machines; el; el = el->next) {
> @@ -1437,13 +1437,12 @@ static MachineClass *find_machine(const char *name)
>          }
>      }
>  
> -    g_slist_free(machines);
>      return mc;
>  }

Can be simplified further.  I'll post it as PATCH 3.

>  
> -static MachineClass *find_default_machine(void)
> +static MachineClass *find_default_machine(GSList *machines)
>  {
> -    GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
> +    GSList *el;
>      MachineClass *mc = NULL;
>  
>      for (el = machines; el; el = el->next) {
> @@ -1455,7 +1454,6 @@ static MachineClass *find_default_machine(void)
>          }
>      }
>  
> -    g_slist_free(machines);
>      return mc;
>  }

Likewise.

>  
> @@ -2538,16 +2536,15 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b)
>                    object_class_get_name(OBJECT_CLASS(mc1)));
>  }
>  
> -static MachineClass *machine_parse(const char *name)
> +static MachineClass *machine_parse(const char *name, GSList *machines)
>  {
>      MachineClass *mc = NULL;
> -    GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
> +    GSList *el;
>  
>      if (name) {
> -        mc = find_machine(name);
> +        mc = find_machine(name, machines);
>      }
>      if (mc) {
> -        g_slist_free(machines);
>          return mc;
>      }
>      if (name && !is_help_option(name)) {
> @@ -2567,7 +2564,6 @@ static MachineClass *machine_parse(const char *name)
>          }
>      }
>  
> -    g_slist_free(machines);
>      exit(!name || !is_help_option(name));
>  }

Additional cleanup is possible: argument @name is never null.

While there, I'd check is_help_option() first rather than only after
find_machine() returns null, because "first" is what we commonly do.

I'll post this as PATCH 4.

>  
> @@ -2659,7 +2655,8 @@ static const QEMUOption *lookup_opt(int argc, char **argv,
>  
>  static MachineClass *select_machine(void)
>  {
> -    MachineClass *machine_class = find_default_machine();
> +    GSList *machines = object_class_get_list(TYPE_MACHINE, false);
> +    MachineClass *machine_class = find_default_machine(machines);
>      const char *optarg;
>      QemuOpts *opts;
>      Location loc;
> @@ -2671,7 +2668,7 @@ static MachineClass *select_machine(void)
>  
>      optarg = qemu_opt_get(opts, "type");
>      if (optarg) {
> -        machine_class = machine_parse(optarg);
> +        machine_class = machine_parse(optarg, machines);

Could create and destroy @machines here:

  -        machine_class = machine_parse(optarg);
  +        GSList *machines = object_class_get_list(TYPE_MACHINE, false);
  +        machine_class = machine_parse(optarg, machines);
  +        g_slist_free(machines);

Matter of taste.

>      }
>  
>      if (!machine_class) {
> @@ -2681,6 +2678,7 @@ static MachineClass *select_machine(void)
>      }
>  
>      loc_pop(&loc);
> +    g_slist_free(machines);
>      return machine_class;
>  }

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 2/2] vl.c: allocate TYPE_MACHINE list once during bootup

[Wei Yang]

On Tue, Apr 02, 2019 at 03:28:48PM +0200, Markus Armbruster wrote:
>Wei Yang <richardw.yang@linux.intel.com> writes:
>
>> Now all the functions used to select machine is local and the call flow
>> looks like below:
>>
>>     select_machine()
>>         find_default_machine()
>>         machine_parse()
>>             find_machine()
>>
>> All these related function will need a GSList for TYPE_MACHINE.
>> Currently we allocate this list each time we use it, while this is not
>> necessary to do so because we don't need to modify this.
>>
>> This patch make the TYPE_MACHINE list allocation in select_machine and
>> pass this to its child for use.
>>
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  vl.c | 24 +++++++++++-------------
>>  1 file changed, 11 insertions(+), 13 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 3688e2bc98..cf08d96ce4 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1418,9 +1418,9 @@ static int usb_parse(const char *cmdline)
>>  
>>  MachineState *current_machine;
>>  
>> -static MachineClass *find_machine(const char *name)
>> +static MachineClass *find_machine(const char *name, GSList *machines)
>>  {
>> -    GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
>> +    GSList *el;
>>      MachineClass *mc = NULL;
>>  
>>      for (el = machines; el; el = el->next) {
>> @@ -1437,13 +1437,12 @@ static MachineClass *find_machine(const char *name)
>>          }
>>      }
>>  
>> -    g_slist_free(machines);
>>      return mc;
>>  }
>
>Can be simplified further.  I'll post it as PATCH 3.
>

Markus

Thanks for your comment. Do you sugest to simplify it in this patch? I
am confused here.

>>  
>> -static MachineClass *find_default_machine(void)
>> +static MachineClass *find_default_machine(GSList *machines)
>>  {
>> -    GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
>> +    GSList *el;
>>      MachineClass *mc = NULL;
>>  
>>      for (el = machines; el; el = el->next) {
>> @@ -1455,7 +1454,6 @@ static MachineClass *find_default_machine(void)
>>          }
>>      }
>>  
>> -    g_slist_free(machines);
>>      return mc;
>>  }
>
>Likewise.
>
>>  
>> @@ -2538,16 +2536,15 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b)
>>                    object_class_get_name(OBJECT_CLASS(mc1)));
>>  }
>>  
>> -static MachineClass *machine_parse(const char *name)
>> +static MachineClass *machine_parse(const char *name, GSList *machines)
>>  {
>>      MachineClass *mc = NULL;
>> -    GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
>> +    GSList *el;
>>  
>>      if (name) {
>> -        mc = find_machine(name);
>> +        mc = find_machine(name, machines);
>>      }
>>      if (mc) {
>> -        g_slist_free(machines);
>>          return mc;
>>      }
>>      if (name && !is_help_option(name)) {
>> @@ -2567,7 +2564,6 @@ static MachineClass *machine_parse(const char *name)
>>          }
>>      }
>>  
>> -    g_slist_free(machines);
>>      exit(!name || !is_help_option(name));
>>  }
>
>Additional cleanup is possible: argument @name is never null.
>
>While there, I'd check is_help_option() first rather than only after
>find_machine() returns null, because "first" is what we commonly do.
>
>I'll post this as PATCH 4.
>
>>  
>> @@ -2659,7 +2655,8 @@ static const QEMUOption *lookup_opt(int argc, char **argv,
>>  
>>  static MachineClass *select_machine(void)
>>  {
>> -    MachineClass *machine_class = find_default_machine();
>> +    GSList *machines = object_class_get_list(TYPE_MACHINE, false);
>> +    MachineClass *machine_class = find_default_machine(machines);
>>      const char *optarg;
>>      QemuOpts *opts;
>>      Location loc;
>> @@ -2671,7 +2668,7 @@ static MachineClass *select_machine(void)
>>  
>>      optarg = qemu_opt_get(opts, "type");
>>      if (optarg) {
>> -        machine_class = machine_parse(optarg);
>> +        machine_class = machine_parse(optarg, machines);
>
>Could create and destroy @machines here:
>
>  -        machine_class = machine_parse(optarg);
>  +        GSList *machines = object_class_get_list(TYPE_MACHINE, false);
>  +        machine_class = machine_parse(optarg, machines);
>  +        g_slist_free(machines);
>
>Matter of taste.
>
>>      }
>>  
>>      if (!machine_class) {
>> @@ -2681,6 +2678,7 @@ static MachineClass *select_machine(void)
>>      }
>>  
>>      loc_pop(&loc);
>> +    g_slist_free(machines);
>>      return machine_class;
>>  }
>
>Reviewed-by: Markus Armbruster <armbru@redhat.com>

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 2/2] vl.c: allocate TYPE_MACHINE list once during bootup

[Markus Armbruster]

Wei Yang <richard.weiyang@gmail.com> writes:

> On Tue, Apr 02, 2019 at 03:28:48PM +0200, Markus Armbruster wrote:
>>Wei Yang <richardw.yang@linux.intel.com> writes:
>>
>>> Now all the functions used to select machine is local and the call flow
>>> looks like below:
>>>
>>>     select_machine()
>>>         find_default_machine()
>>>         machine_parse()
>>>             find_machine()
>>>
>>> All these related function will need a GSList for TYPE_MACHINE.
>>> Currently we allocate this list each time we use it, while this is not
>>> necessary to do so because we don't need to modify this.
>>>
>>> This patch make the TYPE_MACHINE list allocation in select_machine and
>>> pass this to its child for use.
>>>
>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>> ---
>>>  vl.c | 24 +++++++++++-------------
>>>  1 file changed, 11 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/vl.c b/vl.c
>>> index 3688e2bc98..cf08d96ce4 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -1418,9 +1418,9 @@ static int usb_parse(const char *cmdline)
>>>  
>>>  MachineState *current_machine;
>>>  
>>> -static MachineClass *find_machine(const char *name)
>>> +static MachineClass *find_machine(const char *name, GSList *machines)
>>>  {
>>> -    GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
>>> +    GSList *el;
>>>      MachineClass *mc = NULL;
>>>  
>>>      for (el = machines; el; el = el->next) {
>>> @@ -1437,13 +1437,12 @@ static MachineClass *find_machine(const char *name)
>>>          }
>>>      }
>>>  
>>> -    g_slist_free(machines);
>>>      return mc;
>>>  }
>>
>>Can be simplified further.  I'll post it as PATCH 3.
>>
>
> Markus
>
> Thanks for your comment. Do you sugest to simplify it in this patch? I
> am confused here.

I feel the additional simplifcations I posted as PATCH 3 and 4 are best
kept as separate patches.

Remark [*] below is the only one that's not addressed by my PATCH 3+4.

>>> -static MachineClass *find_default_machine(void)
>>> +static MachineClass *find_default_machine(GSList *machines)
>>>  {
>>> -    GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
>>> +    GSList *el;
>>>      MachineClass *mc = NULL;
>>>  
>>>      for (el = machines; el; el = el->next) {
>>> @@ -1455,7 +1454,6 @@ static MachineClass *find_default_machine(void)
>>>          }
>>>      }
>>>  
>>> -    g_slist_free(machines);
>>>      return mc;
>>>  }
>>
>>Likewise.
>>
>>>  
>>> @@ -2538,16 +2536,15 @@ static gint machine_class_cmp(gconstpointer a, gconstpointer b)
>>>                    object_class_get_name(OBJECT_CLASS(mc1)));
>>>  }
>>>  
>>> -static MachineClass *machine_parse(const char *name)
>>> +static MachineClass *machine_parse(const char *name, GSList *machines)
>>>  {
>>>      MachineClass *mc = NULL;
>>> -    GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
>>> +    GSList *el;
>>>  
>>>      if (name) {
>>> -        mc = find_machine(name);
>>> +        mc = find_machine(name, machines);
>>>      }
>>>      if (mc) {
>>> -        g_slist_free(machines);
>>>          return mc;
>>>      }
>>>      if (name && !is_help_option(name)) {
>>> @@ -2567,7 +2564,6 @@ static MachineClass *machine_parse(const char *name)
>>>          }
>>>      }
>>>  
>>> -    g_slist_free(machines);
>>>      exit(!name || !is_help_option(name));
>>>  }
>>
>>Additional cleanup is possible: argument @name is never null.
>>
>>While there, I'd check is_help_option() first rather than only after
>>find_machine() returns null, because "first" is what we commonly do.
>>
>>I'll post this as PATCH 4.
>>
>>>  
>>> @@ -2659,7 +2655,8 @@ static const QEMUOption *lookup_opt(int argc, char **argv,
>>>  
>>>  static MachineClass *select_machine(void)
>>>  {
>>> -    MachineClass *machine_class = find_default_machine();
>>> +    GSList *machines = object_class_get_list(TYPE_MACHINE, false);
>>> +    MachineClass *machine_class = find_default_machine(machines);
>>>      const char *optarg;
>>>      QemuOpts *opts;
>>>      Location loc;
>>> @@ -2671,7 +2668,7 @@ static MachineClass *select_machine(void)
>>>  
>>>      optarg = qemu_opt_get(opts, "type");
>>>      if (optarg) {
>>> -        machine_class = machine_parse(optarg);
>>> +        machine_class = machine_parse(optarg, machines);
>>
>>Could create and destroy @machines here:
>>
>>  -        machine_class = machine_parse(optarg);
>>  +        GSList *machines = object_class_get_list(TYPE_MACHINE, false);
>>  +        machine_class = machine_parse(optarg, machines);
>>  +        g_slist_free(machines);
>>
>>Matter of taste.

[*]

Matter of taste means the choice between your version and mine is up to
the maintainer, or if the maintainer remains silent, up to you.

>>>      }
>>>  
>>>      if (!machine_class) {
>>> @@ -2681,6 +2678,7 @@ static MachineClass *select_machine(void)
>>>      }
>>>  
>>>      loc_pop(&loc);
>>> +    g_slist_free(machines);
>>>      return machine_class;
>>>  }
>>
>>Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 2/2] vl.c: allocate TYPE_MACHINE list once during bootup

[Wei Yang]

On Tue, Apr 02, 2019 at 06:10:23PM +0200, Markus Armbruster wrote:
>Wei Yang <richard.weiyang@gmail.com> writes:

[...]

>>>>  
>>>> @@ -2659,7 +2655,8 @@ static const QEMUOption *lookup_opt(int argc, char **argv,
>>>>  
>>>>  static MachineClass *select_machine(void)
>>>>  {
>>>> -    MachineClass *machine_class = find_default_machine();
>>>> +    GSList *machines = object_class_get_list(TYPE_MACHINE, false);
>>>> +    MachineClass *machine_class = find_default_machine(machines);
>>>>      const char *optarg;
>>>>      QemuOpts *opts;
>>>>      Location loc;
>>>> @@ -2671,7 +2668,7 @@ static MachineClass *select_machine(void)
>>>>  
>>>>      optarg = qemu_opt_get(opts, "type");
>>>>      if (optarg) {
>>>> -        machine_class = machine_parse(optarg);
>>>> +        machine_class = machine_parse(optarg, machines);
>>>
>>>Could create and destroy @machines here:
>>>
>>>  -        machine_class = machine_parse(optarg);
>>>  +        GSList *machines = object_class_get_list(TYPE_MACHINE, false);
>>>  +        machine_class = machine_parse(optarg, machines);
>>>  +        g_slist_free(machines);
>>>
>>>Matter of taste.
>
>[*]
>
>Matter of taste means the choice between your version and mine is up to
>the maintainer, or if the maintainer remains silent, up to you.
>

Ok, I get your meaning.

But machines should be used in find_default_machine(), after move the
allocation in "if", would there be a problem?

I may not understand your point here.

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 2/2] vl.c: allocate TYPE_MACHINE list once during bootup

[Markus Armbruster]

Wei Yang <richardw.yang@linux.intel.com> writes:

> On Tue, Apr 02, 2019 at 06:10:23PM +0200, Markus Armbruster wrote:
>>Wei Yang <richard.weiyang@gmail.com> writes:
>
> [...]
>
>>>>>  
>>>>> @@ -2659,7 +2655,8 @@ static const QEMUOption *lookup_opt(int argc, char **argv,
>>>>>  
>>>>>  static MachineClass *select_machine(void)
>>>>>  {
>>>>> -    MachineClass *machine_class = find_default_machine();
>>>>> +    GSList *machines = object_class_get_list(TYPE_MACHINE, false);
>>>>> +    MachineClass *machine_class = find_default_machine(machines);
>>>>>      const char *optarg;
>>>>>      QemuOpts *opts;
>>>>>      Location loc;
>>>>> @@ -2671,7 +2668,7 @@ static MachineClass *select_machine(void)
>>>>>  
>>>>>      optarg = qemu_opt_get(opts, "type");
>>>>>      if (optarg) {
>>>>> -        machine_class = machine_parse(optarg);
>>>>> +        machine_class = machine_parse(optarg, machines);
>>>>
>>>>Could create and destroy @machines here:
>>>>
>>>>  -        machine_class = machine_parse(optarg);
>>>>  +        GSList *machines = object_class_get_list(TYPE_MACHINE, false);
>>>>  +        machine_class = machine_parse(optarg, machines);
>>>>  +        g_slist_free(machines);
>>>>
>>>>Matter of taste.
>>
>>[*]
>>
>>Matter of taste means the choice between your version and mine is up to
>>the maintainer, or if the maintainer remains silent, up to you.
>>
>
> Ok, I get your meaning.
>
> But machines should be used in find_default_machine(), after move the
> allocation in "if", would there be a problem?
>
> I may not understand your point here.

You're right, I overlooked that use of @machines.  Keep your patch as it
is.

[Qemu-devel] [PATCH 3/4] vl: Clean up after previous commit

[Markus Armbruster]

Since the previous commit, find_machine() and find_default_machine()
don't have to deallocate on return.  This permits further
simplifications.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 vl.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/vl.c b/vl.c
index 126081f595..6a31e5bfac 100644
--- a/vl.c
+++ b/vl.c
@@ -1467,40 +1467,29 @@ MachineState *current_machine;
 static MachineClass *find_machine(const char *name, GSList *machines)
 {
     GSList *el;
-    MachineClass *mc = NULL;
 
     for (el = machines; el; el = el->next) {
-        MachineClass *temp = el->data;
+        MachineClass *mc = el->data;
 
-        if (!strcmp(temp->name, name)) {
-            mc = temp;
-            break;
-        }
-        if (temp->alias &&
-            !strcmp(temp->alias, name)) {
-            mc = temp;
-            break;
+        if (!strcmp(mc->name, name) || !g_strcmp0(mc->alias, name)) {
+            return mc;
         }
     }
 
-    return mc;
+    return NULL;
 }
 
 static MachineClass *find_default_machine(GSList *machines)
 {
     GSList *el;
-    MachineClass *mc = NULL;
 
     for (el = machines; el; el = el->next) {
-        MachineClass *temp = el->data;
-
-        if (temp->is_default) {
-            mc = temp;
-            break;
+        if (((MachineClass *)el->data)->is_default) {
+            return el->data;
         }
     }
 
-    return mc;
+    return NULL;
 }
 
 MachineInfoList *qmp_query_machines(Error **errp)
-- 
2.17.2

Re: [Qemu-devel] [PATCH 3/4] vl: Clean up after previous commit

[Wei Yang]

On Tue, Apr 02, 2019 at 03:26:49PM +0200, Markus Armbruster wrote:
>Since the previous commit, find_machine() and find_default_machine()
>don't have to deallocate on return.  This permits further
>simplifications.
>
>Signed-off-by: Markus Armbruster <armbru@redhat.com>
>---
> vl.c | 25 +++++++------------------
> 1 file changed, 7 insertions(+), 18 deletions(-)
>
>diff --git a/vl.c b/vl.c
>index 126081f595..6a31e5bfac 100644
>--- a/vl.c
>+++ b/vl.c
>@@ -1467,40 +1467,29 @@ MachineState *current_machine;
> static MachineClass *find_machine(const char *name, GSList *machines)
> {
>     GSList *el;
>-    MachineClass *mc = NULL;
> 
>     for (el = machines; el; el = el->next) {
>-        MachineClass *temp = el->data;
>+        MachineClass *mc = el->data;
> 
>-        if (!strcmp(temp->name, name)) {
>-            mc = temp;
>-            break;
>-        }
>-        if (temp->alias &&
>-            !strcmp(temp->alias, name)) {
>-            mc = temp;
>-            break;
>+        if (!strcmp(mc->name, name) || !g_strcmp0(mc->alias, name)) {
>+            return mc;
>         }
>     }
> 
>-    return mc;
>+    return NULL;
> }
> 
> static MachineClass *find_default_machine(GSList *machines)
> {
>     GSList *el;
>-    MachineClass *mc = NULL;
> 
>     for (el = machines; el; el = el->next) {
>-        MachineClass *temp = el->data;
>-
>-        if (temp->is_default) {
>-            mc = temp;
>-            break;
>+        if (((MachineClass *)el->data)->is_default) {
>+            return el->data;
>         }
>     }

Generally it looks good to me.

One tiny suggestion here is to define 

        MachineClass *mc = el->data;

just as it does in find_machin() and return mc instead of raw el->data.

If you agree with that I will modify this at my place.

> 
>-    return mc;
>+    return NULL;
> }
> 
> MachineInfoList *qmp_query_machines(Error **errp)
>-- 
>2.17.2

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 3/4] vl: Clean up after previous commit

[Markus Armbruster]

Wei Yang <richardw.yang@linux.intel.com> writes:

> On Tue, Apr 02, 2019 at 03:26:49PM +0200, Markus Armbruster wrote:
>>Since the previous commit, find_machine() and find_default_machine()
>>don't have to deallocate on return.  This permits further
>>simplifications.
>>
>>Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>---
>> vl.c | 25 +++++++------------------
>> 1 file changed, 7 insertions(+), 18 deletions(-)
>>
>>diff --git a/vl.c b/vl.c
>>index 126081f595..6a31e5bfac 100644
>>--- a/vl.c
>>+++ b/vl.c
>>@@ -1467,40 +1467,29 @@ MachineState *current_machine;
>> static MachineClass *find_machine(const char *name, GSList *machines)
>> {
>>     GSList *el;
>>-    MachineClass *mc = NULL;
>> 
>>     for (el = machines; el; el = el->next) {
>>-        MachineClass *temp = el->data;
>>+        MachineClass *mc = el->data;
>> 
>>-        if (!strcmp(temp->name, name)) {
>>-            mc = temp;
>>-            break;
>>-        }
>>-        if (temp->alias &&
>>-            !strcmp(temp->alias, name)) {
>>-            mc = temp;
>>-            break;
>>+        if (!strcmp(mc->name, name) || !g_strcmp0(mc->alias, name)) {
>>+            return mc;
>>         }
>>     }
>> 
>>-    return mc;
>>+    return NULL;
>> }
>> 
>> static MachineClass *find_default_machine(GSList *machines)
>> {
>>     GSList *el;
>>-    MachineClass *mc = NULL;
>> 
>>     for (el = machines; el; el = el->next) {
>>-        MachineClass *temp = el->data;
>>-
>>-        if (temp->is_default) {
>>-            mc = temp;
>>-            break;
>>+        if (((MachineClass *)el->data)->is_default) {
>>+            return el->data;
>>         }
>>     }
>
> Generally it looks good to me.
>
> One tiny suggestion here is to define 
>
>         MachineClass *mc = el->data;
>
> just as it does in find_machin() and return mc instead of raw el->data.
>
> If you agree with that I will modify this at my place.

No objection.

>> 
>>-    return mc;
>>+    return NULL;
>> }
>> 
>> MachineInfoList *qmp_query_machines(Error **errp)
>>-- 
>>2.17.2

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

[Markus Armbruster]

Exploit that argument @name is nerver null.  Check is_help_option()
first, because that's what we do elsewhere.

Signed-off-by: Markus Armbruster <armbru@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;
 }
 
 void qemu_add_exit_notifier(Notifier *notify)
-- 
2.17.2

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

[Wei Yang]

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

> }
> 
> void qemu_add_exit_notifier(Notifier *notify)
>-- 
>2.17.2

-- 
Wei Yang
Help you, Help me

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

[Markus Armbruster]

Wei Yang <richardw.yang@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 <armbru@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).

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 <richardw.yang@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 <armbru@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

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 <richardw.yang@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 <armbru@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

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

[Markus Armbruster]

Wei Yang <richardw.yang@linux.intel.com> writes:

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

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.