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.

* 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.

  Also note that 'qom-config-any:user-creatable' is new, too. The
  'qom-config:...' types introduced by this series don't work for
  subclasses, but only for the exact class.

  On the external interface, the new 'qom-config-any:...' type including
  subclasses would basically behave (and be introspected) like the union
  we have today, just without being defined explicitly.

  As for the C representation for configurations that include
  subclasses, I'm imagining a struct that just contains the qom_type
  string (so we can call the right handler) and a pointer to the real
  config (that is treated as opaque by the generic code).

I could probably add more, but let's just start with this for discussion
now.

Kevin Wolf (12):
  qapi: Add visit_next_struct_member()
  qom: Create object_configure()
  qom: Make object_configure() public
  qom: Add instance_config() to TypeInfo
  rng-random: Implement .instance_config
  rng-backend: Implement .instance_config
  qapi: Allow defining QOM classes
  qapi: Create qom-config:... type for classes
  qapi/qom: Convert rng-backend/random to class
  qapi: Generate QOM config marshalling code
  qapi/qom: Add class definition for rng-builtin
  qapi/qom: Add class definition for rng-egd

 qapi/qom.json                |  46 ++++++++++-----
 include/qapi/visitor-impl.h  |   3 +
 include/qapi/visitor.h       |   2 +
 include/qom/object.h         |   7 +++
 backends/rng-egd.c           |  18 +++---
 backends/rng-random.c        |  18 +++---
 backends/rng.c               |  22 ++++++--
 qapi/qapi-visit-core.c       |   6 ++
 qapi/qobject-input-visitor.c |  16 ++++++
 qom/object.c                 |  32 +++++++++++
 qom/object_interfaces.c      |  22 +-------
 scripts/qapi/expr.py         |  28 ++++++++-
 scripts/qapi/main.py         |   2 +
 scripts/qapi/qom.py          |  91 ++++++++++++++++++++++++++++++
 scripts/qapi/schema.py       | 106 +++++++++++++++++++++++++++++++++++
 qapi/meson.build             |   3 +
 qapi/trace-events            |   1 +
 17 files changed, 362 insertions(+), 61 deletions(-)
 create mode 100644 scripts/qapi/qom.py

-- 
2.31.1

On 11/3/21 18:29, Kevin Wolf wrote:
> 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.

That's pretty cool!

Just one question: why not always use boxed configuration?  It should 
not make the instance_config types any larger, and it avoids unwieldy 
argument lists.

Also, for the obligatory bikeshedding remark, do you have any other 
plans or ideas for the colon-separated auto generated typenames?  Having 
both the "namespace" (qom) and the more specific use (config) before the 
classname is a bit weird, compared to the existing structs like 
RngRandomProperties.  Especially if boxed config is more used (or 
becomes the only possibility), it might be that qom:class-name:config, 
or even just class-name:config, make for nicer code in the object 
implementation.

Paolo

Am 03.11.2021 um 22:26 hat Paolo Bonzini geschrieben:
> On 11/3/21 18:29, Kevin Wolf wrote:
> > 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.
> 
> That's pretty cool!
> 
> Just one question: why not always use boxed configuration?  It should not
> make the instance_config types any larger, and it avoids unwieldy argument
> lists.

Basically for the same reason as for commands (and for consistency with
commands): If you have only one or two options, then creating a separate
type for them feels just a little over the top, and boxing doesn't work
with implicit types.

I really like the concise definitions without a separate struct like in:

{ 'class': 'rng-egd',
  'parent': 'rng-backend',
  'config': { 'chardev': 'str' } }

Though maybe we could make it work by giving the implicit type another
prefixed name?

> Also, for the obligatory bikeshedding remark, do you have any other plans or
> ideas for the colon-separated auto generated typenames?  Having both the
> "namespace" (qom) and the more specific use (config) before the classname is
> a bit weird, compared to the existing structs like RngRandomProperties.
> Especially if boxed config is more used (or becomes the only possibility),
> it might be that qom:class-name:config, or even just class-name:config, make
> for nicer code in the object implementation.

'qom-config:classname' isn't a type that is useful for the object
implementations at all. Its only use is really storing the whole
configuration temporarily in a QAPI C struct before applying it.

