| Message ID | 1443760312-656-6-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > A future patch will enable deferred error detection in the > various QAPISchema*.check() methods (rather than the current > ad hoc parse checks). What's "deferred" about them? Perhaps simply: A future patch will move error checking into the various QAPISchema*.check() methods. > But that means the user can request > a QAPI entity that will only fail validation after it has > been initialized. I'm not sure I get this sentence. > Since all errors have to have an > associated 'info' location, we need a location to be > associated with all user-triggered implicit types. The > intuitive info to use is the location of the enclosing > entity that caused the creation of the implicit type. Yes. > Note that we do not anticipate builtin types being used in > an error message (as they are not part of the user's QAPI > input, the user can't cause a semantic error in their > behavior), so we exempt those types from requiring info, Yes, no errors should be reported for built-in entities. Sometimes, an error message for one entity wants to refer to some other entity. We'll have to take care not to blindly use info then. > by > setting a flag to track the completion of _def_predefineds(). Explaining the implementation here seems premature. > No change to the generated code. > > RFC: I used a class-level static flag to track whether we expected > 'info is None' when creating a QAPISchemaEntity. This is gross, > because the flag will only be set on the first QAPISchema() instance > (it works because none of our client scripts ever instantiate more > than one schema). But the only other thing I could think of would > be passing the QAPISchema instance into the constructor for each > QAPISchemaEntity, which is a lot of churn. Any better ideas on how > best to do the assertion, or should I just drop it? > > Signed-off-by: Eric Blake <eblake@redhat.com> I'd check in QAPISchema._def_entity(). Patch looks good otherwise.
On 10/02/2015 02:54 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> A future patch will enable deferred error detection in the >> various QAPISchema*.check() methods (rather than the current >> ad hoc parse checks). > > What's "deferred" about them? With ad hoc parse checks, we validate the .json before calling QAPISchemaEntity constructors. With QAPISchemaEntity.check(), the constructor is called on various strings, but the strings may not resolve; we don't know about the problem until check() is called. > > Perhaps simply: A future patch will move error checking into the various > QAPISchema*.check() methods. > >> But that means the user can request >> a QAPI entity that will only fail validation after it has >> been initialized. > > I'm not sure I get this sentence. Trying to point out that while pre-patch, the check() method was only run on well-formed entities, now post-patch it can raise errors that we chose not to detect prior to __init__ time. >> RFC: I used a class-level static flag to track whether we expected >> 'info is None' when creating a QAPISchemaEntity. This is gross, >> because the flag will only be set on the first QAPISchema() instance >> (it works because none of our client scripts ever instantiate more >> than one schema). But the only other thing I could think of would >> be passing the QAPISchema instance into the constructor for each >> QAPISchemaEntity, which is a lot of churn. Any better ideas on how >> best to do the assertion, or should I just drop it? >> >> Signed-off-by: Eric Blake <eblake@redhat.com> > > I'd check in QAPISchema._def_entity(). Ah, instead of an assert in QAPISchemaEntity.__init__() (which requires a leaky abstraction), instead write the assert into QAPISchema (so the flag can now be instance-local). Makes sense; I'll play with the idea.
Eric Blake <eblake@redhat.com> writes: > On 10/02/2015 02:54 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> A future patch will enable deferred error detection in the >>> various QAPISchema*.check() methods (rather than the current >>> ad hoc parse checks). >> >> What's "deferred" about them? > > With ad hoc parse checks, we validate the .json before calling > QAPISchemaEntity constructors. With QAPISchemaEntity.check(), the > constructor is called on various strings, but the strings may not > resolve; we don't know about the problem until check() is called. I guess I'd say something like A future patch will move some error checking from the parser to the various QAPISchema*.check() methods. These run only after parsing completes. >> >> Perhaps simply: A future patch will move error checking into the various >> QAPISchema*.check() methods. >> >>> But that means the user can request >>> a QAPI entity that will only fail validation after it has >>> been initialized. >> >> I'm not sure I get this sentence. > > Trying to point out that while pre-patch, the check() method was only > run on well-formed entities, now post-patch it can raise errors that we > chose not to detect prior to __init__ time. > >>> RFC: I used a class-level static flag to track whether we expected >>> 'info is None' when creating a QAPISchemaEntity. This is gross, >>> because the flag will only be set on the first QAPISchema() instance >>> (it works because none of our client scripts ever instantiate more >>> than one schema). But the only other thing I could think of would >>> be passing the QAPISchema instance into the constructor for each >>> QAPISchemaEntity, which is a lot of churn. Any better ideas on how >>> best to do the assertion, or should I just drop it? >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> I'd check in QAPISchema._def_entity(). > > Ah, instead of an assert in QAPISchemaEntity.__init__() (which requires > a leaky abstraction), instead write the assert into QAPISchema (so the > flag can now be instance-local). Makes sense; I'll play with the idea. :)
On 10/02/2015 10:07 AM, Markus Armbruster wrote: >>>> RFC: I used a class-level static flag to track whether we expected >>>> 'info is None' when creating a QAPISchemaEntity. This is gross, >>>> because the flag will only be set on the first QAPISchema() instance >>>> (it works because none of our client scripts ever instantiate more >>>> than one schema). But the only other thing I could think of would >>>> be passing the QAPISchema instance into the constructor for each >>>> QAPISchemaEntity, which is a lot of churn. Any better ideas on how >>>> best to do the assertion, or should I just drop it? >>> >>> I'd check in QAPISchema._def_entity(). >> >> Ah, instead of an assert in QAPISchemaEntity.__init__() (which requires >> a leaky abstraction), instead write the assert into QAPISchema (so the >> flag can now be instance-local). Makes sense; I'll play with the idea. > > :) Oh, and this means accessing QAPISchemaEntity.info outside of the class, which absolutely kills off my idea of renaming it to _info for hiding purposes (so patch 12/12 is now officially gone).
diff --git a/scripts/qapi.py b/scripts/qapi.py index 255001a..19cca97 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -792,6 +792,7 @@ class QAPISchemaEntity(object): def __init__(self, name, info): assert isinstance(name, str) self.name = name + assert info or not QAPISchema.predefined_initialized self.info = info def c_name(self): @@ -1114,6 +1115,8 @@ class QAPISchemaEvent(QAPISchemaEntity): class QAPISchema(object): + predefined_initialized = False + def __init__(self, fname): try: self.exprs = check_exprs(QAPISchemaParser(open(fname, "r")).exprs) @@ -1122,6 +1125,7 @@ class QAPISchema(object): exit(1) self._entity_dict = {} self._def_predefineds() + QAPISchema.predefined_initialized = True self._def_exprs() self.check() @@ -1163,9 +1167,9 @@ class QAPISchema(object): [], None) self._def_entity(self.the_empty_object_type) - def _make_implicit_enum_type(self, name, values): + def _make_implicit_enum_type(self, name, info, values): name = name + 'Kind' - self._def_entity(QAPISchemaEnumType(name, None, values, None)) + self._def_entity(QAPISchemaEnumType(name, info, values, None)) return name def _make_array_type(self, element_type, info): @@ -1174,12 +1178,12 @@ class QAPISchema(object): self._def_entity(QAPISchemaArrayType(name, info, element_type)) return name - def _make_implicit_object_type(self, name, role, members): + def _make_implicit_object_type(self, name, info, role, members): if not members: return None name = ':obj-%s-%s' % (name, role) if not self.lookup_entity(name, QAPISchemaObjectType): - self._def_entity(QAPISchemaObjectType(name, None, None, + self._def_entity(QAPISchemaObjectType(name, info, None, members, None)) return name @@ -1218,13 +1222,13 @@ class QAPISchema(object): if isinstance(typ, list): assert len(typ) == 1 typ = self._make_array_type(typ[0], info) - typ = self._make_implicit_object_type(typ, 'wrapper', + typ = self._make_implicit_object_type(typ, info, 'wrapper', [self._make_member('data', typ, info)]) return QAPISchemaObjectTypeVariant(case, typ) - def _make_tag_enum(self, type_name, variants): - typ = self._make_implicit_enum_type(type_name, + def _make_tag_enum(self, type_name, info, variants): + typ = self._make_implicit_enum_type(type_name, info, [v.name for v in variants]) return QAPISchemaObjectTypeMember('type', typ, False) @@ -1240,7 +1244,7 @@ class QAPISchema(object): else: variants = [self._make_simple_variant(key, value, info) for (key, value) in data.iteritems()] - tag_enum = self._make_tag_enum(name, variants) + tag_enum = self._make_tag_enum(name, info, variants) self._def_entity( QAPISchemaObjectType(name, info, base, self._make_members(OrderedDict(), info), @@ -1253,7 +1257,7 @@ class QAPISchema(object): data = expr['data'] variants = [self._make_variant(key, value) for (key, value) in data.iteritems()] - tag_enum = self._make_tag_enum(name, variants) + tag_enum = self._make_tag_enum(name, info, variants) self._def_entity( QAPISchemaAlternateType(name, info, QAPISchemaObjectTypeVariants(None, @@ -1267,7 +1271,7 @@ class QAPISchema(object): gen = expr.get('gen', True) success_response = expr.get('success-response', True) if isinstance(data, OrderedDict): - data = self._make_implicit_object_type(name, 'arg', + data = self._make_implicit_object_type(name, info, 'arg', self._make_members(data, info)) if isinstance(rets, list): @@ -1280,7 +1284,7 @@ class QAPISchema(object): name = expr['event'] data = expr.get('data') if isinstance(data, OrderedDict): - data = self._make_implicit_object_type(name, 'arg', + data = self._make_implicit_object_type(name, info, 'arg', self._make_members(data, info)) self._def_entity(QAPISchemaEvent(name, info, data))
A future patch will enable deferred error detection in the various QAPISchema*.check() methods (rather than the current ad hoc parse checks). But that means the user can request a QAPI entity that will only fail validation after it has been initialized. Since all errors have to have an associated 'info' location, we need a location to be associated with all user-triggered implicit types. The intuitive info to use is the location of the enclosing entity that caused the creation of the implicit type. Note that we do not anticipate builtin types being used in an error message (as they are not part of the user's QAPI input, the user can't cause a semantic error in their behavior), so we exempt those types from requiring info, by setting a flag to track the completion of _def_predefineds(). No change to the generated code. RFC: I used a class-level static flag to track whether we expected 'info is None' when creating a QAPISchemaEntity. This is gross, because the flag will only be set on the first QAPISchema() instance (it works because none of our client scripts ever instantiate more than one schema). But the only other thing I could think of would be passing the QAPISchema instance into the constructor for each QAPISchemaEntity, which is a lot of churn. Any better ideas on how best to do the assertion, or should I just drop it? Signed-off-by: Eric Blake <eblake@redhat.com> --- v6: improve commit message, track implicit enum info, rebase on new lazy array handling --- scripts/qapi.py | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)