1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH 0/2] cleanup select_machine
  4. View patch

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

Wei Yang posted 2 patches 6 years, 8 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Eduardo Habkost <ehabkost@redhat.com>
There is a newer version of this series
[Qemu-devel] [PATCH 2/2] vl.c: allocate TYPE_MACHINE list once during bootup
Posted by Wei Yang 6 years, 8 months ago 
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
Posted by Markus Armbruster 6 years, 7 months ago 
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
Posted by Wei Yang 6 years, 7 months ago 
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
Posted by Markus Armbruster 6 years, 7 months ago 
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
Posted by Wei Yang 6 years, 7 months ago 
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
Posted by Markus Armbruster 6 years, 7 months ago 
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.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.