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

[Kevin Wolf]

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.

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.

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

Kevin