| Message ID | 20190311060823.18360-3-richardw.yang@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | cleanup select_machine | expand |
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>
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 <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>
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 <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.
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; }
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(-)