The class implementations always want to store only their "local" config
options, but 'qom-config:classname' contains those of the parent class
as well.

In my example conversion, I left RngProperties as a separate type that
is just referenced in the class definition:

{ 'class': 'rng-backend',
  'config': 'RngProperties' }

This is what you would do when rng-backend wants to store its options in
a single struct, and then it's still RngProperties.

As for the naming, I chose a prefix because I think QOM doesn't restrict
the characters used in class names, so suffixes could become ambiguous.
The colon is just a character that is invalid in QAPI user types and
looked nice to me, any other reserved character would do.

Oh, and I also wanted to say something about why not just directly using
the class name, which was my first idea: 'foo': 'iothread' looks more
like referencing an existing iothread rather than the configuration for
a new one. I wanted to leave us the option that we could possibly later
take a string for such options (a QOM path) and then pass the referenced
object to QMP commands as the proper QOM type.

(Maybe there aren't currently many commands that take specific QOM
objects, but I'm dreaming of a future state with QMP commands that take
'block-node' arguments etc.)

Kevin

On 11/4/21 10:07, Kevin Wolf wrote:
>> Also, for the obligatory bikeshedding remark, do you have any other plans or
>> ideas for the colon-separated auto generated typenames?  Having both the
>> "namespace" (qom) and the more specific use (config) before the classname is
>> a bit weird, compared to the existing structs like RngRandomProperties.
>> Especially if boxed config is more used (or becomes the only possibility),
>> it might be that qom:class-name:config, or even just class-name:config, make
>> for nicer code in the object implementation.
> 
> 'qom-config:classname' isn't a type that is useful for the object
> implementations at all. Its only use is really storing the whole
> configuration temporarily in a QAPI C struct before applying it.

<rubbish>
It would be useful as the (auto-boxed) argument of the configuration 
code.  So basically you'd not need the RngProperties function anymore 
because you use something like QomConfigRngBackend (or 
RngBackendQomConfig - hence the question on how to name the 
auto-generated types).
</rubbish>

> The class implementations always want to store only their "local" config
> options, but 'qom-config:classname' contains those of the parent class
> as well.

Ah, I didn't understand that (hence the rubbish tag above).  It makes 
sense given that instance_config is called per-class while ObjectOptions 
stores all the info in one class.  That's a major change from my sketch, 
which planned to call the base class config function by hand (and handle 
the marshalling via QAPI 'base': '...').

> Oh, and I also wanted to say something about why not just directly using
> the class name, which was my first idea: 'foo': 'iothread' looks more
> like referencing an existing iothread rather than the configuration for
> a new one. I wanted to leave us the option that we could possibly later
> take a string for such options (a QOM path) and then pass the referenced
> object to QMP commands as the proper QOM type.

I agree that 'iothread' is going to be confusing when you're referring 
to the configuration.

Anyway I'm totally fine with 'qom-config:classname'.  Given this 
explanation, however, one alternative that makes sense could be 
'classname:full-config'. Then you could use 'classname:config' for the 
autoboxed configs---and reserve 'classname' to mean the pointer to an 
object.  Classes are (generally) lowercase and QAPI structs are 
CamelCase, so there is not much potential for collisions.

Paolo

Am 04.11.2021 um 13:39 hat Paolo Bonzini geschrieben:
> On 11/4/21 10:07, Kevin Wolf wrote:
> > The class implementations always want to store only their "local" config
> > options, but 'qom-config:classname' contains those of the parent class
> > as well.
> 
> Ah, I didn't understand that (hence the rubbish tag above).  It makes sense
> given that instance_config is called per-class while ObjectOptions stores
> all the info in one class.  That's a major change from my sketch, which
> planned to call the base class config function by hand (and handle the
> marshalling via QAPI 'base': '...').

Yeah, handling inheritance and how to represent things in the schema is
probably the two more interesting things this series changes compared to
your proposal.

I started with your model, but it just didn't work out nicely, because I
always had the full configuration in the child class and apart from just
being ugly, having all options of the parent class duplicated, but
ignored, would certainly be a source for a lot of confusion and bugs.

