[Qemu-devel] [PATCH v2 0/4] Error location reporting fixes

[Qemu-devel] [PATCH v2 0/4] Error location reporting fixes

[Eduardo Habkost]

This fixes the following bugs in error reporting:

  $ qemu-system-x86_64 -icount rr=x -vnc :0
  qemu-system-x86_64: -vnc :0: Invalid icount rr option: x

  $ qemu-system-x86_64 -m size= -vnc :0
  qemu-system-x86_64: -vnc :0: missing 'size' option value

The last command-line option (-vnc) is being shown in the error
message, instead of the -m or -icount options.

This also includes a patch submitted previously by Marcel, to
ensure there are no ordering conflicts when applying the patches.
Marcel's patch fixes the following bug:

  $ qemu-system-x86_64 -M q35-1.5 -redir tcp:8022::22
  qemu-system-x86_64: -redir tcp:8022::22: unsupported machine type
  Use -machine help to list supported machines

Eduardo Habkost (3):
  vl: Reset location after handling command-line arguments
  replay: Set error location properly when parsing options
  vl: Set error location when parsing memory options

Marcel Apfelbaum (1):
  vl.c: Fix regression in machine error message

 replay/replay.c | 10 ++++++++++
 vl.c            | 47 ++++++++++++++++++++++++++++++++++-------------
 2 files changed, 44 insertions(+), 13 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v2 1/4] vl.c: Fix regression in machine error message

[Eduardo Habkost]

From: Marcel Apfelbaum <marcel@redhat.com>

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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 vl.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/vl.c b/vl.c
index 175ebcc..afbf13f 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)
@@ -4030,17 +4055,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.1.0

Re: [Qemu-devel] [PATCH v2 1/4] vl.c: Fix regression in machine error message

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> From: Marcel Apfelbaum <marcel@redhat.com>
>
> 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>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  vl.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 175ebcc..afbf13f 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)
> @@ -4030,17 +4055,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);

Style nit: I'd prefer

    machine_class = parse_machine_options();

Can convert to that when I apply to error-next.

>  
>      set_memory_options(&ram_slots, &maxram_size, machine_class);

Re: [Qemu-devel] [PATCH v2 1/4] vl.c: Fix regression in machine error message

[Marcel Apfelbaum]

On 02/15/2016 12:20 PM, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
>
>> From: Marcel Apfelbaum <marcel@redhat.com>
>>
>> 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>
>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>>   vl.c | 37 ++++++++++++++++++++++++++-----------
>>   1 file changed, 26 insertions(+), 11 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 175ebcc..afbf13f 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)
>> @@ -4030,17 +4055,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);
>
> Style nit: I'd prefer
>
>      machine_class = parse_machine_options();
>
> Can convert to that when I apply to error-next.

Hi Markus,

Sure, please do that.
It was suggested by Laszlo - I think - to match "set_memory_options" below.
We can add a trivial patch to rename it too, of course.

Thanks,
Marcel

>
>>
>>       set_memory_options(&ram_slots, &maxram_size, machine_class);

Re: [Qemu-devel] [PATCH v2 1/4] vl.c: Fix regression in machine error message

[Laszlo Ersek]

On 02/15/16 11:54, Marcel Apfelbaum wrote:
> On 02/15/2016 12:20 PM, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>
>>> From: Marcel Apfelbaum <marcel@redhat.com>
>>>
>>> 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>
>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>> ---
>>>   vl.c | 37 ++++++++++++++++++++++++++-----------
>>>   1 file changed, 26 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/vl.c b/vl.c
>>> index 175ebcc..afbf13f 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)
>>> @@ -4030,17 +4055,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);
>>
>> Style nit: I'd prefer
>>
>>      machine_class = parse_machine_options();
>>
>> Can convert to that when I apply to error-next.
> 
> Hi Markus,
> 
> Sure, please do that.
> It was suggested by Laszlo - I think - to match "set_memory_options" below.

Yes, I suggested that.

I do defer to Markus though.

Thanks
Laszlo

> We can add a trivial patch to rename it too, of course.
> 
> Thanks,
> Marcel
> 
>>
>>>
>>>       set_memory_options(&ram_slots, &maxram_size, machine_class);
> 

Re: [Qemu-devel] [PATCH v2 1/4] vl.c: Fix regression in machine error message

