softmmu/vl.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+)
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 <mkfssion@mkfssion.com>
---
v2:
1.Set device option when group is 'device' only
2.Store value in type that properties actually take
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));
+ } else {
+ const char *driver = qdict_get_try_str(opt->opts, "driver");
+ if (driver) {
+ ObjectClass *klass = object_class_by_name(driver);
+ ObjectProperty *prop = object_class_property_find(klass, key);
+ 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);
+ }
+ } 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);
+ }
+ }
+ 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;
}
--
2.34.1
ping https://lore.kernel.org/qemu-devel/20211229064421.5LPUBTk_b7lwFSu6jdh7beB7kZHoVtGGztQSJR1SClI@z/
ping https://lore.kernel.org/qemu-devel/20211224072511.63894-1-mkfssion@mkfssion.com
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 > > 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 <mkfssion@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; > }
On 2022/1/19 22:08, Markus Armbruster wrote: > 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 >> >> 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 <mkfssion@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. Thanks for your detailed review. The following is my opinion towards implementing -set option for JSON arguments. Having -set option worked for JSON argument improved compatability with libvirt (libvirt has switched to use JSON arguments for device by default). -set option is useful for libvirt user as libvirt doesn't support all functionality that QEMU provides. I have another idea for implementing this feature which seems addressed the above issue. We can implement this feature by add new parameter that refers to options provided by -set option to qdev_device_add_from_qdict() (This api seems is not widely used in QEMU tree) function and use old qobject_input_visitor_new() visitor for setting them. Do you think is OK to implement this feature in that way? Best wishes, YuanYang Meng > > >> + } 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; >> } >
MkfsSion <mkfssion@mkfssion.com> writes: > On 2022/1/19 22:08, Markus Armbruster wrote: [...] >> I don't think we should try to make -set work when using JSON arguments. >> > Thanks for your detailed review. You're welcome! > The following is my opinion towards implementing -set option for JSON arguments. > Having -set option worked for JSON argument improved compatability with libvirt (libvirt has switched to use JSON arguments for device by default). -set option is useful for libvirt user as libvirt doesn't support all functionality that QEMU provides. I understand you'd like to tweak how libvirt configures things. -set used to work, but doesn't anymore. > I have another idea for implementing this feature which seems addressed the above issue. We can implement this feature by add new parameter that refers to options provided by -set option to qdev_device_add_from_qdict() (This api seems is not widely used in QEMU tree) function and use old qobject_input_visitor_new() visitor for setting them. > Do you think is OK to implement this feature in that way? I'm afraid I don't understand what you're proposing. Daniel Berrangé suggested to provide the means to tweak in libvirt: Message-ID: <YdRIOC4XbSOLDpMj@redhat.com> https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg00289.html Would that work for you? [...]
© 2016 - 2024 Red Hat, Inc.