Re: [PATCH] vl: Add -set options to device opts dict when using JSON syntax for -device

[Markus Armbruster]

MkfsSion <mkfssion@mkfssion.com> writes:

> When using JSON syntax for -device, -set option can not find device
> specified in JSON by id field. The following commandline is an example:
>
> $ qemu-system-x86_64 -device '{"id":"foo"}' -set device.foo.bar=1
> qemu-system-x86_64: -set device.foo.bar=1: there is no device "foo" defined

Is this a regression?  I suspect commit 5dacda5167 "vl: Enable JSON
syntax for -device" (v6.2.0).

I believe any conversion away from QemuOpts loses -set.

> The patch adds -set options to JSON device opts dict for adding
> options to device by latter object_set_properties_from_keyval call.
>
> Signed-off-by: YuanYang Meng <mkfssion@mkfssion.com>
> ---
>  include/qemu/option.h |  4 ++++
>  softmmu/vl.c          | 28 ++++++++++++++++++++++++++++
>  util/qemu-option.c    |  2 +-
>  3 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 306bf07575..31fa9fdc25 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -45,6 +45,10 @@ const char *get_opt_value(const char *p, char **value);
>  
>  bool parse_option_size(const char *name, const char *value,
>                         uint64_t *ret, Error **errp);
> +
> +bool parse_option_number(const char *name, const char *value,
> +                         uint64_t *ret, Error **errp);
> +
>  bool has_help_option(const char *param);
>  
>  enum QemuOptType {
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 620a1f1367..feb3c49a65 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -30,7 +30,9 @@
>  #include "hw/qdev-properties.h"
>  #include "qapi/compat-policy.h"
>  #include "qapi/error.h"
> +#include "qapi/qmp/qbool.h"
>  #include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qnum.h"
>  #include "qapi/qmp/qstring.h"
>  #include "qapi/qmp/qjson.h"
>  #include "qemu-version.h"
> @@ -2279,6 +2281,7 @@ static void qemu_set_option(const char *str, Error 
> **errp)
>      char group[64], id[64], arg[64];
>      QemuOptsList *list;
>      QemuOpts *opts;
> +    DeviceOption *opt;
>      int rc, offset;
>  
>      rc = sscanf(str, "%63[^.].%63[^.].%63[^=]%n", group, id, arg, &offset);
> @@ -2294,6 +2297,31 @@ static void qemu_set_option(const char *str, Error 
> **errp)
>          if (list) {
>              opts = qemu_opts_find(list, id);
>              if (!opts) {
> +                QTAILQ_FOREACH(opt, &device_opts, next) {
> +                    const char *device_id = qdict_get_try_str(opt->opts, 
> "id");
> +                    if (device_id && (strcmp(device_id, id) == 0)) {
> +                        if (qdict_get(opt->opts, arg)) {
> +                            qdict_del(opt->opts, arg);
> +                        }
> +                        const char *value = str + offset + 1;
> +                        QObject *obj = NULL;
> +                        bool boolean;
> +                        uint64_t num;
> +                        if (qapi_bool_parse(arg, value, &boolean, NULL)) {
> +                            obj = QOBJECT(qbool_from_bool(boolean));
> +                        } else if (parse_option_number(arg, value, &num, 
> NULL)) {
> +                            obj = QOBJECT(qnum_from_uint(num));
> +                        } else if (parse_option_size(arg, value, &num, 
> NULL)) {
> +                            obj = QOBJECT(qnum_from_uint(num));
> +                        } else {
> +                            obj = QOBJECT(qstring_from_str(value));
> +                        }
> +                        if (obj) {
> +                            qdict_put_obj(opt->opts, arg, obj);
> +                            return;
> +                        }
> +                    }
> +                }
>                  error_setg(errp, "there is no %s \"%s\" defined", group, id);
>                  return;
>              }
               qemu_opt_set(opts, arg, str + offset + 1, errp);
           }
       }
   }

Two issues, and only looks fixable.

-device accepts either a QemuOpts or a JSON argument.

It parses the former with qemu_opts_parse_noisily() into a QemuOpt
stored in @qemu_device_opts.

It parses the latter with qobject_from_json() into a QObject stored in
@device_opts.  Yes, the names are confusing.

-set parses its argument into @group, @id, and @arg (the value).

Before the patch, it uses @group and @id to look up the QemuOpt in
@qemu_device_opts.  If found, it updates it with qemu_opt_set().

By design, -set operates on the QemuOpts store.

Options stored in @device_opts are not found.  Your patch tries to fix
that.  Before I can explain what's wrong with it, I need more
background.

QemuOpts arguments are parsed as a set of (key, value) pairs, where the
values are all strings (because qemu_device_opts.desc[] is empty).  We
access them with a qobject_input_visitor_new_keyval() visitor.  This
parses the strings according to the types being visited.

Example: key=42 gets stored as {"key": "42"}.  If we visit it with
visit_type_str(), we use string value "42".  If we visit it with
visit_type_int() or similar, we use integer value 42.  If we visit it
with visit_type_number(), we use double value 42.0.  If we visit it with
something else, we error out.

JSON arguments are parsed as arbitrary JSON object.  We access them with
a qobject_input_visitor_new() visitor.  This expects the values to have
JSON types appropriate for the types being visited.

Example: {"key": "42"} gets stored as is.  Now everything but
visit_type_str() errors out.

Back to your patch.  It adds code to look up a QObject in @device_opts.
If found, it updates it.

Issue#1: it does so regardless of @group.

Example:

    $ qemu-system-x86_64 -nodefaults -S -display none -M q35,usb=on -device 
'{"driver": "usb-mouse", "id": "ms0"}' -set chardev.ms0.serial=456

Here, -set chardev... gets misinterpreted as -set device...

Issue#2 is the value to store in @device_opts.  Always storing a string,
like in the QemuOpts case, would be wrong, because it works only when
its accessed with visit_type_str(), i.e. the property is actually of
string type.

Example:

    $ qemu-system-x86_64 -nodefaults -S -display none -M q35,usb=on -device 
'{"driver": "usb-mouse", "id": "ms0"}' -set device.ms0.serial=123

    $ qemu-system-x86_64 -nodefaults -S -display none -M q35,usb=on -device 
'{"driver": "usb-mouse", "id": "ms0"}' -set device.ms0.msos-desc=off
    qemu-system-x86_64: -device {"driver": "usb-mouse", "id": "ms0"}: Invalid 
parameter type for 'msos-desc', expected: boolean

Your patch stores a boolean if possible, else a number if possible, else
a string.  This is differently wrong.

Example:

    $ qemu-system-x86_64 -nodefaults -S -display none -M q35,usb=on -device 
'{"driver": "usb-mouse", "id": "ms0"}'

Example:

    $ qemu-system-x86_64 -nodefaults -S -display none -M q35,usb=on -device 
'{"driver": "usb-mouse", "id": "ms0"}' -set device.ms0.serial=123
    qemu-system-x86_64: -device {"driver": "usb-mouse", "id": "ms0", "serial": 
"123"}: Invalid parameter type for 'serial', expected: string

I can't see how -set can store the right thing.

Aside: the error messages refer to -device instead of -set.  Known bug
in -set, hard to fix.

> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index eedd08929b..b2427cba9f 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -88,7 +88,7 @@ const char *get_opt_value(const char *p, char **value)
>      return offset;
>  }
>  
> -static bool parse_option_number(const char *name, const char *value,
> +bool parse_option_number(const char *name, const char *value,
>                                  uint64_t *ret, Error **errp)
>  {
>      uint64_t number;