[Markus Armbruster]

Marcel Apfelbaum <marcel@redhat.com> writes:

> On 02/15/2016 12:20 PM, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>
>>> From: Marcel Apfelbaum <marcel@redhat.com>
>>>
>>> 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>
>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>> ---
>>>   vl.c | 37 ++++++++++++++++++++++++++-----------
>>>   1 file changed, 26 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/vl.c b/vl.c
>>> index 175ebcc..afbf13f 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)
>>> @@ -4030,17 +4055,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);
>>
>> Style nit: I'd prefer
>>
>>      machine_class = parse_machine_options();
>>
>> Can convert to that when I apply to error-next.
>
> Hi Markus,
>
> Sure, please do that.
> It was suggested by Laszlo - I think - to match "set_memory_options" below.

Ah, didn't see that one.  set_memory_options() is that way, because it
needs to return two values, which is always awkward in C.

While I value consistency, I doubt there's enough of a pattern here to
justify returning a single result through a pointer parameter instead of
the function value.

> We can add a trivial patch to rename it too, of course.
>
> Thanks,
> Marcel
>
>>
>>>
>>>       set_memory_options(&ram_slots, &maxram_size, machine_class);

Re: [Qemu-devel] [PATCH v2 1/4] vl.c: Fix regression in machine error message

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Marcel Apfelbaum <marcel@redhat.com> writes:
>
>> On 02/15/2016 12:20 PM, Markus Armbruster wrote:
>>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>>
>>>> From: Marcel Apfelbaum <marcel@redhat.com>
>>>>
>>>> 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>
>>>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>>> ---
>>>>   vl.c | 37 ++++++++++++++++++++++++++-----------
>>>>   1 file changed, 26 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/vl.c b/vl.c
>>>> index 175ebcc..afbf13f 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)
>>>> @@ -4030,17 +4055,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);
>>>
>>> Style nit: I'd prefer
>>>
>>>      machine_class = parse_machine_options();

Uh, set_machine_options() also reads machine_class, so this won't do.

Better:

         machine_class = pick_machine(find_default_machine());

but that's a bit more than what I'm willing to do on commit.  I'll post
a follow-up patch.

[...]

[Qemu-devel] [PATCH v2 2/4] vl: Reset location after handling command-line arguments

[Eduardo Habkost]

After looping through all command-line arguments, error location
info becomes obsolete, and any function calling error_report()
will print misleading information. This breaks error reporting
for some option handling, like:

  $ qemu-system-x86_64 -icount rr=x -vnc :0
  qemu-system-x86_64: -vnc :0: Invalid icount rr option: x

  $ qemu-system-x86_64 -m size= -vnc :0
  qemu-system-x86_64: -vnc :0: missing 'size' option value

Fix this by resetting location info as soon as we exit the
command-line handling loop.

With this, replay_configure() and set_memory_options() won't
print any location info yet, but at least they won't print
incorrect information.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 vl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index afbf13f..50cd018 100644
--- a/vl.c
+++ b/vl.c
@@ -4053,14 +4053,14 @@ int main(int argc, char **argv, char **envp)
         }
     }
 
+    loc_set_none();
+
     replay_configure(icount_opts);
 
     set_machine_options(&machine_class);
 
     set_memory_options(&ram_slots, &maxram_size, machine_class);
 
-    loc_set_none();
-
     os_daemonize();
 
     if (qemu_init_main_loop(&main_loop_err)) {
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 2/4] vl: Reset location after handling command-line arguments

[Marcel Apfelbaum]

