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
>
> The patch fixes the above issue by trying to convert value provided by -set
> option to the type that the setting property actually takes.
>
> Signed-off-by: YuanYang Meng <mkfss...@mkfssion.com>
> ---
> v2:
> 1.Set device option when group is 'device' only
> 2.Store value in type that properties actually take
2. is an attempt to fix the issue I pointed out in review of v1
(example output corrected in places):
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"}' -set device.ms0.serial=123
qemu-system-x86_64: -device {"driver": "usb-mouse", "id": "ms0"}:
Invalid parameter type for 'serial', expected: string
I can't see how -set can store the right thing.
See code below.
Aside: the error messages refer to -device instead of -set. Known bug
in -set, hard to fix.
>
>
> softmmu/vl.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 620a1f1367..c213e9e022 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"
> @@ -2274,6 +2276,61 @@ static void qemu_read_default_config_file(Error **errp)
> }
> }
>
> +static bool qemu_set_device_option_property(const char *id, const char *key,
> + const char *value, Error **errp)
> {
> + DeviceOption *opt;
> + 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)) {
> + QObject *obj = NULL;
> + if ((strcmp(key, "id") == 0) ||
> + (strcmp(key, "bus") == 0) ||
> + (strcmp(key, "driver") == 0)) {
> + obj = QOBJECT(qstring_from_str(value));
Special case, because these are not QOM properties. Similarly
special-cased in qdev_device_add_from_qdict(). Okay.
> + } else {
> + const char *driver = qdict_get_try_str(opt->opts, "driver");
> + if (driver) {
> + ObjectClass *klass = object_class_by_name(driver);
This may fail.
> + ObjectProperty *prop = object_class_property_find(klass,
> key);
If it does, this crashes:
$ qemu-system-x86_64 -nodefaults -S -display none -device '{"driver":
"nonexistent", "id": "foo"}' -set device.foo.bar=42
Segmentation fault (core dumped)
> + if (prop) {
> + if (strcmp(prop->type, "str") == 0) {
> + obj = QOBJECT(qstring_from_str(value));
> + } else if (strcmp(prop->type, "bool") == 0) {
> + bool boolean;
> + if (qapi_bool_parse(key, value, &boolean, errp))
> {
> + obj = QOBJECT(qbool_from_bool(boolean));
> + }
> + } else if (strncmp(prop->type, "uint", 4) == 0) {
> + uint64_t num;
> + if (parse_option_size(key, value, &num, errp)) {
> + obj = QOBJECT(qnum_from_uint(num));
> + }
> + } else {
> + error_setg(errp,
> + "Setting property %s on device %s
> with "
> + "type %s is unsupported via -set
> option",
> + key, id, prop->type);
> + }
This guesses based on prop->type. Unfortunately, its values are a mess.
They are documented in qom.json:
# @type: the type of the property. This will typically come in one of four
# forms:
#
# 1) A primitive type such as 'u8', 'u16', 'bool', 'str', or
'double'.
# These types are mapped to the appropriate JSON type.
#
# 2) A child type in the form 'child<subtype>' where subtype is a
qdev
# device type name. Child properties create the composition tree.
#
# 3) A link type in the form 'link<subtype>' where subtype is a qdev
# device type name. Link properties form the device model graph.
I like that it says "one of four", then lists three. Fair warning to
the reader not to trust this.
In fact, 1) is aspirational. The value is whatever C code passes to
object_property_add(). Actually values include "bool", "int", "int32",
"size", "string", "uint16", "uint32", "uint64", "uint8",
"GuestPanicInformation", "QemuUUID", "X86CPUFeatureWordInfo", my
favorites "container", "guest statistics", "struct tm", and my special
favorite "struct".
Your code recognizes only some values we actually use. Even if it
recognized all, keeping it that way would be an impossible mission.
It parses (unsigned) integers with parse_option_size(). Apropriate only
sometimes.
The patch covers only -device. We've extended more options from just
QemuOpts (where -set works) to also JSON (where it doesn't),
e.g. -object. More to come.
This is more elaborate guesswork than v1, but it's still guesswork, and
still incomplete.
I don't think we should try to make -set work when using JSON arguments.
> + } else {
> + error_setg(errp, "Unable to find property %s on
> device %s",
> + key, id);
> + }
> + } else {
> + error_setg(errp, "Unable to get driver for device %s",
> id);
Masks the real error.
$ qemu-system-x86_64 -nodefaults -S -display none -device '{"id": "foo"}'
-set device.foo.bar=42
qemu-system-x86_64: -set device.foo.bar=42: Unable to get driver for device
foo
$ qemu-system-x86_64 -nodefaults -S -display none -device '{"id": "foo"}'
qemu-system-x86_64: -device {"id": "foo"}: Parameter 'driver' is missing
> + }
> + }
> + if (obj) {
> + qdict_del(opt->opts, key);
> + qdict_put_obj(opt->opts, key, obj);
> + return true;
> + } else {
> + return false;
> + }
> + }
> + }
> + return false;
> +}
> +
> static void qemu_set_option(const char *str, Error **errp)
> {
> char group[64], id[64], arg[64];
> @@ -2294,6 +2351,11 @@ static void qemu_set_option(const char *str, Error
> **errp)
> if (list) {
> opts = qemu_opts_find(list, id);
> if (!opts) {
> + if (strcmp(group, "device") == 0) {
> + if (qemu_set_device_option_property(id, arg,
> + str + offset + 1,
> errp))
> + return;
> + }
> error_setg(errp, "there is no %s \"%s\" defined", group, id);
> return;
> }
