Re: [RFC PATCH 08/12] qapi: Create qom-config:... type for classes

[Kevin Wolf]

Am 23.11.2021 um 14:00 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > For every class that has a 'config' definition, a corresponding
> > 'qom-config:$QOM_TYPE' type is automatically created that contains the
> > configuration for the class and all of its parent classes.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> I assume $QOM_TYPE stands for the class's name.
> 
> What kind of type does this define?  Peeking at the code below... looks
> like it's a struct type.
> 
> I wonder how it's related to the the type we already use or create for
> the the class's 'config' member.  Which is either the name of a struct
> or union type to use, or a dictionary that tells us what struct type to
> create.  Let's see below.

The members of 'config' (i.e. the local configuration options of the
class) are a subset of the member of this new type (i.e. the
configuration options of this class and all of its parent classes).

The new type is really just needed internally in the generated QAPI code
so that the full input can be stored in a C struct temporarily. All of
the manually written code only uses the 'config' type.

> > ---
> >  scripts/qapi/schema.py | 60 +++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 50 insertions(+), 10 deletions(-)
> >
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index ebf69341d7..79db42b810 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -761,6 +761,13 @@ def connect_doc(self, doc):
> >              for f in self.features:
> >                  doc.connect_feature(f)
> >  
> > +    def clone(self):
> > +        features = [QAPISchemaFeature(f.name, f.info, f.ifcond)
> > +                    for f in self.features]
> > +        return QAPISchemaObjectTypeMember(
> > +            self.name, self.info, self._type_name, self.optional,
> > +            self.ifcond, features)
> > +
> 
> A copy that's somewhere between shallow and deep.  Reserving judgement.
> 
> >  
> >  class QAPISchemaVariant(QAPISchemaObjectTypeMember):
> >      role = 'branch'
> > @@ -783,17 +790,11 @@ def __init__(self, name, info, doc, ifcond, features, 
> > parent,
> >          self._config_type_name = config_type
> >          self.config_type = None
> >          self.config_boxed = config_boxed
> > +        self.full_config_type = None
> >  
> > -    def check(self, schema):
> > -        super().check(schema)
> > -
> > -        if self._parent_name:
> > -            self.parent = schema.lookup_entity(self._parent_name,
> > -                                               QAPISchemaClass)
> > -            if not self.parent:
> > -                raise QAPISemError(
> > -                    self.info,
> > -                    "Unknown parent class '%s'" % self._parent_name)
> > +    def get_qom_config_type(self, schema):
> > +        if self.full_config_type:
> > +            return self.full_config_type
> >  
> >          if self._config_type_name:
> >              self.config_type = schema.resolve_type(
> > @@ -809,6 +810,40 @@ def check(self, schema):
> >                      "class 'config' can take %s only with 'boxed': true"
> >                      % self.config_type.describe())
> >  
> > +            # FIXME That's a bit ugly
> > +            self.config_type.check(schema)
> > +            members = [m.clone() for m in self.config_type.members]
> > +        else:
> > +            members = []
> 
> Have you considered defaulting ._config_type_name to 'q_empty'?

No. Sounds like a minor simplification here because the if condition
would always become true and therefore the code can be unconditional.

The more important point to talk about in this context is the FIXME
comment. We need to define the right order in which things should be
done.

Before this series, we have a split into two phases: First process all
type definitions in the schema and create the objects representing them,
and then check whether all of the created objects are valid. Among
others, these two phases are needed because we allow definitions to
refer to types that are defined only later.

I'm trying to insert a third phase in the middle that creates new
implicit types based on the schema definitions.

The thing that makes trouble here is that .check() doesn't really only
check, but it's in fact the thing that finalises the initialisation of
object types. Before it, we don't have a member list, but we need it in
order to create the derived type. So I ended up calling .check() for
some types earlier than it should be called, basically intertwining
these two phases.

This can probably fail when .check() tries to access an implicit type
that hasn't been created yet. Not sure if this is a practically relevant
limitation, but mixing the phases this way is definitely ugly.

Any ideas how to solve this cleanly?

> > +
> > +        if self._parent_name:
> > +            self.parent = schema.lookup_entity(self._parent_name,
> > +                                               QAPISchemaClass)
> > +            if not self.parent:
> > +                raise QAPISemError(
> > +                    self.info,
> > +                    "Unknown parent class '%s'" % self._parent_name)
> > +
> > +            self.parent.get_qom_config_type(schema)
> > +            members += [m.clone() for m in self.parent.config_type.members]
> 
> @members is the list of common members of the class's and all its
> parents' 'config' types.
> 
> Any variant members of 'config' types get silently ignored.  Should we
> reject them instead?

We (try to) allow them for boxed 'config' types currently. I think this
was only because commands do the same, but I don't see any reason why
variants would be needed at all for 'config'. So we can just reject them
for both boxed and non-boxed configs.

> > +
> > +        self.full_config_type = QAPISchemaObjectType(
> > +            f"qom-config:{self.name}", self.info, None, self._ifcond,
> > +            self.features, None, members, None)
> 
> The "full config type" inherits conditional and features from the class.
> Its (common) members are the members of the class's and all its parents'
> 'config' types.
> 
> Could we use the parent's full config type as the base type here?

Hmm, good question. I guess that could work.

> Do we need the non-full config type (the type named by or implicitly
> defined by the 'config' member') for anything other than a source of
> local members for the full config type?

Yes, this is the practically relevant type in manually written C code.
The implementation of .instance_config in a QOM object has no use for
all the properties that are really meant for its parent class. It might
even commonly include the local config type in its runtime state, and
having options of the parent class duplicated both in the child class
and in the parent class would be a sure way to introduce bugs.

This is the reason why the non-full config type has a nice C type name
when using an explicit type for 'config', whereas the full config type
is a QAPI implementation detail that can have an ugly name in C without
bothering anyone.

> > +
> > +        return self.full_config_type
> > +
> > +    def check(self, schema):
> > +        super().check(schema)
> > +        assert self.full_config_type
> 
> New constraint: must call .get_qom_config_type() before .check().
> 
> This side effect makes me unsure sure about the "get" in the name.
> Let's worry about that later.

Aye, related to the (theoretical) three phases mentioned above.

> > +
> > +    def connect_doc(self, doc=None):
> > +        super().connect_doc(doc)
> > +        doc = doc or self.doc
> > +        if doc:
> > +            if self.config_type and self.config_type.is_implicit():
> > +                self.config_type.connect_doc(doc)
> 
> Brain cramp.

Apart from the attribute name for the config type, it's an exact copy
from QAPISchemaCommand.

Kevin