Re: [PATCH v2] vl: Add support to set properties when using JSON syntax for -device via -set option

[MkfsSion]

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;
>>              }
>