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

[Qemu-devel] [PATCH V4] 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>
---

v3 -> v4:
   - removed double call of loc_push_none (thanks again Laszlo)

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 2c03f54..5e22a35 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)
@@ -4028,17 +4053,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 V4] vl.c: fixed regression in machine error message

[Laszlo Ersek]

On 02/08/16 12:54, 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>
> ---
> 
> v3 -> v4:
>    - removed double call of loc_push_none (thanks again Laszlo)
> 
> 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 2c03f54..5e22a35 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)
> @@ -4028,17 +4053,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);
>  
> 

Looks good, thanks!
Laszlo

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

[Eduardo Habkost]

On Mon, Feb 08, 2016 at 01:54:29PM +0200, 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>
> ---
> 
> v3 -> v4:
>    - removed double call of loc_push_none (thanks again Laszlo)
> 
> 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 2c03f54..5e22a35 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)
> @@ -4028,17 +4053,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);
[Extra context for reference:]
> 
>    loc_set_none();

You are fixing the machine error message, but not the root cause:

  $ ./x86_64-softmmu/qemu-system-x86_64 -m size= -vnc :0
  qemu-system-x86_64: -vnc :0: missing 'size' option value
  $ ./x86_64-softmmu/qemu-system-x86_64 -icount rr=x -vnc :0
  qemu-system-x86_64: -vnc :0: Invalid icount rr option: x

Moving the loc_set_none() call above replay_configure() should fix the bug in
the three cases.

Setting location in set_machine_options(), set_memory_options(), and
replay_configure() would be nice, too. But I would prefer to apply a one-line
fix first, and then consider improving the machine/memory/icount error
messages.

-- 
Eduardo

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

[Marcel Apfelbaum]

On 02/11/2016 08:31 PM, Eduardo Habkost wrote:
> On Mon, Feb 08, 2016 at 01:54:29PM +0200, 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>
>> ---
>>
>> v3 -> v4:
>>     - removed double call of loc_push_none (thanks again Laszlo)
>>
>> 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 2c03f54..5e22a35 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)
>> @@ -4028,17 +4053,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);
> [Extra context for reference:]
>>
>>     loc_set_none();
>
> You are fixing the machine error message, but not the root cause:

Hi,

I knew that it would not solve all the cases but
the machine error message is a regression while other
were there way before (as far as I know)

I was specifically looking to fix the machine error regression.

>
>    $ ./x86_64-softmmu/qemu-system-x86_64 -m size= -vnc :0
>    qemu-system-x86_64: -vnc :0: missing 'size' option value
>    $ ./x86_64-softmmu/qemu-system-x86_64 -icount rr=x -vnc :0
>    qemu-system-x86_64: -vnc :0: Invalid icount rr option: x
>
> Moving the loc_set_none() call above replay_configure() should fix the bug in
> the three cases.

I am really no expert in loc stuff, but it seamed to me it is placed
especially after all the options are parsed. Maybe we need another
call to loc_set_none before replay_configure instead of moving it?

If you think it should be moved, please go ahead :)
is an one liner and the idea was yours.

>
> Setting location in set_machine_options()

This is done in this patch.

, set_memory_options(), and
> replay_configure() would be nice, too.

I can do it on those functions too, if you agree to the way
is done in set_machine_options

  But I would prefer to apply a one-line
> fix first, and then consider improving the machine/memory/icount error
> messages.

Sure. (set_machine_options is introduced in this patch)

Thanks,
Marcel

>

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

[Eduardo Habkost]

On Thu, Feb 11, 2016 at 08:49:25PM +0200, Marcel Apfelbaum wrote:
> On 02/11/2016 08:31 PM, Eduardo Habkost wrote:
> >On Mon, Feb 08, 2016 at 01:54:29PM +0200, 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>
> >>---
> >>
> >>v3 -> v4:
> >>    - removed double call of loc_push_none (thanks again Laszlo)
> >>
> >>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 2c03f54..5e22a35 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)
> >>@@ -4028,17 +4053,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);
> >[Extra context for reference:]
> >>
> >>    loc_set_none();
> >
> >You are fixing the machine error message, but not the root cause:
> 
> Hi,
> 
> I knew that it would not solve all the cases but
> the machine error message is a regression while other
> were there way before (as far as I know)
> 
> I was specifically looking to fix the machine error regression.
> 
> >
> >   $ ./x86_64-softmmu/qemu-system-x86_64 -m size= -vnc :0
> >   qemu-system-x86_64: -vnc :0: missing 'size' option value
> >   $ ./x86_64-softmmu/qemu-system-x86_64 -icount rr=x -vnc :0
> >   qemu-system-x86_64: -vnc :0: Invalid icount rr option: x
> >
> >Moving the loc_set_none() call above replay_configure() should fix the bug in
> >the three cases.
> 
> I am really no expert in loc stuff, but it seamed to me it is placed
> especially after all the options are parsed. Maybe we need another
> call to loc_set_none before replay_configure instead of moving it?

It needs to be reset immediately after the option-parsing 'for'
loop, because the location info becomes obsolete as soon as we
exit the loop. All the remaining code is supposed to push/pop
location info if necessary, so we don't need another call.

> 
> If you think it should be moved, please go ahead :)
> is an one liner and the idea was yours.

I will do that.

> 
> >
> >Setting location in set_machine_options()
> 
> This is done in this patch.

Yes. And if we reorder my patch and yours, the conflicts look
trivial.

> 
> , set_memory_options(), and
> >replay_configure() would be nice, too.
> 
> I can do it on those functions too, if you agree to the way
> is done in set_machine_options

It looks good to me. I would like location setting to be done
automatically somehow, instead of requiring every option parser
to manually set location, but this can be figured out later.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo