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

[Paolo Bonzini]

On 09/11/20 16:21, Eduardo Habkost wrote:
> On Mon, Nov 09, 2020 at 03:15:26PM +0100, Paolo Bonzini wrote:
> > On 09/11/20 12:34, Kevin Wolf wrote:
> > > > 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?
> >
> > Because some properties would not be field properties, for example.  For
> > example, any non-scalar property would need to invoke visit_SomeQapiStruct
> > manually and would not be a field property.
>
> Nothing prevents us from describing those properties inside the
> same property array.

Do you mean adding PropertyInfos for them?  Adding a once-only 
PropertyInfo is worse than writing a custom getter/setter pair, because:

- without (DEFINE_)PROP_* you lose the type safety.

- with (DEFINE_)PROP_* you have much more boilerplate to write

> More precisely, it is
>    device_class_set_props(dc, foo);
>
> which is supposed to become a one-line wrapper to
> object_class_add_field_properties().

You're right, I'm a few years late.  So that objection is withdrawn.

> (There's also the possibility we let the class provide a default
> allow_set function, and both would become 100% the same)

That's a possibility too.  Though if we ever have a need for multiple 
allow_set functions it would be somewhat complicated to add it back.

Instead of class-wide allow_set, we might as well have a "bool 
constructed" field in Object and remove the function pointer altogether: 
just replace prop->allow_set(obj) with just "!obj->constructed".

> > > 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).
> >
> > Right, but qdev's DEFINE_PROP_STRING would be easy to change to something
> > like
> >
> > - DEFINE_PROP_STRING("name", ...),
> > + device_class_add_field_property(dc, "name", PROP_STRING(...));
>
> I'm not worried about this direction of conversion (which is
> easy).  I'm worried about the function call => QAPI schema
> conversion.  Function calls are too flexible and requires parsing
> and executing C code.

Converting DEFINE_PROP_STRING to a schema also requires parsing C code, 
since you can have handwritten Property literals (especially for custom 
PropertyInfo).  Converting DEFINE_PROP_STRING it also requires matching 
the array against calls to object_class_add_field_properties (which 
could be hidden behind helpers such as device_class_set_props).  (Plus 
matching class_init functions against TypeInfo).

So, you don't save any parsing by using arrays.  (In fact I would 
probably skip the parsing, and use your suggestion of *executing* C 
code: write the QAPI schema generator in C, link into QEMU and run it 
just once to generate the QOM schema).

QOM has been using function calls for many years, are there any cases of 
misuse of that flexibility that you have in mind?  I can only think of 
two *uses*, in fact.  One is eepro100_register_types is the only case I 
can remember where types are registered dynamically.  The other is S390 
CPU features.  In fact,

  $ git grep \ object_class_property_add|grep -v '([a-z0-9_]*, \"'

shows some cases where property names are macros (an mst-ism :), but no 
other case where properties are being defined dynamically.

Paolo