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

[Eduardo Habkost <ehabkost@redhat.com>]

On Mon, Nov 09, 2020 at 05:34:01PM +0100, Paolo Bonzini wrote:
> 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

I mean extending the API to let custom setters and getters appear
on the Property array, not using the existing API.

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

That's a possibility, and maybe that could be the default
allow_set behavior.  It may be a bit tricky to convert the
existing backend objects to the new behavior in a gradual way,
though.

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

Parsing an array containing a handful of macros (a tiny subset of
C) isn't even comparable to parsing and executing C code where
object*_property_add*() calls can be buried deep in many levels
of C function calls (which may or may not be conditional).

(Also, I don't think we should allow handwritten Property literals.)

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

If we do that with the existing code, we can't be sure the
generated schema doesn't depend on configure flags or run time
checks inside class_init.  Even locating the cases where this is
happening is being a challenge because the API is too flexible.

However, if we require the property list to be always evaluated
at compile time, we can be sure that this method will be
reliable.

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

The list of tricky dynamic properties is large and I don't think
we even found all cases yet.  John documented many of them here:

https://gitlab.com/jsnow/qemu/-/blob/cli_audit/docs/cli_audit.md

Look, for example, for the sections named "Features" for CPU
options.


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

You are excluding instance-level object_property_add*() calls.
Most of them are supposed to be class properties but were never
converted.

You are also ignoring the complexity of the code path that leads
to the object*_property_add*() calls, which is the main problem
on most cases.

-- 
Eduardo