From 0ca7b11709b8e81e5e8f28245b5b594b597b062b Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:14 +0200 Subject: qapi: Tighten QAPISchemaFOO.check() assertions When we introduced the QAPISchema intermediate representation (commit ac88219a6c7), we took a shortcut: we left check_exprs() & friends alone instead of moving semantic checks into the QAPISchemaFOO.check(). check_exprs() still checks and reports errors, and the .check() assert check_exprs() did the job. There are a few gaps, though. QAPISchemaArrayType.check() neglects to assert the element type is not an array. Add the assertion. QAPISchemaObjectTypeVariants.check() neglects to assert the tag member is not optional. Add the assertion. It neglects to assert the tag member is not conditional. Add the assertion. It neglects to assert we actually have variants. Add the assertion. It asserts the variants are object types, but neglects to assert they don't have variants. Tighten the assertion. QAPISchemaObjectTypeVariants.check_clash() has the same issue. However, it can run only after .check(). Delete the assertion instead of tightening it. QAPISchemaAlternateType.check() neglects to assert the branch types don't conflict. Fixing that isn't trivial, so add just a TODO comment for now. It'll be resolved later in this series. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-2-armbru@redhat.com> --- scripts/qapi/common.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index b00caac..155b87b 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -1362,6 +1362,7 @@ class QAPISchemaArrayType(QAPISchemaType): QAPISchemaType.check(self, schema) self.element_type = schema.lookup_type(self._element_type_name) assert self.element_type + assert not isinstance(self.element_type, QAPISchemaArrayType) @property def ifcond(self): @@ -1606,6 +1607,8 @@ class QAPISchemaObjectTypeVariants(object): self.tag_member = seen[c_name(self._tag_name)] assert self._tag_name == self.tag_member.name assert isinstance(self.tag_member.type, QAPISchemaEnumType) + assert not self.tag_member.optional + assert self.tag_member.ifcond == [] if self._tag_name: # flat union # branches that are not explicitly covered get an empty type cases = set([v.name for v in self.variants]) @@ -1615,20 +1618,21 @@ class QAPISchemaObjectTypeVariants(object): m.ifcond) v.set_owner(self.tag_member.owner) self.variants.append(v) + assert self.variants for v in self.variants: v.check(schema) # Union names must match enum values; alternate names are # checked separately. Use 'seen' to tell the two apart. if seen: assert v.name in self.tag_member.type.member_names() - assert isinstance(v.type, QAPISchemaObjectType) + assert (isinstance(v.type, QAPISchemaObjectType) + and not v.type.variants) v.type.check(schema) def check_clash(self, info, seen): for v in self.variants: # Reset seen map for each variant, since qapi names from one # branch do not affect another branch - assert isinstance(v.type, QAPISchemaObjectType) v.type.check_clash(info, dict(seen)) @@ -1659,6 +1663,7 @@ class QAPISchemaAlternateType(QAPISchemaType): seen = {} for v in self.variants.variants: v.check_clash(self.info, seen) + # TODO check conflicting qtypes if self.doc: self.doc.connect_member(v) if self.doc: -- cgit v1.1 From 57608a5299064f7219d00447160103946c796436 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:15 +0200 Subject: qapi: Rename .owner to .defined_in QAPISchemaMember.owner is the name of the defining entity. That's a confusing name when an object type inherits members from a base type. Rename it to .defined_in. Rename .set_owner() and ._pretty_owner() to match. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-3-armbru@redhat.com> --- scripts/qapi/common.py | 61 +++++++++++++++++++++++++------------------------- 1 file changed, 31 insertions(+), 30 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 155b87b..bfb3e8a 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -1319,7 +1319,7 @@ class QAPISchemaEnumType(QAPISchemaType): QAPISchemaType.__init__(self, name, info, doc, ifcond) for m in members: assert isinstance(m, QAPISchemaEnumMember) - m.set_owner(name) + m.set_defined_in(name) assert prefix is None or isinstance(prefix, str) self.members = members self.prefix = prefix @@ -1405,13 +1405,13 @@ class QAPISchemaObjectType(QAPISchemaType): assert base is None or isinstance(base, str) for m in local_members: assert isinstance(m, QAPISchemaObjectTypeMember) - m.set_owner(name) + m.set_defined_in(name) if variants is not None: assert isinstance(variants, QAPISchemaObjectTypeVariants) - variants.set_owner(name) + variants.set_defined_in(name) for f in features: assert isinstance(f, QAPISchemaFeature) - f.set_owner(name) + f.set_defined_in(name) self._base_name = base self.base = None self.local_members = local_members @@ -1521,15 +1521,16 @@ class QAPISchemaMember(object): assert isinstance(name, str) self.name = name self.ifcond = ifcond or [] - self.owner = None + self.defined_in = None - def set_owner(self, name): - assert not self.owner - self.owner = name + def set_defined_in(self, name): + assert not self.defined_in + self.defined_in = name def check_clash(self, info, seen): cname = c_name(self.name) - if cname.lower() != cname and self.owner not in name_case_whitelist: + if (cname.lower() != cname + and self.defined_in not in name_case_whitelist): raise QAPISemError(info, "%s should not use uppercase" % self.describe()) if cname in seen: @@ -1537,27 +1538,27 @@ class QAPISchemaMember(object): (self.describe(), seen[cname].describe())) seen[cname] = self - def _pretty_owner(self): - owner = self.owner - if owner.startswith('q_obj_'): + def _pretty_defined_in(self): + defined_in = self.defined_in + if defined_in.startswith('q_obj_'): # See QAPISchema._make_implicit_object_type() - reverse the # mapping there to create a nice human-readable description - owner = owner[6:] - if owner.endswith('-arg'): - return '(parameter of %s)' % owner[:-4] - elif owner.endswith('-base'): - return '(base of %s)' % owner[:-5] + defined_in = defined_in[6:] + if defined_in.endswith('-arg'): + return '(parameter of %s)' % defined_in[:-4] + elif defined_in.endswith('-base'): + return '(base of %s)' % defined_in[:-5] else: - assert owner.endswith('-wrapper') + assert defined_in.endswith('-wrapper') # Unreachable and not implemented assert False - if owner.endswith('Kind'): + if defined_in.endswith('Kind'): # See QAPISchema._make_implicit_enum_type() - return '(branch of %s)' % owner[:-4] - return '(%s of %s)' % (self.role, owner) + return '(branch of %s)' % defined_in[:-4] + return '(%s of %s)' % (self.role, defined_in) def describe(self): - return "'%s' %s" % (self.name, self._pretty_owner()) + return "'%s' %s" % (self.name, self._pretty_defined_in()) class QAPISchemaEnumMember(QAPISchemaMember): @@ -1578,7 +1579,7 @@ class QAPISchemaObjectTypeMember(QAPISchemaMember): self.optional = optional def check(self, schema): - assert self.owner + assert self.defined_in self.type = schema.lookup_type(self._type_name) assert self.type @@ -1598,9 +1599,9 @@ class QAPISchemaObjectTypeVariants(object): self.tag_member = tag_member self.variants = variants - def set_owner(self, name): + def set_defined_in(self, name): for v in self.variants: - v.set_owner(name) + v.set_defined_in(name) def check(self, schema, seen): if not self.tag_member: # flat union @@ -1616,7 +1617,7 @@ class QAPISchemaObjectTypeVariants(object): if m.name not in cases: v = QAPISchemaObjectTypeVariant(m.name, 'q_empty', m.ifcond) - v.set_owner(self.tag_member.owner) + v.set_defined_in(self.tag_member.defined_in) self.variants.append(v) assert self.variants for v in self.variants: @@ -1648,8 +1649,8 @@ class QAPISchemaAlternateType(QAPISchemaType): QAPISchemaType.__init__(self, name, info, doc, ifcond) assert isinstance(variants, QAPISchemaObjectTypeVariants) assert variants.tag_member - variants.set_owner(name) - variants.tag_member.set_owner(self.name) + variants.set_defined_in(name) + variants.tag_member.set_defined_in(self.name) self.variants = variants def check(self, schema): @@ -1829,7 +1830,7 @@ class QAPISchema(object): for v in values] def _make_implicit_enum_type(self, name, info, ifcond, values): - # See also QAPISchemaObjectTypeMember._pretty_owner() + # See also QAPISchemaObjectTypeMember._pretty_defined_in() name = name + 'Kind' # Use namespace reserved by add_name() self._def_entity(QAPISchemaEnumType( name, info, None, ifcond, self._make_enum_members(values), None)) @@ -1845,7 +1846,7 @@ class QAPISchema(object): role, members): if not members: return None - # See also QAPISchemaObjectTypeMember._pretty_owner() + # See also QAPISchemaObjectTypeMember._pretty_defined_in() name = 'q_obj_%s-%s' % (name, role) typ = self.lookup_entity(name, QAPISchemaObjectType) if typ: -- cgit v1.1 From 19e950d9d47d3efe4c8d5c6c872a53889a54363c Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:16 +0200 Subject: qapi: New QAPISourceInfo, replacing dict We track source locations with a dict of the form {'file': FNAME, 'line': LINENO, 'parent': PARENT} where PARENT is None for the main file, and the include directive's source location for included files. This is serviceable enough, but the next commit will add information, and that's going to come out cleaner if we turn this into a class. So do that. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-4-armbru@redhat.com> --- scripts/qapi/common.py | 69 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 28 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index bfb3e8a..5843f3e 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -13,6 +13,7 @@ from __future__ import print_function from contextlib import contextmanager +import copy import errno import os import re @@ -53,34 +54,50 @@ struct_types = {} union_types = {} all_names = {} + # # Parsing the schema into expressions # +class QAPISourceInfo(object): + def __init__(self, fname, line, parent): + self.fname = fname + self.line = line + self.parent = parent + + def next_line(self): + info = copy.copy(self) + info.line += 1 + return info + + def loc(self): + return '%s:%d' % (self.fname, self.line) + + def include_path(self): + ret = '' + parent = self.parent + while parent: + ret = 'In file included from %s:\n' % parent.loc() + ret + parent = parent.parent + return ret -def error_path(parent): - res = '' - while parent: - res = ('In file included from %s:%d:\n' % (parent['file'], - parent['line'])) + res - parent = parent['parent'] - return res + def __str__(self): + return self.include_path() + self.loc() class QAPIError(Exception): - def __init__(self, fname, line, col, incl_info, msg): + def __init__(self, info, col, msg): Exception.__init__(self) - self.fname = fname - self.line = line + self.info = info self.col = col - self.info = incl_info self.msg = msg def __str__(self): - loc = '%s:%d' % (self.fname, self.line) + loc = str(self.info) if self.col is not None: + assert self.info.line is not None loc += ':%s' % self.col - return error_path(self.info) + '%s: %s' % (loc, self.msg) + return loc + ': ' + self.msg class QAPIParseError(QAPIError): @@ -91,14 +108,12 @@ class QAPIParseError(QAPIError): col = (col + 7) % 8 + 1 else: col += 1 - QAPIError.__init__(self, parser.fname, parser.line, col, - parser.incl_info, msg) + QAPIError.__init__(self, parser.info, col, msg) class QAPISemError(QAPIError): def __init__(self, info, msg): - QAPIError.__init__(self, info['file'], info['line'], None, - info['parent'], msg) + QAPIError.__init__(self, info, None, msg) class QAPIDoc(object): @@ -382,12 +397,11 @@ class QAPISchemaParser(object): def __init__(self, fp, previously_included=[], incl_info=None): self.fname = fp.name previously_included.append(os.path.abspath(fp.name)) - self.incl_info = incl_info self.src = fp.read() if self.src == '' or self.src[-1] != '\n': self.src += '\n' self.cursor = 0 - self.line = 1 + self.info = QAPISourceInfo(self.fname, 1, incl_info) self.line_pos = 0 self.exprs = [] self.docs = [] @@ -395,8 +409,7 @@ class QAPISchemaParser(object): cur_doc = None while self.tok is not None: - info = {'file': self.fname, 'line': self.line, - 'parent': self.incl_info} + info = self.info if self.tok == '#': self.reject_expr_doc(cur_doc) cur_doc = self.get_doc(info) @@ -456,9 +469,9 @@ class QAPISchemaParser(object): # catch inclusion cycle inf = info while inf: - if incl_abs_fname == os.path.abspath(inf['file']): + if incl_abs_fname == os.path.abspath(inf.fname): raise QAPISemError(info, "Inclusion loop for %s" % include) - inf = inf['parent'] + inf = inf.parent # skip multiple include of the same file if incl_abs_fname in previously_included: @@ -552,7 +565,7 @@ class QAPISchemaParser(object): if self.cursor == len(self.src): self.tok = None return - self.line += 1 + self.info = self.info.next_line() self.line_pos = self.cursor elif not self.tok.isspace(): # Show up to next structural, whitespace or quote @@ -1172,7 +1185,7 @@ class QAPISchemaEntity(object): def check(self, schema): assert not self._checked if self.info: - self._module = os.path.relpath(self.info['file'], + self._module = os.path.relpath(self.info.fname, os.path.dirname(schema.fname)) self._checked = True @@ -1781,9 +1794,9 @@ class QAPISchema(object): include = expr['include'] assert doc is None main_info = info - while main_info['parent']: - main_info = main_info['parent'] - fname = os.path.relpath(include, os.path.dirname(main_info['file'])) + while main_info.parent: + main_info = main_info.parent + fname = os.path.relpath(include, os.path.dirname(main_info.fname)) self._def_entity(QAPISchemaInclude(fname, info)) def _def_builtin_type(self, name, json_type, c_type): -- cgit v1.1 From 7be6c511943613c60b3e5b640e09bdc916be3b65 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:17 +0200 Subject: qapi: Prefix frontend errors with an "in definition" line We take pains to include the offending expression in error messages, e.g. tests/qapi-schema/alternate-any.json:2: alternate 'Alt' member 'one' cannot use type 'any' But not always: tests/qapi-schema/enum-if-invalid.json:2: 'if' condition must be a string or a list of strings Instead of improving them one by one, report the offending expression whenever it is known, like this: tests/qapi-schema/enum-if-invalid.json: In enum 'TestIfEnum': tests/qapi-schema/enum-if-invalid.json:2: 'if' condition must be a string or a list of strings Error messages that mention the offending expression become a bit redundant, e.g. tests/qapi-schema/alternate-any.json: In alternate 'Alt': tests/qapi-schema/alternate-any.json:2: alternate 'Alt' member 'one' cannot use type 'any' I'll take care of that later in this series. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-5-armbru@redhat.com> --- scripts/qapi/common.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 5843f3e..f0e7d5a 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -64,6 +64,12 @@ class QAPISourceInfo(object): self.fname = fname self.line = line self.parent = parent + self.defn_meta = None + self.defn_name = None + + def set_defn(self, meta, name): + self.defn_meta = meta + self.defn_name = name def next_line(self): info = copy.copy(self) @@ -73,6 +79,12 @@ class QAPISourceInfo(object): def loc(self): return '%s:%d' % (self.fname, self.line) + def in_defn(self): + if self.defn_name: + return "%s: In %s '%s':\n" % (self.fname, + self.defn_meta, self.defn_name) + return '' + def include_path(self): ret = '' parent = self.parent @@ -82,7 +94,7 @@ class QAPISourceInfo(object): return ret def __str__(self): - return self.include_path() + self.loc() + return self.include_path() + self.in_defn() + self.loc() class QAPIError(Exception): @@ -1127,6 +1139,7 @@ def check_exprs(exprs): normalize_if(expr) name = expr[meta] add_name(name, info, meta) + info.set_defn(meta, name) if doc and doc.symbol != name: raise QAPISemError(info, "Definition of '%s' follows documentation" " for '%s'" % (name, doc.symbol)) -- cgit v1.1 From 638c4af9310ee1f8bf878da99c87c0af26417679 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:18 +0200 Subject: qapi: Clean up member name case checking QAPISchemaMember.check_clash() checks for member names that map to the same c_name(). Takes care of rejecting duplicate names. It also checks a naming rule: no uppercase in member names. That's a rather odd place to do it. Enforcing naming rules is check_name_str()'s job. qapi-code-gen.txt specifies the name case rule applies to the name as it appears in the schema. check_clash() checks c_name(name) instead. No difference, as c_name() leaves alone case, but unclean. Move the name case check into check_name_str(), less the c_name(). New argument @permit_upper suppresses it. Pass permit_upper=True for definitions (which are not members), and when the member's owner is whitelisted with pragma name-case-whitelist. Bonus: name-case-whitelist now applies to a union's inline base, too. Update qapi/qapi-schema.json pragma to whitelist union CpuInfo instead of CpuInfo's implicit base type's name q_obj_CpuInfo-base. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-6-armbru@redhat.com> --- scripts/qapi/common.py | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index f0e7d5a..ed4bff4 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -704,8 +704,8 @@ valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' '[a-zA-Z][a-zA-Z0-9_-]*$') -def check_name(info, source, name, allow_optional=False, - enum_member=False): +def check_name(info, source, name, + allow_optional=False, enum_member=False, permit_upper=False): global valid_name membername = name @@ -725,11 +725,14 @@ def check_name(info, source, name, allow_optional=False, if not valid_name.match(membername) or \ c_name(membername, False).startswith('q_'): raise QAPISemError(info, "%s uses invalid name '%s'" % (source, name)) + if not permit_upper and name.lower() != name: + raise QAPISemError( + info, "%s uses uppercase in name '%s'" % (source, name)) def add_name(name, info, meta): global all_names - check_name(info, "'%s'" % meta, name) + check_name(info, "'%s'" % meta, name, permit_upper=True) # FIXME should reject names that differ only in '_' vs. '.' # vs. '-', because they're liable to clash in generated C. if name in all_names: @@ -797,10 +800,12 @@ def check_type(info, source, value, raise QAPISemError(info, "%s should be an object or type name" % source) + permit_upper = allow_dict in name_case_whitelist + # value is a dictionary, check that each member is okay for (key, arg) in value.items(): check_name(info, "Member of %s" % source, key, - allow_optional=True) + allow_optional=True, permit_upper=permit_upper) if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'): raise QAPISemError(info, "Member of %s uses reserved name '%s'" % (source, key)) @@ -870,7 +875,7 @@ def check_union(expr, info): else: # The object must have a string or dictionary 'base'. check_type(info, "'base' for union '%s'" % name, - base, allow_dict=True, + base, allow_dict=name, allow_metas=['struct']) if not base: raise QAPISemError(info, "Flat union '%s' must have a base" @@ -982,13 +987,15 @@ def check_enum(expr, info): raise QAPISemError(info, "Enum '%s' requires a string for 'prefix'" % name) + permit_upper = name in name_case_whitelist + for member in members: check_known_keys(info, "member of enum '%s'" % name, member, ['name'], ['if']) check_if(member, info) normalize_if(member) check_name(info, "Member of enum '%s'" % name, member['name'], - enum_member=True) + enum_member=True, permit_upper=permit_upper) def check_struct(expr, info): @@ -997,7 +1004,7 @@ def check_struct(expr, info): features = expr.get('features') check_type(info, "'data' for struct '%s'" % name, members, - allow_dict=True) + allow_dict=name) check_type(info, "'base' for struct '%s'" % name, expr.get('base'), allow_metas=['struct']) @@ -1555,10 +1562,6 @@ class QAPISchemaMember(object): def check_clash(self, info, seen): cname = c_name(self.name) - if (cname.lower() != cname - and self.defined_in not in name_case_whitelist): - raise QAPISemError(info, - "%s should not use uppercase" % self.describe()) if cname in seen: raise QAPISemError(info, "%s collides with %s" % (self.describe(), seen[cname].describe())) -- cgit v1.1 From 2ab218aad6e2ddf4e95a7d583492ad7142927ca5 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:19 +0200 Subject: qapi: Change frontend error messages to start with lower case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Starting error messages with a capital letter complicates things when text can get interpolated both at the beginning and in the middle of an error message. The next patch will do that. Switch to lower case to keep it simpler. For what it's worth, the GNU Coding Standards advise the message "should not begin with a capital letter when it follows a program name and/or file name, because that isn’t the beginning of a sentence. (The sentence conceptually starts at the beginning of the line.)" While there, avoid breaking lines containing multiple arguments in the middle of an argument. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-7-armbru@redhat.com> --- scripts/qapi/common.py | 192 ++++++++++++++++++++++++++----------------------- 1 file changed, 102 insertions(+), 90 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index ed4bff4..3d73332 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -207,7 +207,7 @@ class QAPIDoc(object): return if line[0] != ' ': - raise QAPIParseError(self._parser, "Missing space after #") + raise QAPIParseError(self._parser, "missing space after #") line = line[1:] self._append_line(line) @@ -241,11 +241,11 @@ class QAPIDoc(object): # recognized, and get silently treated as ordinary text if not self.symbol and not self.body.text and line.startswith('@'): if not line.endswith(':'): - raise QAPIParseError(self._parser, "Line should end with ':'") + raise QAPIParseError(self._parser, "line should end with ':'") self.symbol = line[1:-1] # FIXME invalid names other than the empty string aren't flagged if not self.symbol: - raise QAPIParseError(self._parser, "Invalid name") + raise QAPIParseError(self._parser, "invalid name") elif self.symbol: # This is a definition documentation block if name.startswith('@') and name.endswith(':'): @@ -344,7 +344,7 @@ class QAPIDoc(object): def _start_symbol_section(self, symbols_dict, name): # FIXME invalid names other than the empty string aren't flagged if not name: - raise QAPIParseError(self._parser, "Invalid parameter name") + raise QAPIParseError(self._parser, "invalid parameter name") if name in symbols_dict: raise QAPIParseError(self._parser, "'%s' parameter name duplicated" % name) @@ -362,7 +362,7 @@ class QAPIDoc(object): def _start_section(self, name=None): if name in ('Returns', 'Since') and self.has_section(name): raise QAPIParseError(self._parser, - "Duplicated '%s' section" % name) + "duplicated '%s' section" % name) self._end_section() self._section = QAPIDoc.Section(name) self.sections.append(self._section) @@ -371,8 +371,9 @@ class QAPIDoc(object): if self._section: text = self._section.text = self._section.text.strip() if self._section.name and (not text or text.isspace()): - raise QAPIParseError(self._parser, "Empty doc section '%s'" - % self._section.name) + raise QAPIParseError( + self._parser, + "empty doc section '%s'" % self._section.name) self._section = None def _append_freeform(self, line): @@ -400,7 +401,7 @@ class QAPIDoc(object): if bogus: raise QAPISemError( self.info, - "The following documented members are not in " + "the following documented members are not in " "the declaration: %s" % ", ".join(bogus)) @@ -432,11 +433,11 @@ class QAPISchemaParser(object): if 'include' in expr: self.reject_expr_doc(cur_doc) if len(expr) != 1: - raise QAPISemError(info, "Invalid 'include' directive") + raise QAPISemError(info, "invalid 'include' directive") include = expr['include'] if not isinstance(include, str): raise QAPISemError(info, - "Value of 'include' must be a string") + "value of 'include' must be a string") incl_fname = os.path.join(os.path.dirname(self.fname), include) self.exprs.append({'expr': {'include': incl_fname}, @@ -449,11 +450,11 @@ class QAPISchemaParser(object): elif "pragma" in expr: self.reject_expr_doc(cur_doc) if len(expr) != 1: - raise QAPISemError(info, "Invalid 'pragma' directive") + raise QAPISemError(info, "invalid 'pragma' directive") pragma = expr['pragma'] if not isinstance(pragma, dict): raise QAPISemError( - info, "Value of 'pragma' must be an object") + info, "value of 'pragma' must be an object") for name, value in pragma.items(): self._pragma(name, value, info) else: @@ -462,7 +463,7 @@ class QAPISchemaParser(object): if cur_doc: if not cur_doc.symbol: raise QAPISemError( - cur_doc.info, "Definition documentation required") + cur_doc.info, "definition documentation required") expr_elem['doc'] = cur_doc self.exprs.append(expr_elem) cur_doc = None @@ -473,7 +474,7 @@ class QAPISchemaParser(object): if doc and doc.symbol: raise QAPISemError( doc.info, - "Documentation for '%s' is not followed by the definition" + "documentation for '%s' is not followed by the definition" % doc.symbol) def _include(self, include, info, incl_fname, previously_included): @@ -482,7 +483,7 @@ class QAPISchemaParser(object): inf = info while inf: if incl_abs_fname == os.path.abspath(inf.fname): - raise QAPISemError(info, "Inclusion loop for %s" % include) + raise QAPISemError(info, "inclusion loop for %s" % include) inf = inf.parent # skip multiple include of the same file @@ -503,24 +504,24 @@ class QAPISchemaParser(object): if name == 'doc-required': if not isinstance(value, bool): raise QAPISemError(info, - "Pragma 'doc-required' must be boolean") + "pragma 'doc-required' must be boolean") doc_required = value elif name == 'returns-whitelist': if (not isinstance(value, list) or any([not isinstance(elt, str) for elt in value])): - raise QAPISemError(info, - "Pragma returns-whitelist must be" - " a list of strings") + raise QAPISemError( + info, + "pragma returns-whitelist must be a list of strings") returns_whitelist = value elif name == 'name-case-whitelist': if (not isinstance(value, list) or any([not isinstance(elt, str) for elt in value])): - raise QAPISemError(info, - "Pragma name-case-whitelist must be" - " a list of strings") + raise QAPISemError( + info, + "pragma name-case-whitelist must be a list of strings") name_case_whitelist = value else: - raise QAPISemError(info, "Unknown pragma '%s'" % name) + raise QAPISemError(info, "unknown pragma '%s'" % name) def accept(self, skip_comment=True): while True: @@ -547,13 +548,13 @@ class QAPISchemaParser(object): ch = self.src[self.cursor] self.cursor += 1 if ch == '\n': - raise QAPIParseError(self, "Missing terminating \"'\"") + raise QAPIParseError(self, "missing terminating \"'\"") if esc: # Note: we recognize only \\ because we have # no use for funny characters in strings if ch != '\\': raise QAPIParseError(self, - "Unknown escape \\%s" % ch) + "unknown escape \\%s" % ch) esc = False elif ch == '\\': esc = True @@ -563,7 +564,7 @@ class QAPISchemaParser(object): return if ord(ch) < 32 or ord(ch) >= 127: raise QAPIParseError( - self, "Funny character in string") + self, "funny character in string") string += ch elif self.src.startswith('true', self.pos): self.val = True @@ -584,7 +585,7 @@ class QAPISchemaParser(object): # character match = re.match('[^[\\]{}:,\\s\'"]+', self.src[self.cursor-1:]) - raise QAPIParseError(self, "Stray '%s'" % match.group(0)) + raise QAPIParseError(self, "stray '%s'" % match.group(0)) def get_members(self): expr = OrderedDict() @@ -592,24 +593,24 @@ class QAPISchemaParser(object): self.accept() return expr if self.tok != "'": - raise QAPIParseError(self, "Expected string or '}'") + raise QAPIParseError(self, "expected string or '}'") while True: key = self.val self.accept() if self.tok != ':': - raise QAPIParseError(self, "Expected ':'") + raise QAPIParseError(self, "expected ':'") self.accept() if key in expr: - raise QAPIParseError(self, "Duplicate key '%s'" % key) + raise QAPIParseError(self, "duplicate key '%s'" % key) expr[key] = self.get_expr(True) if self.tok == '}': self.accept() return expr if self.tok != ',': - raise QAPIParseError(self, "Expected ',' or '}'") + raise QAPIParseError(self, "expected ',' or '}'") self.accept() if self.tok != "'": - raise QAPIParseError(self, "Expected string") + raise QAPIParseError(self, "expected string") def get_values(self): expr = [] @@ -618,19 +619,19 @@ class QAPISchemaParser(object): return expr if self.tok not in "{['tfn": raise QAPIParseError( - self, "Expected '{', '[', ']', string, boolean or 'null'") + self, "expected '{', '[', ']', string, boolean or 'null'") while True: expr.append(self.get_expr(True)) if self.tok == ']': self.accept() return expr if self.tok != ',': - raise QAPIParseError(self, "Expected ',' or ']'") + raise QAPIParseError(self, "expected ',' or ']'") self.accept() def get_expr(self, nested): if self.tok != '{' and not nested: - raise QAPIParseError(self, "Expected '{'") + raise QAPIParseError(self, "expected '{'") if self.tok == '{': self.accept() expr = self.get_members() @@ -642,13 +643,13 @@ class QAPISchemaParser(object): self.accept() else: raise QAPIParseError( - self, "Expected '{', '[', string, boolean or 'null'") + self, "expected '{', '[', string, boolean or 'null'") return expr def get_doc(self, info): if self.val != '##': - raise QAPIParseError(self, "Junk after '##' at start of " - "documentation comment") + raise QAPIParseError( + self, "junk after '##' at start of documentation comment") doc = QAPIDoc(self, info) self.accept(False) @@ -656,8 +657,9 @@ class QAPISchemaParser(object): if self.val.startswith('##'): # End of doc comment if self.val != '##': - raise QAPIParseError(self, "Junk after '##' at end of " - "documentation comment") + raise QAPIParseError( + self, + "junk after '##' at end of documentation comment") doc.end_comment() self.accept() return doc @@ -665,7 +667,7 @@ class QAPISchemaParser(object): doc.append(self.val) self.accept(False) - raise QAPIParseError(self, "Documentation comment must end with '##'") + raise QAPIParseError(self, "documentation comment must end with '##'") # @@ -804,18 +806,18 @@ def check_type(info, source, value, # value is a dictionary, check that each member is okay for (key, arg) in value.items(): - check_name(info, "Member of %s" % source, key, + check_name(info, "member of %s" % source, key, allow_optional=True, permit_upper=permit_upper) if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'): - raise QAPISemError(info, "Member of %s uses reserved name '%s'" - % (source, key)) + raise QAPISemError( + info, "member of %s uses reserved name '%s'" % (source, key)) # Todo: allow dictionaries to represent default values of # an optional argument. check_known_keys(info, "member '%s' of %s" % (key, source), arg, ['type'], ['if']) check_if(arg, info) normalize_if(arg) - check_type(info, "Member '%s' of %s" % (key, source), + check_type(info, "member '%s' of %s" % (key, source), arg['type'], allow_array=True, allow_metas=['built-in', 'union', 'alternate', 'struct', 'enum']) @@ -868,8 +870,8 @@ def check_union(expr, info): enum_values = members.keys() allow_metas = ['built-in', 'union', 'alternate', 'struct', 'enum'] if base is not None: - raise QAPISemError(info, "Simple union '%s' must not have a base" % - name) + raise QAPISemError( + info, "simple union '%s' must not have a base" % name) # Else, it's a flat union. else: @@ -878,46 +880,47 @@ def check_union(expr, info): base, allow_dict=name, allow_metas=['struct']) if not base: - raise QAPISemError(info, "Flat union '%s' must have a base" - % name) + raise QAPISemError( + info, "flat union '%s' must have a base" % name) base_members = find_base_members(base) assert base_members is not None # The value of member 'discriminator' must name a non-optional # member of the base struct. - check_name(info, "Discriminator of flat union '%s'" % name, + check_name(info, "discriminator of flat union '%s'" % name, discriminator) discriminator_value = base_members.get(discriminator) if not discriminator_value: raise QAPISemError(info, - "Discriminator '%s' is not a member of 'base'" + "discriminator '%s' is not a member of 'base'" % discriminator) if discriminator_value.get('if'): raise QAPISemError( info, - "The discriminator '%s' for union %s must not be conditional" + "the discriminator '%s' for union %s must not be conditional" % (discriminator, name)) enum_define = enum_types.get(discriminator_value['type']) # Do not allow string discriminator if not enum_define: - raise QAPISemError(info, - "Discriminator '%s' must be of enumeration " - "type" % discriminator) + raise QAPISemError( + info, + "discriminator '%s' must be of enumeration type" + % discriminator) enum_values = enum_get_names(enum_define) allow_metas = ['struct'] if (len(enum_values) == 0): - raise QAPISemError(info, "Union '%s' has no branches" % name) + raise QAPISemError(info, "union '%s' has no branches" % name) for (key, value) in members.items(): - check_name(info, "Member of union '%s'" % name, key) + check_name(info, "member of union '%s'" % name, key) check_known_keys(info, "member '%s' of union '%s'" % (key, name), value, ['type'], ['if']) check_if(value, info) normalize_if(value) # Each value must name a known type - check_type(info, "Member '%s' of union '%s'" % (key, name), + check_type(info, "member '%s' of union '%s'" % (key, name), value['type'], allow_array=not base, allow_metas=allow_metas) @@ -925,10 +928,10 @@ def check_union(expr, info): # of 'data' must also be members of the enum type. if discriminator is not None: if key not in enum_values: - raise QAPISemError(info, - "Discriminator value '%s' is not found in " - "enum '%s'" - % (key, enum_define['enum'])) + raise QAPISemError( + info, + "discriminator value '%s' is not found in enum '%s'" + % (key, enum_define['enum'])) def check_alternate(expr, info): @@ -938,9 +941,9 @@ def check_alternate(expr, info): if len(members) == 0: raise QAPISemError(info, - "Alternate '%s' cannot have empty 'data'" % name) + "alternate '%s' cannot have empty 'data'" % name) for (key, value) in members.items(): - check_name(info, "Member of alternate '%s'" % name, key) + check_name(info, "member of alternate '%s'" % name, key) check_known_keys(info, "member '%s' of alternate '%s'" % (key, name), value, ['type'], ['if']) @@ -949,12 +952,14 @@ def check_alternate(expr, info): typ = value['type'] # Ensure alternates have no type conflicts. - check_type(info, "Member '%s' of alternate '%s'" % (key, name), typ, + check_type(info, "member '%s' of alternate '%s'" % (key, name), typ, allow_metas=['built-in', 'union', 'struct', 'enum']) qtype = find_alternate_member_qtype(typ) if not qtype: - raise QAPISemError(info, "Alternate '%s' member '%s' cannot use " - "type '%s'" % (name, key, typ)) + raise QAPISemError( + info, + "alternate '%s' member '%s' cannot use type '%s'" + % (name, key, typ)) conflicting = set([qtype]) if qtype == 'QTYPE_QSTRING': enum_expr = enum_types.get(typ) @@ -969,9 +974,11 @@ def check_alternate(expr, info): conflicting.add('QTYPE_QBOOL') for qt in conflicting: if qt in types_seen: - raise QAPISemError(info, "Alternate '%s' member '%s' can't " - "be distinguished from member '%s'" - % (name, key, types_seen[qt])) + raise QAPISemError( + info, + "alternate '%s' member '%s' can't be distinguished " + "from member '%s'" + % (name, key, types_seen[qt])) types_seen[qt] = key @@ -982,10 +989,10 @@ def check_enum(expr, info): if not isinstance(members, list): raise QAPISemError(info, - "Enum '%s' requires an array for 'data'" % name) + "enum '%s' requires an array for 'data'" % name) if prefix is not None and not isinstance(prefix, str): raise QAPISemError(info, - "Enum '%s' requires a string for 'prefix'" % name) + "enum '%s' requires a string for 'prefix'" % name) permit_upper = name in name_case_whitelist @@ -994,7 +1001,7 @@ def check_enum(expr, info): ['name'], ['if']) check_if(member, info) normalize_if(member) - check_name(info, "Member of enum '%s'" % name, member['name'], + check_name(info, "member of enum '%s'" % name, member['name'], enum_member=True, permit_upper=permit_upper) @@ -1010,9 +1017,8 @@ def check_struct(expr, info): if features: if not isinstance(features, list): - raise QAPISemError(info, - "Struct '%s' requires an array for 'features'" % - name) + raise QAPISemError( + info, "struct '%s' requires an array for 'features'" % name) for f in features: assert isinstance(f, dict) check_known_keys(info, "feature of struct %s" % name, f, @@ -1020,7 +1026,7 @@ def check_struct(expr, info): check_if(f, info) normalize_if(f) - check_name(info, "Feature of struct %s" % name, f['name']) + check_name(info, "feature of struct %s" % name, f['name']) def check_known_keys(info, source, value, required, optional): @@ -1030,15 +1036,19 @@ def check_known_keys(info, source, value, required, optional): missing = set(required) - set(value) if missing: - raise QAPISemError(info, "Key%s %s %s missing from %s" - % ('s' if len(missing) > 1 else '', pprint(missing), - 'are' if len(missing) > 1 else 'is', source)) + raise QAPISemError( + info, + "key%s %s %s missing from %s" + % ('s' if len(missing) > 1 else '', pprint(missing), + 'are' if len(missing) > 1 else 'is', source)) allowed = set(required + optional) unknown = set(value) - allowed if unknown: - raise QAPISemError(info, "Unknown key%s %s in %s\nValid keys are %s." - % ('s' if len(unknown) > 1 else '', pprint(unknown), - source, pprint(allowed))) + raise QAPISemError( + info, + "unknown key%s %s in %s\nValid keys are %s." + % ('s' if len(unknown) > 1 else '', pprint(unknown), + source, pprint(allowed))) def check_keys(expr, info, meta, required, optional=[]): @@ -1106,7 +1116,7 @@ def check_exprs(exprs): if not doc and doc_required: raise QAPISemError(info, - "Definition missing documentation comment") + "definition missing documentation comment") if 'enum' in expr: meta = 'enum' @@ -1142,14 +1152,16 @@ def check_exprs(exprs): check_keys(expr, info, 'event', [], ['data', 'boxed', 'if']) normalize_members(expr.get('data')) else: - raise QAPISemError(info, "Expression is missing metatype") + raise QAPISemError(info, "expression is missing metatype") normalize_if(expr) name = expr[meta] add_name(name, info, meta) info.set_defn(meta, name) if doc and doc.symbol != name: - raise QAPISemError(info, "Definition of '%s' follows documentation" - " for '%s'" % (name, doc.symbol)) + raise QAPISemError( + info, + "definition of '%s' follows documentation for '%s'" + % (name, doc.symbol)) # Validate that exprs make sense for expr_elem in exprs: @@ -1462,7 +1474,7 @@ class QAPISchemaObjectType(QAPISchemaType): if self._checked: # Recursed: C struct contains itself raise QAPISemError(self.info, - "Object %s contains itself" % self.name) + "object %s contains itself" % self.name) QAPISchemaType.check(self, schema) assert self._checked and self.members is None @@ -1734,7 +1746,7 @@ class QAPISchemaCommand(QAPISchemaEntity): assert isinstance(self.arg_type, QAPISchemaObjectType) assert not self.arg_type.variants or self.boxed elif self.boxed: - raise QAPISemError(self.info, "Use of 'boxed' requires 'data'") + raise QAPISemError(self.info, "use of 'boxed' requires 'data'") if self._ret_type_name: self.ret_type = schema.lookup_type(self._ret_type_name) assert isinstance(self.ret_type, QAPISchemaType) @@ -1763,7 +1775,7 @@ class QAPISchemaEvent(QAPISchemaEntity): assert isinstance(self.arg_type, QAPISchemaObjectType) assert not self.arg_type.variants or self.boxed elif self.boxed: - raise QAPISemError(self.info, "Use of 'boxed' requires 'data'") + raise QAPISemError(self.info, "use of 'boxed' requires 'data'") def visit(self, visitor): QAPISchemaEntity.visit(self, visitor) -- cgit v1.1 From 481a6bd15c4fb99429c3337584c66b40384cb09c Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:20 +0200 Subject: qapi: Improve reporting of member name clashes We report name clashes like this: struct-base-clash.json: In struct 'Sub': struct-base-clash.json:5: 'name' (member of Sub) collides with 'name' (member of Base) The "(member of Sub)" is redundant with "In struct 'Sub'". Comes from QAPISchemaMember.describe(). Pass info to it, so it can detect the redundancy and avoid it. Result: struct-base-clash.json: In struct 'Sub': struct-base-clash.json:5: member 'name' collides with member 'name' of type 'Base' Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-8-armbru@redhat.com> --- scripts/qapi/common.py | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 3d73332..14d1e34 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -1575,31 +1575,41 @@ class QAPISchemaMember(object): def check_clash(self, info, seen): cname = c_name(self.name) if cname in seen: - raise QAPISemError(info, "%s collides with %s" % - (self.describe(), seen[cname].describe())) + raise QAPISemError( + info, + "%s collides with %s" + % (self.describe(info), seen[cname].describe(info))) seen[cname] = self - def _pretty_defined_in(self): + def describe(self, info): + role = self.role defined_in = self.defined_in + assert defined_in + if defined_in.startswith('q_obj_'): # See QAPISchema._make_implicit_object_type() - reverse the # mapping there to create a nice human-readable description defined_in = defined_in[6:] if defined_in.endswith('-arg'): - return '(parameter of %s)' % defined_in[:-4] + # Implicit type created for a command's dict 'data' + assert role == 'member' + role = 'parameter' elif defined_in.endswith('-base'): - return '(base of %s)' % defined_in[:-5] + # Implicit type created for a flat union's dict 'base' + role = 'base ' + role else: + # Implicit type created for a simple union's branch assert defined_in.endswith('-wrapper') # Unreachable and not implemented assert False - if defined_in.endswith('Kind'): + elif defined_in.endswith('Kind'): # See QAPISchema._make_implicit_enum_type() - return '(branch of %s)' % defined_in[:-4] - return '(%s of %s)' % (self.role, defined_in) - - def describe(self): - return "'%s' %s" % (self.name, self._pretty_defined_in()) + # Implicit enum created for simple union's branches + assert role == 'value' + role = 'branch' + elif defined_in != info.defn_name: + return "%s '%s' of type '%s'" % (role, self.name, defined_in) + return "%s '%s'" % (role, self.name) class QAPISchemaEnumMember(QAPISchemaMember): @@ -1871,7 +1881,7 @@ class QAPISchema(object): for v in values] def _make_implicit_enum_type(self, name, info, ifcond, values): - # See also QAPISchemaObjectTypeMember._pretty_defined_in() + # See also QAPISchemaObjectTypeMember.describe() name = name + 'Kind' # Use namespace reserved by add_name() self._def_entity(QAPISchemaEnumType( name, info, None, ifcond, self._make_enum_members(values), None)) @@ -1887,7 +1897,7 @@ class QAPISchema(object): role, members): if not members: return None - # See also QAPISchemaObjectTypeMember._pretty_defined_in() + # See also QAPISchemaObjectTypeMember.describe() name = 'q_obj_%s-%s' % (name, role) typ = self.lookup_entity(name, QAPISchemaObjectType) if typ: -- cgit v1.1 From c9efc984ca5b5f75f184680e1ab9a7784d241578 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:21 +0200 Subject: qapi: Reorder check_FOO() parameters for consistency Most check_FOO() take the thing being checked as first argument. check_name(), check_type(), check_known_keys() don't. Clean that up. While there, drop a "Todo" comment that should have been dropped in commit 87adbbffd4 "qapi: add a dictionary form for TYPE". Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-9-armbru@redhat.com> --- scripts/qapi/common.py | 80 ++++++++++++++++++++++++-------------------------- 1 file changed, 39 insertions(+), 41 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 14d1e34..c909821 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -706,7 +706,7 @@ valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' '[a-zA-Z][a-zA-Z0-9_-]*$') -def check_name(info, source, name, +def check_name(name, info, source, allow_optional=False, enum_member=False, permit_upper=False): global valid_name membername = name @@ -734,7 +734,7 @@ def check_name(info, source, name, def add_name(name, info, meta): global all_names - check_name(info, "'%s'" % meta, name, permit_upper=True) + check_name(name, info, "'%s'" % meta, permit_upper=True) # FIXME should reject names that differ only in '_' vs. '.' # vs. '-', because they're liable to clash in generated C. if name in all_names: @@ -768,7 +768,7 @@ def check_if(expr, info): check_if_str(ifcond, info) -def check_type(info, source, value, +def check_type(value, info, source, allow_array=False, allow_dict=False, allow_metas=[]): global all_names @@ -806,19 +806,17 @@ def check_type(info, source, value, # value is a dictionary, check that each member is okay for (key, arg) in value.items(): - check_name(info, "member of %s" % source, key, + check_name(key, info, "member of %s" % source, allow_optional=True, permit_upper=permit_upper) if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'): raise QAPISemError( info, "member of %s uses reserved name '%s'" % (source, key)) - # Todo: allow dictionaries to represent default values of - # an optional argument. - check_known_keys(info, "member '%s' of %s" % (key, source), - arg, ['type'], ['if']) + check_known_keys(arg, info, "member '%s' of %s" % (key, source), + ['type'], ['if']) check_if(arg, info) normalize_if(arg) - check_type(info, "member '%s' of %s" % (key, source), - arg['type'], allow_array=True, + check_type(arg['type'], info, "member '%s' of %s" % (key, source), + allow_array=True, allow_metas=['built-in', 'union', 'alternate', 'struct', 'enum']) @@ -830,15 +828,15 @@ def check_command(expr, info): args_meta = ['struct'] if boxed: args_meta += ['union'] - check_type(info, "'data' for command '%s'" % name, - expr.get('data'), allow_dict=not boxed, - allow_metas=args_meta) + check_type(expr.get('data'), info, + "'data' for command '%s'" % name, + allow_dict=not boxed, allow_metas=args_meta) returns_meta = ['union', 'struct'] if name in returns_whitelist: returns_meta += ['built-in', 'alternate', 'enum'] - check_type(info, "'returns' for command '%s'" % name, - expr.get('returns'), allow_array=True, - allow_metas=returns_meta) + check_type(expr.get('returns'), info, + "'returns' for command '%s'" % name, + allow_array=True, allow_metas=returns_meta) def check_event(expr, info): @@ -848,9 +846,9 @@ def check_event(expr, info): meta = ['struct'] if boxed: meta += ['union'] - check_type(info, "'data' for event '%s'" % name, - expr.get('data'), allow_dict=not boxed, - allow_metas=meta) + check_type(expr.get('data'), info, + "'data' for event '%s'" % name, + allow_dict=not boxed, allow_metas=meta) def enum_get_names(expr): @@ -876,9 +874,8 @@ def check_union(expr, info): # Else, it's a flat union. else: # The object must have a string or dictionary 'base'. - check_type(info, "'base' for union '%s'" % name, - base, allow_dict=name, - allow_metas=['struct']) + check_type(base, info, "'base' for union '%s'" % name, + allow_dict=name, allow_metas=['struct']) if not base: raise QAPISemError( info, "flat union '%s' must have a base" % name) @@ -887,8 +884,8 @@ def check_union(expr, info): # The value of member 'discriminator' must name a non-optional # member of the base struct. - check_name(info, "discriminator of flat union '%s'" % name, - discriminator) + check_name(discriminator, info, + "discriminator of flat union '%s'" % name) discriminator_value = base_members.get(discriminator) if not discriminator_value: raise QAPISemError(info, @@ -913,15 +910,16 @@ def check_union(expr, info): raise QAPISemError(info, "union '%s' has no branches" % name) for (key, value) in members.items(): - check_name(info, "member of union '%s'" % name, key) + check_name(key, info, "member of union '%s'" % name) - check_known_keys(info, "member '%s' of union '%s'" % (key, name), - value, ['type'], ['if']) + check_known_keys(value, info, + "member '%s' of union '%s'" % (key, name), + ['type'], ['if']) check_if(value, info) normalize_if(value) # Each value must name a known type - check_type(info, "member '%s' of union '%s'" % (key, name), - value['type'], + check_type(value['type'], info, + "member '%s' of union '%s'" % (key, name), allow_array=not base, allow_metas=allow_metas) # If the discriminator names an enum type, then all members @@ -943,16 +941,16 @@ def check_alternate(expr, info): raise QAPISemError(info, "alternate '%s' cannot have empty 'data'" % name) for (key, value) in members.items(): - check_name(info, "member of alternate '%s'" % name, key) - check_known_keys(info, + check_name(key, info, "member of alternate '%s'" % name) + check_known_keys(value, info, "member '%s' of alternate '%s'" % (key, name), - value, ['type'], ['if']) + ['type'], ['if']) check_if(value, info) normalize_if(value) typ = value['type'] # Ensure alternates have no type conflicts. - check_type(info, "member '%s' of alternate '%s'" % (key, name), typ, + check_type(typ, info, "member '%s' of alternate '%s'" % (key, name), allow_metas=['built-in', 'union', 'struct', 'enum']) qtype = find_alternate_member_qtype(typ) if not qtype: @@ -997,11 +995,11 @@ def check_enum(expr, info): permit_upper = name in name_case_whitelist for member in members: - check_known_keys(info, "member of enum '%s'" % name, member, + check_known_keys(member, info, "member of enum '%s'" % name, ['name'], ['if']) check_if(member, info) normalize_if(member) - check_name(info, "member of enum '%s'" % name, member['name'], + check_name(member['name'], info, "member of enum '%s'" % name, enum_member=True, permit_upper=permit_upper) @@ -1010,9 +1008,9 @@ def check_struct(expr, info): members = expr['data'] features = expr.get('features') - check_type(info, "'data' for struct '%s'" % name, members, + check_type(members, info, "'data' for struct '%s'" % name, allow_dict=name) - check_type(info, "'base' for struct '%s'" % name, expr.get('base'), + check_type(expr.get('base'), info, "'base' for struct '%s'" % name, allow_metas=['struct']) if features: @@ -1021,15 +1019,15 @@ def check_struct(expr, info): info, "struct '%s' requires an array for 'features'" % name) for f in features: assert isinstance(f, dict) - check_known_keys(info, "feature of struct %s" % name, f, + check_known_keys(f, info, "feature of struct %s" % name, ['name'], ['if']) check_if(f, info) normalize_if(f) - check_name(info, "feature of struct %s" % name, f['name']) + check_name(f['name'], info, "feature of struct %s" % name) -def check_known_keys(info, source, value, required, optional): +def check_known_keys(value, info, source, required, optional): def pprint(elems): return ', '.join("'" + e + "'" for e in sorted(elems)) @@ -1057,7 +1055,7 @@ def check_keys(expr, info, meta, required, optional=[]): raise QAPISemError(info, "'%s' key must have a string value" % meta) required = required + [meta] source = "%s '%s'" % (meta, name) - check_known_keys(info, source, expr, required, optional) + check_known_keys(expr, info, source, required, optional) for (key, value) in expr.items(): if key in ['gen', 'success-response'] and value is not False: raise QAPISemError(info, -- cgit v1.1 From d7bc17c602f128bb358376e6976b3b5dee1ad732 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:22 +0200 Subject: qapi: Improve reporting of invalid name errors Split check_name() into check_name_is_str() and check_name_str(), keep check_name() as a wrapper. Move add_name()'s call into its caller check_exprs(), and inline. This permits delaying check_name_str() there, so its error message gains an "in definition" line. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-10-armbru@redhat.com> --- scripts/qapi/common.py | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index c909821..6f35cd1 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -708,11 +708,22 @@ valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' def check_name(name, info, source, allow_optional=False, enum_member=False, permit_upper=False): - global valid_name - membername = name + check_name_is_str(name, info, source) + check_name_str(name, info, source, + allow_optional, enum_member, permit_upper) + +def check_name_is_str(name, info, source): if not isinstance(name, str): raise QAPISemError(info, "%s requires a string name" % source) + + +def check_name_str(name, info, source, + allow_optional=False, enum_member=False, + permit_upper=False): + global valid_name + membername = name + if name.startswith('*'): membername = name[1:] if not allow_optional: @@ -734,7 +745,6 @@ def check_name(name, info, source, def add_name(name, info, meta): global all_names - check_name(name, info, "'%s'" % meta, permit_upper=True) # FIXME should reject names that differ only in '_' vs. '.' # vs. '-', because they're liable to clash in generated C. if name in all_names: @@ -1153,8 +1163,10 @@ def check_exprs(exprs): raise QAPISemError(info, "expression is missing metatype") normalize_if(expr) name = expr[meta] - add_name(name, info, meta) + check_name_is_str(name, info, "'%s'" % meta) info.set_defn(meta, name) + check_name_str(name, info, "'%s'" % meta, permit_upper=True) + add_name(name, info, meta) if doc and doc.symbol != name: raise QAPISemError( info, -- cgit v1.1 From 6ba1ba7f0e54f100af8d4e28e9bc9978c971c0e0 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:23 +0200 Subject: qapi: Use check_name_str() where it suffices Replace check_name() by check_name_str() where the name is known to be a string. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-11-armbru@redhat.com> --- scripts/qapi/common.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 6f35cd1..d0d997f 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -816,8 +816,8 @@ def check_type(value, info, source, # value is a dictionary, check that each member is okay for (key, arg) in value.items(): - check_name(key, info, "member of %s" % source, - allow_optional=True, permit_upper=permit_upper) + check_name_str(key, info, "member of %s" % source, + allow_optional=True, permit_upper=permit_upper) if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'): raise QAPISemError( info, "member of %s uses reserved name '%s'" % (source, key)) @@ -920,8 +920,7 @@ def check_union(expr, info): raise QAPISemError(info, "union '%s' has no branches" % name) for (key, value) in members.items(): - check_name(key, info, "member of union '%s'" % name) - + check_name_str(key, info, "member of union '%s'" % name) check_known_keys(value, info, "member '%s' of union '%s'" % (key, name), ['type'], ['if']) @@ -951,7 +950,7 @@ def check_alternate(expr, info): raise QAPISemError(info, "alternate '%s' cannot have empty 'data'" % name) for (key, value) in members.items(): - check_name(key, info, "member of alternate '%s'" % name) + check_name_str(key, info, "member of alternate '%s'" % name) check_known_keys(value, info, "member '%s' of alternate '%s'" % (key, name), ['type'], ['if']) -- cgit v1.1 From 64e04f7149dd7f0f32b8e7aa5a89a0c1e6d0b5d6 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:24 +0200 Subject: qapi: Report invalid '*' prefix like any other invalid name The special "does not allow optional name" error is well meant, but confusing in practice. Drop it. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-12-armbru@redhat.com> --- scripts/qapi/common.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index d0d997f..a4cf41f 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -724,11 +724,8 @@ def check_name_str(name, info, source, global valid_name membername = name - if name.startswith('*'): + if allow_optional and name.startswith('*'): membername = name[1:] - if not allow_optional: - raise QAPISemError(info, "%s does not allow optional name '%s'" - % (source, name)) # Enum members can start with a digit, because the generated C # code always prefixes it with the enum name if enum_member and membername[0].isdigit(): @@ -741,6 +738,7 @@ def check_name_str(name, info, source, if not permit_upper and name.lower() != name: raise QAPISemError( info, "%s uses uppercase in name '%s'" % (source, name)) + assert not membername.startswith('*') def add_name(name, info, meta): -- cgit v1.1 From 67fa64ce0ef92943a25de0f0760f8cfc10c2bbf3 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:25 +0200 Subject: qapi: Move check for reserved names out of add_name() The checks for reserved names are spread far and wide. Move one from add_name() to new check_defn_name_str(). This is a first step towards collecting them all in dedicated name checking functions next to check_name(). While there, drop the quotes around the meta-type in check_name_str()'s error messages: "'command' uses ... name 'NAME'" becomes "command uses ... name 'NAME'". Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-13-armbru@redhat.com> --- scripts/qapi/common.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index a4cf41f..111a4bb 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -741,6 +741,13 @@ def check_name_str(name, info, source, assert not membername.startswith('*') +def check_defn_name_str(name, info, meta): + check_name_str(name, info, meta, permit_upper=True) + if name.endswith('Kind') or name.endswith('List'): + raise QAPISemError( + info, "%s '%s' should not end in '%s'" % (meta, name, name[-4:])) + + def add_name(name, info, meta): global all_names # FIXME should reject names that differ only in '_' vs. '.' @@ -748,9 +755,6 @@ def add_name(name, info, meta): if name in all_names: raise QAPISemError(info, "%s '%s' is already defined" % (all_names[name], name)) - if name.endswith('Kind') or name.endswith('List'): - raise QAPISemError(info, "%s '%s' should not end in '%s'" - % (meta, name, name[-4:])) all_names[name] = meta @@ -1162,7 +1166,7 @@ def check_exprs(exprs): name = expr[meta] check_name_is_str(name, info, "'%s'" % meta) info.set_defn(meta, name) - check_name_str(name, info, "'%s'" % meta, permit_upper=True) + check_defn_name_str(name, info, meta) add_name(name, info, meta) if doc and doc.symbol != name: raise QAPISemError( @@ -1889,13 +1893,13 @@ class QAPISchema(object): def _make_implicit_enum_type(self, name, info, ifcond, values): # See also QAPISchemaObjectTypeMember.describe() - name = name + 'Kind' # Use namespace reserved by add_name() + name = name + 'Kind' # reserved by check_defn_name_str() self._def_entity(QAPISchemaEnumType( name, info, None, ifcond, self._make_enum_members(values), None)) return name def _make_array_type(self, element_type, info): - name = element_type + 'List' # Use namespace reserved by add_name() + name = element_type + 'List' # reserved by check_defn_name_str() if not self.lookup_type(name): self._def_entity(QAPISchemaArrayType(name, info, element_type)) return name -- cgit v1.1 From 88112488cf228df8b7588c8aa38e16ecd0dff48e Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:26 +0200 Subject: qapi: Make check_type()'s array case a bit more obvious check_type() checks the array's contents, then peels off the array and falls through to the "not array" code without resetting allow_array and allow_dict to False. Works because the peeled value is a string, and allow_array and allow_dict aren't used then. Tidy up anyway: recurse instead, defaulting allow_array and allow_dict to False. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-14-armbru@redhat.com> --- scripts/qapi/common.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 111a4bb..870d8b0 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -795,7 +795,8 @@ def check_type(value, info, source, raise QAPISemError(info, "%s: array type must contain single type name" % source) - value = value[0] + check_type(value[0], info, source, allow_metas=allow_metas) + return # Check if type name for value is okay if isinstance(value, str): -- cgit v1.1 From e6f9678da5371642e237d6d93595dbc11bd17f85 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:27 +0200 Subject: qapi: Plumb info to the QAPISchemaMember Future commits will need info in the .check() methods of QAPISchemaMember and its descendants. Get it there. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-15-armbru@redhat.com> --- scripts/qapi/common.py | 76 ++++++++++++++++++++++++++++---------------------- 1 file changed, 42 insertions(+), 34 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 870d8b0..8894580 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -1574,9 +1574,10 @@ class QAPISchemaMember(object): """ Represents object members, enum members and features """ role = 'member' - def __init__(self, name, ifcond=None): + def __init__(self, name, info, ifcond=None): assert isinstance(name, str) self.name = name + self.info = info self.ifcond = ifcond or [] self.defined_in = None @@ -1633,8 +1634,8 @@ class QAPISchemaFeature(QAPISchemaMember): class QAPISchemaObjectTypeMember(QAPISchemaMember): - def __init__(self, name, typ, optional, ifcond=None): - QAPISchemaMember.__init__(self, name, ifcond) + def __init__(self, name, info, typ, optional, ifcond=None): + QAPISchemaMember.__init__(self, name, info, ifcond) assert isinstance(typ, str) assert isinstance(optional, bool) self._type_name = typ @@ -1648,7 +1649,7 @@ class QAPISchemaObjectTypeMember(QAPISchemaMember): class QAPISchemaObjectTypeVariants(object): - def __init__(self, tag_name, tag_member, variants): + def __init__(self, tag_name, info, tag_member, variants): # Flat unions pass tag_name but not tag_member. # Simple unions and alternates pass tag_member but not tag_name. # After check(), tag_member is always set, and tag_name remains @@ -1659,6 +1660,7 @@ class QAPISchemaObjectTypeVariants(object): for v in variants: assert isinstance(v, QAPISchemaObjectTypeVariant) self._tag_name = tag_name + self.info = info self.tag_member = tag_member self.variants = variants @@ -1678,8 +1680,8 @@ class QAPISchemaObjectTypeVariants(object): cases = set([v.name for v in self.variants]) for m in self.tag_member.type.members: if m.name not in cases: - v = QAPISchemaObjectTypeVariant(m.name, 'q_empty', - m.ifcond) + v = QAPISchemaObjectTypeVariant(m.name, self.info, + 'q_empty', m.ifcond) v.set_defined_in(self.tag_member.defined_in) self.variants.append(v) assert self.variants @@ -1703,8 +1705,9 @@ class QAPISchemaObjectTypeVariants(object): class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): role = 'branch' - def __init__(self, name, typ, ifcond=None): - QAPISchemaObjectTypeMember.__init__(self, name, typ, False, ifcond) + def __init__(self, name, info, typ, ifcond=None): + QAPISchemaObjectTypeMember.__init__(self, name, info, typ, + False, ifcond) class QAPISchemaAlternateType(QAPISchemaType): @@ -1880,23 +1883,26 @@ class QAPISchema(object): qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool'] - qtype_values = self._make_enum_members([{'name': n} for n in qtypes]) + qtype_values = self._make_enum_members( + [{'name': n} for n in qtypes], None) self._def_entity(QAPISchemaEnumType('QType', None, None, None, qtype_values, 'QTYPE')) - def _make_features(self, features): - return [QAPISchemaFeature(f['name'], f.get('if')) for f in features] + def _make_features(self, features, info): + return [QAPISchemaFeature(f['name'], info, f.get('if')) + for f in features] - def _make_enum_members(self, values): - return [QAPISchemaEnumMember(v['name'], v.get('if')) + def _make_enum_members(self, values, info): + return [QAPISchemaEnumMember(v['name'], info, v.get('if')) for v in values] def _make_implicit_enum_type(self, name, info, ifcond, values): # See also QAPISchemaObjectTypeMember.describe() name = name + 'Kind' # reserved by check_defn_name_str() self._def_entity(QAPISchemaEnumType( - name, info, None, ifcond, self._make_enum_members(values), None)) + name, info, None, ifcond, self._make_enum_members(values, info), + None)) return name def _make_array_type(self, element_type, info): @@ -1935,7 +1941,7 @@ class QAPISchema(object): ifcond = expr.get('if') self._def_entity(QAPISchemaEnumType( name, info, doc, ifcond, - self._make_enum_members(data), prefix)) + self._make_enum_members(data, info), prefix)) def _make_member(self, name, typ, ifcond, info): optional = False @@ -1945,7 +1951,7 @@ class QAPISchema(object): if isinstance(typ, list): assert len(typ) == 1 typ = self._make_array_type(typ[0], info) - return QAPISchemaObjectTypeMember(name, typ, optional, ifcond) + return QAPISchemaObjectTypeMember(name, info, typ, optional, ifcond) def _make_members(self, data, info): return [self._make_member(key, value['type'], value.get('if'), info) @@ -1957,13 +1963,14 @@ class QAPISchema(object): data = expr['data'] ifcond = expr.get('if') features = expr.get('features', []) - self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, base, - self._make_members(data, info), - None, - self._make_features(features))) + self._def_entity(QAPISchemaObjectType( + name, info, doc, ifcond, base, + self._make_members(data, info), + None, + self._make_features(features, info))) - def _make_variant(self, case, typ, ifcond): - return QAPISchemaObjectTypeVariant(case, typ, ifcond) + def _make_variant(self, case, typ, ifcond, info): + return QAPISchemaObjectTypeVariant(case, info, typ, ifcond) def _make_simple_variant(self, case, typ, ifcond, info): if isinstance(typ, list): @@ -1972,7 +1979,7 @@ class QAPISchema(object): typ = self._make_implicit_object_type( typ, info, None, self.lookup_type(typ), 'wrapper', [self._make_member('data', typ, None, info)]) - return QAPISchemaObjectTypeVariant(case, typ, ifcond) + return QAPISchemaObjectTypeVariant(case, info, typ, ifcond) def _def_union_type(self, expr, info, doc): name = expr['union'] @@ -1986,7 +1993,8 @@ class QAPISchema(object): name, info, doc, ifcond, 'base', self._make_members(base, info)) if tag_name: - variants = [self._make_variant(key, value['type'], value.get('if')) + variants = [self._make_variant(key, value['type'], + value.get('if'), info) for (key, value) in data.items()] members = [] else: @@ -1995,26 +2003,26 @@ class QAPISchema(object): for (key, value) in data.items()] enum = [{'name': v.name, 'if': v.ifcond} for v in variants] typ = self._make_implicit_enum_type(name, info, ifcond, enum) - tag_member = QAPISchemaObjectTypeMember('type', typ, False) + tag_member = QAPISchemaObjectTypeMember('type', info, typ, False) members = [tag_member] self._def_entity( QAPISchemaObjectType(name, info, doc, ifcond, base, members, - QAPISchemaObjectTypeVariants(tag_name, - tag_member, - variants), [])) + QAPISchemaObjectTypeVariants( + tag_name, info, tag_member, variants), + [])) def _def_alternate_type(self, expr, info, doc): name = expr['alternate'] data = expr['data'] ifcond = expr.get('if') - variants = [self._make_variant(key, value['type'], value.get('if')) + variants = [self._make_variant(key, value['type'], value.get('if'), + info) for (key, value) in data.items()] - tag_member = QAPISchemaObjectTypeMember('type', 'QType', False) + tag_member = QAPISchemaObjectTypeMember('type', info, 'QType', False) self._def_entity( QAPISchemaAlternateType(name, info, doc, ifcond, - QAPISchemaObjectTypeVariants(None, - tag_member, - variants))) + QAPISchemaObjectTypeVariants( + None, info, tag_member, variants))) def _def_command(self, expr, info, doc): name = expr['command'] @@ -2306,7 +2314,7 @@ const QEnumLookup %(c_name)s_lookup = { def gen_enum(name, members, prefix=None): # append automatically generated _MAX value - enum_members = members + [QAPISchemaEnumMember('_MAX')] + enum_members = members + [QAPISchemaEnumMember('_MAX', None)] ret = mcgen(''' -- cgit v1.1 From 77daece3d95dc7edaa5982fbbfd7afe3bc4121ac Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:28 +0200 Subject: qapi: Inline check_name() into check_union() check_name() consists of check_name_is_str() and check_name_str(). check_union() relies on the latter to catch optional discriminators. The next commit will replace that by a more straightforward check. Inlining check_name() into check_union() now should make that easier to review. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-16-armbru@redhat.com> --- scripts/qapi/common.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 8894580..9acff01 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -897,8 +897,10 @@ def check_union(expr, info): # The value of member 'discriminator' must name a non-optional # member of the base struct. - check_name(discriminator, info, - "discriminator of flat union '%s'" % name) + check_name_is_str(discriminator, info, + "discriminator of flat union '%s'" % name) + check_name_str(discriminator, info, + "discriminator of flat union '%s'" % name) discriminator_value = base_members.get(discriminator) if not discriminator_value: raise QAPISemError(info, -- cgit v1.1 From fa110c6a9e6c0ae0ce2d4bcf5771cdb8c3e53a7e Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:29 +0200 Subject: qapi: Move context-sensitive checking to the proper place When we introduced the QAPISchema intermediate representation (commit ac88219a6c7), we took a shortcut: we left check_exprs() & friends alone instead of moving semantic checks into the QAPISchemaFOO.check(). The .check() assert check_exprs() did its job. Time to finish the conversion job. Move exactly the context-sensitive checks to the .check(). They replace assertions there. Context-free checks stay put. Fixes the misleading optional tag error demonstrated by test flat-union-optional-discriminator. A few other error message improve. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-17-armbru@redhat.com> --- scripts/qapi/common.py | 424 ++++++++++++++++++++++--------------------------- 1 file changed, 193 insertions(+), 231 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 9acff01..f22e84c 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -21,25 +21,6 @@ import string import sys from collections import OrderedDict -builtin_types = { - 'null': 'QTYPE_QNULL', - 'str': 'QTYPE_QSTRING', - 'int': 'QTYPE_QNUM', - 'number': 'QTYPE_QNUM', - 'bool': 'QTYPE_QBOOL', - 'int8': 'QTYPE_QNUM', - 'int16': 'QTYPE_QNUM', - 'int32': 'QTYPE_QNUM', - 'int64': 'QTYPE_QNUM', - 'uint8': 'QTYPE_QNUM', - 'uint16': 'QTYPE_QNUM', - 'uint32': 'QTYPE_QNUM', - 'uint64': 'QTYPE_QNUM', - 'size': 'QTYPE_QNUM', - 'any': None, # any QType possible, actually - 'QType': 'QTYPE_QSTRING', -} - # Are documentation comments required? doc_required = False @@ -49,11 +30,6 @@ returns_whitelist = [] # Whitelist of entities allowed to violate case conventions name_case_whitelist = [] -enum_types = {} -struct_types = {} -union_types = {} -all_names = {} - # # Parsing the schema into expressions @@ -671,34 +647,9 @@ class QAPISchemaParser(object): # -# Semantic analysis of schema expressions -# TODO fold into QAPISchema -# TODO catching name collisions in generated code would be nice +# Check (context-free) schema expression structure # - -def find_base_members(base): - if isinstance(base, dict): - return base - base_struct_define = struct_types.get(base) - if not base_struct_define: - return None - return base_struct_define['data'] - - -# Return the qtype of an alternate branch, or None on error. -def find_alternate_member_qtype(qapi_type): - if qapi_type in builtin_types: - return builtin_types[qapi_type] - elif qapi_type in struct_types: - return 'QTYPE_QDICT' - elif qapi_type in enum_types: - return 'QTYPE_QSTRING' - elif qapi_type in union_types: - return 'QTYPE_QDICT' - return None - - # Names must be letters, numbers, -, and _. They must start with letter, # except for downstream extensions which must start with __RFQDN_. # Dots are only valid in the downstream extension prefix. @@ -748,16 +699,6 @@ def check_defn_name_str(name, info, meta): info, "%s '%s' should not end in '%s'" % (meta, name, name[-4:])) -def add_name(name, info, meta): - global all_names - # FIXME should reject names that differ only in '_' vs. '.' - # vs. '-', because they're liable to clash in generated C. - if name in all_names: - raise QAPISemError(info, "%s '%s' is already defined" - % (all_names[name], name)) - all_names[name] = meta - - def check_if(expr, info): def check_if_str(ifcond, info): @@ -781,13 +722,11 @@ def check_if(expr, info): def check_type(value, info, source, - allow_array=False, allow_dict=False, allow_metas=[]): - global all_names - + allow_array=False, allow_dict=False): if value is None: return - # Check if array type for value is okay + # Array type if isinstance(value, list): if not allow_array: raise QAPISemError(info, "%s cannot be an array" % source) @@ -795,19 +734,14 @@ def check_type(value, info, source, raise QAPISemError(info, "%s: array type must contain single type name" % source) - check_type(value[0], info, source, allow_metas=allow_metas) return - # Check if type name for value is okay + # Type name if isinstance(value, str): - if value not in all_names: - raise QAPISemError(info, "%s uses unknown type '%s'" - % (source, value)) - if not all_names[value] in allow_metas: - raise QAPISemError(info, "%s cannot use %s type '%s'" % - (source, all_names[value], value)) return + # Anonymous type + if not allow_dict: raise QAPISemError(info, "%s should be a type name" % source) @@ -829,43 +763,28 @@ def check_type(value, info, source, check_if(arg, info) normalize_if(arg) check_type(arg['type'], info, "member '%s' of %s" % (key, source), - allow_array=True, - allow_metas=['built-in', 'union', 'alternate', 'struct', - 'enum']) + allow_array=True) def check_command(expr, info): name = expr['command'] boxed = expr.get('boxed', False) - args_meta = ['struct'] - if boxed: - args_meta += ['union'] check_type(expr.get('data'), info, "'data' for command '%s'" % name, - allow_dict=not boxed, allow_metas=args_meta) - returns_meta = ['union', 'struct'] - if name in returns_whitelist: - returns_meta += ['built-in', 'alternate', 'enum'] + allow_dict=not boxed) check_type(expr.get('returns'), info, "'returns' for command '%s'" % name, - allow_array=True, allow_metas=returns_meta) + allow_array=True) def check_event(expr, info): name = expr['event'] boxed = expr.get('boxed', False) - meta = ['struct'] - if boxed: - meta += ['union'] check_type(expr.get('data'), info, "'data' for event '%s'" % name, - allow_dict=not boxed, allow_metas=meta) - - -def enum_get_names(expr): - return [e['name'] for e in expr['data']] + allow_dict=not boxed) def check_union(expr, info): @@ -874,55 +793,18 @@ def check_union(expr, info): discriminator = expr.get('discriminator') members = expr['data'] - # Two types of unions, determined by discriminator. - - # With no discriminator it is a simple union. - if discriminator is None: - enum_values = members.keys() - allow_metas = ['built-in', 'union', 'alternate', 'struct', 'enum'] + if discriminator is None: # simple union if base is not None: raise QAPISemError( info, "simple union '%s' must not have a base" % name) - - # Else, it's a flat union. - else: - # The object must have a string or dictionary 'base'. + else: # flat union check_type(base, info, "'base' for union '%s'" % name, - allow_dict=name, allow_metas=['struct']) + allow_dict=name) if not base: raise QAPISemError( info, "flat union '%s' must have a base" % name) - base_members = find_base_members(base) - assert base_members is not None - - # The value of member 'discriminator' must name a non-optional - # member of the base struct. check_name_is_str(discriminator, info, "discriminator of flat union '%s'" % name) - check_name_str(discriminator, info, - "discriminator of flat union '%s'" % name) - discriminator_value = base_members.get(discriminator) - if not discriminator_value: - raise QAPISemError(info, - "discriminator '%s' is not a member of 'base'" - % discriminator) - if discriminator_value.get('if'): - raise QAPISemError( - info, - "the discriminator '%s' for union %s must not be conditional" - % (discriminator, name)) - enum_define = enum_types.get(discriminator_value['type']) - # Do not allow string discriminator - if not enum_define: - raise QAPISemError( - info, - "discriminator '%s' must be of enumeration type" - % discriminator) - enum_values = enum_get_names(enum_define) - allow_metas = ['struct'] - - if (len(enum_values) == 0): - raise QAPISemError(info, "union '%s' has no branches" % name) for (key, value) in members.items(): check_name_str(key, info, "member of union '%s'" % name) @@ -931,25 +813,14 @@ def check_union(expr, info): ['type'], ['if']) check_if(value, info) normalize_if(value) - # Each value must name a known type check_type(value['type'], info, "member '%s' of union '%s'" % (key, name), - allow_array=not base, allow_metas=allow_metas) - - # If the discriminator names an enum type, then all members - # of 'data' must also be members of the enum type. - if discriminator is not None: - if key not in enum_values: - raise QAPISemError( - info, - "discriminator value '%s' is not found in enum '%s'" - % (key, enum_define['enum'])) + allow_array=not base) def check_alternate(expr, info): name = expr['alternate'] members = expr['data'] - types_seen = {} if len(members) == 0: raise QAPISemError(info, @@ -961,37 +832,8 @@ def check_alternate(expr, info): ['type'], ['if']) check_if(value, info) normalize_if(value) - typ = value['type'] - - # Ensure alternates have no type conflicts. - check_type(typ, info, "member '%s' of alternate '%s'" % (key, name), - allow_metas=['built-in', 'union', 'struct', 'enum']) - qtype = find_alternate_member_qtype(typ) - if not qtype: - raise QAPISemError( - info, - "alternate '%s' member '%s' cannot use type '%s'" - % (name, key, typ)) - conflicting = set([qtype]) - if qtype == 'QTYPE_QSTRING': - enum_expr = enum_types.get(typ) - if enum_expr: - for v in enum_get_names(enum_expr): - if v in ['on', 'off']: - conflicting.add('QTYPE_QBOOL') - if re.match(r'[-+0-9.]', v): # lazy, could be tightened - conflicting.add('QTYPE_QNUM') - else: - conflicting.add('QTYPE_QNUM') - conflicting.add('QTYPE_QBOOL') - for qt in conflicting: - if qt in types_seen: - raise QAPISemError( - info, - "alternate '%s' member '%s' can't be distinguished " - "from member '%s'" - % (name, key, types_seen[qt])) - types_seen[qt] = key + check_type(value['type'], info, + "member '%s' of alternate '%s'" % (key, name)) def check_enum(expr, info): @@ -1024,8 +866,7 @@ def check_struct(expr, info): check_type(members, info, "'data' for struct '%s'" % name, allow_dict=name) - check_type(expr.get('base'), info, "'base' for struct '%s'" % name, - allow_metas=['struct']) + check_type(expr.get('base'), info, "'base' for struct '%s'" % name) if features: if not isinstance(features, list): @@ -1111,13 +952,6 @@ def normalize_if(expr): def check_exprs(exprs): - global all_names - - # Populate name table with names of built-in types - for builtin in builtin_types.keys(): - all_names[builtin] = 'built-in' - - # Learn the types and check for valid expression keys for expr_elem in exprs: expr = expr_elem['expr'] info = expr_elem['info'] @@ -1134,14 +968,12 @@ def check_exprs(exprs): meta = 'enum' check_keys(expr, info, 'enum', ['data'], ['if', 'prefix']) normalize_enum(expr) - enum_types[expr[meta]] = expr elif 'union' in expr: meta = 'union' check_keys(expr, info, 'union', ['data'], ['base', 'discriminator', 'if']) normalize_members(expr.get('base')) normalize_members(expr['data']) - union_types[expr[meta]] = expr elif 'alternate' in expr: meta = 'alternate' check_keys(expr, info, 'alternate', ['data'], ['if']) @@ -1152,7 +984,6 @@ def check_exprs(exprs): ['base', 'if', 'features']) normalize_members(expr['data']) normalize_features(expr.get('features')) - struct_types[expr[meta]] = expr elif 'command' in expr: meta = 'command' check_keys(expr, info, 'command', [], @@ -1166,36 +997,29 @@ def check_exprs(exprs): else: raise QAPISemError(info, "expression is missing metatype") normalize_if(expr) + name = expr[meta] check_name_is_str(name, info, "'%s'" % meta) info.set_defn(meta, name) check_defn_name_str(name, info, meta) - add_name(name, info, meta) + if doc and doc.symbol != name: raise QAPISemError( info, "definition of '%s' follows documentation for '%s'" % (name, doc.symbol)) - # Validate that exprs make sense - for expr_elem in exprs: - expr = expr_elem['expr'] - info = expr_elem['info'] - doc = expr_elem.get('doc') - - if 'include' in expr: - continue - if 'enum' in expr: + if meta == 'enum': check_enum(expr, info) - elif 'union' in expr: + elif meta == 'union': check_union(expr, info) - elif 'alternate' in expr: + elif meta == 'alternate': check_alternate(expr, info) - elif 'struct' in expr: + elif meta == 'struct': check_struct(expr, info) - elif 'command' in expr: + elif meta == 'command': check_command(expr, info) - elif 'event' in expr: + elif meta == 'event': check_event(expr, info) else: assert False, 'unexpected meta type' @@ -1208,9 +1032,12 @@ def check_exprs(exprs): # # Schema compiler frontend +# TODO catching name collisions in generated code would be nice # class QAPISchemaEntity(object): + meta = None + def __init__(self, name, info, doc, ifcond=None): assert name is None or isinstance(name, str) self.name = name @@ -1251,6 +1078,10 @@ class QAPISchemaEntity(object): def visit(self, visitor): assert self._checked + def describe(self): + assert self.meta + return "%s '%s'" % (self.meta, self.name) + class QAPISchemaVisitor(object): def visit_begin(self, schema): @@ -1341,8 +1172,14 @@ class QAPISchemaType(QAPISchemaEntity): return None return self.name + def describe(self): + assert self.meta + return "%s type '%s'" % (self.meta, self.name) + class QAPISchemaBuiltinType(QAPISchemaType): + meta = 'built-in' + def __init__(self, name, json_type, c_type): QAPISchemaType.__init__(self, name, None, None) assert not c_type or isinstance(c_type, str) @@ -1374,6 +1211,8 @@ class QAPISchemaBuiltinType(QAPISchemaType): class QAPISchemaEnumType(QAPISchemaType): + meta = 'enum' + def __init__(self, name, info, doc, ifcond, members, prefix): QAPISchemaType.__init__(self, name, info, doc, ifcond) for m in members: @@ -1411,6 +1250,8 @@ class QAPISchemaEnumType(QAPISchemaType): class QAPISchemaArrayType(QAPISchemaType): + meta = 'array' + def __init__(self, name, info, element_type): QAPISchemaType.__init__(self, name, info, None, None) assert isinstance(element_type, str) @@ -1419,8 +1260,9 @@ class QAPISchemaArrayType(QAPISchemaType): def check(self, schema): QAPISchemaType.check(self, schema) - self.element_type = schema.lookup_type(self._element_type_name) - assert self.element_type + self.element_type = schema.resolve_type( + self._element_type_name, self.info, + self.info and self.info.defn_meta) assert not isinstance(self.element_type, QAPISchemaArrayType) @property @@ -1453,6 +1295,10 @@ class QAPISchemaArrayType(QAPISchemaType): visitor.visit_array_type(self.name, self.info, self.ifcond, self.element_type) + def describe(self): + assert self.meta + return "%s type ['%s']" % (self.meta, self._element_type_name) + class QAPISchemaObjectType(QAPISchemaType): def __init__(self, name, info, doc, ifcond, @@ -1461,6 +1307,7 @@ class QAPISchemaObjectType(QAPISchemaType): # flat union has base, variants, and no local_members # simple union has local_members, variants, and no base QAPISchemaType.__init__(self, name, info, doc, ifcond) + self.meta = 'union' if variants else 'struct' assert base is None or isinstance(base, str) for m in local_members: assert isinstance(m, QAPISchemaObjectTypeMember) @@ -1495,8 +1342,14 @@ class QAPISchemaObjectType(QAPISchemaType): seen = OrderedDict() if self._base_name: - self.base = schema.lookup_type(self._base_name) - assert isinstance(self.base, QAPISchemaObjectType) + self.base = schema.resolve_type(self._base_name, self.info, + "'base'") + if (not isinstance(self.base, QAPISchemaObjectType) + or self.base.variants): + raise QAPISemError( + self.info, + "'base' requires a struct type, %s isn't" + % self.base.describe()) self.base.check(schema) self.base.check_clash(self.info, seen) for m in self.local_members: @@ -1508,7 +1361,6 @@ class QAPISchemaObjectType(QAPISchemaType): if self.variants: self.variants.check(schema, seen) - assert self.variants.tag_member in members self.variants.check_clash(self.info, seen) # Features are in a name space separate from members @@ -1646,8 +1498,8 @@ class QAPISchemaObjectTypeMember(QAPISchemaMember): def check(self, schema): assert self.defined_in - self.type = schema.lookup_type(self._type_name) - assert self.type + self.type = schema.resolve_type(self._type_name, self.info, + self.describe) class QAPISchemaObjectTypeVariants(object): @@ -1671,12 +1523,40 @@ class QAPISchemaObjectTypeVariants(object): v.set_defined_in(name) def check(self, schema, seen): - if not self.tag_member: # flat union - self.tag_member = seen[c_name(self._tag_name)] - assert self._tag_name == self.tag_member.name - assert isinstance(self.tag_member.type, QAPISchemaEnumType) - assert not self.tag_member.optional - assert self.tag_member.ifcond == [] + if not self.tag_member: # flat union + self.tag_member = seen.get(c_name(self._tag_name)) + base = "'base'" + # Pointing to the base type when not implicit would be + # nice, but we don't know it here + if not self.tag_member or self._tag_name != self.tag_member.name: + raise QAPISemError( + self.info, + "discriminator '%s' is not a member of %s" + % (self._tag_name, base)) + # Here we do: + base_type = schema.lookup_type(self.tag_member.defined_in) + assert base_type + if not base_type.is_implicit(): + base = "base type '%s'" % self.tag_member.defined_in + if not isinstance(self.tag_member.type, QAPISchemaEnumType): + raise QAPISemError( + self.info, + "discriminator member '%s' of %s must be of enum type" + % (self._tag_name, base)) + if self.tag_member.optional: + raise QAPISemError( + self.info, + "discriminator member '%s' of %s must not be optional" + % (self._tag_name, base)) + if self.tag_member.ifcond: + raise QAPISemError( + self.info, + "discriminator member '%s' of %s must not be conditional" + % (self._tag_name, base)) + else: # simple union + assert isinstance(self.tag_member.type, QAPISchemaEnumType) + assert not self.tag_member.optional + assert self.tag_member.ifcond == [] if self._tag_name: # flat union # branches that are not explicitly covered get an empty type cases = set([v.name for v in self.variants]) @@ -1686,15 +1566,24 @@ class QAPISchemaObjectTypeVariants(object): 'q_empty', m.ifcond) v.set_defined_in(self.tag_member.defined_in) self.variants.append(v) - assert self.variants + if not self.variants: + raise QAPISemError(self.info, "union has no branches") for v in self.variants: v.check(schema) # Union names must match enum values; alternate names are # checked separately. Use 'seen' to tell the two apart. if seen: - assert v.name in self.tag_member.type.member_names() - assert (isinstance(v.type, QAPISchemaObjectType) - and not v.type.variants) + if v.name not in self.tag_member.type.member_names(): + raise QAPISemError( + self.info, + "branch '%s' is not a value of %s" + % (v.name, self.tag_member.type.describe())) + if (not isinstance(v.type, QAPISchemaObjectType) + or v.type.variants): + raise QAPISemError( + self.info, + "%s cannot use %s" + % (v.describe(self.info), v.type.describe())) v.type.check(schema) def check_clash(self, info, seen): @@ -1713,6 +1602,8 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): class QAPISchemaAlternateType(QAPISchemaType): + meta = 'alternate' + def __init__(self, name, info, doc, ifcond, variants): QAPISchemaType.__init__(self, name, info, doc, ifcond) assert isinstance(variants, QAPISchemaObjectTypeVariants) @@ -1730,9 +1621,34 @@ class QAPISchemaAlternateType(QAPISchemaType): # Alternate branch names have no relation to the tag enum values; # so we have to check for potential name collisions ourselves. seen = {} + types_seen = {} for v in self.variants.variants: v.check_clash(self.info, seen) - # TODO check conflicting qtypes + qtype = v.type.alternate_qtype() + if not qtype: + raise QAPISemError( + self.info, + "%s cannot use %s" + % (v.describe(self.info), v.type.describe())) + conflicting = set([qtype]) + if qtype == 'QTYPE_QSTRING': + if isinstance(v.type, QAPISchemaEnumType): + for m in v.type.members: + if m.name in ['on', 'off']: + conflicting.add('QTYPE_QBOOL') + if re.match(r'[-+0-9.]', m.name): + # lazy, could be tightened + conflicting.add('QTYPE_QNUM') + else: + conflicting.add('QTYPE_QNUM') + conflicting.add('QTYPE_QBOOL') + for qt in conflicting: + if qt in types_seen: + raise QAPISemError( + self.info, + "%s can't be distinguished from '%s'" + % (v.describe(self.info), types_seen[qt])) + types_seen[qt] = v.name if self.doc: self.doc.connect_member(v) if self.doc: @@ -1751,6 +1667,8 @@ class QAPISchemaAlternateType(QAPISchemaType): class QAPISchemaCommand(QAPISchemaEntity): + meta = 'command' + def __init__(self, name, info, doc, ifcond, arg_type, ret_type, gen, success_response, boxed, allow_oob, allow_preconfig): QAPISchemaEntity.__init__(self, name, info, doc, ifcond) @@ -1769,14 +1687,32 @@ class QAPISchemaCommand(QAPISchemaEntity): def check(self, schema): QAPISchemaEntity.check(self, schema) if self._arg_type_name: - self.arg_type = schema.lookup_type(self._arg_type_name) - assert isinstance(self.arg_type, QAPISchemaObjectType) - assert not self.arg_type.variants or self.boxed + self.arg_type = schema.resolve_type( + self._arg_type_name, self.info, "command's 'data'") + if not isinstance(self.arg_type, QAPISchemaObjectType): + raise QAPISemError( + self.info, + "command's 'data' cannot take %s" + % self.arg_type.describe()) + if self.arg_type.variants and not self.boxed: + raise QAPISemError( + self.info, + "command's 'data' can take %s only with 'boxed': true" + % self.arg_type.describe()) elif self.boxed: raise QAPISemError(self.info, "use of 'boxed' requires 'data'") if self._ret_type_name: - self.ret_type = schema.lookup_type(self._ret_type_name) - assert isinstance(self.ret_type, QAPISchemaType) + self.ret_type = schema.resolve_type( + self._ret_type_name, self.info, "command's 'returns'") + if self.name not in returns_whitelist: + if not (isinstance(self.ret_type, QAPISchemaObjectType) + or (isinstance(self.ret_type, QAPISchemaArrayType) + and isinstance(self.ret_type.element_type, + QAPISchemaObjectType))): + raise QAPISemError( + self.info, + "command's 'returns' cannot take %s" + % self.ret_type.describe()) def visit(self, visitor): QAPISchemaEntity.visit(self, visitor) @@ -1788,6 +1724,8 @@ class QAPISchemaCommand(QAPISchemaEntity): class QAPISchemaEvent(QAPISchemaEntity): + meta = 'event' + def __init__(self, name, info, doc, ifcond, arg_type, boxed): QAPISchemaEntity.__init__(self, name, info, doc, ifcond) assert not arg_type or isinstance(arg_type, str) @@ -1798,9 +1736,18 @@ class QAPISchemaEvent(QAPISchemaEntity): def check(self, schema): QAPISchemaEntity.check(self, schema) if self._arg_type_name: - self.arg_type = schema.lookup_type(self._arg_type_name) - assert isinstance(self.arg_type, QAPISchemaObjectType) - assert not self.arg_type.variants or self.boxed + self.arg_type = schema.resolve_type( + self._arg_type_name, self.info, "event's 'data'") + if not isinstance(self.arg_type, QAPISchemaObjectType): + raise QAPISemError( + self.info, + "event's 'data' cannot take %s" + % self.arg_type.describe()) + if self.arg_type.variants and not self.boxed: + raise QAPISemError( + self.info, + "event's 'data' can take %s only with 'boxed': true" + % self.arg_type.describe()) elif self.boxed: raise QAPISemError(self.info, "use of 'boxed' requires 'data'") @@ -1831,10 +1778,16 @@ class QAPISchema(object): def _def_entity(self, ent): # Only the predefined types are allowed to not have info assert ent.info or self._predefining - assert ent.name is None or ent.name not in self._entity_dict self._entity_list.append(ent) - if ent.name is not None: - self._entity_dict[ent.name] = ent + if ent.name is None: + return + # TODO reject names that differ only in '_' vs. '.' vs. '-', + # because they're liable to clash in generated C. + other_ent = self._entity_dict.get(ent.name) + if other_ent: + raise QAPISemError( + ent.info, "%s is already defined" % other_ent.describe()) + self._entity_dict[ent.name] = ent def lookup_entity(self, name, typ=None): ent = self._entity_dict.get(name) @@ -1845,6 +1798,15 @@ class QAPISchema(object): def lookup_type(self, name): return self.lookup_entity(name, QAPISchemaType) + def resolve_type(self, name, info, what): + typ = self.lookup_type(name) + if not typ: + if callable(what): + what = what(info) + raise QAPISemError( + info, "%s uses unknown type '%s'" % (what, name)) + return typ + def _def_include(self, expr, info, doc): include = expr['include'] assert doc is None @@ -1930,7 +1892,7 @@ class QAPISchema(object): # But it's not tight: the disjunction need not imply it. We # may end up compiling useless wrapper types. # TODO kill simple unions or implement the disjunction - assert ifcond == typ._ifcond # pylint: disable=protected-access + assert (ifcond or []) == typ._ifcond # pylint: disable=protected-access else: self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, None, members, None, [])) -- cgit v1.1 From 4ebda5abdb9704a3bde299209f8fcaf034079095 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:30 +0200 Subject: qapi: Move context-free checking to the proper place QAPISchemaCommand.check() and QAPISchemaEvent().check() check 'data' is present when 'boxed': true. That's context-free. Move to check_command() and check_event(). Tweak the error message while there. check_exprs() & friends now check exactly what qapi-code-gen.txt calls the second layer of syntax. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-18-armbru@redhat.com> --- scripts/qapi/common.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index f22e84c..2e1d815 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -768,10 +768,12 @@ def check_type(value, info, source, def check_command(expr, info): name = expr['command'] + args = expr.get('data') boxed = expr.get('boxed', False) - check_type(expr.get('data'), info, - "'data' for command '%s'" % name, + if boxed and args is None: + raise QAPISemError(info, "'boxed': true requires 'data'") + check_type(args, info, "'data' for command '%s'" % name, allow_dict=not boxed) check_type(expr.get('returns'), info, "'returns' for command '%s'" % name, @@ -780,10 +782,12 @@ def check_command(expr, info): def check_event(expr, info): name = expr['event'] + args = expr.get('data') boxed = expr.get('boxed', False) - check_type(expr.get('data'), info, - "'data' for event '%s'" % name, + if boxed and args is None: + raise QAPISemError(info, "'boxed': true requires 'data'") + check_type(args, info, "'data' for event '%s'" % name, allow_dict=not boxed) @@ -1699,8 +1703,6 @@ class QAPISchemaCommand(QAPISchemaEntity): self.info, "command's 'data' can take %s only with 'boxed': true" % self.arg_type.describe()) - elif self.boxed: - raise QAPISemError(self.info, "use of 'boxed' requires 'data'") if self._ret_type_name: self.ret_type = schema.resolve_type( self._ret_type_name, self.info, "command's 'returns'") @@ -1748,8 +1750,6 @@ class QAPISchemaEvent(QAPISchemaEntity): self.info, "event's 'data' can take %s only with 'boxed': true" % self.arg_type.describe()) - elif self.boxed: - raise QAPISemError(self.info, "use of 'boxed' requires 'data'") def visit(self, visitor): QAPISchemaEntity.visit(self, visitor) -- cgit v1.1 From 576f0b8a53ab12d6ca46ec8f89ec0901d80d0ad8 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:31 +0200 Subject: qapi: Improve reporting of invalid 'if' errors Move check_if() from check_keys() to check_exprs() and call it later, so its error messages gain an "in definition" line. Checking values in a function named check_keys() is unclean anyway. The original sin was commit 0545f6b887 "qapi: Better error messages for bad expressions", which checks the value of key 'name'. More sinning in commit 2cbf09925a "qapi: More rigorous checking for type safety bypass", commit c818408e44 "qapi: Implement boxed types for commands/events", and commit 967c885108 "qapi: add 'if' to top-level expressions". This commit does penance for the latter. The next commits will do penance for the others. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-19-armbru@redhat.com> --- scripts/qapi/common.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 2e1d815..8f96974 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -925,8 +925,6 @@ def check_keys(expr, info, meta, required, optional=[]): raise QAPISemError(info, "'%s' of %s '%s' should only use true value" % (key, meta, name)) - if key == 'if': - check_if(expr, info) def normalize_enum(expr): @@ -1028,6 +1026,8 @@ def check_exprs(exprs): else: assert False, 'unexpected meta type' + check_if(expr, info) + if doc: doc.check_expr(expr) -- cgit v1.1 From a6735a574382b214d7f1ee7b315cc81421aaa77e Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:32 +0200 Subject: qapi: Improve reporting of invalid flags Split check_flags() off check_keys() and have check_exprs() call it later, so its error messages gain an "in definition" line. Tweak the error messages. Checking values in a function named check_keys() is unclean anyway. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-20-armbru@redhat.com> --- scripts/qapi/common.py | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 8f96974..4f67b73 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -915,16 +915,17 @@ def check_keys(expr, info, meta, required, optional=[]): required = required + [meta] source = "%s '%s'" % (meta, name) check_known_keys(expr, info, source, required, optional) - for (key, value) in expr.items(): - if key in ['gen', 'success-response'] and value is not False: - raise QAPISemError(info, - "'%s' of %s '%s' should only use false value" - % (key, meta, name)) - if (key in ['boxed', 'allow-oob', 'allow-preconfig'] - and value is not True): - raise QAPISemError(info, - "'%s' of %s '%s' should only use true value" - % (key, meta, name)) + + +def check_flags(expr, info): + for key in ['gen', 'success-response']: + if key in expr and expr[key] is not False: + raise QAPISemError( + info, "flag '%s' may only use false value" % key) + for key in ['boxed', 'allow-oob', 'allow-preconfig']: + if key in expr and expr[key] is not True: + raise QAPISemError( + info, "flag '%s' may only use true value" % key) def normalize_enum(expr): @@ -1027,6 +1028,7 @@ def check_exprs(exprs): assert False, 'unexpected meta type' check_if(expr, info) + check_flags(expr, info) if doc: doc.check_expr(expr) -- cgit v1.1 From 3f58cc29a8d2f01dc498ff4624a25e72448059a1 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:33 +0200 Subject: qapi: Improve reporting of missing / unknown definition keys Have check_exprs() call check_keys() later, so its error messages gain an "in definition" line. Both check_keys() and check_name_is_str() check the definition's name is a string. Since check_keys() now runs after check_name_is_str() rather than before, its check is dead. Bury it. Checking values in check_keys() is unclean anyway. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-21-armbru@redhat.com> --- scripts/qapi/common.py | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 4f67b73..42b9c2e 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -910,8 +910,6 @@ def check_known_keys(value, info, source, required, optional): def check_keys(expr, info, meta, required, optional=[]): name = expr[meta] - if not isinstance(name, str): - raise QAPISemError(info, "'%s' key must have a string value" % meta) required = required + [meta] source = "%s '%s'" % (meta, name) check_known_keys(expr, info, source, required, optional) @@ -969,37 +967,18 @@ def check_exprs(exprs): if 'enum' in expr: meta = 'enum' - check_keys(expr, info, 'enum', ['data'], ['if', 'prefix']) - normalize_enum(expr) elif 'union' in expr: meta = 'union' - check_keys(expr, info, 'union', ['data'], - ['base', 'discriminator', 'if']) - normalize_members(expr.get('base')) - normalize_members(expr['data']) elif 'alternate' in expr: meta = 'alternate' - check_keys(expr, info, 'alternate', ['data'], ['if']) - normalize_members(expr['data']) elif 'struct' in expr: meta = 'struct' - check_keys(expr, info, 'struct', ['data'], - ['base', 'if', 'features']) - normalize_members(expr['data']) - normalize_features(expr.get('features')) elif 'command' in expr: meta = 'command' - check_keys(expr, info, 'command', [], - ['data', 'returns', 'gen', 'success-response', - 'boxed', 'allow-oob', 'allow-preconfig', 'if']) - normalize_members(expr.get('data')) elif 'event' in expr: meta = 'event' - check_keys(expr, info, 'event', [], ['data', 'boxed', 'if']) - normalize_members(expr.get('data')) else: raise QAPISemError(info, "expression is missing metatype") - normalize_if(expr) name = expr[meta] check_name_is_str(name, info, "'%s'" % meta) @@ -1013,20 +992,39 @@ def check_exprs(exprs): % (name, doc.symbol)) if meta == 'enum': + check_keys(expr, info, 'enum', ['data'], ['if', 'prefix']) + normalize_enum(expr) check_enum(expr, info) elif meta == 'union': + check_keys(expr, info, 'union', ['data'], + ['base', 'discriminator', 'if']) + normalize_members(expr.get('base')) + normalize_members(expr['data']) check_union(expr, info) elif meta == 'alternate': + check_keys(expr, info, 'alternate', ['data'], ['if']) + normalize_members(expr['data']) check_alternate(expr, info) elif meta == 'struct': + check_keys(expr, info, 'struct', ['data'], + ['base', 'if', 'features']) + normalize_members(expr['data']) + normalize_features(expr.get('features')) check_struct(expr, info) elif meta == 'command': + check_keys(expr, info, 'command', [], + ['data', 'returns', 'gen', 'success-response', + 'boxed', 'allow-oob', 'allow-preconfig', 'if']) + normalize_members(expr.get('data')) check_command(expr, info) elif meta == 'event': + check_keys(expr, info, 'event', [], ['data', 'boxed', 'if']) + normalize_members(expr.get('data')) check_event(expr, info) else: assert False, 'unexpected meta type' + normalize_if(expr) check_if(expr, info) check_flags(expr, info) -- cgit v1.1 From eeb57c85da04b0c120562b25e3eac5cb16d8bf15 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:34 +0200 Subject: qapi: Avoid redundant definition references in error messages Many error messages refer to the offending definition even though they're preceded by an "in definition" line. Rephrase them. Signed-off-by: Markus Armbruster Message-Id: <20190927134639.4284-22-armbru@redhat.com> Reviewed-by: Eric Blake --- scripts/qapi/common.py | 129 +++++++++++++++++++------------------------------ 1 file changed, 49 insertions(+), 80 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 42b9c2e..81c217c 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -657,13 +657,6 @@ valid_name = re.compile(r'^(__[a-zA-Z0-9.-]+_)?' '[a-zA-Z][a-zA-Z0-9_-]*$') -def check_name(name, info, source, - allow_optional=False, enum_member=False, permit_upper=False): - check_name_is_str(name, info, source) - check_name_str(name, info, source, - allow_optional, enum_member, permit_upper) - - def check_name_is_str(name, info, source): if not isinstance(name, str): raise QAPISemError(info, "%s requires a string name" % source) @@ -685,10 +678,10 @@ def check_name_str(name, info, source, # and 'q_obj_*' implicit type names. if not valid_name.match(membername) or \ c_name(membername, False).startswith('q_'): - raise QAPISemError(info, "%s uses invalid name '%s'" % (source, name)) + raise QAPISemError(info, "%s has an invalid name" % source) if not permit_upper and name.lower() != name: raise QAPISemError( - info, "%s uses uppercase in name '%s'" % (source, name)) + info, "%s uses uppercase in name" % source) assert not membername.startswith('*') @@ -696,7 +689,7 @@ def check_defn_name_str(name, info, meta): check_name_str(name, info, meta, permit_upper=True) if name.endswith('Kind') or name.endswith('List'): raise QAPISemError( - info, "%s '%s' should not end in '%s'" % (meta, name, name[-4:])) + info, "%s name should not end in '%s'" % (meta, name[-4:])) def check_if(expr, info): @@ -753,42 +746,35 @@ def check_type(value, info, source, # value is a dictionary, check that each member is okay for (key, arg) in value.items(): - check_name_str(key, info, "member of %s" % source, + key_source = "%s member '%s'" % (source, key) + check_name_str(key, info, key_source, allow_optional=True, permit_upper=permit_upper) if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'): - raise QAPISemError( - info, "member of %s uses reserved name '%s'" % (source, key)) - check_known_keys(arg, info, "member '%s' of %s" % (key, source), - ['type'], ['if']) + raise QAPISemError(info, "%s uses reserved name" % key_source) + check_known_keys(arg, info, key_source, ['type'], ['if']) check_if(arg, info) normalize_if(arg) - check_type(arg['type'], info, "member '%s' of %s" % (key, source), - allow_array=True) + check_type(arg['type'], info, key_source, allow_array=True) def check_command(expr, info): - name = expr['command'] args = expr.get('data') + rets = expr.get('returns') boxed = expr.get('boxed', False) if boxed and args is None: raise QAPISemError(info, "'boxed': true requires 'data'") - check_type(args, info, "'data' for command '%s'" % name, - allow_dict=not boxed) - check_type(expr.get('returns'), info, - "'returns' for command '%s'" % name, - allow_array=True) + check_type(args, info, "'data'", allow_dict=not boxed) + check_type(rets, info, "'returns'", allow_array=True) def check_event(expr, info): - name = expr['event'] args = expr.get('data') boxed = expr.get('boxed', False) if boxed and args is None: raise QAPISemError(info, "'boxed': true requires 'data'") - check_type(args, info, "'data' for event '%s'" % name, - allow_dict=not boxed) + check_type(args, info, "'data'", allow_dict=not boxed) def check_union(expr, info): @@ -799,45 +785,34 @@ def check_union(expr, info): if discriminator is None: # simple union if base is not None: - raise QAPISemError( - info, "simple union '%s' must not have a base" % name) + raise QAPISemError(info, "'base' requires 'discriminator'") else: # flat union - check_type(base, info, "'base' for union '%s'" % name, - allow_dict=name) + check_type(base, info, "'base'", allow_dict=name) if not base: - raise QAPISemError( - info, "flat union '%s' must have a base" % name) - check_name_is_str(discriminator, info, - "discriminator of flat union '%s'" % name) + raise QAPISemError(info, "'discriminator' requires 'base'") + check_name_is_str(discriminator, info, "'discriminator'") for (key, value) in members.items(): - check_name_str(key, info, "member of union '%s'" % name) - check_known_keys(value, info, - "member '%s' of union '%s'" % (key, name), - ['type'], ['if']) + source = "'data' member '%s'" % key + check_name_str(key, info, source) + check_known_keys(value, info, source, ['type'], ['if']) check_if(value, info) normalize_if(value) - check_type(value['type'], info, - "member '%s' of union '%s'" % (key, name), - allow_array=not base) + check_type(value['type'], info, source, allow_array=not base) def check_alternate(expr, info): - name = expr['alternate'] members = expr['data'] if len(members) == 0: - raise QAPISemError(info, - "alternate '%s' cannot have empty 'data'" % name) + raise QAPISemError(info, "'data' must not be empty") for (key, value) in members.items(): - check_name_str(key, info, "member of alternate '%s'" % name) - check_known_keys(value, info, - "member '%s' of alternate '%s'" % (key, name), - ['type'], ['if']) + source = "'data' member '%s'" % key + check_name_str(key, info, source) + check_known_keys(value, info, source, ['type'], ['if']) check_if(value, info) normalize_if(value) - check_type(value['type'], info, - "member '%s' of alternate '%s'" % (key, name)) + check_type(value['type'], info, source) def check_enum(expr, info): @@ -846,21 +821,21 @@ def check_enum(expr, info): prefix = expr.get('prefix') if not isinstance(members, list): - raise QAPISemError(info, - "enum '%s' requires an array for 'data'" % name) + raise QAPISemError(info, "'data' must be an array") if prefix is not None and not isinstance(prefix, str): - raise QAPISemError(info, - "enum '%s' requires a string for 'prefix'" % name) + raise QAPISemError(info, "'prefix' must be a string") permit_upper = name in name_case_whitelist for member in members: - check_known_keys(member, info, "member of enum '%s'" % name, - ['name'], ['if']) + source = "'data' member" + check_known_keys(member, info, source, ['name'], ['if']) + check_name_is_str(member['name'], info, source) + source = "%s '%s'" % (source, member['name']) + check_name_str(member['name'], info, source, + enum_member=True, permit_upper=permit_upper) check_if(member, info) normalize_if(member) - check_name(member['name'], info, "member of enum '%s'" % name, - enum_member=True, permit_upper=permit_upper) def check_struct(expr, info): @@ -868,22 +843,21 @@ def check_struct(expr, info): members = expr['data'] features = expr.get('features') - check_type(members, info, "'data' for struct '%s'" % name, - allow_dict=name) - check_type(expr.get('base'), info, "'base' for struct '%s'" % name) + check_type(members, info, "'data'", allow_dict=name) + check_type(expr.get('base'), info, "'base'") if features: if not isinstance(features, list): - raise QAPISemError( - info, "struct '%s' requires an array for 'features'" % name) + raise QAPISemError(info, "'features' must be an array") for f in features: + source = "'features' member" assert isinstance(f, dict) - check_known_keys(f, info, "feature of struct %s" % name, - ['name'], ['if']) - + check_known_keys(f, info, source, ['name'], ['if']) + check_name_is_str(f['name'], info, source) + source = "%s '%s'" % (source, f['name']) + check_name_str(f['name'], info, source) check_if(f, info) normalize_if(f) - check_name(f['name'], info, "feature of struct %s" % name) def check_known_keys(value, info, source, required, optional): @@ -895,24 +869,21 @@ def check_known_keys(value, info, source, required, optional): if missing: raise QAPISemError( info, - "key%s %s %s missing from %s" - % ('s' if len(missing) > 1 else '', pprint(missing), - 'are' if len(missing) > 1 else 'is', source)) + "%s misses key%s %s" + % (source, 's' if len(missing) > 1 else '', + pprint(missing))) allowed = set(required + optional) unknown = set(value) - allowed if unknown: raise QAPISemError( info, - "unknown key%s %s in %s\nValid keys are %s." - % ('s' if len(unknown) > 1 else '', pprint(unknown), - source, pprint(allowed))) + "%s has unknown key%s %s\nValid keys are %s." + % (source, 's' if len(unknown) > 1 else '', + pprint(unknown), pprint(allowed))) def check_keys(expr, info, meta, required, optional=[]): - name = expr[meta] - required = required + [meta] - source = "%s '%s'" % (meta, name) - check_known_keys(expr, info, source, required, optional) + check_known_keys(expr, info, meta, required + [meta], optional) def check_flags(expr, info): @@ -987,9 +958,7 @@ def check_exprs(exprs): if doc and doc.symbol != name: raise QAPISemError( - info, - "definition of '%s' follows documentation for '%s'" - % (name, doc.symbol)) + info, "documentation comment is for '%s'" % doc.symbol) if meta == 'enum': check_keys(expr, info, 'enum', ['data'], ['if', 'prefix']) -- cgit v1.1 From fab12376d0234ce46517f88b9ecc9a6080522118 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:35 +0200 Subject: qapi: Improve reporting of invalid 'if' further check_if()'s errors don't point to the offending part of the expression. For instance: tests/qapi-schema/alternate-branch-if-invalid.json:2: 'if' condition ' ' makes no sense Other check_FOO() do, with the help of a @source argument. Make check_if() do that, too. The example above improves to: tests/qapi-schema/alternate-branch-if-invalid.json:2: 'if' condition ' ' of 'data' member 'branch' makes no sense Signed-off-by: Markus Armbruster Message-Id: <20190927134639.4284-23-armbru@redhat.com> Reviewed-by: Eric Blake --- scripts/qapi/common.py | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 81c217c..4bc8c80 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -692,22 +692,27 @@ def check_defn_name_str(name, info, meta): info, "%s name should not end in '%s'" % (meta, name[-4:])) -def check_if(expr, info): +def check_if(expr, info, source): def check_if_str(ifcond, info): if not isinstance(ifcond, str): raise QAPISemError( - info, "'if' condition must be a string or a list of strings") + info, + "'if' condition of %s must be a string or a list of strings" + % source) if ifcond.strip() == '': - raise QAPISemError(info, "'if' condition '%s' makes no sense" - % ifcond) + raise QAPISemError( + info, + "'if' condition '%s' of %s makes no sense" + % (ifcond, source)) ifcond = expr.get('if') if ifcond is None: return if isinstance(ifcond, list): if ifcond == []: - raise QAPISemError(info, "'if' condition [] is useless") + raise QAPISemError( + info, "'if' condition [] of %s is useless" % source) for elt in ifcond: check_if_str(elt, info) else: @@ -752,7 +757,7 @@ def check_type(value, info, source, if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'): raise QAPISemError(info, "%s uses reserved name" % key_source) check_known_keys(arg, info, key_source, ['type'], ['if']) - check_if(arg, info) + check_if(arg, info, key_source) normalize_if(arg) check_type(arg['type'], info, key_source, allow_array=True) @@ -796,7 +801,7 @@ def check_union(expr, info): source = "'data' member '%s'" % key check_name_str(key, info, source) check_known_keys(value, info, source, ['type'], ['if']) - check_if(value, info) + check_if(value, info, source) normalize_if(value) check_type(value['type'], info, source, allow_array=not base) @@ -810,7 +815,7 @@ def check_alternate(expr, info): source = "'data' member '%s'" % key check_name_str(key, info, source) check_known_keys(value, info, source, ['type'], ['if']) - check_if(value, info) + check_if(value, info, source) normalize_if(value) check_type(value['type'], info, source) @@ -834,7 +839,7 @@ def check_enum(expr, info): source = "%s '%s'" % (source, member['name']) check_name_str(member['name'], info, source, enum_member=True, permit_upper=permit_upper) - check_if(member, info) + check_if(member, info, source) normalize_if(member) @@ -856,7 +861,7 @@ def check_struct(expr, info): check_name_is_str(f['name'], info, source) source = "%s '%s'" % (source, f['name']) check_name_str(f['name'], info, source) - check_if(f, info) + check_if(f, info, source) normalize_if(f) @@ -994,7 +999,7 @@ def check_exprs(exprs): assert False, 'unexpected meta type' normalize_if(expr) - check_if(expr, info) + check_if(expr, info, meta) check_flags(expr, info) if doc: -- cgit v1.1 From 13b3997f148bc35d2050bc2fe4bb405791ece94f Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:36 +0200 Subject: qapi: Eliminate check_keys(), rename check_known_keys() check_keys() has become a trivial wrapper for check_known_keys(). Eliminate it. This makes its name available. Rename check_known_keys(). Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-24-armbru@redhat.com> --- scripts/qapi/common.py | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index 4bc8c80..fa354b3 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -756,7 +756,7 @@ def check_type(value, info, source, allow_optional=True, permit_upper=permit_upper) if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'): raise QAPISemError(info, "%s uses reserved name" % key_source) - check_known_keys(arg, info, key_source, ['type'], ['if']) + check_keys(arg, info, key_source, ['type'], ['if']) check_if(arg, info, key_source) normalize_if(arg) check_type(arg['type'], info, key_source, allow_array=True) @@ -800,7 +800,7 @@ def check_union(expr, info): for (key, value) in members.items(): source = "'data' member '%s'" % key check_name_str(key, info, source) - check_known_keys(value, info, source, ['type'], ['if']) + check_keys(value, info, source, ['type'], ['if']) check_if(value, info, source) normalize_if(value) check_type(value['type'], info, source, allow_array=not base) @@ -814,7 +814,7 @@ def check_alternate(expr, info): for (key, value) in members.items(): source = "'data' member '%s'" % key check_name_str(key, info, source) - check_known_keys(value, info, source, ['type'], ['if']) + check_keys(value, info, source, ['type'], ['if']) check_if(value, info, source) normalize_if(value) check_type(value['type'], info, source) @@ -834,7 +834,7 @@ def check_enum(expr, info): for member in members: source = "'data' member" - check_known_keys(member, info, source, ['name'], ['if']) + check_keys(member, info, source, ['name'], ['if']) check_name_is_str(member['name'], info, source) source = "%s '%s'" % (source, member['name']) check_name_str(member['name'], info, source, @@ -857,7 +857,7 @@ def check_struct(expr, info): for f in features: source = "'features' member" assert isinstance(f, dict) - check_known_keys(f, info, source, ['name'], ['if']) + check_keys(f, info, source, ['name'], ['if']) check_name_is_str(f['name'], info, source) source = "%s '%s'" % (source, f['name']) check_name_str(f['name'], info, source) @@ -865,7 +865,7 @@ def check_struct(expr, info): normalize_if(f) -def check_known_keys(value, info, source, required, optional): +def check_keys(value, info, source, required, optional): def pprint(elems): return ', '.join("'" + e + "'" for e in sorted(elems)) @@ -887,10 +887,6 @@ def check_known_keys(value, info, source, required, optional): pprint(unknown), pprint(allowed))) -def check_keys(expr, info, meta, required, optional=[]): - check_known_keys(expr, info, meta, required + [meta], optional) - - def check_flags(expr, info): for key in ['gen', 'success-response']: if key in expr and expr[key] is not False: @@ -966,33 +962,39 @@ def check_exprs(exprs): info, "documentation comment is for '%s'" % doc.symbol) if meta == 'enum': - check_keys(expr, info, 'enum', ['data'], ['if', 'prefix']) + check_keys(expr, info, meta, + ['enum', 'data'], ['if', 'prefix']) normalize_enum(expr) check_enum(expr, info) elif meta == 'union': - check_keys(expr, info, 'union', ['data'], + check_keys(expr, info, meta, + ['union', 'data'], ['base', 'discriminator', 'if']) normalize_members(expr.get('base')) normalize_members(expr['data']) check_union(expr, info) elif meta == 'alternate': - check_keys(expr, info, 'alternate', ['data'], ['if']) + check_keys(expr, info, meta, + ['alternate', 'data'], ['if']) normalize_members(expr['data']) check_alternate(expr, info) elif meta == 'struct': - check_keys(expr, info, 'struct', ['data'], - ['base', 'if', 'features']) + check_keys(expr, info, meta, + ['struct', 'data'], ['base', 'if', 'features']) normalize_members(expr['data']) normalize_features(expr.get('features')) check_struct(expr, info) elif meta == 'command': - check_keys(expr, info, 'command', [], - ['data', 'returns', 'gen', 'success-response', - 'boxed', 'allow-oob', 'allow-preconfig', 'if']) + check_keys(expr, info, meta, + ['command'], + ['data', 'returns', 'boxed', 'if', + 'gen', 'success-response', 'allow-oob', + 'allow-preconfig']) normalize_members(expr.get('data')) check_command(expr, info) elif meta == 'event': - check_keys(expr, info, 'event', [], ['data', 'boxed', 'if']) + check_keys(expr, info, meta, + ['event'], ['data', 'boxed', 'if']) normalize_members(expr.get('data')) check_event(expr, info) else: -- cgit v1.1 From f63326985a23cc1bb6327a003050e6fdb52eb9cd Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:37 +0200 Subject: qapi: Improve reporting of missing documentation comment Have check_exprs() check this later, so the error message gains an "in definition line". Tweak the error message. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-25-armbru@redhat.com> --- scripts/qapi/common.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index fa354b3..bd83427 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -933,10 +933,6 @@ def check_exprs(exprs): if 'include' in expr: continue - if not doc and doc_required: - raise QAPISemError(info, - "definition missing documentation comment") - if 'enum' in expr: meta = 'enum' elif 'union' in expr: @@ -957,9 +953,14 @@ def check_exprs(exprs): info.set_defn(meta, name) check_defn_name_str(name, info, meta) - if doc and doc.symbol != name: - raise QAPISemError( - info, "documentation comment is for '%s'" % doc.symbol) + if doc: + if doc.symbol != name: + raise QAPISemError( + info, "documentation comment is for '%s'" % doc.symbol) + doc.check_expr(expr) + elif doc_required: + raise QAPISemError(info, + "documentation comment required") if meta == 'enum': check_keys(expr, info, meta, @@ -1004,9 +1005,6 @@ def check_exprs(exprs): check_if(expr, info, meta) check_flags(expr, info) - if doc: - doc.check_expr(expr) - return exprs -- cgit v1.1 From 56d2df5e65d873ca0e9841f7bb7676cab759f8da Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:38 +0200 Subject: qapi: Improve reporting of redefinition Point to the previous definition, unless it's a built-in. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-26-armbru@redhat.com> --- scripts/qapi/common.py | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index bd83427..a74cd95 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -1759,6 +1759,11 @@ class QAPISchema(object): # because they're liable to clash in generated C. other_ent = self._entity_dict.get(ent.name) if other_ent: + if other_ent.info: + where = QAPIError(other_ent.info, None, "previous definition") + raise QAPISemError( + ent.info, + "'%s' is already defined\n%s" % (ent.name, where)) raise QAPISemError( ent.info, "%s is already defined" % other_ent.describe()) self._entity_dict[ent.name] = ent -- cgit v1.1 From c615550df306a7b16e75d21f65ee38898c756bac Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 27 Sep 2019 15:46:39 +0200 Subject: qapi: Improve source file read error handling qapi-gen.py crashes when it can't open the main schema file, and when it can't read from any schema file. Lazy. Change QAPISchema.__init__() to take a file name instead of a file object. Move the open code from _include() to __init__(), so it's used for the main schema file, too. Move the read into the try for good measure, and rephrase the error message. Reporting open or read failure for the main schema file needs a QAPISourceInfo representing "no source". Make QAPISourceInfo cope with fname=None. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Message-Id: <20190927134639.4284-27-armbru@redhat.com> --- scripts/qapi/common.py | 46 ++++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 20 deletions(-) (limited to 'scripts/qapi/common.py') diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index a74cd95..d6e00c8 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -53,7 +53,12 @@ class QAPISourceInfo(object): return info def loc(self): - return '%s:%d' % (self.fname, self.line) + if self.fname is None: + return sys.argv[0] + ret = self.fname + if self.line is not None: + ret += ':%d' % self.line + return ret def in_defn(self): if self.defn_name: @@ -383,14 +388,26 @@ class QAPIDoc(object): class QAPISchemaParser(object): - def __init__(self, fp, previously_included=[], incl_info=None): - self.fname = fp.name - previously_included.append(os.path.abspath(fp.name)) - self.src = fp.read() + def __init__(self, fname, previously_included=[], incl_info=None): + previously_included.append(os.path.abspath(fname)) + + try: + if sys.version_info[0] >= 3: + fp = open(fname, 'r', encoding='utf-8') + else: + fp = open(fname, 'r') + self.src = fp.read() + except IOError as e: + raise QAPISemError(incl_info or QAPISourceInfo(None, None, None), + "can't read %s file '%s': %s" + % ("include" if incl_info else "schema", + fname, + e.strerror)) + if self.src == '' or self.src[-1] != '\n': self.src += '\n' self.cursor = 0 - self.info = QAPISourceInfo(self.fname, 1, incl_info) + self.info = QAPISourceInfo(fname, 1, incl_info) self.line_pos = 0 self.exprs = [] self.docs = [] @@ -414,7 +431,7 @@ class QAPISchemaParser(object): if not isinstance(include, str): raise QAPISemError(info, "value of 'include' must be a string") - incl_fname = os.path.join(os.path.dirname(self.fname), + incl_fname = os.path.join(os.path.dirname(fname), include) self.exprs.append({'expr': {'include': incl_fname}, 'info': info}) @@ -466,14 +483,7 @@ class QAPISchemaParser(object): if incl_abs_fname in previously_included: return None - try: - if sys.version_info[0] >= 3: - fobj = open(incl_fname, 'r', encoding='utf-8') - else: - fobj = open(incl_fname, 'r') - except IOError as e: - raise QAPISemError(info, "%s: %s" % (e.strerror, incl_fname)) - return QAPISchemaParser(fobj, previously_included, info) + return QAPISchemaParser(incl_fname, previously_included, info) def _pragma(self, name, value, info): global doc_required, returns_whitelist, name_case_whitelist @@ -1734,11 +1744,7 @@ class QAPISchemaEvent(QAPISchemaEntity): class QAPISchema(object): def __init__(self, fname): self.fname = fname - if sys.version_info[0] >= 3: - f = open(fname, 'r', encoding='utf-8') - else: - f = open(fname, 'r') - parser = QAPISchemaParser(f) + parser = QAPISchemaParser(fname) exprs = check_exprs(parser.exprs) self.docs = parser.docs self._entity_list = [] -- cgit v1.1