Re: [RFC PATCH 00/12] QOM/QAPI integration part 1

[Kevin Wolf]

Am 14.12.2021 um 15:45 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 23.11.2021 um 17:05 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> 
> >> > This series adds QOM class definitions to the QAPI schema, introduces
> >> > a new TypeInfo.instance_config() callback that configures the object at
> >> > creation time (instead of setting properties individually) and is
> >> > separate from runtime property setters (which often used to be not
> >> > really tested for runtime use), and finally generates a marshalling
> >> > function for .instance_config() from the QAPI schema that makes this a
> >> > natural C interface rather than a visitor based one.
> >> >
> >> > This is loosely based on Paolo's old proposal in the wiki:
> >> > https://wiki.qemu.org/Features/QOM-QAPI_integration
> >> >
> >> > The series is in a rather early stage and I don't really have any
> >> > automated tests or documentation in this series yet. I'm also only
> >> > converting the class hierarchy for the random number generator backends
> >> > to show what the result looks like, the other objects still need to be
> >> > done.
> >> >
> >> > So the question to you isn't whether this is mergeable (it isn't), but
> >> > whether you think this is the right approach for starting to integrate
> >> > QOM and QAPI better.
> >> >
> >> > You'll also see that this doesn't really remove the duplication between
> >> > property definitions in the code and configuration struct definitions in
> >> > the QAPI schema yet (because we want to keep at least a read-only
> >> > runtime property for every configuration option), but at least they mean
> >> > somewhat different things now (creation vs. runtime) instead of being
> >> > completely redundant.
> >> >
> >> > Possible future steps:
> >> >
> >> > * Define at least those properties to the schema that correspond to a
> >> >   config option. For both setters and getters for each option, we'll
> >> >   probably want to select in the schema between 'not available',
> >> >   'automatically generated function' and 'manually implemented'.
> >> >
> >> >   Other runtime properties could be either left in the code or added to
> >> >   the schema as well. Either way, we need to figure out how to best
> >> >   describe these things in the schema.
> >> 
> >> Permit me a diversion of sorts.
> >> 
> >> With QOM, we have properties.  A property is readable if it has a
> >> getter, writable if it has a setter.  There is no real concept of
> >> configuration vs. state.  Writable properties can be written at any
> >> time.
> >> 
> >> In practice, some properties are to be used only like configuration, and
> >> we check configuration at realize time (for devices), or by a surrogate
> >> like qemu_add_machine_init_done_notifier().  If you set them later,
> >> things may break, and you get to keep the pieces.
> >> 
> >> In this "QOM/QAPI integration part 1", configuration (expressed in QAPI
> >> schema) makes it into QOM.
> >> 
> >> Now we have configuration *and* properties.
> >> 
> >> Do we need the properties?
> >
> > Configuration is for creating objects, properties are for runtime after
> > the creation. So for the practical answer, as long as you can find a QOM
> > type that wants to allow either changing an option at runtime or just
> > exposing its current value, I would say, yes, we need both. And I can
> > easily list some QOM types that do.
> >
> > The theoretical answer is that of course you can replace properties with
> > custom query-* and set-* QMP commands, but that's not only hardly an
> > improvment, but also a compatibility problem.
> 
> That would be nuts.
> 
> > The approach I'm taking here with QAPIfication of objects (and planning
> > to take for future conversions) is to drop setters that can't work at
> > runtime (which might be the majority of properties), but keep properties
> > around otherwise. Everything else would be a per-object decision, not
> > part of the infrastructure work.
> 
> Getting rid of such setters makes sense.
> 
> It's been a while since I reviewed...  I don't remember anymore whether
> we can have configuration parameters that are also properties.  If yes,
> would it make sense to generate such properties?

It's not only possible, but the normal case. These properties are often
read-only after realize, but with existing types, we (almost?) always
allow querying properties later even if they can be set only before
realize (and therefore become part of the configuration after this
series).

As for generating, yes, I think that we do that at least partially
(setters will probably have to be manually implemented in the common
case). This is in fact the very proposal in the original cover letter
that you replied to here.

> >> Note I'm not asking whether we need setters.  I'm asking whether we
> >> need to expose configuration bits via qom-set & friends in addition to
> >> the QAPI schema and query-qmp-schema.
> >
> > I'm not sure I follow here. How is querying or changing option values
> > redundant with querying which options exist?
> >
> > Maybe qom-list could become obsolete if we move all properties (and not
> > just the configuration) into the QAPI schema, but I don't see qom-get
> > and qom-set going away.
> >
> >> > * Getting rid of the big 'object-add' union: While the union is not too
> >> >   bad for the rather small number of user-creatable objects, it
> >> >   wouldn't scale at all for devices.
> >> >
> >> >   My idea there is that we could define something like this:
> >> >
> >> >   { 'struct': 'ObjectOptions',
> >> >     'data': {
> >> >         'id': 'str',
> >> >         'config': { 'type': 'qom-config-any:user-creatable',
> >> >                     'embed': true } } }
> >> >
> >> >   Obviously this would be an extension of the schema language to add an
> >> >   'embed' option (another hopefully more acceptable attempt to flatten
> >> >   things...), so I'd like to hear opinions on this first before I go to
> >> >   implement it.
> >> 
> >> 'embed': true would splice in the members of a struct type instead of a
> >> single member of that struct type.  Correct?
> >> 
> >> Stretch goal: make it work for union types, too :)
> >> 
> >> I've thought of this before.  Plenty of nesting in the wire format
> >> exists pretty much only to let us have the C structs we want.  Right
> >> now, the only way to "splice in" such a struct is the base type.
> >> General splicing could be useful.  It may take an introspection flag
> >> day.
> >
> > Base types aren't visible in the introspection either, so probably not
> > if you continue to just report the resulting structure?
> 
> Yes, this should be feasible, except for splicing a union into a union,
> because then you get multiple (tag, variants), which the introspection
> schema can't do.  So don't go there, at least for now.

Right. Let's think about that when we have a use case for splicing
unions into unions.

Kevin