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

[Markus Armbruster <armbru@redhat.com>]

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.

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

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

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

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?

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

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

> +
>      def visit(self, visitor):
>          super().visit(visitor)
>          visitor.visit_class(self)
> @@ -1236,6 +1271,11 @@ def _def_exprs(self, exprs):
>              else:
>                  assert False
>  
> +        classes = [c for c in self._entity_list
> +                   if isinstance(c,QAPISchemaClass)]
> +        for c in classes:
> +            self._def_entity(c.get_qom_config_type(self))
> +

Either use a generator expression

           classes = (c for c in self._entity_list if ... )
           for c in classes:

Or do it the old-fashioned way

           for ent in self._entity_list:
               if isinstance(ent, QAPISchemaClass):

>      def check(self):
>          for ent in self._entity_list:
>              ent.check(self)