| Message ID | 1454862254-9089-1-git-send-email-marcel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/07/16 17:24, Marcel Apfelbaum wrote: > 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 saving the error location and also extracted the code > dealing with machine options into a separate function. > > Reported-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > > Hi, > > I hope I used "location" correctly, if anyone is aware of a simpler > approach please advice. > > Thanks, > Marcel > > vl.c | 34 +++++++++++++++++++++++----------- > 1 file changed, 23 insertions(+), 11 deletions(-) > > diff --git a/vl.c b/vl.c > index f043009..f8a9213 100644 > --- a/vl.c > +++ b/vl.c > @@ -2751,6 +2751,26 @@ static const QEMUOption *lookup_opt(int argc, char **argv, > return popt; > } > > +static void machine_set_options(Location *machine_opt_loc, > + MachineClass **machine_class) > +{ > + const char *optarg; > + Location *curr = loc_push_restore(machine_opt_loc); > + > + optarg = qemu_opt_get(qemu_get_machine_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(curr); > +} > + > static int machine_set_property(void *opaque, > const char *name, const char *value, > Error **errp) > @@ -2975,6 +2995,7 @@ int main(int argc, char **argv, char **envp) > const char *optarg; > const char *loadvm = NULL; > MachineClass *machine_class; > + Location machine_opt_loc = {0}; > const char *cpu_model; > const char *vga_model = NULL; > const char *qtest_chrdev = NULL; > @@ -3671,6 +3692,7 @@ int main(int argc, char **argv, char **envp) > break; > case QEMU_OPTION_M: > case QEMU_OPTION_machine: > + loc_save(&machine_opt_loc); > olist = qemu_find_opts("machine"); > opts = qemu_opts_parse_noisily(olist, optarg, true); > if (!opts) { > @@ -4019,17 +4041,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); > - } > + machine_set_options(&machine_opt_loc, &machine_class); > > set_memory_options(&ram_slots, &maxram_size, machine_class); > > Sorry, I sent my email before noticing yours (in the original thread, "Re: [Qemu-devel] command line error handling broken?"). Looks good (extracting machine_set_options()), but I think you don't need to save the location for the option separately; the location gets saved in the option automatically. It can be restored with qemu_opts_loc_restore(), however it doesn't combine push & restore, so a none location should be pushed first. Also, "set_machine_options" would match "set_memory_options" better, just below. Thanks Laszlo
On 02/08/2016 10:58 AM, Laszlo Ersek wrote: > On 02/07/16 17:24, Marcel Apfelbaum wrote: >> 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 saving the error location and also extracted the code >> dealing with machine options into a separate function. >> >> Reported-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >> --- >> >> Hi, >> >> I hope I used "location" correctly, if anyone is aware of a simpler >> approach please advice. >> >> Thanks, >> Marcel >> >> vl.c | 34 +++++++++++++++++++++++----------- >> 1 file changed, 23 insertions(+), 11 deletions(-) >> >> diff --git a/vl.c b/vl.c >> index f043009..f8a9213 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -2751,6 +2751,26 @@ static const QEMUOption *lookup_opt(int argc, char **argv, >> return popt; >> } >> >> +static void machine_set_options(Location *machine_opt_loc, >> + MachineClass **machine_class) >> +{ >> + const char *optarg; >> + Location *curr = loc_push_restore(machine_opt_loc); >> + >> + optarg = qemu_opt_get(qemu_get_machine_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(curr); >> +} >> + >> static int machine_set_property(void *opaque, >> const char *name, const char *value, >> Error **errp) >> @@ -2975,6 +2995,7 @@ int main(int argc, char **argv, char **envp) >> const char *optarg; >> const char *loadvm = NULL; >> MachineClass *machine_class; >> + Location machine_opt_loc = {0}; >> const char *cpu_model; >> const char *vga_model = NULL; >> const char *qtest_chrdev = NULL; >> @@ -3671,6 +3692,7 @@ int main(int argc, char **argv, char **envp) >> break; >> case QEMU_OPTION_M: >> case QEMU_OPTION_machine: >> + loc_save(&machine_opt_loc); >> olist = qemu_find_opts("machine"); >> opts = qemu_opts_parse_noisily(olist, optarg, true); >> if (!opts) { >> @@ -4019,17 +4041,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); >> - } >> + machine_set_options(&machine_opt_loc, &machine_class); >> >> set_memory_options(&ram_slots, &maxram_size, machine_class); >> >> > > Sorry, I sent my email before noticing yours (in the original thread, > "Re: [Qemu-devel] command line error handling broken?"). > > Looks good (extracting machine_set_options()), but I think you don't > need to save the location for the option separately; the location gets > saved in the option automatically. It can be restored with > qemu_opts_loc_restore(), however it doesn't combine push & restore, so a > none location should be pushed first. > > Also, "set_machine_options" would match "set_memory_options" better, > just below. Hi Laszlo, Thanks for the review. I'll post an updated patch shortly. Thanks, Marcel > > Thanks > Laszlo >
diff --git a/vl.c b/vl.c index f043009..f8a9213 100644 --- a/vl.c +++ b/vl.c @@ -2751,6 +2751,26 @@ static const QEMUOption *lookup_opt(int argc, char **argv, return popt; } +static void machine_set_options(Location *machine_opt_loc, + MachineClass **machine_class) +{ + const char *optarg; + Location *curr = loc_push_restore(machine_opt_loc); + + optarg = qemu_opt_get(qemu_get_machine_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(curr); +} + static int machine_set_property(void *opaque, const char *name, const char *value, Error **errp) @@ -2975,6 +2995,7 @@ int main(int argc, char **argv, char **envp) const char *optarg; const char *loadvm = NULL; MachineClass *machine_class; + Location machine_opt_loc = {0}; const char *cpu_model; const char *vga_model = NULL; const char *qtest_chrdev = NULL; @@ -3671,6 +3692,7 @@ int main(int argc, char **argv, char **envp) break; case QEMU_OPTION_M: case QEMU_OPTION_machine: + loc_save(&machine_opt_loc); olist = qemu_find_opts("machine"); opts = qemu_opts_parse_noisily(olist, optarg, true); if (!opts) { @@ -4019,17 +4041,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); - } + machine_set_options(&machine_opt_loc, &machine_class); set_memory_options(&ram_slots, &maxram_size, machine_class);
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 saving the error location and also extracted the code dealing with machine options into a separate function. Reported-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> --- Hi, I hope I used "location" correctly, if anyone is aware of a simpler approach please advice. Thanks, Marcel vl.c | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-)