From 29f6bd15eb8a55ed37b2a443f7275b3d134eb2b2 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 17 Mar 2016 16:48:26 -0600 Subject: qapi: Assert in places where variants are not handled We are getting closer to the point where we could use one union as the base or variant type within another union type (as long as there are no collisions between any possible combination of member names allowed across all discriminator choices). But until we get to that point, it is worth asserting that variants are not present in places where we are not prepared to handle them: when exploding a type into a parameter list, we do not expect variants. The qapi.py code is already checking this, via the older check_type() method; but someday we hope to get rid of that and move checking into QAPISchema*.check(). The two asserts added here make sure any refactoring still catches problems, and makes it locally obvious why we can iterate over only type.members without worrying about type.variants. Signed-off-by: Eric Blake Message-Id: <1458254921-17042-2-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-commands.py | 3 ++- scripts/qapi-event.py | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) (limited to 'scripts') diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index f44e01f..edcbd10 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -2,7 +2,7 @@ # QAPI command marshaller generator # # Copyright IBM, Corp. 2011 -# Copyright (C) 2014-2015 Red Hat, Inc. +# Copyright (C) 2014-2016 Red Hat, Inc. # # Authors: # Anthony Liguori @@ -30,6 +30,7 @@ def gen_call(name, arg_type, ret_type): argstr = '' if arg_type: + assert not arg_type.variants for memb in arg_type.members: if memb.optional: argstr += 'has_%s, ' % c_name(memb.name) diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py index fb579dd..c03cb78 100644 --- a/scripts/qapi-event.py +++ b/scripts/qapi-event.py @@ -59,6 +59,7 @@ def gen_event_send(name, arg_type): name=name) if arg_type and arg_type.members: + assert not arg_type.variants ret += mcgen(''' qov = qmp_output_visitor_new(); v = qmp_output_get_visitor(qov); -- cgit v1.1 From 972a110162677fe5155f68a718ec6e999cd059a7 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 17 Mar 2016 16:48:27 -0600 Subject: qapi: Fix command with named empty argument type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The generator special-cased { 'command':'foo', 'data': {} } to avoid emitting a visitor variable, but failed to see that { 'struct':'NamedEmptyType, 'data': {} } { 'command':'foo', 'data':'NamedEmptyType' } needs the same treatment. There, the generator happily generates a visitor to get no arguments, and a visitor to destroy no arguments; and the compiler isn't happy with that, as demonstrated by the updated qapi-schema-test.json: tests/test-qmp-marshal.c: In function ‘qmp_marshal_user_def_cmd0’: tests/test-qmp-marshal.c:264:14: error: variable ‘v’ set but not used [-Werror=unused-but-set-variable] Visitor *v; ^ No change to generated code except for the testsuite addition. Signed-off-by: Eric Blake Message-Id: <1458254921-17042-3-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-commands.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index edcbd10..3784f33 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -66,7 +66,7 @@ def gen_marshal_vars(arg_type, ret_type): ''', c_type=ret_type.c_type()) - if arg_type: + if arg_type and arg_type.members: ret += mcgen(''' QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args)); QapiDeallocVisitor *qdv; @@ -98,7 +98,7 @@ def gen_marshal_vars(arg_type, ret_type): def gen_marshal_input_visit(arg_type, dealloc=False): ret = '' - if not arg_type: + if not arg_type or not arg_type.members: return ret if dealloc: -- cgit v1.1 From 4040d995e49c5b818be79e50a18c1bf8d2354d12 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 17 Mar 2016 16:48:28 -0600 Subject: qapi: Make c_type() more OO-like QAPISchemaType.c_type() is a bit awkward: it takes two optional boolean flags is_param and is_unboxed, and they should never both be True. Add a new method for each of the flags, and drop the flags from c_type(). Most callers pass no flags; they remain unchanged. One caller passes is_param=True; call the new .c_param_type() instead. One caller passes is_unboxed=True, except for simple union types. This is actually an ugly special case that will go away soon, so until then, we now have to call either .c_type() or the new .c_unboxed_type(). Tolerable in the interim. It requires slightly more Python, but is arguably easier to read. Suggested-by: Markus Armbruster Signed-off-by: Eric Blake Message-Id: <1458254921-17042-4-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-types.py | 7 +++++-- scripts/qapi.py | 39 ++++++++++++++++++++++++++++++--------- 2 files changed, 35 insertions(+), 11 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 0306a88..f194bea 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -124,11 +124,14 @@ def gen_variants(variants): for var in variants.variants: # Ugly special case for simple union TODO get rid of it simple_union_type = var.simple_union_type() - typ = simple_union_type or var.type + if simple_union_type: + typ = simple_union_type.c_type() + else: + typ = var.type.c_unboxed_type() ret += mcgen(''' %(c_type)s %(c_name)s; ''', - c_type=typ.c_type(is_unboxed=not simple_union_type), + c_type=typ, c_name=c_name(var.name)) ret += mcgen(''' diff --git a/scripts/qapi.py b/scripts/qapi.py index 6b2aa6e..b7fbdae 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -822,8 +822,18 @@ class QAPISchemaVisitor(object): class QAPISchemaType(QAPISchemaEntity): - def c_type(self, is_param=False, is_unboxed=False): - return c_name(self.name) + pointer_suffix + # Return the C type for common use. + # For the types we commonly box, this is a pointer type. + def c_type(self): + pass + + # Return the C type to be used in a parameter list. + def c_param_type(self): + return self.c_type() + + # Return the C type to be used where we suppress boxing. + def c_unboxed_type(self): + return self.c_type() def c_null(self): return 'NULL' @@ -855,8 +865,11 @@ class QAPISchemaBuiltinType(QAPISchemaType): def c_name(self): return self.name - def c_type(self, is_param=False, is_unboxed=False): - if is_param and self.name == 'str': + def c_type(self): + return self._c_type_name + + def c_param_type(self): + if self.name == 'str': return 'const ' + self._c_type_name return self._c_type_name @@ -889,7 +902,7 @@ class QAPISchemaEnumType(QAPISchemaType): # See QAPISchema._make_implicit_enum_type() return self.name.endswith('Kind') - def c_type(self, is_param=False, is_unboxed=False): + def c_type(self): return c_name(self.name) def member_names(self): @@ -921,6 +934,9 @@ class QAPISchemaArrayType(QAPISchemaType): def is_implicit(self): return True + def c_type(self): + return c_name(self.name) + pointer_suffix + def json_type(self): return 'array' @@ -985,12 +1001,14 @@ class QAPISchemaObjectType(QAPISchemaType): assert not self.is_implicit() return QAPISchemaType.c_name(self) - def c_type(self, is_param=False, is_unboxed=False): + def c_type(self): assert not self.is_implicit() - if is_unboxed: - return c_name(self.name) return c_name(self.name) + pointer_suffix + def c_unboxed_type(self): + assert not self.is_implicit() + return c_name(self.name) + def json_type(self): return 'object' @@ -1139,6 +1157,9 @@ class QAPISchemaAlternateType(QAPISchemaType): for v in self.variants.variants: v.check_clash(self.info, seen) + def c_type(self): + return c_name(self.name) + pointer_suffix + def json_type(self): return 'value' @@ -1630,7 +1651,7 @@ def gen_params(arg_type, extra): sep = ', ' if memb.optional: ret += 'bool has_%s, ' % c_name(memb.name) - ret += '%s %s' % (memb.type.c_type(is_param=True), c_name(memb.name)) + ret += '%s %s' % (memb.type.c_param_type(), c_name(memb.name)) if extra: ret += sep + extra return ret -- cgit v1.1 From 7599697c66d22ff4c859ba6ccea30e6a9aae6b9b Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 17 Mar 2016 16:48:29 -0600 Subject: qapi: Adjust names of implicit types The original choice of ':obj-' as the prefix for implicit types made it obvious that we weren't going to clash with any user-defined names, which cannot contain ':'. But now we want to create structs for implicit types, to get rid of special cases in the generators, and our use of ':' in implicit names needs a tweak to produce valid C code. We could transliterate ':' to '_', except that C99 mandates that "identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces". So it's time to change our naming convention: we can instead use the 'q_' prefix that we reserved for ourselves back in commit 9fb081e0. Technically, since we aren't planning on exposing the empty type in generated code, we could keep the name ':empty', but renaming it to 'q_empty' makes the check for startswith('q_') cover all implicit types, whether or not code is generated for them. As long as we don't declare 'empty' or 'obj' ticklish, it shouldn't clash with c_name() prepending 'q_' to the user's ticklish names. Signed-off-by: Eric Blake Message-Id: <1458254921-17042-5-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi.py b/scripts/qapi.py index b7fbdae..f6701f5 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -391,7 +391,8 @@ def check_name(expr_info, source, name, allow_optional=False, # code always prefixes it with the enum name if enum_member and membername[0].isdigit(): membername = 'D' + membername - # Reserve the entire 'q_' namespace for c_name() + # Reserve the entire 'q_' namespace for c_name(), and for 'q_empty' + # and 'q_obj_*' implicit type names. if not valid_name.match(membername) or \ c_name(membername, False).startswith('q_'): raise QAPIExprError(expr_info, @@ -994,8 +995,9 @@ class QAPISchemaObjectType(QAPISchemaType): m.check_clash(info, seen) def is_implicit(self): - # See QAPISchema._make_implicit_object_type() - return self.name[0] == ':' + # See QAPISchema._make_implicit_object_type(), as well as + # _def_predefineds() + return self.name.startswith('q_') def c_name(self): assert not self.is_implicit() @@ -1044,10 +1046,10 @@ class QAPISchemaMember(object): def _pretty_owner(self): owner = self.owner - if owner.startswith(':obj-'): + if owner.startswith('q_obj_'): # See QAPISchema._make_implicit_object_type() - reverse the # mapping there to create a nice human-readable description - owner = owner[5:] + owner = owner[6:] if owner.endswith('-arg'): return '(parameter of %s)' % owner[:-4] else: @@ -1266,8 +1268,8 @@ class QAPISchema(object): ('bool', 'boolean', 'bool', 'false'), ('any', 'value', 'QObject' + pointer_suffix, 'NULL')]: self._def_builtin_type(*t) - self.the_empty_object_type = QAPISchemaObjectType(':empty', None, None, - [], None) + self.the_empty_object_type = QAPISchemaObjectType('q_empty', None, + None, [], None) self._def_entity(self.the_empty_object_type) qtype_values = self._make_enum_members(['none', 'qnull', 'qint', 'qstring', 'qdict', 'qlist', @@ -1295,7 +1297,7 @@ class QAPISchema(object): if not members: return None # See also QAPISchemaObjectTypeMember._pretty_owner() - name = ':obj-%s-%s' % (name, role) + name = 'q_obj_%s-%s' % (name, role) if not self.lookup_entity(name, QAPISchemaObjectType): self._def_entity(QAPISchemaObjectType(name, info, None, members, None)) -- cgit v1.1 From 7ce106a96feee4d46bfcdb47127b0935804c9357 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 17 Mar 2016 16:48:30 -0600 Subject: qapi: Emit implicit structs in generated C We already have several places that want to visit all the members of an implicit object within a larger context (simple union variant, event with anonymous data, command with anonymous arguments struct); and will be adding another one soon (the ability to declare an anonymous base for a flat union). Having a C struct declared for these implicit types, along with a visit_type_FOO_members() helper function, will make for fewer special cases in our generator. We do not, however, need qapi_free_FOO() or visit_type_FOO() functions for implicit types, because they should not be used directly outside of the generated code. This is done by adding a conditional in visit_object_type() for both qapi-types.py and qapi-visit.py based on the object name. The comparison of "name.startswith('q_')" is a bit hacky (it's basically duplicating what .is_implicit() already uses), but beats changing the signature of the visit_object_type() callback to pass a new 'implicit' flag. The hack should be temporary: we are considering adding a future patch that consolidates the narrow visit_object_type(..., base, local_members, variants) and visit_object_type_flat(..., all_members, variants) [where different sets of information are already broken out, and the QAPISchemaObjectType is no longer available] into a broader visit_object_type(obj_type) [where the visitor can query the needed fields from obj_type directly]. Also, now that we WANT to output C code for implicits, we no longer need the visit_needed() filter, leaving 'q_empty' as the only object still needing a special case. Remember, 'q_empty' is the only built-in generated object, which means that without a special case it would be emitted in multiple files (the main qapi-types.h and in qga-qapi-types.h) causing compilation failure due to redefinition. But since it has no members, it's easier to just avoid an attempt to visit that particular type; since gen_object() is called recursively, we also prime the objects_seen set to cover any recursion into the empty type. The patch relies on the changed naming of implicit types in the previous patch. It is a bit unfortunate that the generated struct names and visit_type_FOO_members() don't match normal naming conventions, but it's not too bad, since they will only be used in generated code. The generated code grows substantially in size: the implicit '-wrapper' types must be emitted in qapi-types.h before any union can include an unboxed member of that type. Arguably, the '-args' types could be emitted in a private header for just qapi-visit.c and qmp-marshal.c, rather than polluting qapi-types.h; but adding complexity to the generator to split the output location according to role doesn't seem worth the maintenance costs. Signed-off-by: Eric Blake Message-Id: <1458254921-17042-6-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-types.py | 19 +++++++++++-------- scripts/qapi-visit.py | 23 +++++++++++------------ scripts/qapi.py | 2 -- 3 files changed, 22 insertions(+), 22 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index f194bea..79416b2 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -61,8 +61,7 @@ def gen_object(name, base, members, variants): ret = '' if variants: for v in variants.variants: - if (isinstance(v.type, QAPISchemaObjectType) and - not v.type.is_implicit()): + if isinstance(v.type, QAPISchemaObjectType): ret += gen_object(v.type.name, v.type.base, v.type.local_members, v.type.variants) @@ -180,6 +179,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): self._btin = None def visit_begin(self, schema): + # gen_object() is recursive, ensure it doesn't visit the empty type + objects_seen.add(schema.the_empty_object_type.name) self.decl = '' self.defn = '' self._fwdecl = '' @@ -196,11 +197,6 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): self.decl = self._btin + self.decl self._btin = None - def visit_needed(self, entity): - # Visit everything except implicit objects - return not (entity.is_implicit() and - isinstance(entity, QAPISchemaObjectType)) - def _gen_type_cleanup(self, name): self.decl += gen_type_cleanup_decl(name) self.defn += gen_type_cleanup(name) @@ -229,11 +225,18 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): self._gen_type_cleanup(name) def visit_object_type(self, name, info, base, members, variants): + # Nothing to do for the special empty builtin + if name == 'q_empty': + return self._fwdecl += gen_fwd_object_or_array(name) self.decl += gen_object(name, base, members, variants) if base: self.decl += gen_upcast(name, base) - self._gen_type_cleanup(name) + # TODO Worth changing the visitor signature, so we could + # directly use rather than repeat type.is_implicit()? + if not name.startswith('q_'): + # implicit types won't be directly allocated/freed + self._gen_type_cleanup(name) def visit_alternate_type(self, name, info, variants): self._fwdecl += gen_fwd_object_or_array(name) diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index a712e9a..4923b2e 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -215,13 +215,11 @@ out: def gen_visit_object(name, base, members, variants): - ret = gen_visit_object_members(name, base, members, variants) - # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to # *obj, but then visit_type_FOO_members() fails, we should clean up *obj # rather than leaving it non-NULL. As currently written, the caller must # call qapi_free_FOO() to avoid a memory leak of the partial FOO. - ret += mcgen(''' + return mcgen(''' void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error **errp) { @@ -245,8 +243,6 @@ out: ''', c_name=c_name(name)) - return ret - class QAPISchemaGenVisitVisitor(QAPISchemaVisitor): def __init__(self): @@ -268,11 +264,6 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor): self.decl = self._btin + self.decl self._btin = None - def visit_needed(self, entity): - # Visit everything except implicit objects - return not (entity.is_implicit() and - isinstance(entity, QAPISchemaObjectType)) - def visit_enum_type(self, name, info, values, prefix): # Special case for our lone builtin enum type # TODO use something cleaner than existence of info @@ -296,9 +287,17 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor): self.defn += defn def visit_object_type(self, name, info, base, members, variants): + # Nothing to do for the special empty builtin + if name == 'q_empty': + return self.decl += gen_visit_members_decl(name) - self.decl += gen_visit_decl(name) - self.defn += gen_visit_object(name, base, members, variants) + self.defn += gen_visit_object_members(name, base, members, variants) + # TODO Worth changing the visitor signature, so we could + # directly use rather than repeat type.is_implicit()? + if not name.startswith('q_'): + # only explicit types need an allocating visit + self.decl += gen_visit_decl(name) + self.defn += gen_visit_object(name, base, members, variants) def visit_alternate_type(self, name, info, variants): self.decl += gen_visit_decl(name) diff --git a/scripts/qapi.py b/scripts/qapi.py index f6701f5..96fb216 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -1000,7 +1000,6 @@ class QAPISchemaObjectType(QAPISchemaType): return self.name.startswith('q_') def c_name(self): - assert not self.is_implicit() return QAPISchemaType.c_name(self) def c_type(self): @@ -1008,7 +1007,6 @@ class QAPISchemaObjectType(QAPISchemaType): return c_name(self.name) + pointer_suffix def c_unboxed_type(self): - assert not self.is_implicit() return c_name(self.name) def json_type(self): -- cgit v1.1 From 8df59565d2c27dec8c96a2090f0eb73303efce14 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 17 Mar 2016 16:48:31 -0600 Subject: qapi-event: Drop qmp_output_get_qobject() null check qmp_output_get_qobject() was changed never to return null some time ago (in commit 6c2f9a15), but the qapi_event_send_FOO() functions still check. Clean that up: |@@ -28,7 +28,6 @@ void qapi_event_send_acpi_device_ost(ACP | QMPEventFuncEmit emit; | QmpOutputVisitor *qov; | Visitor *v; |- QObject *obj; | | emit = qmp_event_get_func_emit(); | if (!emit) { |@@ -54,10 +53,7 @@ out_obj: | goto out; | } | |- obj = qmp_output_get_qobject(qov); |- g_assert(obj); |- |- qdict_put_obj(qmp, "data", obj); |+ qdict_put_obj(qmp, "data", qmp_output_get_qobject(qov)); | emit(QAPI_EVENT_ACPI_DEVICE_OST, qmp, &err); | | out: Signed-off-by: Eric Blake Message-Id: <1458254921-17042-7-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-event.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py index c03cb78..27af206 100644 --- a/scripts/qapi-event.py +++ b/scripts/qapi-event.py @@ -43,7 +43,6 @@ def gen_event_send(name, arg_type): ret += mcgen(''' QmpOutputVisitor *qov; Visitor *v; - QObject *obj; ''') @@ -77,10 +76,7 @@ out_obj: goto out; } - obj = qmp_output_get_qobject(qov); - g_assert(obj); - - qdict_put_obj(qmp, "data", obj); + qdict_put_obj(qmp, "data", qmp_output_get_qobject(qov)); ''') ret += mcgen(''' -- cgit v1.1 From 0949e95b48e30715e157cabbc59dcb0ed912d3ff Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 17 Mar 2016 16:48:32 -0600 Subject: qapi-event: Utilize implicit struct visits Rather than generate inline per-member visits, take advantage of the 'visit_type_FOO_members()' function for emitting events. This is possible now that implicit structs can be visited like any other. Generated code shrinks accordingly; by initializing a struct based on parameters, through a new gen_param_var() helper, like: |@@ -338,6 +250,9 @@ void qapi_event_send_block_job_error(con | QMPEventFuncEmit emit = qmp_event_get_func_emit(); | QmpOutputVisitor *qov; | Visitor *v; |+ q_obj_BLOCK_JOB_ERROR_arg param = { |+ (char *)device, operation, action |+ }; | | if (!emit) { | return; @@ -351,19 +266,7 @@ void qapi_event_send_block_job_error(con | if (err) { | goto out; | } |- visit_type_str(v, "device", (char **)&device, &err); |- if (err) { |- goto out_obj; |- } |- visit_type_IoOperationType(v, "operation", &operation, &err); |- if (err) { |- goto out_obj; |- } |- visit_type_BlockErrorAction(v, "action", &action, &err); |- if (err) { |- goto out_obj; |- } |-out_obj: |+ visit_type_q_obj_BLOCK_JOB_ERROR_arg_members(v, ¶m, &err); | visit_end_struct(v, err ? NULL : &err); Notice that the initialization of 'param' has to cast away const (just as the old gen_visit_members() had to do): we can't change the signature of the user function (which uses 'const char *'), but have to assign it to a non-const QAPI object (which requires 'char *'). While touching this, document with a FIXME comment that there is still a potential collision between QMP members and our choice of local variable names within qapi_event_send_FOO(). This patch also paves the way for some followup simplifications in the generator, in subsequent patches. Signed-off-by: Eric Blake Message-Id: <1458254921-17042-8-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-event.py | 48 ++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 10 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py index 27af206..9b5c5b5 100644 --- a/scripts/qapi-event.py +++ b/scripts/qapi-event.py @@ -28,7 +28,37 @@ def gen_event_send_decl(name, arg_type): proto=gen_event_send_proto(name, arg_type)) +# Declare and initialize an object 'qapi' using parameters from gen_params() +def gen_param_var(typ): + assert not typ.variants + ret = mcgen(''' + %(c_name)s param = { +''', + c_name=typ.c_name()) + sep = ' ' + for memb in typ.members: + ret += sep + sep = ', ' + if memb.optional: + ret += 'has_' + c_name(memb.name) + sep + if memb.type.name == 'str': + # Cast away const added in gen_params() + ret += '(char *)' + ret += c_name(memb.name) + ret += mcgen(''' + + }; +''') + return ret + + def gen_event_send(name, arg_type): + # FIXME: Our declaration of local variables (and of 'errp' in the + # parameter list) can collide with exploded members of the event's + # data type passed in as parameters. If this collision ever hits in + # practice, we can rename our local variables with a leading _ prefix, + # or split the code into a wrapper function that creates a boxed + # 'param' object then calls another to do the real work. ret = mcgen(''' %(proto)s @@ -43,10 +73,11 @@ def gen_event_send(name, arg_type): ret += mcgen(''' QmpOutputVisitor *qov; Visitor *v; - ''') + ret += gen_param_var(arg_type) ret += mcgen(''' + emit = qmp_event_get_func_emit(); if (!emit) { return; @@ -58,26 +89,23 @@ def gen_event_send(name, arg_type): name=name) if arg_type and arg_type.members: - assert not arg_type.variants ret += mcgen(''' qov = qmp_output_visitor_new(); v = qmp_output_get_visitor(qov); visit_start_struct(v, "%(name)s", NULL, 0, &err); -''', - name=name) - ret += gen_err_check() - ret += gen_visit_members(arg_type.members, need_cast=True, - label='out_obj') - ret += mcgen(''' -out_obj: + if (err) { + goto out; + } + visit_type_%(c_name)s_members(v, ¶m, &err); visit_end_struct(v, err ? NULL : &err); if (err) { goto out; } qdict_put_obj(qmp, "data", qmp_output_get_qobject(qov)); -''') +''', + name=name, c_name=arg_type.c_name()) ret += mcgen(''' emit(%(c_enum)s, qmp, &err); -- cgit v1.1 From 386230a249ca6ed0204d8616dfc30c5ea95a809c Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 17 Mar 2016 16:48:33 -0600 Subject: qapi-commands: Utilize implicit struct visits Rather than generate inline per-member visits, take advantage of the 'visit_type_FOO_members()' function for command marshalling. This is possible now that implicit structs can be visited like any other. Generate call arguments from a stack- allocated struct, rather than a list of local variables: |@@ -57,26 +57,15 @@ void qmp_marshal_add_fd(QDict *args, QOb | QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args)); | QapiDeallocVisitor *qdv; | Visitor *v; |- bool has_fdset_id = false; |- int64_t fdset_id = 0; |- bool has_opaque = false; |- char *opaque = NULL; |+ q_obj_add_fd_arg arg = {0}; | | v = qmp_input_get_visitor(qiv); |- if (visit_optional(v, "fdset-id", &has_fdset_id)) { |- visit_type_int(v, "fdset-id", &fdset_id, &err); |- if (err) { |- goto out; |- } |- } |- if (visit_optional(v, "opaque", &has_opaque)) { |- visit_type_str(v, "opaque", &opaque, &err); |- if (err) { |- goto out; |- } |+ visit_type_q_obj_add_fd_arg_members(v, &arg, &err); |+ if (err) { |+ goto out; | } | |- retval = qmp_add_fd(has_fdset_id, fdset_id, has_opaque, opaque, &err); |+ retval = qmp_add_fd(arg.has_fdset_id, arg.fdset_id, arg.has_opaque, arg.opaque, &err); | if (err) { | goto out; | } |@@ -88,12 +77,7 @@ out: | qmp_input_visitor_cleanup(qiv); | qdv = qapi_dealloc_visitor_new(); | v = qapi_dealloc_get_visitor(qdv); |- if (visit_optional(v, "fdset-id", &has_fdset_id)) { |- visit_type_int(v, "fdset-id", &fdset_id, NULL); |- } |- if (visit_optional(v, "opaque", &has_opaque)) { |- visit_type_str(v, "opaque", &opaque, NULL); |- } |+ visit_type_q_obj_add_fd_arg_members(v, &arg, NULL); | qapi_dealloc_visitor_cleanup(qdv); | } This also has the nice side effect of eliminating a chance of collision between argument QMP names and local variables. This patch also paves the way for some followup simplifications in the generator, in subsequent patches. Signed-off-by: Eric Blake Message-Id: <1458254921-17042-9-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-commands.py | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index 3784f33..28b9cbe 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -33,8 +33,8 @@ def gen_call(name, arg_type, ret_type): assert not arg_type.variants for memb in arg_type.members: if memb.optional: - argstr += 'has_%s, ' % c_name(memb.name) - argstr += '%s, ' % c_name(memb.name) + argstr += 'arg.has_%s, ' % c_name(memb.name) + argstr += 'arg.%s, ' % c_name(memb.name) lhs = '' if ret_type: @@ -71,21 +71,10 @@ def gen_marshal_vars(arg_type, ret_type): QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args)); QapiDeallocVisitor *qdv; Visitor *v; -''') + %(c_name)s arg = {0}; - for memb in arg_type.members: - if memb.optional: - ret += mcgen(''' - bool has_%(c_name)s = false; -''', - c_name=c_name(memb.name)) - ret += mcgen(''' - %(c_type)s %(c_name)s = %(c_null)s; ''', - c_name=c_name(memb.name), - c_type=memb.type.c_type(), - c_null=memb.type.c_null()) - ret += '\n' + c_name=arg_type.c_name()) else: ret += mcgen(''' @@ -107,17 +96,24 @@ def gen_marshal_input_visit(arg_type, dealloc=False): qdv = qapi_dealloc_visitor_new(); v = qapi_dealloc_get_visitor(qdv); ''') + errp = 'NULL' else: ret += mcgen(''' v = qmp_input_get_visitor(qiv); ''') + errp = '&err' - ret += gen_visit_members(arg_type.members, skiperr=dealloc) + ret += mcgen(''' + visit_type_%(c_name)s_members(v, &arg, %(errp)s); +''', + c_name=arg_type.c_name(), errp=errp) if dealloc: ret += mcgen(''' qapi_dealloc_visitor_cleanup(qdv); ''') + else: + ret += gen_err_check() return ret -- cgit v1.1 From c1ff0e6c853111496a3c5ae392b9adae5043c7be Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 17 Mar 2016 16:48:34 -0600 Subject: qapi-commands: Inline single-use helpers of gen_marshal() Originally, gen_marshal_input_visit() (or gen_visitor_input_block() before commit f1538019) was factored out to make it easy to do two passes of a visit to each member of a (possibly-implicit) object, without duplicating lots of code. But after recent changes, those visits now occupy a single line of emitted code, and the helper method has become a series of conditionals both before and after the one important line, making it rather awkward to see at a glance what gets emitted on the first (parsing) or second (deallocation) pass. It's a lot easier to read the generator code if we just inline both uses directly into gen_marshal(), without all the conditionals. Once we've done that, it's easy to notice that gen_marshal_vars() is used only once, and inlining it too lets us consolidate some mcgen() calls that used to be split across helpers. gen_call() remains a single-use helper function, but it has enough indentation and complexity that inlining it would hamper legibility. No change to generated output. The fact that the diffstat shows a net reduction in lines is an argument in favor of this cleanup. Signed-off-by: Eric Blake Message-Id: <1458254921-17042-10-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-commands.py | 106 +++++++++++++++++------------------------------ 1 file changed, 39 insertions(+), 67 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py index 28b9cbe..b570069 100644 --- a/scripts/qapi-commands.py +++ b/scripts/qapi-commands.py @@ -55,68 +55,6 @@ def gen_call(name, arg_type, ret_type): return ret -def gen_marshal_vars(arg_type, ret_type): - ret = mcgen(''' - Error *err = NULL; -''') - - if ret_type: - ret += mcgen(''' - %(c_type)s retval; -''', - c_type=ret_type.c_type()) - - if arg_type and arg_type.members: - ret += mcgen(''' - QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args)); - QapiDeallocVisitor *qdv; - Visitor *v; - %(c_name)s arg = {0}; - -''', - c_name=arg_type.c_name()) - else: - ret += mcgen(''' - - (void)args; -''') - - return ret - - -def gen_marshal_input_visit(arg_type, dealloc=False): - ret = '' - - if not arg_type or not arg_type.members: - return ret - - if dealloc: - ret += mcgen(''' - qmp_input_visitor_cleanup(qiv); - qdv = qapi_dealloc_visitor_new(); - v = qapi_dealloc_get_visitor(qdv); -''') - errp = 'NULL' - else: - ret += mcgen(''' - v = qmp_input_get_visitor(qiv); -''') - errp = '&err' - - ret += mcgen(''' - visit_type_%(c_name)s_members(v, &arg, %(errp)s); -''', - c_name=arg_type.c_name(), errp=errp) - - if dealloc: - ret += mcgen(''' - qapi_dealloc_visitor_cleanup(qdv); -''') - else: - ret += gen_err_check() - return ret - - def gen_marshal_output(ret_type): return mcgen(''' @@ -165,15 +103,40 @@ def gen_marshal(name, arg_type, ret_type): %(proto)s { + Error *err = NULL; ''', proto=gen_marshal_proto(name)) - ret += gen_marshal_vars(arg_type, ret_type) - ret += gen_marshal_input_visit(arg_type) + if ret_type: + ret += mcgen(''' + %(c_type)s retval; +''', + c_type=ret_type.c_type()) + + if arg_type and arg_type.members: + ret += mcgen(''' + QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args)); + QapiDeallocVisitor *qdv; + Visitor *v; + %(c_name)s arg = {0}; + + v = qmp_input_get_visitor(qiv); + visit_type_%(c_name)s_members(v, &arg, &err); + if (err) { + goto out; + } +''', + c_name=arg_type.c_name()) + + else: + ret += mcgen(''' + + (void)args; +''') + ret += gen_call(name, arg_type, ret_type) - # 'goto out' produced by gen_marshal_input_visit->gen_visit_members() - # for each arg_type member, and by gen_call() for ret_type + # 'goto out' produced above for arg_type, and by gen_call() for ret_type if (arg_type and arg_type.members) or ret_type: ret += mcgen(''' @@ -182,7 +145,16 @@ out: ret += mcgen(''' error_propagate(errp, err); ''') - ret += gen_marshal_input_visit(arg_type, dealloc=True) + if arg_type and arg_type.members: + ret += mcgen(''' + qmp_input_visitor_cleanup(qiv); + qdv = qapi_dealloc_visitor_new(); + v = qapi_dealloc_get_visitor(qdv); + visit_type_%(c_name)s_members(v, &arg, NULL); + qapi_dealloc_visitor_cleanup(qdv); +''', + c_name=arg_type.c_name()) + ret += mcgen(''' } ''') -- cgit v1.1 From 12f254fd5f98717d17f079c73500123303b232da Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 17 Mar 2016 16:48:35 -0600 Subject: qapi: Inline gen_visit_members() into lone caller Commit 82ca8e46 noticed that we had multiple implementations of visiting every member of a struct, and consolidated it into gen_visit_fields() (now gen_visit_members()) with enough parameters to cater to slight differences between the clients. But recent exposure of implicit types has meant that we are now down to a single use of that method, so we can clean up the unused conditionals and just inline it into the remaining caller: gen_visit_object_members(). Likewise, gen_err_check() no longer needs optional parameters, as the lone use of non-defaults was via gen_visit_members(). No change to generated code. Suggested-by: Markus Armbruster Signed-off-by: Eric Blake Message-Id: <1458254921-17042-11-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-visit.py | 23 ++++++++++++++++++++--- scripts/qapi.py | 46 ++-------------------------------------------- 2 files changed, 22 insertions(+), 47 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 4923b2e..5220bad 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -51,7 +51,24 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) c_type=base.c_name()) ret += gen_err_check() - ret += gen_visit_members(members, prefix='obj->') + for memb in members: + if memb.optional: + ret += mcgen(''' + if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) { +''', + name=memb.name, c_name=c_name(memb.name)) + push_indent() + ret += mcgen(''' + visit_type_%(c_type)s(v, "%(name)s", &obj->%(c_name)s, &err); +''', + c_type=memb.type.c_name(), name=memb.name, + c_name=c_name(memb.name)) + ret += gen_err_check() + if memb.optional: + pop_indent() + ret += mcgen(''' + } +''') if variants: ret += mcgen(''' @@ -90,8 +107,8 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) } ''') - # 'goto out' produced for base, by gen_visit_members() for each member, - # and if variants were present + # 'goto out' produced for base, for each member, and if variants were + # present if base or members or variants: ret += mcgen(''' diff --git a/scripts/qapi.py b/scripts/qapi.py index 96fb216..3b50e4d 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -1657,54 +1657,12 @@ def gen_params(arg_type, extra): return ret -def gen_err_check(label='out', skiperr=False): - if skiperr: - return '' +def gen_err_check(): return mcgen(''' if (err) { - goto %(label)s; - } -''', - label=label) - - -def gen_visit_members(members, prefix='', need_cast=False, skiperr=False, - label='out'): - ret = '' - if skiperr: - errparg = 'NULL' - else: - errparg = '&err' - - for memb in members: - if memb.optional: - ret += mcgen(''' - if (visit_optional(v, "%(name)s", &%(prefix)shas_%(c_name)s)) { -''', - prefix=prefix, c_name=c_name(memb.name), - name=memb.name) - push_indent() - - # Ugly: sometimes we need to cast away const - if need_cast and memb.type.name == 'str': - cast = '(char **)' - else: - cast = '' - - ret += mcgen(''' - visit_type_%(c_type)s(v, "%(name)s", %(cast)s&%(prefix)s%(c_name)s, %(errp)s); -''', - c_type=memb.type.c_name(), prefix=prefix, cast=cast, - c_name=c_name(memb.name), name=memb.name, - errp=errparg) - ret += gen_err_check(skiperr=skiperr, label=label) - - if memb.optional: - pop_indent() - ret += mcgen(''' + goto out; } ''') - return ret # -- cgit v1.1 From 861877a0dd0a8e1bdbcc9743530f4dc9745a736a Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 17 Mar 2016 16:48:36 -0600 Subject: qapi: Drop unused c_null() Now that we are always bulk-initializing a QAPI C struct to 0 (whether by g_malloc0() or by 'Type arg = {0};'), we no longer have any clients of c_null() in the generator for per-element initialization. This patch is easy enough to revert if we find a use in the future, but in the present, get rid of the dead code. Signed-off-by: Eric Blake Message-Id: <1458254921-17042-12-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi.py | 46 +++++++++++++++++----------------------------- 1 file changed, 17 insertions(+), 29 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi.py b/scripts/qapi.py index 3b50e4d..08d63bf 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -836,9 +836,6 @@ class QAPISchemaType(QAPISchemaEntity): def c_unboxed_type(self): return self.c_type() - def c_null(self): - return 'NULL' - def json_type(self): pass @@ -854,14 +851,13 @@ class QAPISchemaType(QAPISchemaEntity): class QAPISchemaBuiltinType(QAPISchemaType): - def __init__(self, name, json_type, c_type, c_null): + def __init__(self, name, json_type, c_type): QAPISchemaType.__init__(self, name, None) assert not c_type or isinstance(c_type, str) assert json_type in ('string', 'number', 'int', 'boolean', 'null', 'value') self._json_type_name = json_type self._c_type_name = c_type - self._c_null_val = c_null def c_name(self): return self.name @@ -874,9 +870,6 @@ class QAPISchemaBuiltinType(QAPISchemaType): return 'const ' + self._c_type_name return self._c_type_name - def c_null(self): - return self._c_null_val - def json_type(self): return self._json_type_name @@ -909,10 +902,6 @@ class QAPISchemaEnumType(QAPISchemaType): def member_names(self): return [v.name for v in self.values] - def c_null(self): - return c_enum_const(self.name, (self.member_names() + ['_MAX'])[0], - self.prefix) - def json_type(self): return 'string' @@ -1240,9 +1229,8 @@ class QAPISchema(object): def lookup_type(self, name): return self.lookup_entity(name, QAPISchemaType) - def _def_builtin_type(self, name, json_type, c_type, c_null): - self._def_entity(QAPISchemaBuiltinType(name, json_type, - c_type, c_null)) + def _def_builtin_type(self, name, json_type, c_type): + self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type)) # TODO As long as we have QAPI_TYPES_BUILTIN to share multiple # qapi-types.h from a single .c, all arrays of builtins must be # declared in the first file whether or not they are used. Nicer @@ -1251,20 +1239,20 @@ class QAPISchema(object): self._make_array_type(name, None) def _def_predefineds(self): - for t in [('str', 'string', 'char' + pointer_suffix, 'NULL'), - ('number', 'number', 'double', '0'), - ('int', 'int', 'int64_t', '0'), - ('int8', 'int', 'int8_t', '0'), - ('int16', 'int', 'int16_t', '0'), - ('int32', 'int', 'int32_t', '0'), - ('int64', 'int', 'int64_t', '0'), - ('uint8', 'int', 'uint8_t', '0'), - ('uint16', 'int', 'uint16_t', '0'), - ('uint32', 'int', 'uint32_t', '0'), - ('uint64', 'int', 'uint64_t', '0'), - ('size', 'int', 'uint64_t', '0'), - ('bool', 'boolean', 'bool', 'false'), - ('any', 'value', 'QObject' + pointer_suffix, 'NULL')]: + for t in [('str', 'string', 'char' + pointer_suffix), + ('number', 'number', 'double'), + ('int', 'int', 'int64_t'), + ('int8', 'int', 'int8_t'), + ('int16', 'int', 'int16_t'), + ('int32', 'int', 'int32_t'), + ('int64', 'int', 'int64_t'), + ('uint8', 'int', 'uint8_t'), + ('uint16', 'int', 'uint16_t'), + ('uint32', 'int', 'uint32_t'), + ('uint64', 'int', 'uint64_t'), + ('size', 'int', 'uint64_t'), + ('bool', 'boolean', 'bool'), + ('any', 'value', 'QObject' + pointer_suffix)]: self._def_builtin_type(*t) self.the_empty_object_type = QAPISchemaObjectType('q_empty', None, None, [], None) -- cgit v1.1 From 32bafa8fdd098d52fbf1102d5a5e48d29398c0aa Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 17 Mar 2016 16:48:37 -0600 Subject: qapi: Don't special-case simple union wrappers Simple unions were carrying a special case that hid their 'data' QMP member from the resulting C struct, via the hack method QAPISchemaObjectTypeVariant.simple_union_type(). But by using the work we started by unboxing flat union and alternate branches, coupled with the ability to visit the members of an implicit type, we can now expose the simple union's implicit type in qapi-types.h: | struct q_obj_ImageInfoSpecificQCow2_wrapper { | ImageInfoSpecificQCow2 *data; | }; | | struct q_obj_ImageInfoSpecificVmdk_wrapper { | ImageInfoSpecificVmdk *data; | }; ... | struct ImageInfoSpecific { | ImageInfoSpecificKind type; | union { /* union tag is @type */ | void *data; |- ImageInfoSpecificQCow2 *qcow2; |- ImageInfoSpecificVmdk *vmdk; |+ q_obj_ImageInfoSpecificQCow2_wrapper qcow2; |+ q_obj_ImageInfoSpecificVmdk_wrapper vmdk; | } u; | }; Doing this removes asymmetry between QAPI's QMP side and its C side (both sides now expose 'data'), and means that the treatment of a simple union as sugar for a flat union is now equivalent in both languages (previously the two approaches used a different layer of dereferencing, where the simple union could be converted to a flat union with equivalent C layout but different {} on the wire, or to an equivalent QMP wire form but with different C representation). Using the implicit type also lets us get rid of the simple_union_type() hack. Of course, now all clients of simple unions have to adjust from using su->u.member to using su->u.member.data; while this touches a number of files in the tree, some earlier cleanup patches helped minimize the change to the initialization of a temporary variable rather than every single member access. The generated qapi-visit.c code is also affected by the layout change: |@@ -7393,10 +7393,10 @@ void visit_type_ImageInfoSpecific_member | } | switch (obj->type) { | case IMAGE_INFO_SPECIFIC_KIND_QCOW2: |- visit_type_ImageInfoSpecificQCow2(v, "data", &obj->u.qcow2, &err); |+ visit_type_q_obj_ImageInfoSpecificQCow2_wrapper_members(v, &obj->u.qcow2, &err); | break; | case IMAGE_INFO_SPECIFIC_KIND_VMDK: |- visit_type_ImageInfoSpecificVmdk(v, "data", &obj->u.vmdk, &err); |+ visit_type_q_obj_ImageInfoSpecificVmdk_wrapper_members(v, &obj->u.vmdk, &err); | break; | default: | abort(); Signed-off-by: Eric Blake Message-Id: <1458254921-17042-13-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-types.py | 8 +------- scripts/qapi-visit.py | 22 ++++------------------ scripts/qapi.py | 10 ---------- 3 files changed, 5 insertions(+), 35 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 79416b2..92ae619 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -121,16 +121,10 @@ def gen_variants(variants): c_name=c_name(variants.tag_member.name)) for var in variants.variants: - # Ugly special case for simple union TODO get rid of it - simple_union_type = var.simple_union_type() - if simple_union_type: - typ = simple_union_type.c_type() - else: - typ = var.type.c_unboxed_type() ret += mcgen(''' %(c_type)s %(c_name)s; ''', - c_type=typ, + c_type=var.type.c_unboxed_type(), c_name=c_name(var.name)) ret += mcgen(''' diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 5220bad..c147990 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -77,29 +77,15 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) c_name=c_name(variants.tag_member.name)) for var in variants.variants: - # TODO ugly special case for simple union - simple_union_type = var.simple_union_type() ret += mcgen(''' case %(case)s: + visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err); + break; ''', case=c_enum_const(variants.tag_member.type.name, var.name, - variants.tag_member.type.prefix)) - if simple_union_type: - ret += mcgen(''' - visit_type_%(c_type)s(v, "data", &obj->u.%(c_name)s, &err); -''', - c_type=simple_union_type.c_name(), - c_name=c_name(var.name)) - else: - ret += mcgen(''' - visit_type_%(c_type)s_members(v, &obj->u.%(c_name)s, &err); -''', - c_type=var.type.c_name(), - c_name=c_name(var.name)) - ret += mcgen(''' - break; -''') + variants.tag_member.type.prefix), + c_type=var.type.c_name(), c_name=c_name(var.name)) ret += mcgen(''' default: diff --git a/scripts/qapi.py b/scripts/qapi.py index 08d63bf..d91af94 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -1115,16 +1115,6 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): def __init__(self, name, typ): QAPISchemaObjectTypeMember.__init__(self, name, typ, False) - # This function exists to support ugly simple union special cases - # TODO get rid of them, and drop the function - def simple_union_type(self): - if (self.type.is_implicit() and - isinstance(self.type, QAPISchemaObjectType)): - assert len(self.type.members) == 1 - assert not self.type.variants - return self.type.members[0].type - return None - class QAPISchemaAlternateType(QAPISchemaType): def __init__(self, name, info, variants): -- cgit v1.1 From ac4338f8eb783fd421aae492ca262a586918471e Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 17 Mar 2016 16:48:39 -0600 Subject: qapi: Allow anonymous base for flat union Rather than requiring all flat unions to explicitly create a separate base struct, we can allow the qapi schema to specify the common members via an inline dictionary. This is similar to how commands can specify an inline anonymous type for its 'data'. We already have several struct types that only exist to serve as a single flat union's base; the next commit will clean them up. In particular, this patch's change to the BlockdevOptions example in qapi-code-gen.txt will actually be done in the real QAPI schema. Now that anonymous bases are legal, we need to rework the flat-union-bad-base negative test (as previously written, it forms what is now valid QAPI; tweak it to now provide coverage of a new error message path), and add a positive test in qapi-schema-test to use an anonymous base (making the integer argument optional, for even more coverage). Note that this patch only allows anonymous bases for flat unions; simple unions are already enough syntactic sugar that we do not want to burden them further. Meanwhile, while it would be easy to also allow an anonymous base for structs, that would be quite redundant, as the members can be put right into the struct instead. Signed-off-by: Eric Blake Message-Id: <1458254921-17042-15-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi-types.py | 10 ++++++---- scripts/qapi.py | 12 ++++++++++-- 2 files changed, 16 insertions(+), 6 deletions(-) (limited to 'scripts') diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index 92ae619..e09c875 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -72,12 +72,14 @@ struct %(c_name)s { c_name=c_name(name)) if base: - ret += mcgen(''' + if not base.is_implicit(): + ret += mcgen(''' /* Members inherited from %(c_name)s: */ ''', - c_name=base.c_name()) + c_name=base.c_name()) ret += gen_struct_members(base.members) - ret += mcgen(''' + if not base.is_implicit(): + ret += mcgen(''' /* Own members: */ ''') ret += gen_struct_members(members) @@ -224,7 +226,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): return self._fwdecl += gen_fwd_object_or_array(name) self.decl += gen_object(name, base, members, variants) - if base: + if base and not base.is_implicit(): self.decl += gen_upcast(name, base) # TODO Worth changing the visitor signature, so we could # directly use rather than repeat type.is_implicit()? diff --git a/scripts/qapi.py b/scripts/qapi.py index d91af94..a38ef52 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -327,6 +327,8 @@ class QAPISchemaParser(object): def find_base_members(base): + if isinstance(base, dict): + return base base_struct_define = find_struct(base) if not base_struct_define: return None @@ -561,9 +563,10 @@ def check_union(expr, expr_info): # Else, it's a flat union. else: - # The object must have a string member 'base'. + # The object must have a string or dictionary 'base'. check_type(expr_info, "'base' for union '%s'" % name, - base, allow_metas=['struct']) + base, allow_dict=True, allow_optional=True, + allow_metas=['struct']) if not base: raise QAPIExprError(expr_info, "Flat union '%s' must have a base" @@ -1039,6 +1042,8 @@ class QAPISchemaMember(object): owner = owner[6:] if owner.endswith('-arg'): return '(parameter of %s)' % owner[:-4] + elif owner.endswith('-base'): + return '(base of %s)' % owner[:-5] else: assert owner.endswith('-wrapper') # Unreachable and not implemented @@ -1325,6 +1330,9 @@ class QAPISchema(object): base = expr.get('base') tag_name = expr.get('discriminator') tag_member = None + if isinstance(base, dict): + base = (self._make_implicit_object_type( + name, info, 'base', self._make_members(base, info))) if tag_name: variants = [self._make_variant(key, value) for (key, value) in data.iteritems()] -- cgit v1.1 From 3666a97f78704b941c360dc917acb14c8774eca7 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 17 Mar 2016 16:48:40 -0600 Subject: qapi: Use anonymous bases in QMP flat unions Now that the generator supports it, we might as well use an anonymous base rather than breaking out a single-use Base structure, for all three of our current QMP flat unions. Oddly enough, this change does not affect the resulting introspection output (because we already inline the members of a base type into an object, and had no independent use of the base type reachable from a command). The case_whitelist now has to list the name of an implicit type; which is not too bad (consider it a feature if it makes it harder for developers to make the whitelist grow :) Signed-off-by: Eric Blake Message-Id: <1458254921-17042-16-git-send-email-eblake@redhat.com> Signed-off-by: Markus Armbruster --- scripts/qapi.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'scripts') diff --git a/scripts/qapi.py b/scripts/qapi.py index a38ef52..b13ae47 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -63,12 +63,12 @@ returns_whitelist = [ case_whitelist = [ # From QMP: 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status - 'CpuInfoBase', # CPU, visible through query-cpu 'CpuInfoMIPS', # PC, visible through query-cpu 'CpuInfoTricore', # PC, visible through query-cpu 'QapiErrorClass', # all members, visible through errors 'UuidInfo', # UUID, visible through query-uuid 'X86CPURegister32', # all members, visible indirectly through qom-get + 'q_obj_CpuInfo-base', # CPU, visible through query-cpu ] enum_types = [] -- cgit v1.1