It took me a while to figure out how to deal with this, but I'm quite
happy with the result.

> > Oh, and I also wanted to say something about why not just directly using
> > the class name, which was my first idea: 'foo': 'iothread' looks more
> > like referencing an existing iothread rather than the configuration for
> > a new one. I wanted to leave us the option that we could possibly later
> > take a string for such options (a QOM path) and then pass the referenced
> > object to QMP commands as the proper QOM type.
> 
> I agree that 'iothread' is going to be confusing when you're referring to
> the configuration.
> 
> Anyway I'm totally fine with 'qom-config:classname'.  Given this
> explanation, however, one alternative that makes sense could be
> 'classname:full-config'. Then you could use 'classname:config' for the
> autoboxed configs---and reserve 'classname' to mean the pointer to an
> object.  Classes are (generally) lowercase and QAPI structs are
> CamelCase, so there is not much potential for collisions.

Makes sense to me, too.

I just checked and I actually already forbid class names with colons in
them (check_name_str() takes care of this), so yes, suffixes actually
work on the QAPI level.

If we actually want to use these types in manually written C code, we
might have to convert the name to CamelCase, though, for consistency
with the coding style.

We already have a function camel_to_upper(), we'd need a new
lower_to_camel(), so that from a class 'rng-random', you would get types
'RngRandomConfig' (the local ones) and 'RngRandomFullConfig' (with
parent options).

Kevin

On 11/4/21 15:26, Kevin Wolf wrote:
> It took me a while to figure out how to deal with this, but I'm quite
> happy with the result.

I like it too.

>>> Oh, and I also wanted to say something about why not just directly using
>>> the class name, which was my first idea: 'foo': 'iothread' looks more
>>> like referencing an existing iothread rather than the configuration for
>>> a new one. I wanted to leave us the option that we could possibly later
>>> take a string for such options (a QOM path) and then pass the referenced
>>> object to QMP commands as the proper QOM type.
>>
>> I agree that 'iothread' is going to be confusing when you're referring to
>> the configuration.
>>
>> Anyway I'm totally fine with 'qom-config:classname'.  Given this
>> explanation, however, one alternative that makes sense could be
>> 'classname:full-config'. Then you could use 'classname:config' for the
>> autoboxed configs---and reserve 'classname' to mean the pointer to an
>> object.  Classes are (generally) lowercase and QAPI structs are
>> CamelCase, so there is not much potential for collisions.
> 
> Makes sense to me, too.
> 
> I just checked and I actually already forbid class names with colons in
> them (check_name_str() takes care of this), so yes, suffixes actually
> work on the QAPI level.
> 
> If we actually want to use these types in manually written C code, we
> might have to convert the name to CamelCase, though, for consistency
> with the coding style.
> 
> We already have a function camel_to_upper(), we'd need a new
> lower_to_camel(), so that from a class 'rng-random', you would get types
> 'RngRandomConfig' (the local ones) and 'RngRandomFullConfig' (with
> parent options).

That's nice.  IMO with these changes the autoboxing becomes again more 
appealing.  With the auto-generated local config struct,

     { 'class': 'rng-egd',
       'parent': 'rng-backend',
       'config': { 'chardev': 'str' } }

now maps to

     bool qom_rng_egd_config(Object *obj, RngEgdConfig *config,
                             Error **errp)
     {
         RngEgd *s = RNG_EGD(obj);

         s->chr_name = g_strdup(config->chardev);
         return true;
     }

The three arguments follow the same prototype as .instance_config:

     bool (*instance_config)(Object *obj, Visitor *v, Error **errp);

just with the visitor replaced by a nice C struct.  You started 
(obviously) with the simplest cases, and it's good to check whether easy 
things remain easy, but it seems to me that all but the simplest objects 
would end up using boxed config anyway.

