[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Paolo Bonzini
Subject: Re: [PATCH v2 00/44] Make qdev static property API usable by any QOM type
Date: Mon, 9 Nov 2020 17:34:01 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0

On 09/11/20 16:21, Eduardo Habkost wrote:
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.

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

More precisely, it is
   device_class_set_props(dc, foo);

which is supposed to become a one-line wrapper to

You're right, I'm a few years late.  So that objection is withdrawn.

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

That's a possibility too. Though if we ever have a need for multiple allow_set functions it would be somewhat complicated to add it back.

Instead of class-wide allow_set, we might as well have a "bool constructed" field in Object and remove the function pointer altogether: just replace prop->allow_set(obj) with just "!obj->constructed".

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

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

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

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. In fact,

  $ git grep \ object_class_property_add|grep -v '([a-z0-9_]*, \"'

shows some cases where property names are macros (an mst-ism :), but no other case where properties are being defined dynamically.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]