On 02/12/2016 09:02 PM, Eduardo Habkost wrote:
> After looping through all command-line arguments, error location
> info becomes obsolete, and any function calling error_report()
> will print misleading information. This breaks error reporting
> for some option handling, like:
>
>    $ qemu-system-x86_64 -icount rr=x -vnc :0
>    qemu-system-x86_64: -vnc :0: Invalid icount rr option: x
>
>    $ qemu-system-x86_64 -m size= -vnc :0
>    qemu-system-x86_64: -vnc :0: missing 'size' option value
>
> Fix this by resetting location info as soon as we exit the
> command-line handling loop.
>
> With this, replay_configure() and set_memory_options() won't
> print any location info yet, but at least they won't print
> incorrect information.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   vl.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index afbf13f..50cd018 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4053,14 +4053,14 @@ int main(int argc, char **argv, char **envp)
>           }
>       }
>
> +    loc_set_none();
> +
>       replay_configure(icount_opts);
>
>       set_machine_options(&machine_class);
>
>       set_memory_options(&ram_slots, &maxram_size, machine_class);
>
> -    loc_set_none();
> -
>       os_daemonize();
>
>       if (qemu_init_main_loop(&main_loop_err)) {
>


Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>


Thanks,
Marcel

Re: [Qemu-devel] [PATCH v2 2/4] vl: Reset location after handling command-line arguments

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> After looping through all command-line arguments, error location
> info becomes obsolete, and any function calling error_report()
> will print misleading information. This breaks error reporting
> for some option handling, like:
>
>   $ qemu-system-x86_64 -icount rr=x -vnc :0
>   qemu-system-x86_64: -vnc :0: Invalid icount rr option: x
>
>   $ qemu-system-x86_64 -m size= -vnc :0
>   qemu-system-x86_64: -vnc :0: missing 'size' option value
>
> Fix this by resetting location info as soon as we exit the
> command-line handling loop.
>
> With this, replay_configure() and set_memory_options() won't
> print any location info yet, but at least they won't print
> incorrect information.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  vl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index afbf13f..50cd018 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4053,14 +4053,14 @@ int main(int argc, char **argv, char **envp)
>          }
>      }
>  
> +    loc_set_none();
> +

When I added loc_set_none() in commit 0f0bc3f, I intentionally put no
empty line between the loop's closing brace and the loc_set_none(), to
reduce the chance of people sticking something in between unthinkingly.
It failed.  Let's try again with a billboard:

    }
    /*
     * Clear error location left behind by the loop.
     * Best done right after the loop.  Do not insert code here!
     */
   loc_set_none()

Can do that when I apply to error-next.

>      replay_configure(icount_opts);
>  
>      set_machine_options(&machine_class);
>  
>      set_memory_options(&ram_slots, &maxram_size, machine_class);
>  
> -    loc_set_none();
> -
>      os_daemonize();
>  
>      if (qemu_init_main_loop(&main_loop_err)) {

Re: [Qemu-devel] [PATCH v2 2/4] vl: Reset location after handling command-line arguments

[Eduardo Habkost]

On Mon, Feb 15, 2016 at 11:29:44AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > After looping through all command-line arguments, error location
> > info becomes obsolete, and any function calling error_report()
> > will print misleading information. This breaks error reporting
> > for some option handling, like:
> >
> >   $ qemu-system-x86_64 -icount rr=x -vnc :0
> >   qemu-system-x86_64: -vnc :0: Invalid icount rr option: x
> >
> >   $ qemu-system-x86_64 -m size= -vnc :0
> >   qemu-system-x86_64: -vnc :0: missing 'size' option value
> >
> > Fix this by resetting location info as soon as we exit the
> > command-line handling loop.
> >
> > With this, replay_configure() and set_memory_options() won't
> > print any location info yet, but at least they won't print
> > incorrect information.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  vl.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/vl.c b/vl.c
> > index afbf13f..50cd018 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -4053,14 +4053,14 @@ int main(int argc, char **argv, char **envp)
> >          }
> >      }
> >  
> > +    loc_set_none();
> > +
> 
> When I added loc_set_none() in commit 0f0bc3f, I intentionally put no
> empty line between the loop's closing brace and the loc_set_none(), to
> reduce the chance of people sticking something in between unthinkingly.
> It failed.  Let's try again with a billboard:
> 
>     }
>     /*
>      * Clear error location left behind by the loop.
>      * Best done right after the loop.  Do not insert code here!
>      */
>    loc_set_none()

ACK.

-- 
Eduardo

[Qemu-devel] [PATCH v2 3/4] replay: Set error location properly when parsing options

[Eduardo Habkost]

Set error location so the error_report() calls will show
appropriate command-line argument or config file info.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 replay/replay.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/replay/replay.c b/replay/replay.c
index 9cac178..f8739c2 100644
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -262,6 +262,14 @@ void replay_configure(QemuOpts *opts)
     const char *fname;
     const char *rr;
     ReplayMode mode = REPLAY_MODE_NONE;
