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

[Paolo Bonzini <pbonzini@redhat.com>]

On 09/11/20 18:16, Eduardo Habkost wrote:
> On Mon, Nov 09, 2020 at 05:34:01PM +0100, Paolo Bonzini wrote:
>> On 09/11/20 16:21, Eduardo Habkost wrote:
>>> Nothing prevents us from describing those properties inside the
>>> same property array.
>>
>> Do you mean adding PropertyInfos for them?  Adding a once-only PropertyInfo
>> is worse than writing a custom getter/setter pair, because:
>>
>> - without (DEFINE_)PROP_* you lose the type safety.
>>
>> - with (DEFINE_)PROP_* you have much more boilerplate to write
> 
> 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.

>>>>> 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.
>>
>> Converting DEFINE_PROP_STRING to a schema also requires parsing C code,
>> since you can have handwritten Property literals (especially for custom
>> PropertyInfo).  Converting DEFINE_PROP_STRING it also requires matching the
>> array against calls to object_class_add_field_properties (which could be
>> hidden behind helpers such as device_class_set_props).  (Plus matching
>> class_init functions against TypeInfo).
> 
> 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?

> (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?

>> So, you don't save any parsing by using arrays.  (In fact I would probably
>> skip the parsing, and use your suggestion of *executing* C code: write the
>> QAPI schema generator in C, link into QEMU and run it just once to generate
>> the QOM schema).
> 
> If we do that with the existing code, we can't be sure the
> generated schema 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.

> Even locating the cases where this is
> happening is being a challenge because the API is too flexible.
> 
> However, if we require the property list to be always evaluated
> at compile time, we can be sure that this method will be
> reliable.
> 
>> QOM has been using function calls for many years, are there any cases of
>> misuse of that flexibility that you have in mind?  I can only think of two
>> *uses*, in fact.  One is eepro100_register_types is the only case I can
>> remember where types are registered dynamically.  The other is S390 CPU
>> features.  [...]
> 
> The list of tricky dynamic properties is large and I don't think
> we even found all cases yet.  John documented many of them here:
> 
> https://gitlab.com/jsnow/qemu/-/blob/cli_audit/docs/cli_audit.md
> 
> Look, for example, for the sections named "Features" for CPU
> options.

Yes, I'm only considering object_class_property calls.  Those are the 
ones that I claim aren't being misused enough for this to be a problem.

Making instance-level properties appear in the schema is a completely 
different kind of conversion, because there is plenty of manual work 
(and unsolved problems for e.g. subobject property aliases).

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

Paolo