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

[Eduardo Habkost]

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.

> 
> > I'm not entirely sure what you mean with allow-set. The only things I
> > can find are related to link properties, specifically the check()
> > callback of object_class_property_add_link(). If it's this, what would
> > be the problem with just adding it to DEFINE_PROP_LINK instead of
> > using a separate function call to define link properties?
> 
> Eduardo's series is adding allow-set functions to field properties as well.
> If it's be specified in the function call to
> object_class_add_field_properties, you'd have part of the property described
> in the array and part in the object_class_add_field_properties.
> 
> > > - we would have different ways to handle device field properties (with
> > > dc->props) compared to object properties.
> > 
> > You mean dynamic per-object properties, i.e. not class properties?
> 
> No, I mean that device properties would be handled as
> 
>    dc->props = foo;

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

> 
> while object properties would be handled as
> 
>    object_class_add_field_properties(oc, foo, prop_allow_set_always);
> 
> There would be two different preferred ways to do field properties in qdev
> vs. non-qdev.

They should become exactly the same method, just with a different
allow_set function.

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

> 
> > 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.  Requiring all property descriptions to be
evaluated at compilation time is an intentional feature of the
new API.

> 
> > > The choice to describe class properties as function calls was made in 2016
> > > (commit 16bf7f522a, "qom: Allow properties to be registered against
> > > classes", 2016-01-18); so far I don't see that it has been misused.
> > 
> > This was the obvious incremental step forward at the time because you
> > just had to replace one function call with another function call. The
> > commit message doesn't explain that not using data was a conscious
> > decision. I think it would probably have been out of scope then.
> > 
> > > Also, I don't think it's any easier to write an introspection code 
> > > generator
> > > with DEFINE_PROP_*.  You would still have to parse the class init function
> > > to find the reference to the array (and likewise the TypeInfo struct to 
> > > find
> > > the class init function).
> > 
> > I don't think we should parse any C code. In my opinion, both
> > introspection and the array should eventually be generated by the QAPI
> > generator from the schema.
> 
> That'd be a good plan, and I'd add generating the property description from
> the doc comment.  (Though there's still the issue of how to add non-field
> properties to the introspection.  Those would be harder to move to the QAPI
> generator).
> 
> But at that point the array vs. function call would not change anything (if
> anything the function call would be a tiny bit better), so that's another
> reason to stay with the API that Eduardo has in v2.

I don't agree the function call is a tiny bit better.  In the
best case, I find it a tiny bit worse.

-- 
Eduardo