From f9bee751be1292c9433a95d835474dc38a134a95 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 7 May 2014 09:53:44 +0200 Subject: qapi: Normalize marshalling's visitor initialization and cleanup Input and output marshalling functions do it differently. Change them to work the same: initialize the I/O visitor, use it, clean it up, initialize the dealloc visitor, use it, clean it up. This delays dealloc visitor initialization in output marshalling functions, and input visitor cleanup in input marshalling functions. No functional change, but the latter will be convenient when I change the error handling. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Luiz Capitulino --- scripts/qapi-commands.py | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index 8d9096f..1399826 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -69,16 +69,17 @@ def gen_marshal_output_call(name, ret_type): return "" return "qmp_marshal_output_%s(retval, ret, errp);" % c_fun(name) -def gen_visitor_input_containers_decl(args): +def gen_visitor_input_containers_decl(args, obj): ret = "" push_indent() if len(args) > 0: ret += mcgen(''' -QmpInputVisitor *mi; +QmpInputVisitor *mi = qmp_input_visitor_new_strict(%(obj)s); QapiDeallocVisitor *md; Visitor *v; -''') +''', + obj=obj) pop_indent() return ret.rstrip() @@ -106,7 +107,7 @@ bool has_%(argname)s = false; pop_indent() return ret.rstrip() -def gen_visitor_input_block(args, obj, dealloc=False): +def gen_visitor_input_block(args, dealloc=False): ret = "" errparg = 'errp' @@ -118,15 +119,14 @@ def gen_visitor_input_block(args, obj, dealloc=False): if dealloc: errparg = 'NULL' ret += mcgen(''' +qmp_input_visitor_cleanup(mi); md = qapi_dealloc_visitor_new(); v = qapi_dealloc_get_visitor(md); ''') else: ret += mcgen(''' -mi = qmp_input_visitor_new_strict(%(obj)s); v = qmp_input_get_visitor(mi); -''', - obj=obj) +''') for argname, argtype, optional, structured in parse_args(args): if optional: @@ -152,10 +152,6 @@ visit_end_optional(v, %(errp)s); ret += mcgen(''' qapi_dealloc_visitor_cleanup(md); ''') - else: - ret += mcgen(''' -qmp_input_visitor_cleanup(mi); -''') pop_indent() return ret.rstrip() @@ -166,8 +162,8 @@ def gen_marshal_output(name, args, ret_type, middle_mode): ret = mcgen(''' static void qmp_marshal_output_%(c_name)s(%(c_ret_type)s ret_in, QObject **ret_out, Error **errp) { - QapiDeallocVisitor *md = qapi_dealloc_visitor_new(); QmpOutputVisitor *mo = qmp_output_visitor_new(); + QapiDeallocVisitor *md; Visitor *v; v = qmp_output_get_visitor(mo); @@ -176,6 +172,7 @@ static void qmp_marshal_output_%(c_name)s(%(c_ret_type)s ret_in, QObject **ret_o *ret_out = qmp_output_get_qobject(mo); } qmp_output_visitor_cleanup(mo); + md = qapi_dealloc_visitor_new(); v = qapi_dealloc_get_visitor(md); %(visitor)s(v, &ret_in, "unused", NULL); qapi_dealloc_visitor_cleanup(md); @@ -228,9 +225,9 @@ def gen_marshal_input(name, args, ret_type, middle_mode): %(visitor_input_block)s ''', - visitor_input_containers_decl=gen_visitor_input_containers_decl(args), + visitor_input_containers_decl=gen_visitor_input_containers_decl(args, "QOBJECT(args)"), visitor_input_vars_decl=gen_visitor_input_vars_decl(args), - visitor_input_block=gen_visitor_input_block(args, "QOBJECT(args)")) + visitor_input_block=gen_visitor_input_block(args)) else: ret += mcgen(''' (void)args; @@ -250,7 +247,7 @@ out: ret += mcgen(''' %(visitor_input_block_cleanup)s ''', - visitor_input_block_cleanup=gen_visitor_input_block(args, None, + visitor_input_block_cleanup=gen_visitor_input_block(args, dealloc=True)) if middle_mode: -- cgit v1.1 From e2cd0f4fb42b1fae65ad22e8efde9804446e6254 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 7 May 2014 09:53:46 +0200 Subject: qapi: Replace start_optional()/end_optional() by optional() Semantics of end_optional() differ subtly from the other end_FOO() callbacks: when start_FOO() succeeds, the matching end_FOO() gets called regardless of what happens in between. end_optional() gets called only when everything in between succeeds as well. Entirely undocumented, like all of the visitor API. The only user of Visitor Callback end_optional() never did anything, and was removed in commit 9f9ab46. I'm about to clean up error handling in the generated visitor code, and end_optional() is in my way. No users mean no test cases, and making non-trivial cleanup transformations without test cases doesn't strike me as a good idea. Drop end_optional(), and rename start_optional() to optional(). We can always go back to a pair of callbacks when we have an actual need. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Luiz Capitulino --- scripts/qapi-commands.py | 5 ++--- scripts/qapi-visit.py | 3 +-- 2 files changed, 3 insertions(+), 5 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index 1399826..341dba2 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -131,7 +131,7 @@ v = qmp_input_get_visitor(mi); for argname, argtype, optional, structured in parse_args(args): if optional: ret += mcgen(''' -visit_start_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s); +visit_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s); if (has_%(c_name)s) { ''', c_name=c_var(argname), name=argname, errp=errparg) @@ -145,8 +145,7 @@ if (has_%(c_name)s) { pop_indent() ret += mcgen(''' } -visit_end_optional(v, %(errp)s); -''', errp=errparg) +''') if dealloc: ret += mcgen(''' diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index c6579be..9f22eee 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -61,7 +61,7 @@ if (!err) { for argname, argentry, optional, structured in parse_args(members): if optional: ret += mcgen(''' -visit_start_optional(m, &(*obj)->%(c_prefix)shas_%(c_name)s, "%(name)s", &err); +visit_optional(m, &(*obj)->%(c_prefix)shas_%(c_name)s, "%(name)s", &err); if ((*obj)->%(prefix)shas_%(c_name)s) { ''', c_prefix=c_var(field_prefix), prefix=field_prefix, @@ -82,7 +82,6 @@ visit_type_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, "%(name)s", &err); pop_indent() ret += mcgen(''' } -visit_end_optional(m, &err); ''') pop_indent() -- cgit v1.1 From 468866b816f13c8141b692ac0280706c16b30185 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 7 May 2014 09:53:47 +0200 Subject: qapi-visit.py: Clean up confusing push_indent() / pop_indent() use Changing implicit indentation in the middle of generating a block makes following the code being generated unnecessarily hard. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Luiz Capitulino --- scripts/qapi-visit.py | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 9f22eee..28176ba 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -128,12 +128,14 @@ if (!err) { ''', name=full_name) + ret += mcgen(''' + /* Always call end_struct if start_struct succeeded. */ + visit_end_struct(m, &err); +} +error_propagate(errp, err); +''') pop_indent() ret += mcgen(''' - /* Always call end_struct if start_struct succeeded. */ - visit_end_struct(m, &err); - } - error_propagate(errp, err); } ''') return ret @@ -289,19 +291,15 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error ** ''', name=name) - - push_indent() push_indent() push_indent() if base: ret += mcgen(''' - visit_type_%(name)s_fields(m, obj, &err); + visit_type_%(name)s_fields(m, obj, &err); ''', name=name) - pop_indent() - if not discriminator: disc_key = "type" else: @@ -343,19 +341,17 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error ** } error_propagate(errp, err); err = NULL; - } ''') pop_indent() - ret += mcgen(''' - /* Always call end_struct if start_struct succeeded. */ - visit_end_struct(m, &err); - } - error_propagate(errp, err); -} -''') + pop_indent() - pop_indent(); ret += mcgen(''' + } + /* Always call end_struct if start_struct succeeded. */ + visit_end_struct(m, &err); + } + error_propagate(errp, err); + } } ''') -- cgit v1.1 From 4fa953f20d418256504d24ea16f1b39791d12e5b Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 7 May 2014 09:53:48 +0200 Subject: qapi: Clean up shadowing of parameters and locals in inner scopes By un-inlining the visit of nested complex types. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Luiz Capitulino --- scripts/qapi-visit.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 28176ba..57be9bf 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -35,6 +35,19 @@ def generate_visit_struct_fields(name, field_prefix, fn_prefix, members, base = 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) +{ + Error *err = NULL; +''', + name=name, full_name=full_name, c_name=c_var(argname)) + push_indent() + ret += generate_visit_struct_body(full_name, argname, argentry) + pop_indent() + ret += mcgen(''' +} +''') ret += mcgen(''' @@ -69,7 +82,10 @@ if ((*obj)->%(prefix)shas_%(c_name)s) { push_indent() if structured: - ret += generate_visit_struct_body(full_name, argname, argentry) + 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(''' visit_type_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, "%(name)s", &err); @@ -106,8 +122,6 @@ if (!error_is_set(errp)) { if len(field_prefix): ret += mcgen(''' -Error **errp = &err; /* from outer scope */ -Error *err = NULL; visit_start_struct(m, NULL, "", "%(name)s", 0, &err); ''', name=name) -- cgit v1.1 From 192cca60aec59eda9a36501f03fc9dabf4d66a16 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 7 May 2014 09:53:49 +0200 Subject: qapi-visit.py: Clean up a sloppy use of field prefix generate_visit_struct_fields() generates the base type's struct member name both with and without the field prefix. Harmless, because the field prefix is always empty there: only unboxed complex members have a prefix, and those can't have a base type. Clean it up anyway. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Luiz Capitulino --- scripts/qapi-visit.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'scripts') diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 57be9bf..1e368be 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -60,7 +60,7 @@ static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error * if base: ret += mcgen(''' -visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(type)s), &err); +visit_start_implicit_struct(m, (void**) &(*obj)->%(c_prefix)s%(c_name)s, sizeof(%(type)s), &err); if (!err) { visit_type_%(type)s_fields(m, &(*obj)->%(c_prefix)s%(c_name)s, &err); error_propagate(errp, err); -- cgit v1.1 From be3c771796cb5ca4e7de2e04ed3cd2e3b6b76185 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 7 May 2014 09:53:50 +0200 Subject: qapi: Un-inline visit of implicit struct In preparation of error handling changes. Bonus: generates less duplicated code. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Luiz Capitulino --- scripts/qapi-visit.py | 48 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 14 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 1e368be..1ef753a 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -17,6 +17,31 @@ import os import getopt import errno +implicit_structs = [] + +def generate_visit_implicit_struct(type): + global implicit_structs + if type in implicit_structs: + return '' + implicit_structs.append(type) + return mcgen(''' + +static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error **errp) +{ + Error *err = NULL; + + visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), &err); + if (!err) { + visit_type_%(c_type)s_fields(m, obj, &err); + error_propagate(errp, err); + err = NULL; + visit_end_implicit_struct(m, &err); + } + error_propagate(errp, err); +} +''', + c_type=type_name(type)) + def generate_visit_struct_fields(name, field_prefix, fn_prefix, members, base = None): substructs = [] ret = '' @@ -49,6 +74,9 @@ static void visit_type_%(full_name)s_field_%(c_name)s(Visitor *m, %(name)s **obj } ''') + if base: + ret += generate_visit_implicit_struct(base) + ret += mcgen(''' static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error **errp) @@ -60,13 +88,7 @@ static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error * if base: ret += mcgen(''' -visit_start_implicit_struct(m, (void**) &(*obj)->%(c_prefix)s%(c_name)s, sizeof(%(type)s), &err); -if (!err) { - visit_type_%(type)s_fields(m, &(*obj)->%(c_prefix)s%(c_name)s, &err); - error_propagate(errp, err); - err = NULL; - visit_end_implicit_struct(m, &err); -} +visit_type_implicit_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, &err); ''', c_prefix=c_var(field_prefix), type=type_name(base), c_name=c_var('base')) @@ -292,6 +314,10 @@ def generate_visit_union(expr): del base_fields[discriminator] ret += generate_visit_struct_fields(name, "", "", base_fields) + if discriminator: + for key in members: + ret += generate_visit_implicit_struct(members[key]) + ret += mcgen(''' void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp) @@ -330,13 +356,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error ** if not discriminator: fmt = 'visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);' else: - fmt = '''visit_start_implicit_struct(m, (void**) &(*obj)->%(c_name)s, sizeof(%(c_type)s), &err); - if (!err) { - visit_type_%(c_type)s_fields(m, &(*obj)->%(c_name)s, &err); - error_propagate(errp, err); - err = NULL; - visit_end_implicit_struct(m, &err); - }''' + fmt = 'visit_type_implicit_%(c_type)s(m, &(*obj)->%(c_name)s, &err);' enum_full_value = generate_enum_full_value(disc_type, key) ret += mcgen(''' -- cgit v1.1 From 297a3646c2947ee64a6d42ca264039732c6218e0 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 7 May 2014 09:53:54 +0200 Subject: qapi: Replace uncommon use of the error API by the common one We commonly use the error API like this: err = NULL; foo(..., &err); if (err) { goto out; } bar(..., &err); Every error source is checked separately. The second function is only called when the first one succeeds. Both functions are free to pass their argument to error_set(). Because error_set() asserts no error has been set, this effectively means they must not be called with an error set. The qapi-generated code uses the error API differently: // *errp was initialized to NULL somewhere up the call chain frob(..., errp); gnat(..., errp); Errors accumulate in *errp: first error wins, subsequent errors get dropped. To make this work, the second function does nothing when called with an error set. Requires non-null errp, or else the second function can't see the first one fail. This usage has also bled into visitor tests, and two device model object property getters rtc_get_date() and balloon_stats_get_all(). With the "accumulate" technique, you need fewer error checks in callers, and buy that with an error check in every callee. Can be nice. However, mixing the two techniques is confusing. You can't use the "accumulate" technique with functions designed for the "check separately" technique. You can use the "check separately" technique with functions designed for the "accumulate" technique, but then error_set() can't catch you setting an error more than once. Standardize on the "check separately" technique for now, because it's overwhelmingly prevalent. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Luiz Capitulino --- scripts/qapi-commands.py | 57 +++++++++++----- scripts/qapi-visit.py | 171 ++++++++++++++++++++++++----------------------- 2 files changed, 128 insertions(+), 100 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index 341dba2..386f17e 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -2,16 +2,19 @@ # QAPI command marshaller generator # # Copyright IBM, Corp. 2011 +# Copyright (C) 2014 Red Hat, Inc. # # Authors: # Anthony Liguori # Michael Roth +# Markus Armbruster # # This work is licensed under the terms of the GNU GPL, version 2. # See the COPYING file in the top-level directory. from ordereddict import OrderedDict from qapi import * +import re import sys import os import getopt @@ -37,6 +40,15 @@ def generate_command_decl(name, args, ret_type): ''', ret_type=c_type(ret_type), name=c_fun(name), args=arglist).strip() +def gen_err_check(errvar): + if errvar: + return mcgen(''' +if (local_err) { + goto out; +} +''') + return '' + def gen_sync_call(name, args, ret_type, indent=0): ret = "" arglist="" @@ -49,15 +61,14 @@ def gen_sync_call(name, args, ret_type, indent=0): arglist += "%s, " % (c_var(argname)) push_indent(indent) ret = mcgen(''' -%(retval)sqmp_%(name)s(%(args)serrp); +%(retval)sqmp_%(name)s(%(args)s&local_err); ''', name=c_fun(name), args=arglist, retval=retval).rstrip() if ret_type: + ret += "\n" + gen_err_check('local_err') ret += "\n" + mcgen('''' -if (!error_is_set(errp)) { - %(marshal_output_call)s -} +%(marshal_output_call)s ''', marshal_output_call=gen_marshal_output_call(name, ret_type)).rstrip() pop_indent(indent) @@ -67,7 +78,7 @@ if (!error_is_set(errp)) { def gen_marshal_output_call(name, ret_type): if not ret_type: return "" - return "qmp_marshal_output_%s(retval, ret, errp);" % c_fun(name) + return "qmp_marshal_output_%s(retval, ret, &local_err);" % c_fun(name) def gen_visitor_input_containers_decl(args, obj): ret = "" @@ -109,7 +120,8 @@ bool has_%(argname)s = false; def gen_visitor_input_block(args, dealloc=False): ret = "" - errparg = 'errp' + errparg = '&local_err' + errarg = 'local_err' if len(args) == 0: return ret @@ -118,6 +130,7 @@ def gen_visitor_input_block(args, dealloc=False): if dealloc: errparg = 'NULL' + errarg = None; ret += mcgen(''' qmp_input_visitor_cleanup(mi); md = qapi_dealloc_visitor_new(); @@ -132,15 +145,20 @@ v = qmp_input_get_visitor(mi); if optional: ret += mcgen(''' visit_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s); -if (has_%(c_name)s) { ''', c_name=c_var(argname), name=argname, errp=errparg) + ret += gen_err_check(errarg) + ret += mcgen(''' +if (has_%(c_name)s) { +''', + c_name=c_var(argname)) push_indent() ret += mcgen(''' %(visitor)s(v, &%(c_name)s, "%(name)s", %(errp)s); ''', c_name=c_var(argname), name=argname, argtype=argtype, visitor=type_visitor(argtype), errp=errparg) + ret += gen_err_check(errarg) if optional: pop_indent() ret += mcgen(''' @@ -161,15 +179,20 @@ def gen_marshal_output(name, args, ret_type, middle_mode): ret = mcgen(''' static void qmp_marshal_output_%(c_name)s(%(c_ret_type)s ret_in, QObject **ret_out, Error **errp) { + Error *local_err = NULL; QmpOutputVisitor *mo = qmp_output_visitor_new(); QapiDeallocVisitor *md; Visitor *v; v = qmp_output_get_visitor(mo); - %(visitor)s(v, &ret_in, "unused", errp); - if (!error_is_set(errp)) { - *ret_out = qmp_output_get_qobject(mo); + %(visitor)s(v, &ret_in, "unused", &local_err); + if (local_err) { + goto out; } + *ret_out = qmp_output_get_qobject(mo); + +out: + error_propagate(errp, local_err); qmp_output_visitor_cleanup(mo); md = qapi_dealloc_visitor_new(); v = qapi_dealloc_get_visitor(md); @@ -196,13 +219,12 @@ def gen_marshal_input(name, args, ret_type, middle_mode): ret = mcgen(''' %(header)s { + Error *local_err = NULL; ''', header=hdr) if middle_mode: ret += mcgen(''' - Error *local_err = NULL; - Error **errp = &local_err; QDict *args = (QDict *)qdict; ''') @@ -229,20 +251,23 @@ def gen_marshal_input(name, args, ret_type, middle_mode): visitor_input_block=gen_visitor_input_block(args)) else: ret += mcgen(''' + (void)args; ''') ret += mcgen(''' - if (error_is_set(errp)) { - goto out; - } %(sync_call)s ''', sync_call=gen_sync_call(name, args, ret_type, indent=4)) - ret += mcgen(''' + if re.search('^ *goto out\\;', ret, re.MULTILINE): + ret += mcgen(''' out: ''') + if not middle_mode: + ret += mcgen(''' + error_propagate(errp, local_err); +''') ret += mcgen(''' %(visitor_input_block_cleanup)s ''', diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 1ef753a..06a79f1 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -2,16 +2,19 @@ # QAPI visitor generator # # Copyright IBM, Corp. 2011 +# Copyright (C) 2014 Red Hat, Inc. # # Authors: # Anthony Liguori # Michael Roth +# Markus Armbruster # # This work is licensed under the terms of the GNU GPL, version 2. # See the COPYING file in the top-level directory. from ordereddict import OrderedDict from qapi import * +import re import sys import os import getopt @@ -32,9 +35,7 @@ static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error * visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), &err); if (!err) { - visit_type_%(c_type)s_fields(m, obj, &err); - error_propagate(errp, err); - err = NULL; + visit_type_%(c_type)s_fields(m, obj, errp); visit_end_implicit_struct(m, &err); } error_propagate(errp, err); @@ -64,12 +65,9 @@ def generate_visit_struct_fields(name, field_prefix, fn_prefix, members, base = static void visit_type_%(full_name)s_field_%(c_name)s(Visitor *m, %(name)s **obj, Error **errp) { - Error *err = NULL; ''', name=name, full_name=full_name, c_name=c_var(argname)) - push_indent() ret += generate_visit_struct_body(full_name, argname, argentry) - pop_indent() ret += mcgen(''' } ''') @@ -89,6 +87,9 @@ static void visit_type_%(full_name)s_fields(Visitor *m, %(name)s ** obj, Error * if base: ret += mcgen(''' visit_type_implicit_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, &err); +if (err) { + goto out; +} ''', c_prefix=c_var(field_prefix), type=type_name(base), c_name=c_var('base')) @@ -97,7 +98,7 @@ visit_type_implicit_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, &err); if optional: ret += mcgen(''' visit_optional(m, &(*obj)->%(c_prefix)shas_%(c_name)s, "%(name)s", &err); -if ((*obj)->%(prefix)shas_%(c_name)s) { +if (!err && (*obj)->%(prefix)shas_%(c_name)s) { ''', c_prefix=c_var(field_prefix), prefix=field_prefix, c_name=c_var(argname), name=argname) @@ -121,10 +122,19 @@ visit_type_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, "%(name)s", &err); ret += mcgen(''' } ''') + ret += mcgen(''' +if (err) { + goto out; +} +''') pop_indent() - ret += mcgen(''' + if re.search('^ *goto out\\;', ret, re.MULTILINE): + ret += mcgen(''' +out: +''') + ret += mcgen(''' error_propagate(errp, err); } ''') @@ -133,9 +143,9 @@ visit_type_%(type)s(m, &(*obj)->%(c_prefix)s%(c_name)s, "%(name)s", &err); def generate_visit_struct_body(field_prefix, name, members): ret = mcgen(''' -if (!error_is_set(errp)) { + Error *err = NULL; + ''') - push_indent() if not field_prefix: full_name = name @@ -144,36 +154,26 @@ if (!error_is_set(errp)) { if len(field_prefix): ret += mcgen(''' -visit_start_struct(m, NULL, "", "%(name)s", 0, &err); + visit_start_struct(m, NULL, "", "%(name)s", 0, &err); ''', name=name) else: ret += mcgen(''' -Error *err = NULL; -visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err); + visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err); ''', name=name) ret += mcgen(''' -if (!err) { - if (*obj) { - visit_type_%(name)s_fields(m, obj, &err); - error_propagate(errp, err); - err = NULL; + if (!err) { + if (*obj) { + visit_type_%(name)s_fields(m, obj, errp); + } + visit_end_struct(m, &err); } + error_propagate(errp, err); ''', name=full_name) - ret += mcgen(''' - /* Always call end_struct if start_struct succeeded. */ - visit_end_struct(m, &err); -} -error_propagate(errp, err); -''') - pop_indent() - ret += mcgen(''' -} -''') return ret def generate_visit_struct(expr): @@ -191,9 +191,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error ** ''', name=name) - push_indent() ret += generate_visit_struct_body("", name, members) - pop_indent() ret += mcgen(''' } @@ -205,24 +203,26 @@ def generate_visit_list(name, members): void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp) { - GenericList *i, **prev = (GenericList **)obj; Error *err = NULL; + GenericList *i, **prev; - if (!error_is_set(errp)) { - visit_start_list(m, name, &err); - if (!err) { - for (; (i = visit_next_list(m, prev, &err)) != NULL; prev = &i) { - %(name)sList *native_i = (%(name)sList *)i; - visit_type_%(name)s(m, &native_i->value, NULL, &err); - } - error_propagate(errp, err); - err = NULL; - - /* Always call end_list if start_list succeeded. */ - visit_end_list(m, &err); - } - error_propagate(errp, err); + visit_start_list(m, name, &err); + if (err) { + goto out; + } + + for (prev = (GenericList **)obj; + !err && (i = visit_next_list(m, prev, &err)) != NULL; + prev = &i) { + %(name)sList *native_i = (%(name)sList *)i; + visit_type_%(name)s(m, &native_i->value, NULL, &err); } + + error_propagate(errp, err); + err = NULL; + visit_end_list(m, &err); +out: + error_propagate(errp, err); } ''', name=name) @@ -244,10 +244,15 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error ** { Error *err = NULL; - if (!error_is_set(errp)) { - visit_start_implicit_struct(m, (void**) obj, sizeof(%(name)s), &err); - visit_get_next_type(m, (int*) &(*obj)->kind, %(name)s_qtypes, name, &err); - switch ((*obj)->kind) { + visit_start_implicit_struct(m, (void**) obj, sizeof(%(name)s), &err); + if (err) { + goto out; + } + visit_get_next_type(m, (int*) &(*obj)->kind, %(name)s_qtypes, name, &err); + if (err) { + goto out_end; + } + switch ((*obj)->kind) { ''', name=name) @@ -262,22 +267,24 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error ** enum_full_value = generate_enum_full_value(disc_type, key) ret += mcgen(''' - case %(enum_full_value)s: - visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err); - break; + case %(enum_full_value)s: + visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err); + break; ''', enum_full_value = enum_full_value, c_type = type_name(members[key]), c_name = c_fun(key)) ret += mcgen(''' - default: - abort(); - } - error_propagate(errp, err); - err = NULL; - visit_end_implicit_struct(m, &err); + default: + abort(); } +out_end: + error_propagate(errp, err); + err = NULL; + visit_end_implicit_struct(m, &err); +out: + error_propagate(errp, err); } ''') @@ -324,19 +331,20 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error ** { Error *err = NULL; - if (!error_is_set(errp)) { - visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err); - if (!err) { - if (*obj) { + visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err); + if (err) { + goto out; + } + if (*obj) { ''', name=name) - push_indent() - push_indent() - if base: ret += mcgen(''' visit_type_%(name)s_fields(m, obj, &err); + if (err) { + goto out_obj; + } ''', name=name) @@ -346,8 +354,10 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error ** disc_key = discriminator ret += mcgen(''' visit_type_%(disc_type)s(m, &(*obj)->kind, "%(disc_key)s", &err); - if (!err) { - switch ((*obj)->kind) { + if (err) { + goto out_obj; + } + switch ((*obj)->kind) { ''', disc_type = disc_type, disc_key = disc_key) @@ -360,32 +370,25 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error ** enum_full_value = generate_enum_full_value(disc_type, key) ret += mcgen(''' - case %(enum_full_value)s: - ''' + fmt + ''' - break; + case %(enum_full_value)s: + ''' + fmt + ''' + break; ''', enum_full_value = enum_full_value, c_type=type_name(members[key]), c_name=c_fun(key)) ret += mcgen(''' - default: - abort(); - } + default: + abort(); } +out_obj: error_propagate(errp, err); err = NULL; -''') - pop_indent() - pop_indent() - - ret += mcgen(''' - } - /* Always call end_struct if start_struct succeeded. */ - visit_end_struct(m, &err); - } - error_propagate(errp, err); } + visit_end_struct(m, &err); +out: + error_propagate(errp, err); } ''') @@ -505,7 +508,7 @@ fdecl.write(mcgen(''' /* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */ /* - * schema-defined QAPI visitor function + * schema-defined QAPI visitor functions * * Copyright IBM, Corp. 2011 * -- cgit v1.1 From 24fd848950b44de7e2d71fb69ba52b90d6acb220 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Canet?= Date: Fri, 16 May 2014 12:51:56 +0200 Subject: qapi: skip redundant includes The purpose of this change is to help create a json file containing common definitions; each bit of generated C code must be emitted only one time. A second history global to all QAPISchema instances has been added to detect when a file is included more than one time and skip these includes. It does not act as a stack and the changes made to it by the __init__ function are propagated back to the caller so it's really a global state. Signed-off-by: Benoit Canet Reviewed-by: Eric Blake Signed-off-by: Luiz Capitulino --- scripts/qapi.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi.py b/scripts/qapi.py index ec806aa..0265b40 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -73,13 +73,18 @@ class QAPIExprError(Exception): class QAPISchema: - def __init__(self, fp, input_relname=None, include_hist=[], parent_info=None): + def __init__(self, fp, input_relname=None, include_hist=[], + previously_included=[], parent_info=None): + """ include_hist is a stack used to detect inclusion cycles + previously_included is a global state used to avoid multiple + inclusions of the same file""" input_fname = os.path.abspath(fp.name) if input_relname is None: input_relname = fp.name self.input_dir = os.path.dirname(input_fname) self.input_file = input_relname self.include_hist = include_hist + [(input_relname, input_fname)] + previously_included.append(input_fname) self.parent_info = parent_info self.src = fp.read() if self.src == '' or self.src[-1] != '\n': @@ -106,13 +111,16 @@ class QAPISchema: for elem in self.include_hist): raise QAPIExprError(expr_info, "Inclusion loop for %s" % include) + # skip multiple include of the same file + if include_path in previously_included: + continue try: fobj = open(include_path, 'r') except IOError as e: raise QAPIExprError(expr_info, '%s: %s' % (e.strerror, include)) - exprs_include = QAPISchema(fobj, include, - self.include_hist, expr_info) + exprs_include = QAPISchema(fobj, include, self.include_hist, + previously_included, expr_info) self.exprs.extend(exprs_include.exprs) else: expr_elem = {'expr': expr, -- cgit v1.1