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

From: Eduardo Habkost <ehabkost@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>,
	lvivier@redhat.com, thuth@redhat.com, pkrempa@redhat.com,
	berrange@redhat.com, qemu-block@nongnu.org,
	libvir-list@redhat.com, armbru@redhat.com, jasowang@redhat.com,
	qemu-devel@nongnu.org, mreitz@redhat.com, kraxel@redhat.com
Subject: Re: [PATCH 00/18] qapi/qom: QAPIfy object-add
Date: Tue, 1 Dec 2020 17:08:54 -0500	[thread overview]
Message-ID: <20201201220854.GC3836@habkost.net> (raw)
In-Reply-To: <3449b5d6-d094-84c8-a0ea-4cd25364db2d@redhat.com>

On Tue, Dec 01, 2020 at 10:23:57PM +0100, Paolo Bonzini wrote:
> On 01/12/20 20:35, Kevin Wolf wrote:
> > Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben:
> > I don't think this is actually a new things. We already have types and
> > commands declared with things like 'if': 'defined(TARGET_S390X)'.
> > As far as I understand, QAPI generated files are already built per
> > target, so not compiling things into all binaries should be entirely
> > possible.
> 
> There is some complication due to having different discriminators per
> target.  So yes it should be possible.  But probably best left after objects
> because it's so much bigger a task and because objects have a bit more
> freedom for experimentation (less ties to other qdev-specific concepts, e.g.
> the magic "bus" property).
> 
> > So maybe only the abstract base class that actually defines the machine
> > properties (like generic-pc-machine) should be described in QAPI, and
> > then the concrete machine types would inherit from it without being
> > described in QAPI themselves?
> 
> Yes, maybe.
> 
> > > 1) whether to generate _all_ boilerplate or only properties
> > 
> > I would like to generate as much boilerplate as possible. That is, I
> > don't want to constrain us to only properties, but at the same time, I'm
> > not sure if it's possible to get rid of all boilerplate.
> > 
> > Basically, the vision I have in mind is that QAPI would generate code
> > that is the same for most instances, and then provide an option that
> > prevents code generation for a specific part for more complicated cases,
> > so that the respective function can (and must) be provided in the C
> > source.
> 
> Ok, so that's a bit different from what I am thinking of.  I don't care very
> much about the internal boilerplate, only the external interface for
> configuration.  So I don't care about type registration, dynamic cast macros
> etc., only essentially the part that leads to ucc->complete.
> 
> > > 2) whether we want to introduce a separation between configuration
> > > schema and run-time state
> > 
> > You mean the internal run-time state? How is this separation not already
> > present with getter/setter functions for each property? In many cases
> > they just directly access the run-time state, but there are other cases
> > where they actually do things.
> 
> I mean moving more towards the blockdev/chardev way of doing things,
> increasing the separation very much by having separate configuration structs
> that have (potentially) no link to the run-time state struct.
> 
> > > 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.
> > 
> > How do you define properties, i.e. at which point would they stop
> > existing and what would be a non-property alternative?
> 
> Properties are only a useful concept if they have a use.  If
> -object/object_add/object-add can do the same job without properties,
> properties are not needed anymore.

Do you mean "not needed for -object anymore"?  Properties are
still used by internal C code (esp. board code),
-device/device_add, -machine, -cpu, and debugging commands (like
"info qtree" and qom-list/qom-get/qom-set).

> 
> Right now QOM is all about exposing properties, and having multiple
> interfaces to set them (by picking a different visitor).  But in practice
> most QOM objects have a lifetime that consists of 1) set properties 2) flip
> a switch (realized/complete/open) 3) let the object live on its own.  1+2
> are a single monitor command or CLI option; during 3 you access the object
> through monitor commands, not properties.

I agree with this, except for the word "all" in "QOM is all
about".  QOM is also an extensively used internal QEMU API,
including internal usage of the QOM property system.

> 
> > So in summary, it seems to me that the QOM way is more flexible because
> > you can get both models out of it. Whether we actually need this
> > flexibility I can't say.
> 
> I'm thinking there's no need for it, but maybe I'm overly optimistic.
> 
> > * Configuration options are described in the QAPI schema. This is mainly
> >    for object creation, but runtime modifiable properties are a subset of
> >    this.
> > 
> > * Properties are generated for each option. By default, the getter
> >    just returns the value from the configuration at creation time, though
> >    generation of it can be disabled so that it can be overridden. Also,
> >    setters just return an error by default.
> > 
> > * Property setters aren't called for object creation. Instead, the
> >    relevant ObjectOptions branch is made available to some init method.
> > 
> > * Runtime modifiable properties (declared as such in the schema) don't
> >    get the default setter, so you have to provide an implementation for
> >    them.
> 
> I wouldn't bother with properties at all in the QAPI schema.  Just do the
> first and third bullet.  Declaring read-only QOM properties is trivial.

I'm liking the direction this is taking.  However, I would still
like to have a clearer and feasible plan that would work for
-device, -machine, and -cpu.

> 
> > So while this series is doing only one part of the whole solution, that
> > the second part is missing doesn't make the first part wrong.
> 
> Yeah, I think it's clear that for the long term we're not really disagreeing
> (or perhaps I'm even more radical than you :)).  I'm just worried about
> having yet another incomplete transition.
> 
> > One possibly nasty detail to consider there is that we sometimes declare
> > the USER_CREATABLE interface in the base class, so ucc->complete is for
> > the base class rather than the actually instantiated class. If we only
> > instantiate leaf classes (I think this is true), we can move
> > USER_CREATABLE there.
> 
> You can also use a while loop covering each superclass to decide how to
> dispatch ucc->complete.  I don't care much about these details, they're
> Simple Matter Of Programming. :)
> 
> > I also had in mind just passing the whole configuration struct
> > (essentially always 'boxed': true), but you're right that individual
> > parameters like for commands would be possible. I'm not entirely
> > convinced that they would be better (there was a reason why we
> > introduced 'boxed': true), but it's an option.
> 
> Having 'boxed': true by default would be just an implementation choice,
> nothing to worry about.  (When I said the arguments would be the
> configuration, having a boxed struct as the argument would fit the
> description just as well).
> 
> > I was hoping that by converting object-add in this series, and the CLI
> > options soon afterwards, it would be very obvious if you forget to
> > change the schema because your new property just wouldn't work (at least
> > not during creation).
> 
> Converting the CLI options is not entirely trivial due to -readconfig and
> friends, so I was expecting that to last until that part of my 6.0 keyval
> work goes in.  (It's almost ready for posting BTW,
> https://gitlab.com/bonzini/qemu/-/commit/b59288c86c).
> 
> As soon as we have an idea of what we want UserCreatable to look in the end,
> on both the QAPI side and the object implementation side.  That's also the
> part where we have the biggest need to document the schema. With that part
> at least roughly sketched out (no code needed), I'm okay with this series
> going in.
> 
> I still don't like the triplication, but as George Michael puts it I just
> gotta have faith---because I must admit, I'm positively surprised at the
> ideas that came out of the discussion.
> 
> Paolo
> 

-- 
Eduardo