| Message ID | 20211221071818.34731-1-mkfssion@mkfssion.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vl: Add -set options to device opts dict when using JSON syntax for -device | expand |
On 12/21/21 13:58, Markus Armbruster wrote: >> Is this a regression? I suspect commit 5dacda5167 "vl: Enable JSON >> syntax for -device" (v6.2.0). > Obviously not a regression: everything that used to work still works. > FWIW I think -set should be deprecated. I'm not aware of any particularly useful use of it. There are a couple in the QEMU tests (in vhost-user-test and in qemu-iotests 068), but in both cases the code would be easier to follow without; patches can be dusted off if desired. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 12/21/21 13:58, Markus Armbruster wrote: >>> Is this a regression? I suspect commit 5dacda5167 "vl: Enable JSON >>> syntax for -device" (v6.2.0). >> >> Obviously not a regression: everything that used to work still works. > > FWIW I think -set should be deprecated. I'm not aware of any > particularly useful use of it. There are a couple in the QEMU tests > (in vhost-user-test and in qemu-iotests 068), but in both cases the > code would be easier to follow without; patches can be dusted off if > desired. -set has its uses, but they're kind of obscure. When you want to use some canned configuration with slight modifications, then -readconfig canned.cfg -set ... is nicer than editing a copy of canned.cfg. For what it's worth, we have a few cans in docs/config/, and we refer to at least one of them in the manual (in docs/system/devices/usb.rst). There are a few really good ideas in QemuOpts. I count -readconfig, -writeconfig and -set among them. Unfortunately, they have been marred by us not converting the whole CLI to QemuOpts as envisaged. And now we never will, because our needs have long outgrown what QemuOpts can provide. I'd love to have unmarred, QAPI-based replacements. However, I doubt maintaining backwards compatibility will be practical and worthwhile. Declare these options unstable?
On 12/21/21 16:40, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 12/21/21 13:58, Markus Armbruster wrote: >>>> Is this a regression? I suspect commit 5dacda5167 "vl: Enable JSON >>>> syntax for -device" (v6.2.0). >>> >>> Obviously not a regression: everything that used to work still works. >> >> FWIW I think -set should be deprecated. I'm not aware of any >> particularly useful use of it. There are a couple in the QEMU tests >> (in vhost-user-test and in qemu-iotests 068), but in both cases the >> code would be easier to follow without; patches can be dusted off if >> desired. > > -set has its uses, but they're kind of obscure. When you want to use > some canned configuration with slight modifications, then -readconfig > canned.cfg -set ... is nicer than editing a copy of canned.cfg. For > what it's worth, we have a few cans in docs/config/, and we refer to at > least one of them in the manual (in docs/system/devices/usb.rst). > > There are a few really good ideas in QemuOpts. I count -readconfig, > -writeconfig and -set among them. Unfortunately, they have been marred > by us not converting the whole CLI to QemuOpts as envisaged. And now we > never will, because our needs have long outgrown what QemuOpts can > provide. > > I'd love to have unmarred, QAPI-based replacements. However, I doubt > maintaining backwards compatibility will be practical and worthwhile. > > Declare these options unstable? > > I agree. Without QemuOpts, and more precisely the ability to alter them before they are really handled, this kind of feature will be impossible to have. Only way I see, is to reverse the mechanism: + handle set option before, it store the key,value somewhere + an option like -device checks that store and fetch their values. -- Damien
On 12/21/21 16:40, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 12/21/21 13:58, Markus Armbruster wrote: >>>> Is this a regression? I suspect commit 5dacda5167 "vl: Enable JSON >>>> syntax for -device" (v6.2.0). >>> >>> Obviously not a regression: everything that used to work still works. >> >> FWIW I think -set should be deprecated. I'm not aware of any >> particularly useful use of it. There are a couple in the QEMU tests >> (in vhost-user-test and in qemu-iotests 068), but in both cases the >> code would be easier to follow without; patches can be dusted off if >> desired. > > -set has its uses, but they're kind of obscure. When you want to use > some canned configuration with slight modifications, then -readconfig > canned.cfg -set ... is nicer than editing a copy of canned.cfg. For > what it's worth, we have a few cans in docs/config/, and we refer to at > least one of them in the manual (in docs/system/devices/usb.rst). > > There are a few really good ideas in QemuOpts. I count -readconfig, > -writeconfig and -set among them. Unfortunately, they have been marred > by us not converting the whole CLI to QemuOpts as envisaged. And now we > never will, because our needs have long outgrown what QemuOpts can > provide. > > I'd love to have unmarred, QAPI-based replacements. However, I doubt > maintaining backwards compatibility will be practical and worthwhile. > > Declare these options unstable? > > I agree. Without QemuOpts, and more precisely the ability to alter them before they are really handled, this kind of feature will be impossible to have. Only way I see, is to reverse the mechanism: + handle set option before, it store the key,value somewhere + an option like -device checks that store and fetch their values.
On 2021/12/21 19:26, Markus Armbruster wrote:> 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. > Thanks for the detailed explanation. Since I am new to QEMU codebase, I did not notice that the visitor is different when -device JSON syntax is used. > 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... > Oh, I forgot to match the group name. > 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. > There seems no way to know what type of value the property actually take. I am trying to add a QDict* parameter in qdev_device_add_from_qdict() function and set thoses properties via object_set_properties_from_keyval() call but with false option for from_json argument, which will use qobject_input_visitor_new_keyval() visitor for the properties provided via -set option. Maybe the issue can be fixed in that way.
On Tue, Dec 21, 2021 at 04:40:28PM +0100, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > > > On 12/21/21 13:58, Markus Armbruster wrote: > >>> Is this a regression? I suspect commit 5dacda5167 "vl: Enable JSON > >>> syntax for -device" (v6.2.0). > >> > >> Obviously not a regression: everything that used to work still works. > > > > FWIW I think -set should be deprecated. I'm not aware of any > > particularly useful use of it. There are a couple in the QEMU tests > > (in vhost-user-test and in qemu-iotests 068), but in both cases the > > code would be easier to follow without; patches can be dusted off if > > desired. > > -set has its uses, but they're kind of obscure. When you want to use > some canned configuration with slight modifications, then -readconfig > canned.cfg -set ... is nicer than editing a copy of canned.cfg. Simliar: configure stuff not supported by libvirt: <qemu:commandline> <qemu:arg value='-set'/> <qemu:arg value='device.video0.guestdebug=1'/> </qemu:commandline> There will always be a gap between qemu and libvirt, even if most of them are temporary only (while developing a new feature). I think we need some way to deal with this kind of tweaks when moving to QAPI-based machine setup. Possibly not in qemu, maybe it's easier to add new '<qemu:set device=... property=... value=...>' syntax to libvirt. take care, Gerd
On 12/22/21 09:22, Gerd Hoffmann wrote: > On Tue, Dec 21, 2021 at 04:40:28PM +0100, Markus Armbruster wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> On 12/21/21 13:58, Markus Armbruster wrote: >>>>> Is this a regression? I suspect commit 5dacda5167 "vl: Enable JSON >>>>> syntax for -device" (v6.2.0). >>>> >>>> Obviously not a regression: everything that used to work still works. >>> >>> FWIW I think -set should be deprecated. I'm not aware of any >>> particularly useful use of it. There are a couple in the QEMU tests >>> (in vhost-user-test and in qemu-iotests 068), but in both cases the >>> code would be easier to follow without; patches can be dusted off if >>> desired. >> >> -set has its uses, but they're kind of obscure. When you want to use >> some canned configuration with slight modifications, then -readconfig >> canned.cfg -set ... is nicer than editing a copy of canned.cfg. > > Simliar: configure stuff not supported by libvirt: > > <qemu:commandline> > <qemu:arg value='-set'/> > <qemu:arg value='device.video0.guestdebug=1'/> > </qemu:commandline> > > There will always be a gap between qemu and libvirt, even if most of > them are temporary only (while developing a new feature). I think we > need some way to deal with this kind of tweaks when moving to QAPI-based > machine setup. Possibly not in qemu, maybe it's easier to add new > '<qemu:set device=... property=... value=...>' syntax to libvirt. > > take care, > Gerd > > Can the set feature be handled by libvirt ? I mean, libvirt could do the merge itself because if I understand it correctly, the snippset just say: please add/override the "guestdebug=1" key/value pair to the 'video0' device command option. In QAPI, otherwise, we have qom-set, but it will happens after the device has been created, so it don't work for all properties. -- Damien
Hi, > > Simliar: configure stuff not supported by libvirt: > > > > <qemu:commandline> > > <qemu:arg value='-set'/> > > <qemu:arg value='device.video0.guestdebug=1'/> > > </qemu:commandline> > > > > There will always be a gap between qemu and libvirt, even if most of > > them are temporary only (while developing a new feature). I think we > > need some way to deal with this kind of tweaks when moving to QAPI-based > > machine setup. Possibly not in qemu, maybe it's easier to add new > > '<qemu:set device=... property=... value=...>' syntax to libvirt. > Can the set feature be handled by libvirt ? > I mean, libvirt could do the merge itself because if I understand it > correctly, the snippset just say: > please add/override the "guestdebug=1" key/value pair to the 'video0' device > command option. Yes. The above is the same as -device qxl,id=video0,${more-libvirt-opts},guestdebug=1 take care, Gerd
On Wed, Dec 22, 2021 at 09:22:47AM +0100, Gerd Hoffmann wrote: > On Tue, Dec 21, 2021 at 04:40:28PM +0100, Markus Armbruster wrote: > > Paolo Bonzini <pbonzini@redhat.com> writes: > > > > > On 12/21/21 13:58, Markus Armbruster wrote: > > >>> Is this a regression? I suspect commit 5dacda5167 "vl: Enable JSON > > >>> syntax for -device" (v6.2.0). > > >> > > >> Obviously not a regression: everything that used to work still works. > > > > > > FWIW I think -set should be deprecated. I'm not aware of any > > > particularly useful use of it. There are a couple in the QEMU tests > > > (in vhost-user-test and in qemu-iotests 068), but in both cases the > > > code would be easier to follow without; patches can be dusted off if > > > desired. > > > > -set has its uses, but they're kind of obscure. When you want to use > > some canned configuration with slight modifications, then -readconfig > > canned.cfg -set ... is nicer than editing a copy of canned.cfg. > > Simliar: configure stuff not supported by libvirt: > > <qemu:commandline> > <qemu:arg value='-set'/> > <qemu:arg value='device.video0.guestdebug=1'/> > </qemu:commandline> > > There will always be a gap between qemu and libvirt, even if most of > them are temporary only (while developing a new feature). I think we > need some way to deal with this kind of tweaks when moving to QAPI-based > machine setup. Possibly not in qemu, maybe it's easier to add new > '<qemu:set device=... property=... value=...>' syntax to libvirt. Yes, I'd suggest we get <qemu:device alias="video0" name="guestdebug" value="1/> and then libvirt can use it to add 'guestdebug: 1' directly to the JSON it generates, avoiding -set entirely. Regards, Daniel
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; } 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;
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 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(-)