| Message ID | 20211103173002.209906-9-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QOM/QAPI integration part 1 | expand |
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)
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
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) + 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 = [] + + 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] + + self.full_config_type = QAPISchemaObjectType( + f"qom-config:{self.name}", self.info, None, self._ifcond, + self.features, None, members, None) + + return self.full_config_type + + def check(self, schema): + super().check(schema) + assert self.full_config_type + + 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) + 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)) + def check(self): for ent in self._entity_list: ent.check(self)
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> --- scripts/qapi/schema.py | 60 +++++++++++++++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 10 deletions(-)