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

[Paolo Bonzini]

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

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

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

Paolo