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

[Eduardo Habkost]

On Mon, Nov 09, 2020 at 08:27:21PM +0100, Paolo Bonzini wrote:
> On 09/11/20 19:55, Eduardo Habkost wrote:
> > On Mon, Nov 09, 2020 at 06:33:04PM +0100, Paolo Bonzini wrote:
> > > On 09/11/20 18:16, Eduardo Habkost wrote:
> > > > I mean extending the API to let custom setters and getters appear
> > > > on the Property array, not using the existing API.
> > > 
> > > That seems like conflicting goals.  The field property API is based on
> > > getters and setters hidden in PropertyInfo.  The "other" property API is
> > > based on getters and setters in plain sight in the declaration of the
> > > property.
> > 
> > There's nothing that prevents a
> >    void object_class_add_properties(oc, Property *props);
> > function from supporting both.
> 
> Sorry but I don't believe this until I see it.  The two APIs are just too
> different.  And at some point the complexity of DEFINE_PROP becomes:
> 
> 1) harder to document
> 
> 2) just as hard to parse and build a QAPI schema from
> 
> And in the final desired result where QAPI generators are what generates the
> list of properties, it's pointless to shoehorn both kinds of properties in
> the same array if different things can just generate calls to different
> functions.
> 
> > > > 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).
> > > 
> > > Finding the array would also require finding calls buried deep in C code,
> > > wouldn't they?
> > 
> > Yes, but I don't expect this to happen if the API doesn't
> > encourage that.
> 
> Out of 700 calls to object_class_property_add*, there are maybe 5 that are
> dynamic.  So on one hand I understand why you want an API that makes those
> things harder, but on the other hand I don't see such a big risk of misuse,
> and it won't even matter at all if we later end up with properties described
> in a QAPI schema.
> 
> > > > (Also, I don't think we should allow handwritten Property literals.)
> > > 
> > > How would you do custom setters and getters then---without separate
> > > PropertyInfos, without Property literals, and without an exploding number 
> > > of
> > > macros?
> > 
> > Prop with no struct field, and custom setter/getter:
> > 
> >    DEFINE_PROP("myproperty", prop_type_uint32,
> >                .custom_getter = my_getter,
> >                .custom_setter = my_setter)
> 
> It would have to use all the Visitor crap and would be even harder to use
> than object_class_property_add_str.  Thanks but no thanks. :)

Point taken, I dislike the visitor API too.

> 
> > > > we can't be sure the [set of QOM properties]
> > > > doesn't depend on configure flags or run time
> > > > checks inside class_init.
> > > 
> > > We can use grep or Coccinelle or manual code review to identify 
> > > problematic
> > > cases.
> > 
> > We can, but I believe it is better and simpler to have an API
> > that enforces (or at least encourages) this.
> 
> I don't see how
> 
>     if (...) {
>         object_class_add_field_properties(oc, props);
>     }
> 
> is discouraged any more than
> 
>     if (...) {
>         object_class_add_field_property(oc, "prop1",
>                                         PROP_STRING(...));
>         object_class_add_field_property(oc, "prop2",
>                                         PROP_STRING(...));
>         object_class_add_field_property(oc, "prop3",
>                                         PROP_STRING(...));
>         object_class_add_field_property(oc, "prop4",
>                                         PROP_STRING(...));
>     }
> 
> (If anything, the former is more natural and less ugly than the latter).

On the former, "adding a new property" means adding an entry to a
const array.  On the latter, it means adding a new function call.

On the former, a conditional property would require defining a
new array.  A non-constant property name or type would require
making the array non-const and modifying it at runtime.

On the latter, adding a if statement on the front of that
function call or a non-constant expression as argument to the
function is trivial.

> 
> > I'd like us to convert instance-level properties to an API that
> > is easy to use and where the same problems won't happen again.
> 
> I agree.  I just don't think that arrays are enough to make sure the same
> problems won't happen again.
> 
> > > > 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.
> > > 
> > > I would like an example of the complexity of those code paths.  I don't 
> > > see
> > > much complexity, as long as the object exists at all, and I don't see how 
> > > it
> > > would be simpler to find the code paths that lead to
> > > object_class_add_field_properties.
> > 
> > Possibly the most complex case is x86_cpu_register_bit_prop().
> > The qdev_property_add_static() calls at arm_cpu_post_init() are
> > tricky too.
> 
> The problem with those code paths is that there's a reason why they look
> like they do.  For x86_cpu_register_feature_bit_props, for example either
> you introduce duplication between QOM property definitions and feat_names
> array, or you resort to run-time logic like that.
> 
> If you want to make those properties introspectable (i.e. known at
> compilation time) you wouldn't anyway use DEFINE_PROP*, because it would
> cause duplication.  Instead, you could have a plug-in parser for qapi-gen,
> reading files akin to target/s390x/cpu_features_def.h.inc. The parser would
> generate both QAPI schema and calls to x86_cpu_register_bit_prop().
> 
> To sum up: for users where properties are heavily dependent on run-time
> logic, the solution doesn't come from providing a more limited API.  A
> crippled API will simply not solve the problem that prompted the usage of
> run-time logic, and therefore won't be used.

I don't know yet what's the best solution for the x86 feature
case.  Maybe duplicating the list of feature names would be a
small price to pay to get a static list of properties defined at
compilation time?  Maybe we can replace
FeatureWordInfo.feat_names[] with property introspection code
that will find the property name for a given struct field?

In either case, we need something that works for x86 and other
complex cases, or it won't be used.  Point taken.

> 
> (I don't know enough of the ARM case to say something meaningful about it).
> 
> > If object*_property_add*() is hidden behind a function call or a
> > `if` statement, it's already too much complexity to me.
> 
> You want to remove hiding behind a function call, but why is it any better
> to hide behind layers of macros?  Just the example you had in your email
> included DEFINE_PROP, DEFINE_FIELD_PROP, DEFINE_PROP_UINT32.  It's still
> impossible to figure out without either parsing or executing C code.

Because we can be absolutely sure the macros (and the property
array) will be constant expressions evaluated at compilation
time.

                             * * *

Anyway, If we are the only ones discussing this, I will just
defer to your suggestions as QOM maintainer.  I hope we hear from
others.

-- 
Eduardo