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

[Kevin Wolf <kwolf@redhat.com>]

Am 30.11.2020 um 15:58 hat Paolo Bonzini geschrieben:
> On 30/11/20 13:25, Kevin Wolf wrote:
> > This series adds a QAPI type for the properties of all user creatable
> > QOM types and finally makes QMP object-add use the new ObjectOptions
> > union so that QAPI introspection can be used for user creatable objects.
> > 
> > After this series, there is least one obvious next step that needs to be
> > done: Change HMP and all of the command line parser to use
> > ObjectOptions, too, so that the QAPI schema is consistently enforced in
> > all external interfaces. I am planning to send another series to address
> > this.
> > 
> > In a third step, we can try to start deduplicating and integrating things
> > better between QAPI and the QOM implementation, e.g. by generating parts
> > of the QOM boilerplate from the QAPI schema.
> 
> 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.

After working through all the user creatable objects, I would say that
separating these from the creation-time options is actually a good thing
because there are basically two types of property setters in the
existing implementations:

1. It starts with something like 'if (completed)' and takes two different
   paths, so they are already separated. Often one path is just
   returning an error, but sometimes we actually make an effort to
   update the internal state according to the new value.

2. No distinction is made. Usually the result is inconsistent state
   because the property values themselves are updated, but they have
   been interpreted once in ucc->complete and are ignored afterwards. Or
   maybe even worse, they are still used, but no care is taken that they
   are consistent with the rest of the internal state.

   Unfortunately my impression is that this is the more common type.

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.

> Instead, you are basically having half of QEMU's backend data model
> into a single struct.
> 
> 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.

We probably need to have it present in the schema in some way so we can
actually check input against the schema. Maybe we can have it
automatically compiled by the QAPI generator so that we don't need to
manually update the enum and the union each time.

In the C source, I guess the other option would be to have pointers
rather than directly embedding all struct types. In the long run this
might make more sense. As long as it's only user-creatable objects, it's
no worse than BlockdevOptions.

> 1) QOM basically does not need properties anymore except for devices and
> machines (accelerators could be converted to QAPI as well). All
> user-creatable objects can be changed to something like chardev's "get a
> struct and use it fill in the fields", and only leave properties to devices
> and machines.

True for those properties that don't support updates after object
creation. For these, leaving the work to QAPI simplifies things a lot.

> 2) User-creatable objects can have a much more flexible schema.  This means
> there's no reason to have block device creation as its own command and
> struct for example.

In theory yes. The block layer isn't really QAPIfied, though, it just
has a QAPI wrapper (similar to how this series doesn't QAPIfy QOM, but
justs wraps it). But for the long term vision, I think it's a reasonable
goal to have block nodes represented as QOM-with-QAPI objects.

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

The only reason why I wouldn't like to wait too long with merging this
is because of merge conflicts (the list of properties or their details
might change and this could go unnoticed).

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.

> 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