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

[Paolo Bonzini <pbonzini@redhat.com>]

On 01/12/20 17:20, Kevin Wolf wrote:
> Am 30.11.2020 um 20:35 hat Paolo Bonzini geschrieben:
>> For devices it's just the practical issue that there are too many to have
>> something like this series.  For machine types the main issue is that the
>> version-specific machine types would have to be defined in one more place.
>
> Sure, the number of devices means that we can't convert all of them at
> once. And we don't need to, we can make the change incrementally.

There's also the question that most devices are not present in all 
binaries.  So QAPI introspection would tell you what properties are 
supported but not what devices are.  Also the marshaling/unmarshaling 
code for hundreds of devices would bloat the QEMU executables 
unnecessarily (it'd all be reachable from visit_DeviceOptions so it'd 
not be possible to compile it out, even with LTO).

Plus the above issue with machine types.

>>>> 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.
>>>
>>> But isn't it only natural that a list of these objects will exist in
>>> some way, implicitly or explicitly? object-add must somehow decide which
>>> object types it allows the user to create.
>>
>> There's already one, it's objects that implement user creatable.  I don't
>> think having it as a QAPI enum (or discriminator) is worthwhile, and it's
>> one more thing that can go out of sync between the QAPI schema and the C
>> code.
> 
> Well, we all know that this series duplicates things. But at the same
> time, we also know that this isn't going to be the final state.
> 
> Once QAPI learns about QOM inheritance (which it has to if it should
> generate the boilerplate), it will know which objects are user creatable
> without a an explicitly defined separate enum.
> 
> I think it will still need to have the enum internally, but as long as
> it's automatically generated, that shouldn't be a big deal.

Right, I don't want to have the final state now but I'd like to have a 
rough idea of the plan:

1) whether to generate _all_ boilerplate or only properties

2) whether we want to introduce a separation between configuration 
schema and run-time state

3) in the latter case, whether properties will survive at all---iothread 
and throttle-groups don't really need them even if they're writable 
after creation.

These questions have a substantial effect on how to handle this series. 
  Without answers to this questions I cannot understand the long term 
and its interaction with other long term plans for QOM.

>>> A modified QOM might be the right solution, though. I would like to
>>> bring QAPI and QOM together because most of these weaknesses are
>>> strengths of QAPI.
>>
>> I agree wholeheartedly.  But your series goes to the opposite excess.
>> Eduardo is doing work in QOM to mitigate the issues you point out, and you
>> have to meet in the middle with him.  Starting with the user-visible parts
>> focuses on the irrelevant parts.
> 
> QAPI is first and foremost about user-visible parts, and in fact most of
> the problems I listed are about external interfaces.

Yes, but QAPI is also about interfacing with existing code.  Also, QAPI 
does not generate only structs, it also generate C function prototypes. 
I'm not sure whether a QOM object's more similar to the struct case 
(what you do here) or to the QMP command case:

- there's a 1:1 correspondence between ObjectOptions cases and 
ucc->complete implementations

- there's a registry of object types just like there's one for QMP commands.

So another possibility could be that the QAPI generator essentially 
generated the user_creatable_add_type function that called out to 
user-provided functions qom_scsi_pr_helper_complete, 
qom_throttle_group_complete, etc.  The arguments to those functions 
would be the configuration.  That is a very interesting prospect (though 
one that would require changes to the QAPI code generator).

> BlockdevOptions is about external interfaces, not about
> implementation details. Same thing as QOM properties are external
> interfaces, not implementation details. There may be fields in the
> internal state that correspond 1:1 to the externally visible QAPI
> type/QOM property, but it's not necessarily the case.

Right.  It may well be that we decide that, as a result of this series, 
QOM's property interface is declared essentially a failed experiment.  I 
wouldn't be surprised, and that's why I want to understand better where 
we want to go.

For example, Eduardo is focusing specifically on external interfaces 
that correspond 1:1 to the internal implementation.  If we decide that 
non-1:1-mappings and checks on mandatory properties are an important 
part of QOM, that may make large parts of his work moot.  If we decide 
that most QOM objects need no properties at all, then we don't want to 
move more qdev-specific stuff from to QOM.

> QAPI is already here and it's going to stay. QOM doesn't have to
> duplicate input validation that existing code can already perform.
> 
> I'm not sure which complexity you think I'm introducing: QAPI is already
> there. I'm adding the schema, which you agree is valuable documentation,
> so we want to have it either case. The actual change to QOM that we have
> in this series is this:

The complexity is that properties used to be split in two places, and 
now they're split in three places.

It may very well be that this is a good first step to at least have 
classes described in the QAPI schema.  But since _in the short term_ 
there are things that the series makes worse (and has a risk of bringing 
things out of sync), I'd like to understand the long term plan and 
ensure that the QAPI maintainers are on board with it.

Can you at least add a comment to all UserCreatable classes that says 
"if you add a property, remember to modify ... as well in the QAPI schema"?

>> Are there any validation bugs that you're fixing?  Is that
>> something that cannot be fixed elsewhere, or are you papering over bad QOM
>> coding?  (Again, I'm not debating that QOM properties are hard to write
>> correctly).
> 
> Yes, I found bugs that the QAPI schema would have prevented. They were
> generally about not checking whether mandatory propertes are actually
> set.

Okay, I found your series at 
https://patchew.org/QEMU/20201130105615.21799-1-kwolf@redhat.com/ too, 
good to know.

So that's another useful thing that can be chalked to this series at 
least if -object and object_add are converted (and also, another thing 
against QOM properties and 1:1 mappings between configuration schema and 
run-time state).

Paolo