* [Qemu-devel] [PATCH v6 01/16] qapi: Sort qapi-schema tests
2015-09-29 3:27 [Qemu-devel] [PATCH v6 00/16] post-introspection cleanups, subset A Eric Blake
@ 2015-09-29 3:27 ` Eric Blake
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 02/16] qapi: Improve 'include' error message Eric Blake
` (14 subsequent siblings)
15 siblings, 0 replies; 39+ messages in thread
From: Eric Blake @ 2015-09-29 3:27 UTC (permalink / raw)
To: qemu-devel; +Cc: marcandre.lureau, armbru, ehabkost
Recent changes to qapi have provided quite a bit of churn in
the makefile, because we are inconsistent on what order test
names appear in, and on whether to re-wrap the list of tests or
just add arbitrary line lengths. Writing the list in a sorted
fashion, one test per line, will make future patches easier
to see what tests are being added or removed by a patch.
Although it is tempting to use $(wildcard qapi-schema/*.json)
for a more compact listing, such an approach would risk picking
up leftover garbage .json files in the directory; so keeping
the list explicit is safer for ensuring reproducible tarballs
and test results.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: improve commit message to mention $(wildcard) consideration
---
tests/Makefile | 160 ++++++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 114 insertions(+), 46 deletions(-)
diff --git a/tests/Makefile b/tests/Makefile
index 4063639..6fd5885 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -221,52 +221,120 @@ $(foreach target,$(SYSEMU_TARGET_LIST), \
$(if $(findstring tests/qom-test$(EXESUF), $(check-qtest-$(target)-y)),, \
$(eval check-qtest-$(target)-y += tests/qom-test$(EXESUF))))
-check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
- comments.json empty.json enum-empty.json enum-missing-data.json \
- enum-wrong-data.json enum-int-member.json enum-dict-member.json \
- enum-clash-member.json enum-max-member.json enum-union-clash.json \
- enum-bad-name.json enum-bad-prefix.json \
- funny-char.json indented-expr.json \
- missing-type.json bad-ident.json ident-with-escape.json \
- escape-outside-string.json unknown-escape.json \
- escape-too-short.json escape-too-big.json unicode-str.json \
- double-type.json bad-base.json bad-type-bool.json bad-type-int.json \
- bad-type-dict.json double-data.json unknown-expr-key.json \
- redefined-type.json redefined-command.json redefined-builtin.json \
- redefined-event.json command-int.json bad-data.json event-max.json \
- type-bypass-bad-gen.json \
- args-invalid.json \
- args-array-empty.json args-array-unknown.json args-int.json \
- args-unknown.json args-member-unknown.json args-member-array.json \
- args-member-array-bad.json args-alternate.json args-union.json \
- args-any.json \
- returns-array-bad.json returns-int.json returns-dict.json \
- returns-unknown.json returns-alternate.json returns-whitelist.json \
- missing-colon.json missing-comma-list.json missing-comma-object.json \
- struct-data-invalid.json struct-member-invalid.json \
- nested-struct-data.json non-objects.json \
- qapi-schema-test.json quoted-structural-chars.json \
- leading-comma-list.json leading-comma-object.json \
- trailing-comma-list.json trailing-comma-object.json \
- unclosed-list.json unclosed-object.json unclosed-string.json \
- duplicate-key.json union-invalid-base.json union-bad-branch.json \
- union-optional-branch.json union-unknown.json union-max.json \
- flat-union-optional-discriminator.json flat-union-no-base.json \
- flat-union-invalid-discriminator.json flat-union-inline.json \
- flat-union-invalid-branch-key.json flat-union-reverse-define.json \
- flat-union-string-discriminator.json union-base-no-discriminator.json \
- flat-union-bad-discriminator.json flat-union-bad-base.json \
- flat-union-base-any.json \
- flat-union-array-branch.json flat-union-int-branch.json \
- flat-union-base-union.json flat-union-branch-clash.json \
- alternate-nested.json alternate-unknown.json alternate-clash.json \
- alternate-good.json alternate-base.json alternate-array.json \
- alternate-conflict-string.json alternate-conflict-dict.json \
- include-simple.json include-relpath.json include-format-err.json \
- include-non-file.json include-no-file.json include-before-err.json \
- include-nested-err.json include-self-cycle.json include-cycle.json \
- include-repetition.json event-nest-struct.json event-case.json \
- struct-base-clash.json struct-base-clash-deep.json )
+qapi-schema += alternate-array.json
+qapi-schema += alternate-base.json
+qapi-schema += alternate-clash.json
+qapi-schema += alternate-conflict-dict.json
+qapi-schema += alternate-conflict-string.json
+qapi-schema += alternate-good.json
+qapi-schema += alternate-nested.json
+qapi-schema += alternate-unknown.json
+qapi-schema += args-alternate.json
+qapi-schema += args-any.json
+qapi-schema += args-array-empty.json
+qapi-schema += args-array-unknown.json
+qapi-schema += args-int.json
+qapi-schema += args-invalid.json
+qapi-schema += args-member-array-bad.json
+qapi-schema += args-member-array.json
+qapi-schema += args-member-unknown.json
+qapi-schema += args-union.json
+qapi-schema += args-unknown.json
+qapi-schema += bad-base.json
+qapi-schema += bad-data.json
+qapi-schema += bad-ident.json
+qapi-schema += bad-type-bool.json
+qapi-schema += bad-type-dict.json
+qapi-schema += bad-type-int.json
+qapi-schema += command-int.json
+qapi-schema += comments.json
+qapi-schema += double-data.json
+qapi-schema += double-type.json
+qapi-schema += duplicate-key.json
+qapi-schema += empty.json
+qapi-schema += enum-bad-name.json
+qapi-schema += enum-bad-prefix.json
+qapi-schema += enum-clash-member.json
+qapi-schema += enum-dict-member.json
+qapi-schema += enum-empty.json
+qapi-schema += enum-int-member.json
+qapi-schema += enum-max-member.json
+qapi-schema += enum-missing-data.json
+qapi-schema += enum-union-clash.json
+qapi-schema += enum-wrong-data.json
+qapi-schema += escape-outside-string.json
+qapi-schema += escape-too-big.json
+qapi-schema += escape-too-short.json
+qapi-schema += event-case.json
+qapi-schema += event-max.json
+qapi-schema += event-nest-struct.json
+qapi-schema += flat-union-array-branch.json
+qapi-schema += flat-union-bad-base.json
+qapi-schema += flat-union-bad-discriminator.json
+qapi-schema += flat-union-base-any.json
+qapi-schema += flat-union-base-union.json
+qapi-schema += flat-union-branch-clash.json
+qapi-schema += flat-union-inline.json
+qapi-schema += flat-union-int-branch.json
+qapi-schema += flat-union-invalid-branch-key.json
+qapi-schema += flat-union-invalid-discriminator.json
+qapi-schema += flat-union-no-base.json
+qapi-schema += flat-union-optional-discriminator.json
+qapi-schema += flat-union-reverse-define.json
+qapi-schema += flat-union-string-discriminator.json
+qapi-schema += funny-char.json
+qapi-schema += ident-with-escape.json
+qapi-schema += include-before-err.json
+qapi-schema += include-cycle.json
+qapi-schema += include-format-err.json
+qapi-schema += include-nested-err.json
+qapi-schema += include-no-file.json
+qapi-schema += include-non-file.json
+qapi-schema += include-relpath.json
+qapi-schema += include-repetition.json
+qapi-schema += include-self-cycle.json
+qapi-schema += include-simple.json
+qapi-schema += indented-expr.json
+qapi-schema += leading-comma-list.json
+qapi-schema += leading-comma-object.json
+qapi-schema += missing-colon.json
+qapi-schema += missing-comma-list.json
+qapi-schema += missing-comma-object.json
+qapi-schema += missing-type.json
+qapi-schema += nested-struct-data.json
+qapi-schema += non-objects.json
+qapi-schema += qapi-schema-test.json
+qapi-schema += quoted-structural-chars.json
+qapi-schema += redefined-builtin.json
+qapi-schema += redefined-command.json
+qapi-schema += redefined-event.json
+qapi-schema += redefined-type.json
+qapi-schema += returns-alternate.json
+qapi-schema += returns-array-bad.json
+qapi-schema += returns-dict.json
+qapi-schema += returns-int.json
+qapi-schema += returns-unknown.json
+qapi-schema += returns-whitelist.json
+qapi-schema += struct-base-clash-deep.json
+qapi-schema += struct-base-clash.json
+qapi-schema += struct-data-invalid.json
+qapi-schema += struct-member-invalid.json
+qapi-schema += trailing-comma-list.json
+qapi-schema += trailing-comma-object.json
+qapi-schema += type-bypass-bad-gen.json
+qapi-schema += unclosed-list.json
+qapi-schema += unclosed-object.json
+qapi-schema += unclosed-string.json
+qapi-schema += unicode-str.json
+qapi-schema += union-bad-branch.json
+qapi-schema += union-base-no-discriminator.json
+qapi-schema += union-invalid-base.json
+qapi-schema += union-max.json
+qapi-schema += union-optional-branch.json
+qapi-schema += union-unknown.json
+qapi-schema += unknown-escape.json
+qapi-schema += unknown-expr-key.json
+check-qapi-schema-y := $(addprefix tests/qapi-schema/, $(qapi-schema))
GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h \
tests/test-qmp-commands.h tests/test-qapi-event.h \
--
2.4.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v6 02/16] qapi: Improve 'include' error message
2015-09-29 3:27 [Qemu-devel] [PATCH v6 00/16] post-introspection cleanups, subset A Eric Blake
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 01/16] qapi: Sort qapi-schema tests Eric Blake
@ 2015-09-29 3:27 ` Eric Blake
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 03/16] qapi: Invoke exception superclass initializer Eric Blake
` (13 subsequent siblings)
15 siblings, 0 replies; 39+ messages in thread
From: Eric Blake @ 2015-09-29 3:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
Use of '"...%s" % include' to print non-strings can lead to
ugly messages, such as this (if the .json change is applied
without the qapi.py change):
Expected a file name (string), got: OrderedDict()
Better is to just omit the actual non-string value in the
message.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: hoist earlier in series (was 6/46), tweak wording of message
---
scripts/qapi.py | 3 +--
tests/qapi-schema/include-non-file.err | 2 +-
tests/qapi-schema/include-non-file.json | 2 +-
3 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 06478bb..362e007 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -132,8 +132,7 @@ class QAPISchemaParser(object):
include = expr["include"]
if not isinstance(include, str):
raise QAPIExprError(expr_info,
- 'Expected a file name (string), got: %s'
- % include)
+ "Value of 'include' must be a string")
incl_abs_fname = os.path.join(os.path.dirname(abs_fname),
include)
# catch inclusion cycle
diff --git a/tests/qapi-schema/include-non-file.err b/tests/qapi-schema/include-non-file.err
index 9658c78..faae1ea 100644
--- a/tests/qapi-schema/include-non-file.err
+++ b/tests/qapi-schema/include-non-file.err
@@ -1 +1 @@
-tests/qapi-schema/include-non-file.json:1: Expected a file name (string), got: ['foo', 'bar']
+tests/qapi-schema/include-non-file.json:1: Value of 'include' must be a string
diff --git a/tests/qapi-schema/include-non-file.json b/tests/qapi-schema/include-non-file.json
index cd43c3f..4711aa4 100644
--- a/tests/qapi-schema/include-non-file.json
+++ b/tests/qapi-schema/include-non-file.json
@@ -1 +1 @@
-{ 'include': [ 'foo', 'bar' ] }
+{ 'include': {} }
--
2.4.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v6 03/16] qapi: Invoke exception superclass initializer
2015-09-29 3:27 [Qemu-devel] [PATCH v6 00/16] post-introspection cleanups, subset A Eric Blake
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 01/16] qapi: Sort qapi-schema tests Eric Blake
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 02/16] qapi: Improve 'include' error message Eric Blake
@ 2015-09-29 3:27 ` Eric Blake
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 04/16] qapi: Clean up qapi.py per pep8 Eric Blake
` (12 subsequent siblings)
15 siblings, 0 replies; 39+ messages in thread
From: Eric Blake @ 2015-09-29 3:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
pylint recommends that every exception class should explicitly
invoke the superclass __init__, even though things seem to work
fine without it.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: split off of 2/26
---
scripts/qapi.py | 2 ++
1 file changed, 2 insertions(+)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 362e007..a2d869e 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -81,6 +81,7 @@ def error_path(parent):
class QAPISchemaError(Exception):
def __init__(self, schema, msg):
+ Exception.__init__(self)
self.fname = schema.fname
self.msg = msg
self.col = 1
@@ -98,6 +99,7 @@ class QAPISchemaError(Exception):
class QAPIExprError(Exception):
def __init__(self, expr_info, msg):
+ Exception.__init__(self)
self.info = expr_info
self.msg = msg
--
2.4.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v6 04/16] qapi: Clean up qapi.py per pep8
2015-09-29 3:27 [Qemu-devel] [PATCH v6 00/16] post-introspection cleanups, subset A Eric Blake
` (2 preceding siblings ...)
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 03/16] qapi: Invoke exception superclass initializer Eric Blake
@ 2015-09-29 3:27 ` Eric Blake
2015-09-29 10:58 ` Markus Armbruster
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 05/16] qapi: Test for various name collisions Eric Blake
` (11 subsequent siblings)
15 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2015-09-29 3:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
Silence pep8, and make pylint a bit happier. Just style cleanups,
plus killing a useless comment in camel_to_upper(); no semantic
changes.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: split off non-style changes, add in ordereddict.py, and
rework the camel_to_upper conditional
---
scripts/ordereddict.py | 3 +-
scripts/qapi.py | 161 ++++++++++++++++++++++++++++++++-----------------
2 files changed, 106 insertions(+), 58 deletions(-)
diff --git a/scripts/ordereddict.py b/scripts/ordereddict.py
index 7242b50..2d1d813 100644
--- a/scripts/ordereddict.py
+++ b/scripts/ordereddict.py
@@ -22,6 +22,7 @@
from UserDict import DictMixin
+
class OrderedDict(dict, DictMixin):
def __init__(self, *args, **kwds):
@@ -117,7 +118,7 @@ class OrderedDict(dict, DictMixin):
if isinstance(other, OrderedDict):
if len(self) != len(other):
return False
- for p, q in zip(self.items(), other.items()):
+ for p, q in zip(self.items(), other.items()):
if p != q:
return False
return True
diff --git a/scripts/qapi.py b/scripts/qapi.py
index a2d869e..7e8a99d 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -71,6 +71,7 @@ all_names = {}
# Parsing the schema into expressions
#
+
def error_path(parent):
res = ""
while parent:
@@ -79,6 +80,7 @@ def error_path(parent):
parent = parent['parent']
return res
+
class QAPISchemaError(Exception):
def __init__(self, schema, msg):
Exception.__init__(self)
@@ -97,6 +99,7 @@ class QAPISchemaError(Exception):
return error_path(self.info) + \
"%s:%d:%d: %s" % (self.fname, self.line, self.col, self.msg)
+
class QAPIExprError(Exception):
def __init__(self, expr_info, msg):
Exception.__init__(self)
@@ -107,9 +110,10 @@ class QAPIExprError(Exception):
return error_path(self.info['parent']) + \
"%s:%d: %s" % (self.info['file'], self.info['line'], self.msg)
+
class QAPISchemaParser(object):
- def __init__(self, fp, previously_included = [], incl_info = None):
+ def __init__(self, fp, previously_included=[], incl_info=None):
abs_fname = os.path.abspath(fp.name)
fname = fp.name
self.fname = fname
@@ -124,13 +128,14 @@ class QAPISchemaParser(object):
self.exprs = []
self.accept()
- while self.tok != None:
+ while self.tok is not None:
expr_info = {'file': fname, 'line': self.line,
'parent': self.incl_info}
expr = self.get_expr(False)
if isinstance(expr, dict) and "include" in expr:
if len(expr) != 1:
- raise QAPIExprError(expr_info, "Invalid 'include' directive")
+ raise QAPIExprError(expr_info,
+ "Invalid 'include' directive")
include = expr["include"]
if not isinstance(include, str):
raise QAPIExprError(expr_info,
@@ -193,7 +198,7 @@ class QAPISchemaParser(object):
string += '\t'
elif ch == 'u':
value = 0
- for x in range(0, 4):
+ for _ in range(0, 4):
ch = self.src[self.cursor]
self.cursor += 1
if ch not in "0123456789abcdefABCDEF":
@@ -215,7 +220,7 @@ class QAPISchemaParser(object):
string += ch
else:
raise QAPISchemaError(self,
- "Unknown escape \\%s" %ch)
+ "Unknown escape \\%s" % ch)
esc = False
elif ch == "\\":
esc = True
@@ -275,7 +280,7 @@ class QAPISchemaParser(object):
if self.tok == ']':
self.accept()
return expr
- if not self.tok in "{['tfn":
+ if self.tok not in "{['tfn":
raise QAPISchemaError(self, 'Expected "{", "[", "]", string, '
'boolean or "null"')
while True:
@@ -309,15 +314,17 @@ class QAPISchemaParser(object):
# TODO catching name collisions in generated code would be nice
#
+
def find_base_fields(base):
base_struct_define = find_struct(base)
if not base_struct_define:
return None
return base_struct_define['data']
+
# Return the qtype of an alternate branch, or None on error.
def find_alternate_member_qtype(qapi_type):
- if builtin_types.has_key(qapi_type):
+ if qapi_type in builtin_types:
return builtin_types[qapi_type]
elif find_struct(qapi_type):
return "QTYPE_QDICT"
@@ -327,6 +334,7 @@ def find_alternate_member_qtype(qapi_type):
return "QTYPE_QDICT"
return None
+
# Return the discriminator enum define if discriminator is specified as an
# enum type, otherwise return None.
def discriminator_find_enum_define(expr):
@@ -346,11 +354,14 @@ def discriminator_find_enum_define(expr):
return find_enum(discriminator_type)
+
# FIXME should enforce "other than downstream extensions [...], all
# names should begin with a letter".
valid_name = re.compile('^[a-zA-Z_][a-zA-Z0-9_.-]*$')
-def check_name(expr_info, source, name, allow_optional = False,
- enum_member = False):
+
+
+def check_name(expr_info, source, name, allow_optional=False,
+ enum_member=False):
global valid_name
membername = name
@@ -371,7 +382,8 @@ def check_name(expr_info, source, name, allow_optional = False,
raise QAPIExprError(expr_info,
"%s uses invalid name '%s'" % (source, name))
-def add_name(name, info, meta, implicit = False):
+
+def add_name(name, info, meta, implicit=False):
global all_names
check_name(info, "'%s'" % meta, name)
# FIXME should reject names that differ only in '_' vs. '.'
@@ -386,12 +398,14 @@ def add_name(name, info, meta, implicit = False):
% (meta, name))
all_names[name] = meta
+
def add_struct(definition, info):
global struct_types
name = definition['struct']
add_name(name, info, 'struct')
struct_types.append(definition)
+
def find_struct(name):
global struct_types
for struct in struct_types:
@@ -399,12 +413,14 @@ def find_struct(name):
return struct
return None
+
def add_union(definition, info):
global union_types
name = definition['union']
add_name(name, info, 'union')
union_types.append(definition)
+
def find_union(name):
global union_types
for union in union_types:
@@ -412,11 +428,13 @@ def find_union(name):
return union
return None
-def add_enum(name, info, enum_values = None, implicit = False):
+
+def add_enum(name, info, enum_values=None, implicit=False):
global enum_types
add_name(name, info, 'enum', implicit)
enum_types.append({"enum_name": name, "enum_values": enum_values})
+
def find_enum(name):
global enum_types
for enum in enum_types:
@@ -424,12 +442,14 @@ def find_enum(name):
return enum
return None
+
def is_enum(name):
- return find_enum(name) != None
+ return find_enum(name) is not None
-def check_type(expr_info, source, value, allow_array = False,
- allow_dict = False, allow_optional = False,
- allow_metas = []):
+
+def check_type(expr_info, source, value, allow_array=False,
+ allow_dict=False, allow_optional=False,
+ allow_metas=[]):
global all_names
if value is None:
@@ -448,7 +468,7 @@ def check_type(expr_info, source, value, allow_array = False,
# Check if type name for value is okay
if isinstance(value, str):
- if not value in all_names:
+ if value not in all_names:
raise QAPIExprError(expr_info,
"%s uses unknown type '%s'"
% (source, value))
@@ -477,7 +497,8 @@ def check_type(expr_info, source, value, allow_array = False,
allow_metas=['built-in', 'union', 'alternate', 'struct',
'enum'])
-def check_member_clash(expr_info, base_name, data, source = ""):
+
+def check_member_clash(expr_info, base_name, data, source=""):
base = find_struct(base_name)
assert base
base_members = base['data']
@@ -491,6 +512,7 @@ def check_member_clash(expr_info, base_name, data, source = ""):
if base.get('base'):
check_member_clash(expr_info, base['base'], data, source)
+
def check_command(expr, expr_info):
name = expr['command']
@@ -504,6 +526,7 @@ def check_command(expr, expr_info):
expr.get('returns'), allow_array=True,
allow_optional=True, allow_metas=returns_meta)
+
def check_event(expr, expr_info):
global events
name = expr['event']
@@ -515,19 +538,20 @@ def check_event(expr, expr_info):
expr.get('data'), allow_dict=True, allow_optional=True,
allow_metas=['struct'])
+
def check_union(expr, expr_info):
name = expr['union']
base = expr.get('base')
discriminator = expr.get('discriminator')
members = expr['data']
- values = { 'MAX': '(automatic)' }
+ values = {'MAX': '(automatic)'}
# Two types of unions, determined by discriminator.
# With no discriminator it is a simple union.
if discriminator is None:
enum_define = None
- allow_metas=['built-in', 'union', 'alternate', 'struct', 'enum']
+ allow_metas = ['built-in', 'union', 'alternate', 'struct', 'enum']
if base is not None:
raise QAPIExprError(expr_info,
"Simple union '%s' must not have a base"
@@ -557,7 +581,7 @@ def check_union(expr, expr_info):
"struct '%s'"
% (discriminator, base))
enum_define = find_enum(discriminator_type)
- allow_metas=['struct']
+ allow_metas = ['struct']
# Do not allow string discriminator
if not enum_define:
raise QAPIExprError(expr_info,
@@ -581,7 +605,7 @@ def check_union(expr, expr_info):
# If the discriminator names an enum type, then all members
# of 'data' must also be members of the enum type.
if enum_define:
- if not key in enum_define['enum_values']:
+ if key not in enum_define['enum_values']:
raise QAPIExprError(expr_info,
"Discriminator value '%s' is not found in "
"enum '%s'" %
@@ -596,10 +620,11 @@ def check_union(expr, expr_info):
% (name, key, values[c_key]))
values[c_key] = key
+
def check_alternate(expr, expr_info):
name = expr['alternate']
members = expr['data']
- values = { 'MAX': '(automatic)' }
+ values = {'MAX': '(automatic)'}
types_seen = {}
# Check every branch
@@ -627,11 +652,12 @@ def check_alternate(expr, expr_info):
% (name, key, types_seen[qtype]))
types_seen[qtype] = key
+
def check_enum(expr, expr_info):
name = expr['enum']
members = expr.get('data')
prefix = expr.get('prefix')
- values = { 'MAX': '(automatic)' }
+ values = {'MAX': '(automatic)'}
if not isinstance(members, list):
raise QAPIExprError(expr_info,
@@ -640,7 +666,7 @@ def check_enum(expr, expr_info):
raise QAPIExprError(expr_info,
"Enum '%s' requires a string for 'prefix'" % name)
for member in members:
- check_name(expr_info, "Member of enum '%s'" %name, member,
+ check_name(expr_info, "Member of enum '%s'" % name, member,
enum_member=True)
key = camel_to_upper(member)
if key in values:
@@ -649,6 +675,7 @@ def check_enum(expr, expr_info):
% (name, member, values[key]))
values[key] = member
+
def check_struct(expr, expr_info):
name = expr['struct']
members = expr['data']
@@ -660,6 +687,7 @@ def check_struct(expr, expr_info):
if expr.get('base'):
check_member_clash(expr_info, expr['base'], expr['data'])
+
def check_keys(expr_elem, meta, required, optional=[]):
expr = expr_elem['expr']
info = expr_elem['info']
@@ -667,22 +695,23 @@ def check_keys(expr_elem, meta, required, optional=[]):
if not isinstance(name, str):
raise QAPIExprError(info,
"'%s' key must have a string value" % meta)
- required = required + [ meta ]
+ required = required + [meta]
for (key, value) in expr.items():
- if not key in required and not key in optional:
+ if key not in required + optional:
raise QAPIExprError(info,
"Unknown key '%s' in %s '%s'"
% (key, meta, name))
- if (key == 'gen' or key == 'success-response') and value != False:
+ if (key == 'gen' or key == 'success-response') and value is not False:
raise QAPIExprError(info,
"'%s' of %s '%s' should only use false value"
% (key, meta, name))
for key in required:
- if not expr.has_key(key):
+ if key not in expr:
raise QAPIExprError(info,
"Key '%s' is missing from %s '%s'"
% (key, meta, name))
+
def check_exprs(exprs):
global all_names
@@ -692,24 +721,24 @@ def check_exprs(exprs):
for expr_elem in exprs:
expr = expr_elem['expr']
info = expr_elem['info']
- if expr.has_key('enum'):
+ if 'enum' in expr:
check_keys(expr_elem, 'enum', ['data'], ['prefix'])
add_enum(expr['enum'], info, expr['data'])
- elif expr.has_key('union'):
+ elif 'union' in expr:
check_keys(expr_elem, 'union', ['data'],
['base', 'discriminator'])
add_union(expr, info)
- elif expr.has_key('alternate'):
+ elif 'alternate' in expr:
check_keys(expr_elem, 'alternate', ['data'])
add_name(expr['alternate'], info, 'alternate')
- elif expr.has_key('struct'):
+ elif 'struct' in expr:
check_keys(expr_elem, 'struct', ['data'], ['base'])
add_struct(expr, info)
- elif expr.has_key('command'):
+ elif 'command' in expr:
check_keys(expr_elem, 'command', [],
['data', 'returns', 'gen', 'success-response'])
add_name(expr['command'], info, 'command')
- elif expr.has_key('event'):
+ elif 'event' in expr:
check_keys(expr_elem, 'event', [], ['data'])
add_name(expr['event'], info, 'event')
else:
@@ -719,11 +748,11 @@ def check_exprs(exprs):
# Try again for hidden UnionKind enum
for expr_elem in exprs:
expr = expr_elem['expr']
- if expr.has_key('union'):
+ if 'union' in expr:
if not discriminator_find_enum_define(expr):
add_enum('%sKind' % expr['union'], expr_elem['info'],
implicit=True)
- elif expr.has_key('alternate'):
+ elif 'alternate' in expr:
add_enum('%sKind' % expr['alternate'], expr_elem['info'],
implicit=True)
@@ -732,17 +761,17 @@ def check_exprs(exprs):
expr = expr_elem['expr']
info = expr_elem['info']
- if expr.has_key('enum'):
+ if 'enum' in expr:
check_enum(expr, info)
- elif expr.has_key('union'):
+ elif 'union' in expr:
check_union(expr, info)
- elif expr.has_key('alternate'):
+ elif 'alternate' in expr:
check_alternate(expr, info)
- elif expr.has_key('struct'):
+ elif 'struct' in expr:
check_struct(expr, info)
- elif expr.has_key('command'):
+ elif 'command' in expr:
check_command(expr, info)
- elif expr.has_key('event'):
+ elif 'event' in expr:
check_event(expr, info)
else:
assert False, 'unexpected meta type'
@@ -994,6 +1023,7 @@ class QAPISchemaObjectTypeVariants(object):
vseen = dict(seen)
v.check(schema, self.tag_member.type, vseen)
+
class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
def __init__(self, name, typ):
QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
@@ -1293,6 +1323,7 @@ def camel_case(name):
new_name += ch.lower()
return new_name
+
# ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
# ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
# ENUM24_Name -> ENUM24_NAME
@@ -1307,14 +1338,14 @@ def camel_to_upper(value):
c = c_fun_str[i]
# When c is upper and no "_" appears before, do more checks
if c.isupper() and (i > 0) and c_fun_str[i - 1] != "_":
- # Case 1: next string is lower
- # Case 2: previous string is digit
- if (i < (l - 1) and c_fun_str[i + 1].islower()) or \
- c_fun_str[i - 1].isdigit():
+ if i < (l - 1) and c_fun_str[i + 1].islower():
+ new_name += '_'
+ elif c_fun_str[i - 1].isdigit():
new_name += '_'
new_name += c
return new_name.lstrip('_').upper()
+
def c_enum_const(type_name, const_name, prefix=None):
if prefix is not None:
type_name = prefix
@@ -1322,6 +1353,7 @@ def c_enum_const(type_name, const_name, prefix=None):
c_name_trans = string.maketrans('.-', '__')
+
# Map @name to a valid C identifier.
# If @protect, avoid returning certain ticklish identifiers (like
# C keywords) by prepending "q_".
@@ -1334,15 +1366,16 @@ c_name_trans = string.maketrans('.-', '__')
def c_name(name, protect=True):
# ANSI X3J11/88-090, 3.1.1
c89_words = set(['auto', 'break', 'case', 'char', 'const', 'continue',
- 'default', 'do', 'double', 'else', 'enum', 'extern', 'float',
- 'for', 'goto', 'if', 'int', 'long', 'register', 'return',
- 'short', 'signed', 'sizeof', 'static', 'struct', 'switch',
- 'typedef', 'union', 'unsigned', 'void', 'volatile', 'while'])
+ 'default', 'do', 'double', 'else', 'enum', 'extern',
+ 'float', 'for', 'goto', 'if', 'int', 'long', 'register',
+ 'return', 'short', 'signed', 'sizeof', 'static',
+ 'struct', 'switch', 'typedef', 'union', 'unsigned',
+ 'void', 'volatile', 'while'])
# ISO/IEC 9899:1999, 6.4.1
c99_words = set(['inline', 'restrict', '_Bool', '_Complex', '_Imaginary'])
# ISO/IEC 9899:2011, 6.4.1
- c11_words = set(['_Alignas', '_Alignof', '_Atomic', '_Generic', '_Noreturn',
- '_Static_assert', '_Thread_local'])
+ c11_words = set(['_Alignas', '_Alignof', '_Atomic', '_Generic',
+ '_Noreturn', '_Static_assert', '_Thread_local'])
# GCC http://gcc.gnu.org/onlinedocs/gcc-4.7.1/gcc/C-Extensions.html
# excluding _.*
gcc_words = set(['asm', 'typeof'])
@@ -1358,29 +1391,34 @@ def c_name(name, protect=True):
'not_eq', 'or', 'or_eq', 'xor', 'xor_eq'])
# namespace pollution:
polluted_words = set(['unix', 'errno'])
- if protect and (name in c89_words | c99_words | c11_words | gcc_words | cpp_words | polluted_words):
+ if protect and (name in c89_words | c99_words | c11_words | gcc_words
+ | cpp_words | polluted_words):
return "q_" + name
return name.translate(c_name_trans)
eatspace = '\033EATSPACE.'
pointer_suffix = ' *' + eatspace
+
def genindent(count):
ret = ""
- for i in range(count):
+ for _ in range(count):
ret += " "
return ret
indent_level = 0
+
def push_indent(indent_amount=4):
global indent_level
indent_level += indent_amount
+
def pop_indent(indent_amount=4):
global indent_level
indent_level -= indent_amount
+
# Generate @code with @kwds interpolated.
# Obey indent_level, and strip eatspace.
def cgen(code, **kwds):
@@ -1393,6 +1431,7 @@ def cgen(code, **kwds):
raw = raw[0]
return re.sub(re.escape(eatspace) + ' *', '', raw)
+
def mcgen(code, **kwds):
if code[0] == '\n':
code = code[1:]
@@ -1402,6 +1441,7 @@ def mcgen(code, **kwds):
def guardname(filename):
return c_name(filename, protect=False).upper()
+
def guardstart(name):
return mcgen('''
@@ -1411,6 +1451,7 @@ def guardstart(name):
''',
name=guardname(name))
+
def guardend(name):
return mcgen('''
@@ -1419,6 +1460,7 @@ def guardend(name):
''',
name=guardname(name))
+
def gen_enum_lookup(name, values, prefix=None):
ret = mcgen('''
@@ -1440,6 +1482,7 @@ const char *const %(c_name)s_lookup[] = {
max_index=max_index)
return ret
+
def gen_enum(name, values, prefix=None):
# append automatically generated _MAX value
enum_values = values + ['MAX']
@@ -1471,6 +1514,7 @@ extern const char *const %(c_name)s_lookup[];
c_name=c_name(name))
return ret
+
def gen_params(arg_type, extra):
if not arg_type:
return extra
@@ -1491,7 +1535,8 @@ def gen_params(arg_type, extra):
# Common command line parsing
#
-def parse_command_line(extra_options = "", extra_long_options = []):
+
+def parse_command_line(extra_options="", extra_long_options=[]):
try:
opts, args = getopt.gnu_getopt(sys.argv[1:],
@@ -1542,6 +1587,7 @@ def parse_command_line(extra_options = "", extra_long_options = []):
# Generate output files with boilerplate
#
+
def open_output(output_dir, do_c, do_h, prefix, c_file, h_file,
c_comment, h_comment):
guard = guardname(prefix + h_file)
@@ -1569,7 +1615,7 @@ def open_output(output_dir, do_c, do_h, prefix, c_file, h_file,
/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
%(comment)s
''',
- comment = c_comment))
+ comment=c_comment))
fdecl.write(mcgen('''
/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
@@ -1578,10 +1624,11 @@ def open_output(output_dir, do_c, do_h, prefix, c_file, h_file,
#define %(guard)s
''',
- comment = h_comment, guard = guard))
+ comment=h_comment, guard=guard))
return (fdef, fdecl)
+
def close_output(fdef, fdecl):
fdecl.write('''
#endif
--
2.4.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 04/16] qapi: Clean up qapi.py per pep8
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 04/16] qapi: Clean up qapi.py per pep8 Eric Blake
@ 2015-09-29 10:58 ` Markus Armbruster
2015-09-29 13:00 ` Eric Blake
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2015-09-29 10:58 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Silence pep8, and make pylint a bit happier. Just style cleanups,
> plus killing a useless comment in camel_to_upper(); no semantic
> changes.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
[...]
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index a2d869e..7e8a99d 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
[...]
> @@ -667,22 +695,23 @@ def check_keys(expr_elem, meta, required, optional=[]):
> if not isinstance(name, str):
> raise QAPIExprError(info,
> "'%s' key must have a string value" % meta)
> - required = required + [ meta ]
> + required = required + [meta]
> for (key, value) in expr.items():
> - if not key in required and not key in optional:
> + if key not in required + optional:
The purely mechanical cleanup would be
if key not in required and key not in optional:
Yours is a bit more concise, but I'm not sure it's actually easier to
read. It's also less efficient unless a sufficiently smart compiler
transforms it back. Not that efficiency matters here.
> raise QAPIExprError(info,
> "Unknown key '%s' in %s '%s'"
> % (key, meta, name))
> - if (key == 'gen' or key == 'success-response') and value != False:
> + if (key == 'gen' or key == 'success-response') and value is not False:
> raise QAPIExprError(info,
> "'%s' of %s '%s' should only use false value"
> % (key, meta, name))
> for key in required:
> - if not expr.has_key(key):
> + if key not in expr:
> raise QAPIExprError(info,
> "Key '%s' is missing from %s '%s'"
> % (key, meta, name))
>
> +
> def check_exprs(exprs):
> global all_names
>
[...]
> @@ -1293,6 +1323,7 @@ def camel_case(name):
> new_name += ch.lower()
> return new_name
>
> +
> # ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
> # ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
> # ENUM24_Name -> ENUM24_NAME
> @@ -1307,14 +1338,14 @@ def camel_to_upper(value):
> c = c_fun_str[i]
> # When c is upper and no "_" appears before, do more checks
> if c.isupper() and (i > 0) and c_fun_str[i - 1] != "_":
> - # Case 1: next string is lower
> - # Case 2: previous string is digit
> - if (i < (l - 1) and c_fun_str[i + 1].islower()) or \
> - c_fun_str[i - 1].isdigit():
> + if i < (l - 1) and c_fun_str[i + 1].islower():
I doubt we need the parenthesis around (l - 1).
> + new_name += '_'
> + elif c_fun_str[i - 1].isdigit():
> new_name += '_'
> new_name += c
> return new_name.lstrip('_').upper()
>
> +
> def c_enum_const(type_name, const_name, prefix=None):
> if prefix is not None:
> type_name = prefix
[...]
Happy to touch up on commit to my tree.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 04/16] qapi: Clean up qapi.py per pep8
2015-09-29 10:58 ` Markus Armbruster
@ 2015-09-29 13:00 ` Eric Blake
0 siblings, 0 replies; 39+ messages in thread
From: Eric Blake @ 2015-09-29 13:00 UTC (permalink / raw)
To: Markus Armbruster; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2528 bytes --]
On 09/29/2015 04:58 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Silence pep8, and make pylint a bit happier. Just style cleanups,
>> plus killing a useless comment in camel_to_upper(); no semantic
>> changes.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> [...]
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index a2d869e..7e8a99d 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
> [...]
>> @@ -667,22 +695,23 @@ def check_keys(expr_elem, meta, required, optional=[]):
>> if not isinstance(name, str):
>> raise QAPIExprError(info,
>> "'%s' key must have a string value" % meta)
>> - required = required + [ meta ]
>> + required = required + [meta]
>> for (key, value) in expr.items():
>> - if not key in required and not key in optional:
>> + if key not in required + optional:
>
> The purely mechanical cleanup would be
>
> if key not in required and key not in optional:
>
> Yours is a bit more concise, but I'm not sure it's actually easier to
> read. It's also less efficient unless a sufficiently smart compiler
> transforms it back. Not that efficiency matters here.
The purely mechanical version still fits the line, so I'm not strongly
attached to the concise version.
>> @@ -1307,14 +1338,14 @@ def camel_to_upper(value):
>> c = c_fun_str[i]
>> # When c is upper and no "_" appears before, do more checks
>> if c.isupper() and (i > 0) and c_fun_str[i - 1] != "_":
>> - # Case 1: next string is lower
>> - # Case 2: previous string is digit
>> - if (i < (l - 1) and c_fun_str[i + 1].islower()) or \
>> - c_fun_str[i - 1].isdigit():
>> + if i < (l - 1) and c_fun_str[i + 1].islower():
>
> I doubt we need the parenthesis around (l - 1).
Probably not; I was just preserving the existing text.
>
>> + new_name += '_'
>> + elif c_fun_str[i - 1].isdigit():
>> new_name += '_'
>> new_name += c
>> return new_name.lstrip('_').upper()
>>
>> +
>> def c_enum_const(type_name, const_name, prefix=None):
>> if prefix is not None:
>> type_name = prefix
> [...]
>
> Happy to touch up on commit to my tree.
Or, since you want a respin of 5, I can catch them.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 05/16] qapi: Test for various name collisions
2015-09-29 3:27 [Qemu-devel] [PATCH v6 00/16] post-introspection cleanups, subset A Eric Blake
` (3 preceding siblings ...)
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 04/16] qapi: Clean up qapi.py per pep8 Eric Blake
@ 2015-09-29 3:27 ` Eric Blake
2015-09-29 12:33 ` Markus Armbruster
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 06/16] qapi: Avoid assertion failure on union 'type' collision Eric Blake
` (10 subsequent siblings)
15 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2015-09-29 3:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
Expose some weaknesses in the generator: we don't always forbid
the generation of structs that contain multiple members that map
to the same C name. This has already been marked FIXME in
qapi.py in commit d90675f, but having more tests will make sure
future patches produce desired behavior. This patch focuses on
C struct members, and does not consider collisions between commands
and events (affecting C function names), or even on collisions
between generated C type names with user type names (for things
like automatic FOOList struct representing array types).
There are two types of struct collisions we want to catch:
1) Collision that would require the QMP user to specify the
same key more than once inside a single JSON {} object (we
already prevent duplicate keys within a single type, so this
is triggered when merging multiple types into a single JSON
object via base classes or flat unions)
2) Collision between two members of the C struct that is
generated for a given qapi type:
a) a collision occurs from two qapi names mapping to the
same C name
b) the collision is with a C name used for another purpose
(we already fixed 'kind' in commit 0f61af3e, and one use of
'base' in commit 1e6c1616 for unions, but we have another
use of 'base' in structs, and still collide with 'data' in
unions, not to mention the tag 'type'/'kind' and the various
tag values).
Oddly enough, while commit 1e6c1616 fixed a type 2b collision
with 'base', it introduced a different collision that was not
previously possible: now that the base members are no longer
boxed behind 'base', they can collide with the enum tag members
(used as member names within the embedded C union).
Ultimately, if we need to have a flat union where an enum
value clashes with a base member name, we could change the
generator to use something like '_tag_value' instead of
'value' for the C name of union members; but until such a
need arises, it will probably be easier to just forbid these
collisions.
Some of these collisions are already caught in the old-style
parser checks, but ultimately we want all collisions to be
caught in the new-style QAPISchema*.check() methods.
Some of these negative tests will be deleted later, and positive
tests added to qapi-schema-test.json in their place, when the
generator code is reworked so that a particular collision no
longer causes failed code generation.
[git rename detection may be a bit confused by the actions of
this commit, since it both creates and renames files that are
equally small and similar in content]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: rework commit message, alter test names and comments to be
more descriptive, repurpose flat-union-cycle to better match its
name (and not duplicate what the renamed flat-union-clash-branch
tests), add new tests (union-clash-type, flat-union-clash-type)
that detect an assertion failures
---
tests/Makefile | 10 +++++++++-
.../{flat-union-branch-clash.out => args-name-clash.err} | 0
tests/qapi-schema/args-name-clash.exit | 1 +
tests/qapi-schema/args-name-clash.json | 2 ++
tests/qapi-schema/args-name-clash.out | 6 ++++++
tests/qapi-schema/flat-union-branch-clash.err | 1 -
tests/qapi-schema/flat-union-clash-branch.err | 0
tests/qapi-schema/flat-union-clash-branch.exit | 1 +
tests/qapi-schema/flat-union-clash-branch.json | 15 +++++++++++++++
tests/qapi-schema/flat-union-clash-branch.out | 14 ++++++++++++++
tests/qapi-schema/flat-union-clash-member.err | 1 +
...on-branch-clash.exit => flat-union-clash-member.exit} | 0
...on-branch-clash.json => flat-union-clash-member.json} | 2 +-
tests/qapi-schema/flat-union-clash-member.out | 0
tests/qapi-schema/flat-union-clash-type.err | 16 ++++++++++++++++
tests/qapi-schema/flat-union-clash-type.exit | 1 +
tests/qapi-schema/flat-union-clash-type.json | 11 +++++++++++
tests/qapi-schema/flat-union-clash-type.out | 0
tests/qapi-schema/flat-union-cycle.err | 1 +
tests/qapi-schema/flat-union-cycle.exit | 1 +
tests/qapi-schema/flat-union-cycle.json | 7 +++++++
tests/qapi-schema/flat-union-cycle.out | 0
tests/qapi-schema/qapi-schema-test.json | 7 +++++--
tests/qapi-schema/qapi-schema-test.out | 2 ++
tests/qapi-schema/struct-base-clash-base.err | 0
tests/qapi-schema/struct-base-clash-base.exit | 1 +
tests/qapi-schema/struct-base-clash-base.json | 6 ++++++
tests/qapi-schema/struct-base-clash-base.out | 5 +++++
tests/qapi-schema/union-clash-data.err | 0
tests/qapi-schema/union-clash-data.exit | 1 +
tests/qapi-schema/union-clash-data.json | 4 ++++
tests/qapi-schema/union-clash-data.out | 6 ++++++
tests/qapi-schema/union-clash-members.err | 1 +
tests/qapi-schema/union-clash-members.exit | 1 +
tests/qapi-schema/union-clash-members.json | 3 +++
tests/qapi-schema/union-clash-members.out | 0
tests/qapi-schema/union-clash-type.err | 16 ++++++++++++++++
tests/qapi-schema/union-clash-type.exit | 1 +
tests/qapi-schema/union-clash-type.json | 3 +++
tests/qapi-schema/union-clash-type.out | 0
40 files changed, 142 insertions(+), 5 deletions(-)
rename tests/qapi-schema/{flat-union-branch-clash.out => args-name-clash.err} (100%)
create mode 100644 tests/qapi-schema/args-name-clash.exit
create mode 100644 tests/qapi-schema/args-name-clash.json
create mode 100644 tests/qapi-schema/args-name-clash.out
delete mode 100644 tests/qapi-schema/flat-union-branch-clash.err
create mode 100644 tests/qapi-schema/flat-union-clash-branch.err
create mode 100644 tests/qapi-schema/flat-union-clash-branch.exit
create mode 100644 tests/qapi-schema/flat-union-clash-branch.json
create mode 100644 tests/qapi-schema/flat-union-clash-branch.out
create mode 100644 tests/qapi-schema/flat-union-clash-member.err
rename tests/qapi-schema/{flat-union-branch-clash.exit => flat-union-clash-member.exit} (100%)
rename tests/qapi-schema/{flat-union-branch-clash.json => flat-union-clash-member.json} (85%)
create mode 100644 tests/qapi-schema/flat-union-clash-member.out
create mode 100644 tests/qapi-schema/flat-union-clash-type.err
create mode 100644 tests/qapi-schema/flat-union-clash-type.exit
create mode 100644 tests/qapi-schema/flat-union-clash-type.json
create mode 100644 tests/qapi-schema/flat-union-clash-type.out
create mode 100644 tests/qapi-schema/flat-union-cycle.err
create mode 100644 tests/qapi-schema/flat-union-cycle.exit
create mode 100644 tests/qapi-schema/flat-union-cycle.json
create mode 100644 tests/qapi-schema/flat-union-cycle.out
create mode 100644 tests/qapi-schema/struct-base-clash-base.err
create mode 100644 tests/qapi-schema/struct-base-clash-base.exit
create mode 100644 tests/qapi-schema/struct-base-clash-base.json
create mode 100644 tests/qapi-schema/struct-base-clash-base.out
create mode 100644 tests/qapi-schema/union-clash-data.err
create mode 100644 tests/qapi-schema/union-clash-data.exit
create mode 100644 tests/qapi-schema/union-clash-data.json
create mode 100644 tests/qapi-schema/union-clash-data.out
create mode 100644 tests/qapi-schema/union-clash-members.err
create mode 100644 tests/qapi-schema/union-clash-members.exit
create mode 100644 tests/qapi-schema/union-clash-members.json
create mode 100644 tests/qapi-schema/union-clash-members.out
create mode 100644 tests/qapi-schema/union-clash-type.err
create mode 100644 tests/qapi-schema/union-clash-type.exit
create mode 100644 tests/qapi-schema/union-clash-type.json
create mode 100644 tests/qapi-schema/union-clash-type.out
diff --git a/tests/Makefile b/tests/Makefile
index 6fd5885..d43b8e9 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -238,6 +238,7 @@ qapi-schema += args-invalid.json
qapi-schema += args-member-array-bad.json
qapi-schema += args-member-array.json
qapi-schema += args-member-unknown.json
+qapi-schema += args-name-clash.json
qapi-schema += args-union.json
qapi-schema += args-unknown.json
qapi-schema += bad-base.json
@@ -273,7 +274,10 @@ qapi-schema += flat-union-bad-base.json
qapi-schema += flat-union-bad-discriminator.json
qapi-schema += flat-union-base-any.json
qapi-schema += flat-union-base-union.json
-qapi-schema += flat-union-branch-clash.json
+qapi-schema += flat-union-clash-branch.json
+qapi-schema += flat-union-clash-member.json
+qapi-schema += flat-union-clash-type.json
+qapi-schema += flat-union-cycle.json
qapi-schema += flat-union-inline.json
qapi-schema += flat-union-int-branch.json
qapi-schema += flat-union-invalid-branch-key.json
@@ -315,6 +319,7 @@ qapi-schema += returns-dict.json
qapi-schema += returns-int.json
qapi-schema += returns-unknown.json
qapi-schema += returns-whitelist.json
+qapi-schema += struct-base-clash-base.json
qapi-schema += struct-base-clash-deep.json
qapi-schema += struct-base-clash.json
qapi-schema += struct-data-invalid.json
@@ -328,6 +333,9 @@ qapi-schema += unclosed-string.json
qapi-schema += unicode-str.json
qapi-schema += union-bad-branch.json
qapi-schema += union-base-no-discriminator.json
+qapi-schema += union-clash-data.json
+qapi-schema += union-clash-members.json
+qapi-schema += union-clash-type.json
qapi-schema += union-invalid-base.json
qapi-schema += union-max.json
qapi-schema += union-optional-branch.json
diff --git a/tests/qapi-schema/flat-union-branch-clash.out b/tests/qapi-schema/args-name-clash.err
similarity index 100%
rename from tests/qapi-schema/flat-union-branch-clash.out
rename to tests/qapi-schema/args-name-clash.err
diff --git a/tests/qapi-schema/args-name-clash.exit b/tests/qapi-schema/args-name-clash.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/args-name-clash.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/args-name-clash.json b/tests/qapi-schema/args-name-clash.json
new file mode 100644
index 0000000..19bf792
--- /dev/null
+++ b/tests/qapi-schema/args-name-clash.json
@@ -0,0 +1,2 @@
+# FIXME - we should reject data with members that clash when mapped to C names
+{ 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }
diff --git a/tests/qapi-schema/args-name-clash.out b/tests/qapi-schema/args-name-clash.out
new file mode 100644
index 0000000..9b2f6e4
--- /dev/null
+++ b/tests/qapi-schema/args-name-clash.out
@@ -0,0 +1,6 @@
+object :empty
+object :obj-oops-arg
+ member a-b: str optional=False
+ member a_b: str optional=False
+command oops :obj-oops-arg -> None
+ gen=True success_response=True
diff --git a/tests/qapi-schema/flat-union-branch-clash.err b/tests/qapi-schema/flat-union-branch-clash.err
deleted file mode 100644
index f112766..0000000
--- a/tests/qapi-schema/flat-union-branch-clash.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/flat-union-branch-clash.json:10: Member name 'name' of branch 'value1' clashes with base 'Base'
diff --git a/tests/qapi-schema/flat-union-clash-branch.err b/tests/qapi-schema/flat-union-clash-branch.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/flat-union-clash-branch.exit b/tests/qapi-schema/flat-union-clash-branch.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/flat-union-clash-branch.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/flat-union-clash-branch.json b/tests/qapi-schema/flat-union-clash-branch.json
new file mode 100644
index 0000000..cda3af5
--- /dev/null
+++ b/tests/qapi-schema/flat-union-clash-branch.json
@@ -0,0 +1,15 @@
+# FIXME: this parses, but then fails to compile due to a duplicate 'c_d'
+# we should either detect the collision or allow this to compile
+{ 'enum': 'TestEnum',
+ 'data': [ 'base', 'c-d' ] }
+{ 'struct': 'Base',
+ 'data': { 'enum1': 'TestEnum', '*c_d': 'str' } }
+{ 'struct': 'Branch1',
+ 'data': { 'string': 'str' } }
+{ 'struct': 'Branch2',
+ 'data': { 'value': 'int' } }
+{ 'union': 'TestUnion',
+ 'base': 'Base',
+ 'discriminator': 'enum1',
+ 'data': { 'base': 'Branch1',
+ 'c-d': 'Branch2' } }
diff --git a/tests/qapi-schema/flat-union-clash-branch.out b/tests/qapi-schema/flat-union-clash-branch.out
new file mode 100644
index 0000000..8e0da73
--- /dev/null
+++ b/tests/qapi-schema/flat-union-clash-branch.out
@@ -0,0 +1,14 @@
+object :empty
+object Base
+ member enum1: TestEnum optional=False
+ member c_d: str optional=True
+object Branch1
+ member string: str optional=False
+object Branch2
+ member value: int optional=False
+enum TestEnum ['base', 'c-d']
+object TestUnion
+ base Base
+ tag enum1
+ case base: Branch1
+ case c-d: Branch2
diff --git a/tests/qapi-schema/flat-union-clash-member.err b/tests/qapi-schema/flat-union-clash-member.err
new file mode 100644
index 0000000..152d402
--- /dev/null
+++ b/tests/qapi-schema/flat-union-clash-member.err
@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-clash-member.json:10: Member name 'name' of branch 'value1' clashes with base 'Base'
diff --git a/tests/qapi-schema/flat-union-branch-clash.exit b/tests/qapi-schema/flat-union-clash-member.exit
similarity index 100%
rename from tests/qapi-schema/flat-union-branch-clash.exit
rename to tests/qapi-schema/flat-union-clash-member.exit
diff --git a/tests/qapi-schema/flat-union-branch-clash.json b/tests/qapi-schema/flat-union-clash-member.json
similarity index 85%
rename from tests/qapi-schema/flat-union-branch-clash.json
rename to tests/qapi-schema/flat-union-clash-member.json
index 8fb054f..3d7e6c6 100644
--- a/tests/qapi-schema/flat-union-branch-clash.json
+++ b/tests/qapi-schema/flat-union-clash-member.json
@@ -1,4 +1,4 @@
-# we check for no duplicate keys between branches and base
+# we check for no duplicate keys between branch members and base
{ 'enum': 'TestEnum',
'data': [ 'value1', 'value2' ] }
{ 'struct': 'Base',
diff --git a/tests/qapi-schema/flat-union-clash-member.out b/tests/qapi-schema/flat-union-clash-member.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err
new file mode 100644
index 0000000..6e64d1d
--- /dev/null
+++ b/tests/qapi-schema/flat-union-clash-type.err
@@ -0,0 +1,16 @@
+Traceback (most recent call last):
+ File "tests/qapi-schema/test-qapi.py", line 55, in <module>
+ schema = QAPISchema(sys.argv[1])
+ File "scripts/qapi.py", line 1116, in __init__
+ self.check()
+ File "scripts/qapi.py", line 1299, in check
+ ent.check(self)
+ File "scripts/qapi.py", line 962, in check
+ self.variants.check(schema, members, seen)
+ File "scripts/qapi.py", line 1024, in check
+ v.check(schema, self.tag_member.type, vseen)
+ File "scripts/qapi.py", line 1032, in check
+ QAPISchemaObjectTypeMember.check(self, schema, [], seen)
+ File "scripts/qapi.py", line 994, in check
+ assert self.name not in seen
+AssertionError
diff --git a/tests/qapi-schema/flat-union-clash-type.exit b/tests/qapi-schema/flat-union-clash-type.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/flat-union-clash-type.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/flat-union-clash-type.json b/tests/qapi-schema/flat-union-clash-type.json
new file mode 100644
index 0000000..87e5fd8
--- /dev/null
+++ b/tests/qapi-schema/flat-union-clash-type.json
@@ -0,0 +1,11 @@
+# FIXME: detect this collision, rather than triggering an assertion failure
+{ 'enum': 'TestEnum',
+ 'data': [ 'type' ] }
+{ 'struct': 'Base',
+ 'data': { 'type': 'TestEnum' } }
+{ 'struct': 'Branch1',
+ 'data': { 'string': 'str' } }
+{ 'union': 'TestUnion',
+ 'base': 'Base',
+ 'discriminator': 'type',
+ 'data': { 'type': 'Branch1' } }
diff --git a/tests/qapi-schema/flat-union-clash-type.out b/tests/qapi-schema/flat-union-clash-type.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/flat-union-cycle.err b/tests/qapi-schema/flat-union-cycle.err
new file mode 100644
index 0000000..d4f2a09
--- /dev/null
+++ b/tests/qapi-schema/flat-union-cycle.err
@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-cycle.json:6: Base 'Union' is not a valid struct
diff --git a/tests/qapi-schema/flat-union-cycle.exit b/tests/qapi-schema/flat-union-cycle.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/flat-union-cycle.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/flat-union-cycle.json b/tests/qapi-schema/flat-union-cycle.json
new file mode 100644
index 0000000..7414277
--- /dev/null
+++ b/tests/qapi-schema/flat-union-cycle.json
@@ -0,0 +1,7 @@
+# ensure that we have a sane error message for attempts at self-inheritance
+# (this test currently fails because we don't permit a union base, but
+# even if we relax things to allow that in the future, this should still fail)
+{ 'enum': 'Enum', 'data': [ 'okay' ] }
+{ 'struct': 'Okay', 'data': { 'int': 'int' } }
+{ 'union': 'Union', 'base': 'Union', 'discriminator': 'switch',
+ 'data': { 'okay': 'Okay' } }
diff --git a/tests/qapi-schema/flat-union-cycle.out b/tests/qapi-schema/flat-union-cycle.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 6897a6e..c511338 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -32,11 +32,14 @@
'dict1': 'UserDefTwoDict' } }
# for testing unions
+# Among other things, test that a name collision between branches does
+# not cause any problems (since only one branch can be in use at a time),
+# by intentionally using two branches that both have a C member 'a_b'
{ 'struct': 'UserDefA',
- 'data': { 'boolean': 'bool' } }
+ 'data': { 'boolean': 'bool', '*a_b': 'int' } }
{ 'struct': 'UserDefB',
- 'data': { 'intb': 'int' } }
+ 'data': { 'intb': 'int', '*a-b': 'bool' } }
{ 'union': 'UserDefFlatUnion',
'base': 'UserDefUnionBase', # intentional forward reference
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 1f6e858..28a0b3c 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -71,6 +71,7 @@ enum QEnumTwo ['value1', 'value2']
prefix QENUM_TWO
object UserDefA
member boolean: bool optional=False
+ member a_b: int optional=True
alternate UserDefAlternate
case uda: UserDefA
case s: str
@@ -78,6 +79,7 @@ alternate UserDefAlternate
enum UserDefAlternateKind ['uda', 's', 'i']
object UserDefB
member intb: int optional=False
+ member a-b: bool optional=True
object UserDefC
member string1: str optional=False
member string2: str optional=False
diff --git a/tests/qapi-schema/struct-base-clash-base.err b/tests/qapi-schema/struct-base-clash-base.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/struct-base-clash-base.exit b/tests/qapi-schema/struct-base-clash-base.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/struct-base-clash-base.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/struct-base-clash-base.json b/tests/qapi-schema/struct-base-clash-base.json
new file mode 100644
index 0000000..fb25128
--- /dev/null
+++ b/tests/qapi-schema/struct-base-clash-base.json
@@ -0,0 +1,6 @@
+# FIXME: this parses, but then fails to compile due to a duplicate 'base'
+# we should either detect the collision or allow this to compile
+{ 'struct': 'Base', 'data': {} }
+{ 'struct': 'Sub',
+ 'base': 'Base',
+ 'data': { 'base': 'str' } }
diff --git a/tests/qapi-schema/struct-base-clash-base.out b/tests/qapi-schema/struct-base-clash-base.out
new file mode 100644
index 0000000..e69a416
--- /dev/null
+++ b/tests/qapi-schema/struct-base-clash-base.out
@@ -0,0 +1,5 @@
+object :empty
+object Base
+object Sub
+ base Base
+ member base: str optional=False
diff --git a/tests/qapi-schema/union-clash-data.err b/tests/qapi-schema/union-clash-data.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/union-clash-data.exit b/tests/qapi-schema/union-clash-data.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/union-clash-data.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/union-clash-data.json b/tests/qapi-schema/union-clash-data.json
new file mode 100644
index 0000000..322f61b
--- /dev/null
+++ b/tests/qapi-schema/union-clash-data.json
@@ -0,0 +1,4 @@
+# FIXME: this parses, but then fails to compile due to a duplicate 'data'
+# we should either detect the collision or allow this to compile
+{ 'union': 'TestUnion',
+ 'data': { 'data': 'int' } }
diff --git a/tests/qapi-schema/union-clash-data.out b/tests/qapi-schema/union-clash-data.out
new file mode 100644
index 0000000..6277239
--- /dev/null
+++ b/tests/qapi-schema/union-clash-data.out
@@ -0,0 +1,6 @@
+object :empty
+object :obj-int-wrapper
+ member data: int optional=False
+object TestUnion
+ case data: :obj-int-wrapper
+enum TestUnionKind ['data']
diff --git a/tests/qapi-schema/union-clash-members.err b/tests/qapi-schema/union-clash-members.err
new file mode 100644
index 0000000..b77aeb8
--- /dev/null
+++ b/tests/qapi-schema/union-clash-members.err
@@ -0,0 +1 @@
+tests/qapi-schema/union-clash-members.json:2: Union 'TestUnion' member 'a_b' clashes with 'a-b'
diff --git a/tests/qapi-schema/union-clash-members.exit b/tests/qapi-schema/union-clash-members.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/union-clash-members.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/union-clash-members.json b/tests/qapi-schema/union-clash-members.json
new file mode 100644
index 0000000..0393ed8
--- /dev/null
+++ b/tests/qapi-schema/union-clash-members.json
@@ -0,0 +1,3 @@
+# we reject unions where branch names clash when mapped to C
+{ 'union': 'TestUnion',
+ 'data': { 'a-b': 'int', 'a_b': 'str' } }
diff --git a/tests/qapi-schema/union-clash-members.out b/tests/qapi-schema/union-clash-members.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/union-clash-type.err b/tests/qapi-schema/union-clash-type.err
new file mode 100644
index 0000000..6e64d1d
--- /dev/null
+++ b/tests/qapi-schema/union-clash-type.err
@@ -0,0 +1,16 @@
+Traceback (most recent call last):
+ File "tests/qapi-schema/test-qapi.py", line 55, in <module>
+ schema = QAPISchema(sys.argv[1])
+ File "scripts/qapi.py", line 1116, in __init__
+ self.check()
+ File "scripts/qapi.py", line 1299, in check
+ ent.check(self)
+ File "scripts/qapi.py", line 962, in check
+ self.variants.check(schema, members, seen)
+ File "scripts/qapi.py", line 1024, in check
+ v.check(schema, self.tag_member.type, vseen)
+ File "scripts/qapi.py", line 1032, in check
+ QAPISchemaObjectTypeMember.check(self, schema, [], seen)
+ File "scripts/qapi.py", line 994, in check
+ assert self.name not in seen
+AssertionError
diff --git a/tests/qapi-schema/union-clash-type.exit b/tests/qapi-schema/union-clash-type.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/union-clash-type.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/union-clash-type.json b/tests/qapi-schema/union-clash-type.json
new file mode 100644
index 0000000..2294f4f
--- /dev/null
+++ b/tests/qapi-schema/union-clash-type.json
@@ -0,0 +1,3 @@
+# FIXME: detect this collision, rather than triggering an assertion failure
+{ 'union': 'TestUnion',
+ 'data': { 'kind': 'int', 'type': 'str' } }
diff --git a/tests/qapi-schema/union-clash-type.out b/tests/qapi-schema/union-clash-type.out
new file mode 100644
index 0000000..e69de29
--
2.4.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 05/16] qapi: Test for various name collisions
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 05/16] qapi: Test for various name collisions Eric Blake
@ 2015-09-29 12:33 ` Markus Armbruster
2015-09-29 14:11 ` Eric Blake
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2015-09-29 12:33 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Expose some weaknesses in the generator: we don't always forbid
> the generation of structs that contain multiple members that map
> to the same C name.
C or QMP name, perhaps?
> This has already been marked FIXME in
> qapi.py in commit d90675f, but having more tests will make sure
> future patches produce desired behavior. This patch focuses on
> C struct members, and does not consider collisions between commands
> and events (affecting C function names), or even on collisions
> between generated C type names with user type names (for things
> like automatic FOOList struct representing array types).
>
> There are two types of struct collisions we want to catch:
> 1) Collision that would require the QMP user to specify the
> same key more than once inside a single JSON {} object (we
> already prevent duplicate keys within a single type, so this
> is triggered when merging multiple types into a single JSON
> object via base classes or flat unions)
I understand what you want to say here, but find it a bit awkward.
Perhaps:
1) Collision between two keys in a JSON object. qapi.py should prevent
that, but currently fails to do so for collisions between a type's
members, its base type's members, and its flat union variant members.
Could add pointers to the tests covering this type of collision.
> 2) Collision between two members of the C struct that is
> generated for a given qapi type:
We generally spell it QAPI.
> a) a collision occurs from two qapi names mapping to the
> same C name
> b) the collision is with a C name used for another purpose
> (we already fixed 'kind' in commit 0f61af3e, and one use of
> 'base' in commit 1e6c1616 for unions, but we have another
> use of 'base' in structs, and still collide with 'data' in
> unions, not to mention the tag 'type'/'kind' and the various
> tag values).
I'm not sure this makes sense to readers who aren't already familiar
with our name clashing issues.
Perhaps:
2) Collision between members of the C struct that is generated for a
given qapi type. Two cases:
a) Multiple QAPI names map to the same C name.
b) A QAPI name maps to a C name that is used for another purpose.
We already fixed such cases in commit 0f61af3e and commit 1e6c1616,
but more cases are left.
Avoids getting bogged down in details prematurely.
Could add pointers to the tests covering each type of collision.
> Oddly enough, while commit 1e6c1616 fixed a type 2b collision
> with 'base',
What collided with 'base' before 1e6c1616 and not after?
> it introduced a different collision that was not
> previously possible: now that the base members are no longer
> boxed behind 'base', they can collide with the enum tag members
> (used as member names within the embedded C union).
>
> Ultimately, if we need to have a flat union where an enum
> value clashes with a base member name, we could change the
Suggest "an enum tag member", because this is confusing enough without
using multiple names for the same thing :)
> generator to use something like '_tag_value' instead of
> 'value' for the C name of union members;
Or wrap the base in a struct, or give the union member a name.
> but until such a
> need arises, it will probably be easier to just forbid these
> collisions.
Again, I'm not sure this makes sense to readers who aren't already
familiar with our name clashing issues.
Do you fix any of this later in your series? If yes, you could punt
detailed bug descriptions to the patches that fixes them.
> Some of these collisions are already caught in the old-style
> parser checks, but ultimately we want all collisions to be
> caught in the new-style QAPISchema*.check() methods.
>
> Some of these negative tests will be deleted later, and positive
> tests added to qapi-schema-test.json in their place, when the
> generator code is reworked so that a particular collision no
> longer causes failed code generation.
>
> [git rename detection may be a bit confused by the actions of
> this commit, since it both creates and renames files that are
> equally small and similar in content]
Why square brackets?
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: rework commit message, alter test names and comments to be
> more descriptive, repurpose flat-union-cycle to better match its
> name (and not duplicate what the renamed flat-union-clash-branch
> tests), add new tests (union-clash-type, flat-union-clash-type)
> that detect an assertion failures
> ---
> tests/Makefile | 10 +++++++++-
> .../{flat-union-branch-clash.out => args-name-clash.err} | 0
> tests/qapi-schema/args-name-clash.exit | 1 +
> tests/qapi-schema/args-name-clash.json | 2 ++
> tests/qapi-schema/args-name-clash.out | 6 ++++++
> tests/qapi-schema/flat-union-branch-clash.err | 1 -
> tests/qapi-schema/flat-union-clash-branch.err | 0
> tests/qapi-schema/flat-union-clash-branch.exit | 1 +
> tests/qapi-schema/flat-union-clash-branch.json | 15 +++++++++++++++
> tests/qapi-schema/flat-union-clash-branch.out | 14 ++++++++++++++
> tests/qapi-schema/flat-union-clash-member.err | 1 +
> ...on-branch-clash.exit => flat-union-clash-member.exit} | 0
> ...on-branch-clash.json => flat-union-clash-member.json} | 2 +-
> tests/qapi-schema/flat-union-clash-member.out | 0
> tests/qapi-schema/flat-union-clash-type.err | 16 ++++++++++++++++
> tests/qapi-schema/flat-union-clash-type.exit | 1 +
> tests/qapi-schema/flat-union-clash-type.json | 11 +++++++++++
> tests/qapi-schema/flat-union-clash-type.out | 0
> tests/qapi-schema/flat-union-cycle.err | 1 +
> tests/qapi-schema/flat-union-cycle.exit | 1 +
> tests/qapi-schema/flat-union-cycle.json | 7 +++++++
> tests/qapi-schema/flat-union-cycle.out | 0
> tests/qapi-schema/qapi-schema-test.json | 7 +++++--
> tests/qapi-schema/qapi-schema-test.out | 2 ++
> tests/qapi-schema/struct-base-clash-base.err | 0
> tests/qapi-schema/struct-base-clash-base.exit | 1 +
> tests/qapi-schema/struct-base-clash-base.json | 6 ++++++
> tests/qapi-schema/struct-base-clash-base.out | 5 +++++
> tests/qapi-schema/union-clash-data.err | 0
> tests/qapi-schema/union-clash-data.exit | 1 +
> tests/qapi-schema/union-clash-data.json | 4 ++++
> tests/qapi-schema/union-clash-data.out | 6 ++++++
> tests/qapi-schema/union-clash-members.err | 1 +
> tests/qapi-schema/union-clash-members.exit | 1 +
> tests/qapi-schema/union-clash-members.json | 3 +++
> tests/qapi-schema/union-clash-members.out | 0
> tests/qapi-schema/union-clash-type.err | 16 ++++++++++++++++
> tests/qapi-schema/union-clash-type.exit | 1 +
> tests/qapi-schema/union-clash-type.json | 3 +++
> tests/qapi-schema/union-clash-type.out | 0
> 40 files changed, 142 insertions(+), 5 deletions(-)
> rename tests/qapi-schema/{flat-union-branch-clash.out => args-name-clash.err} (100%)
> create mode 100644 tests/qapi-schema/args-name-clash.exit
> create mode 100644 tests/qapi-schema/args-name-clash.json
> create mode 100644 tests/qapi-schema/args-name-clash.out
> delete mode 100644 tests/qapi-schema/flat-union-branch-clash.err
> create mode 100644 tests/qapi-schema/flat-union-clash-branch.err
> create mode 100644 tests/qapi-schema/flat-union-clash-branch.exit
> create mode 100644 tests/qapi-schema/flat-union-clash-branch.json
> create mode 100644 tests/qapi-schema/flat-union-clash-branch.out
> create mode 100644 tests/qapi-schema/flat-union-clash-member.err
> rename tests/qapi-schema/{flat-union-branch-clash.exit => flat-union-clash-member.exit} (100%)
> rename tests/qapi-schema/{flat-union-branch-clash.json => flat-union-clash-member.json} (85%)
> create mode 100644 tests/qapi-schema/flat-union-clash-member.out
> create mode 100644 tests/qapi-schema/flat-union-clash-type.err
> create mode 100644 tests/qapi-schema/flat-union-clash-type.exit
> create mode 100644 tests/qapi-schema/flat-union-clash-type.json
> create mode 100644 tests/qapi-schema/flat-union-clash-type.out
> create mode 100644 tests/qapi-schema/flat-union-cycle.err
> create mode 100644 tests/qapi-schema/flat-union-cycle.exit
> create mode 100644 tests/qapi-schema/flat-union-cycle.json
> create mode 100644 tests/qapi-schema/flat-union-cycle.out
> create mode 100644 tests/qapi-schema/struct-base-clash-base.err
> create mode 100644 tests/qapi-schema/struct-base-clash-base.exit
> create mode 100644 tests/qapi-schema/struct-base-clash-base.json
> create mode 100644 tests/qapi-schema/struct-base-clash-base.out
> create mode 100644 tests/qapi-schema/union-clash-data.err
> create mode 100644 tests/qapi-schema/union-clash-data.exit
> create mode 100644 tests/qapi-schema/union-clash-data.json
> create mode 100644 tests/qapi-schema/union-clash-data.out
> create mode 100644 tests/qapi-schema/union-clash-members.err
> create mode 100644 tests/qapi-schema/union-clash-members.exit
> create mode 100644 tests/qapi-schema/union-clash-members.json
> create mode 100644 tests/qapi-schema/union-clash-members.out
> create mode 100644 tests/qapi-schema/union-clash-type.err
> create mode 100644 tests/qapi-schema/union-clash-type.exit
> create mode 100644 tests/qapi-schema/union-clash-type.json
> create mode 100644 tests/qapi-schema/union-clash-type.out
>
> diff --git a/tests/Makefile b/tests/Makefile
> index 6fd5885..d43b8e9 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -238,6 +238,7 @@ qapi-schema += args-invalid.json
> qapi-schema += args-member-array-bad.json
> qapi-schema += args-member-array.json
> qapi-schema += args-member-unknown.json
> +qapi-schema += args-name-clash.json
> qapi-schema += args-union.json
> qapi-schema += args-unknown.json
> qapi-schema += bad-base.json
> @@ -273,7 +274,10 @@ qapi-schema += flat-union-bad-base.json
> qapi-schema += flat-union-bad-discriminator.json
> qapi-schema += flat-union-base-any.json
> qapi-schema += flat-union-base-union.json
> -qapi-schema += flat-union-branch-clash.json
> +qapi-schema += flat-union-clash-branch.json
> +qapi-schema += flat-union-clash-member.json
> +qapi-schema += flat-union-clash-type.json
> +qapi-schema += flat-union-cycle.json
> qapi-schema += flat-union-inline.json
> qapi-schema += flat-union-int-branch.json
> qapi-schema += flat-union-invalid-branch-key.json
> @@ -315,6 +319,7 @@ qapi-schema += returns-dict.json
> qapi-schema += returns-int.json
> qapi-schema += returns-unknown.json
> qapi-schema += returns-whitelist.json
> +qapi-schema += struct-base-clash-base.json
> qapi-schema += struct-base-clash-deep.json
> qapi-schema += struct-base-clash.json
> qapi-schema += struct-data-invalid.json
> @@ -328,6 +333,9 @@ qapi-schema += unclosed-string.json
> qapi-schema += unicode-str.json
> qapi-schema += union-bad-branch.json
> qapi-schema += union-base-no-discriminator.json
> +qapi-schema += union-clash-data.json
> +qapi-schema += union-clash-members.json
> +qapi-schema += union-clash-type.json
> qapi-schema += union-invalid-base.json
> qapi-schema += union-max.json
> qapi-schema += union-optional-branch.json
> diff --git a/tests/qapi-schema/flat-union-branch-clash.out b/tests/qapi-schema/args-name-clash.err
> similarity index 100%
> rename from tests/qapi-schema/flat-union-branch-clash.out
> rename to tests/qapi-schema/args-name-clash.err
> diff --git a/tests/qapi-schema/args-name-clash.exit b/tests/qapi-schema/args-name-clash.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/args-name-clash.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/args-name-clash.json b/tests/qapi-schema/args-name-clash.json
> new file mode 100644
> index 0000000..19bf792
> --- /dev/null
> +++ b/tests/qapi-schema/args-name-clash.json
> @@ -0,0 +1,2 @@
> +# FIXME - we should reject data with members that clash when mapped to C names
> +{ 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }
Here, the collision is fairly obvious. We could spell it out anyway:
# Multiple members mapping to the same C identifier: both 'a-b' and
# 'a_b' map to a_b'
# FIXME they clash in generated C, should reject them cleanly instead.
Unfortunately, the test doesn't actually make the clash visible. Same
for all the other tests that produce clashes in C or QMP without
actually upsetting the generator. Oh well, good enough.
> diff --git a/tests/qapi-schema/args-name-clash.out b/tests/qapi-schema/args-name-clash.out
> new file mode 100644
> index 0000000..9b2f6e4
> --- /dev/null
> +++ b/tests/qapi-schema/args-name-clash.out
> @@ -0,0 +1,6 @@
> +object :empty
> +object :obj-oops-arg
> + member a-b: str optional=False
> + member a_b: str optional=False
> +command oops :obj-oops-arg -> None
> + gen=True success_response=True
> diff --git a/tests/qapi-schema/flat-union-branch-clash.err b/tests/qapi-schema/flat-union-branch-clash.err
> deleted file mode 100644
> index f112766..0000000
> --- a/tests/qapi-schema/flat-union-branch-clash.err
> +++ /dev/null
> @@ -1 +0,0 @@
> -tests/qapi-schema/flat-union-branch-clash.json:10: Member name 'name' of branch 'value1' clashes with base 'Base'
> diff --git a/tests/qapi-schema/flat-union-clash-branch.err b/tests/qapi-schema/flat-union-clash-branch.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/flat-union-clash-branch.exit b/tests/qapi-schema/flat-union-clash-branch.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-branch.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/flat-union-clash-branch.json b/tests/qapi-schema/flat-union-clash-branch.json
> new file mode 100644
> index 0000000..cda3af5
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-branch.json
> @@ -0,0 +1,15 @@
> +# FIXME: this parses, but then fails to compile due to a duplicate 'c_d'
> +# we should either detect the collision or allow this to compile
Here, the collision is less obvious, mostly because it gets lost in the
scaffolding. Let's spell it out:
# Base members and tag enum members mapping to the same C identifier:
# both 'c-d' and 'c_d' map to c_d'
# FIXME they clash in generated C, should reject them cleanly instead.
> +{ 'enum': 'TestEnum',
> + 'data': [ 'base', 'c-d' ] }
> +{ 'struct': 'Base',
> + 'data': { 'enum1': 'TestEnum', '*c_d': 'str' } }
> +{ 'struct': 'Branch1',
> + 'data': { 'string': 'str' } }
> +{ 'struct': 'Branch2',
> + 'data': { 'value': 'int' } }
> +{ 'union': 'TestUnion',
> + 'base': 'Base',
> + 'discriminator': 'enum1',
> + 'data': { 'base': 'Branch1',
> + 'c-d': 'Branch2' } }
> diff --git a/tests/qapi-schema/flat-union-clash-branch.out b/tests/qapi-schema/flat-union-clash-branch.out
> new file mode 100644
> index 0000000..8e0da73
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-branch.out
> @@ -0,0 +1,14 @@
> +object :empty
> +object Base
> + member enum1: TestEnum optional=False
> + member c_d: str optional=True
> +object Branch1
> + member string: str optional=False
> +object Branch2
> + member value: int optional=False
> +enum TestEnum ['base', 'c-d']
> +object TestUnion
> + base Base
> + tag enum1
> + case base: Branch1
> + case c-d: Branch2
> diff --git a/tests/qapi-schema/flat-union-clash-member.err b/tests/qapi-schema/flat-union-clash-member.err
> new file mode 100644
> index 0000000..152d402
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-member.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/flat-union-clash-member.json:10: Member name 'name' of branch 'value1' clashes with base 'Base'
> diff --git a/tests/qapi-schema/flat-union-branch-clash.exit b/tests/qapi-schema/flat-union-clash-member.exit
> similarity index 100%
> rename from tests/qapi-schema/flat-union-branch-clash.exit
> rename to tests/qapi-schema/flat-union-clash-member.exit
> diff --git a/tests/qapi-schema/flat-union-branch-clash.json b/tests/qapi-schema/flat-union-clash-member.json
> similarity index 85%
> rename from tests/qapi-schema/flat-union-branch-clash.json
> rename to tests/qapi-schema/flat-union-clash-member.json
> index 8fb054f..3d7e6c6 100644
> --- a/tests/qapi-schema/flat-union-branch-clash.json
> +++ b/tests/qapi-schema/flat-union-clash-member.json
> @@ -1,4 +1,4 @@
> -# we check for no duplicate keys between branches and base
> +# we check for no duplicate keys between branch members and base
Spelling out the clashing members wouldn't hurt.
> { 'enum': 'TestEnum',
> 'data': [ 'value1', 'value2' ] }
> { 'struct': 'Base',
> diff --git a/tests/qapi-schema/flat-union-clash-member.out b/tests/qapi-schema/flat-union-clash-member.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err
> new file mode 100644
> index 0000000..6e64d1d
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-type.err
> @@ -0,0 +1,16 @@
> +Traceback (most recent call last):
> + File "tests/qapi-schema/test-qapi.py", line 55, in <module>
> + schema = QAPISchema(sys.argv[1])
> + File "scripts/qapi.py", line 1116, in __init__
> + self.check()
> + File "scripts/qapi.py", line 1299, in check
> + ent.check(self)
> + File "scripts/qapi.py", line 962, in check
> + self.variants.check(schema, members, seen)
> + File "scripts/qapi.py", line 1024, in check
> + v.check(schema, self.tag_member.type, vseen)
> + File "scripts/qapi.py", line 1032, in check
> + QAPISchemaObjectTypeMember.check(self, schema, [], seen)
> + File "scripts/qapi.py", line 994, in check
> + assert self.name not in seen
> +AssertionError
> diff --git a/tests/qapi-schema/flat-union-clash-type.exit b/tests/qapi-schema/flat-union-clash-type.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-type.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/flat-union-clash-type.json b/tests/qapi-schema/flat-union-clash-type.json
> new file mode 100644
> index 0000000..87e5fd8
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-clash-type.json
> @@ -0,0 +1,11 @@
> +# FIXME: detect this collision, rather than triggering an assertion failure
Here, the collision is unobvious. Could you spell it out, please?
> +{ 'enum': 'TestEnum',
> + 'data': [ 'type' ] }
> +{ 'struct': 'Base',
> + 'data': { 'type': 'TestEnum' } }
> +{ 'struct': 'Branch1',
> + 'data': { 'string': 'str' } }
> +{ 'union': 'TestUnion',
> + 'base': 'Base',
> + 'discriminator': 'type',
> + 'data': { 'type': 'Branch1' } }
> diff --git a/tests/qapi-schema/flat-union-clash-type.out b/tests/qapi-schema/flat-union-clash-type.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/flat-union-cycle.err b/tests/qapi-schema/flat-union-cycle.err
> new file mode 100644
> index 0000000..d4f2a09
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-cycle.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/flat-union-cycle.json:6: Base 'Union' is not a valid struct
> diff --git a/tests/qapi-schema/flat-union-cycle.exit b/tests/qapi-schema/flat-union-cycle.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-cycle.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/flat-union-cycle.json b/tests/qapi-schema/flat-union-cycle.json
> new file mode 100644
> index 0000000..7414277
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-cycle.json
> @@ -0,0 +1,7 @@
> +# ensure that we have a sane error message for attempts at self-inheritance
> +# (this test currently fails because we don't permit a union base, but
> +# even if we relax things to allow that in the future, this should still fail)
Humor me: start sentences with a capital letter and end them with
punctuation. Headlines don't end with punctuation, but still start with
a capital letter.
> +{ 'enum': 'Enum', 'data': [ 'okay' ] }
> +{ 'struct': 'Okay', 'data': { 'int': 'int' } }
> +{ 'union': 'Union', 'base': 'Union', 'discriminator': 'switch',
> + 'data': { 'okay': 'Okay' } }
Currently tests the exact same thing as flat-union-base-union.json,
doesn't it?
> diff --git a/tests/qapi-schema/flat-union-cycle.out b/tests/qapi-schema/flat-union-cycle.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index 6897a6e..c511338 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -32,11 +32,14 @@
> 'dict1': 'UserDefTwoDict' } }
>
> # for testing unions
> +# Among other things, test that a name collision between branches does
> +# not cause any problems (since only one branch can be in use at a time),
> +# by intentionally using two branches that both have a C member 'a_b'
> { 'struct': 'UserDefA',
> - 'data': { 'boolean': 'bool' } }
> + 'data': { 'boolean': 'bool', '*a_b': 'int' } }
>
> { 'struct': 'UserDefB',
> - 'data': { 'intb': 'int' } }
> + 'data': { 'intb': 'int', '*a-b': 'bool' } }
>
> { 'union': 'UserDefFlatUnion',
> 'base': 'UserDefUnionBase', # intentional forward reference
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 1f6e858..28a0b3c 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -71,6 +71,7 @@ enum QEnumTwo ['value1', 'value2']
> prefix QENUM_TWO
> object UserDefA
> member boolean: bool optional=False
> + member a_b: int optional=True
> alternate UserDefAlternate
> case uda: UserDefA
> case s: str
> @@ -78,6 +79,7 @@ alternate UserDefAlternate
> enum UserDefAlternateKind ['uda', 's', 'i']
> object UserDefB
> member intb: int optional=False
> + member a-b: bool optional=True
> object UserDefC
> member string1: str optional=False
> member string2: str optional=False
> diff --git a/tests/qapi-schema/struct-base-clash-base.err b/tests/qapi-schema/struct-base-clash-base.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/struct-base-clash-base.exit b/tests/qapi-schema/struct-base-clash-base.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash-base.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/struct-base-clash-base.json b/tests/qapi-schema/struct-base-clash-base.json
> new file mode 100644
> index 0000000..fb25128
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash-base.json
> @@ -0,0 +1,6 @@
> +# FIXME: this parses, but then fails to compile due to a duplicate 'base'
> +# we should either detect the collision or allow this to compile
Suggest:
# Struct type member 'base'
# FIXME clashes in generated C, should either reject 'base' cleanly or
# make it work.
> +{ 'struct': 'Base', 'data': {} }
> +{ 'struct': 'Sub',
> + 'base': 'Base',
> + 'data': { 'base': 'str' } }
> diff --git a/tests/qapi-schema/struct-base-clash-base.out b/tests/qapi-schema/struct-base-clash-base.out
> new file mode 100644
> index 0000000..e69a416
> --- /dev/null
> +++ b/tests/qapi-schema/struct-base-clash-base.out
> @@ -0,0 +1,5 @@
> +object :empty
> +object Base
> +object Sub
> + base Base
> + member base: str optional=False
> diff --git a/tests/qapi-schema/union-clash-data.err b/tests/qapi-schema/union-clash-data.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/union-clash-data.exit b/tests/qapi-schema/union-clash-data.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-data.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/union-clash-data.json b/tests/qapi-schema/union-clash-data.json
> new file mode 100644
> index 0000000..322f61b
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-data.json
> @@ -0,0 +1,4 @@
> +# FIXME: this parses, but then fails to compile due to a duplicate 'data'
> +# we should either detect the collision or allow this to compile
Suggest:
# Union tag member 'data'
# FIXME clashes in generated C, should either reject 'data' cleanly or
# make it work.
> +{ 'union': 'TestUnion',
> + 'data': { 'data': 'int' } }
> diff --git a/tests/qapi-schema/union-clash-data.out b/tests/qapi-schema/union-clash-data.out
> new file mode 100644
> index 0000000..6277239
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-data.out
> @@ -0,0 +1,6 @@
> +object :empty
> +object :obj-int-wrapper
> + member data: int optional=False
> +object TestUnion
> + case data: :obj-int-wrapper
> +enum TestUnionKind ['data']
> diff --git a/tests/qapi-schema/union-clash-members.err b/tests/qapi-schema/union-clash-members.err
> new file mode 100644
> index 0000000..b77aeb8
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-members.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/union-clash-members.json:2: Union 'TestUnion' member 'a_b' clashes with 'a-b'
> diff --git a/tests/qapi-schema/union-clash-members.exit b/tests/qapi-schema/union-clash-members.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-members.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/union-clash-members.json b/tests/qapi-schema/union-clash-members.json
> new file mode 100644
> index 0000000..0393ed8
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-members.json
> @@ -0,0 +1,3 @@
> +# we reject unions where branch names clash when mapped to C
> +{ 'union': 'TestUnion',
> + 'data': { 'a-b': 'int', 'a_b': 'str' } }
> diff --git a/tests/qapi-schema/union-clash-members.out b/tests/qapi-schema/union-clash-members.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/union-clash-type.err b/tests/qapi-schema/union-clash-type.err
> new file mode 100644
> index 0000000..6e64d1d
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-type.err
> @@ -0,0 +1,16 @@
> +Traceback (most recent call last):
> + File "tests/qapi-schema/test-qapi.py", line 55, in <module>
> + schema = QAPISchema(sys.argv[1])
> + File "scripts/qapi.py", line 1116, in __init__
> + self.check()
> + File "scripts/qapi.py", line 1299, in check
> + ent.check(self)
> + File "scripts/qapi.py", line 962, in check
> + self.variants.check(schema, members, seen)
> + File "scripts/qapi.py", line 1024, in check
> + v.check(schema, self.tag_member.type, vseen)
> + File "scripts/qapi.py", line 1032, in check
> + QAPISchemaObjectTypeMember.check(self, schema, [], seen)
> + File "scripts/qapi.py", line 994, in check
> + assert self.name not in seen
> +AssertionError
> diff --git a/tests/qapi-schema/union-clash-type.exit b/tests/qapi-schema/union-clash-type.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-type.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/union-clash-type.json b/tests/qapi-schema/union-clash-type.json
> new file mode 100644
> index 0000000..2294f4f
> --- /dev/null
> +++ b/tests/qapi-schema/union-clash-type.json
> @@ -0,0 +1,3 @@
> +# FIXME: detect this collision, rather than triggering an assertion failure
Could you spell out the collision?
> +{ 'union': 'TestUnion',
> + 'data': { 'kind': 'int', 'type': 'str' } }
> diff --git a/tests/qapi-schema/union-clash-type.out b/tests/qapi-schema/union-clash-type.out
> new file mode 100644
> index 0000000..e69de29
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 05/16] qapi: Test for various name collisions
2015-09-29 12:33 ` Markus Armbruster
@ 2015-09-29 14:11 ` Eric Blake
0 siblings, 0 replies; 39+ messages in thread
From: Eric Blake @ 2015-09-29 14:11 UTC (permalink / raw)
To: Markus Armbruster; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 4967 bytes --]
On 09/29/2015 06:33 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Expose some weaknesses in the generator: we don't always forbid
>> the generation of structs that contain multiple members that map
>> to the same C name.
>
> C or QMP name, perhaps?
Lots of good advice for an improved commit message and test comments;
looks like I'll do a v7 respin to address them.
>
>> Oddly enough, while commit 1e6c1616 fixed a type 2b collision
>> with 'base',
>
> What collided with 'base' before 1e6c1616 and not after?
Uggh. I wrote that comment late at night, and it shows. I was mixing up
struct bases (which are boxed behind a 'FOO *base' member) and flat
unions; but flat union base classes have always been expressed on a
per-member basis. I'll either drop this paragraph entirely, or come up
with a real example (about the only one that might be left is when we
dropped the mis-use of 'kind', we were then introducing the possibility
of collision with the 'discriminator':'name' - and that's one of the two
tests added in this commit where we assert).
>
>> it introduced a different collision that was not
>> previously possible: now that the base members are no longer
>> boxed behind 'base', they can collide with the enum tag members
>> (used as member names within the embedded C union).
>>
>> Ultimately, if we need to have a flat union where an enum
>> value clashes with a base member name, we could change the
>
> Suggest "an enum tag member", because this is confusing enough without
> using multiple names for the same thing :)
>
>> generator to use something like '_tag_value' instead of
>> 'value' for the C name of union members;
>
> Or wrap the base in a struct, or give the union member a name.
Giving the union member a name might be the least amount of typing, but
would still touch quite a bit of code if we have to do it.
>
>> but until such a
>> need arises, it will probably be easier to just forbid these
>> collisions.
>
> Again, I'm not sure this makes sense to readers who aren't already
> familiar with our name clashing issues.
>
> Do you fix any of this later in your series? If yes, you could punt
> detailed bug descriptions to the patches that fixes them.
Yes, I fix a number of the issues later on (although only the two
assertion failures get fixed in subset A; the rest of the fixups are in
the tail end of v5 which will become subset B).
>
>> Some of these collisions are already caught in the old-style
>> parser checks, but ultimately we want all collisions to be
>> caught in the new-style QAPISchema*.check() methods.
>>
>> Some of these negative tests will be deleted later, and positive
>> tests added to qapi-schema-test.json in their place, when the
>> generator code is reworked so that a particular collision no
>> longer causes failed code generation.
>>
>> [git rename detection may be a bit confused by the actions of
>> this commit, since it both creates and renames files that are
>> equally small and similar in content]
>
> Why square brackets?
>
To delineate that this paragraph is is unrelated to the contents of the
patch, but instead guides a reader to how to interpret the git diff of
the patch - if rename detection is enabled, git botches the rename
detection, doing things like:
>> ---
>> tests/Makefile | 10 +++++++++-
>> .../{flat-union-branch-clash.out => args-name-clash.err} | 0
claiming this file is a rename (in reality, I renamed
flat-union-branch-clash.out to flat-union-clash-branch.out; and .out
files never really rename to .err files other than the fact that both
happened to be empty).
We could drop it if desired, but I think it makes sense to keep the
comment in qemu.git (because 'git diff' will botch the rename detection
no matter how much in the future you are inspecting this patch).
>> +++ b/tests/qapi-schema/args-name-clash.json
>> @@ -0,0 +1,2 @@
>> +# FIXME - we should reject data with members that clash when mapped to C names
>> +{ 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }
>
> Here, the collision is fairly obvious. We could spell it out anyway:
>
> # Multiple members mapping to the same C identifier: both 'a-b' and
> # 'a_b' map to a_b'
> # FIXME they clash in generated C, should reject them cleanly instead.
>
> Unfortunately, the test doesn't actually make the clash visible. Same
> for all the other tests that produce clashes in C or QMP without
> actually upsetting the generator. Oh well, good enough.
And for every FIXME I add here, a later patch in the v5 series resolves
it (either by making it a parse error, or by removing the generated
collision).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 06/16] qapi: Avoid assertion failure on union 'type' collision
2015-09-29 3:27 [Qemu-devel] [PATCH v6 00/16] post-introspection cleanups, subset A Eric Blake
` (4 preceding siblings ...)
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 05/16] qapi: Test for various name collisions Eric Blake
@ 2015-09-29 3:27 ` Eric Blake
2015-09-29 12:36 ` Markus Armbruster
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 07/16] qapi: Add tests for empty unions Eric Blake
` (9 subsequent siblings)
15 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2015-09-29 3:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
The previous commit added two tests that triggered an assertion
failure. It's fairly straightforward to avoid the failure by
just outright forbidding the collision between a union's tag
values and its discriminator name (including the implicit name
'kind' supplied for simple unions [*]). Ultimately, we'd like
to move the collision detection into QAPISchema*.check(), but
for now it is easier just to enhance the existing checks.
[*] Of course, down the road, we have plans to rename the simple
union tag name to 'type' to match the QMP wire name, but the
idea of the collision will still be present even then.
Technically, we could avoid the collision by naming the C union
members representing each enum value as '_case_value' rather
than 'value'; but until we have an actual qapi client (and not
just our testsuite) that has a legitimate reason to match a
case label to the name of a QMP key and needs the name munging
to satisfy the compiler, it's easier to just reject the qapi
as invalid.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: new patch
---
scripts/qapi.py | 10 ++++++++--
tests/qapi-schema/flat-union-clash-type.err | 17 +----------------
tests/qapi-schema/union-clash-type.err | 17 +----------------
3 files changed, 10 insertions(+), 34 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 7e8a99d..ec450a6 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -544,7 +544,7 @@ def check_union(expr, expr_info):
base = expr.get('base')
discriminator = expr.get('discriminator')
members = expr['data']
- values = {'MAX': '(automatic)'}
+ values = {'MAX': '(automatic)', 'KIND': '(automatic)'}
# Two types of unions, determined by discriminator.
@@ -603,13 +603,19 @@ def check_union(expr, expr_info):
" of branch '%s'" % key)
# If the discriminator names an enum type, then all members
- # of 'data' must also be members of the enum type.
+ # of 'data' must also be members of the enum type, which in turn
+ # must not collide with the discriminator name.
if enum_define:
if key not in enum_define['enum_values']:
raise QAPIExprError(expr_info,
"Discriminator value '%s' is not found in "
"enum '%s'" %
(key, enum_define["enum_name"]))
+ if discriminator in enum_define['enum_values']:
+ raise QAPIExprError(expr_info,
+ "Discriminator name '%s' collides with "
+ "enum value in '%s'" %
+ (discriminator, enum_define["enum_name"]))
# Otherwise, check for conflicts in the generated enum
else:
diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err
index 6e64d1d..879beb8 100644
--- a/tests/qapi-schema/flat-union-clash-type.err
+++ b/tests/qapi-schema/flat-union-clash-type.err
@@ -1,16 +1 @@
-Traceback (most recent call last):
- File "tests/qapi-schema/test-qapi.py", line 55, in <module>
- schema = QAPISchema(sys.argv[1])
- File "scripts/qapi.py", line 1116, in __init__
- self.check()
- File "scripts/qapi.py", line 1299, in check
- ent.check(self)
- File "scripts/qapi.py", line 962, in check
- self.variants.check(schema, members, seen)
- File "scripts/qapi.py", line 1024, in check
- v.check(schema, self.tag_member.type, vseen)
- File "scripts/qapi.py", line 1032, in check
- QAPISchemaObjectTypeMember.check(self, schema, [], seen)
- File "scripts/qapi.py", line 994, in check
- assert self.name not in seen
-AssertionError
+tests/qapi-schema/flat-union-clash-type.json:8: Discriminator name 'type' collides with enum value in 'TestEnum'
diff --git a/tests/qapi-schema/union-clash-type.err b/tests/qapi-schema/union-clash-type.err
index 6e64d1d..773900b 100644
--- a/tests/qapi-schema/union-clash-type.err
+++ b/tests/qapi-schema/union-clash-type.err
@@ -1,16 +1 @@
-Traceback (most recent call last):
- File "tests/qapi-schema/test-qapi.py", line 55, in <module>
- schema = QAPISchema(sys.argv[1])
- File "scripts/qapi.py", line 1116, in __init__
- self.check()
- File "scripts/qapi.py", line 1299, in check
- ent.check(self)
- File "scripts/qapi.py", line 962, in check
- self.variants.check(schema, members, seen)
- File "scripts/qapi.py", line 1024, in check
- v.check(schema, self.tag_member.type, vseen)
- File "scripts/qapi.py", line 1032, in check
- QAPISchemaObjectTypeMember.check(self, schema, [], seen)
- File "scripts/qapi.py", line 994, in check
- assert self.name not in seen
-AssertionError
+tests/qapi-schema/union-clash-type.json:2: Union 'TestUnion' member 'kind' clashes with '(automatic)'
--
2.4.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 06/16] qapi: Avoid assertion failure on union 'type' collision
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 06/16] qapi: Avoid assertion failure on union 'type' collision Eric Blake
@ 2015-09-29 12:36 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2015-09-29 12:36 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> The previous commit added two tests that triggered an assertion
> failure. It's fairly straightforward to avoid the failure by
> just outright forbidding the collision between a union's tag
> values and its discriminator name (including the implicit name
> 'kind' supplied for simple unions [*]). Ultimately, we'd like
> to move the collision detection into QAPISchema*.check(), but
> for now it is easier just to enhance the existing checks.
>
> [*] Of course, down the road, we have plans to rename the simple
> union tag name to 'type' to match the QMP wire name, but the
> idea of the collision will still be present even then.
>
> Technically, we could avoid the collision by naming the C union
> members representing each enum value as '_case_value' rather
> than 'value'; but until we have an actual qapi client (and not
> just our testsuite) that has a legitimate reason to match a
> case label to the name of a QMP key and needs the name munging
> to satisfy the compiler, it's easier to just reject the qapi
> as invalid.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: new patch
> ---
> scripts/qapi.py | 10 ++++++++--
> tests/qapi-schema/flat-union-clash-type.err | 17 +----------------
> tests/qapi-schema/union-clash-type.err | 17 +----------------
> 3 files changed, 10 insertions(+), 34 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 7e8a99d..ec450a6 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -544,7 +544,7 @@ def check_union(expr, expr_info):
> base = expr.get('base')
> discriminator = expr.get('discriminator')
> members = expr['data']
> - values = {'MAX': '(automatic)'}
> + values = {'MAX': '(automatic)', 'KIND': '(automatic)'}
>
> # Two types of unions, determined by discriminator.
>
> @@ -603,13 +603,19 @@ def check_union(expr, expr_info):
> " of branch '%s'" % key)
>
> # If the discriminator names an enum type, then all members
> - # of 'data' must also be members of the enum type.
> + # of 'data' must also be members of the enum type, which in turn
> + # must not collide with the discriminator name.
> if enum_define:
> if key not in enum_define['enum_values']:
> raise QAPIExprError(expr_info,
> "Discriminator value '%s' is not found in "
> "enum '%s'" %
> (key, enum_define["enum_name"]))
> + if discriminator in enum_define['enum_values']:
> + raise QAPIExprError(expr_info,
> + "Discriminator name '%s' collides with "
> + "enum value in '%s'" %
> + (discriminator, enum_define["enum_name"]))
>
> # Otherwise, check for conflicts in the generated enum
> else:
> diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err
> index 6e64d1d..879beb8 100644
> --- a/tests/qapi-schema/flat-union-clash-type.err
> +++ b/tests/qapi-schema/flat-union-clash-type.err
> @@ -1,16 +1 @@
> -Traceback (most recent call last):
> - File "tests/qapi-schema/test-qapi.py", line 55, in <module>
> - schema = QAPISchema(sys.argv[1])
> - File "scripts/qapi.py", line 1116, in __init__
> - self.check()
> - File "scripts/qapi.py", line 1299, in check
> - ent.check(self)
> - File "scripts/qapi.py", line 962, in check
> - self.variants.check(schema, members, seen)
> - File "scripts/qapi.py", line 1024, in check
> - v.check(schema, self.tag_member.type, vseen)
> - File "scripts/qapi.py", line 1032, in check
> - QAPISchemaObjectTypeMember.check(self, schema, [], seen)
> - File "scripts/qapi.py", line 994, in check
> - assert self.name not in seen
> -AssertionError
> +tests/qapi-schema/flat-union-clash-type.json:8: Discriminator name 'type' collides with enum value in 'TestEnum'
> diff --git a/tests/qapi-schema/union-clash-type.err b/tests/qapi-schema/union-clash-type.err
> index 6e64d1d..773900b 100644
> --- a/tests/qapi-schema/union-clash-type.err
> +++ b/tests/qapi-schema/union-clash-type.err
> @@ -1,16 +1 @@
> -Traceback (most recent call last):
> - File "tests/qapi-schema/test-qapi.py", line 55, in <module>
> - schema = QAPISchema(sys.argv[1])
> - File "scripts/qapi.py", line 1116, in __init__
> - self.check()
> - File "scripts/qapi.py", line 1299, in check
> - ent.check(self)
> - File "scripts/qapi.py", line 962, in check
> - self.variants.check(schema, members, seen)
> - File "scripts/qapi.py", line 1024, in check
> - v.check(schema, self.tag_member.type, vseen)
> - File "scripts/qapi.py", line 1032, in check
> - QAPISchemaObjectTypeMember.check(self, schema, [], seen)
> - File "scripts/qapi.py", line 994, in check
> - assert self.name not in seen
> -AssertionError
> +tests/qapi-schema/union-clash-type.json:2: Union 'TestUnion' member 'kind' clashes with '(automatic)'
Preexisting: this error message sucks. But improving it isn't this
patch's job.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 07/16] qapi: Add tests for empty unions
2015-09-29 3:27 [Qemu-devel] [PATCH v6 00/16] post-introspection cleanups, subset A Eric Blake
` (5 preceding siblings ...)
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 06/16] qapi: Avoid assertion failure on union 'type' collision Eric Blake
@ 2015-09-29 3:27 ` Eric Blake
2015-09-29 13:17 ` Markus Armbruster
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 08/16] qapi: Test use of 'number' within alternates Eric Blake
` (8 subsequent siblings)
15 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2015-09-29 3:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
The documentation claims that alternates are useful for
allowing two types, although nothing enforces this. Meanwhile,
it is silent on whether empty unions are allowed. In practice,
the generated code will compile, in part because we have a
'void *data' branch; but attempting to visit such a type will
cause an abort(). While there's no technical reason that a
degenerate union could not be made to work, it's harder to
justify the time spent in chasing known (the current abort()
during visit) and unknown corner cases, than it would be to
just outlaw them. A future patch will probably take the
approach of forbidding them; in the meantime, we can at least
add testsuite coverage to make it obvious where things stand.
In addition to adding tests to expose the problems, we also
need to adjust existing tests that are meant to test something
else, but which could fail for the wrong reason if we reject
degenerate alternates/unions.
Note that empty structs are explicitly supported (for example,
right now they are the only way to specify that one branch of a
flat union adds no additional members), and empty enums are
covered by the testsuite as working (even if they do not seem
to have much use).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: commit message improved, rebase to earlier added tests
---
tests/Makefile | 3 +++
tests/qapi-schema/alternate-empty.err | 0
tests/qapi-schema/alternate-empty.exit | 1 +
tests/qapi-schema/alternate-empty.json | 2 ++
tests/qapi-schema/alternate-empty.out | 4 ++++
tests/qapi-schema/alternate-nested.json | 2 +-
tests/qapi-schema/alternate-unknown.json | 2 +-
tests/qapi-schema/flat-union-empty.err | 0
tests/qapi-schema/flat-union-empty.exit | 1 +
tests/qapi-schema/flat-union-empty.json | 4 ++++
tests/qapi-schema/flat-union-empty.out | 7 +++++++
tests/qapi-schema/union-empty.err | 0
tests/qapi-schema/union-empty.exit | 1 +
tests/qapi-schema/union-empty.json | 2 ++
tests/qapi-schema/union-empty.out | 3 +++
15 files changed, 30 insertions(+), 2 deletions(-)
create mode 100644 tests/qapi-schema/alternate-empty.err
create mode 100644 tests/qapi-schema/alternate-empty.exit
create mode 100644 tests/qapi-schema/alternate-empty.json
create mode 100644 tests/qapi-schema/alternate-empty.out
create mode 100644 tests/qapi-schema/flat-union-empty.err
create mode 100644 tests/qapi-schema/flat-union-empty.exit
create mode 100644 tests/qapi-schema/flat-union-empty.json
create mode 100644 tests/qapi-schema/flat-union-empty.out
create mode 100644 tests/qapi-schema/union-empty.err
create mode 100644 tests/qapi-schema/union-empty.exit
create mode 100644 tests/qapi-schema/union-empty.json
create mode 100644 tests/qapi-schema/union-empty.out
diff --git a/tests/Makefile b/tests/Makefile
index d43b8e9..dd866af 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -226,6 +226,7 @@ qapi-schema += alternate-base.json
qapi-schema += alternate-clash.json
qapi-schema += alternate-conflict-dict.json
qapi-schema += alternate-conflict-string.json
+qapi-schema += alternate-empty.json
qapi-schema += alternate-good.json
qapi-schema += alternate-nested.json
qapi-schema += alternate-unknown.json
@@ -278,6 +279,7 @@ qapi-schema += flat-union-clash-branch.json
qapi-schema += flat-union-clash-member.json
qapi-schema += flat-union-clash-type.json
qapi-schema += flat-union-cycle.json
+qapi-schema += flat-union-empty.json
qapi-schema += flat-union-inline.json
qapi-schema += flat-union-int-branch.json
qapi-schema += flat-union-invalid-branch-key.json
@@ -336,6 +338,7 @@ qapi-schema += union-base-no-discriminator.json
qapi-schema += union-clash-data.json
qapi-schema += union-clash-members.json
qapi-schema += union-clash-type.json
+qapi-schema += union-empty.json
qapi-schema += union-invalid-base.json
qapi-schema += union-max.json
qapi-schema += union-optional-branch.json
diff --git a/tests/qapi-schema/alternate-empty.err b/tests/qapi-schema/alternate-empty.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/alternate-empty.exit b/tests/qapi-schema/alternate-empty.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/alternate-empty.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/alternate-empty.json b/tests/qapi-schema/alternate-empty.json
new file mode 100644
index 0000000..db3820f
--- /dev/null
+++ b/tests/qapi-schema/alternate-empty.json
@@ -0,0 +1,2 @@
+# FIXME - alternates should list at least two types to be useful
+{ 'alternate': 'Alt', 'data': { 'i': 'int' } }
diff --git a/tests/qapi-schema/alternate-empty.out b/tests/qapi-schema/alternate-empty.out
new file mode 100644
index 0000000..0f153b6
--- /dev/null
+++ b/tests/qapi-schema/alternate-empty.out
@@ -0,0 +1,4 @@
+object :empty
+alternate Alt
+ case i: int
+enum AltKind ['i']
diff --git a/tests/qapi-schema/alternate-nested.json b/tests/qapi-schema/alternate-nested.json
index c4233b9..8e22186 100644
--- a/tests/qapi-schema/alternate-nested.json
+++ b/tests/qapi-schema/alternate-nested.json
@@ -2,4 +2,4 @@
{ 'alternate': 'Alt1',
'data': { 'name': 'str', 'value': 'int' } }
{ 'alternate': 'Alt2',
- 'data': { 'nested': 'Alt1' } }
+ 'data': { 'nested': 'Alt1', 'b': 'bool' } }
diff --git a/tests/qapi-schema/alternate-unknown.json b/tests/qapi-schema/alternate-unknown.json
index ad5c103..08c80dc 100644
--- a/tests/qapi-schema/alternate-unknown.json
+++ b/tests/qapi-schema/alternate-unknown.json
@@ -1,3 +1,3 @@
# we reject an alternate with unknown type in branch
{ 'alternate': 'Alt',
- 'data': { 'unknown': 'MissingType' } }
+ 'data': { 'unknown': 'MissingType', 'i': 'int' } }
diff --git a/tests/qapi-schema/flat-union-empty.err b/tests/qapi-schema/flat-union-empty.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/flat-union-empty.exit b/tests/qapi-schema/flat-union-empty.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/flat-union-empty.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/flat-union-empty.json b/tests/qapi-schema/flat-union-empty.json
new file mode 100644
index 0000000..67dd297
--- /dev/null
+++ b/tests/qapi-schema/flat-union-empty.json
@@ -0,0 +1,4 @@
+# FIXME - flat unions should not be empty
+{ 'enum': 'Empty', 'data': [ ] }
+{ 'struct': 'Base', 'data': { 'type': 'Empty' } }
+{ 'union': 'Union', 'base': 'Base', 'discriminator': 'type', 'data': { } }
diff --git a/tests/qapi-schema/flat-union-empty.out b/tests/qapi-schema/flat-union-empty.out
new file mode 100644
index 0000000..0e0665a
--- /dev/null
+++ b/tests/qapi-schema/flat-union-empty.out
@@ -0,0 +1,7 @@
+object :empty
+object Base
+ member type: Empty optional=False
+enum Empty []
+object Union
+ base Base
+ tag type
diff --git a/tests/qapi-schema/union-empty.err b/tests/qapi-schema/union-empty.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/union-empty.exit b/tests/qapi-schema/union-empty.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/union-empty.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/union-empty.json b/tests/qapi-schema/union-empty.json
new file mode 100644
index 0000000..1785007
--- /dev/null
+++ b/tests/qapi-schema/union-empty.json
@@ -0,0 +1,2 @@
+# FIXME - unions should not be empty
+{ 'union': 'Union', 'data': { } }
diff --git a/tests/qapi-schema/union-empty.out b/tests/qapi-schema/union-empty.out
new file mode 100644
index 0000000..8b5a7bf
--- /dev/null
+++ b/tests/qapi-schema/union-empty.out
@@ -0,0 +1,3 @@
+object :empty
+object Union
+enum UnionKind []
--
2.4.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 07/16] qapi: Add tests for empty unions
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 07/16] qapi: Add tests for empty unions Eric Blake
@ 2015-09-29 13:17 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2015-09-29 13:17 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> The documentation claims that alternates are useful for
> allowing two types, although nothing enforces this. Meanwhile,
"two or more"
Can touch up on commit.
> it is silent on whether empty unions are allowed. In practice,
> the generated code will compile, in part because we have a
> 'void *data' branch; but attempting to visit such a type will
> cause an abort(). While there's no technical reason that a
> degenerate union could not be made to work, it's harder to
> justify the time spent in chasing known (the current abort()
> during visit) and unknown corner cases, than it would be to
> just outlaw them. A future patch will probably take the
> approach of forbidding them; in the meantime, we can at least
> add testsuite coverage to make it obvious where things stand.
>
> In addition to adding tests to expose the problems, we also
> need to adjust existing tests that are meant to test something
> else, but which could fail for the wrong reason if we reject
> degenerate alternates/unions.
>
> Note that empty structs are explicitly supported (for example,
> right now they are the only way to specify that one branch of a
> flat union adds no additional members), and empty enums are
> covered by the testsuite as working (even if they do not seem
> to have much use).
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: commit message improved, rebase to earlier added tests
Patch is fine.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 08/16] qapi: Test use of 'number' within alternates
2015-09-29 3:27 [Qemu-devel] [PATCH v6 00/16] post-introspection cleanups, subset A Eric Blake
` (6 preceding siblings ...)
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 07/16] qapi: Add tests for empty unions Eric Blake
@ 2015-09-29 3:27 ` Eric Blake
2015-09-29 13:38 ` Markus Armbruster
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 09/16] qapi: Reuse code for flat union base validation Eric Blake
` (7 subsequent siblings)
15 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2015-09-29 3:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
Add some testsuite exposure for use of a 'number' as part of
an alternate. The current state of the tree has a few bugs
exposed by this: our input parser depends on the ordering of
how the qapi schema declared the alternate, and the parser
does not accept integers for a 'number' in an alternate even
though it does for numbers outside of an alternate.
Mixing 'int' and 'number' in the same alternate is unusual,
since both are supplied by json-numbers, but there does not
seem to be a technical reason to forbid it given that our
json lexer distinguishes between json-numbers that can be
represented as an int vs. those that cannot.
Improve the existing test_visitor_in_alternate() to match the
style of the new test_visitor_in_alternate_number(), and to
ensure full coverage of all possible qtype parsing.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: Rename from AltOne to AltStrBool (and so forth), drop
dead 'one = NULL' assignments, avoid leaking memory of
test data, tweak failing tests to give better hints of what
desired behavior is
---
tests/qapi-schema/qapi-schema-test.json | 8 ++
tests/qapi-schema/qapi-schema-test.out | 24 ++++++
tests/test-qmp-input-visitor.c | 129 +++++++++++++++++++++++++++++++-
3 files changed, 158 insertions(+), 3 deletions(-)
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index c511338..abe59fd 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -67,6 +67,14 @@
{ 'struct': 'UserDefC',
'data': { 'string1': 'str', 'string2': 'str' } }
+# for testing use of 'number' within alternates
+{ 'alternate': 'AltStrBool', 'data': { 's': 'str', 'b': 'bool' } }
+{ 'alternate': 'AltStrNum', 'data': { 's': 'str', 'n': 'number' } }
+{ 'alternate': 'AltNumStr', 'data': { 'n': 'number', 's': 'str' } }
+{ 'alternate': 'AltStrInt', 'data': { 's': 'str', 'i': 'int' } }
+{ 'alternate': 'AltIntNum', 'data': { 'i': 'int', 'n': 'number' } }
+{ 'alternate': 'AltNumInt', 'data': { 'n': 'number', 'i': 'int' } }
+
# for testing native lists
{ 'union': 'UserDefNativeListUnion',
'data': { 'integer': ['int'],
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 28a0b3c..8f81784 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -53,6 +53,30 @@ object :obj-user_def_cmd2-arg
object :obj-user_def_cmd3-arg
member a: int optional=False
member b: int optional=True
+alternate AltIntNum
+ case i: int
+ case n: number
+enum AltIntNumKind ['i', 'n']
+alternate AltNumInt
+ case n: number
+ case i: int
+enum AltNumIntKind ['n', 'i']
+alternate AltNumStr
+ case n: number
+ case s: str
+enum AltNumStrKind ['n', 's']
+alternate AltStrBool
+ case s: str
+ case b: bool
+enum AltStrBoolKind ['s', 'b']
+alternate AltStrInt
+ case s: str
+ case i: int
+enum AltStrIntKind ['s', 'i']
+alternate AltStrNum
+ case s: str
+ case n: number
+enum AltStrNumKind ['s', 'n']
event EVENT_A None
event EVENT_B None
event EVENT_C :obj-EVENT_C-arg
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 61715b3..641a3ab 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -371,12 +371,133 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
UserDefAlternate *tmp;
v = visitor_input_test_init(data, "42");
-
- visit_type_UserDefAlternate(v, &tmp, NULL, &err);
- g_assert(err == NULL);
+ visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_I);
g_assert_cmpint(tmp->i, ==, 42);
qapi_free_UserDefAlternate(tmp);
+ visitor_input_teardown(data, NULL);
+
+ v = visitor_input_test_init(data, "'string'");
+ visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
+ g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_S);
+ g_assert_cmpstr(tmp->s, ==, "string");
+ qapi_free_UserDefAlternate(tmp);
+ visitor_input_teardown(data, NULL);
+
+ v = visitor_input_test_init(data, "false");
+ visit_type_UserDefAlternate(v, &tmp, NULL, &err);
+ g_assert(err);
+ error_free(err);
+ err = NULL;
+ qapi_free_UserDefAlternate(tmp);
+ visitor_input_teardown(data, NULL);
+}
+
+static void test_visitor_in_alternate_number(TestInputVisitorData *data,
+ const void *unused)
+{
+ Visitor *v;
+ Error *err = NULL;
+ AltStrBool *asb;
+ AltStrNum *asn;
+ AltNumStr *ans;
+ AltStrInt *asi;
+ AltIntNum *ain;
+ AltNumInt *ani;
+
+ /* Parsing an int */
+
+ v = visitor_input_test_init(data, "42");
+ visit_type_AltStrBool(v, &asb, NULL, &err);
+ g_assert(err);
+ qapi_free_AltStrBool(asb);
+ visitor_input_teardown(data, NULL);
+
+ /* FIXME: Order of alternate should not affect semantics; asn should
+ * parse the same as ans */
+ v = visitor_input_test_init(data, "42");
+ visit_type_AltStrNum(v, &asn, NULL, &err);
+ /* FIXME g_assert_cmpint(asn->kind, == ALT_STR_NUM_KIND_N); */
+ /* FIXME g_assert_cmpfloat(asn->n, ==, 42); */
+ g_assert(err);
+ error_free(err);
+ err = NULL;
+ qapi_free_AltStrNum(asn);
+ visitor_input_teardown(data, NULL);
+
+ v = visitor_input_test_init(data, "42");
+ visit_type_AltNumStr(v, &ans, NULL, &error_abort);
+ g_assert_cmpint(ans->kind, ==, ALT_NUM_STR_KIND_N);
+ g_assert_cmpfloat(ans->n, ==, 42);
+ qapi_free_AltNumStr(ans);
+ visitor_input_teardown(data, NULL);
+
+ v = visitor_input_test_init(data, "42");
+ visit_type_AltStrInt(v, &asi, NULL, &error_abort);
+ g_assert_cmpint(asi->kind, ==, ALT_STR_INT_KIND_I);
+ g_assert_cmpint(asi->i, ==, 42);
+ qapi_free_AltStrInt(asi);
+ visitor_input_teardown(data, NULL);
+
+ v = visitor_input_test_init(data, "42");
+ visit_type_AltIntNum(v, &ain, NULL, &error_abort);
+ g_assert_cmpint(ain->kind, ==, ALT_INT_NUM_KIND_I);
+ g_assert_cmpint(ain->i, ==, 42);
+ qapi_free_AltIntNum(ain);
+ visitor_input_teardown(data, NULL);
+
+ v = visitor_input_test_init(data, "42");
+ visit_type_AltNumInt(v, &ani, NULL, &error_abort);
+ g_assert_cmpint(ani->kind, ==, ALT_NUM_INT_KIND_I);
+ g_assert_cmpint(ani->i, ==, 42);
+ qapi_free_AltNumInt(ani);
+ visitor_input_teardown(data, NULL);
+
+ /* Parsing a double */
+
+ v = visitor_input_test_init(data, "42.5");
+ visit_type_AltStrBool(v, &asb, NULL, &err);
+ g_assert(err);
+ error_free(err);
+ err = NULL;
+ qapi_free_AltStrBool(asb);
+ visitor_input_teardown(data, NULL);
+
+ v = visitor_input_test_init(data, "42.5");
+ visit_type_AltStrNum(v, &asn, NULL, &error_abort);
+ g_assert_cmpint(asn->kind, ==, ALT_STR_NUM_KIND_N);
+ g_assert_cmpfloat(asn->n, ==, 42.5);
+ qapi_free_AltStrNum(asn);
+ visitor_input_teardown(data, NULL);
+
+ v = visitor_input_test_init(data, "42.5");
+ visit_type_AltNumStr(v, &ans, NULL, &error_abort);
+ g_assert_cmpint(ans->kind, ==, ALT_NUM_STR_KIND_N);
+ g_assert_cmpfloat(ans->n, ==, 42.5);
+ qapi_free_AltNumStr(ans);
+ visitor_input_teardown(data, NULL);
+
+ v = visitor_input_test_init(data, "42.5");
+ visit_type_AltStrInt(v, &asi, NULL, &err);
+ g_assert(err);
+ error_free(err);
+ err = NULL;
+ qapi_free_AltStrInt(asi);
+ visitor_input_teardown(data, NULL);
+
+ v = visitor_input_test_init(data, "42.5");
+ visit_type_AltIntNum(v, &ain, NULL, &error_abort);
+ g_assert_cmpint(ain->kind, ==, ALT_INT_NUM_KIND_N);
+ g_assert_cmpfloat(ain->n, ==, 42.5);
+ qapi_free_AltIntNum(ain);
+ visitor_input_teardown(data, NULL);
+
+ v = visitor_input_test_init(data, "42.5");
+ visit_type_AltNumInt(v, &ani, NULL, &error_abort);
+ g_assert_cmpint(ani->kind, ==, ALT_NUM_INT_KIND_N);
+ g_assert_cmpint(ani->n, ==, 42.5);
+ qapi_free_AltNumInt(ani);
+ visitor_input_teardown(data, NULL);
}
static void test_native_list_integer_helper(TestInputVisitorData *data,
@@ -720,6 +841,8 @@ int main(int argc, char **argv)
&in_visitor_data, test_visitor_in_alternate);
input_visitor_test_add("/visitor/input/errors",
&in_visitor_data, test_visitor_in_errors);
+ input_visitor_test_add("/visitor/input/alternate-number",
+ &in_visitor_data, test_visitor_in_alternate_number);
input_visitor_test_add("/visitor/input/native_list/int",
&in_visitor_data,
test_visitor_in_native_list_int);
--
2.4.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 08/16] qapi: Test use of 'number' within alternates
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 08/16] qapi: Test use of 'number' within alternates Eric Blake
@ 2015-09-29 13:38 ` Markus Armbruster
2015-09-29 18:07 ` Eric Blake
2015-10-05 22:49 ` Eric Blake
0 siblings, 2 replies; 39+ messages in thread
From: Markus Armbruster @ 2015-09-29 13:38 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Add some testsuite exposure for use of a 'number' as part of
> an alternate. The current state of the tree has a few bugs
> exposed by this: our input parser depends on the ordering of
> how the qapi schema declared the alternate, and the parser
> does not accept integers for a 'number' in an alternate even
> though it does for numbers outside of an alternate.
>
> Mixing 'int' and 'number' in the same alternate is unusual,
> since both are supplied by json-numbers, but there does not
> seem to be a technical reason to forbid it given that our
> json lexer distinguishes between json-numbers that can be
> represented as an int vs. those that cannot.
>
> Improve the existing test_visitor_in_alternate() to match the
> style of the new test_visitor_in_alternate_number(), and to
> ensure full coverage of all possible qtype parsing.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: Rename from AltOne to AltStrBool (and so forth), drop
> dead 'one = NULL' assignments, avoid leaking memory of
> test data, tweak failing tests to give better hints of what
> desired behavior is
> ---
> tests/qapi-schema/qapi-schema-test.json | 8 ++
> tests/qapi-schema/qapi-schema-test.out | 24 ++++++
> tests/test-qmp-input-visitor.c | 129 +++++++++++++++++++++++++++++++-
> 3 files changed, 158 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index c511338..abe59fd 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -67,6 +67,14 @@
> { 'struct': 'UserDefC',
> 'data': { 'string1': 'str', 'string2': 'str' } }
>
> +# for testing use of 'number' within alternates
> +{ 'alternate': 'AltStrBool', 'data': { 's': 'str', 'b': 'bool' } }
> +{ 'alternate': 'AltStrNum', 'data': { 's': 'str', 'n': 'number' } }
> +{ 'alternate': 'AltNumStr', 'data': { 'n': 'number', 's': 'str' } }
> +{ 'alternate': 'AltStrInt', 'data': { 's': 'str', 'i': 'int' } }
> +{ 'alternate': 'AltIntNum', 'data': { 'i': 'int', 'n': 'number' } }
> +{ 'alternate': 'AltNumInt', 'data': { 'n': 'number', 'i': 'int' } }
> +
> # for testing native lists
> { 'union': 'UserDefNativeListUnion',
> 'data': { 'integer': ['int'],
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 28a0b3c..8f81784 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -53,6 +53,30 @@ object :obj-user_def_cmd2-arg
> object :obj-user_def_cmd3-arg
> member a: int optional=False
> member b: int optional=True
> +alternate AltIntNum
> + case i: int
> + case n: number
> +enum AltIntNumKind ['i', 'n']
> +alternate AltNumInt
> + case n: number
> + case i: int
> +enum AltNumIntKind ['n', 'i']
> +alternate AltNumStr
> + case n: number
> + case s: str
> +enum AltNumStrKind ['n', 's']
> +alternate AltStrBool
> + case s: str
> + case b: bool
> +enum AltStrBoolKind ['s', 'b']
> +alternate AltStrInt
> + case s: str
> + case i: int
> +enum AltStrIntKind ['s', 'i']
> +alternate AltStrNum
> + case s: str
> + case n: number
> +enum AltStrNumKind ['s', 'n']
> event EVENT_A None
> event EVENT_B None
> event EVENT_C :obj-EVENT_C-arg
> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index 61715b3..641a3ab 100644
> --- a/tests/test-qmp-input-visitor.c
> +++ b/tests/test-qmp-input-visitor.c
> @@ -371,12 +371,133 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
> UserDefAlternate *tmp;
>
> v = visitor_input_test_init(data, "42");
> -
> - visit_type_UserDefAlternate(v, &tmp, NULL, &err);
> - g_assert(err == NULL);
> + visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
> g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_I);
> g_assert_cmpint(tmp->i, ==, 42);
> qapi_free_UserDefAlternate(tmp);
> + visitor_input_teardown(data, NULL);
Ugly in this test: visitor_input_test_init() is to be paired with
visitor_input_teardown(), but each test's last visitor_input_teardown()
can be omitted, because we also pass visitor_input_teardown to
g_test_add(). Not your patch's fault. Let's ignore it for now.
> +
> + v = visitor_input_test_init(data, "'string'");
> + visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
> + g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_S);
> + g_assert_cmpstr(tmp->s, ==, "string");
> + qapi_free_UserDefAlternate(tmp);
> + visitor_input_teardown(data, NULL);
> +
> + v = visitor_input_test_init(data, "false");
> + visit_type_UserDefAlternate(v, &tmp, NULL, &err);
> + g_assert(err);
> + error_free(err);
> + err = NULL;
> + qapi_free_UserDefAlternate(tmp);
> + visitor_input_teardown(data, NULL);
This case tests an alternate type without a bool member rejects false.
> +}
> +
> +static void test_visitor_in_alternate_number(TestInputVisitorData *data,
> + const void *unused)
> +{
> + Visitor *v;
> + Error *err = NULL;
> + AltStrBool *asb;
> + AltStrNum *asn;
> + AltNumStr *ans;
> + AltStrInt *asi;
> + AltIntNum *ain;
> + AltNumInt *ani;
> +
> + /* Parsing an int */
> +
> + v = visitor_input_test_init(data, "42");
> + visit_type_AltStrBool(v, &asb, NULL, &err);
> + g_assert(err);
> + qapi_free_AltStrBool(asb);
> + visitor_input_teardown(data, NULL);
This test case tests an alternate type without a numeric member (number,
int, int8, ...) rejects 42. Almost identical to the case above. I
wouldn't have done that one, but since you did, I'm not going demand you
drop it.
> +
> + /* FIXME: Order of alternate should not affect semantics; asn should
> + * parse the same as ans */
> + v = visitor_input_test_init(data, "42");
> + visit_type_AltStrNum(v, &asn, NULL, &err);
> + /* FIXME g_assert_cmpint(asn->kind, == ALT_STR_NUM_KIND_N); */
> + /* FIXME g_assert_cmpfloat(asn->n, ==, 42); */
> + g_assert(err);
> + error_free(err);
> + err = NULL;
> + qapi_free_AltStrNum(asn);
> + visitor_input_teardown(data, NULL);
> +
> + v = visitor_input_test_init(data, "42");
> + visit_type_AltNumStr(v, &ans, NULL, &error_abort);
> + g_assert_cmpint(ans->kind, ==, ALT_NUM_STR_KIND_N);
> + g_assert_cmpfloat(ans->n, ==, 42);
> + qapi_free_AltNumStr(ans);
> + visitor_input_teardown(data, NULL);
[More...]
Reads easier now. Thanks!
> @@ -720,6 +841,8 @@ int main(int argc, char **argv)
> &in_visitor_data, test_visitor_in_alternate);
> input_visitor_test_add("/visitor/input/errors",
> &in_visitor_data, test_visitor_in_errors);
> + input_visitor_test_add("/visitor/input/alternate-number",
> + &in_visitor_data, test_visitor_in_alternate_number);
> input_visitor_test_add("/visitor/input/native_list/int",
> &in_visitor_data,
> test_visitor_in_native_list_int);
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 08/16] qapi: Test use of 'number' within alternates
2015-09-29 13:38 ` Markus Armbruster
@ 2015-09-29 18:07 ` Eric Blake
2015-10-01 6:23 ` Markus Armbruster
2015-10-05 22:49 ` Eric Blake
1 sibling, 1 reply; 39+ messages in thread
From: Eric Blake @ 2015-09-29 18:07 UTC (permalink / raw)
To: Markus Armbruster; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2470 bytes --]
On 09/29/2015 07:38 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Add some testsuite exposure for use of a 'number' as part of
>> an alternate. The current state of the tree has a few bugs
>> exposed by this: our input parser depends on the ordering of
>> how the qapi schema declared the alternate, and the parser
>> does not accept integers for a 'number' in an alternate even
>> though it does for numbers outside of an alternate.
>>
>> Mixing 'int' and 'number' in the same alternate is unusual,
>> since both are supplied by json-numbers, but there does not
>> seem to be a technical reason to forbid it given that our
>> json lexer distinguishes between json-numbers that can be
>> represented as an int vs. those that cannot.
>>
>> Improve the existing test_visitor_in_alternate() to match the
>> style of the new test_visitor_in_alternate_number(), and to
>> ensure full coverage of all possible qtype parsing.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> +++ b/tests/test-qmp-input-visitor.c
>> @@ -371,12 +371,133 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
>> UserDefAlternate *tmp;
>>
>> v = visitor_input_test_init(data, "42");
>> -
>> - visit_type_UserDefAlternate(v, &tmp, NULL, &err);
>> - g_assert(err == NULL);
>> + visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
>> g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_I);
>> g_assert_cmpint(tmp->i, ==, 42);
>> qapi_free_UserDefAlternate(tmp);
>> + visitor_input_teardown(data, NULL);
>
> Ugly in this test: visitor_input_test_init() is to be paired with
> visitor_input_teardown(), but each test's last visitor_input_teardown()
> can be omitted, because we also pass visitor_input_teardown to
> g_test_add(). Not your patch's fault. Let's ignore it for now.
v5 25/46 claims otherwise (valgrind was complaining about leaks that
additional calls to visitor_input_teardown() resolved; maybe the test
engine is not automatically doing the expected teardown?). And maybe it
would be smarter to rework the tests to allow reusing an existing
visitor on a new string, instead of completely allocating a new visitor
framework for every new string to parse. But indeed, any further
cleanups are better done in that later patch.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 08/16] qapi: Test use of 'number' within alternates
2015-09-29 18:07 ` Eric Blake
@ 2015-10-01 6:23 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2015-10-01 6:23 UTC (permalink / raw)
To: Eric Blake; +Cc: Michael Roth, marcandre.lureau, qemu-devel, ehabkost
Eric Blake <eblake@redhat.com> writes:
> On 09/29/2015 07:38 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> Add some testsuite exposure for use of a 'number' as part of
>>> an alternate. The current state of the tree has a few bugs
>>> exposed by this: our input parser depends on the ordering of
>>> how the qapi schema declared the alternate, and the parser
>>> does not accept integers for a 'number' in an alternate even
>>> though it does for numbers outside of an alternate.
>>>
>>> Mixing 'int' and 'number' in the same alternate is unusual,
>>> since both are supplied by json-numbers, but there does not
>>> seem to be a technical reason to forbid it given that our
>>> json lexer distinguishes between json-numbers that can be
>>> represented as an int vs. those that cannot.
>>>
>>> Improve the existing test_visitor_in_alternate() to match the
>>> style of the new test_visitor_in_alternate_number(), and to
>>> ensure full coverage of all possible qtype parsing.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>
>>> +++ b/tests/test-qmp-input-visitor.c
>>> @@ -371,12 +371,133 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
>>> UserDefAlternate *tmp;
>>>
>>> v = visitor_input_test_init(data, "42");
>>> -
>>> - visit_type_UserDefAlternate(v, &tmp, NULL, &err);
>>> - g_assert(err == NULL);
>>> + visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
>>> g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_I);
>>> g_assert_cmpint(tmp->i, ==, 42);
>>> qapi_free_UserDefAlternate(tmp);
>>> + visitor_input_teardown(data, NULL);
>>
>> Ugly in this test: visitor_input_test_init() is to be paired with
>> visitor_input_teardown(), but each test's last visitor_input_teardown()
>> can be omitted, because we also pass visitor_input_teardown to
>> g_test_add(). Not your patch's fault. Let's ignore it for now.
>
> v5 25/46 claims otherwise (valgrind was complaining about leaks that
> additional calls to visitor_input_teardown() resolved; maybe the test
> engine is not automatically doing the expected teardown?). And maybe it
> would be smarter to rework the tests to allow reusing an existing
> visitor on a new string, instead of completely allocating a new visitor
> framework for every new string to parse. But indeed, any further
> cleanups are better done in that later patch.
Let me rephrase.
visitor_input_test_init() creates what visitor_input_teardown()
destroys. The two need to be paired.
The pairing is done in an ugly way: the tests call
visitor_input_teardown() for every visitor_input_test_init() *except*
the last one. That one's teardown is set up by
input_visitor_test_add(), which passes visitor_input_teardown() to
g_test_add().
When a function calls visitor_input_test_init() just once, this ugliness
isn't very visible. When it calls it multiple times, it's quite visible
and actually confusing.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 08/16] qapi: Test use of 'number' within alternates
2015-09-29 13:38 ` Markus Armbruster
2015-09-29 18:07 ` Eric Blake
@ 2015-10-05 22:49 ` Eric Blake
1 sibling, 0 replies; 39+ messages in thread
From: Eric Blake @ 2015-10-05 22:49 UTC (permalink / raw)
To: Markus Armbruster; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2544 bytes --]
On 09/29/2015 07:38 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Add some testsuite exposure for use of a 'number' as part of
>> an alternate. The current state of the tree has a few bugs
>> exposed by this: our input parser depends on the ordering of
>> how the qapi schema declared the alternate, and the parser
>> does not accept integers for a 'number' in an alternate even
>> though it does for numbers outside of an alternate.
>>
>> Mixing 'int' and 'number' in the same alternate is unusual,
>> since both are supplied by json-numbers, but there does not
>> seem to be a technical reason to forbid it given that our
>> json lexer distinguishes between json-numbers that can be
>> represented as an int vs. those that cannot.
>>
>> Improve the existing test_visitor_in_alternate() to match the
>> style of the new test_visitor_in_alternate_number(), and to
>> ensure full coverage of all possible qtype parsing.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> +++ b/tests/test-qmp-input-visitor.c
>> @@ -371,12 +371,133 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
>> UserDefAlternate *tmp;
>>
>> v = visitor_input_test_init(data, "42");
>> -
>> - visit_type_UserDefAlternate(v, &tmp, NULL, &err);
>> - g_assert(err == NULL);
>> + visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
>> g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_I);
>> g_assert_cmpint(tmp->i, ==, 42);
>> qapi_free_UserDefAlternate(tmp);
>> + visitor_input_teardown(data, NULL);
>
> Ugly in this test: visitor_input_test_init() is to be paired with
> visitor_input_teardown(), but each test's last visitor_input_teardown()
> can be omitted, because we also pass visitor_input_teardown to
> g_test_add(). Not your patch's fault. Let's ignore it for now.
In fact, I have another patch coming down the pipeline that moves
visitor_input_teardown() into visitor_input_test_init() (teardown is
idempotent. And while working on it, I noticed that we could consider
doing something like:
init(data, ...);
func(blah, &data->err);
g_assert(data->err);
teardown(data);
to let teardown() take care of err, instead of having to manually
error_free(err); err = NULL;
on each reset (particularly nice if visitor_input_test_init() invokes
teardown() itself, for automatic reset semantics).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 09/16] qapi: Reuse code for flat union base validation
2015-09-29 3:27 [Qemu-devel] [PATCH v6 00/16] post-introspection cleanups, subset A Eric Blake
` (7 preceding siblings ...)
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 08/16] qapi: Test use of 'number' within alternates Eric Blake
@ 2015-09-29 3:27 ` Eric Blake
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 10/16] qapi: Consistent generated code: prefer error 'err' Eric Blake
` (6 subsequent siblings)
15 siblings, 0 replies; 39+ messages in thread
From: Eric Blake @ 2015-09-29 3:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
Rather than open-code the check for a valid base type, we
should reuse the common functionality. This allows for
consistent error messages, and also makes it easier for a
later patch to turn on support for inline anonymous base
structures.
Test flat-union-inline is updated to test only one feature
(anonymous branch dictionaries), which can be implemented
independently (test flat-union-bad-base already covers the
idea of an anonymous base dictionary).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: rebase to earlier testsuite improvements, wording tweak
---
scripts/qapi.py | 11 +++++------
tests/qapi-schema/flat-union-bad-base.err | 2 +-
tests/qapi-schema/flat-union-base-any.err | 2 +-
tests/qapi-schema/flat-union-base-union.err | 2 +-
tests/qapi-schema/flat-union-cycle.err | 2 +-
tests/qapi-schema/flat-union-inline.err | 2 +-
tests/qapi-schema/flat-union-inline.json | 4 ++--
tests/qapi-schema/flat-union-no-base.err | 2 +-
tests/qapi-schema/union-invalid-base.err | 2 +-
9 files changed, 14 insertions(+), 15 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index ec450a6..4825e62 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -560,15 +560,14 @@ def check_union(expr, expr_info):
# Else, it's a flat union.
else:
# The object must have a string member 'base'.
- if not isinstance(base, str):
+ check_type(expr_info, "'base' for union '%s'" % name,
+ base, allow_metas=['struct'])
+ if not base:
raise QAPIExprError(expr_info,
- "Flat union '%s' must have a string base field"
+ "Flat union '%s' must have a base"
% name)
base_fields = find_base_fields(base)
- if not base_fields:
- raise QAPIExprError(expr_info,
- "Base '%s' is not a valid struct"
- % base)
+ assert base_fields
# The value of member 'discriminator' must name a non-optional
# member of the base struct.
diff --git a/tests/qapi-schema/flat-union-bad-base.err b/tests/qapi-schema/flat-union-bad-base.err
index f9c31b2..79b8a71 100644
--- a/tests/qapi-schema/flat-union-bad-base.err
+++ b/tests/qapi-schema/flat-union-bad-base.err
@@ -1 +1 @@
-tests/qapi-schema/flat-union-bad-base.json:9: Flat union 'TestUnion' must have a string base field
+tests/qapi-schema/flat-union-bad-base.json:9: 'base' for union 'TestUnion' should be a type name
diff --git a/tests/qapi-schema/flat-union-base-any.err b/tests/qapi-schema/flat-union-base-any.err
index ad4d629..646f1c9 100644
--- a/tests/qapi-schema/flat-union-base-any.err
+++ b/tests/qapi-schema/flat-union-base-any.err
@@ -1 +1 @@
-tests/qapi-schema/flat-union-base-any.json:8: Base 'any' is not a valid struct
+tests/qapi-schema/flat-union-base-any.json:8: 'base' for union 'TestUnion' cannot use built-in type 'any'
diff --git a/tests/qapi-schema/flat-union-base-union.err b/tests/qapi-schema/flat-union-base-union.err
index ede9859..d50e687 100644
--- a/tests/qapi-schema/flat-union-base-union.err
+++ b/tests/qapi-schema/flat-union-base-union.err
@@ -1 +1 @@
-tests/qapi-schema/flat-union-base-union.json:11: Base 'UnionBase' is not a valid struct
+tests/qapi-schema/flat-union-base-union.json:11: 'base' for union 'TestUnion' cannot use union type 'UnionBase'
diff --git a/tests/qapi-schema/flat-union-cycle.err b/tests/qapi-schema/flat-union-cycle.err
index d4f2a09..834d3f3 100644
--- a/tests/qapi-schema/flat-union-cycle.err
+++ b/tests/qapi-schema/flat-union-cycle.err
@@ -1 +1 @@
-tests/qapi-schema/flat-union-cycle.json:6: Base 'Union' is not a valid struct
+tests/qapi-schema/flat-union-cycle.json:6: 'base' for union 'Union' cannot use union type 'Union'
diff --git a/tests/qapi-schema/flat-union-inline.err b/tests/qapi-schema/flat-union-inline.err
index ec58627..2333358 100644
--- a/tests/qapi-schema/flat-union-inline.err
+++ b/tests/qapi-schema/flat-union-inline.err
@@ -1 +1 @@
-tests/qapi-schema/flat-union-inline.json:7: Flat union 'TestUnion' must have a string base field
+tests/qapi-schema/flat-union-inline.json:7: Member 'value1' of union 'TestUnion' should be a type name
diff --git a/tests/qapi-schema/flat-union-inline.json b/tests/qapi-schema/flat-union-inline.json
index 6bfdd65..62c7cda 100644
--- a/tests/qapi-schema/flat-union-inline.json
+++ b/tests/qapi-schema/flat-union-inline.json
@@ -1,11 +1,11 @@
# we require branches to be a struct name
-# TODO: should we allow anonymous inline types?
+# TODO: should we allow anonymous inline branch types?
{ 'enum': 'TestEnum',
'data': [ 'value1', 'value2' ] }
{ 'struct': 'Base',
'data': { 'enum1': 'TestEnum', 'kind': 'str' } }
{ 'union': 'TestUnion',
- 'base': { 'enum1': 'TestEnum', 'kind': 'str' },
+ 'base': 'Base',
'discriminator': 'enum1',
'data': { 'value1': { 'string': 'str' },
'value2': { 'integer': 'int' } } }
diff --git a/tests/qapi-schema/flat-union-no-base.err b/tests/qapi-schema/flat-union-no-base.err
index bb3f708..841c93b 100644
--- a/tests/qapi-schema/flat-union-no-base.err
+++ b/tests/qapi-schema/flat-union-no-base.err
@@ -1 +1 @@
-tests/qapi-schema/flat-union-no-base.json:9: Flat union 'TestUnion' must have a string base field
+tests/qapi-schema/flat-union-no-base.json:9: Flat union 'TestUnion' must have a base
diff --git a/tests/qapi-schema/union-invalid-base.err b/tests/qapi-schema/union-invalid-base.err
index 9f63796..03d7b97 100644
--- a/tests/qapi-schema/union-invalid-base.err
+++ b/tests/qapi-schema/union-invalid-base.err
@@ -1 +1 @@
-tests/qapi-schema/union-invalid-base.json:8: Base 'int' is not a valid struct
+tests/qapi-schema/union-invalid-base.json:8: 'base' for union 'TestUnion' cannot use built-in type 'int'
--
2.4.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v6 10/16] qapi: Consistent generated code: prefer error 'err'
2015-09-29 3:27 [Qemu-devel] [PATCH v6 00/16] post-introspection cleanups, subset A Eric Blake
` (8 preceding siblings ...)
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 09/16] qapi: Reuse code for flat union base validation Eric Blake
@ 2015-09-29 3:27 ` Eric Blake
2015-09-29 13:46 ` Markus Armbruster
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 11/16] qapi: Consistent generated code: prefer visitor 'v' Eric Blake
` (5 subsequent siblings)
15 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2015-09-29 3:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
We had some pointless differences in the generated code for visit,
command marshalling, and events; unifying them makes it easier for
future patches to consolidate to common helper functions.
This is patch 1/4, focusing on naming the local error variable
'err' rather than 'local_err'.
No change in semantics to the generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: split 9/46 into four patches, update docs where they are affected,
rebase with 7/46 placed later in series
Signed-off-by: Eric Blake <eblake@redhat.com>
---
docs/qapi-code-gen.txt | 28 ++++++++++++++--------------
scripts/qapi-commands.py | 22 +++++++++++-----------
scripts/qapi-event.py | 18 +++++++++---------
3 files changed, 34 insertions(+), 34 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index b1c8361..b99e6fa 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -916,20 +916,20 @@ Example:
static void qmp_marshal_output_UserDefOne(UserDefOne *ret_in, QObject **ret_out, Error **errp)
{
- Error *local_err = NULL;
+ Error *err = NULL;
QmpOutputVisitor *mo = qmp_output_visitor_new();
QapiDeallocVisitor *md;
Visitor *v;
v = qmp_output_get_visitor(mo);
- visit_type_UserDefOne(v, &ret_in, "unused", &local_err);
- if (local_err) {
+ visit_type_UserDefOne(v, &ret_in, "unused", &err);
+ if (err) {
goto out;
}
*ret_out = qmp_output_get_qobject(mo);
out:
- error_propagate(errp, local_err);
+ error_propagate(errp, err);
qmp_output_visitor_cleanup(mo);
md = qapi_dealloc_visitor_new();
v = qapi_dealloc_get_visitor(md);
@@ -939,7 +939,7 @@ Example:
static void qmp_marshal_my_command(QDict *args, QObject **ret, Error **errp)
{
- Error *local_err = NULL;
+ Error *err = NULL;
UserDefOne *retval;
QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
QapiDeallocVisitor *md;
@@ -947,20 +947,20 @@ Example:
UserDefOne *arg1 = NULL;
v = qmp_input_get_visitor(mi);
- visit_type_UserDefOne(v, &arg1, "arg1", &local_err);
- if (local_err) {
+ visit_type_UserDefOne(v, &arg1, "arg1", &err);
+ if (err) {
goto out;
}
- retval = qmp_my_command(arg1, &local_err);
- if (local_err) {
+ retval = qmp_my_command(arg1, &err);
+ if (err) {
goto out;
}
- qmp_marshal_output_UserDefOne(retval, ret, &local_err);
+ qmp_marshal_output_UserDefOne(retval, ret, &err);
out:
- error_propagate(errp, local_err);
+ error_propagate(errp, err);
qmp_input_visitor_cleanup(mi);
md = qapi_dealloc_visitor_new();
v = qapi_dealloc_get_visitor(md);
@@ -1007,7 +1007,7 @@ Example:
void qapi_event_send_my_event(Error **errp)
{
QDict *qmp;
- Error *local_err = NULL;
+ Error *err = NULL;
QMPEventFuncEmit emit;
emit = qmp_event_get_func_emit();
if (!emit) {
@@ -1016,9 +1016,9 @@ Example:
qmp = qmp_event_build_dict("MY_EVENT");
- emit(EXAMPLE_QAPI_EVENT_MY_EVENT, qmp, &local_err);
+ emit(EXAMPLE_QAPI_EVENT_MY_EVENT, qmp, &err);
- error_propagate(errp, local_err);
+ error_propagate(errp, err);
QDECREF(qmp);
}
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 810a897..2151120 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -53,14 +53,14 @@ def gen_call(name, arg_type, ret_type):
push_indent()
ret = mcgen('''
-%(lhs)sqmp_%(c_name)s(%(args)s&local_err);
+%(lhs)sqmp_%(c_name)s(%(args)s&err);
''',
c_name=c_name(name), args=argstr, lhs=lhs)
if ret_type:
- ret += gen_err_check('local_err')
+ ret += gen_err_check('err')
ret += mcgen('''
-qmp_marshal_output_%(c_name)s(retval, ret, &local_err);
+qmp_marshal_output_%(c_name)s(retval, ret, &err);
''',
c_name=ret_type.c_name())
pop_indent()
@@ -69,7 +69,7 @@ qmp_marshal_output_%(c_name)s(retval, ret, &local_err);
def gen_marshal_vars(arg_type, ret_type):
ret = mcgen('''
- Error *local_err = NULL;
+ Error *err = NULL;
''')
push_indent()
@@ -127,8 +127,8 @@ md = qapi_dealloc_visitor_new();
v = qapi_dealloc_get_visitor(md);
''')
else:
- errparg = '&local_err'
- errarg = 'local_err'
+ errparg = '&err'
+ errarg = 'err'
ret += mcgen('''
v = qmp_input_get_visitor(mi);
''')
@@ -171,20 +171,20 @@ def gen_marshal_output(ret_type):
static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out, Error **errp)
{
- Error *local_err = NULL;
+ Error *err = NULL;
QmpOutputVisitor *mo = qmp_output_visitor_new();
QapiDeallocVisitor *md;
Visitor *v;
v = qmp_output_get_visitor(mo);
- visit_type_%(c_name)s(v, &ret_in, "unused", &local_err);
- if (local_err) {
+ visit_type_%(c_name)s(v, &ret_in, "unused", &err);
+ if (err) {
goto out;
}
*ret_out = qmp_output_get_qobject(mo);
out:
- error_propagate(errp, local_err);
+ error_propagate(errp, err);
qmp_output_visitor_cleanup(mo);
md = qapi_dealloc_visitor_new();
v = qapi_dealloc_get_visitor(md);
@@ -227,7 +227,7 @@ def gen_marshal(name, arg_type, ret_type):
out:
''')
ret += mcgen('''
- error_propagate(errp, local_err);
+ error_propagate(errp, err);
''')
ret += gen_marshal_input_visit(arg_type, dealloc=True)
ret += mcgen('''
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index d15fad9..d41af40 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -34,7 +34,7 @@ def gen_event_send(name, arg_type):
%(proto)s
{
QDict *qmp;
- Error *local_err = NULL;
+ Error *err = NULL;
QMPEventFuncEmit emit;
''',
proto=gen_event_send_proto(name, arg_type))
@@ -67,8 +67,8 @@ def gen_event_send(name, arg_type):
g_assert(v);
/* Fake visit, as if all members are under a structure */
- visit_start_struct(v, NULL, "", "%(name)s", 0, &local_err);
- if (local_err) {
+ visit_start_struct(v, NULL, "", "%(name)s", 0, &err);
+ if (err) {
goto clean;
}
@@ -90,8 +90,8 @@ def gen_event_send(name, arg_type):
cast = ''
ret += mcgen('''
- visit_type_%(c_type)s(v, %(cast)s&%(c_name)s, "%(name)s", &local_err);
- if (local_err) {
+ visit_type_%(c_type)s(v, %(cast)s&%(c_name)s, "%(name)s", &err);
+ if (err) {
goto clean;
}
''',
@@ -108,8 +108,8 @@ def gen_event_send(name, arg_type):
ret += mcgen('''
- visit_end_struct(v, &local_err);
- if (local_err) {
+ visit_end_struct(v, &err);
+ if (err) {
goto clean;
}
@@ -120,7 +120,7 @@ def gen_event_send(name, arg_type):
''')
ret += mcgen('''
- emit(%(c_enum)s, qmp, &local_err);
+ emit(%(c_enum)s, qmp, &err);
''',
c_enum=c_enum_const(event_enum_name, name))
@@ -131,7 +131,7 @@ def gen_event_send(name, arg_type):
qmp_output_visitor_cleanup(qov);
''')
ret += mcgen('''
- error_propagate(errp, local_err);
+ error_propagate(errp, err);
QDECREF(qmp);
}
''')
--
2.4.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 10/16] qapi: Consistent generated code: prefer error 'err'
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 10/16] qapi: Consistent generated code: prefer error 'err' Eric Blake
@ 2015-09-29 13:46 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2015-09-29 13:46 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> We had some pointless differences in the generated code for visit,
> command marshalling, and events; unifying them makes it easier for
> future patches to consolidate to common helper functions.
>
> This is patch 1/4, focusing on naming the local error variable
> 'err' rather than 'local_err'.
Suggest "is the first of four patches", to avoid any confusion with the
numbering of patches within the series. Likewise for the other three.
>
> No change in semantics to the generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: split 9/46 into four patches, update docs where they are affected,
> rebase with 7/46 placed later in series
Review is now boring. That's good :)
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 11/16] qapi: Consistent generated code: prefer visitor 'v'
2015-09-29 3:27 [Qemu-devel] [PATCH v6 00/16] post-introspection cleanups, subset A Eric Blake
` (9 preceding siblings ...)
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 10/16] qapi: Consistent generated code: prefer error 'err' Eric Blake
@ 2015-09-29 3:27 ` Eric Blake
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 12/16] qapi: Consistent generated code: prefer common labels Eric Blake
` (4 subsequent siblings)
15 siblings, 0 replies; 39+ messages in thread
From: Eric Blake @ 2015-09-29 3:27 UTC (permalink / raw)
To: qemu-devel
Cc: Michael Roth, marcandre.lureau, armbru, Andreas Färber, ehabkost
We had some pointless differences in the generated code for visit,
command marshalling, and events; unifying them makes it easier for
future patches to consolidate to common helper functions.
This is patch 2/4, focusing on naming the local visitor variable
'v' rather than 'm'. Related objects, such as 'QapiDeallocVisitor',
are also named by their initials instead of an unrelated leading m.
No change in semantics to the generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: split 9/46 into four patches, update docs where they are affected,
rebase with 7/46 placed later in series, cover all uses in tree of
"...Visitor *m..."
Signed-off-by: Eric Blake <eblake@redhat.com>
---
docs/qapi-code-gen.txt | 74 ++++++++++++++++++++++++------------------------
qom/object.c | 18 ++++++------
qom/qom-qobject.c | 18 ++++++------
scripts/qapi-commands.py | 30 ++++++++++----------
scripts/qapi-types.py | 8 +++---
scripts/qapi-visit.py | 70 ++++++++++++++++++++++-----------------------
6 files changed, 109 insertions(+), 109 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index b99e6fa..2afab20 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -740,32 +740,32 @@ Example:
void qapi_free_UserDefOne(UserDefOne *obj)
{
- QapiDeallocVisitor *md;
+ QapiDeallocVisitor *qdv;
Visitor *v;
if (!obj) {
return;
}
- md = qapi_dealloc_visitor_new();
- v = qapi_dealloc_get_visitor(md);
+ qdv = qapi_dealloc_visitor_new();
+ v = qapi_dealloc_get_visitor(qdv);
visit_type_UserDefOne(v, &obj, NULL, NULL);
- qapi_dealloc_visitor_cleanup(md);
+ qapi_dealloc_visitor_cleanup(qdv);
}
void qapi_free_UserDefOneList(UserDefOneList *obj)
{
- QapiDeallocVisitor *md;
+ QapiDeallocVisitor *qdv;
Visitor *v;
if (!obj) {
return;
}
- md = qapi_dealloc_visitor_new();
- v = qapi_dealloc_get_visitor(md);
+ qdv = qapi_dealloc_visitor_new();
+ v = qapi_dealloc_get_visitor(qdv);
visit_type_UserDefOneList(v, &obj, NULL, NULL);
- qapi_dealloc_visitor_cleanup(md);
+ qapi_dealloc_visitor_cleanup(qdv);
}
$ cat qapi-generated/example-qapi-types.h
[Uninteresting stuff omitted...]
@@ -823,15 +823,15 @@ Example:
$ cat qapi-generated/example-qapi-visit.c
[Uninteresting stuff omitted...]
- static void visit_type_UserDefOne_fields(Visitor *m, UserDefOne **obj, Error **errp)
+ static void visit_type_UserDefOne_fields(Visitor *v, UserDefOne **obj, Error **errp)
{
Error *err = NULL;
- visit_type_int(m, &(*obj)->integer, "integer", &err);
+ visit_type_int(v, &(*obj)->integer, "integer", &err);
if (err) {
goto out;
}
- visit_type_str(m, &(*obj)->string, "string", &err);
+ visit_type_str(v, &(*obj)->string, "string", &err);
if (err) {
goto out;
}
@@ -840,40 +840,40 @@ Example:
error_propagate(errp, err);
}
- void visit_type_UserDefOne(Visitor *m, UserDefOne **obj, const char *name, Error **errp)
+ void visit_type_UserDefOne(Visitor *v, UserDefOne **obj, const char *name, Error **errp)
{
Error *err = NULL;
- visit_start_struct(m, (void **)obj, "UserDefOne", name, sizeof(UserDefOne), &err);
+ visit_start_struct(v, (void **)obj, "UserDefOne", name, sizeof(UserDefOne), &err);
if (!err) {
if (*obj) {
- visit_type_UserDefOne_fields(m, obj, errp);
+ visit_type_UserDefOne_fields(v, obj, errp);
}
- visit_end_struct(m, &err);
+ visit_end_struct(v, &err);
}
error_propagate(errp, err);
}
- void visit_type_UserDefOneList(Visitor *m, UserDefOneList **obj, const char *name, Error **errp)
+ void visit_type_UserDefOneList(Visitor *v, UserDefOneList **obj, const char *name, Error **errp)
{
Error *err = NULL;
GenericList *i, **prev;
- visit_start_list(m, name, &err);
+ visit_start_list(v, name, &err);
if (err) {
goto out;
}
for (prev = (GenericList **)obj;
- !err && (i = visit_next_list(m, prev, &err)) != NULL;
+ !err && (i = visit_next_list(v, prev, &err)) != NULL;
prev = &i) {
UserDefOneList *native_i = (UserDefOneList *)i;
- visit_type_UserDefOne(m, &native_i->value, NULL, &err);
+ visit_type_UserDefOne(v, &native_i->value, NULL, &err);
}
error_propagate(errp, err);
err = NULL;
- visit_end_list(m, &err);
+ visit_end_list(v, &err);
out:
error_propagate(errp, err);
}
@@ -885,8 +885,8 @@ Example:
[Visitors for built-in types omitted...]
- void visit_type_UserDefOne(Visitor *m, UserDefOne **obj, const char *name, Error **errp);
- void visit_type_UserDefOneList(Visitor *m, UserDefOneList **obj, const char *name, Error **errp);
+ void visit_type_UserDefOne(Visitor *v, UserDefOne **obj, const char *name, Error **errp);
+ void visit_type_UserDefOneList(Visitor *v, UserDefOneList **obj, const char *name, Error **errp);
#endif
@@ -917,36 +917,36 @@ Example:
static void qmp_marshal_output_UserDefOne(UserDefOne *ret_in, QObject **ret_out, Error **errp)
{
Error *err = NULL;
- QmpOutputVisitor *mo = qmp_output_visitor_new();
- QapiDeallocVisitor *md;
+ QmpOutputVisitor *qov = qmp_output_visitor_new();
+ QapiDeallocVisitor *qdv;
Visitor *v;
- v = qmp_output_get_visitor(mo);
+ v = qmp_output_get_visitor(qov);
visit_type_UserDefOne(v, &ret_in, "unused", &err);
if (err) {
goto out;
}
- *ret_out = qmp_output_get_qobject(mo);
+ *ret_out = qmp_output_get_qobject(qov);
out:
error_propagate(errp, err);
- qmp_output_visitor_cleanup(mo);
- md = qapi_dealloc_visitor_new();
- v = qapi_dealloc_get_visitor(md);
+ qmp_output_visitor_cleanup(qov);
+ qdv = qapi_dealloc_visitor_new();
+ v = qapi_dealloc_get_visitor(qdv);
visit_type_UserDefOne(v, &ret_in, "unused", NULL);
- qapi_dealloc_visitor_cleanup(md);
+ qapi_dealloc_visitor_cleanup(qdv);
}
static void qmp_marshal_my_command(QDict *args, QObject **ret, Error **errp)
{
Error *err = NULL;
UserDefOne *retval;
- QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
- QapiDeallocVisitor *md;
+ QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
+ QapiDeallocVisitor *qdv;
Visitor *v;
UserDefOne *arg1 = NULL;
- v = qmp_input_get_visitor(mi);
+ v = qmp_input_get_visitor(qiv);
visit_type_UserDefOne(v, &arg1, "arg1", &err);
if (err) {
goto out;
@@ -961,11 +961,11 @@ Example:
out:
error_propagate(errp, err);
- qmp_input_visitor_cleanup(mi);
- md = qapi_dealloc_visitor_new();
- v = qapi_dealloc_get_visitor(md);
+ qmp_input_visitor_cleanup(qiv);
+ qdv = qapi_dealloc_visitor_new();
+ v = qapi_dealloc_get_visitor(qdv);
visit_type_UserDefOne(v, &arg1, "arg1", NULL);
- qapi_dealloc_visitor_cleanup(md);
+ qapi_dealloc_visitor_cleanup(qdv);
}
static void qmp_init_marshal(void)
diff --git a/qom/object.c b/qom/object.c
index 4805328..11cd86b 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1167,31 +1167,31 @@ out:
void object_property_parse(Object *obj, const char *string,
const char *name, Error **errp)
{
- StringInputVisitor *mi;
- mi = string_input_visitor_new(string);
- object_property_set(obj, string_input_get_visitor(mi), name, errp);
+ StringInputVisitor *siv;
+ siv = string_input_visitor_new(string);
+ object_property_set(obj, string_input_get_visitor(siv), name, errp);
- string_input_visitor_cleanup(mi);
+ string_input_visitor_cleanup(siv);
}
char *object_property_print(Object *obj, const char *name, bool human,
Error **errp)
{
- StringOutputVisitor *mo;
+ StringOutputVisitor *sov;
char *string = NULL;
Error *local_err = NULL;
- mo = string_output_visitor_new(human);
- object_property_get(obj, string_output_get_visitor(mo), name, &local_err);
+ sov = string_output_visitor_new(human);
+ object_property_get(obj, string_output_get_visitor(sov), name, &local_err);
if (local_err) {
error_propagate(errp, local_err);
goto out;
}
- string = string_output_get_string(mo);
+ string = string_output_get_string(sov);
out:
- string_output_visitor_cleanup(mo);
+ string_output_visitor_cleanup(sov);
return string;
}
diff --git a/qom/qom-qobject.c b/qom/qom-qobject.c
index 6384b8e..9649890 100644
--- a/qom/qom-qobject.c
+++ b/qom/qom-qobject.c
@@ -19,11 +19,11 @@
void object_property_set_qobject(Object *obj, QObject *value,
const char *name, Error **errp)
{
- QmpInputVisitor *mi;
- mi = qmp_input_visitor_new(value);
- object_property_set(obj, qmp_input_get_visitor(mi), name, errp);
+ QmpInputVisitor *qiv;
+ qiv = qmp_input_visitor_new(value);
+ object_property_set(obj, qmp_input_get_visitor(qiv), name, errp);
- qmp_input_visitor_cleanup(mi);
+ qmp_input_visitor_cleanup(qiv);
}
QObject *object_property_get_qobject(Object *obj, const char *name,
@@ -31,14 +31,14 @@ QObject *object_property_get_qobject(Object *obj, const char *name,
{
QObject *ret = NULL;
Error *local_err = NULL;
- QmpOutputVisitor *mo;
+ QmpOutputVisitor *qov;
- mo = qmp_output_visitor_new();
- object_property_get(obj, qmp_output_get_visitor(mo), name, &local_err);
+ qov = qmp_output_visitor_new();
+ object_property_get(obj, qmp_output_get_visitor(qov), name, &local_err);
if (!local_err) {
- ret = qmp_output_get_qobject(mo);
+ ret = qmp_output_get_qobject(qov);
}
error_propagate(errp, local_err);
- qmp_output_visitor_cleanup(mo);
+ qmp_output_visitor_cleanup(qov);
return ret;
}
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 2151120..475735d 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -82,8 +82,8 @@ def gen_marshal_vars(arg_type, ret_type):
if arg_type:
ret += mcgen('''
-QmpInputVisitor *mi = qmp_input_visitor_new_strict(QOBJECT(args));
-QapiDeallocVisitor *md;
+QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
+QapiDeallocVisitor *qdv;
Visitor *v;
''')
@@ -122,15 +122,15 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
errparg = 'NULL'
errarg = None
ret += mcgen('''
-qmp_input_visitor_cleanup(mi);
-md = qapi_dealloc_visitor_new();
-v = qapi_dealloc_get_visitor(md);
+qmp_input_visitor_cleanup(qiv);
+qdv = qapi_dealloc_visitor_new();
+v = qapi_dealloc_get_visitor(qdv);
''')
else:
errparg = '&err'
errarg = 'err'
ret += mcgen('''
-v = qmp_input_get_visitor(mi);
+v = qmp_input_get_visitor(qiv);
''')
for memb in arg_type.members:
@@ -160,7 +160,7 @@ visit_type_%(c_type)s(v, &%(c_name)s, "%(name)s", %(errp)s);
if dealloc:
ret += mcgen('''
-qapi_dealloc_visitor_cleanup(md);
+qapi_dealloc_visitor_cleanup(qdv);
''')
pop_indent()
return ret
@@ -172,24 +172,24 @@ def gen_marshal_output(ret_type):
static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out, Error **errp)
{
Error *err = NULL;
- QmpOutputVisitor *mo = qmp_output_visitor_new();
- QapiDeallocVisitor *md;
+ QmpOutputVisitor *qov = qmp_output_visitor_new();
+ QapiDeallocVisitor *qdv;
Visitor *v;
- v = qmp_output_get_visitor(mo);
+ v = qmp_output_get_visitor(qov);
visit_type_%(c_name)s(v, &ret_in, "unused", &err);
if (err) {
goto out;
}
- *ret_out = qmp_output_get_qobject(mo);
+ *ret_out = qmp_output_get_qobject(qov);
out:
error_propagate(errp, err);
- qmp_output_visitor_cleanup(mo);
- md = qapi_dealloc_visitor_new();
- v = qapi_dealloc_get_visitor(md);
+ qmp_output_visitor_cleanup(qov);
+ qdv = qapi_dealloc_visitor_new();
+ v = qapi_dealloc_get_visitor(qdv);
visit_type_%(c_name)s(v, &ret_in, "unused", NULL);
- qapi_dealloc_visitor_cleanup(md);
+ qapi_dealloc_visitor_cleanup(qdv);
}
''',
c_type=ret_type.c_type(), c_name=ret_type.c_name())
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index b292682..d405f8d 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -188,17 +188,17 @@ def gen_type_cleanup(name):
void qapi_free_%(c_name)s(%(c_name)s *obj)
{
- QapiDeallocVisitor *md;
+ QapiDeallocVisitor *qdv;
Visitor *v;
if (!obj) {
return;
}
- md = qapi_dealloc_visitor_new();
- v = qapi_dealloc_get_visitor(md);
+ qdv = qapi_dealloc_visitor_new();
+ v = qapi_dealloc_get_visitor(qdv);
visit_type_%(c_name)s(v, &obj, NULL, NULL);
- qapi_dealloc_visitor_cleanup(md);
+ qapi_dealloc_visitor_cleanup(qdv);
}
''',
c_name=c_name(name))
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 97343cf..348efe0 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -24,7 +24,7 @@ def gen_visit_decl(name, scalar=False):
if not scalar:
c_type += '*'
return mcgen('''
-void visit_type_%(c_name)s(Visitor *m, %(c_type)sobj, const char *name, Error **errp);
+void visit_type_%(c_name)s(Visitor *v, %(c_type)sobj, const char *name, Error **errp);
''',
c_name=c_name(name), c_type=c_type)
@@ -39,20 +39,20 @@ def gen_visit_implicit_struct(typ):
# Need a forward declaration
ret += mcgen('''
-static void visit_type_%(c_type)s_fields(Visitor *m, %(c_type)s **obj, Error **errp);
+static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error **errp);
''',
c_type=typ.c_name())
ret += mcgen('''
-static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error **errp)
+static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error **errp)
{
Error *err = NULL;
- visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), &err);
+ visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err);
if (!err) {
- visit_type_%(c_type)s_fields(m, obj, errp);
- visit_end_implicit_struct(m, &err);
+ visit_type_%(c_type)s_fields(v, obj, errp);
+ visit_end_implicit_struct(v, &err);
}
error_propagate(errp, err);
}
@@ -71,7 +71,7 @@ def gen_visit_struct_fields(name, base, members):
ret += mcgen('''
-static void visit_type_%(c_name)s_fields(Visitor *m, %(c_name)s **obj, Error **errp)
+static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **errp)
{
Error *err = NULL;
@@ -81,7 +81,7 @@ static void visit_type_%(c_name)s_fields(Visitor *m, %(c_name)s **obj, Error **e
if base:
ret += mcgen('''
-visit_type_implicit_%(c_type)s(m, &(*obj)->%(c_name)s, &err);
+visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
if (err) {
goto out;
}
@@ -91,14 +91,14 @@ if (err) {
for memb in members:
if memb.optional:
ret += mcgen('''
-visit_optional(m, &(*obj)->has_%(c_name)s, "%(name)s", &err);
+visit_optional(v, &(*obj)->has_%(c_name)s, "%(name)s", &err);
if (!err && (*obj)->has_%(c_name)s) {
''',
c_name=c_name(memb.name), name=memb.name)
push_indent()
ret += mcgen('''
-visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "%(name)s", &err);
+visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
''',
c_type=memb.type.c_name(), c_name=c_name(memb.name),
name=memb.name)
@@ -136,16 +136,16 @@ def gen_visit_struct(name, base, members):
# call qapi_free_FOO() to avoid a memory leak of the partial FOO.
ret += mcgen('''
-void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
+void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
{
Error *err = NULL;
- visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
+ visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
if (!err) {
if (*obj) {
- visit_type_%(c_name)s_fields(m, obj, errp);
+ visit_type_%(c_name)s_fields(v, obj, errp);
}
- visit_end_struct(m, &err);
+ visit_end_struct(v, &err);
}
error_propagate(errp, err);
}
@@ -158,26 +158,26 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
def gen_visit_list(name, element_type):
return mcgen('''
-void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
+void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
{
Error *err = NULL;
GenericList *i, **prev;
- visit_start_list(m, name, &err);
+ visit_start_list(v, name, &err);
if (err) {
goto out;
}
for (prev = (GenericList **)obj;
- !err && (i = visit_next_list(m, prev, &err)) != NULL;
+ !err && (i = visit_next_list(v, prev, &err)) != NULL;
prev = &i) {
%(c_name)s *native_i = (%(c_name)s *)i;
- visit_type_%(c_elt_type)s(m, &native_i->value, NULL, &err);
+ visit_type_%(c_elt_type)s(v, &native_i->value, NULL, &err);
}
error_propagate(errp, err);
err = NULL;
- visit_end_list(m, &err);
+ visit_end_list(v, &err);
out:
error_propagate(errp, err);
}
@@ -188,9 +188,9 @@ out:
def gen_visit_enum(name):
return mcgen('''
-void visit_type_%(c_name)s(Visitor *m, %(c_name)s *obj, const char *name, Error **errp)
+void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name, Error **errp)
{
- visit_type_enum(m, (int *)obj, %(c_name)s_lookup, "%(name)s", name, errp);
+ visit_type_enum(v, (int *)obj, %(c_name)s_lookup, "%(name)s", name, errp);
}
''',
c_name=c_name(name), name=name)
@@ -199,15 +199,15 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s *obj, const char *name, Error
def gen_visit_alternate(name, variants):
ret = mcgen('''
-void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
+void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
{
Error *err = NULL;
- visit_start_implicit_struct(m, (void**) obj, sizeof(%(c_name)s), &err);
+ visit_start_implicit_struct(v, (void**) obj, sizeof(%(c_name)s), &err);
if (err) {
goto out;
}
- visit_get_next_type(m, (int*) &(*obj)->kind, %(c_name)s_qtypes, name, &err);
+ visit_get_next_type(v, (int*) &(*obj)->kind, %(c_name)s_qtypes, name, &err);
if (err) {
goto out_end;
}
@@ -218,7 +218,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
for var in variants.variants:
ret += mcgen('''
case %(case)s:
- visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err);
+ visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, name, &err);
break;
''',
case=c_enum_const(variants.tag_member.type.name,
@@ -233,7 +233,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
out_end:
error_propagate(errp, err);
err = NULL;
- visit_end_implicit_struct(m, &err);
+ visit_end_implicit_struct(v, &err);
out:
error_propagate(errp, err);
}
@@ -256,11 +256,11 @@ def gen_visit_union(name, base, variants):
ret += mcgen('''
-void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
+void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
{
Error *err = NULL;
- visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
+ visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
if (err) {
goto out;
}
@@ -270,7 +270,7 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
if base:
ret += mcgen('''
- visit_type_%(c_name)s_fields(m, obj, &err);
+ visit_type_%(c_name)s_fields(v, obj, &err);
if (err) {
goto out_obj;
}
@@ -282,11 +282,11 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
# we pointlessly use a different key for simple unions
tag_key = 'type'
ret += mcgen('''
- visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "%(name)s", &err);
+ visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
if (err) {
goto out_obj;
}
- if (!visit_start_union(m, !!(*obj)->data, &err) || err) {
+ if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
goto out_obj;
}
switch ((*obj)->%(c_name)s) {
@@ -308,13 +308,13 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
var.name))
if simple_union_type:
ret += mcgen('''
- visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);
+ visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "data", &err);
''',
c_type=simple_union_type.c_name(),
c_name=c_name(var.name))
else:
ret += mcgen('''
- visit_type_implicit_%(c_type)s(m, &(*obj)->%(c_name)s, &err);
+ visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
''',
c_type=var.type.c_name(),
c_name=c_name(var.name))
@@ -329,11 +329,11 @@ void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
out_obj:
error_propagate(errp, err);
err = NULL;
- visit_end_union(m, !!(*obj)->data, &err);
+ visit_end_union(v, !!(*obj)->data, &err);
error_propagate(errp, err);
err = NULL;
}
- visit_end_struct(m, &err);
+ visit_end_struct(v, &err);
out:
error_propagate(errp, err);
}
--
2.4.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v6 12/16] qapi: Consistent generated code: prefer common labels
2015-09-29 3:27 [Qemu-devel] [PATCH v6 00/16] post-introspection cleanups, subset A Eric Blake
` (10 preceding siblings ...)
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 11/16] qapi: Consistent generated code: prefer visitor 'v' Eric Blake
@ 2015-09-29 3:27 ` Eric Blake
2015-09-29 13:56 ` Markus Armbruster
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 13/16] qapi: Consistent generated code: prefer common indentation Eric Blake
` (3 subsequent siblings)
15 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2015-09-29 3:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
We had some pointless differences in the generated code for visit,
command marshalling, and events; unifying them makes it easier for
future patches to consolidate to common helper functions.
This is patch 3/4, focusing on naming the goto labels 'out' (not
'clean') and 'out_obj' (not 'out_end'). Additionally, the generator
was inconsistent on whether labels had a leading space; prefer the
emacs style of including the space, so that only function names and
comments occur in column 1 of the generated file [HACKING gives no
preference, and both styles occur elsewhere in qemu.git].
No change in semantics to the generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: split 9/46 into four patches, update docs where they are affected,
pick consistent spacing of label
Signed-off-by: Eric Blake <eblake@redhat.com>
---
docs/qapi-code-gen.txt | 8 ++++----
scripts/qapi-commands.py | 4 ++--
scripts/qapi-event.py | 8 ++++----
scripts/qapi-visit.py | 14 +++++++-------
4 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 2afab20..fd390ef 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -836,7 +836,7 @@ Example:
goto out;
}
- out:
+ out:
error_propagate(errp, err);
}
@@ -874,7 +874,7 @@ Example:
error_propagate(errp, err);
err = NULL;
visit_end_list(v, &err);
- out:
+ out:
error_propagate(errp, err);
}
$ cat qapi-generated/example-qapi-visit.h
@@ -928,7 +928,7 @@ Example:
}
*ret_out = qmp_output_get_qobject(qov);
- out:
+ out:
error_propagate(errp, err);
qmp_output_visitor_cleanup(qov);
qdv = qapi_dealloc_visitor_new();
@@ -959,7 +959,7 @@ Example:
qmp_marshal_output_UserDefOne(retval, ret, &err);
- out:
+ out:
error_propagate(errp, err);
qmp_input_visitor_cleanup(qiv);
qdv = qapi_dealloc_visitor_new();
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 475735d..8fab1ce 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -183,7 +183,7 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out,
}
*ret_out = qmp_output_get_qobject(qov);
-out:
+ out:
error_propagate(errp, err);
qmp_output_visitor_cleanup(qov);
qdv = qapi_dealloc_visitor_new();
@@ -224,7 +224,7 @@ def gen_marshal(name, arg_type, ret_type):
if re.search('^ *goto out;', ret, re.MULTILINE):
ret += mcgen('''
-out:
+ out:
''')
ret += mcgen('''
error_propagate(errp, err);
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index d41af40..b43bbc2 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -69,7 +69,7 @@ def gen_event_send(name, arg_type):
/* Fake visit, as if all members are under a structure */
visit_start_struct(v, NULL, "", "%(name)s", 0, &err);
if (err) {
- goto clean;
+ goto out;
}
''',
@@ -92,7 +92,7 @@ def gen_event_send(name, arg_type):
ret += mcgen('''
visit_type_%(c_type)s(v, %(cast)s&%(c_name)s, "%(name)s", &err);
if (err) {
- goto clean;
+ goto out;
}
''',
cast=cast,
@@ -110,7 +110,7 @@ def gen_event_send(name, arg_type):
visit_end_struct(v, &err);
if (err) {
- goto clean;
+ goto out;
}
obj = qmp_output_get_qobject(qov);
@@ -127,7 +127,7 @@ def gen_event_send(name, arg_type):
if arg_type and arg_type.members:
ret += mcgen('''
- clean:
+ out:
qmp_output_visitor_cleanup(qov);
''')
ret += mcgen('''
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 348efe0..a5d7be1 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -118,7 +118,7 @@ if (err) {
if re.search('^ *goto out;', ret, re.MULTILINE):
ret += mcgen('''
-out:
+ out:
''')
ret += mcgen('''
error_propagate(errp, err);
@@ -178,7 +178,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
error_propagate(errp, err);
err = NULL;
visit_end_list(v, &err);
-out:
+ out:
error_propagate(errp, err);
}
''',
@@ -209,7 +209,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
}
visit_get_next_type(v, (int*) &(*obj)->kind, %(c_name)s_qtypes, name, &err);
if (err) {
- goto out_end;
+ goto out_obj;
}
switch ((*obj)->kind) {
''',
@@ -230,11 +230,11 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
default:
abort();
}
-out_end:
+ out_obj:
error_propagate(errp, err);
err = NULL;
visit_end_implicit_struct(v, &err);
-out:
+ out:
error_propagate(errp, err);
}
''')
@@ -326,7 +326,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
default:
abort();
}
-out_obj:
+ out_obj:
error_propagate(errp, err);
err = NULL;
visit_end_union(v, !!(*obj)->data, &err);
@@ -334,7 +334,7 @@ out_obj:
err = NULL;
}
visit_end_struct(v, &err);
-out:
+ out:
error_propagate(errp, err);
}
''')
--
2.4.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 12/16] qapi: Consistent generated code: prefer common labels
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 12/16] qapi: Consistent generated code: prefer common labels Eric Blake
@ 2015-09-29 13:56 ` Markus Armbruster
2015-09-29 14:59 ` Eric Blake
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2015-09-29 13:56 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> We had some pointless differences in the generated code for visit,
> command marshalling, and events; unifying them makes it easier for
> future patches to consolidate to common helper functions.
>
> This is patch 3/4, focusing on naming the goto labels 'out' (not
> 'clean') and 'out_obj' (not 'out_end'). Additionally, the generator
> was inconsistent on whether labels had a leading space; prefer the
> emacs style of including the space, so that only function names and
> comments occur in column 1 of the generated file [HACKING gives no
> preference, and both styles occur elsewhere in qemu.git].
Your Emacs prefers this? Mine doesn't.
If I read CC Mode correctly, only style "gnu" does this. I use
"stroustrup" for QEMU, because it's very close. It doesn't do this.
As far as I can see, the only label we generate indented before this
patch is qapi-event.py's clean.
When in doubt, avoid the churn: we rename that one anyway, so let's drop
the space there, and leave the others alone.
> No change in semantics to the generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: split 9/46 into four patches, update docs where they are affected,
> pick consistent spacing of label
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 12/16] qapi: Consistent generated code: prefer common labels
2015-09-29 13:56 ` Markus Armbruster
@ 2015-09-29 14:59 ` Eric Blake
0 siblings, 0 replies; 39+ messages in thread
From: Eric Blake @ 2015-09-29 14:59 UTC (permalink / raw)
To: Markus Armbruster
Cc: marcandre.lureau, Michael Tokarev, qemu-devel, ehabkost, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2563 bytes --]
On 09/29/2015 07:56 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> We had some pointless differences in the generated code for visit,
>> command marshalling, and events; unifying them makes it easier for
>> future patches to consolidate to common helper functions.
>>
>> This is patch 3/4, focusing on naming the goto labels 'out' (not
>> 'clean') and 'out_obj' (not 'out_end'). Additionally, the generator
>> was inconsistent on whether labels had a leading space; prefer the
>> emacs style of including the space, so that only function names and
>> comments occur in column 1 of the generated file [HACKING gives no
>> preference, and both styles occur elsewhere in qemu.git].
>
> Your Emacs prefers this? Mine doesn't.
I'm using Dan's patch "Add .dir-locals.el file to configure emacs coding
style"; which has been claimed to be on the trivial patch queue
(although I haven't seen a trivial pull request since that date, nor can
I find that queue pushed anywhere public):
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg04148.html
>
> If I read CC Mode correctly, only style "gnu" does this. I use
> "stroustrup" for QEMU, because it's very close. It doesn't do this.
Dan's patch uses "K&R" rather than "stroustrup", although the two are
similar. Digging deeper, I see that my buffer when visiting a .c in
qemu sets c-label-minimum-indentation to 1, and attributes it to
set-from-style; but I'm not sure why I'm getting it, as I agree with
your reading of c-style-alist that c-gnu-impose-minimum (which is what
typically sets c-label-minimum-indentation to 1) is only tied to "gnu"
mode. So I'm lost as to why I'm seeing different behavior.
>
> As far as I can see, the only label we generate indented before this
> patch is qapi-event.py's clean.
>
> When in doubt, avoid the churn: we rename that one anyway, so let's drop
> the space there, and leave the others alone.
Sure, I can go that way for less churn, and since I can't figure out why
it's getting set for me.
>
>> No change in semantics to the generated code.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v6: split 9/46 into four patches, update docs where they are affected,
>> pick consistent spacing of label
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
Oh weird - I didn't notice the double s-o-b, but the second one gets
lost during 'git am'.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 13/16] qapi: Consistent generated code: prefer common indentation
2015-09-29 3:27 [Qemu-devel] [PATCH v6 00/16] post-introspection cleanups, subset A Eric Blake
` (11 preceding siblings ...)
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 12/16] qapi: Consistent generated code: prefer common labels Eric Blake
@ 2015-09-29 3:27 ` Eric Blake
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 14/16] qapi: Consistent generated code: minimize push_indent() usage Eric Blake
` (2 subsequent siblings)
15 siblings, 0 replies; 39+ messages in thread
From: Eric Blake @ 2015-09-29 3:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
We had some pointless differences in the generated code for visit,
command marshalling, and events; unifying them makes it easier for
future patches to consolidate to common helper functions.
This is patch 4/4, focusing on adjusting gen_visit_union() to use
the same indentation as other functions, namely, by jumping early
to the error label if the object was not set rather than placing
the rest of the body inside an if for when it is set.
No change in semantics to the generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: split 9/46 into four patches, update docs where they are affected,
rebase with 7/46 placed later in series
Signed-off-by: Eric Blake <eblake@redhat.com>
---
scripts/qapi-visit.py | 53 ++++++++++++++++++++++++++-------------------------
1 file changed, 27 insertions(+), 26 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index a5d7be1..3b1e7b0 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -264,16 +264,18 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
if (err) {
goto out;
}
- if (*obj) {
+ if (!*obj) {
+ goto out_obj;
+ }
''',
c_name=c_name(name), name=name)
if base:
ret += mcgen('''
- visit_type_%(c_name)s_fields(v, obj, &err);
- if (err) {
- goto out_obj;
- }
+ visit_type_%(c_name)s_fields(v, obj, &err);
+ if (err) {
+ goto out_obj;
+ }
''',
c_name=c_name(name))
@@ -282,14 +284,14 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
# we pointlessly use a different key for simple unions
tag_key = 'type'
ret += mcgen('''
- visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
- if (err) {
- goto out_obj;
- }
- if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
- goto out_obj;
- }
- switch ((*obj)->%(c_name)s) {
+ visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
+ if (err) {
+ goto out_obj;
+ }
+ if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
+ goto out_obj;
+ }
+ switch ((*obj)->%(c_name)s) {
''',
c_type=variants.tag_member.type.c_name(),
# TODO ugly special case for simple union
@@ -302,37 +304,36 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
# TODO ugly special case for simple union
simple_union_type = var.simple_union_type()
ret += mcgen('''
- case %(case)s:
+ case %(case)s:
''',
case=c_enum_const(variants.tag_member.type.name,
var.name))
if simple_union_type:
ret += mcgen('''
- visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "data", &err);
+ visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "data", &err);
''',
c_type=simple_union_type.c_name(),
c_name=c_name(var.name))
else:
ret += mcgen('''
- visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
+ visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
''',
c_type=var.type.c_name(),
c_name=c_name(var.name))
ret += mcgen('''
- break;
+ break;
''')
ret += mcgen('''
- default:
- abort();
- }
- out_obj:
- error_propagate(errp, err);
- err = NULL;
- visit_end_union(v, !!(*obj)->data, &err);
- error_propagate(errp, err);
- err = NULL;
+ default:
+ abort();
}
+ out_obj:
+ error_propagate(errp, err);
+ err = NULL;
+ visit_end_union(v, !!(*obj)->data, &err);
+ error_propagate(errp, err);
+ err = NULL;
visit_end_struct(v, &err);
out:
error_propagate(errp, err);
--
2.4.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v6 14/16] qapi: Consistent generated code: minimize push_indent() usage
2015-09-29 3:27 [Qemu-devel] [PATCH v6 00/16] post-introspection cleanups, subset A Eric Blake
` (12 preceding siblings ...)
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 13/16] qapi: Consistent generated code: prefer common indentation Eric Blake
@ 2015-09-29 3:27 ` Eric Blake
2015-09-29 14:10 ` Markus Armbruster
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 15/16] qapi: Share gen_err_check() Eric Blake
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 16/16] qapi: Share gen_visit_fields() Eric Blake
15 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2015-09-29 3:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
We had some pointless differences in the generated code for visit,
command marshalling, and events; unifying them makes it easier for
future patches to consolidate to common helper functions.
This patch makes no difference to the generated code, but rather
focuses on reducing the number of push_indent()/pop_indent() pairs
so that generated code is typically already at its natural output
indentation in the python files. It is easier to reason about
generated code if the reader does not have to track how much
spacing will be inserted alongside the code, and moreso when all
of the generators use the same patterns (qapi-type and qapi-event
were already using in-place indentation).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: new patch, but based on theme of 9/46 and code sharing of 10/46
---
scripts/qapi-commands.py | 54 ++++++++++++++++++++----------------------------
scripts/qapi-visit.py | 24 ++++++++++-----------
2 files changed, 33 insertions(+), 45 deletions(-)
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 8fab1ce..1d3c0d5 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -29,9 +29,9 @@ def gen_err_check(err):
if not err:
return ''
return mcgen('''
-if (%(err)s) {
- goto out;
-}
+ if (%(err)s) {
+ goto out;
+ }
''',
err=err)
@@ -50,20 +50,18 @@ def gen_call(name, arg_type, ret_type):
if ret_type:
lhs = 'retval = '
- push_indent()
ret = mcgen('''
-%(lhs)sqmp_%(c_name)s(%(args)s&err);
+ %(lhs)sqmp_%(c_name)s(%(args)s&err);
''',
c_name=c_name(name), args=argstr, lhs=lhs)
if ret_type:
ret += gen_err_check('err')
ret += mcgen('''
-qmp_marshal_output_%(c_name)s(retval, ret, &err);
+ qmp_marshal_output_%(c_name)s(retval, ret, &err);
''',
c_name=ret_type.c_name())
- pop_indent()
return ret
@@ -72,29 +70,27 @@ def gen_marshal_vars(arg_type, ret_type):
Error *err = NULL;
''')
- push_indent()
-
if ret_type:
ret += mcgen('''
-%(c_type)s retval;
+ %(c_type)s retval;
''',
c_type=ret_type.c_type())
if arg_type:
ret += mcgen('''
-QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
-QapiDeallocVisitor *qdv;
-Visitor *v;
+ QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
+ QapiDeallocVisitor *qdv;
+ Visitor *v;
''')
for memb in arg_type.members:
if memb.optional:
ret += mcgen('''
-bool has_%(c_name)s = false;
+ bool has_%(c_name)s = false;
''',
c_name=c_name(memb.name))
ret += mcgen('''
-%(c_type)s %(c_name)s = %(c_null)s;
+ %(c_type)s %(c_name)s = %(c_null)s;
''',
c_name=c_name(memb.name),
c_type=memb.type.c_type(),
@@ -103,10 +99,9 @@ bool has_%(c_name)s = false;
else:
ret += mcgen('''
-(void)args;
+ (void)args;
''')
- pop_indent()
return ret
@@ -116,38 +111,36 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
if not arg_type:
return ret
- push_indent()
-
if dealloc:
errparg = 'NULL'
errarg = None
ret += mcgen('''
-qmp_input_visitor_cleanup(qiv);
-qdv = qapi_dealloc_visitor_new();
-v = qapi_dealloc_get_visitor(qdv);
+ qmp_input_visitor_cleanup(qiv);
+ qdv = qapi_dealloc_visitor_new();
+ v = qapi_dealloc_get_visitor(qdv);
''')
else:
errparg = '&err'
errarg = 'err'
ret += mcgen('''
-v = qmp_input_get_visitor(qiv);
+ v = qmp_input_get_visitor(qiv);
''')
for memb in arg_type.members:
if memb.optional:
ret += mcgen('''
-visit_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s);
+ visit_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s);
''',
c_name=c_name(memb.name), name=memb.name,
errp=errparg)
ret += gen_err_check(errarg)
ret += mcgen('''
-if (has_%(c_name)s) {
+ if (has_%(c_name)s) {
''',
c_name=c_name(memb.name))
push_indent()
ret += mcgen('''
-visit_type_%(c_type)s(v, &%(c_name)s, "%(name)s", %(errp)s);
+ visit_type_%(c_type)s(v, &%(c_name)s, "%(name)s", %(errp)s);
''',
c_name=c_name(memb.name), name=memb.name,
c_type=memb.type.c_name(), errp=errparg)
@@ -155,14 +148,13 @@ visit_type_%(c_type)s(v, &%(c_name)s, "%(name)s", %(errp)s);
if memb.optional:
pop_indent()
ret += mcgen('''
-}
+ }
''')
if dealloc:
ret += mcgen('''
-qapi_dealloc_visitor_cleanup(qdv);
+ qapi_dealloc_visitor_cleanup(qdv);
''')
- pop_indent()
return ret
@@ -237,17 +229,15 @@ def gen_marshal(name, arg_type, ret_type):
def gen_register_command(name, success_response):
- push_indent()
options = 'QCO_NO_OPTIONS'
if not success_response:
options = 'QCO_NO_SUCCESS_RESP'
ret = mcgen('''
-qmp_register_command("%(name)s", qmp_marshal_%(c_name)s, %(opts)s);
+ qmp_register_command("%(name)s", qmp_marshal_%(c_name)s, %(opts)s);
''',
name=name, c_name=c_name(name),
opts=options)
- pop_indent()
return ret
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 3b1e7b0..7ce8616 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -77,28 +77,27 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
''',
c_name=c_name(name))
- push_indent()
if base:
ret += mcgen('''
-visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
-if (err) {
- goto out;
-}
+ visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
+ if (err) {
+ goto out;
+ }
''',
c_type=base.c_name(), c_name=c_name('base'))
for memb in members:
if memb.optional:
ret += mcgen('''
-visit_optional(v, &(*obj)->has_%(c_name)s, "%(name)s", &err);
-if (!err && (*obj)->has_%(c_name)s) {
+ visit_optional(v, &(*obj)->has_%(c_name)s, "%(name)s", &err);
+ if (!err && (*obj)->has_%(c_name)s) {
''',
c_name=c_name(memb.name), name=memb.name)
push_indent()
ret += mcgen('''
-visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
+ visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
''',
c_type=memb.type.c_name(), c_name=c_name(memb.name),
name=memb.name)
@@ -106,15 +105,14 @@ visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
if memb.optional:
pop_indent()
ret += mcgen('''
-}
+ }
''')
ret += mcgen('''
-if (err) {
- goto out;
-}
+ if (err) {
+ goto out;
+ }
''')
- pop_indent()
if re.search('^ *goto out;', ret, re.MULTILINE):
ret += mcgen('''
--
2.4.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 14/16] qapi: Consistent generated code: minimize push_indent() usage
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 14/16] qapi: Consistent generated code: minimize push_indent() usage Eric Blake
@ 2015-09-29 14:10 ` Markus Armbruster
2015-09-29 15:07 ` Eric Blake
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2015-09-29 14:10 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> We had some pointless differences in the generated code for visit,
> command marshalling, and events; unifying them makes it easier for
> future patches to consolidate to common helper functions.
>
> This patch makes no difference to the generated code, but rather
> focuses on reducing the number of push_indent()/pop_indent() pairs
> so that generated code is typically already at its natural output
> indentation in the python files. It is easier to reason about
> generated code if the reader does not have to track how much
> spacing will be inserted alongside the code, and moreso when all
> of the generators use the same patterns (qapi-type and qapi-event
> were already using in-place indentation).
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
A few places become slightly less readable because the generated code is
now indented lile the surrounding Python code. Other places become more
readable. Having to think less about push_indent() / pop_indent() tips
the balance, I guess.
Have you diffed the generated code before and after the patch?
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 14/16] qapi: Consistent generated code: minimize push_indent() usage
2015-09-29 14:10 ` Markus Armbruster
@ 2015-09-29 15:07 ` Eric Blake
2015-10-01 6:26 ` Markus Armbruster
0 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2015-09-29 15:07 UTC (permalink / raw)
To: Markus Armbruster; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1405 bytes --]
On 09/29/2015 08:10 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> We had some pointless differences in the generated code for visit,
>> command marshalling, and events; unifying them makes it easier for
>> future patches to consolidate to common helper functions.
>>
>> This patch makes no difference to the generated code, but rather
>> focuses on reducing the number of push_indent()/pop_indent() pairs
>> so that generated code is typically already at its natural output
>> indentation in the python files. It is easier to reason about
>> generated code if the reader does not have to track how much
>> spacing will be inserted alongside the code, and moreso when all
>> of the generators use the same patterns (qapi-type and qapi-event
>> were already using in-place indentation).
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> A few places become slightly less readable because the generated code is
> now indented lile the surrounding Python code. Other places become more
> readable. Having to think less about push_indent() / pop_indent() tips
> the balance, I guess.
>
> Have you diffed the generated code before and after the patch?
Yes; quoting above:
>> This patch makes no difference to the generated code,
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 14/16] qapi: Consistent generated code: minimize push_indent() usage
2015-09-29 15:07 ` Eric Blake
@ 2015-10-01 6:26 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2015-10-01 6:26 UTC (permalink / raw)
To: Eric Blake; +Cc: Michael Roth, marcandre.lureau, qemu-devel, ehabkost
Eric Blake <eblake@redhat.com> writes:
> On 09/29/2015 08:10 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> We had some pointless differences in the generated code for visit,
>>> command marshalling, and events; unifying them makes it easier for
>>> future patches to consolidate to common helper functions.
>>>
>>> This patch makes no difference to the generated code, but rather
>>> focuses on reducing the number of push_indent()/pop_indent() pairs
>>> so that generated code is typically already at its natural output
>>> indentation in the python files. It is easier to reason about
>>> generated code if the reader does not have to track how much
>>> spacing will be inserted alongside the code, and moreso when all
>>> of the generators use the same patterns (qapi-type and qapi-event
>>> were already using in-place indentation).
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> A few places become slightly less readable because the generated code is
>> now indented lile the surrounding Python code. Other places become more
>> readable. Having to think less about push_indent() / pop_indent() tips
>> the balance, I guess.
>>
>> Have you diffed the generated code before and after the patch?
>
> Yes; quoting above:
>
>>> This patch makes no difference to the generated code,
Missed it, my apologies.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 15/16] qapi: Share gen_err_check()
2015-09-29 3:27 [Qemu-devel] [PATCH v6 00/16] post-introspection cleanups, subset A Eric Blake
` (13 preceding siblings ...)
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 14/16] qapi: Consistent generated code: minimize push_indent() usage Eric Blake
@ 2015-09-29 3:27 ` Eric Blake
2015-09-29 14:31 ` Markus Armbruster
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 16/16] qapi: Share gen_visit_fields() Eric Blake
15 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2015-09-29 3:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
qapi-commands had a nice helper gen_err_check(), but did not
use it everywhere. In fact, using it in more places makes it
easier to reduce the lines of code used in appending an error
check in generated code (previously required a multi-line
mcgen(), which didn't add any use of parameterization), which
in turn makes it easier to write the next patch which will
consolidate another common pattern among the generators.
The diffstat of this patch doesn't quite show as big a
reduction in lines as I had hoped, but that is in part due to
the duplication of some FIXME comments.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: new, split off of 10/46, add label parameter, and catch more
instances that could use it
---
scripts/qapi-commands.py | 29 +++++++-------------
scripts/qapi-event.py | 16 +++++------
scripts/qapi-visit.py | 69 ++++++++++++++++++++++++------------------------
scripts/qapi.py | 12 +++++++++
4 files changed, 62 insertions(+), 64 deletions(-)
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 1d3c0d5..713aa0b 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -25,17 +25,6 @@ def gen_command_decl(name, arg_type, ret_type):
params=gen_params(arg_type, 'Error **errp'))
-def gen_err_check(err):
- if not err:
- return ''
- return mcgen('''
- if (%(err)s) {
- goto out;
- }
-''',
- err=err)
-
-
def gen_call(name, arg_type, ret_type):
ret = ''
@@ -56,7 +45,7 @@ def gen_call(name, arg_type, ret_type):
''',
c_name=c_name(name), args=argstr, lhs=lhs)
if ret_type:
- ret += gen_err_check('err')
+ ret += gen_err_check()
ret += mcgen('''
qmp_marshal_output_%(c_name)s(retval, ret, &err);
@@ -133,7 +122,7 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
''',
c_name=c_name(memb.name), name=memb.name,
errp=errparg)
- ret += gen_err_check(errarg)
+ ret += gen_err_check(err=errarg)
ret += mcgen('''
if (has_%(c_name)s) {
''',
@@ -144,7 +133,7 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
''',
c_name=c_name(memb.name), name=memb.name,
c_type=memb.type.c_name(), errp=errparg)
- ret += gen_err_check(errarg)
+ ret += gen_err_check(err=errarg)
if memb.optional:
pop_indent()
ret += mcgen('''
@@ -159,7 +148,7 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
def gen_marshal_output(ret_type):
- return mcgen('''
+ ret = mcgen('''
static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out, Error **errp)
{
@@ -170,9 +159,10 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out,
v = qmp_output_get_visitor(qov);
visit_type_%(c_name)s(v, &ret_in, "unused", &err);
- if (err) {
- goto out;
- }
+''',
+ c_type=ret_type.c_type(), c_name=ret_type.c_name())
+ ret += gen_err_check()
+ ret += mcgen('''
*ret_out = qmp_output_get_qobject(qov);
out:
@@ -184,7 +174,8 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out,
qapi_dealloc_visitor_cleanup(qdv);
}
''',
- c_type=ret_type.c_type(), c_name=ret_type.c_name())
+ c_name=ret_type.c_name())
+ return ret
def gen_marshal_proto(name):
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index b43bbc2..bbc6169 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -68,12 +68,10 @@ def gen_event_send(name, arg_type):
/* Fake visit, as if all members are under a structure */
visit_start_struct(v, NULL, "", "%(name)s", 0, &err);
- if (err) {
- goto out;
- }
-
''',
name=name)
+ ret += gen_err_check()
+ ret += '\n'
for memb in arg_type.members:
if memb.optional:
@@ -91,14 +89,12 @@ def gen_event_send(name, arg_type):
ret += mcgen('''
visit_type_%(c_type)s(v, %(cast)s&%(c_name)s, "%(name)s", &err);
- if (err) {
- goto out;
- }
''',
cast=cast,
c_name=c_name(memb.name),
c_type=memb.type.c_name(),
name=memb.name)
+ ret += gen_err_check()
if memb.optional:
pop_indent()
@@ -109,9 +105,9 @@ def gen_event_send(name, arg_type):
ret += mcgen('''
visit_end_struct(v, &err);
- if (err) {
- goto out;
- }
+''')
+ ret += gen_err_check()
+ ret += mcgen('''
obj = qmp_output_get_qobject(qov);
g_assert(obj != NULL);
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 7ce8616..fec07ad 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -81,11 +81,9 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
if base:
ret += mcgen('''
visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
- if (err) {
- goto out;
- }
''',
c_type=base.c_name(), c_name=c_name('base'))
+ ret += gen_err_check()
for memb in members:
if memb.optional:
@@ -107,11 +105,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
ret += mcgen('''
}
''')
- ret += mcgen('''
- if (err) {
- goto out;
- }
-''')
+ ret += gen_err_check()
if re.search('^ *goto out;', ret, re.MULTILINE):
ret += mcgen('''
@@ -154,7 +148,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
def gen_visit_list(name, element_type):
- return mcgen('''
+ ret = mcgen('''
void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
{
@@ -162,9 +156,10 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
GenericList *i, **prev;
visit_start_list(v, name, &err);
- if (err) {
- goto out;
- }
+''',
+ c_name=c_name(name), c_elt_type=element_type.c_name())
+ ret += gen_err_check()
+ ret += mcgen('''
for (prev = (GenericList **)obj;
!err && (i = visit_next_list(v, prev, &err)) != NULL;
@@ -181,6 +176,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
}
''',
c_name=c_name(name), c_elt_type=element_type.c_name())
+ return ret
def gen_visit_enum(name):
@@ -202,16 +198,17 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
Error *err = NULL;
visit_start_implicit_struct(v, (void**) obj, sizeof(%(c_name)s), &err);
- if (err) {
- goto out;
- }
+''',
+ c_name=c_name(name))
+ ret += gen_err_check()
+ ret += mcgen('''
visit_get_next_type(v, (int*) &(*obj)->kind, %(c_name)s_qtypes, name, &err);
- if (err) {
- goto out_obj;
- }
+''',
+ c_name=c_name(name))
+ ret += gen_err_check(label='out_obj')
+ ret += mcgen('''
switch ((*obj)->kind) {
-''',
- c_name=c_name(name))
+''')
for var in variants.variants:
ret += mcgen('''
@@ -259,23 +256,21 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
Error *err = NULL;
visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
- if (err) {
- goto out;
- }
+''',
+ c_name=c_name(name), name=name)
+ ret += gen_err_check()
+ ret += mcgen('''
if (!*obj) {
goto out_obj;
}
-''',
- c_name=c_name(name), name=name)
+''')
if base:
ret += mcgen('''
visit_type_%(c_name)s_fields(v, obj, &err);
- if (err) {
- goto out_obj;
- }
''',
c_name=c_name(name))
+ ret += gen_err_check(label='out_obj')
tag_key = variants.tag_member.name
if not variants.tag_name:
@@ -283,13 +278,6 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
tag_key = 'type'
ret += mcgen('''
visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
- if (err) {
- goto out_obj;
- }
- if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
- goto out_obj;
- }
- switch ((*obj)->%(c_name)s) {
''',
c_type=variants.tag_member.type.c_name(),
# TODO ugly special case for simple union
@@ -297,6 +285,17 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
# it, then: c_name=c_name(variants.tag_member.name)
c_name=c_name(variants.tag_name or 'kind'),
name=tag_key)
+ ret += gen_err_check(label='out_obj')
+ ret += mcgen('''
+ if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
+ goto out_obj;
+ }
+ switch ((*obj)->%(c_name)s) {
+''',
+ # TODO ugly special case for simple union
+ # Use same tag name in C as on the wire to get rid of
+ # it, then: c_name=c_name(variants.tag_member.name)
+ c_name=c_name(variants.tag_name or 'kind'))
for var in variants.variants:
# TODO ugly special case for simple union
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 4825e62..0593b71 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1536,6 +1536,18 @@ def gen_params(arg_type, extra):
ret += sep + extra
return ret
+
+def gen_err_check(err='err', label='out'):
+ if not err:
+ return ''
+ return mcgen('''
+ if (%(err)s) {
+ goto %(label)s;
+ }
+''',
+ err=err, label=label)
+
+
#
# Common command line parsing
#
--
2.4.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 15/16] qapi: Share gen_err_check()
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 15/16] qapi: Share gen_err_check() Eric Blake
@ 2015-09-29 14:31 ` Markus Armbruster
2015-09-29 15:15 ` Eric Blake
2015-09-29 20:33 ` Eric Blake
0 siblings, 2 replies; 39+ messages in thread
From: Markus Armbruster @ 2015-09-29 14:31 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> qapi-commands had a nice helper gen_err_check(), but did not
In fact, it still has :)
> use it everywhere. In fact, using it in more places makes it
> easier to reduce the lines of code used in appending an error
Suggest "used for generating error checks"
> check in generated code (previously required a multi-line
> mcgen(), which didn't add any use of parameterization), which
> in turn makes it easier to write the next patch which will
> consolidate another common pattern among the generators.
I think we should burn some of the whiches ;)
Drop the paranthesis?
> The diffstat of this patch doesn't quite show as big a
> reduction in lines as I had hoped, but that is in part due to
Almost none...
> the duplication of some FIXME comments.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
Have you diffed the generated code before and after the patch?
> ---
> v6: new, split off of 10/46, add label parameter, and catch more
> instances that could use it
> ---
> scripts/qapi-commands.py | 29 +++++++-------------
> scripts/qapi-event.py | 16 +++++------
> scripts/qapi-visit.py | 69 ++++++++++++++++++++++++------------------------
> scripts/qapi.py | 12 +++++++++
> 4 files changed, 62 insertions(+), 64 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 1d3c0d5..713aa0b 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -25,17 +25,6 @@ def gen_command_decl(name, arg_type, ret_type):
> params=gen_params(arg_type, 'Error **errp'))
>
>
> -def gen_err_check(err):
> - if not err:
> - return ''
> - return mcgen('''
> - if (%(err)s) {
> - goto out;
> - }
> -''',
> - err=err)
> -
> -
> def gen_call(name, arg_type, ret_type):
> ret = ''
>
> @@ -56,7 +45,7 @@ def gen_call(name, arg_type, ret_type):
> ''',
> c_name=c_name(name), args=argstr, lhs=lhs)
> if ret_type:
> - ret += gen_err_check('err')
> + ret += gen_err_check()
> ret += mcgen('''
>
> qmp_marshal_output_%(c_name)s(retval, ret, &err);
> @@ -133,7 +122,7 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
> ''',
> c_name=c_name(memb.name), name=memb.name,
> errp=errparg)
> - ret += gen_err_check(errarg)
> + ret += gen_err_check(err=errarg)
> ret += mcgen('''
> if (has_%(c_name)s) {
> ''',
> @@ -144,7 +133,7 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
> ''',
> c_name=c_name(memb.name), name=memb.name,
> c_type=memb.type.c_name(), errp=errparg)
> - ret += gen_err_check(errarg)
> + ret += gen_err_check(err=errarg)
> if memb.optional:
> pop_indent()
> ret += mcgen('''
> @@ -159,7 +148,7 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
>
>
> def gen_marshal_output(ret_type):
> - return mcgen('''
> + ret = mcgen('''
>
> static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out, Error **errp)
> {
> @@ -170,9 +159,10 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out,
>
> v = qmp_output_get_visitor(qov);
> visit_type_%(c_name)s(v, &ret_in, "unused", &err);
> - if (err) {
> - goto out;
> - }
> +''',
> + c_type=ret_type.c_type(), c_name=ret_type.c_name())
> + ret += gen_err_check()
> + ret += mcgen('''
> *ret_out = qmp_output_get_qobject(qov);
>
> out:
Here's a case that becomes more verbose, so it's not just comment
duplication. Also becomes a bit harder to read, I think.
> @@ -184,7 +174,8 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out,
> qapi_dealloc_visitor_cleanup(qdv);
> }
> ''',
> - c_type=ret_type.c_type(), c_name=ret_type.c_name())
> + c_name=ret_type.c_name())
> + return ret
>
>
> def gen_marshal_proto(name):
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index b43bbc2..bbc6169 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -68,12 +68,10 @@ def gen_event_send(name, arg_type):
>
> /* Fake visit, as if all members are under a structure */
> visit_start_struct(v, NULL, "", "%(name)s", 0, &err);
> - if (err) {
> - goto out;
> - }
> -
> ''',
> name=name)
> + ret += gen_err_check()
> + ret += '\n'
>
> for memb in arg_type.members:
> if memb.optional:
This one gets marginally shorter, but hardly simpler.
> @@ -91,14 +89,12 @@ def gen_event_send(name, arg_type):
>
> ret += mcgen('''
> visit_type_%(c_type)s(v, %(cast)s&%(c_name)s, "%(name)s", &err);
> - if (err) {
> - goto out;
> - }
> ''',
> cast=cast,
> c_name=c_name(memb.name),
> c_type=memb.type.c_name(),
> name=memb.name)
> + ret += gen_err_check()
>
> if memb.optional:
> pop_indent()
> @@ -109,9 +105,9 @@ def gen_event_send(name, arg_type):
> ret += mcgen('''
>
> visit_end_struct(v, &err);
> - if (err) {
> - goto out;
> - }
> +''')
> + ret += gen_err_check()
> + ret += mcgen('''
>
> obj = qmp_output_get_qobject(qov);
> g_assert(obj != NULL);
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 7ce8616..fec07ad 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -81,11 +81,9 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
> if base:
> ret += mcgen('''
> visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
> - if (err) {
> - goto out;
> - }
> ''',
> c_type=base.c_name(), c_name=c_name('base'))
> + ret += gen_err_check()
>
> for memb in members:
> if memb.optional:
> @@ -107,11 +105,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
> ret += mcgen('''
> }
> ''')
> - ret += mcgen('''
> - if (err) {
> - goto out;
> - }
> -''')
> + ret += gen_err_check()
>
> if re.search('^ *goto out;', ret, re.MULTILINE):
> ret += mcgen('''
> @@ -154,7 +148,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
>
>
> def gen_visit_list(name, element_type):
> - return mcgen('''
> + ret = mcgen('''
>
> void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
> {
> @@ -162,9 +156,10 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
> GenericList *i, **prev;
>
> visit_start_list(v, name, &err);
> - if (err) {
> - goto out;
> - }
> +''',
> + c_name=c_name(name), c_elt_type=element_type.c_name())
> + ret += gen_err_check()
> + ret += mcgen('''
>
> for (prev = (GenericList **)obj;
> !err && (i = visit_next_list(v, prev, &err)) != NULL;
> @@ -181,6 +176,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
> }
> ''',
> c_name=c_name(name), c_elt_type=element_type.c_name())
> + return ret
>
>
> def gen_visit_enum(name):
> @@ -202,16 +198,17 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
> Error *err = NULL;
>
> visit_start_implicit_struct(v, (void**) obj, sizeof(%(c_name)s), &err);
> - if (err) {
> - goto out;
> - }
> +''',
> + c_name=c_name(name))
> + ret += gen_err_check()
> + ret += mcgen('''
> visit_get_next_type(v, (int*) &(*obj)->kind, %(c_name)s_qtypes, name, &err);
> - if (err) {
> - goto out_obj;
> - }
> +''',
> + c_name=c_name(name))
> + ret += gen_err_check(label='out_obj')
> + ret += mcgen('''
> switch ((*obj)->kind) {
> -''',
> - c_name=c_name(name))
> +''')
>
> for var in variants.variants:
> ret += mcgen('''
> @@ -259,23 +256,21 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
> Error *err = NULL;
>
> visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
> - if (err) {
> - goto out;
> - }
> +''',
> + c_name=c_name(name), name=name)
> + ret += gen_err_check()
> + ret += mcgen('''
> if (!*obj) {
> goto out_obj;
> }
> -''',
> - c_name=c_name(name), name=name)
> +''')
>
> if base:
> ret += mcgen('''
> visit_type_%(c_name)s_fields(v, obj, &err);
> - if (err) {
> - goto out_obj;
> - }
> ''',
> c_name=c_name(name))
> + ret += gen_err_check(label='out_obj')
>
> tag_key = variants.tag_member.name
> if not variants.tag_name:
> @@ -283,13 +278,6 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
> tag_key = 'type'
> ret += mcgen('''
> visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
> - if (err) {
> - goto out_obj;
> - }
> - if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
> - goto out_obj;
> - }
> - switch ((*obj)->%(c_name)s) {
> ''',
> c_type=variants.tag_member.type.c_name(),
> # TODO ugly special case for simple union
> @@ -297,6 +285,17 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
> # it, then: c_name=c_name(variants.tag_member.name)
> c_name=c_name(variants.tag_name or 'kind'),
> name=tag_key)
> + ret += gen_err_check(label='out_obj')
> + ret += mcgen('''
> + if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
> + goto out_obj;
> + }
> + switch ((*obj)->%(c_name)s) {
> +''',
> + # TODO ugly special case for simple union
> + # Use same tag name in C as on the wire to get rid of
> + # it, then: c_name=c_name(variants.tag_member.name)
> + c_name=c_name(variants.tag_name or 'kind'))
>
> for var in variants.variants:
> # TODO ugly special case for simple union
Here's the duplicated comment. It's a TODO, not a FIXME.
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4825e62..0593b71 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1536,6 +1536,18 @@ def gen_params(arg_type, extra):
> ret += sep + extra
> return ret
>
> +
> +def gen_err_check(err='err', label='out'):
> + if not err:
> + return ''
> + return mcgen('''
> + if (%(err)s) {
> + goto %(label)s;
> + }
> +''',
> + err=err, label=label)
> +
> +
> #
> # Common command line parsing
> #
I don't know. Could be worthwhile if it really makes further work
easier.
To really cut the verbosity, I figure we'd have to do something more
radical, like having cgen() recognize a (short!) pattern and replace it
with a full-blown error check. Not sure that's actually a good idea,
though :)
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 15/16] qapi: Share gen_err_check()
2015-09-29 14:31 ` Markus Armbruster
@ 2015-09-29 15:15 ` Eric Blake
2015-10-01 6:33 ` Markus Armbruster
2015-09-29 20:33 ` Eric Blake
1 sibling, 1 reply; 39+ messages in thread
From: Eric Blake @ 2015-09-29 15:15 UTC (permalink / raw)
To: Markus Armbruster; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2936 bytes --]
On 09/29/2015 08:31 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> qapi-commands had a nice helper gen_err_check(), but did not
>
> In fact, it still has :)
>
>> use it everywhere. In fact, using it in more places makes it
>> easier to reduce the lines of code used in appending an error
>
> Suggest "used for generating error checks"
>
>> check in generated code (previously required a multi-line
>> mcgen(), which didn't add any use of parameterization), which
>> in turn makes it easier to write the next patch which will
>> consolidate another common pattern among the generators.
>
> I think we should burn some of the whiches ;)
Yay - a which-hunt. I'll get the pitchfork.
>
> Drop the paranthesis?
>
>> The diffstat of this patch doesn't quite show as big a
>> reduction in lines as I had hoped, but that is in part due to
>
> Almost none...
>
>> the duplication of some FIXME comments.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> Have you diffed the generated code before and after the patch?
Oops, forgot to mention. No change to generated code.
>> @@ -170,9 +159,10 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out,
>>
>> v = qmp_output_get_visitor(qov);
>> visit_type_%(c_name)s(v, &ret_in, "unused", &err);
>> - if (err) {
>> - goto out;
>> - }
>> +''',
>> + c_type=ret_type.c_type(), c_name=ret_type.c_name())
>> + ret += gen_err_check()
>> + ret += mcgen('''
>> *ret_out = qmp_output_get_qobject(qov);
>>
>> out:
>
> Here's a case that becomes more verbose, so it's not just comment
> duplication. Also becomes a bit harder to read, I think.
Due to splitting a larger chunk into smaller pieces to inject the error
in the middle.
I guess we could pick and choose to use the function only where it
doesn't split an already-existing large block, in order to maximize the
diffstat ratio rather than in favor of eliminating the common code
pattern duplication?
>
> I don't know. Could be worthwhile if it really makes further work
> easier.
>
> To really cut the verbosity, I figure we'd have to do something more
> radical, like having cgen() recognize a (short!) pattern and replace it
> with a full-blown error check. Not sure that's actually a good idea,
> though :)
In v5, it was only used by the shared gen_visit_fields(), until I added
uses of it later in the series to make anonymous base classes of a flat
union easier to implement.
I guess the conservative thing is to scale this patch back a bit (move
the function to the common location, and use it where it makes an
obvious difference to the diffstat, but leaving other places alone for
now). Should I try that for v7?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 15/16] qapi: Share gen_err_check()
2015-09-29 15:15 ` Eric Blake
@ 2015-10-01 6:33 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2015-10-01 6:33 UTC (permalink / raw)
To: Eric Blake; +Cc: Michael Roth, marcandre.lureau, qemu-devel, ehabkost
Eric Blake <eblake@redhat.com> writes:
> On 09/29/2015 08:31 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> qapi-commands had a nice helper gen_err_check(), but did not
>>
>> In fact, it still has :)
>>
>>> use it everywhere. In fact, using it in more places makes it
>>> easier to reduce the lines of code used in appending an error
>>
>> Suggest "used for generating error checks"
>>
>>> check in generated code (previously required a multi-line
>>> mcgen(), which didn't add any use of parameterization), which
>>> in turn makes it easier to write the next patch which will
>>> consolidate another common pattern among the generators.
>>
>> I think we should burn some of the whiches ;)
>
> Yay - a which-hunt. I'll get the pitchfork.
I think you'll need a pichfork for this ;-)
>>
>> Drop the paranthesis?
>>
>>> The diffstat of this patch doesn't quite show as big a
>>> reduction in lines as I had hoped, but that is in part due to
>>
>> Almost none...
>>
>>> the duplication of some FIXME comments.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> Have you diffed the generated code before and after the patch?
>
> Oops, forgot to mention. No change to generated code.
Let's state it in the commit message.
>>> @@ -170,9 +159,10 @@ static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out,
>>>
>>> v = qmp_output_get_visitor(qov);
>>> visit_type_%(c_name)s(v, &ret_in, "unused", &err);
>>> - if (err) {
>>> - goto out;
>>> - }
>>> +''',
>>> + c_type=ret_type.c_type(), c_name=ret_type.c_name())
>>> + ret += gen_err_check()
>>> + ret += mcgen('''
>>> *ret_out = qmp_output_get_qobject(qov);
>>>
>>> out:
>>
>> Here's a case that becomes more verbose, so it's not just comment
>> duplication. Also becomes a bit harder to read, I think.
>
> Due to splitting a larger chunk into smaller pieces to inject the error
> in the middle.
>
> I guess we could pick and choose to use the function only where it
> doesn't split an already-existing large block, in order to maximize the
> diffstat ratio rather than in favor of eliminating the common code
> pattern duplication?
Two good reasons for capturing common code in a helper: one, ensuring
consistency and making it easier to change, and two, readability.
I don't think our "if (err) goto out" idiom is likely to change. That
leaves just readability, I think. A case-by-case approach makes sense
then.
>> I don't know. Could be worthwhile if it really makes further work
>> easier.
>>
>> To really cut the verbosity, I figure we'd have to do something more
>> radical, like having cgen() recognize a (short!) pattern and replace it
>> with a full-blown error check. Not sure that's actually a good idea,
>> though :)
>
> In v5, it was only used by the shared gen_visit_fields(), until I added
> uses of it later in the series to make anonymous base classes of a flat
> union easier to implement.
>
> I guess the conservative thing is to scale this patch back a bit (move
> the function to the common location, and use it where it makes an
> obvious difference to the diffstat, but leaving other places alone for
> now). Should I try that for v7?
Yes, please. Except let yourself be guided by your eye, not by
diffstat.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 15/16] qapi: Share gen_err_check()
2015-09-29 14:31 ` Markus Armbruster
2015-09-29 15:15 ` Eric Blake
@ 2015-09-29 20:33 ` Eric Blake
2015-10-01 6:40 ` Markus Armbruster
1 sibling, 1 reply; 39+ messages in thread
From: Eric Blake @ 2015-09-29 20:33 UTC (permalink / raw)
To: Markus Armbruster; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 800 bytes --]
On 09/29/2015 08:31 AM, Markus Armbruster wrote:
>
> To really cut the verbosity, I figure we'd have to do something more
> radical, like having cgen() recognize a (short!) pattern and replace it
> with a full-blown error check. Not sure that's actually a good idea,
> though :)
Another radical idea would be write the python code to not care about
whitespace or indentation discrepancies, because we instead run things
through 'indent' as part of the code generation. Then we can emit ugly
code without caring about indentation or multi-line, if it makes the
python easier to write, and the output would still be nicely formatted.
I'm not sure I want to do that, though.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v6 15/16] qapi: Share gen_err_check()
2015-09-29 20:33 ` Eric Blake
@ 2015-10-01 6:40 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2015-10-01 6:40 UTC (permalink / raw)
To: Eric Blake; +Cc: Michael Roth, marcandre.lureau, qemu-devel, ehabkost
Eric Blake <eblake@redhat.com> writes:
> On 09/29/2015 08:31 AM, Markus Armbruster wrote:
>>
>> To really cut the verbosity, I figure we'd have to do something more
>> radical, like having cgen() recognize a (short!) pattern and replace it
>> with a full-blown error check. Not sure that's actually a good idea,
>> though :)
>
> Another radical idea would be write the python code to not care about
> whitespace or indentation discrepancies, because we instead run things
> through 'indent' as part of the code generation. Then we can emit ugly
> code without caring about indentation or multi-line, if it makes the
> python easier to write, and the output would still be nicely formatted.
I believe generating perfectly neat code the way we generate code
(printing text) isn't practical. Avoiding long lines in particular
would take far too much effort, and make the generators much more
complex and less readable.
Of course, even if we piped through indent, we'd still want the
generated code look neat in the generator source. But that's a
different kind of neat.
> I'm not sure I want to do that, though.
I'm open to the idea. Right now it doesn't seem worthwhile, because we
already did the work for reasonably legible generated code.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v6 16/16] qapi: Share gen_visit_fields()
2015-09-29 3:27 [Qemu-devel] [PATCH v6 00/16] post-introspection cleanups, subset A Eric Blake
` (14 preceding siblings ...)
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 15/16] qapi: Share gen_err_check() Eric Blake
@ 2015-09-29 3:27 ` Eric Blake
2015-09-29 14:38 ` Markus Armbruster
15 siblings, 1 reply; 39+ messages in thread
From: Eric Blake @ 2015-09-29 3:27 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, marcandre.lureau, armbru, ehabkost
Consolidate the code between visit, command marshalling, and
event generation that iterates over the members of a struct.
It reduces code duplication in the generator, so that a future
patch can reduce the size of generated code while touching only
one instead of three locations.
There are no changes to generated marshal code.
The visitor code is slightly more verbose, but semantically
equivalent, and actually easier to read as it follows a more
common idiom:
| visit_optional(v, &(*obj)->has_device, "device", &err);
|- if (!err && (*obj)->has_device) {
|- visit_type_str(v, &(*obj)->device, "device", &err);
|- }
| if (err) {
| goto out;
| }
|+ if ((*obj)->has_device) {
|+ visit_type_str(v, &(*obj)->device, "device", &err);
|+ if (err) {
|+ goto out;
|+ }
|+ }
The event code is slightly more verbose, but arguably a bug
fix (although the visitors are not well documented, use of an
optional member should not be attempted unless guarded by a
prior call to visit_optional()) - we were lucky that the output
qmp visitor has a no-op visit_optional():
|+ visit_optional(v, &has_offset, "offset", &err);
|+ if (err) {
|+ goto out;
|+ }
| if (has_offset) {
| visit_type_int(v, &offset, "offset", &err);
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v6: retitle 10/46, split gen_err_check() to its own patch,
improve commit message, use named parameters in gen_visit_fields()
to minimize effort of callers
---
scripts/qapi-commands.py | 27 +--------------------------
scripts/qapi-event.py | 31 +------------------------------
scripts/qapi-visit.py | 22 +---------------------
scripts/qapi.py | 43 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 46 insertions(+), 77 deletions(-)
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 713aa0b..b44ab1e 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -101,7 +101,6 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
return ret
if dealloc:
- errparg = 'NULL'
errarg = None
ret += mcgen('''
qmp_input_visitor_cleanup(qiv);
@@ -109,36 +108,12 @@ def gen_marshal_input_visit(arg_type, dealloc=False):
v = qapi_dealloc_get_visitor(qdv);
''')
else:
- errparg = '&err'
errarg = 'err'
ret += mcgen('''
v = qmp_input_get_visitor(qiv);
''')
- for memb in arg_type.members:
- if memb.optional:
- ret += mcgen('''
- visit_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s);
-''',
- c_name=c_name(memb.name), name=memb.name,
- errp=errparg)
- ret += gen_err_check(err=errarg)
- ret += mcgen('''
- if (has_%(c_name)s) {
-''',
- c_name=c_name(memb.name))
- push_indent()
- ret += mcgen('''
- visit_type_%(c_type)s(v, &%(c_name)s, "%(name)s", %(errp)s);
-''',
- c_name=c_name(memb.name), name=memb.name,
- c_type=memb.type.c_name(), errp=errparg)
- ret += gen_err_check(err=errarg)
- if memb.optional:
- pop_indent()
- ret += mcgen('''
- }
-''')
+ ret += gen_visit_fields(arg_type.members, errarg=errarg)
if dealloc:
ret += mcgen('''
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index bbc6169..7eecaeb 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -72,36 +72,7 @@ def gen_event_send(name, arg_type):
name=name)
ret += gen_err_check()
ret += '\n'
-
- for memb in arg_type.members:
- if memb.optional:
- ret += mcgen('''
- if (has_%(c_name)s) {
-''',
- c_name=c_name(memb.name))
- push_indent()
-
- # Ugly: need to cast away the const
- if memb.type.name == "str":
- cast = '(char **)'
- else:
- cast = ''
-
- ret += mcgen('''
- visit_type_%(c_type)s(v, %(cast)s&%(c_name)s, "%(name)s", &err);
-''',
- cast=cast,
- c_name=c_name(memb.name),
- c_type=memb.type.c_name(),
- name=memb.name)
- ret += gen_err_check()
-
- if memb.optional:
- pop_indent()
- ret += mcgen('''
- }
-''')
-
+ ret += gen_visit_fields(arg_type.members, need_cast=True)
ret += mcgen('''
visit_end_struct(v, &err);
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index fec07ad..65b5e8d 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -85,27 +85,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
c_type=base.c_name(), c_name=c_name('base'))
ret += gen_err_check()
- for memb in members:
- if memb.optional:
- ret += mcgen('''
- visit_optional(v, &(*obj)->has_%(c_name)s, "%(name)s", &err);
- if (!err && (*obj)->has_%(c_name)s) {
-''',
- c_name=c_name(memb.name), name=memb.name)
- push_indent()
-
- ret += mcgen('''
- visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
-''',
- c_type=memb.type.c_name(), c_name=c_name(memb.name),
- name=memb.name)
-
- if memb.optional:
- pop_indent()
- ret += mcgen('''
- }
-''')
- ret += gen_err_check()
+ ret += gen_visit_fields(members, prefix='(*obj)->')
if re.search('^ *goto out;', ret, re.MULTILINE):
ret += mcgen('''
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 0593b71..d3fdf46 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1548,6 +1548,49 @@ def gen_err_check(err='err', label='out'):
err=err, label=label)
+def gen_visit_fields(members, prefix='', need_cast=False, errarg='err'):
+ ret = ''
+ if errarg:
+ errparg = '&' + errarg
+ else:
+ errparg = 'NULL'
+
+ for memb in members:
+ if memb.optional:
+ ret += mcgen('''
+ visit_optional(v, &%(prefix)shas_%(c_name)s, "%(name)s", %(errp)s);
+''',
+ prefix=prefix, c_name=c_name(memb.name),
+ name=memb.name, errp=errparg)
+ ret += gen_err_check(err=errarg)
+ ret += mcgen('''
+ if (%(prefix)shas_%(c_name)s) {
+''',
+ prefix=prefix, c_name=c_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, %(cast)s&%(prefix)s%(c_name)s, "%(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(err=errarg)
+
+ if memb.optional:
+ pop_indent()
+ ret += mcgen('''
+ }
+''')
+ return ret
+
+
#
# Common command line parsing
#
--
2.4.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v6 16/16] qapi: Share gen_visit_fields()
2015-09-29 3:27 ` [Qemu-devel] [PATCH v6 16/16] qapi: Share gen_visit_fields() Eric Blake
@ 2015-09-29 14:38 ` Markus Armbruster
0 siblings, 0 replies; 39+ messages in thread
From: Markus Armbruster @ 2015-09-29 14:38 UTC (permalink / raw)
To: Eric Blake; +Cc: marcandre.lureau, qemu-devel, ehabkost, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Consolidate the code between visit, command marshalling, and
> event generation that iterates over the members of a struct.
> It reduces code duplication in the generator, so that a future
> patch can reduce the size of generated code while touching only
> one instead of three locations.
>
> There are no changes to generated marshal code.
>
> The visitor code is slightly more verbose, but semantically
Suggest "becomes slightly more verbose".
> equivalent, and actually easier to read as it follows a more
> common idiom:
>
> | visit_optional(v, &(*obj)->has_device, "device", &err);
> |- if (!err && (*obj)->has_device) {
> |- visit_type_str(v, &(*obj)->device, "device", &err);
> |- }
> | if (err) {
> | goto out;
> | }
> |+ if ((*obj)->has_device) {
> |+ visit_type_str(v, &(*obj)->device, "device", &err);
> |+ if (err) {
> |+ goto out;
> |+ }
> |+ }
>
> The event code is slightly more verbose, but arguably a bug
> fix (although the visitors are not well documented, use of an
> optional member should not be attempted unless guarded by a
> prior call to visit_optional()) - we were lucky that the output
> qmp visitor has a no-op visit_optional():
Suggest "becomes slightly more verbose, but this is arguably a bug fix:
although [...] prior call to visit_optional(). Works only because the
output qmp visitor [...]".
>
> |+ visit_optional(v, &has_offset, "offset", &err);
> |+ if (err) {
> |+ goto out;
> |+ }
> | if (has_offset) {
> | visit_type_int(v, &offset, "offset", &err);
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v6: retitle 10/46, split gen_err_check() to its own patch,
> improve commit message, use named parameters in gen_visit_fields()
> to minimize effort of callers
Interdiff to v5 looks sane.
^ permalink raw reply [flat|nested] 39+ messages in thread