Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini <pbonzini@redhat.com>]

On 30/11/20 16:46, Kevin Wolf wrote:
> Am 30.11.2020 um 15:58 hat Paolo Bonzini geschrieben:
>> With this series it's basically pointless to have QOM properties at
>> all.
> 
> Not entirely, because there are still some writable properties that can
> be changed later on.

Are there really any (that are not bugs like opened/loaded)? That's also 
why Eduardo and I discussed a class-wide allow_set function for his 
field properties series.

> So with this in mind, I think I'm in favour of completely leaving the
> initialisation of properties on object creation to QAPI, and only
> providing individual setters if we actually intend to allow property
> changes after creation.

The main problem is that it wouldn't extend well, if at all, to machines 
and devices.  So those would still not be integrated into the QAPI schema.

>> So the question is, are we okay with shoveling half of QEMU's backend data
>> model into a single struct?  If so, there are important consequences.
> 
> Yeah, the single struct bothers me a bit, both in the QAPI schema and in
> the C source.

The single struct doesn't bother me _too much_ actually.  What bothers 
me is that it won't be a single source of all QOM objects, only those 
that happen to be created by object-add.  So I start to wonder if QOM as 
it exists now is the right solution for all kind of objects:

- backends and other object-add types (not per-target, relatively few 
classes and even fewer hierarchies)

- machine (per-target, many classes, no hierarchy)

- device (can be treated as not per-target, many many classes, very few 
hierarchies)

- accelerator (per-target, few classes, no hierarchy)

- chardev (ok those are the same as the first category)

If QOM is the right solution, this patch goes in the wrong direction.

If QOM is the wrong solution, this patch is okay but then we have 
another problem to solve. :)

>> The problem with this series is that you are fine with deduplicating things
>> as a third step, but you cannot be sure that such deduplication is possible
>> at all.  So while I don't have any problems in principle with the
>> ObjectOptions concept, I don't think it should be committed without a clear
>> idea of how to do the third step.
> 
> Do you have any specific concerns why the deduplication might not
> possible, or just because it's uncharted territory?

Mostly because it's more or less the same issue that you have with 
BlockdevOptions, with the extra complication that this series only deals 
with the easy one of the four above categories.

> Maybe if we don't want to commit to keeping the ObjectOptions schema,
> the part that should wait is object-add and I should do the command line
> options first? Then the schema remains an implementation detail for now
> that is invisible in introspection.

I don't see much benefit in converting _any_ of the three actually.  The 
only good reason I see for QAPIfying this is the documentation, and the 
promise of deduplicating QOM boilerplate.  The latter is only a promise, 
but documentation alone is a damn good reason and it's enough to get 
this work into a mergeable shape as soon as possible!

But maybe, we could start in the opposite direction: start with the use 
QAPI to eliminate QOM boilerplate.  Basing your work on Eduardo's field 
properties series, you could add a new 'object' "kind" to QAPI that 
would create an array of field properties (e.g. a macro expanding to a 
compound literal?)
.  Something like


+{ 'object': 'InputBarrier',
+  'data': { 'name': 'str',
+            'x-origin': 'int16',
+            'y-origin': 'int16',
+            'width': 'int16',
+            'height': 'int16' },
+  'properties': { 'server': 'str',
+                  'port': 'str' } }

would create a macro QOM_InputBarrier_FIELDS defining properties for the 
following fields of the InputBarrier struct:

     gchar *name;
     int16_t x_origin, y_origin;
     int16_t width, height;

while server and port would only appear in the documentation (or 
alternatively you could leave them out completely, as you wish).
The advantages would be noticeable:

1) the information would be moved in the QAPI schema JSON from the 
beginning, decoupling the conflict-heavy part from the complex question 
of how to expose the QOM schema in the introspection data

2) there would not be any more duplication than before (there would be 
duplication between structs and QAPI schema, but not between structs and 
C code that defines properties).

3) it would be opt-in, so it doesn't put on you the burden of keeping 
the series in sync with new objects that are added (I have one for the 
qtest server for example).  At the same time it would be quite appealing 
for owners of QOM code to convert their objects to field properties and 
get documentation for free.

4) we could special-case 'object' definitions and generate them in the 
system manual as well, since they are also useful to document -object.

Yes it's a huge change but you have the boring part already done.  What 
do you think?

Paolo

>> In the meanwhile, of course I have no problem with deprecating the opened
>> and loaded properties.
> 
> If we decide that we don't want to have the schema at all (which I hope
> we won't decide), I can split the deprecation into separate patches.
> 
> Kevin
>