| Message ID | 1454926170-6915-1-git-send-email-marcel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/08/16 11:09, 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. > > v1 -> v2: > - Addressed Laszlo Ersek's comments: > - no need to save the machine options location, is saved in opts > - rename the extracted method to set_machine_options > - added the bug reporter to the CC > > - tested with and without the config file and the error message is no OK: --> "is *now* OK". The typo ("no OK") should be please fixed up by the maintainer (unless another round is necessary). > config file: > - qemu-system-x86_64:machine-bug.conf:3: unsupported machine type > cli: > - qemu-system-x86_64: -M q35-1.5: unsupported machine type > > Reported-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > vl.c | 38 +++++++++++++++++++++++++++----------- > 1 file changed, 27 insertions(+), 11 deletions(-) > > diff --git a/vl.c b/vl.c > index f043009..dd29807 100644 > --- a/vl.c > +++ b/vl.c > @@ -2751,6 +2751,32 @@ 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(); > + loc_push_none(&loc); > + qemu_opts_loc_restore(opts); > + > + optarg = qemu_opt_get(qemu_get_machine_opts(), "type"); Not particularly important, but you could just pass "opts" as first arg here, rather than calling qemu_get_machine_opts() again. For style reasons, can you clean it up? With that: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo > + 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) > @@ -4019,17 +4045,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); > >
On 02/08/2016 12:28 PM, Laszlo Ersek wrote: > On 02/08/16 11:09, 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. >> >> v1 -> v2: >> - Addressed Laszlo Ersek's comments: >> - no need to save the machine options location, is saved in opts >> - rename the extracted method to set_machine_options >> - added the bug reporter to the CC >> >> - tested with and without the config file and the error message is no OK: > > --> "is *now* OK". The typo ("no OK") should be please fixed up by the > maintainer (unless another round is necessary). Oh, v1->v2 section should be under -- line, I'll resend > >> config file: >> - qemu-system-x86_64:machine-bug.conf:3: unsupported machine type >> cli: >> - qemu-system-x86_64: -M q35-1.5: unsupported machine type >> >> Reported-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >> --- >> vl.c | 38 +++++++++++++++++++++++++++----------- >> 1 file changed, 27 insertions(+), 11 deletions(-) >> >> diff --git a/vl.c b/vl.c >> index f043009..dd29807 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -2751,6 +2751,32 @@ 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(); >> + loc_push_none(&loc); >> + qemu_opts_loc_restore(opts); >> + >> + optarg = qemu_opt_get(qemu_get_machine_opts(), "type"); > > Not particularly important, but you could just pass "opts" as first arg > here, rather than calling qemu_get_machine_opts() again. For style > reasons, can you clean it up? I would have nothing against it, but how can we be sure that opts poins to machine opts at the time set_machine_options is called? Or I am missing something. > > With that: > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks! Waiting for your comment to the above and I'll post again. Marcel > > Thanks > Laszlo > >> + 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) >> @@ -4019,17 +4045,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); >> >> >
On 02/08/16 11:46, Marcel Apfelbaum wrote: > On 02/08/2016 12:28 PM, Laszlo Ersek wrote: >> On 02/08/16 11:09, 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. >>> >>> v1 -> v2: >>> - Addressed Laszlo Ersek's comments: >>> - no need to save the machine options location, is saved in opts >>> - rename the extracted method to set_machine_options >>> - added the bug reporter to the CC >>> >>> - tested with and without the config file and the error message is >>> no OK: >> >> --> "is *now* OK". The typo ("no OK") should be please fixed up by the >> maintainer (unless another round is necessary). > > Oh, v1->v2 section should be under -- line, I'll resend > >> >>> config file: >>> - qemu-system-x86_64:machine-bug.conf:3: unsupported machine type >>> cli: >>> - qemu-system-x86_64: -M q35-1.5: unsupported machine type >>> >>> Reported-by: Michael S. Tsirkin <mst@redhat.com> >>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >>> --- >>> vl.c | 38 +++++++++++++++++++++++++++----------- >>> 1 file changed, 27 insertions(+), 11 deletions(-) >>> >>> diff --git a/vl.c b/vl.c >>> index f043009..dd29807 100644 >>> --- a/vl.c >>> +++ b/vl.c >>> @@ -2751,6 +2751,32 @@ 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(); >>> + loc_push_none(&loc); >>> + qemu_opts_loc_restore(opts); >>> + >>> + optarg = qemu_opt_get(qemu_get_machine_opts(), "type"); >> >> Not particularly important, but you could just pass "opts" as first arg >> here, rather than calling qemu_get_machine_opts() again. For style >> reasons, can you clean it up? > > I would have nothing against it, but how can we be sure that opts poins > to machine opts > at the time set_machine_options is called? Or I am missing something. Please search this new function in your text editor for occurrences of "qemu_get_machine_opts", and you will see. :) Thanks Laszlo > >> >> With that: >> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > Thanks! Waiting for your comment to the above and I'll post again. > Marcel > >> >> Thanks >> Laszlo >> >>> + 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) >>> @@ -4019,17 +4045,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); >>> >>> >> >
On 02/08/2016 01:02 PM, Laszlo Ersek wrote: > On 02/08/16 11:46, Marcel Apfelbaum wrote: >> On 02/08/2016 12:28 PM, Laszlo Ersek wrote: >>> On 02/08/16 11:09, 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. >>>> >>>> v1 -> v2: >>>> - Addressed Laszlo Ersek's comments: >>>> - no need to save the machine options location, is saved in opts >>>> - rename the extracted method to set_machine_options >>>> - added the bug reporter to the CC >>>> >>>> - tested with and without the config file and the error message is >>>> no OK: >>> >>> --> "is *now* OK". The typo ("no OK") should be please fixed up by the >>> maintainer (unless another round is necessary). >> >> Oh, v1->v2 section should be under -- line, I'll resend >> >>> >>>> config file: >>>> - qemu-system-x86_64:machine-bug.conf:3: unsupported machine type >>>> cli: >>>> - qemu-system-x86_64: -M q35-1.5: unsupported machine type >>>> >>>> Reported-by: Michael S. Tsirkin <mst@redhat.com> >>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >>>> --- >>>> vl.c | 38 +++++++++++++++++++++++++++----------- >>>> 1 file changed, 27 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/vl.c b/vl.c >>>> index f043009..dd29807 100644 >>>> --- a/vl.c >>>> +++ b/vl.c >>>> @@ -2751,6 +2751,32 @@ 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(); >>>> + loc_push_none(&loc); >>>> + qemu_opts_loc_restore(opts); >>>> + >>>> + optarg = qemu_opt_get(qemu_get_machine_opts(), "type"); >>> >>> Not particularly important, but you could just pass "opts" as first arg >>> here, rather than calling qemu_get_machine_opts() again. For style >>> reasons, can you clean it up? >> >> I would have nothing against it, but how can we be sure that opts poins >> to machine opts >> at the time set_machine_options is called? Or I am missing something. > > Please search this new function in your text editor for occurrences of > "qemu_get_machine_opts", and you will see. :) Ooops... I was thinking you wanted me to pass "opts" as parameter to set_machine_options. Anyway, it wasn't for nothing, I saw that machine_opts is assigned twice, I'll send a patch shortly :) Finally got it, thanks Marcel > Thanksargument > Laszlo > >> >>> >>> With that: >>> >>> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >> >> Thanks! Waiting for your comment to the above and I'll post again. >> Marcel >> >>> >>> Thanks >>> Laszlo >>> >>>> + 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) >>>> @@ -4019,17 +4045,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); >>>> >>>> >>> >> >
diff --git a/vl.c b/vl.c index f043009..dd29807 100644 --- a/vl.c +++ b/vl.c @@ -2751,6 +2751,32 @@ 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(); + loc_push_none(&loc); + qemu_opts_loc_restore(opts); + + 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(&loc); +} + static int machine_set_property(void *opaque, const char *name, const char *value, Error **errp) @@ -4019,17 +4045,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);
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. v1 -> v2: - Addressed Laszlo Ersek's comments: - no need to save the machine options location, is saved in opts - rename the extracted method to set_machine_options - added the bug reporter to the CC - tested with and without the config file and the error message is no OK: config file: - qemu-system-x86_64:machine-bug.conf:3: unsupported machine type cli: - qemu-system-x86_64: -M q35-1.5: unsupported machine type Reported-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> --- vl.c | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-)