From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42197) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbRll-0006Tb-Ao for qemu-devel@nongnu.org; Fri, 27 Mar 2015 06:45:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YbRli-0003id-2r for qemu-devel@nongnu.org; Fri, 27 Mar 2015 06:45:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52501) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbRlh-0003iX-NZ for qemu-devel@nongnu.org; Fri, 27 Mar 2015 06:45:38 -0400 From: Markus Armbruster References: <1427227433-5030-1-git-send-email-eblake@redhat.com> <1427227433-5030-29-git-send-email-eblake@redhat.com> Date: Fri, 27 Mar 2015 11:45:33 +0100 In-Reply-To: <1427227433-5030-29-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 24 Mar 2015 14:03:53 -0600") Message-ID: <87sicqoh0y.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v5 28/28] qapi: Drop support for inline nested types List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, lcapitulino@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, wenchaoqemu@gmail.com Eric Blake writes: > A future patch will be using a 'name':{dictionary} entry in the > QAPI schema to specify a default value for an optional argument; > but existing use of inline nested structs conflicts with that goal. > Now that all commands have been changed to avoid inline nested > structs, nuke support for them, and turn it into a hard error. > Update the testsuite to reflect tighter parsing rules. > > Signed-off-by: Eric Blake > --- > scripts/qapi-commands.py | 8 +++--- > scripts/qapi-event.py | 4 +-- > scripts/qapi-types.py | 9 ++----- > scripts/qapi-visit.py | 37 ++++------------------------ > scripts/qapi.py | 18 +++++--------- > tests/qapi-schema/event-nest-struct.err | 2 +- > tests/qapi-schema/nested-struct-data.err | 1 + > tests/qapi-schema/nested-struct-data.exit | 2 +- > tests/qapi-schema/nested-struct-data.json | 2 +- > tests/qapi-schema/nested-struct-data.out | 3 --- > tests/qapi-schema/nested-struct-returns.err | 1 + > tests/qapi-schema/nested-struct-returns.exit | 2 +- > tests/qapi-schema/nested-struct-returns.json | 2 +- > tests/qapi-schema/nested-struct-returns.out | 3 --- > 14 files changed, 26 insertions(+), 68 deletions(-) > > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > index 053ba85..db81044 100644 > --- a/scripts/qapi-commands.py > +++ b/scripts/qapi-commands.py > @@ -28,7 +28,7 @@ def type_visitor(name): > > def generate_command_decl(name, args, ret_type): > arglist="" > - for argname, argtype, optional, structured in parse_args(args): > + for argname, argtype, optional in parse_args(args): > argtype = c_type(argtype, is_param=True) > if optional: > arglist += "bool has_%s, " % c_var(argname) > @@ -53,7 +53,7 @@ def gen_sync_call(name, args, ret_type, indent=0): > retval="" > if ret_type: > retval = "retval = " > - for argname, argtype, optional, structured in parse_args(args): > + for argname, argtype, optional in parse_args(args): > if optional: > arglist += "has_%s, " % c_var(argname) > arglist += "%s, " % (c_var(argname)) > @@ -96,7 +96,7 @@ Visitor *v; > def gen_visitor_input_vars_decl(args): > ret = "" > push_indent() > - for argname, argtype, optional, structured in parse_args(args): > + for argname, argtype, optional in parse_args(args): > if optional: > ret += mcgen(''' > bool has_%(argname)s = false; > @@ -139,7 +139,7 @@ v = qapi_dealloc_get_visitor(md); > v = qmp_input_get_visitor(mi); > ''') > > - for argname, argtype, optional, structured in parse_args(args): > + for argname, argtype, optional in parse_args(args): > if optional: > ret += mcgen(''' > visit_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s); > diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py > index 601e307..47dc041 100644 > --- a/scripts/qapi-event.py > +++ b/scripts/qapi-event.py > @@ -21,7 +21,7 @@ def _generate_event_api_name(event_name, params): > l = len(api_name) > > if params: > - for argname, argentry, optional, structured in parse_args(params): > + for argname, argentry, optional in parse_args(params): > if optional: > api_name += "bool has_%s,\n" % c_var(argname) > api_name += "".ljust(l) > @@ -93,7 +93,7 @@ def generate_event_implement(api_name, event_name, params): > """, > event_name = event_name) > > - for argname, argentry, optional, structured in parse_args(params): > + for argname, argentry, optional in parse_args(params): > if optional: > ret += mcgen(""" > if (has_%(var)s) { > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index 9c8d68c..4789528 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -63,18 +63,13 @@ typedef struct %(name)sList > def generate_struct_fields(members): > ret = '' > > - for argname, argentry, optional, structured in parse_args(members): > + for argname, argentry, optional in parse_args(members): > if optional: > ret += mcgen(''' > bool has_%(c_name)s; > ''', > c_name=c_var(argname)) > - if structured: > - push_indent() > - ret += generate_struct({ "field": argname, "data": argentry}) > - pop_indent() > - else: > - ret += mcgen(''' > + ret += mcgen(''' > %(c_type)s %(c_name)s; > ''', > c_type=c_type(argentry), c_name=c_var(argname)) > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 3e11089..10b08c6 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -51,27 +51,6 @@ def generate_visit_struct_fields(name, field_prefix, fn_prefix, members, base = > else: > full_name = "%s_%s" % (name, fn_prefix) > > - for argname, argentry, optional, structured in parse_args(members): > - if structured: > - if not fn_prefix: > - nested_fn_prefix = argname > - else: > - nested_fn_prefix = "%s_%s" % (fn_prefix, argname) > - > - nested_field_prefix = "%s%s." % (field_prefix, argname) > - ret += generate_visit_struct_fields(name, nested_field_prefix, > - nested_fn_prefix, argentry) > - ret += mcgen(''' > - > -static void visit_type_%(full_name)s_field_%(c_name)s(Visitor *m, %(name)s **obj, Error **errp) > -{ > -''', > - name=name, full_name=full_name, c_name=c_var(argname)) > - ret += generate_visit_struct_body(full_name, argname, argentry) > - ret += mcgen(''' > -} > -''') > - > if base: > ret += generate_visit_implicit_struct(base) > > @@ -94,7 +73,7 @@ if (err) { > c_prefix=c_var(field_prefix), > type=type_name(base), c_name=c_var('base')) > > - for argname, argentry, optional, structured in parse_args(members): > + for argname, argentry, optional in parse_args(members): > if optional: > ret += mcgen(''' > visit_optional(m, &(*obj)->%(c_prefix)shas_%(c_name)s, "%(name)s", &err); > @@ -104,18 +83,12 @@ if (!err && (*obj)->%(prefix)shas_%(c_name)s) { > c_name=c_var(argname), name=argname) > push_indent() > > - if structured: > - ret += mcgen(''' > -visit_type_%(full_name)s_field_%(c_name)s(m, obj, &err); > -''', > - full_name=full_name, c_name=c_var(argname)) > - else: > - ret += mcgen(''' > + ret += mcgen(''' > visit_type_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, "%(name)s", &err); > ''', > - c_prefix=c_var(field_prefix), prefix=field_prefix, > - type=type_name(argentry), c_name=c_var(argname), > - name=argname) > + c_prefix=c_var(field_prefix), prefix=field_prefix, > + type=type_name(argentry), c_name=c_var(argname), > + name=argname) > > if optional: > pop_indent() > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 800e8e4..8e5b4ad 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -368,11 +368,13 @@ def check_type(expr_info, source, data, allow_array = False, > for (key, value) in data.items(): > check_name(expr_info, "Member of %s" % source, key, > allow_optional=allow_optional) > + # Todo: allow dictionaries to represent default values of > + # an optional argument. > check_type(expr_info, "Member '%s' of %s" % (key, source), value, > allow_array=True, > allowed_metas=['built-in', 'union', 'alternate', 'struct', > 'enum'], > - allow_dict=True, allow_optional=True, allow_star=allow_star) > + allow_dict=False, allow_star=allow_star) check_type() uses allow_optional only when allow_dict. Okay. > > def check_command(expr, expr_info): > name = expr['command'] > @@ -404,13 +406,6 @@ def check_event(expr, expr_info): > check_type(expr_info, "'data' for event '%s'" % name, > expr.get('data'), allowed_metas=['union', 'struct'], > allow_optional=True) > - if params: > - for argname, argentry, optional, structured in parse_args(params): > - if structured: > - raise QAPIExprError(expr_info, > - "Nested structure define in event is not " > - "supported, event '%s', argname '%s'" > - % (expr['event'], argname)) > > def check_union(expr, expr_info): > name = expr['union'] > @@ -667,13 +662,12 @@ def parse_args(typeinfo): > argname = member > argentry = typeinfo[member] > optional = False > - structured = False > if member.startswith('*'): > argname = member[1:] > optional = True > - if isinstance(argentry, OrderedDict): > - structured = True > - yield (argname, argentry, optional, structured) > + # Todo: allow argentry to be OrderedDict, for providing the > + # value of an optional argument. > + yield (argname, argentry, optional) > > def de_camel_case(name): > new_name = '' > diff --git a/tests/qapi-schema/event-nest-struct.err b/tests/qapi-schema/event-nest-struct.err > index 91bde1c..5a42701 100644 > --- a/tests/qapi-schema/event-nest-struct.err > +++ b/tests/qapi-schema/event-nest-struct.err > @@ -1 +1 @@ > -tests/qapi-schema/event-nest-struct.json:1: Nested structure define in event is not supported, event 'EVENT_A', argname 'a' > +tests/qapi-schema/event-nest-struct.json:1: Member 'a' of 'data' for event 'EVENT_A' should be a type name > diff --git a/tests/qapi-schema/nested-struct-data.err b/tests/qapi-schema/nested-struct-data.err > index e69de29..da767ba 100644 > --- a/tests/qapi-schema/nested-struct-data.err > +++ b/tests/qapi-schema/nested-struct-data.err > @@ -0,0 +1 @@ > +tests/qapi-schema/nested-struct-data.json:2: Member 'a' of 'data' for command 'foo' should be a type name > diff --git a/tests/qapi-schema/nested-struct-data.exit b/tests/qapi-schema/nested-struct-data.exit > index 573541a..d00491f 100644 > --- a/tests/qapi-schema/nested-struct-data.exit > +++ b/tests/qapi-schema/nested-struct-data.exit > @@ -1 +1 @@ > -0 > +1 > diff --git a/tests/qapi-schema/nested-struct-data.json b/tests/qapi-schema/nested-struct-data.json > index 0247c8c..3d52d2b 100644 > --- a/tests/qapi-schema/nested-struct-data.json > +++ b/tests/qapi-schema/nested-struct-data.json > @@ -1,4 +1,4 @@ > -# FIXME: inline subtypes collide with our desired future use of defaults Seen only now: I'd mark this TODO rather than FIXME in PATCH 19, because it's not actually broken. Only if you respin anyway, of course. > +# inline subtypes collide with our desired future use of defaults > { 'command': 'foo', > 'data': { 'a' : { 'string' : 'str', 'integer': 'int' }, 'b' : 'str' }, > 'returns': {} } > diff --git a/tests/qapi-schema/nested-struct-data.out b/tests/qapi-schema/nested-struct-data.out > index 999cbb8..e69de29 100644 > --- a/tests/qapi-schema/nested-struct-data.out > +++ b/tests/qapi-schema/nested-struct-data.out > @@ -1,3 +0,0 @@ > -[OrderedDict([('command', 'foo'), ('data', OrderedDict([('a', OrderedDict([('string', 'str'), ('integer', 'int')])), ('b', 'str')])), ('returns', OrderedDict())])] > -[] > -[] > diff --git a/tests/qapi-schema/nested-struct-returns.err b/tests/qapi-schema/nested-struct-returns.err > index e69de29..5238d07 100644 > --- a/tests/qapi-schema/nested-struct-returns.err > +++ b/tests/qapi-schema/nested-struct-returns.err > @@ -0,0 +1 @@ > +tests/qapi-schema/nested-struct-returns.json:2: Member 'a' of 'returns' for command 'foo' should be a type name > diff --git a/tests/qapi-schema/nested-struct-returns.exit b/tests/qapi-schema/nested-struct-returns.exit > index 573541a..d00491f 100644 > --- a/tests/qapi-schema/nested-struct-returns.exit > +++ b/tests/qapi-schema/nested-struct-returns.exit > @@ -1 +1 @@ > -0 > +1 > diff --git a/tests/qapi-schema/nested-struct-returns.json b/tests/qapi-schema/nested-struct-returns.json > index 5a46840..d2cd047 100644 > --- a/tests/qapi-schema/nested-struct-returns.json > +++ b/tests/qapi-schema/nested-struct-returns.json > @@ -1,3 +1,3 @@ > -# FIXME: inline subtypes collide with our desired future use of defaults > +# inline subtypes collide with our desired future use of defaults > { 'command': 'foo', > 'returns': { 'a' : { 'string' : 'str', 'integer': 'int' }, 'b' : 'str' } } > diff --git a/tests/qapi-schema/nested-struct-returns.out b/tests/qapi-schema/nested-struct-returns.out > index c53d23b..e69de29 100644 > --- a/tests/qapi-schema/nested-struct-returns.out > +++ b/tests/qapi-schema/nested-struct-returns.out > @@ -1,3 +0,0 @@ > -[OrderedDict([('command', 'foo'), ('returns', OrderedDict([('a', OrderedDict([('string', 'str'), ('integer', 'int')])), ('b', 'str')]))])] > -[] > -[] Reviewed-by: Markus Armbruster