Re: [PATCH v2 00/44] Make qdev static property API usable by any QOM type

[Paolo Bonzini]

On 06/11/20 22:10, Eduardo Habkost wrote:
> This was implemented at:
> https://gitlab.com/ehabkost/qemu/-/commits/work/qdev-make-generic
>
> This is the interface I'd like to submit as v3:
>
> static Property machine_props[] = {
>      DEFINE_PROP_STRING("kernel", MachineState, kernel_filename,
>                         .description = "Linux kernel image file"),
>      DEFINE_PROP_STRING("initrd", MachineState, initrd_filename,
>                         .description = "Linux initial ramdisk file"),
>      DEFINE_PROP_STRING("append", MachineState, kernel_cmdline,
>                         .description = "Linux kernel command line"),
>      DEFINE_PROP_STRING("dtb", MachineState, dtb,
>                         .description = "Linux kernel device tree file"),
>      DEFINE_PROP_STRING("dumpdtb", MachineState, dumpdtb,
>                         .description = "Dump current dtb to a file and quit"),
>      DEFINE_PROP_STRING("dt-compatible", MachineState, dt_compatible,
>                         .description = "Overrides the \"compatible\" "
>                                        "property of the dt root node"),
>      DEFINE_PROP_STRING("firmware", MachineState, firmware,
>                         .description = "Firmware image"),
>      DEFINE_PROP_STRING("memory-backend", MachineState, ram_memdev_id,
>                         .description = "ID of memory backend object"),
>      DEFINE_PROP_BOOL("dump-guest-core", MachineState, dump_guest_core, true,
>                       .description = "Include guest memory in a core dump"),
>      DEFINE_PROP_BOOL("mem-merge", MachineState, mem_merge, true,
>                       .description = "Enable/disable memory merge support"),
>      DEFINE_PROP_BOOL("graphics", MachineState, enable_graphics, true,
>                       .description = "Set on/off to enable/disable graphics 
> emulation"),
>      DEFINE_PROP_BOOL("suppress-vmdesc", MachineState, suppress_vmdesc, false,
>                       .description = "Set on to disable self-describing 
> migration"),
>      DEFINE_PROP_UINT32("phandle-start", MachineState, phandle_start, 0,
>                         .description = "The first phandle ID we may generate 
> dynamically"),
>      DEFINE_PROP_END_OF_LIST(),
> };
>
> static void machine_class_init(ObjectClass *oc, void *data)
> {
>      ...
>      object_class_add_field_properties(oc, machine_props, 
> prop_allow_set_always);
>      ...
> }

If all properties were like this, it would be okay.  But the API in v2 
is the one that is most consistent with QOM in general. Here is how this 
change would be a loss in term of consistency:

- you have the field properties split in two (with the property itself 
in one place and the allow-set function in a different place), and also 
you'd have some properties listed as array and some as function calls.

- we would have different ways to handle device field properties (with 
dc->props) compared to object properties.

- while it's true that the QEMU code base has ~500 matches for 
"object*_property_add*" calls, and ~2100 for "DEFINE_PROP*", the new 
field properties would pretty much be used only in places that use 
object_class_property_add*.  (And converting DEFINE_PROP* to PROP* would 
be relatively easy to script, unlike having an array-based definition 
for all uses of object_class_property*).

The choice to describe class properties as function calls was made in 
2016 (commit 16bf7f522a, "qom: Allow properties to be registered against 
classes", 2016-01-18); so far I don't see that it has been misused.

Also, I don't think it's any easier to write an introspection code 
generator with DEFINE_PROP_*.  You would still have to parse the class 
init function to find the reference to the array (and likewise the 
TypeInfo struct to find the class init function).

Paolo