| Message ID | 20211119182644.480115-1-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,PATCH-for-6.2,v3] qdev-monitor: Only allow full --global <driver>.<property>=<val> option | expand |
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) >
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; >> - }
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.
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
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).
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).
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)
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(-)