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

[Eduardo Habkost]

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.

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