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 <address@hidden> 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 <address@hidden>
>> ---
>>  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 <address@hidden>

-- 
Wei Yang
Help you, Help me