| Message ID | 1455303747-19776-2-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eduardo Habkost <ehabkost@redhat.com> writes: > From: Marcel Apfelbaum <marcel@redhat.com> > > Commit e1ce0c3cb (vl.c: fix regression when reading machine type > from config file) fixed the error message when the machine type > was supplied inside the config file. However now the option name > is not displayed correctly if the error happens when the machine > is specified at command line. > > Running > ./x86_64-softmmu/qemu-system-x86_64 -M q35-1.5 -redir tcp:8022::22 > will result in the error message: > qemu-system-x86_64: -redir tcp:8022::22: unsupported machine type > Use -machine help to list supported machines > > Fixed it by restoring the error location and also extracted the code > dealing with machine options into a separate function. > > Reported-by: Michael S. Tsirkin <mst@redhat.com> > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > vl.c | 37 ++++++++++++++++++++++++++----------- > 1 file changed, 26 insertions(+), 11 deletions(-) > > diff --git a/vl.c b/vl.c > index 175ebcc..afbf13f 100644 > --- a/vl.c > +++ b/vl.c > @@ -2748,6 +2748,31 @@ static const QEMUOption *lookup_opt(int argc, char **argv, > return popt; > } > > +static void set_machine_options(MachineClass **machine_class) > +{ > + const char *optarg; > + QemuOpts *opts; > + Location loc; > + > + loc_push_none(&loc); > + > + opts = qemu_get_machine_opts(); > + qemu_opts_loc_restore(opts); > + > + optarg = qemu_opt_get(opts, "type"); > + if (optarg) { > + *machine_class = machine_parse(optarg); > + } > + > + if (*machine_class == NULL) { > + error_report("No machine specified, and there is no default"); > + error_printf("Use -machine help to list supported machines\n"); > + exit(1); > + } > + > + loc_pop(&loc); > +} > + > static int machine_set_property(void *opaque, > const char *name, const char *value, > Error **errp) > @@ -4030,17 +4055,7 @@ int main(int argc, char **argv, char **envp) > > replay_configure(icount_opts); > > - opts = qemu_get_machine_opts(); > - optarg = qemu_opt_get(opts, "type"); > - if (optarg) { > - machine_class = machine_parse(optarg); > - } > - > - if (machine_class == NULL) { > - error_report("No machine specified, and there is no default"); > - error_printf("Use -machine help to list supported machines\n"); > - exit(1); > - } > + set_machine_options(&machine_class); Style nit: I'd prefer machine_class = parse_machine_options(); Can convert to that when I apply to error-next. > > set_memory_options(&ram_slots, &maxram_size, machine_class);
On 02/15/2016 12:20 PM, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > >> From: Marcel Apfelbaum <marcel@redhat.com> >> >> Commit e1ce0c3cb (vl.c: fix regression when reading machine type >> from config file) fixed the error message when the machine type >> was supplied inside the config file. However now the option name >> is not displayed correctly if the error happens when the machine >> is specified at command line. >> >> Running >> ./x86_64-softmmu/qemu-system-x86_64 -M q35-1.5 -redir tcp:8022::22 >> will result in the error message: >> qemu-system-x86_64: -redir tcp:8022::22: unsupported machine type >> Use -machine help to list supported machines >> >> Fixed it by restoring the error location and also extracted the code >> dealing with machine options into a separate function. >> >> Reported-by: Michael S. Tsirkin <mst@redhat.com> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >> --- >> vl.c | 37 ++++++++++++++++++++++++++----------- >> 1 file changed, 26 insertions(+), 11 deletions(-) >> >> diff --git a/vl.c b/vl.c >> index 175ebcc..afbf13f 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -2748,6 +2748,31 @@ static const QEMUOption *lookup_opt(int argc, char **argv, >> return popt; >> } >> >> +static void set_machine_options(MachineClass **machine_class) >> +{ >> + const char *optarg; >> + QemuOpts *opts; >> + Location loc; >> + >> + loc_push_none(&loc); >> + >> + opts = qemu_get_machine_opts(); >> + qemu_opts_loc_restore(opts); >> + >> + optarg = qemu_opt_get(opts, "type"); >> + if (optarg) { >> + *machine_class = machine_parse(optarg); >> + } >> + >> + if (*machine_class == NULL) { >> + error_report("No machine specified, and there is no default"); >> + error_printf("Use -machine help to list supported machines\n"); >> + exit(1); >> + } >> + >> + loc_pop(&loc); >> +} >> + >> static int machine_set_property(void *opaque, >> const char *name, const char *value, >> Error **errp) >> @@ -4030,17 +4055,7 @@ int main(int argc, char **argv, char **envp) >> >> replay_configure(icount_opts); >> >> - opts = qemu_get_machine_opts(); >> - optarg = qemu_opt_get(opts, "type"); >> - if (optarg) { >> - machine_class = machine_parse(optarg); >> - } >> - >> - if (machine_class == NULL) { >> - error_report("No machine specified, and there is no default"); >> - error_printf("Use -machine help to list supported machines\n"); >> - exit(1); >> - } >> + set_machine_options(&machine_class); > > Style nit: I'd prefer > > machine_class = parse_machine_options(); > > Can convert to that when I apply to error-next. Hi Markus, Sure, please do that. It was suggested by Laszlo - I think - to match "set_memory_options" below. We can add a trivial patch to rename it too, of course. Thanks, Marcel > >> >> set_memory_options(&ram_slots, &maxram_size, machine_class);
On 02/15/16 11:54, Marcel Apfelbaum wrote: > On 02/15/2016 12:20 PM, Markus Armbruster wrote: >> Eduardo Habkost <ehabkost@redhat.com> writes: >> >>> From: Marcel Apfelbaum <marcel@redhat.com> >>> >>> Commit e1ce0c3cb (vl.c: fix regression when reading machine type >>> from config file) fixed the error message when the machine type >>> was supplied inside the config file. However now the option name >>> is not displayed correctly if the error happens when the machine >>> is specified at command line. >>> >>> Running >>> ./x86_64-softmmu/qemu-system-x86_64 -M q35-1.5 -redir tcp:8022::22 >>> will result in the error message: >>> qemu-system-x86_64: -redir tcp:8022::22: unsupported machine type >>> Use -machine help to list supported machines >>> >>> Fixed it by restoring the error location and also extracted the code >>> dealing with machine options into a separate function. >>> >>> Reported-by: Michael S. Tsirkin <mst@redhat.com> >>> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >>> --- >>> vl.c | 37 ++++++++++++++++++++++++++----------- >>> 1 file changed, 26 insertions(+), 11 deletions(-) >>> >>> diff --git a/vl.c b/vl.c >>> index 175ebcc..afbf13f 100644 >>> --- a/vl.c >>> +++ b/vl.c >>> @@ -2748,6 +2748,31 @@ static const QEMUOption *lookup_opt(int argc, >>> char **argv, >>> return popt; >>> } >>> >>> +static void set_machine_options(MachineClass **machine_class) >>> +{ >>> + const char *optarg; >>> + QemuOpts *opts; >>> + Location loc; >>> + >>> + loc_push_none(&loc); >>> + >>> + opts = qemu_get_machine_opts(); >>> + qemu_opts_loc_restore(opts); >>> + >>> + optarg = qemu_opt_get(opts, "type"); >>> + if (optarg) { >>> + *machine_class = machine_parse(optarg); >>> + } >>> + >>> + if (*machine_class == NULL) { >>> + error_report("No machine specified, and there is no default"); >>> + error_printf("Use -machine help to list supported machines\n"); >>> + exit(1); >>> + } >>> + >>> + loc_pop(&loc); >>> +} >>> + >>> static int machine_set_property(void *opaque, >>> const char *name, const char *value, >>> Error **errp) >>> @@ -4030,17 +4055,7 @@ int main(int argc, char **argv, char **envp) >>> >>> replay_configure(icount_opts); >>> >>> - opts = qemu_get_machine_opts(); >>> - optarg = qemu_opt_get(opts, "type"); >>> - if (optarg) { >>> - machine_class = machine_parse(optarg); >>> - } >>> - >>> - if (machine_class == NULL) { >>> - error_report("No machine specified, and there is no default"); >>> - error_printf("Use -machine help to list supported machines\n"); >>> - exit(1); >>> - } >>> + set_machine_options(&machine_class); >> >> Style nit: I'd prefer >> >> machine_class = parse_machine_options(); >> >> Can convert to that when I apply to error-next. > > Hi Markus, > > Sure, please do that. > It was suggested by Laszlo - I think - to match "set_memory_options" below. Yes, I suggested that. I do defer to Markus though. Thanks Laszlo > We can add a trivial patch to rename it too, of course. > > Thanks, > Marcel > >> >>> >>> set_memory_options(&ram_slots, &maxram_size, machine_class); >
Marcel Apfelbaum <marcel@redhat.com> writes: > On 02/15/2016 12:20 PM, Markus Armbruster wrote: >> Eduardo Habkost <ehabkost@redhat.com> writes: >> >>> From: Marcel Apfelbaum <marcel@redhat.com> >>> >>> Commit e1ce0c3cb (vl.c: fix regression when reading machine type >>> from config file) fixed the error message when the machine type >>> was supplied inside the config file. However now the option name >>> is not displayed correctly if the error happens when the machine >>> is specified at command line. >>> >>> Running >>> ./x86_64-softmmu/qemu-system-x86_64 -M q35-1.5 -redir tcp:8022::22 >>> will result in the error message: >>> qemu-system-x86_64: -redir tcp:8022::22: unsupported machine type >>> Use -machine help to list supported machines >>> >>> Fixed it by restoring the error location and also extracted the code >>> dealing with machine options into a separate function. >>> >>> Reported-by: Michael S. Tsirkin <mst@redhat.com> >>> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >>> --- >>> vl.c | 37 ++++++++++++++++++++++++++----------- >>> 1 file changed, 26 insertions(+), 11 deletions(-) >>> >>> diff --git a/vl.c b/vl.c >>> index 175ebcc..afbf13f 100644 >>> --- a/vl.c >>> +++ b/vl.c >>> @@ -2748,6 +2748,31 @@ static const QEMUOption *lookup_opt(int argc, char **argv, >>> return popt; >>> } >>> >>> +static void set_machine_options(MachineClass **machine_class) >>> +{ >>> + const char *optarg; >>> + QemuOpts *opts; >>> + Location loc; >>> + >>> + loc_push_none(&loc); >>> + >>> + opts = qemu_get_machine_opts(); >>> + qemu_opts_loc_restore(opts); >>> + >>> + optarg = qemu_opt_get(opts, "type"); >>> + if (optarg) { >>> + *machine_class = machine_parse(optarg); >>> + } >>> + >>> + if (*machine_class == NULL) { >>> + error_report("No machine specified, and there is no default"); >>> + error_printf("Use -machine help to list supported machines\n"); >>> + exit(1); >>> + } >>> + >>> + loc_pop(&loc); >>> +} >>> + >>> static int machine_set_property(void *opaque, >>> const char *name, const char *value, >>> Error **errp) >>> @@ -4030,17 +4055,7 @@ int main(int argc, char **argv, char **envp) >>> >>> replay_configure(icount_opts); >>> >>> - opts = qemu_get_machine_opts(); >>> - optarg = qemu_opt_get(opts, "type"); >>> - if (optarg) { >>> - machine_class = machine_parse(optarg); >>> - } >>> - >>> - if (machine_class == NULL) { >>> - error_report("No machine specified, and there is no default"); >>> - error_printf("Use -machine help to list supported machines\n"); >>> - exit(1); >>> - } >>> + set_machine_options(&machine_class); >> >> Style nit: I'd prefer >> >> machine_class = parse_machine_options(); >> >> Can convert to that when I apply to error-next. > > Hi Markus, > > Sure, please do that. > It was suggested by Laszlo - I think - to match "set_memory_options" below. Ah, didn't see that one. set_memory_options() is that way, because it needs to return two values, which is always awkward in C. While I value consistency, I doubt there's enough of a pattern here to justify returning a single result through a pointer parameter instead of the function value. > We can add a trivial patch to rename it too, of course. > > Thanks, > Marcel > >> >>> >>> set_memory_options(&ram_slots, &maxram_size, machine_class);
Markus Armbruster <armbru@redhat.com> writes: > Marcel Apfelbaum <marcel@redhat.com> writes: > >> On 02/15/2016 12:20 PM, Markus Armbruster wrote: >>> Eduardo Habkost <ehabkost@redhat.com> writes: >>> >>>> From: Marcel Apfelbaum <marcel@redhat.com> >>>> >>>> Commit e1ce0c3cb (vl.c: fix regression when reading machine type >>>> from config file) fixed the error message when the machine type >>>> was supplied inside the config file. However now the option name >>>> is not displayed correctly if the error happens when the machine >>>> is specified at command line. >>>> >>>> Running >>>> ./x86_64-softmmu/qemu-system-x86_64 -M q35-1.5 -redir tcp:8022::22 >>>> will result in the error message: >>>> qemu-system-x86_64: -redir tcp:8022::22: unsupported machine type >>>> Use -machine help to list supported machines >>>> >>>> Fixed it by restoring the error location and also extracted the code >>>> dealing with machine options into a separate function. >>>> >>>> Reported-by: Michael S. Tsirkin <mst@redhat.com> >>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> >>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >>>> --- >>>> vl.c | 37 ++++++++++++++++++++++++++----------- >>>> 1 file changed, 26 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/vl.c b/vl.c >>>> index 175ebcc..afbf13f 100644 >>>> --- a/vl.c >>>> +++ b/vl.c >>>> @@ -2748,6 +2748,31 @@ static const QEMUOption *lookup_opt(int argc, char **argv, >>>> return popt; >>>> } >>>> >>>> +static void set_machine_options(MachineClass **machine_class) >>>> +{ >>>> + const char *optarg; >>>> + QemuOpts *opts; >>>> + Location loc; >>>> + >>>> + loc_push_none(&loc); >>>> + >>>> + opts = qemu_get_machine_opts(); >>>> + qemu_opts_loc_restore(opts); >>>> + >>>> + optarg = qemu_opt_get(opts, "type"); >>>> + if (optarg) { >>>> + *machine_class = machine_parse(optarg); >>>> + } >>>> + >>>> + if (*machine_class == NULL) { >>>> + error_report("No machine specified, and there is no default"); >>>> + error_printf("Use -machine help to list supported machines\n"); >>>> + exit(1); >>>> + } >>>> + >>>> + loc_pop(&loc); >>>> +} >>>> + >>>> static int machine_set_property(void *opaque, >>>> const char *name, const char *value, >>>> Error **errp) >>>> @@ -4030,17 +4055,7 @@ int main(int argc, char **argv, char **envp) >>>> >>>> replay_configure(icount_opts); >>>> >>>> - opts = qemu_get_machine_opts(); >>>> - optarg = qemu_opt_get(opts, "type"); >>>> - if (optarg) { >>>> - machine_class = machine_parse(optarg); >>>> - } >>>> - >>>> - if (machine_class == NULL) { >>>> - error_report("No machine specified, and there is no default"); >>>> - error_printf("Use -machine help to list supported machines\n"); >>>> - exit(1); >>>> - } >>>> + set_machine_options(&machine_class); >>> >>> Style nit: I'd prefer >>> >>> machine_class = parse_machine_options(); Uh, set_machine_options() also reads machine_class, so this won't do. Better: machine_class = pick_machine(find_default_machine()); but that's a bit more than what I'm willing to do on commit. I'll post a follow-up patch. [...]
diff --git a/vl.c b/vl.c index 175ebcc..afbf13f 100644 --- a/vl.c +++ b/vl.c @@ -2748,6 +2748,31 @@ static const QEMUOption *lookup_opt(int argc, char **argv, return popt; } +static void set_machine_options(MachineClass **machine_class) +{ + const char *optarg; + QemuOpts *opts; + Location loc; + + loc_push_none(&loc); + + opts = qemu_get_machine_opts(); + qemu_opts_loc_restore(opts); + + optarg = qemu_opt_get(opts, "type"); + if (optarg) { + *machine_class = machine_parse(optarg); + } + + if (*machine_class == NULL) { + error_report("No machine specified, and there is no default"); + error_printf("Use -machine help to list supported machines\n"); + exit(1); + } + + loc_pop(&loc); +} + static int machine_set_property(void *opaque, const char *name, const char *value, Error **errp) @@ -4030,17 +4055,7 @@ int main(int argc, char **argv, char **envp) replay_configure(icount_opts); - opts = qemu_get_machine_opts(); - optarg = qemu_opt_get(opts, "type"); - if (optarg) { - machine_class = machine_parse(optarg); - } - - if (machine_class == NULL) { - error_report("No machine specified, and there is no default"); - error_printf("Use -machine help to list supported machines\n"); - exit(1); - } + set_machine_options(&machine_class); set_memory_options(&ram_slots, &maxram_size, machine_class);