Also (and this is something Markus and I have discussed in the past, but 
I'm not sure if we have actually reached an agreement), I would make 
instance_config return void.  The usual convention *is* to return bool 
from functions that have an Error** and no other return value; however, 
that's because in general there will be more calls to the function than 
definitions.

In this case, there will be just one call to the ti->instance_config 
function pointer, in object_configure, and N definitions of the 
function, so the ratio and the rationale are reversed.  See 
object_property_get for an example in qom/object.c.

Thanks,

Paolo

Am 04.11.2021 um 15:49 hat Paolo Bonzini geschrieben:
> On 11/4/21 15:26, Kevin Wolf wrote:
> > It took me a while to figure out how to deal with this, but I'm quite
> > happy with the result.
> 
> I like it too.
> 
> > > > Oh, and I also wanted to say something about why not just directly using
> > > > the class name, which was my first idea: 'foo': 'iothread' looks more
> > > > like referencing an existing iothread rather than the configuration for
> > > > a new one. I wanted to leave us the option that we could possibly later
> > > > take a string for such options (a QOM path) and then pass the referenced
> > > > object to QMP commands as the proper QOM type.
> > > 
> > > I agree that 'iothread' is going to be confusing when you're referring to
> > > the configuration.
> > > 
> > > Anyway I'm totally fine with 'qom-config:classname'.  Given this
> > > explanation, however, one alternative that makes sense could be
> > > 'classname:full-config'. Then you could use 'classname:config' for the
> > > autoboxed configs---and reserve 'classname' to mean the pointer to an
> > > object.  Classes are (generally) lowercase and QAPI structs are
> > > CamelCase, so there is not much potential for collisions.
> > 
> > Makes sense to me, too.
> > 
> > I just checked and I actually already forbid class names with colons in
> > them (check_name_str() takes care of this), so yes, suffixes actually
> > work on the QAPI level.
> > 
> > If we actually want to use these types in manually written C code, we
> > might have to convert the name to CamelCase, though, for consistency
> > with the coding style.
> > 
> > We already have a function camel_to_upper(), we'd need a new
> > lower_to_camel(), so that from a class 'rng-random', you would get types
> > 'RngRandomConfig' (the local ones) and 'RngRandomFullConfig' (with
> > parent options).
> 
> That's nice.  IMO with these changes the autoboxing becomes again more
> appealing.  With the auto-generated local config struct,
> 
>     { 'class': 'rng-egd',
>       'parent': 'rng-backend',
>       'config': { 'chardev': 'str' } }
> 
> now maps to
> 
>     bool qom_rng_egd_config(Object *obj, RngEgdConfig *config,
>                             Error **errp)
>     {
>         RngEgd *s = RNG_EGD(obj);
> 
>         s->chr_name = g_strdup(config->chardev);
>         return true;
>     }
> 
> The three arguments follow the same prototype as .instance_config:
> 
>     bool (*instance_config)(Object *obj, Visitor *v, Error **errp);
> 
> just with the visitor replaced by a nice C struct.  You started (obviously)
> with the simplest cases, and it's good to check whether easy things remain
> easy, but it seems to me that all but the simplest objects would end up
> using boxed config anyway.

I think I'd still like to have the option of unboxed arguments for the
simpler objects (most user creatable objects are relatively simple), but
flipping the default would make sense if we just automatically create a
named type.

But before I implement anything like this, I'd first like to hear
Markus's opinion because I would prefer to avoid -EMAGIC during review.

> Also (and this is something Markus and I have discussed in the past, but I'm
> not sure if we have actually reached an agreement), I would make
> instance_config return void.  The usual convention *is* to return bool from
> functions that have an Error** and no other return value; however, that's
> because in general there will be more calls to the function than
> definitions.
> 
> In this case, there will be just one call to the ti->instance_config
> function pointer, in object_configure, and N definitions of the function, so
> the ratio and the rationale are reversed.  See object_property_get for an
> example in qom/object.c.

Good point. Though not necessarily one for ti->instance_config because
that's the automatically generated marshaller and returning a bool from
there isn't a problem at all. The function that should definitely return
void is the idiomatic C one that is manually written and called by the
marshaller.

Kevin

On 11/4/21 10:07, Kevin Wolf wrote:
> Am 03.11.2021 um 22:26 hat Paolo Bonzini geschrieben:
>> On 11/3/21 18:29, Kevin Wolf wrote:
>>> 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.
>>
>> That's pretty cool!

Hi,

Just to be sure I understand. Is this config a generalization of the 
qdev-properties we have to describe the parameter used to create a device ?

>>
>> Just one question: why not always use boxed configuration?  It should not
>> make the instance_config types any larger, and it avoids unwieldy argument
>> lists.
> 
> Basically for the same reason as for commands (and for consistency with
> commands): If you have only one or two options, then creating a separate
> type for them feels just a little over the top, and boxing doesn't work
> with implicit types.
> 
> I really like the concise definitions without a separate struct like in:
> 
> { 'class': 'rng-egd',
>    'parent': 'rng-backend',
>    'config': { 'chardev': 'str' } }
> 
> Though maybe we could make it work by giving the implicit type another
> prefixed name?

What's an implicit type in this context ?

> 
>> Also, for the obligatory bikeshedding remark, do you have any other plans or
>> ideas for the colon-separated auto generated typenames?  Having both the
>> "namespace" (qom) and the more specific use (config) before the classname is
>> a bit weird, compared to the existing structs like RngRandomProperties.
>> Especially if boxed config is more used (or becomes the only possibility),
>> it might be that qom:class-name:config, or even just class-name:config, make
>> for nicer code in the object implementation.
> 
> 'qom-config:classname' isn't a type that is useful for the object
> implementations at all. Its only use is really storing the whole
> configuration temporarily in a QAPI C struct before applying it.
> 

Would not this type be useful to generate read-only properties (and 
store the values) corresponding to the config if we expect to always 
have such properties ?

--
Damien

Am 04.11.2021 um 16:52 hat Damien Hedde geschrieben:
> On 11/4/21 10:07, Kevin Wolf wrote:
> > Am 03.11.2021 um 22:26 hat Paolo Bonzini geschrieben:
> > > On 11/3/21 18:29, Kevin Wolf wrote:
> > > > 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.
> > > 
> > > That's pretty cool!
> 
> Hi,
> 
> Just to be sure I understand. Is this config a generalization of the
> qdev-properties we have to describe the parameter used to create a
> device ?

Everything I'm doing here is on the QOM level. But qdev is just a layer
on top of QOM, so yes, eventually qdev properties should be declared in
the schema, too. It's useful to keep this long-term goal in mind, but
for now, the immediate goal is making it work for user-creatable
objects.

> > > Just one question: why not always use boxed configuration?  It should not
> > > make the instance_config types any larger, and it avoids unwieldy argument
> > > lists.
> > 
> > Basically for the same reason as for commands (and for consistency with
> > commands): If you have only one or two options, then creating a separate
> > type for them feels just a little over the top, and boxing doesn't work
> > with implicit types.
> > 
> > I really like the concise definitions without a separate struct like in:
> > 
> > { 'class': 'rng-egd',
> >    'parent': 'rng-backend',
> >    'config': { 'chardev': 'str' } }
> > 
> > Though maybe we could make it work by giving the implicit type another
> > prefixed name?
> 
> What's an implicit type in this context ?

The value of 'config' creates an implicit struct type that is considered
anonymous on the QAPI level. With an explicit type, you wouldn't put an
object containing all the options there, but just a type name, like
this:

{ 'struct': 'RngEgdConfig',
  'data': { 'chardev': 'str' } }

{ 'class': 'rng-egd',
   'parent': 'rng-backend',
   'config': 'RngEgdConfig' }

> > 
> > > Also, for the obligatory bikeshedding remark, do you have any other plans or
> > > ideas for the colon-separated auto generated typenames?  Having both the
> > > "namespace" (qom) and the more specific use (config) before the classname is
> > > a bit weird, compared to the existing structs like RngRandomProperties.
> > > Especially if boxed config is more used (or becomes the only possibility),
> > > it might be that qom:class-name:config, or even just class-name:config, make
> > > for nicer code in the object implementation.
> > 
> > 'qom-config:classname' isn't a type that is useful for the object
> > implementations at all. Its only use is really storing the whole
> > configuration temporarily in a QAPI C struct before applying it.
> 
> Would not this type be useful to generate read-only properties (and store
> the values) corresponding to the config if we expect to always have such
> properties ?

No, that's what I explained in the following paragraph. That would be a
different type, because 'qom-config:classname' contains the options for
the parent class, too, but you only want to store the options for the
class itself.

Kevin

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?

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.

> * 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.

>   Also note that 'qom-config-any:user-creatable' is new, too. The
>   'qom-config:...' types introduced by this series don't work for
>   subclasses, but only for the exact class.
>
>   On the external interface, the new 'qom-config-any:...' type including
>   subclasses would basically behave (and be introspected) like the union
>   we have today, just without being defined explicitly.

I'm not sure I follow.  How is the qom-config-any:user-creatable to be
defined?  QAPI collects all the qom-config:* types into a union
automatically?

>   As for the C representation for configurations that include
>   subclasses, I'm imagining a struct that just contains the qom_type
>   string (so we can call the right handler) and a pointer to the real
>   config (that is treated as opaque by the generic code).

Now you lost me.

> I could probably add more, but let's just start with this for discussion
> now.

I wish we could fill a whiteboard together...

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.

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.

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

> >   Also note that 'qom-config-any:user-creatable' is new, too. The
> >   'qom-config:...' types introduced by this series don't work for
> >   subclasses, but only for the exact class.
> >
> >   On the external interface, the new 'qom-config-any:...' type including
> >   subclasses would basically behave (and be introspected) like the union
> >   we have today, just without being defined explicitly.
> 
> I'm not sure I follow.  How is the qom-config-any:user-creatable to be
> defined?  QAPI collects all the qom-config:* types into a union
> automatically?

All classes that inherit from user-creatable, but yes, automatically
collected.

For user-creatable, we can either introduce interfaces in QAPI, too, or
we just pretend it's actually the top-level parent class.

Kevin

On Tue, 14 Dec 2021 at 10:26, Kevin Wolf <kwolf@redhat.com> wrote:
> Configuration is for creating objects, properties are for runtime after
> the creation.

Well, yes and no. In a few places we have some properties which
are morally speaking configuration stuff, but which are runtime
settable. This happens because the property needs to be set after
the device is realized but before the machine is run, and we
don't have a concept of "settable only during the machine creation
phase", only of "settable only before realize". (I can't find an
example of this in the codebase offhand, but I definitely have one
in a patchset I'm working on -- the code which needs to
configure the property of the configured object is far removed
in both location in the codebase and point at which it runs
from the code which is doing the initialize-and-realize.)

