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

[MkfsSion]

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.