[Qemu-devel] [PATCH V3] vl.c: fixed regression in machine error message

[Qemu-devel] [PATCH V3] vl.c: fixed regression in machine error message

[Marcel Apfelbaum]

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>
---

v2 -> v3:
 - fixed commit message and called qemu_get_machine_opts only once. (thanks Laszlo)

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 now 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

Thanks,
Marcel

 vl.c | 38 +++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/vl.c b/vl.c
index c581e39..5873248 100644
--- a/vl.c
+++ b/vl.c
@@ -2748,6 +2748,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(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)
@@ -4028,17 +4054,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);
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH V3] vl.c: fixed regression in machine error message

[Laszlo Ersek]

sorry, but:

On 02/08/16 12:19, 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 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>
> ---
> 
> v2 -> v3:
>  - fixed commit message and called qemu_get_machine_opts only once. (thanks Laszlo)
> 
> 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 now 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
> 
> Thanks,
> Marcel
> 
>  vl.c | 38 +++++++++++++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index c581e39..5873248 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2748,6 +2748,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(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);
> +}

You now have two pushes and one pop only.

I don't know why I didn't notice this in v2. It's Monday. Sorry.

Laszlo

> +
>  static int machine_set_property(void *opaque,
>                                  const char *name, const char *value,
>                                  Error **errp)
> @@ -4028,17 +4054,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);
>  
> 

Re: [Qemu-devel] [PATCH V3] vl.c: fixed regression in machine error message

[Marcel Apfelbaum]

On 02/08/2016 01:41 PM, Laszlo Ersek wrote:
> sorry, but:
>
> On 02/08/16 12:19, 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 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>
>> ---
>>
>> v2 -> v3:
>>   - fixed commit message and called qemu_get_machine_opts only once. (thanks Laszlo)
>>
>> 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 now 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
>>
>> Thanks,
>> Marcel
>>
>>   vl.c | 38 +++++++++++++++++++++++++++-----------
>>   1 file changed, 27 insertions(+), 11 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index c581e39..5873248 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2748,6 +2748,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(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);
>> +}
>
> You now have two pushes and one pop only.
>
> I don't know why I didn't notice this in v2. It's Monday. Sorry.

yep, definitely not my day. I was looking for a quick fix. I'll send another one...

Thanks,
Marcel

>
> Laszlo
>
>> +
>>   static int machine_set_property(void *opaque,
>>                                   const char *name, const char *value,
>>                                   Error **errp)
>> @@ -4028,17 +4054,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);
>>
>>
>