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