+    Location loc;
+
+    if (!opts) {
+        return;
+    }
+
+    loc_push_none(&loc);
+    qemu_opts_loc_restore(opts);
 
     rr = qemu_opt_get(opts, "rr");
     if (!rr) {
@@ -283,6 +291,8 @@ void replay_configure(QemuOpts *opts)
     }
 
     replay_enable(fname, mode);
+
+    loc_pop(&loc);
 }
 
 void replay_start(void)
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 3/4] replay: Set error location properly when parsing options

[Marcel Apfelbaum]

On 02/12/2016 09:02 PM, Eduardo Habkost wrote:
> Set error location so the error_report() calls will show
> appropriate command-line argument or config file info.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   replay/replay.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/replay/replay.c b/replay/replay.c
> index 9cac178..f8739c2 100644
> --- a/replay/replay.c
> +++ b/replay/replay.c
> @@ -262,6 +262,14 @@ void replay_configure(QemuOpts *opts)
>       const char *fname;
>       const char *rr;
>       ReplayMode mode = REPLAY_MODE_NONE;
> +    Location loc;
> +
> +    if (!opts) {
> +        return;
> +    }
> +
> +    loc_push_none(&loc);
> +    qemu_opts_loc_restore(opts);
>
>       rr = qemu_opt_get(opts, "rr");
>       if (!rr) {
> @@ -283,6 +291,8 @@ void replay_configure(QemuOpts *opts)
>       }
>
>       replay_enable(fname, mode);
> +
> +    loc_pop(&loc);
>   }
>
>   void replay_start(void)
>

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>


Thanks,
Marcel

[Qemu-devel] [PATCH v2 4/4] vl: Set error location when parsing memory options

[Eduardo Habkost]

Set error location so the error_report() calls will show
appropriate command-line argument or config file info.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 vl.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/vl.c b/vl.c
index 50cd018..8fe79dd 100644
--- a/vl.c
+++ b/vl.c
@@ -2907,6 +2907,10 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
     const char *maxmem_str, *slots_str;
     const ram_addr_t default_ram_size = mc->default_ram_size;
     QemuOpts *opts = qemu_find_opts_singleton("memory");
+    Location loc;
+
+    loc_push_none(&loc);
+    qemu_opts_loc_restore(opts);
 
     sz = 0;
     mem_str = qemu_opt_get(opts, "size");
@@ -2981,6 +2985,8 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
                 "'%s' option", slots_str ? "maxmem" : "slots");
         exit(EXIT_FAILURE);
     }
+
+    loc_pop(&loc);
 }
 
 int main(int argc, char **argv, char **envp)
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 4/4] vl: Set error location when parsing memory options

[Marcel Apfelbaum]

On 02/12/2016 09:02 PM, Eduardo Habkost wrote:
> Set error location so the error_report() calls will show
> appropriate command-line argument or config file info.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   vl.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/vl.c b/vl.c
> index 50cd018..8fe79dd 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2907,6 +2907,10 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
>       const char *maxmem_str, *slots_str;
>       const ram_addr_t default_ram_size = mc->default_ram_size;
>       QemuOpts *opts = qemu_find_opts_singleton("memory");
> +    Location loc;
> +
> +    loc_push_none(&loc);
> +    qemu_opts_loc_restore(opts);
>
>       sz = 0;
>       mem_str = qemu_opt_get(opts, "size");
> @@ -2981,6 +2985,8 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size,
>                   "'%s' option", slots_str ? "maxmem" : "slots");
>           exit(EXIT_FAILURE);
>       }
> +
> +    loc_pop(&loc);
>   }
>
>   int main(int argc, char **argv, char **envp)
>

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>


Thanks,
Marcel

Re: [Qemu-devel] [PATCH v2 0/4] Error location reporting fixes

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> This fixes the following bugs in error reporting:
>
>   $ qemu-system-x86_64 -icount rr=x -vnc :0
>   qemu-system-x86_64: -vnc :0: Invalid icount rr option: x
>
>   $ qemu-system-x86_64 -m size= -vnc :0
>   qemu-system-x86_64: -vnc :0: missing 'size' option value
>
> The last command-line option (-vnc) is being shown in the error
> message, instead of the -m or -icount options.
>
> This also includes a patch submitted previously by Marcel, to
> ensure there are no ordering conflicts when applying the patches.
> Marcel's patch fixes the following bug:
>
>   $ qemu-system-x86_64 -M q35-1.5 -redir tcp:8022::22
>   qemu-system-x86_64: -redir tcp:8022::22: unsupported machine type
>   Use -machine help to list supported machines

