From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55355) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XXRnl-0007z5-40 for qemu-devel@nongnu.org; Fri, 26 Sep 2014 05:27:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XXRnf-0006yL-Cl for qemu-devel@nongnu.org; Fri, 26 Sep 2014 05:26:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34052) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XXRnf-0006y5-20 for qemu-devel@nongnu.org; Fri, 26 Sep 2014 05:26:51 -0400 From: Markus Armbruster References: <1411165504-18198-1-git-send-email-eblake@redhat.com> <1411165504-18198-14-git-send-email-eblake@redhat.com> Date: Fri, 26 Sep 2014 11:26:41 +0200 In-Reply-To: <1411165504-18198-14-git-send-email-eblake@redhat.com> (Eric Blake's message of "Fri, 19 Sep 2014 16:24:58 -0600") Message-ID: <87r3yyohpq.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v4 13/19] qapi: More rigourous checking of types List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Fam Zheng , qemu-devel@nongnu.org, wenchaoqemu@gmail.com, Luiz Capitulino Eric Blake writes: > Now that we know every expression is valid with regards to > its keys, we can add further tests that those keys refer to > valid types. With this patch, all references to a type (the > 'data': of command, type, union, and event, and the 'returns': > of command) must resolve to a builtin or another type declared > by the current qapi parse; this includes recursing into each > member of a data dictionary. Dealing with '**' and nested > sub-structs will be done in later patches. > > Update the testsuite to match improved output. > > Signed-off-by: Eric Blake > --- > scripts/qapi.py | 73 ++++++++++++++++++++++++++-- > tests/qapi-schema/data-array-empty.err | 1 + > tests/qapi-schema/data-array-empty.exit | 2 +- > tests/qapi-schema/data-array-empty.json | 2 +- > tests/qapi-schema/data-array-empty.out | 3 -- > tests/qapi-schema/data-array-unknown.err | 1 + > tests/qapi-schema/data-array-unknown.exit | 2 +- > tests/qapi-schema/data-array-unknown.json | 2 +- > tests/qapi-schema/data-array-unknown.out | 3 -- > tests/qapi-schema/data-int.err | 1 + > tests/qapi-schema/data-int.exit | 2 +- > tests/qapi-schema/data-int.json | 2 +- > tests/qapi-schema/data-int.out | 3 -- > tests/qapi-schema/data-member-array-bad.err | 1 + > tests/qapi-schema/data-member-array-bad.exit | 2 +- > tests/qapi-schema/data-member-array-bad.json | 2 +- > tests/qapi-schema/data-member-array-bad.out | 3 -- > tests/qapi-schema/data-member-unknown.err | 1 + > tests/qapi-schema/data-member-unknown.exit | 2 +- > tests/qapi-schema/data-member-unknown.json | 2 +- > tests/qapi-schema/data-member-unknown.out | 3 -- > tests/qapi-schema/data-unknown.err | 1 + > tests/qapi-schema/data-unknown.exit | 2 +- > tests/qapi-schema/data-unknown.json | 2 +- > tests/qapi-schema/data-unknown.out | 3 -- > tests/qapi-schema/returns-array-bad.err | 1 + > tests/qapi-schema/returns-array-bad.exit | 2 +- > tests/qapi-schema/returns-array-bad.json | 2 +- > tests/qapi-schema/returns-array-bad.out | 3 -- > tests/qapi-schema/returns-unknown.err | 1 + > tests/qapi-schema/returns-unknown.exit | 2 +- > tests/qapi-schema/returns-unknown.json | 2 +- > tests/qapi-schema/returns-unknown.out | 3 -- > 33 files changed, 93 insertions(+), 44 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index bf243fa..20c0ce9 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -255,6 +255,52 @@ def discriminator_find_enum_define(expr): > > return find_enum(discriminator_type) > > +def check_type(expr_info, source, data, allow_array = False, > + allowed_names = [], allow_dict = True): > + global all_types > + > + if data is None: > + return > + > + if data == '**': > + return > + > + # Check if array type for data is okay > + if isinstance(data, list): > + if not allow_array: > + raise QAPIExprError(expr_info, > + "%s cannot be an array" % source) > + if len(data) != 1 or not isinstance(data[0], basestring): > + raise QAPIExprError(expr_info, > + "%s: array type must contain single type name" > + % source) > + data = data[0] > + > + # Check if type name for data is okay > + if isinstance(data, basestring): > + if not data in all_types: > + raise QAPIExprError(expr_info, > + "%s references unknown type '%s'" > + % (source, data)) > + if not all_types[data] in allowed_names: > + raise QAPIExprError(expr_info, > + "%s cannot reference %s type '%s'" > + % (source, all_types[data], data)) > + return > + > + # data is a dictionary, check that each member is okay > + if not isinstance(data, OrderedDict): > + raise QAPIExprError(expr_info, > + "%s should be a dictionary" % source) > + if not allow_dict: > + raise QAPIExprError(expr_info, > + "%s should be a type name" % source) > + for (key, value) in data.items(): > + check_type(expr_info, "member '%s' of %s" % (key, source), value, > + allow_array=True, > + allowed_names=['built-in', 'union', 'struct', 'enum'], > + allow_dict=True) Hmm, allowed_names isn't about allowed names, it's about allowed meta-types. Rename? > + > def check_command(expr, expr_info): > global commands > name = expr['command'] > @@ -262,6 +308,15 @@ def check_command(expr, expr_info): > raise QAPIExprError(expr_info, > "command '%s' is already defined" % name) > commands.append(name) > + check_type(expr_info, "'data' for command '%s'" % name, > + expr.get('data'), allow_array=True, > + allowed_names=['union', 'struct']) > + check_type(expr_info, "'base' for command '%s'" % name, > + expr.get('base'), allowed_names=['struct'], > + allow_dict=False) > + check_type(expr_info, "'returns' for command '%s'" % name, > + expr.get('returns'), allow_array=True, > + allowed_names=['built-in', 'union', 'struct', 'enum']) > Nicely done. > def check_event(expr, expr_info): > global events > @@ -271,6 +326,8 @@ def check_event(expr, expr_info): > raise QAPIExprError(expr_info, > "event '%s' is already defined" % name) > events.append(name) > + check_type(expr_info, "'data' for event '%s'" % name, > + expr.get('data'), allowed_names=['union', 'struct']) > if params: > for argname, argentry, optional, structured in parse_args(params): > if structured: > @@ -357,19 +414,27 @@ def check_enum(expr, expr_info): > % (name, member, values[key])) > values[key] = member > > +def check_struct(expr, expr_info): > + name = expr['type'] > + members = expr['data'] > + > + check_type(expr_info, "'data' for type '%s'" % name, members) > + > def check_exprs(schema): > for expr_elem in schema.exprs: > expr = expr_elem['expr'] > info = expr_elem['info'] > > - if expr.has_key('union'): > - check_union(expr, info) > - if expr.has_key('event'): > - check_event(expr, info) > if expr.has_key('enum'): > check_enum(expr, info) > + if expr.has_key('union'): > + check_union(expr, info) > + if expr.has_key('type'): > + check_struct(expr, info) > if expr.has_key('command'): > check_command(expr, info) > + if expr.has_key('event'): > + check_event(expr, info) Not related to this patch: this chain of ifs bothers me. The conditions should be exclusive, because each expression must have a well-defined meta-type. If I remember correctly, you actually enforce this elsewhere in your series. Do we want to make it obvious in the code here? elif instead of if, perhaps? > > def check_keys(expr_elem, meta, required, optional=[]): > expr = expr_elem['expr'] > diff --git a/tests/qapi-schema/data-array-empty.err b/tests/qapi-schema/data-array-empty.err > index e69de29..94e046b 100644 > --- a/tests/qapi-schema/data-array-empty.err > +++ b/tests/qapi-schema/data-array-empty.err > @@ -0,0 +1 @@ > +tests/qapi-schema/data-array-empty.json:2: 'data' for command 'oops': array type must contain single type name > diff --git a/tests/qapi-schema/data-array-empty.exit b/tests/qapi-schema/data-array-empty.exit > index 573541a..d00491f 100644 > --- a/tests/qapi-schema/data-array-empty.exit > +++ b/tests/qapi-schema/data-array-empty.exit > @@ -1 +1 @@ > -0 > +1 > diff --git a/tests/qapi-schema/data-array-empty.json b/tests/qapi-schema/data-array-empty.json > index 41b6c1e..fdd5612 100644 > --- a/tests/qapi-schema/data-array-empty.json > +++ b/tests/qapi-schema/data-array-empty.json > @@ -1,2 +1,2 @@ > -# FIXME: we should reject an array for data if it does not contain a known type > +# we reject an array for data if it does not contain a known type > { 'command': 'oops', 'data': [ ] } > diff --git a/tests/qapi-schema/data-array-empty.out b/tests/qapi-schema/data-array-empty.out > index 67802be..e69de29 100644 > --- a/tests/qapi-schema/data-array-empty.out > +++ b/tests/qapi-schema/data-array-empty.out > @@ -1,3 +0,0 @@ > -[OrderedDict([('command', 'oops'), ('data', [])])] > -[] > -[] > diff --git a/tests/qapi-schema/data-array-unknown.err b/tests/qapi-schema/data-array-unknown.err > index e69de29..a66c2d6 100644 > --- a/tests/qapi-schema/data-array-unknown.err > +++ b/tests/qapi-schema/data-array-unknown.err > @@ -0,0 +1 @@ > +tests/qapi-schema/data-array-unknown.json:2: 'data' for command 'oops' references unknown type 'NoSuchType' The wording "references type" somehow unduly excites my "reference type" synapses :) Perhaps something like "Type 'NoSuchType' is unknown" would suffice. Entirely up to you; feel free to keep the wording as is. > diff --git a/tests/qapi-schema/data-array-unknown.exit b/tests/qapi-schema/data-array-unknown.exit > index 573541a..d00491f 100644 > --- a/tests/qapi-schema/data-array-unknown.exit > +++ b/tests/qapi-schema/data-array-unknown.exit > @@ -1 +1 @@ > -0 > +1 > diff --git a/tests/qapi-schema/data-array-unknown.json b/tests/qapi-schema/data-array-unknown.json > index 434fb5f..9c75b96 100644 > --- a/tests/qapi-schema/data-array-unknown.json > +++ b/tests/qapi-schema/data-array-unknown.json > @@ -1,2 +1,2 @@ > -# FIXME: we should reject an array for data if it does not contain a known type > +# we reject an array for data if it does not contain a known type > { 'command': 'oops', 'data': [ 'NoSuchType' ] } > diff --git a/tests/qapi-schema/data-array-unknown.out b/tests/qapi-schema/data-array-unknown.out > index c3bc05e..e69de29 100644 > --- a/tests/qapi-schema/data-array-unknown.out > +++ b/tests/qapi-schema/data-array-unknown.out > @@ -1,3 +0,0 @@ > -[OrderedDict([('command', 'oops'), ('data', ['NoSuchType'])])] > -[] > -[] > diff --git a/tests/qapi-schema/data-int.err b/tests/qapi-schema/data-int.err > index e69de29..e1d3ed5 100644 > --- a/tests/qapi-schema/data-int.err > +++ b/tests/qapi-schema/data-int.err > @@ -0,0 +1 @@ > +tests/qapi-schema/data-int.json:2: 'data' for command 'oops' cannot reference built-in type 'int' Perhaps something like "'data' for command 'oops' can't be of built-in type 'int'", or maybe positive instead of negative: "a command's 'data' must be of complex type". Again, entirely up to you. [...]