* [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option
@ 2021-11-19 18:26 Philippe Mathieu-Daudé
2021-11-19 18:46 ` BALATON Zoltan
2021-11-20 6:53 ` Markus Armbruster
0 siblings, 2 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-11-19 18:26 UTC (permalink / raw)
To: qemu-devel
Cc: Thomas Huth, Daniel P. Berrangé,
Eduardo Habkost, Markus Armbruster, Paolo Bonzini,
Philippe Mathieu-Daudé
When not all fields of the --global option are provided,
QEMU might crash:
$ qemu-system-x86_64 -global driver=isa-fdc
qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
string_input_visitor_new: Assertion `str' failed.
Aborted (core dumped)
Fix by only allowing --global with all 3 fields:
$ qemu-system-x86_64 -global driver=isa-fdc
Invalid 'global' option format. It must be provided as:
--global <driver>.<property>=<value>
Reported-by: Thomas Huth <thuth@redhat.com>
Suggested-by: Markus Armbruster <armbru@redhat.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v3: Change qemu_global_option (Markus)
Supersedes: <20211119122911.365036-1-philmd@redhat.com>
---
softmmu/qdev-monitor.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 01f3834db57..558272b147c 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -1029,13 +1029,10 @@ int qemu_global_option(const char *str)
qemu_opt_set(opts, "value", str + offset + 1, &error_abort);
return 0;
}
+ printf("Invalid 'global' option format. It must be provided as:\n");
+ printf(" --global <driver>.<property>=<value>\n");
- opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);
- if (!opts) {
- return -1;
- }
-
- return 0;
+ return -1;
}
bool qmp_command_available(const QmpCommand *cmd, Error **errp)
--
2.31.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option
2021-11-19 18:26 [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option Philippe Mathieu-Daudé
@ 2021-11-19 18:46 ` BALATON Zoltan
2021-11-19 19:07 ` Philippe Mathieu-Daudé
2021-11-20 6:53 ` Markus Armbruster
1 sibling, 1 reply; 7+ messages in thread
From: BALATON Zoltan @ 2021-11-19 18:46 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Thomas Huth, Daniel P. Berrangé,
Eduardo Habkost, Markus Armbruster, qemu-devel, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 1866 bytes --]
On Fri, 19 Nov 2021, Philippe Mathieu-Daudé wrote:
> When not all fields of the --global option are provided,
> QEMU might crash:
>
> $ qemu-system-x86_64 -global driver=isa-fdc
> qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
> string_input_visitor_new: Assertion `str' failed.
> Aborted (core dumped)
>
> Fix by only allowing --global with all 3 fields:
>
> $ qemu-system-x86_64 -global driver=isa-fdc
> Invalid 'global' option format. It must be provided as:
> --global <driver>.<property>=<value>
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v3: Change qemu_global_option (Markus)
>
> Supersedes: <20211119122911.365036-1-philmd@redhat.com>
> ---
> softmmu/qdev-monitor.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 01f3834db57..558272b147c 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -1029,13 +1029,10 @@ int qemu_global_option(const char *str)
> qemu_opt_set(opts, "value", str + offset + 1, &error_abort);
> return 0;
> }
> + printf("Invalid 'global' option format. It must be provided as:\n");
> + printf(" --global <driver>.<property>=<value>\n");
Should these be something else tnan plain printf? (Such as qemu_prinf or
qdev_printf or similar? Not sure how these work but plain printf in QEMU
is usually not what's meant.)
Regards,
BALATON Zoltan
> - opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);
> - if (!opts) {
> - return -1;
> - }
> -
> - return 0;
> + return -1;
> }
>
> bool qmp_command_available(const QmpCommand *cmd, Error **errp)
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option
2021-11-19 18:46 ` BALATON Zoltan
@ 2021-11-19 19:07 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-11-19 19:07 UTC (permalink / raw)
To: BALATON Zoltan
Cc: Thomas Huth, Daniel P. Berrangé,
Eduardo Habkost, Markus Armbruster, qemu-devel, Paolo Bonzini
On 11/19/21 19:46, BALATON Zoltan wrote:
> On Fri, 19 Nov 2021, Philippe Mathieu-Daudé wrote:
>> When not all fields of the --global option are provided,
>> QEMU might crash:
>>
>> $ qemu-system-x86_64 -global driver=isa-fdc
>> qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
>> string_input_visitor_new: Assertion `str' failed.
>> Aborted (core dumped)
>>
>> Fix by only allowing --global with all 3 fields:
>>
>> $ qemu-system-x86_64 -global driver=isa-fdc
>> Invalid 'global' option format. It must be provided as:
>> --global <driver>.<property>=<value>
>>
>> Reported-by: Thomas Huth <thuth@redhat.com>
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> v3: Change qemu_global_option (Markus)
>>
>> Supersedes: <20211119122911.365036-1-philmd@redhat.com>
>> ---
>> softmmu/qdev-monitor.c | 9 +++------
>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>> index 01f3834db57..558272b147c 100644
>> --- a/softmmu/qdev-monitor.c
>> +++ b/softmmu/qdev-monitor.c
>> @@ -1029,13 +1029,10 @@ int qemu_global_option(const char *str)
>> qemu_opt_set(opts, "value", str + offset + 1, &error_abort);
>> return 0;
>> }
>> + printf("Invalid 'global' option format. It must be provided as:\n");
>> + printf(" --global <driver>.<property>=<value>\n");
>
> Should these be something else tnan plain printf? (Such as qemu_prinf or
> qdev_printf or similar? Not sure how these work but plain printf in QEMU
> is usually not what's meant.)
I thought so first, but qemu_opts_print_help() calls plain printf()...
> Regards,
> BALATON Zoltan
>
>> - opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);
>> - if (!opts) {
>> - return -1;
>> - }
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option
2021-11-19 18:26 [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option Philippe Mathieu-Daudé
2021-11-19 18:46 ` BALATON Zoltan
@ 2021-11-20 6:53 ` Markus Armbruster
2021-11-22 13:21 ` Daniel P. Berrangé
1 sibling, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2021-11-20 6:53 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Paolo Bonzini, Thomas Huth, Daniel P. Berrangé,
qemu-devel, Eduardo Habkost
Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> When not all fields of the --global option are provided,
> QEMU might crash:
>
> $ qemu-system-x86_64 -global driver=isa-fdc
> qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
> string_input_visitor_new: Assertion `str' failed.
> Aborted (core dumped)
>
> Fix by only allowing --global with all 3 fields:
>
> $ qemu-system-x86_64 -global driver=isa-fdc
> Invalid 'global' option format. It must be provided as:
> --global <driver>.<property>=<value>
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v3: Change qemu_global_option (Markus)
>
> Supersedes: <20211119122911.365036-1-philmd@redhat.com>
> ---
> softmmu/qdev-monitor.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 01f3834db57..558272b147c 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -1029,13 +1029,10 @@ int qemu_global_option(const char *str)
> qemu_opt_set(opts, "value", str + offset + 1, &error_abort);
> return 0;
> }
> + printf("Invalid 'global' option format. It must be provided as:\n");
> + printf(" --global <driver>.<property>=<value>\n");
>
> - opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);
> - if (!opts) {
> - return -1;
> - }
> -
> - return 0;
> + return -1;
> }
>
> bool qmp_command_available(const QmpCommand *cmd, Error **errp)
This drops a documented part of the external interface:
$ qemu-system-x86_64 -help | grep -C 1 global
i.e. -set drive.$id.file=/path/to/image
-global driver.property=value
--> -global driver=driver,property=property,value=value
set a global default for a driver property
-boot [order=drives][,once=drives][,menu=on|off]
It goes back to commit 3751d7c43f "vl: allow full-blown QemuOpts syntax
for -global", v2.4.0.
The appropriate fix is to check @opts for presence of all three
parameters.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option
2021-11-20 6:53 ` Markus Armbruster
@ 2021-11-22 13:21 ` Daniel P. Berrangé
2021-11-22 14:32 ` Markus Armbruster
0 siblings, 1 reply; 7+ messages in thread
From: Daniel P. Berrangé @ 2021-11-22 13:21 UTC (permalink / raw)
To: Markus Armbruster
Cc: Paolo Bonzini, Thomas Huth, Philippe Mathieu-Daudé,
qemu-devel, Eduardo Habkost
On Sat, Nov 20, 2021 at 07:53:20AM +0100, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
> > When not all fields of the --global option are provided,
> > QEMU might crash:
> >
> > $ qemu-system-x86_64 -global driver=isa-fdc
> > qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
> > string_input_visitor_new: Assertion `str' failed.
> > Aborted (core dumped)
> >
> > Fix by only allowing --global with all 3 fields:
> >
> > $ qemu-system-x86_64 -global driver=isa-fdc
> > Invalid 'global' option format. It must be provided as:
> > --global <driver>.<property>=<value>
> >
> > Reported-by: Thomas Huth <thuth@redhat.com>
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> > v3: Change qemu_global_option (Markus)
> >
> > Supersedes: <20211119122911.365036-1-philmd@redhat.com>
> > ---
> > softmmu/qdev-monitor.c | 9 +++------
> > 1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> > index 01f3834db57..558272b147c 100644
> > --- a/softmmu/qdev-monitor.c
> > +++ b/softmmu/qdev-monitor.c
> > @@ -1029,13 +1029,10 @@ int qemu_global_option(const char *str)
> > qemu_opt_set(opts, "value", str + offset + 1, &error_abort);
> > return 0;
> > }
> > + printf("Invalid 'global' option format. It must be provided as:\n");
> > + printf(" --global <driver>.<property>=<value>\n");
> >
> > - opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);
> > - if (!opts) {
> > - return -1;
> > - }
> > -
> > - return 0;
> > + return -1;
> > }
> >
> > bool qmp_command_available(const QmpCommand *cmd, Error **errp)
>
> This drops a documented part of the external interface:
>
> $ qemu-system-x86_64 -help | grep -C 1 global
> i.e. -set drive.$id.file=/path/to/image
> -global driver.property=value
> --> -global driver=driver,property=property,value=value
> set a global default for a driver property
This doc makes it look like the two syntaxes are functionally
equivalent, but it seems that's not quite the case.
libvirt uses the driver.propert=value syntax for everything
except one case
-global driver=cfi.pflash01,property=secure,value=on
for that one if we try to use
-global cfi.pflash01.secure=on
it complains
qemu-system-x86_64: warning: global cfi.pflash01.secure has invalid class name
what's going on here ?
> -boot [order=drives][,once=drives][,menu=on|off]
>
> It goes back to commit 3751d7c43f "vl: allow full-blown QemuOpts syntax
> for -global", v2.4.0.
>
> The appropriate fix is to check @opts for presence of all three
> parameters.
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option
2021-11-22 13:21 ` Daniel P. Berrangé
@ 2021-11-22 14:32 ` Markus Armbruster
2021-11-23 13:17 ` Paolo Bonzini
0 siblings, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2021-11-22 14:32 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Thomas Huth, Eduardo Habkost, Markus Armbruster, qemu-devel,
Paolo Bonzini, Philippe Mathieu-Daudé
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Sat, Nov 20, 2021 at 07:53:20AM +0100, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>
>> > When not all fields of the --global option are provided,
>> > QEMU might crash:
>> >
>> > $ qemu-system-x86_64 -global driver=isa-fdc
>> > qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
>> > string_input_visitor_new: Assertion `str' failed.
>> > Aborted (core dumped)
>> >
>> > Fix by only allowing --global with all 3 fields:
>> >
>> > $ qemu-system-x86_64 -global driver=isa-fdc
>> > Invalid 'global' option format. It must be provided as:
>> > --global <driver>.<property>=<value>
>> >
>> > Reported-by: Thomas Huth <thuth@redhat.com>
>> > Suggested-by: Markus Armbruster <armbru@redhat.com>
>> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604
>> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> > ---
>> > v3: Change qemu_global_option (Markus)
>> >
>> > Supersedes: <20211119122911.365036-1-philmd@redhat.com>
>> > ---
>> > softmmu/qdev-monitor.c | 9 +++------
>> > 1 file changed, 3 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>> > index 01f3834db57..558272b147c 100644
>> > --- a/softmmu/qdev-monitor.c
>> > +++ b/softmmu/qdev-monitor.c
>> > @@ -1029,13 +1029,10 @@ int qemu_global_option(const char *str)
>> > qemu_opt_set(opts, "value", str + offset + 1, &error_abort);
>> > return 0;
>> > }
>> > + printf("Invalid 'global' option format. It must be provided as:\n");
>> > + printf(" --global <driver>.<property>=<value>\n");
>> >
>> > - opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);
>> > - if (!opts) {
>> > - return -1;
>> > - }
>> > -
>> > - return 0;
>> > + return -1;
>> > }
>> >
>> > bool qmp_command_available(const QmpCommand *cmd, Error **errp)
>>
>> This drops a documented part of the external interface:
>>
>> $ qemu-system-x86_64 -help | grep -C 1 global
>> i.e. -set drive.$id.file=/path/to/image
>> -global driver.property=value
>> --> -global driver=driver,property=property,value=value
>> set a global default for a driver property
>
> This doc makes it look like the two syntaxes are functionally
> equivalent, but it seems that's not quite the case.
>
> libvirt uses the driver.propert=value syntax for everything
> except one case
>
> -global driver=cfi.pflash01,property=secure,value=on
>
> for that one if we try to use
>
> -global cfi.pflash01.secure=on
>
> it complains
>
> qemu-system-x86_64: warning: global cfi.pflash01.secure has invalid class name
>
> what's going on here ?
Off-the-cuff guess: cfi.pflash01.secure=on gets parsed as
driver=cfi
property=pflash01.secure
value=on
Once again our "anything goes" attitude to naming wastes us time and
thus money.
In contrast, QAPI restricts names to "only ASCII letters, digits,
hyphen, and underscore" (see docs/devel/qapi-code-gen.rst section Naming
rules and reserved names).
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option
2021-11-22 14:32 ` Markus Armbruster
@ 2021-11-23 13:17 ` Paolo Bonzini
0 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2021-11-23 13:17 UTC (permalink / raw)
To: Markus Armbruster, Daniel P. Berrangé
Cc: Thomas Huth, Philippe Mathieu-Daudé, Eduardo Habkost, qemu-devel
On 11/22/21 15:32, Markus Armbruster wrote:
>> qemu-system-x86_64: warning: global cfi.pflash01.secure has invalid class name
>>
>> what's going on here ?
> Off-the-cuff guess: cfi.pflash01.secure=on gets parsed as
>
> driver=cfi
> property=pflash01.secure
> value=on
>
> Once again our "anything goes" attitude to naming wastes us time and
> thus money.
I'd blame more the sscanf parsing. Anyway, -global
driver=...,property=...,value=... works just fine in all cases, it's
just more verbose---and it might even be easier to use for Libvirt, if
it can use its usual QemuOpts-building facilities.
Anyhow, this patch breaks existing clients, as pointed out by Markus.
Paolo
> In contrast, QAPI restricts names to "only ASCII letters, digits,
> hyphen, and underscore" (see docs/devel/qapi-code-gen.rst section Naming
> rules and reserved names).
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-11-23 13:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-19 18:26 [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option Philippe Mathieu-Daudé
2021-11-19 18:46 ` BALATON Zoltan
2021-11-19 19:07 ` Philippe Mathieu-Daudé
2021-11-20 6:53 ` Markus Armbruster
2021-11-22 13:21 ` Daniel P. Berrangé
2021-11-22 14:32 ` Markus Armbruster
2021-11-23 13:17 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.