MkfsSion <mkfss...@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 <mkfss...@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;
