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

[Kevin Wolf <kwolf@redhat.com>]

Am 08.11.2020 um 15:05 hat Paolo Bonzini geschrieben:
> 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.

Why would you have properties defined as function calls for the same
object that uses the array?

I'm not entirely sure what you mean with allow-set. The only things I
can find are related to link properties, specifically the check()
callback of object_class_property_add_link(). If it's this, what would
be the problem with just adding it to DEFINE_PROP_LINK instead of
using a separate function call to define link properties?

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

You mean dynamic per-object properties, i.e. not class properties?

I think having different ways for different things (class vs. object) is
better than having different ways for the same things (class in qdev vs.
class in non-qdev).

> - 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.

This was the obvious incremental step forward at the time because you
just had to replace one function call with another function call. The
commit message doesn't explain that not using data was a conscious
decision. I think it would probably have been out of scope then.

> 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).

I don't think we should parse any C code. In my opinion, both
introspection and the array should eventually be generated by the QAPI
generator from the schema.

Kevin