* [Qemu-devel] [PATCH] vl.c: fixed regression of machine error message
@ 2016-02-07 16:24 Marcel Apfelbaum
2016-02-08 8:58 ` Laszlo Ersek
0 siblings, 1 reply; 3+ messages in thread
From: Marcel Apfelbaum @ 2016-02-07 16:24 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel, pbonzini, qemu-stable, armbru, ehabkost
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);
--
2.4.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] vl.c: fixed regression of machine error message
2016-02-07 16:24 [Qemu-devel] [PATCH] vl.c: fixed regression of machine error message Marcel Apfelbaum
@ 2016-02-08 8:58 ` Laszlo Ersek
2016-02-08 9:24 ` Marcel Apfelbaum
0 siblings, 1 reply; 3+ messages in thread
From: Laszlo Ersek @ 2016-02-08 8:58 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel; +Cc: pbonzini, qemu-stable, ehabkost, armbru
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] vl.c: fixed regression of machine error message
2016-02-08 8:58 ` Laszlo Ersek
@ 2016-02-08 9:24 ` Marcel Apfelbaum
0 siblings, 0 replies; 3+ messages in thread
From: Marcel Apfelbaum @ 2016-02-08 9:24 UTC (permalink / raw)
To: Laszlo Ersek, qemu-devel; +Cc: pbonzini, qemu-stable, ehabkost, armbru
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-02-08 9:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-07 16:24 [Qemu-devel] [PATCH] vl.c: fixed regression of machine error message Marcel Apfelbaum
2016-02-08 8:58 ` Laszlo Ersek
2016-02-08 9:24 ` Marcel Apfelbaum
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).