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