I have two minor suggestions.  Regardless:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

I can take the series through my tree.  Of course, Paolo merging it
through his would also be fine.

[Qemu-devel] [PATCH 5/4] vl: Clean up machine selection in main().

[Markus Armbruster]

We set machine_class to the default first, and update it to the real
one later.  Any use of machine_class in between is almost certainly
wrong.  Set it once and for all instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 vl.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/vl.c b/vl.c
index 7918e9f..098728c 100644
--- a/vl.c
+++ b/vl.c
@@ -2748,8 +2748,9 @@ static const QEMUOption *lookup_opt(int argc, char **argv,
     return popt;
 }
 
-static void set_machine_options(MachineClass **machine_class)
+static MachineClass *select_machine(MachineClass *dflt)
 {
+    MachineClass *machine_class = dflt;
     const char *optarg;
     QemuOpts *opts;
     Location loc;
@@ -2761,16 +2762,17 @@ static void set_machine_options(MachineClass **machine_class)
 
     optarg = qemu_opt_get(opts, "type");
     if (optarg) {
-        *machine_class = machine_parse(optarg);
+        machine_class = machine_parse(optarg);
     }
 
-    if (*machine_class == NULL) {
+    if (!machine_class) {
         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);
+    return machine_class;
 }
 
 static int machine_set_property(void *opaque,
@@ -3075,7 +3077,6 @@ int main(int argc, char **argv, char **envp)
     os_setup_early_signal_handling();
 
     module_call_init(MODULE_INIT_MACHINE);
-    machine_class = find_default_machine();
     cpu_model = NULL;
     snapshot = 0;
     cyls = heads = secs = 0;
@@ -4066,7 +4067,7 @@ int main(int argc, char **argv, char **envp)
 
     replay_configure(icount_opts);
 
-    set_machine_options(&machine_class);
+    machine_class = select_machine(find_default_machine());
 
     set_memory_options(&ram_slots, &maxram_size, machine_class);
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH 5/4] vl: Clean up machine selection in main().

[Laszlo Ersek]

On 02/16/16 15:57, Markus Armbruster wrote:
> We set machine_class to the default first, and update it to the real
> one later.  Any use of machine_class in between is almost certainly
> wrong.  Set it once and for all instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  vl.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 7918e9f..098728c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2748,8 +2748,9 @@ static const QEMUOption *lookup_opt(int argc, char **argv,
>      return popt;
>  }
>  
> -static void set_machine_options(MachineClass **machine_class)
> +static MachineClass *select_machine(MachineClass *dflt)
>  {
> +    MachineClass *machine_class = dflt;
>      const char *optarg;
>      QemuOpts *opts;
>      Location loc;
> @@ -2761,16 +2762,17 @@ static void set_machine_options(MachineClass **machine_class)
>  
>      optarg = qemu_opt_get(opts, "type");
>      if (optarg) {
> -        *machine_class = machine_parse(optarg);
> +        machine_class = machine_parse(optarg);
>      }
>  
> -    if (*machine_class == NULL) {
> +    if (!machine_class) {
>          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);
> +    return machine_class;
>  }
>  
>  static int machine_set_property(void *opaque,
> @@ -3075,7 +3077,6 @@ int main(int argc, char **argv, char **envp)
>      os_setup_early_signal_handling();
>  
>      module_call_init(MODULE_INIT_MACHINE);
> -    machine_class = find_default_machine();
>      cpu_model = NULL;
>      snapshot = 0;
>      cyls = heads = secs = 0;
> @@ -4066,7 +4067,7 @@ int main(int argc, char **argv, char **envp)
>  
>      replay_configure(icount_opts);
>  
> -    set_machine_options(&machine_class);
> +    machine_class = select_machine(find_default_machine());
>  
>      set_memory_options(&ram_slots, &maxram_size, machine_class);
>  
> 

Sorry for not being more responsive in this thread. I read through the
patches now (including this one), and they look good to me.

I have one suggestion for the commit message of this patch, after
checking "vl.c" (and keeping the earlier patches of the series in mind):
after the statement

  Any use of machine_class in between is almost certainly wrong

can you please observe

  (there are no such uses right now)

?

series
Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 5/4] vl: Clean up machine selection in main().

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> On 02/16/16 15:57, Markus Armbruster wrote:
>> We set machine_class to the default first, and update it to the real
>> one later.  Any use of machine_class in between is almost certainly
>> wrong.  Set it once and for all instead.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  vl.c | 11 ++++++-----
>>  1 file changed, 6 insertions(+), 5 deletions(-)
>> 
>> diff --git a/vl.c b/vl.c
>> index 7918e9f..098728c 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2748,8 +2748,9 @@ static const QEMUOption *lookup_opt(int argc, char **argv,
>>      return popt;
>>  }
>>  
>> -static void set_machine_options(MachineClass **machine_class)
>> +static MachineClass *select_machine(MachineClass *dflt)
>>  {
>> +    MachineClass *machine_class = dflt;
>>      const char *optarg;
>>      QemuOpts *opts;
>>      Location loc;
>> @@ -2761,16 +2762,17 @@ static void set_machine_options(MachineClass **machine_class)
>>  
>>      optarg = qemu_opt_get(opts, "type");
>>      if (optarg) {
>> -        *machine_class = machine_parse(optarg);
>> +        machine_class = machine_parse(optarg);
>>      }
>>  
>> -    if (*machine_class == NULL) {
>> +    if (!machine_class) {
>>          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);
>> +    return machine_class;
>>  }
>>  
>>  static int machine_set_property(void *opaque,
>> @@ -3075,7 +3077,6 @@ int main(int argc, char **argv, char **envp)
>>      os_setup_early_signal_handling();
>>  
>>      module_call_init(MODULE_INIT_MACHINE);
>> -    machine_class = find_default_machine();
>>      cpu_model = NULL;
>>      snapshot = 0;
>>      cyls = heads = secs = 0;
>> @@ -4066,7 +4067,7 @@ int main(int argc, char **argv, char **envp)
>>  
>>      replay_configure(icount_opts);
>>  
>> -    set_machine_options(&machine_class);
>> +    machine_class = select_machine(find_default_machine());
>>  
>>      set_memory_options(&ram_slots, &maxram_size, machine_class);
>>  
>> 
>
> Sorry for not being more responsive in this thread. I read through the
> patches now (including this one), and they look good to me.
>
> I have one suggestion for the commit message of this patch, after
> checking "vl.c" (and keeping the earlier patches of the series in mind):
> after the statement
>
>   Any use of machine_class in between is almost certainly wrong
>
> can you please observe
>
>   (there are no such uses right now)
>
> ?

Done.

> series
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 5/4] vl: Clean up machine selection in main().

[Marcel Apfelbaum]

On 02/16/2016 04:57 PM, Markus Armbruster wrote:
> We set machine_class to the default first, and update it to the real
> one later.  Any use of machine_class in between is almost certainly
> wrong.  Set it once and for all instead.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   vl.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 7918e9f..098728c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2748,8 +2748,9 @@ static const QEMUOption *lookup_opt(int argc, char **argv,
>       return popt;
>   }
>
> -static void set_machine_options(MachineClass **machine_class)
> +static MachineClass *select_machine(MachineClass *dflt)

Hi Markus,

I am no fan of "dflt" naming, but I can live with it.

>   {
> +    MachineClass *machine_class = dflt;
>       const char *optarg;
>       QemuOpts *opts;
>       Location loc;
> @@ -2761,16 +2762,17 @@ static void set_machine_options(MachineClass **machine_class)
>
>       optarg = qemu_opt_get(opts, "type");
>       if (optarg) {
> -        *machine_class = machine_parse(optarg);
> +        machine_class = machine_parse(optarg);
>       }
>
> -    if (*machine_class == NULL) {
> +    if (!machine_class) {
>           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);
> +    return machine_class;
>   }
>
>   static int machine_set_property(void *opaque,
> @@ -3075,7 +3077,6 @@ int main(int argc, char **argv, char **envp)
>       os_setup_early_signal_handling();
>
>       module_call_init(MODULE_INIT_MACHINE);
> -    machine_class = find_default_machine();
>       cpu_model = NULL;
>       snapshot = 0;
>       cyls = heads = secs = 0;
> @@ -4066,7 +4067,7 @@ int main(int argc, char **argv, char **envp)
>
>       replay_configure(icount_opts);
>
> -    set_machine_options(&machine_class);
> +    machine_class = select_machine(find_default_machine());

I like the approach "going all the way", I would
even hide the call to find_default_machine inside select_machine.

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>


Thanks,
Marcel

>
>       set_memory_options(&ram_slots, &maxram_size, machine_class);
>
>

Re: [Qemu-devel] [PATCH 5/4] vl: Clean up machine selection in main().

[Markus Armbruster]

Marcel Apfelbaum <marcel@redhat.com> writes:

> On 02/16/2016 04:57 PM, Markus Armbruster wrote:
>> We set machine_class to the default first, and update it to the real
>> one later.  Any use of machine_class in between is almost certainly
>> wrong.  Set it once and for all instead.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   vl.c | 11 ++++++-----
>>   1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 7918e9f..098728c 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2748,8 +2748,9 @@ static const QEMUOption *lookup_opt(int argc, char **argv,
>>       return popt;
>>   }
>>
>> -static void set_machine_options(MachineClass **machine_class)
>> +static MachineClass *select_machine(MachineClass *dflt)
>
> Hi Markus,
>
> I am no fan of "dflt" naming, but I can live with it.
>
>>   {
>> +    MachineClass *machine_class = dflt;
>>       const char *optarg;
>>       QemuOpts *opts;
>>       Location loc;
>> @@ -2761,16 +2762,17 @@ static void set_machine_options(MachineClass **machine_class)
>>
>>       optarg = qemu_opt_get(opts, "type");
>>       if (optarg) {
>> -        *machine_class = machine_parse(optarg);
>> +        machine_class = machine_parse(optarg);
>>       }
>>
>> -    if (*machine_class == NULL) {
>> +    if (!machine_class) {
>>           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);
>> +    return machine_class;
>>   }
>>
>>   static int machine_set_property(void *opaque,
>> @@ -3075,7 +3077,6 @@ int main(int argc, char **argv, char **envp)
>>       os_setup_early_signal_handling();
>>
>>       module_call_init(MODULE_INIT_MACHINE);
>> -    machine_class = find_default_machine();
>>       cpu_model = NULL;
>>       snapshot = 0;
>>       cyls = heads = secs = 0;
>> @@ -4066,7 +4067,7 @@ int main(int argc, char **argv, char **envp)
>>
>>       replay_configure(icount_opts);
>>
>> -    set_machine_options(&machine_class);
>> +    machine_class = select_machine(find_default_machine());
>
> I like the approach "going all the way", I would
> even hide the call to find_default_machine inside select_machine.

Good idea.  Squashing in:

diff --git a/vl.c b/vl.c
index 098728c..c0b6747 100644
--- a/vl.c
+++ b/vl.c
@@ -2748,9 +2748,9 @@ static const QEMUOption *lookup_opt(int argc, char **argv,
     return popt;
 }
 
-static MachineClass *select_machine(MachineClass *dflt)
+static MachineClass *select_machine(void)
 {
-    MachineClass *machine_class = dflt;
+    MachineClass *machine_class = find_default_machine();
     const char *optarg;
     QemuOpts *opts;
     Location loc;
@@ -4067,7 +4067,7 @@ int main(int argc, char **argv, char **envp)
 
     replay_configure(icount_opts);
 
-    machine_class = select_machine(find_default_machine());
+    machine_class = select_machine();
 
     set_memory_options(&ram_slots, &maxram_size, machine_class);
 
>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH v2 0/4] Error location reporting fixes

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> This fixes the following bugs in error reporting:
>
>   $ qemu-system-x86_64 -icount rr=x -vnc :0
>   qemu-system-x86_64: -vnc :0: Invalid icount rr option: x
>
>   $ qemu-system-x86_64 -m size= -vnc :0
>   qemu-system-x86_64: -vnc :0: missing 'size' option value
>
> The last command-line option (-vnc) is being shown in the error
> message, instead of the -m or -icount options.
>
> This also includes a patch submitted previously by Marcel, to
> ensure there are no ordering conflicts when applying the patches.
> Marcel's patch fixes the following bug:
>
>   $ qemu-system-x86_64 -M q35-1.5 -redir tcp:8022::22
>   qemu-system-x86_64: -redir tcp:8022::22: unsupported machine type
>   Use -machine help to list supported machines

Applied to error-next, thanks!