-- PMM

Am 14.12.2021 um 11:40 hat Peter Maydell geschrieben:
> On Tue, 14 Dec 2021 at 10:26, Kevin Wolf <kwolf@redhat.com> wrote:
> > Configuration is for creating objects, properties are for runtime after
> > the creation.
> 
> Well, yes and no. In a few places we have some properties which
> are morally speaking configuration stuff, but which are runtime
> settable. This happens because the property needs to be set after
> the device is realized but before the machine is run, and we
> don't have a concept of "settable only during the machine creation
> phase", only of "settable only before realize". (I can't find an
> example of this in the codebase offhand, but I definitely have one
> in a patchset I'm working on -- the code which needs to
> configure the property of the configured object is far removed
> in both location in the codebase and point at which it runs
> from the code which is doing the initialize-and-realize.)

Yes, that's fair, but for the infrastructure it doesn't matter much what
something is "morally speaking". These things use properties at
"runtime" (i.e. after realize) today and will keep using them until we
find a different solution. I have no intention to change anything about
it in the context of QAPIfication. The only liberty I'm taking is
removing setters that can't work after realize.

Kevin

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?

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

>> >   Also note that 'qom-config-any:user-creatable' is new, too. The
>> >   'qom-config:...' types introduced by this series don't work for
>> >   subclasses, but only for the exact class.
>> >
>> >   On the external interface, the new 'qom-config-any:...' type including
>> >   subclasses would basically behave (and be introspected) like the union
>> >   we have today, just without being defined explicitly.
>> 
>> I'm not sure I follow.  How is the qom-config-any:user-creatable to be
>> defined?  QAPI collects all the qom-config:* types into a union
>> automatically?
>
> All classes that inherit from user-creatable, but yes, automatically
> collected.
>
> For user-creatable, we can either introduce interfaces in QAPI, too, or
> we just pretend it's actually the top-level parent class.

Thanks!