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

[Eduardo Habkost <ehabkost@redhat.com>]

On Fri, Nov 06, 2020 at 10:50:19AM -0500, Eduardo Habkost wrote:
> On Fri, Nov 06, 2020 at 10:45:11AM +0100, Kevin Wolf wrote:
> > Am 04.11.2020 um 16:59 hat Eduardo Habkost geschrieben:
> > > This series refactor the qdev property code so the static
> > > property system can be used by any QOM type.  As an example, at
> > > the end of the series some properties in TYPE_MACHINE are
> > > converted to static properties to demonstrate the new API.
> > > 
> > > Changes v1 -> v2
> > > ----------------
> > > 
> > > * Rename functions and source files to call the new feature
> > >   "field property" instead of "static property"
> > > 
> > > * Change the API signature from an array-based interface similar
> > >   to device_class_set_props() to a object_property_add()-like
> > >   interface.
> > > 
> > >   This means instead of doing this:
> > > 
> > >     object_class_property_add_static_props(oc, my_array_of_props);
> > > 
> > >   properties are registered like this:
> > > 
> > >     object_class_property_add_field(oc, "property-name"
> > >                                     PROP_XXX(MyState, my_field),
> > >                                     prop_allow_set_always);
> > > 
> > >   where PROP_XXX is a PROP_* macro like PROP_STRING, PROP_BOOL,
> > >   etc.
> > 
> > In comparison, I really like the resulting code from the array based
> > interface in v1 better.
> > 
> > I think it's mostly for two reasons: First, the array is much more
> > compact and easier to read. And maybe even more importantly, you know
> > it's static data and only static data. The function based interface can
> > be mixed with other code or the parameter list can contain calls to
> > other functions with side effects, so there are a lot more opportunities
> > for surprises.
> 
> This is a really good point, and I strongly agree with it.
> Letting code do funny tricks at runtime is one of the reasons QOM
> properties became hard to introspect.
> 
> > 
> > What I didn't like about the v1 interface is that there is still a
> > separate object_class_property_set_description() for each property, but
> > I think that could have been fixed by moving the description to the
> > definitions in the array, too.
> 
> This would be very easy to implement.

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



> 
> > 
> > On Fri, Oct 30, 2020 at 06:10:34PM +0100, Paolo Bonzini wrote:
> > > On 29/10/20 23:02, Eduardo Habkost wrote:
> > > > +static Property machine_props[] = {
> > > > +    DEFINE_PROP_STRING("kernel", MachineState, kernel_filename),
> > > > +    DEFINE_PROP_STRING("initrd", MachineState, initrd_filename),
> > > > +    DEFINE_PROP_STRING("append", MachineState, kernel_cmdline),
> > > > +    DEFINE_PROP_STRING("dtb", MachineState, dtb),
> > > > +    DEFINE_PROP_STRING("dumpdtb", MachineState, dumpdtb),
> > > > +    DEFINE_PROP_STRING("dt-compatible", MachineState, dt_compatible),
> > > > +    DEFINE_PROP_STRING("firmware", MachineState, firmware),
> > > > +    DEFINE_PROP_STRING("memory-backend", MachineState, ram_memdev_id),
> > > > +    DEFINE_PROP_END_OF_LIST(),
> > > > +};
> > > > +
> > >
> > > While I think generalizing the _code_ for static properties is obviously
> > > a good idea, I am not sure about generalizing the interface for adding them.
> > >
> > > The reason is that we already have a place for adding properties in
> > > class_init, and having a second makes things "less local", so to speak.
> > 
> > As long as you have the function call to apply the properites array in
> > .class_init, it should be obvious enough what you're doing.
> > 
> > Of course, I think we should refrain from mixing both styles in a single
> > object, but generally speaking the array feels so much better that I
> > don't think we should reject it just because QOM only had a different
> > interface so far (and the property array is preexisting in qdev, too, so
> > we already have differences between objects - in fact, the majority of
> > objects is probably qdev today).
> 
> This is also a strong argument.  The QEMU code base has ~500
> matches for "object*_property_add*" calls, and ~2100 for
> "DEFINE_PROP*".
> 
> Converting qdev arrays to object_class_property_add_*() calls
> would be a huge effort with no gains.  The end result would be
> two different APIs for registering class field properties
> coexisting for a long time, and people still confused on what's
> the preferred API.
> 
> -- 
> Eduardo

-- 
Eduardo