[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <drive
From: |
Markus Armbruster |
Subject: |
Re: [RFC PATCH-for-6.2 v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option |
Date: |
Mon, 22 Nov 2021 15:32:36 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) |
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).