* [Qemu-devel] [PATCH v8 01/18] qapi: Use predicate callback to determine visit filtering
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 02/18] qapi: Prepare for errors during check() Eric Blake
` (18 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Previously, qapi-types and qapi-visit filtered out implicit
objects during visit_object_type() by using 'info' (works since
implicit objects do not [yet] have associated info); meanwhile
qapi-introspect filtered out all schema types on the first pass
by returning a python type from visit_begin(), which was then
used at a distance in QAPISchema.visit() to do the filtering.
Rather than keeping these ad hoc approaches, add a new visitor
callback visit_needed() which returns False to skip a given
entity, and which defaults to True unless overridden. Use the
new mechanism to simplify all three filtering visitors.
No change to the generated code.
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: no change
v7: rename to visit_needed(), add comment
v6: new patch
---
scripts/qapi-introspect.py | 5 ++++-
scripts/qapi-types.py | 19 +++++++++++--------
scripts/qapi-visit.py | 17 ++++++++++-------
scripts/qapi.py | 12 ++++++++----
4 files changed, 33 insertions(+), 20 deletions(-)
diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
index 7d39320..c0dad66 100644
--- a/scripts/qapi-introspect.py
+++ b/scripts/qapi-introspect.py
@@ -54,7 +54,6 @@ class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor):
self._jsons = []
self._used_types = []
self._name_map = {}
- return QAPISchemaType # don't visit types for now
def visit_end(self):
# visit the types that are actually used
@@ -82,6 +81,10 @@ const char %(c_name)s[] = %(c_string)s;
self._used_types = None
self._name_map = None
+ def visit_needed(self, entity):
+ # Ignore types on first pass; visit_end() will pick up used types
+ return not isinstance(entity, QAPISchemaType)
+
def _name(self, name):
if self._unmask:
return name
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index d405f8d..2a29c6e 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -233,6 +233,10 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
self.decl = self._btin + self.decl
self._btin = None
+ def visit_needed(self, entity):
+ # Visit everything except implicit objects
+ return not isinstance(entity, QAPISchemaObjectType) or entity.info
+
def _gen_type_cleanup(self, name):
self.decl += gen_type_cleanup_decl(name)
self.defn += gen_type_cleanup(name)
@@ -254,14 +258,13 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
self._gen_type_cleanup(name)
def visit_object_type(self, name, info, base, members, variants):
- if info:
- self._fwdecl += gen_fwd_object_or_array(name)
- if variants:
- assert not members # not implemented
- self.decl += gen_union(name, base, variants)
- else:
- self.decl += gen_struct(name, base, members)
- self._gen_type_cleanup(name)
+ self._fwdecl += gen_fwd_object_or_array(name)
+ if variants:
+ assert not members # not implemented
+ self.decl += gen_union(name, base, variants)
+ else:
+ self.decl += gen_struct(name, base, members)
+ self._gen_type_cleanup(name)
def visit_alternate_type(self, name, info, variants):
self._fwdecl += gen_fwd_object_or_array(name)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 4f97781..9fc79a7 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -333,6 +333,10 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
self.decl = self._btin + self.decl
self._btin = None
+ def visit_needed(self, entity):
+ # Visit everything except implicit objects
+ return not isinstance(entity, QAPISchemaObjectType) or entity.info
+
def visit_enum_type(self, name, info, values, prefix):
self.decl += gen_visit_decl(name, scalar=True)
self.defn += gen_visit_enum(name)
@@ -349,13 +353,12 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
self.defn += defn
def visit_object_type(self, name, info, base, members, variants):
- if info:
- self.decl += gen_visit_decl(name)
- if variants:
- assert not members # not implemented
- self.defn += gen_visit_union(name, base, variants)
- else:
- self.defn += gen_visit_struct(name, base, members)
+ self.decl += gen_visit_decl(name)
+ if variants:
+ assert not members # not implemented
+ self.defn += gen_visit_union(name, base, variants)
+ else:
+ self.defn += gen_visit_struct(name, base, members)
def visit_alternate_type(self, name, info, variants):
self.decl += gen_visit_decl(name)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 26cff3f..543b378 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -811,6 +811,10 @@ class QAPISchemaVisitor(object):
def visit_end(self):
pass
+ def visit_needed(self, entity):
+ # Default to visiting everything
+ return True
+
def visit_builtin_type(self, name, info, json_type):
pass
@@ -1304,10 +1308,10 @@ class QAPISchema(object):
ent.check(self)
def visit(self, visitor):
- ignore = visitor.visit_begin(self)
- for name in sorted(self._entity_dict.keys()):
- if not ignore or not isinstance(self._entity_dict[name], ignore):
- self._entity_dict[name].visit(visitor)
+ visitor.visit_begin(self)
+ for (name, entity) in sorted(self._entity_dict.items()):
+ if visitor.visit_needed(entity):
+ entity.visit(visitor)
visitor.visit_end()
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PATCH v8 02/18] qapi: Prepare for errors during check()
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 01/18] qapi: Use predicate callback to determine visit filtering Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 03/18] qapi: Drop redundant alternate-good test Eric Blake
` (17 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
The next few patches will start migrating error checking from
ad hoc parse methods into the QAPISchema*.check() methods. But
for an error message to display, we first have to fix the
overall 'try' to catch those errors. We also want to enable a
few more assertions, such as making sure every attempt to
raise a semantic error is passed a valid location info, or that
various preconditions hold.
The general approach for moving error checking will then be to
relax an assertion into an if that raises an exception if the
condition does not hold, and removing the counterpart ad hoc
check done during the parse phase.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: no change
v7: Split off of v6 7/12, plus other obvious asserts floated up
---
scripts/qapi.py | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 543b378..4573599 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -103,6 +103,7 @@ class QAPISchemaError(Exception):
class QAPIExprError(Exception):
def __init__(self, expr_info, msg):
Exception.__init__(self)
+ assert expr_info
self.info = expr_info
self.msg = msg
@@ -964,6 +965,7 @@ class QAPISchemaObjectType(QAPISchemaType):
members = []
seen = {}
for m in members:
+ assert c_name(m.name) not in seen
seen[m.name] = m
for m in self.local_members:
m.check(schema, members, seen)
@@ -1116,13 +1118,13 @@ class QAPISchema(object):
def __init__(self, fname):
try:
self.exprs = check_exprs(QAPISchemaParser(open(fname, "r")).exprs)
+ self._entity_dict = {}
+ self._def_predefineds()
+ self._def_exprs()
+ self.check()
except (QAPISchemaError, QAPIExprError), err:
print >>sys.stderr, err
exit(1)
- self._entity_dict = {}
- self._def_predefineds()
- self._def_exprs()
- self.check()
def _def_entity(self, ent):
assert ent.name not in self._entity_dict
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PATCH v8 03/18] qapi: Drop redundant alternate-good test
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 01/18] qapi: Use predicate callback to determine visit filtering Eric Blake
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 02/18] qapi: Prepare for errors during check() Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 04/18] qapi: Move empty-enum to compile-time test Eric Blake
` (16 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru
The alternate-good.json test was already covered by
qapi-schema-test.json. As future commits will be tweaking
how alternates are laid out, removing the duplicate test now
reduces churn.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: no change
v7: new patch
---
tests/Makefile | 1 -
tests/qapi-schema/alternate-good.err | 0
tests/qapi-schema/alternate-good.exit | 1 -
tests/qapi-schema/alternate-good.json | 9 ---------
tests/qapi-schema/alternate-good.out | 10 ----------
5 files changed, 21 deletions(-)
delete mode 100644 tests/qapi-schema/alternate-good.err
delete mode 100644 tests/qapi-schema/alternate-good.exit
delete mode 100644 tests/qapi-schema/alternate-good.json
delete mode 100644 tests/qapi-schema/alternate-good.out
diff --git a/tests/Makefile b/tests/Makefile
index 209eca9..9a6601d 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -230,7 +230,6 @@ 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
qapi-schema += args-alternate.json
diff --git a/tests/qapi-schema/alternate-good.err b/tests/qapi-schema/alternate-good.err
deleted file mode 100644
index e69de29..0000000
diff --git a/tests/qapi-schema/alternate-good.exit b/tests/qapi-schema/alternate-good.exit
deleted file mode 100644
index 573541a..0000000
--- a/tests/qapi-schema/alternate-good.exit
+++ /dev/null
@@ -1 +0,0 @@
-0
diff --git a/tests/qapi-schema/alternate-good.json b/tests/qapi-schema/alternate-good.json
deleted file mode 100644
index 3371770..0000000
--- a/tests/qapi-schema/alternate-good.json
+++ /dev/null
@@ -1,9 +0,0 @@
-# Working example of alternate
-{ 'struct': 'Data',
- 'data': { '*number': 'int', '*name': 'str' } }
-{ 'enum': 'Enum',
- 'data': [ 'hello', 'world' ] }
-{ 'alternate': 'Alt',
- 'data': { 'value': 'int',
- 'string': 'Enum',
- 'struct': 'Data' } }
diff --git a/tests/qapi-schema/alternate-good.out b/tests/qapi-schema/alternate-good.out
deleted file mode 100644
index 65af727..0000000
--- a/tests/qapi-schema/alternate-good.out
+++ /dev/null
@@ -1,10 +0,0 @@
-object :empty
-alternate Alt
- case value: int
- case string: Enum
- case struct: Data
-enum AltKind ['value', 'string', 'struct']
-object Data
- member number: int optional=True
- member name: str optional=True
-enum Enum ['hello', 'world']
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PATCH v8 04/18] qapi: Move empty-enum to compile-time test
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
` (2 preceding siblings ...)
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 03/18] qapi: Drop redundant alternate-good test Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 05/18] qapi: Drop redundant returns-int test Eric Blake
` (15 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Rather than just asserting that we can parse an empty enum,
let's also make sure we can compile it, by including it in
qapi-schema-test.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: (no v7) hoist from subset C into B
v6: new patch; could be hoisted earlier alongside subset B v7 3/14
---
tests/Makefile | 1 -
tests/qapi-schema/enum-empty.err | 0
tests/qapi-schema/enum-empty.exit | 1 -
tests/qapi-schema/enum-empty.json | 2 --
tests/qapi-schema/enum-empty.out | 2 --
tests/qapi-schema/qapi-schema-test.json | 6 ++++++
tests/qapi-schema/qapi-schema-test.out | 1 +
7 files changed, 7 insertions(+), 6 deletions(-)
delete mode 100644 tests/qapi-schema/enum-empty.err
delete mode 100644 tests/qapi-schema/enum-empty.exit
delete mode 100644 tests/qapi-schema/enum-empty.json
delete mode 100644 tests/qapi-schema/enum-empty.out
diff --git a/tests/Makefile b/tests/Makefile
index 9a6601d..e058312 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -260,7 +260,6 @@ 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
diff --git a/tests/qapi-schema/enum-empty.err b/tests/qapi-schema/enum-empty.err
deleted file mode 100644
index e69de29..0000000
diff --git a/tests/qapi-schema/enum-empty.exit b/tests/qapi-schema/enum-empty.exit
deleted file mode 100644
index 573541a..0000000
--- a/tests/qapi-schema/enum-empty.exit
+++ /dev/null
@@ -1 +0,0 @@
-0
diff --git a/tests/qapi-schema/enum-empty.json b/tests/qapi-schema/enum-empty.json
deleted file mode 100644
index 40d4e85..0000000
--- a/tests/qapi-schema/enum-empty.json
+++ /dev/null
@@ -1,2 +0,0 @@
-# An empty enum, although unusual, is currently acceptable
-{ 'enum': 'MyEnum', 'data': [ ] }
diff --git a/tests/qapi-schema/enum-empty.out b/tests/qapi-schema/enum-empty.out
deleted file mode 100644
index a449d45..0000000
--- a/tests/qapi-schema/enum-empty.out
+++ /dev/null
@@ -1,2 +0,0 @@
-object :empty
-enum MyEnum []
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index abe59fd..c8cd2dd 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -1,11 +1,17 @@
# *-*- Mode: Python -*-*
+# This file is a stress test of supported qapi constructs that must
+# parse and compile correctly.
+
# for testing enums
{ 'enum': 'EnumOne',
'data': [ 'value1', 'value2', 'value3' ] }
{ 'struct': 'NestedEnumsOne',
'data': { 'enum1': 'EnumOne', '*enum2': 'EnumOne', 'enum3': 'EnumOne', '*enum4': 'EnumOne' } }
+# An empty enum, although unusual, is currently acceptable
+{ 'enum': 'MyEnum', 'data': [ ] }
+
# for testing override of default naming heuristic
{ 'enum': 'QEnumTwo',
'prefix': 'QENUM_TWO',
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 8f81784..e3504ab 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -86,6 +86,7 @@ object EventStructOne
member struct1: UserDefOne optional=False
member string: str optional=False
member enum2: EnumOne optional=True
+enum MyEnum []
object NestedEnumsOne
member enum1: EnumOne optional=False
member enum2: EnumOne optional=True
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PATCH v8 05/18] qapi: Drop redundant returns-int test
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
` (3 preceding siblings ...)
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 04/18] qapi: Move empty-enum to compile-time test Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 06/18] qapi: Drop redundant flat-union-reverse-define test Eric Blake
` (14 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
qapi-schema-test was already testing that we could have a
command returning int, but burned a command name in the whitelist.
Merge the redundant positive test returns-int, and pick a name
that reduces the whitelist size.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: (no v7) hoist from subset C into B
v6: new patch; could be hoisted earlier alongside subset B v7 3/17
---
scripts/qapi.py | 3 ---
tests/Makefile | 1 -
tests/qapi-schema/qapi-schema-test.json | 5 +++--
tests/qapi-schema/qapi-schema-test.out | 10 +++++-----
tests/qapi-schema/returns-int.err | 0
tests/qapi-schema/returns-int.exit | 1 -
tests/qapi-schema/returns-int.json | 3 ---
tests/qapi-schema/returns-int.out | 3 ---
tests/test-qmp-commands.c | 4 ++--
9 files changed, 10 insertions(+), 20 deletions(-)
delete mode 100644 tests/qapi-schema/returns-int.err
delete mode 100644 tests/qapi-schema/returns-int.exit
delete mode 100644 tests/qapi-schema/returns-int.json
delete mode 100644 tests/qapi-schema/returns-int.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 4573599..68f97a1 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -56,9 +56,6 @@ returns_whitelist = [
'guest-set-vcpus',
'guest-sync',
'guest-sync-delimited',
-
- # From qapi-schema-test:
- 'user_def_cmd3',
]
enum_types = []
diff --git a/tests/Makefile b/tests/Makefile
index e058312..3512f8c 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -318,7 +318,6 @@ 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-base.json
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index c8cd2dd..67c6bca 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -104,9 +104,10 @@
{ 'command': 'user_def_cmd2',
'data': {'ud1a': 'UserDefOne', '*ud1b': 'UserDefOne'},
'returns': 'UserDefTwo' }
-{ 'command': 'user_def_cmd3', 'data': {'a': 'int', '*b': 'int' },
+
+# Returning a non-dictionary requires a name from the whitelist
+{ 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' },
'returns': 'int' }
-# note: command name 'guest-sync' chosen to avoid "cannot use built-in" error
{ 'command': 'guest-sync', 'data': { 'arg': 'any' }, 'returns': 'any' }
# For testing integer range flattening in opts-visitor. The following schema
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index e3504ab..93f6250 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -17,6 +17,9 @@ object :obj-anyList-wrapper
member data: anyList optional=False
object :obj-boolList-wrapper
member data: boolList optional=False
+object :obj-guest-get-time-arg
+ member a: int optional=False
+ member b: int optional=True
object :obj-guest-sync-arg
member arg: any optional=False
object :obj-int16List-wrapper
@@ -50,9 +53,6 @@ object :obj-user_def_cmd1-arg
object :obj-user_def_cmd2-arg
member ud1a: UserDefOne optional=False
member ud1b: UserDefOne optional=True
-object :obj-user_def_cmd3-arg
- member a: int optional=False
- member b: int optional=True
alternate AltIntNum
case i: int
case n: number
@@ -184,6 +184,8 @@ object __org.qemu_x-Union2
case __org.qemu_x-value: __org.qemu_x-Struct2
command __org.qemu_x-command :obj-__org.qemu_x-command-arg -> __org.qemu_x-Union1
gen=True success_response=True
+command guest-get-time :obj-guest-get-time-arg -> int
+ gen=True success_response=True
command guest-sync :obj-guest-sync-arg -> any
gen=True success_response=True
command user_def_cmd None -> None
@@ -192,5 +194,3 @@ command user_def_cmd1 :obj-user_def_cmd1-arg -> None
gen=True success_response=True
command user_def_cmd2 :obj-user_def_cmd2-arg -> UserDefTwo
gen=True success_response=True
-command user_def_cmd3 :obj-user_def_cmd3-arg -> int
- gen=True success_response=True
diff --git a/tests/qapi-schema/returns-int.err b/tests/qapi-schema/returns-int.err
deleted file mode 100644
index e69de29..0000000
diff --git a/tests/qapi-schema/returns-int.exit b/tests/qapi-schema/returns-int.exit
deleted file mode 100644
index 573541a..0000000
--- a/tests/qapi-schema/returns-int.exit
+++ /dev/null
@@ -1 +0,0 @@
-0
diff --git a/tests/qapi-schema/returns-int.json b/tests/qapi-schema/returns-int.json
deleted file mode 100644
index 870ec63..0000000
--- a/tests/qapi-schema/returns-int.json
+++ /dev/null
@@ -1,3 +0,0 @@
-# It is okay (although not extensible) to return a non-dictionary
-# But to make it work, the name must be in a whitelist
-{ 'command': 'guest-get-time', 'returns': 'int' }
diff --git a/tests/qapi-schema/returns-int.out b/tests/qapi-schema/returns-int.out
deleted file mode 100644
index a2da259..0000000
--- a/tests/qapi-schema/returns-int.out
+++ /dev/null
@@ -1,3 +0,0 @@
-object :empty
-command guest-get-time None -> int
- gen=True success_response=True
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 8d5249e..bc59835 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -46,7 +46,7 @@ UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a,
return ret;
}
-int64_t qmp_user_def_cmd3(int64_t a, bool has_b, int64_t b, Error **errp)
+int64_t qmp_guest_get_time(int64_t a, bool has_b, int64_t b, Error **errp)
{
return a + (has_b ? b : 0);
}
@@ -160,7 +160,7 @@ static void test_dispatch_cmd_io(void)
qdict_put(args3, "a", qint_from_int(66));
qdict_put(req, "arguments", args3);
- qdict_put(req, "execute", qstring_from_str("user_def_cmd3"));
+ qdict_put(req, "execute", qstring_from_str("guest-get-time"));
ret3 = qobject_to_qint(test_qmp_dispatch(req));
assert(qint_get_int(ret3) == 66);
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PATCH v8 06/18] qapi: Drop redundant flat-union-reverse-define test
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
` (4 preceding siblings ...)
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 05/18] qapi: Drop redundant returns-int test Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 07/18] qapi: Don't use info as witness of implicit object type Eric Blake
` (13 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
As of commit 8c3f8e77, we test compilation of forward references
for a struct base type (UserDefOne), flat union base type
(UserDefUnionBase), and flat union branch type
(UserDefFlatUnion2). The only remaining forward reference being
tested for parsing in flat-union-reverse-define was a forward
enum declaration. Once we make sure that always compiles,
the smaller parse-only test is redundant and can be deleted.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: (no v7) hoist from subset C into B
v6: new patch; could be hoisted earlier alongside subset B v7 3/14
---
tests/Makefile | 1 -
tests/qapi-schema/flat-union-reverse-define.err | 0
tests/qapi-schema/flat-union-reverse-define.exit | 1 -
tests/qapi-schema/flat-union-reverse-define.json | 17 -----------------
tests/qapi-schema/flat-union-reverse-define.out | 13 -------------
tests/qapi-schema/qapi-schema-test.json | 11 +++++++----
6 files changed, 7 insertions(+), 36 deletions(-)
delete mode 100644 tests/qapi-schema/flat-union-reverse-define.err
delete mode 100644 tests/qapi-schema/flat-union-reverse-define.exit
delete mode 100644 tests/qapi-schema/flat-union-reverse-define.json
delete mode 100644 tests/qapi-schema/flat-union-reverse-define.out
diff --git a/tests/Makefile b/tests/Makefile
index 3512f8c..35a63f3 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -286,7 +286,6 @@ 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
diff --git a/tests/qapi-schema/flat-union-reverse-define.err b/tests/qapi-schema/flat-union-reverse-define.err
deleted file mode 100644
index e69de29..0000000
diff --git a/tests/qapi-schema/flat-union-reverse-define.exit b/tests/qapi-schema/flat-union-reverse-define.exit
deleted file mode 100644
index 573541a..0000000
--- a/tests/qapi-schema/flat-union-reverse-define.exit
+++ /dev/null
@@ -1 +0,0 @@
-0
diff --git a/tests/qapi-schema/flat-union-reverse-define.json b/tests/qapi-schema/flat-union-reverse-define.json
deleted file mode 100644
index 648bbfe..0000000
--- a/tests/qapi-schema/flat-union-reverse-define.json
+++ /dev/null
@@ -1,17 +0,0 @@
-{ 'union': 'TestUnion',
- 'base': 'TestBase',
- 'discriminator': 'enum1',
- 'data': { 'value1': 'TestTypeA',
- 'value2': 'TestTypeB' } }
-
-{ 'struct': 'TestBase',
- 'data': { 'enum1': 'TestEnum' } }
-
-{ 'enum': 'TestEnum',
- 'data': [ 'value1', 'value2' ] }
-
-{ 'struct': 'TestTypeA',
- 'data': { 'string': 'str' } }
-
-{ 'struct': 'TestTypeB',
- 'data': { 'integer': 'int' } }
diff --git a/tests/qapi-schema/flat-union-reverse-define.out b/tests/qapi-schema/flat-union-reverse-define.out
deleted file mode 100644
index a5a9134..0000000
--- a/tests/qapi-schema/flat-union-reverse-define.out
+++ /dev/null
@@ -1,13 +0,0 @@
-object :empty
-object TestBase
- member enum1: TestEnum optional=False
-enum TestEnum ['value1', 'value2']
-object TestTypeA
- member string: str optional=False
-object TestTypeB
- member integer: int optional=False
-object TestUnion
- base TestBase
- tag enum1
- case value1: TestTypeA
- case value2: TestTypeB
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 67c6bca..b48c6bd 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -4,10 +4,9 @@
# parse and compile correctly.
# for testing enums
-{ 'enum': 'EnumOne',
- 'data': [ 'value1', 'value2', 'value3' ] }
{ 'struct': 'NestedEnumsOne',
- 'data': { 'enum1': 'EnumOne', '*enum2': 'EnumOne', 'enum3': 'EnumOne', '*enum4': 'EnumOne' } }
+ 'data': { 'enum1': 'EnumOne', # Intentional forward reference
+ '*enum2': 'EnumOne', 'enum3': 'EnumOne', '*enum4': 'EnumOne' } }
# An empty enum, although unusual, is currently acceptable
{ 'enum': 'MyEnum', 'data': [ ] }
@@ -20,7 +19,11 @@
# for testing nested structs
{ 'struct': 'UserDefOne',
'base': 'UserDefZero', # intentional forward reference
- 'data': { 'string': 'str', '*enum1': 'EnumOne' } }
+ 'data': { 'string': 'str',
+ '*enum1': 'EnumOne' } } # intentional forward reference
+
+{ 'enum': 'EnumOne',
+ 'data': [ 'value1', 'value2', 'value3' ] }
{ 'struct': 'UserDefZero',
'data': { 'integer': 'int' } }
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PATCH v8 07/18] qapi: Don't use info as witness of implicit object type
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
` (5 preceding siblings ...)
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 06/18] qapi: Drop redundant flat-union-reverse-define test Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-13 11:40 ` Markus Armbruster
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 08/18] qapi: Lazy creation of array types Eric Blake
` (12 subsequent siblings)
19 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
A future patch will enable error reporting from the various
QAPISchema*.check() methods. But to report an error related
to an implicit type, we'll need to associate a location with
the type (the same location as the top-level entity that is
causing the creation of the implicit type), and once we do
that, keying off of whether foo.info exists is no longer a
viable way to determine if foo is an implicit type.
Instead, add an is_implicit() method to QAPISchemaEntity, with
overrides for ObjectType and EnumType, and use that function
where needed.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: separate isinstance back to verbose form, add comments
v7: declare at the Entity level, with an optional argument for
filtering by sub-class
v6: split 11/46 into pieces; don't rename _info yet; rework atop
nicer filtering mechanism, including no need to change visitor
signature
---
scripts/qapi-types.py | 3 ++-
scripts/qapi-visit.py | 3 ++-
scripts/qapi.py | 22 ++++++++++++++++++----
3 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 2a29c6e..4fe618e 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -235,7 +235,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
def visit_needed(self, entity):
# Visit everything except implicit objects
- return not isinstance(entity, QAPISchemaObjectType) or entity.info
+ return not (entity.is_implicit() and
+ isinstance(entity, QAPISchemaObjectType))
def _gen_type_cleanup(self, name):
self.decl += gen_type_cleanup_decl(name)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 9fc79a7..2a9fab8 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -335,7 +335,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
def visit_needed(self, entity):
# Visit everything except implicit objects
- return not isinstance(entity, QAPISchemaObjectType) or entity.info
+ return not (entity.is_implicit() and
+ isinstance(entity, QAPISchemaObjectType))
def visit_enum_type(self, name, info, values, prefix):
self.decl += gen_visit_decl(name, scalar=True)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 68f97a1..e263ecf 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -798,6 +798,9 @@ class QAPISchemaEntity(object):
def check(self, schema):
pass
+ def is_implicit(self):
+ return not self.info
+
def visit(self, visitor):
pass
@@ -900,6 +903,10 @@ class QAPISchemaEnumType(QAPISchemaType):
def check(self, schema):
assert len(set(self.values)) == len(self.values)
+ def is_implicit(self):
+ # See QAPISchema._make_implicit_enum_type()
+ return self.name[-4:] == 'Kind'
+
def c_type(self, is_param=False):
return c_name(self.name)
@@ -970,12 +977,16 @@ class QAPISchemaObjectType(QAPISchemaType):
self.variants.check(schema, members, seen)
self.members = members
+ def is_implicit(self):
+ # See QAPISchema._make_implicit_object_type()
+ return self.name[0] == ':'
+
def c_name(self):
- assert self.info
+ assert not self.is_implicit()
return QAPISchemaType.c_name(self)
def c_type(self, is_param=False):
- assert self.info
+ assert not self.is_implicit()
return QAPISchemaType.c_type(self)
def json_type(self):
@@ -1043,7 +1054,8 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
# This function exists to support ugly simple union special cases
# TODO get rid of them, and drop the function
def simple_union_type(self):
- if isinstance(self.type, QAPISchemaObjectType) and not self.type.info:
+ if (self.type.is_implicit() and
+ isinstance(self.type, QAPISchemaObjectType)):
assert len(self.type.members) == 1
assert not self.type.variants
return self.type.members[0].type
@@ -1162,11 +1174,13 @@ class QAPISchema(object):
self._def_entity(self.the_empty_object_type)
def _make_implicit_enum_type(self, name, values):
- name = name + 'Kind'
+ name = name + 'Kind' # Use namespace reserved by add_name()
self._def_entity(QAPISchemaEnumType(name, None, values, None))
return name
def _make_array_type(self, element_type):
+ # TODO fooList namespace is not reserved; user can create collisions,
+ # or abuse our type system with ['fooList'] for 2D array
name = element_type + 'List'
if not self.lookup_type(name):
self._def_entity(QAPISchemaArrayType(name, None, element_type))
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 07/18] qapi: Don't use info as witness of implicit object type
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 07/18] qapi: Don't use info as witness of implicit object type Eric Blake
@ 2015-10-13 11:40 ` Markus Armbruster
2015-10-13 13:05 ` Eric Blake
0 siblings, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2015-10-13 11:40 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> A future patch will enable error reporting from the various
> QAPISchema*.check() methods. But to report an error related
> to an implicit type, we'll need to associate a location with
> the type (the same location as the top-level entity that is
> causing the creation of the implicit type), and once we do
> that, keying off of whether foo.info exists is no longer a
> viable way to determine if foo is an implicit type.
>
> Instead, add an is_implicit() method to QAPISchemaEntity, with
> overrides for ObjectType and EnumType, and use that function
> where needed.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v8: separate isinstance back to verbose form, add comments
> v7: declare at the Entity level, with an optional argument for
> filtering by sub-class
> v6: split 11/46 into pieces; don't rename _info yet; rework atop
> nicer filtering mechanism, including no need to change visitor
> signature
> ---
> scripts/qapi-types.py | 3 ++-
> scripts/qapi-visit.py | 3 ++-
> scripts/qapi.py | 22 ++++++++++++++++++----
> 3 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 2a29c6e..4fe618e 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -235,7 +235,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>
> def visit_needed(self, entity):
> # Visit everything except implicit objects
> - return not isinstance(entity, QAPISchemaObjectType) or entity.info
> + return not (entity.is_implicit() and
> + isinstance(entity, QAPISchemaObjectType))
>
> def _gen_type_cleanup(self, name):
> self.decl += gen_type_cleanup_decl(name)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 9fc79a7..2a9fab8 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -335,7 +335,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
>
> def visit_needed(self, entity):
> # Visit everything except implicit objects
> - return not isinstance(entity, QAPISchemaObjectType) or entity.info
> + return not (entity.is_implicit() and
> + isinstance(entity, QAPISchemaObjectType))
>
> def visit_enum_type(self, name, info, values, prefix):
> self.decl += gen_visit_decl(name, scalar=True)
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 68f97a1..e263ecf 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -798,6 +798,9 @@ class QAPISchemaEntity(object):
> def check(self, schema):
> pass
>
> + def is_implicit(self):
> + return not self.info
> +
> def visit(self, visitor):
> pass
>
> @@ -900,6 +903,10 @@ class QAPISchemaEnumType(QAPISchemaType):
> def check(self, schema):
> assert len(set(self.values)) == len(self.values)
>
> + def is_implicit(self):
> + # See QAPISchema._make_implicit_enum_type()
> + return self.name[-4:] == 'Kind'
> +
> def c_type(self, is_param=False):
> return c_name(self.name)
>
I believe this method...
> @@ -970,12 +977,16 @@ class QAPISchemaObjectType(QAPISchemaType):
> self.variants.check(schema, members, seen)
> self.members = members
>
> + def is_implicit(self):
> + # See QAPISchema._make_implicit_object_type()
> + return self.name[0] == ':'
> +
... as well as this one are redundant at this stage. They become
necessary only when you start passing info != None to the constructor in
PATCH 12. If I'm right, then moving the two to PATCH 12 makes sense.
> def c_name(self):
> - assert self.info
> + assert not self.is_implicit()
> return QAPISchemaType.c_name(self)
>
> def c_type(self, is_param=False):
> - assert self.info
> + assert not self.is_implicit()
> return QAPISchemaType.c_type(self)
>
> def json_type(self):
> @@ -1043,7 +1054,8 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
> # This function exists to support ugly simple union special cases
> # TODO get rid of them, and drop the function
> def simple_union_type(self):
> - if isinstance(self.type, QAPISchemaObjectType) and not self.type.info:
> + if (self.type.is_implicit() and
> + isinstance(self.type, QAPISchemaObjectType)):
> assert len(self.type.members) == 1
> assert not self.type.variants
> return self.type.members[0].type
> @@ -1162,11 +1174,13 @@ class QAPISchema(object):
> self._def_entity(self.the_empty_object_type)
>
> def _make_implicit_enum_type(self, name, values):
> - name = name + 'Kind'
> + name = name + 'Kind' # Use namespace reserved by add_name()
This is the comment I suggested...
> self._def_entity(QAPISchemaEnumType(name, None, values, None))
> return name
>
> def _make_array_type(self, element_type):
> + # TODO fooList namespace is not reserved; user can create collisions,
> + # or abuse our type system with ['fooList'] for 2D array
... and this is its buddy you added on your own initiative. Thanks!
Did you actually try the abuse? If not, I'd say "or maybe abuse", out
of caution.
> name = element_type + 'List'
> if not self.lookup_type(name):
> self._def_entity(QAPISchemaArrayType(name, None, element_type))
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [PATCH v8 07/18] qapi: Don't use info as witness of implicit object type
2015-10-13 11:40 ` Markus Armbruster
@ 2015-10-13 13:05 ` Eric Blake
0 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2015-10-13 13:05 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2322 bytes --]
On 10/13/2015 05:40 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> @@ -900,6 +903,10 @@ class QAPISchemaEnumType(QAPISchemaType):
>> def check(self, schema):
>> assert len(set(self.values)) == len(self.values)
>>
>> + def is_implicit(self):
>> + # See QAPISchema._make_implicit_enum_type()
>> + return self.name[-4:] == 'Kind'
>> +
>> def c_type(self, is_param=False):
>> return c_name(self.name)
>>
>
> I believe this method...
>
>> @@ -970,12 +977,16 @@ class QAPISchemaObjectType(QAPISchemaType):
>> self.variants.check(schema, members, seen)
>> self.members = members
>>
>> + def is_implicit(self):
>> + # See QAPISchema._make_implicit_object_type()
>> + return self.name[0] == ':'
>> +
>
> ... as well as this one are redundant at this stage. They become
> necessary only when you start passing info != None to the constructor in
> PATCH 12. If I'm right, then moving the two to PATCH 12 makes sense.
I think you're right; we could float these hunks to where they become
important if there is a reason for a respin.
>> def _make_implicit_enum_type(self, name, values):
>> - name = name + 'Kind'
>> + name = name + 'Kind' # Use namespace reserved by add_name()
>
> This is the comment I suggested...
>
>> self._def_entity(QAPISchemaEnumType(name, None, values, None))
>> return name
>>
>> def _make_array_type(self, element_type):
>> + # TODO fooList namespace is not reserved; user can create collisions,
>> + # or abuse our type system with ['fooList'] for 2D array
>
> ... and this is its buddy you added on your own initiative. Thanks!
>
> Did you actually try the abuse? If not, I'd say "or maybe abuse", out
> of caution.
Yes, back in an earlier version of your introspection work, we played
with 2D arrays, and concluded that it wasn't worth worrying about until
the queue is flushed:
https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg00551.html
But back then, when I was playing with it, I did confirm that
['fooList'] gets past the generator.
--
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] 51+ messages in thread
* [Qemu-devel] [PATCH v8 08/18] qapi: Lazy creation of array types
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
` (6 preceding siblings ...)
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 07/18] qapi: Don't use info as witness of implicit object type Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-14 7:15 ` Markus Armbruster
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 09/18] qapi: Create simple union type member earlier Eric Blake
` (11 subsequent siblings)
19 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Commit ac88219a had several TODO markers about whether we needed
to automatically create the corresponding array type alongside
any other type. It turns out that most of the time, we don't!
As part of lazy creation of array types, this patch now assigns
an 'info' to array types at their point of first instantiation,
rather than leaving it None.
There are a few exceptions: 1) We have a few situations where we
use an array type in internal code but do not expose that type
through QMP; fix it by declaring a dummy type that forces the
generator to see that we want to use the array type.
2) The builtin arrays (such as intList for QAPI ['int']) must
always be generated, because of the way our QAPI_TYPES_BUILTIN
compile guard works: we have situations (at the very least
tests/test-qmp-output-visitor.c) that include both top-level
"qapi-types.h" (via "error.h") and a secondary
"test-qapi-types.h". If we were to only emit the builtin types
when used locally, then the first .h file would not include all
types, but the second .h does not declare anything at all because
the first .h set QAPI_TYPES_BUILTIN, and we would end up with
compilation error due to things like unknown type 'int8List'.
Actually, we may need to revisit how we do type guards, and
change from a single QAPI_TYPES_BUILTIN over to a different
usage pattern that does one #ifdef per qapi type - right now,
the only types that are declared multiple times between two qapi
.json files for inclusion by a single .c file happen to be the
builtin arrays. But now that we have QAPI 'include' statements,
it is logical to assume that we will soon reach a point where
we want to reuse non-builtin types (yes, I'm thinking about what
it will take to add introspection to QGA, where we will want to
reuse the SchemaInfo type and friends). One #ifdef per type
will help ensure that generating the same qapi type into more
than one qapi-types.h won't cause collisions when both are
included in the same .c file; but we also have to solve how to
avoid creating duplicate qapi-types.c entry points. So that
is a problem left for another day.
Generated code for qapi-types and qapi-visit is drastically
reduced; less than a third of the arrays that were blindly
created were actually needed (a quick grep shows we dropped
from 219 to 69 *List types), and the .o files lost more than
30% of their bulk. [For best results, diff the generated
files with 'git diff --patience --no-index pre post'.]
Interestingly, the introspection output is unchanged - this is
because we already cull all types that are not indirectly
reachable from a command or event, so introspection was already
using only a subset of array types. The subset of types
introspected is now a much larger percentage of the overall set
of array types emitted in qapi-types.h (since the larger set
shrunk), but still not 100% (evidence that the array types
emitted for our new Dummy structs, and the new struct itself,
don't affect QMP).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: commit message wording, rebase qapi-schema-test.out,
improved comment on .info
v7: improve commit message, add comments, rename dummy type,
better line wrapping
v6: new patch
---
qapi-schema.json | 11 +++++++
scripts/qapi.py | 53 +++++++++++++++++++--------------
tests/qapi-schema/qapi-schema-test.json | 4 +++
tests/qapi-schema/qapi-schema-test.out | 3 ++
4 files changed, 48 insertions(+), 23 deletions(-)
diff --git a/qapi-schema.json b/qapi-schema.json
index a386605..702b7b5 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3416,6 +3416,17 @@
'features': 'int' } }
##
+# @DummyForceArrays
+#
+# Not used by QMP; hack to let us use X86CPUFeatureWordInfoList internally
+#
+# Since 2.5
+##
+{ 'struct': 'DummyForceArrays',
+ 'data': { 'unused': ['X86CPUFeatureWordInfo'] } }
+
+
+##
# @RxState:
#
# Packets receiving state
diff --git a/scripts/qapi.py b/scripts/qapi.py
index e263ecf..73080e3 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -790,6 +790,12 @@ class QAPISchemaEntity(object):
def __init__(self, name, info):
assert isinstance(name, str)
self.name = name
+ # For explicitly defined entities, info points to the (explicit)
+ # definition. For builtins (and their arrays), info is None.
+ # For other arrays, info points to an explicit place that uses
+ # the array (there may be more than one such place).
+ # TODO For other implicitly defined entities, it should point to
+ # a place that triggers implicit definition.
self.info = info
def c_name(self):
@@ -1151,7 +1157,12 @@ class QAPISchema(object):
def _def_builtin_type(self, name, json_type, c_type, c_null):
self._def_entity(QAPISchemaBuiltinType(name, json_type,
c_type, c_null))
- self._make_array_type(name) # TODO really needed?
+ # TODO As long as we have QAPI_TYPES_BUILTIN to share multiple
+ # qapi-types.h from a single .c, all arrays of builtins must be
+ # declared in the first file whether or not they are used. Nicer
+ # would be to use lazy instantiation, while figuring out how to
+ # avoid compilation issues with multiple qapi-types.h.
+ self._make_array_type(name, None)
def _def_predefineds(self):
for t in [('str', 'string', 'char' + pointer_suffix, 'NULL'),
@@ -1178,12 +1189,12 @@ class QAPISchema(object):
self._def_entity(QAPISchemaEnumType(name, None, values, None))
return name
- def _make_array_type(self, element_type):
+ def _make_array_type(self, element_type, info):
# TODO fooList namespace is not reserved; user can create collisions,
# or abuse our type system with ['fooList'] for 2D array
name = element_type + 'List'
if not self.lookup_type(name):
- self._def_entity(QAPISchemaArrayType(name, None, element_type))
+ self._def_entity(QAPISchemaArrayType(name, info, element_type))
return name
def _make_implicit_object_type(self, name, role, members):
@@ -1200,20 +1211,19 @@ class QAPISchema(object):
data = expr['data']
prefix = expr.get('prefix')
self._def_entity(QAPISchemaEnumType(name, info, data, prefix))
- self._make_array_type(name) # TODO really needed?
- def _make_member(self, name, typ):
+ def _make_member(self, name, typ, info):
optional = False
if name.startswith('*'):
name = name[1:]
optional = True
if isinstance(typ, list):
assert len(typ) == 1
- typ = self._make_array_type(typ[0])
+ typ = self._make_array_type(typ[0], info)
return QAPISchemaObjectTypeMember(name, typ, optional)
- def _make_members(self, data):
- return [self._make_member(key, value)
+ def _make_members(self, data, info):
+ return [self._make_member(key, value, info)
for (key, value) in data.iteritems()]
def _def_struct_type(self, expr, info):
@@ -1221,19 +1231,18 @@ class QAPISchema(object):
base = expr.get('base')
data = expr['data']
self._def_entity(QAPISchemaObjectType(name, info, base,
- self._make_members(data),
+ self._make_members(data, info),
None))
- self._make_array_type(name) # TODO really needed?
def _make_variant(self, case, typ):
return QAPISchemaObjectTypeVariant(case, typ)
- def _make_simple_variant(self, case, typ):
+ def _make_simple_variant(self, case, typ, info):
if isinstance(typ, list):
assert len(typ) == 1
- typ = self._make_array_type(typ[0])
- typ = self._make_implicit_object_type(typ, 'wrapper',
- [self._make_member('data', typ)])
+ typ = self._make_array_type(typ[0], info)
+ typ = self._make_implicit_object_type(
+ typ, 'wrapper', [self._make_member('data', typ, info)])
return QAPISchemaObjectTypeVariant(case, typ)
def _make_tag_enum(self, type_name, variants):
@@ -1250,16 +1259,15 @@ class QAPISchema(object):
variants = [self._make_variant(key, value)
for (key, value) in data.iteritems()]
else:
- variants = [self._make_simple_variant(key, value)
+ variants = [self._make_simple_variant(key, value, info)
for (key, value) in data.iteritems()]
tag_enum = self._make_tag_enum(name, variants)
self._def_entity(
QAPISchemaObjectType(name, info, base,
- self._make_members(OrderedDict()),
+ self._make_members(OrderedDict(), info),
QAPISchemaObjectTypeVariants(tag_name,
tag_enum,
variants)))
- self._make_array_type(name) # TODO really needed?
def _def_alternate_type(self, expr, info):
name = expr['alternate']
@@ -1272,7 +1280,6 @@ class QAPISchema(object):
QAPISchemaObjectTypeVariants(None,
tag_enum,
variants)))
- self._make_array_type(name) # TODO really needed?
def _def_command(self, expr, info):
name = expr['command']
@@ -1281,11 +1288,11 @@ class QAPISchema(object):
gen = expr.get('gen', True)
success_response = expr.get('success-response', True)
if isinstance(data, OrderedDict):
- data = self._make_implicit_object_type(name, 'arg',
- self._make_members(data))
+ data = self._make_implicit_object_type(
+ name, 'arg', self._make_members(data, info))
if isinstance(rets, list):
assert len(rets) == 1
- rets = self._make_array_type(rets[0])
+ rets = self._make_array_type(rets[0], info)
self._def_entity(QAPISchemaCommand(name, info, data, rets, gen,
success_response))
@@ -1293,8 +1300,8 @@ class QAPISchema(object):
name = expr['event']
data = expr.get('data')
if isinstance(data, OrderedDict):
- data = self._make_implicit_object_type(name, 'arg',
- self._make_members(data))
+ data = self._make_implicit_object_type(
+ name, 'arg', self._make_members(data, info))
self._def_entity(QAPISchemaEvent(name, info, data))
def _def_exprs(self):
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index b48c6bd..4e2d7c2 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -40,6 +40,10 @@
'data': { 'string0': 'str',
'dict1': 'UserDefTwoDict' } }
+# dummy struct to force generation of array types not otherwise mentioned
+{ 'struct': 'ForceArrays',
+ 'data': { 'unused1':['UserDefOne'], 'unused2':['UserDefTwo'] } }
+
# 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),
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 93f6250..a6c80e0 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -86,6 +86,9 @@ object EventStructOne
member struct1: UserDefOne optional=False
member string: str optional=False
member enum2: EnumOne optional=True
+object ForceArrays
+ member unused1: UserDefOneList optional=False
+ member unused2: UserDefTwoList optional=False
enum MyEnum []
object NestedEnumsOne
member enum1: EnumOne optional=False
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 08/18] qapi: Lazy creation of array types
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 08/18] qapi: Lazy creation of array types Eric Blake
@ 2015-10-14 7:15 ` Markus Armbruster
2015-10-14 12:57 ` Eric Blake
0 siblings, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2015-10-14 7:15 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Commit ac88219a had several TODO markers about whether we needed
> to automatically create the corresponding array type alongside
> any other type. It turns out that most of the time, we don't!
>
> As part of lazy creation of array types, this patch now assigns
> an 'info' to array types at their point of first instantiation,
> rather than leaving it None.
I'm afraid this flips the value of .is_implicit() to False. Currently
harmless, but let's keep it correct anyway.
The obvious fix is to define the trivial override method:
def is_implicit(self):
return True
But I'd rather do *all* the "give implicit types info" work in "qapi:
Track location that created an implicit type", i.e. move the plumbing of
info there, add the override method there, drop the "As part of"
paragraph from the commit message here. I append what's left of this
patch then. I like it, because the patch that actually changes
generated code (this one) becomes really simple, and the lengthened
patch remains mere info-plumbing that doesn't affect the generated code.
diff --git a/scripts/qapi.py b/scripts/qapi.py
index d7cf0f3..9e01705 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1143,7 +1143,12 @@ class QAPISchema(object):
def _def_builtin_type(self, name, json_type, c_type, c_null):
self._def_entity(QAPISchemaBuiltinType(name, json_type,
c_type, c_null))
- self._make_array_type(name) # TODO really needed?
+ # TODO As long as we have QAPI_TYPES_BUILTIN to share multiple
+ # qapi-types.h from a single .c, all arrays of builtins must be
+ # declared in the first file whether or not they are used. Nicer
+ # would be to use lazy instantiation, while figuring out how to
+ # avoid compilation issues with multiple qapi-types.h.
+ self._make_array_type(name)
def _def_predefineds(self):
for t in [('str', 'string', 'char' + pointer_suffix, 'NULL'),
@@ -1192,7 +1197,6 @@ class QAPISchema(object):
data = expr['data']
prefix = expr.get('prefix')
self._def_entity(QAPISchemaEnumType(name, info, data, prefix))
- self._make_array_type(name) # TODO really needed?
def _make_member(self, name, typ):
optional = False
@@ -1215,7 +1219,6 @@ class QAPISchema(object):
self._def_entity(QAPISchemaObjectType(name, info, base,
self._make_members(data),
None))
- self._make_array_type(name) # TODO really needed?
def _make_variant(self, case, typ):
return QAPISchemaObjectTypeVariant(case, typ)
@@ -1251,7 +1254,6 @@ class QAPISchema(object):
QAPISchemaObjectTypeVariants(tag_name,
tag_enum,
variants)))
- self._make_array_type(name) # TODO really needed?
def _def_alternate_type(self, expr, info):
name = expr['alternate']
@@ -1264,7 +1266,6 @@ class QAPISchema(object):
QAPISchemaObjectTypeVariants(None,
tag_enum,
variants)))
- self._make_array_type(name) # TODO really needed?
def _def_command(self, expr, info):
name = expr['command']
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 08/18] qapi: Lazy creation of array types
2015-10-14 7:15 ` Markus Armbruster
@ 2015-10-14 12:57 ` Eric Blake
0 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2015-10-14 12:57 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]
On 10/14/2015 01:15 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Commit ac88219a had several TODO markers about whether we needed
>> to automatically create the corresponding array type alongside
>> any other type. It turns out that most of the time, we don't!
>>
>> As part of lazy creation of array types, this patch now assigns
>> an 'info' to array types at their point of first instantiation,
>> rather than leaving it None.
>
> I'm afraid this flips the value of .is_implicit() to False. Currently
> harmless, but let's keep it correct anyway.
>
> The obvious fix is to define the trivial override method:
>
> def is_implicit(self):
> return True
>
> But I'd rather do *all* the "give implicit types info" work in "qapi:
> Track location that created an implicit type", i.e. move the plumbing of
> info there, add the override method there, drop the "As part of"
> paragraph from the commit message here. I append what's left of this
> patch then. I like it, because the patch that actually changes
> generated code (this one) becomes really simple, and the lengthened
> patch remains mere info-plumbing that doesn't affect the generated code.
Makes sense, so I agree with how you've redone the current state of
qapi-next.
--
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] 51+ messages in thread
* [Qemu-devel] [PATCH v8 09/18] qapi: Create simple union type member earlier
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
` (7 preceding siblings ...)
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 08/18] qapi: Lazy creation of array types Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 10/18] qapi: Move union tag quirks into subclass Eric Blake
` (10 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
For simple unions, we were creating the implicit 'type' tag
member during the QAPISchemaObjectTypeVariants constructor.
This is different from every other implicit QAPISchemaEntity
object, which get created by QAPISchema methods. Hoist the
creation to the caller (renaming _make_tag_enum() to
_make_implicit_tag()), and pass the entity rather than the
string name, so that we have the nice property that no
entities are created as a side effect within a different
entity. A later patch will then have an easier time of
associating location info with each entity creation.
No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: rename tag_enum to tag_member
v7: Rework assertions, rename to _make_implicit_tag()
v6: New patch
---
scripts/qapi.py | 35 ++++++++++++++++++-----------------
1 file changed, 18 insertions(+), 17 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 73080e3..5b66264 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1024,18 +1024,18 @@ class QAPISchemaObjectTypeMember(object):
class QAPISchemaObjectTypeVariants(object):
- def __init__(self, tag_name, tag_enum, variants):
- assert tag_name is None or isinstance(tag_name, str)
- assert tag_enum is None or isinstance(tag_enum, str)
+ def __init__(self, tag_name, tag_member, variants):
+ # Flat unions pass tag_name but not tag_member.
+ # Simple unions and alternates pass tag_member but not tag_name.
+ # After check(), tag_member is always set, and tag_name remains
+ # a reliable witness of being used by a flat union.
+ assert bool(tag_member) != bool(tag_name)
+ assert (isinstance(tag_name, str) or
+ isinstance(tag_member, QAPISchemaObjectTypeMember))
for v in variants:
assert isinstance(v, QAPISchemaObjectTypeVariant)
self.tag_name = tag_name
- if tag_name:
- assert not tag_enum
- self.tag_member = None
- else:
- self.tag_member = QAPISchemaObjectTypeMember('type', tag_enum,
- False)
+ self.tag_member = tag_member
self.variants = variants
def check(self, schema, members, seen):
@@ -1245,28 +1245,29 @@ class QAPISchema(object):
typ, 'wrapper', [self._make_member('data', typ, info)])
return QAPISchemaObjectTypeVariant(case, typ)
- def _make_tag_enum(self, type_name, variants):
- return self._make_implicit_enum_type(type_name,
- [v.name for v in variants])
+ def _make_implicit_tag(self, type_name, variants):
+ typ = self._make_implicit_enum_type(type_name,
+ [v.name for v in variants])
+ return QAPISchemaObjectTypeMember('type', typ, False)
def _def_union_type(self, expr, info):
name = expr['union']
data = expr['data']
base = expr.get('base')
tag_name = expr.get('discriminator')
- tag_enum = None
+ tag_member = None
if tag_name:
variants = [self._make_variant(key, value)
for (key, value) in data.iteritems()]
else:
variants = [self._make_simple_variant(key, value, info)
for (key, value) in data.iteritems()]
- tag_enum = self._make_tag_enum(name, variants)
+ tag_member = self._make_implicit_tag(name, variants)
self._def_entity(
QAPISchemaObjectType(name, info, base,
self._make_members(OrderedDict(), info),
QAPISchemaObjectTypeVariants(tag_name,
- tag_enum,
+ tag_member,
variants)))
def _def_alternate_type(self, expr, info):
@@ -1274,11 +1275,11 @@ class QAPISchema(object):
data = expr['data']
variants = [self._make_variant(key, value)
for (key, value) in data.iteritems()]
- tag_enum = self._make_tag_enum(name, variants)
+ tag_member = self._make_implicit_tag(name, variants)
self._def_entity(
QAPISchemaAlternateType(name, info,
QAPISchemaObjectTypeVariants(None,
- tag_enum,
+ tag_member,
variants)))
def _def_command(self, expr, info):
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PATCH v8 10/18] qapi: Move union tag quirks into subclass
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
` (8 preceding siblings ...)
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 09/18] qapi: Create simple union type member earlier Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-13 12:10 ` Markus Armbruster
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 11/18] qapi: Simplify gen_struct_field() Eric Blake
` (9 subsequent siblings)
19 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Right now, simple unions have a quirk of using 'kind' in the C
struct to match the QMP wire name 'type'. This has resulted in
messy clients each doing special cases. While we plan to
eventually rename things to match, it is better in the meantime
to consolidate the quirks into a special subclass, by adding a
new member.c_name() function. This will also make it easier
for reworking how alternate types are laid out in a future
patch. Use the new c_name() function where possible.
No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v7: new patch, but borrows idea of subclass from v6 10/12, as
well as c_name() from 7/12
---
scripts/qapi-commands.py | 8 ++++----
scripts/qapi-types.py | 12 +++++-------
scripts/qapi-visit.py | 17 +++++------------
scripts/qapi.py | 26 +++++++++++++++++++-------
4 files changed, 33 insertions(+), 30 deletions(-)
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 43a893b..53a79ab 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -32,8 +32,8 @@ def gen_call(name, arg_type, ret_type):
if arg_type:
for memb in arg_type.members:
if memb.optional:
- argstr += 'has_%s, ' % c_name(memb.name)
- argstr += '%s, ' % c_name(memb.name)
+ argstr += 'has_' + memb.c_name() + ', '
+ argstr += memb.c_name() + ', '
lhs = ''
if ret_type:
@@ -77,11 +77,11 @@ def gen_marshal_vars(arg_type, ret_type):
ret += mcgen('''
bool has_%(c_name)s = false;
''',
- c_name=c_name(memb.name))
+ c_name=memb.c_name())
ret += mcgen('''
%(c_type)s %(c_name)s = %(c_null)s;
''',
- c_name=c_name(memb.name),
+ c_name=memb.c_name(),
c_type=memb.type.c_type(),
c_null=memb.type.c_null())
ret += '\n'
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 4fe618e..e37d529 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -136,9 +136,10 @@ struct %(c_name)s {
''')
else:
ret += mcgen('''
- %(c_type)s kind;
+ %(c_type)s %(c_name)s;
''',
- c_type=c_name(variants.tag_member.type.name))
+ c_type=variants.tag_member.type.c_name(),
+ c_name=variants.tag_member.c_name())
# FIXME: What purpose does data serve, besides preventing a union that
# has a branch named 'data'? We use it in qapi-visit.py to decide
@@ -152,10 +153,7 @@ struct %(c_name)s {
union { /* union tag is @%(c_name)s */
void *data;
''',
- # 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'))
+ c_name=variants.tag_member.c_name())
for var in variants.variants:
# Ugly special case for simple union TODO get rid of it
@@ -164,7 +162,7 @@ struct %(c_name)s {
%(c_type)s %(c_name)s;
''',
c_type=typ.c_type(),
- c_name=c_name(var.name))
+ c_name=var.c_name())
ret += mcgen('''
};
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 2a9fab8..cb251b2 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -196,7 +196,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
case=c_enum_const(variants.tag_member.type.name,
var.name),
c_type=var.type.c_name(),
- c_name=c_name(var.name))
+ c_name=var.c_name())
ret += mcgen('''
default:
@@ -249,10 +249,6 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
c_name=c_name(name))
ret += gen_err_check(label='out_obj')
- tag_key = variants.tag_member.name
- if not variants.tag_name:
- # 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) {
@@ -264,11 +260,8 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
switch ((*obj)->%(c_name)s) {
''',
c_type=variants.tag_member.type.c_name(),
- # 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'),
- name=tag_key)
+ c_name=variants.tag_member.c_name(),
+ name=variants.tag_member.name)
for var in variants.variants:
# TODO ugly special case for simple union
@@ -283,13 +276,13 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
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))
+ c_name=var.c_name())
else:
ret += mcgen('''
visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
''',
c_type=var.type.c_name(),
- c_name=c_name(var.name))
+ c_name=var.c_name())
ret += mcgen('''
break;
''')
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 5b66264..80c026b 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -975,7 +975,7 @@ class QAPISchemaObjectType(QAPISchemaType):
members = []
seen = {}
for m in members:
- assert c_name(m.name) not in seen
+ assert m.c_name() not in seen
seen[m.name] = m
for m in self.local_members:
m.check(schema, members, seen)
@@ -1022,6 +1022,18 @@ class QAPISchemaObjectTypeMember(object):
all_members.append(self)
seen[self.name] = self
+ def c_name(self):
+ return c_name(self.name)
+
+
+# TODO Drop this class once we no longer have the 'type'/'kind' mismatch
+class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
+ def c_name(self):
+ assert self.name == 'type'
+ assert isinstance(self.type, QAPISchemaEnumType)
+ assert self.type.is_implicit()
+ return 'kind'
+
class QAPISchemaObjectTypeVariants(object):
def __init__(self, tag_name, tag_member, variants):
@@ -1248,7 +1260,7 @@ class QAPISchema(object):
def _make_implicit_tag(self, type_name, variants):
typ = self._make_implicit_enum_type(type_name,
[v.name for v in variants])
- return QAPISchemaObjectTypeMember('type', typ, False)
+ return QAPISchemaObjectTypeUnionTag('type', typ, False)
def _def_union_type(self, expr, info):
name = expr['union']
@@ -1555,8 +1567,8 @@ def gen_params(arg_type, extra):
ret += sep
sep = ', '
if memb.optional:
- ret += 'bool has_%s, ' % c_name(memb.name)
- ret += '%s %s' % (memb.type.c_type(is_param=True), c_name(memb.name))
+ ret += 'bool has_' + memb.c_name() + sep
+ ret += '%s %s' % (memb.type.c_type(is_param=True), memb.c_name())
if extra:
ret += sep + extra
return ret
@@ -1585,13 +1597,13 @@ def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
ret += mcgen('''
visit_optional(v, &%(prefix)shas_%(c_name)s, "%(name)s", %(errp)s);
''',
- prefix=prefix, c_name=c_name(memb.name),
+ prefix=prefix, c_name=memb.c_name(),
name=memb.name, errp=errparg)
ret += gen_err_check(skiperr=skiperr)
ret += mcgen('''
if (%(prefix)shas_%(c_name)s) {
''',
- prefix=prefix, c_name=c_name(memb.name))
+ prefix=prefix, c_name=memb.c_name())
push_indent()
# Ugly: sometimes we need to cast away const
@@ -1604,7 +1616,7 @@ def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
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,
+ c_name=memb.c_name(), name=memb.name,
errp=errparg)
ret += gen_err_check(skiperr=skiperr)
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 10/18] qapi: Move union tag quirks into subclass
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 10/18] qapi: Move union tag quirks into subclass Eric Blake
@ 2015-10-13 12:10 ` Markus Armbruster
2015-10-13 14:15 ` Eric Blake
0 siblings, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2015-10-13 12:10 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Right now, simple unions have a quirk of using 'kind' in the C
> struct to match the QMP wire name 'type'. This has resulted in
> messy clients each doing special cases. While we plan to
> eventually rename things to match, it is better in the meantime
> to consolidate the quirks into a special subclass, by adding a
> new member.c_name() function. This will also make it easier
> for reworking how alternate types are laid out in a future
> patch. Use the new c_name() function where possible.
Terminology: "the new c_name() method".
I don't like having both function c_name() and method c_name(), because
it's very easy to use the function when you should use the method, and
the mistakes will make things break only rarely, so they can go
undetected easily. I'm okay with this patch only because you assure me
the whole thing is temporary: "# TODO Drop this class once we no longer
have the 'type'/'kind' mismatch".
> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
v8: use + instead of interpolation in a few places, rebase to simpler
.is_implicit(), use .c_name() more.
> v7: new patch, but borrows idea of subclass from v6 10/12, as
> well as c_name() from 7/12
> ---
> scripts/qapi-commands.py | 8 ++++----
> scripts/qapi-types.py | 12 +++++-------
> scripts/qapi-visit.py | 17 +++++------------
> scripts/qapi.py | 26 +++++++++++++++++++-------
> 4 files changed, 33 insertions(+), 30 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 43a893b..53a79ab 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -32,8 +32,8 @@ def gen_call(name, arg_type, ret_type):
> if arg_type:
> for memb in arg_type.members:
> if memb.optional:
> - argstr += 'has_%s, ' % c_name(memb.name)
> - argstr += '%s, ' % c_name(memb.name)
> + argstr += 'has_' + memb.c_name() + ', '
> + argstr += memb.c_name() + ', '
I like to use + instead of % in sufficiently simple cases myself. Not
sure this is one. See also change to gen_params() below.
>
> lhs = ''
> if ret_type:
> @@ -77,11 +77,11 @@ def gen_marshal_vars(arg_type, ret_type):
> ret += mcgen('''
> bool has_%(c_name)s = false;
> ''',
> - c_name=c_name(memb.name))
> + c_name=memb.c_name())
> ret += mcgen('''
> %(c_type)s %(c_name)s = %(c_null)s;
> ''',
> - c_name=c_name(memb.name),
> + c_name=memb.c_name(),
> c_type=memb.type.c_type(),
> c_null=memb.type.c_null())
> ret += '\n'
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 4fe618e..e37d529 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -136,9 +136,10 @@ struct %(c_name)s {
> ''')
> else:
> ret += mcgen('''
> - %(c_type)s kind;
> + %(c_type)s %(c_name)s;
> ''',
> - c_type=c_name(variants.tag_member.type.name))
> + c_type=variants.tag_member.type.c_name(),
> + c_name=variants.tag_member.c_name())
>
> # FIXME: What purpose does data serve, besides preventing a union that
> # has a branch named 'data'? We use it in qapi-visit.py to decide
> @@ -152,10 +153,7 @@ struct %(c_name)s {
> union { /* union tag is @%(c_name)s */
> void *data;
> ''',
> - # 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'))
> + c_name=variants.tag_member.c_name())
>
> for var in variants.variants:
> # Ugly special case for simple union TODO get rid of it
> @@ -164,7 +162,7 @@ struct %(c_name)s {
> %(c_type)s %(c_name)s;
> ''',
> c_type=typ.c_type(),
> - c_name=c_name(var.name))
> + c_name=var.c_name())
>
> ret += mcgen('''
> };
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 2a9fab8..cb251b2 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -196,7 +196,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
> case=c_enum_const(variants.tag_member.type.name,
> var.name),
> c_type=var.type.c_name(),
> - c_name=c_name(var.name))
> + c_name=var.c_name())
>
> ret += mcgen('''
> default:
> @@ -249,10 +249,6 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
> c_name=c_name(name))
> ret += gen_err_check(label='out_obj')
>
> - tag_key = variants.tag_member.name
> - if not variants.tag_name:
> - # 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) {
> @@ -264,11 +260,8 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
> switch ((*obj)->%(c_name)s) {
> ''',
> c_type=variants.tag_member.type.c_name(),
> - # 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'),
> - name=tag_key)
> + c_name=variants.tag_member.c_name(),
> + name=variants.tag_member.name)
>
> for var in variants.variants:
> # TODO ugly special case for simple union
> @@ -283,13 +276,13 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
> 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))
> + c_name=var.c_name())
> else:
> ret += mcgen('''
> visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
> ''',
> c_type=var.type.c_name(),
> - c_name=c_name(var.name))
> + c_name=var.c_name())
> ret += mcgen('''
> break;
> ''')
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 5b66264..80c026b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -975,7 +975,7 @@ class QAPISchemaObjectType(QAPISchemaType):
> members = []
> seen = {}
> for m in members:
> - assert c_name(m.name) not in seen
> + assert m.c_name() not in seen
> seen[m.name] = m
> for m in self.local_members:
> m.check(schema, members, seen)
> @@ -1022,6 +1022,18 @@ class QAPISchemaObjectTypeMember(object):
> all_members.append(self)
> seen[self.name] = self
>
> + def c_name(self):
> + return c_name(self.name)
> +
> +
> +# TODO Drop this class once we no longer have the 'type'/'kind' mismatch
> +class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
> + def c_name(self):
> + assert self.name == 'type'
> + assert isinstance(self.type, QAPISchemaEnumType)
> + assert self.type.is_implicit()
> + return 'kind'
> +
>
> class QAPISchemaObjectTypeVariants(object):
> def __init__(self, tag_name, tag_member, variants):
> @@ -1248,7 +1260,7 @@ class QAPISchema(object):
> def _make_implicit_tag(self, type_name, variants):
> typ = self._make_implicit_enum_type(type_name,
> [v.name for v in variants])
> - return QAPISchemaObjectTypeMember('type', typ, False)
> + return QAPISchemaObjectTypeUnionTag('type', typ, False)
>
> def _def_union_type(self, expr, info):
> name = expr['union']
> @@ -1555,8 +1567,8 @@ def gen_params(arg_type, extra):
> ret += sep
> sep = ', '
> if memb.optional:
> - ret += 'bool has_%s, ' % c_name(memb.name)
> - ret += '%s %s' % (memb.type.c_type(is_param=True), c_name(memb.name))
> + ret += 'bool has_' + memb.c_name() + sep
> + ret += '%s %s' % (memb.type.c_type(is_param=True), memb.c_name())
> if extra:
> ret += sep + extra
> return ret
New hunk in v8, to remain consistent with gen_call().
I doubt replacing literal ', ' by sep is making things clearer.
> @@ -1585,13 +1597,13 @@ def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
> ret += mcgen('''
> visit_optional(v, &%(prefix)shas_%(c_name)s, "%(name)s", %(errp)s);
> ''',
> - prefix=prefix, c_name=c_name(memb.name),
> + prefix=prefix, c_name=memb.c_name(),
> name=memb.name, errp=errparg)
> ret += gen_err_check(skiperr=skiperr)
> ret += mcgen('''
> if (%(prefix)shas_%(c_name)s) {
> ''',
> - prefix=prefix, c_name=c_name(memb.name))
> + prefix=prefix, c_name=memb.c_name())
> push_indent()
>
> # Ugly: sometimes we need to cast away const
> @@ -1604,7 +1616,7 @@ def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
> 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,
> + c_name=memb.c_name(), name=memb.name,
> errp=errparg)
> ret += gen_err_check(skiperr=skiperr)
New hunks in v8. Do you change from function c_name() to method
.c_name() Just for consistency, or is there a more serious reason?
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 10/18] qapi: Move union tag quirks into subclass
2015-10-13 12:10 ` Markus Armbruster
@ 2015-10-13 14:15 ` Eric Blake
2015-10-13 16:56 ` Markus Armbruster
0 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2015-10-13 14:15 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 4572 bytes --]
On 10/13/2015 06:10 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Right now, simple unions have a quirk of using 'kind' in the C
>> struct to match the QMP wire name 'type'. This has resulted in
>> messy clients each doing special cases. While we plan to
>> eventually rename things to match, it is better in the meantime
>> to consolidate the quirks into a special subclass, by adding a
>> new member.c_name() function. This will also make it easier
>> for reworking how alternate types are laid out in a future
>> patch. Use the new c_name() function where possible.
>
> Terminology: "the new c_name() method".
>
> I don't like having both function c_name() and method c_name(), because
> it's very easy to use the function when you should use the method, and
> the mistakes will make things break only rarely, so they can go
> undetected easily. I'm okay with this patch only because you assure me
> the whole thing is temporary: "# TODO Drop this class once we no longer
> have the 'type'/'kind' mismatch".
Hmm. Even after my type/kind fix is in, my local tree doesn't (yet)
remove uses of c_name(), because of its shorter typing. But you are
correct that once the rename is in and the temporary
QAPISchemaObjectTypeUnionTag is deleted, then there is no longer a
difference between member.c_name() and the longer c_name(member.name).
On the other hand, my patch subset C adds a member.c_type() function
which is required for use on simplified alternate layout, because it is
not always the same as member.type.c_type() or c_type(member.type.name).
As it is, we already have cases where c_name(entity.name) is not the
same as entity.c_name(), so we already have the confusion of when to use
the global function vs. the member function.
Is it worth renaming things so that the global function and member
functions have different names? If so, which one would be renamed, and
to what?
>
>> No change to generated code.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>
> v8: use + instead of interpolation in a few places, rebase to simpler
> .is_implicit(), use .c_name() more.
Whoops, forgot to 'git commit --amend' this one. Looks like you are
also viewing interdiffs, though, which makes me feel better about how
the series is progressing.
>> +++ b/scripts/qapi-commands.py
>> @@ -32,8 +32,8 @@ def gen_call(name, arg_type, ret_type):
>> if arg_type:
>> for memb in arg_type.members:
>> if memb.optional:
>> - argstr += 'has_%s, ' % c_name(memb.name)
>> - argstr += '%s, ' % c_name(memb.name)
>> + argstr += 'has_' + memb.c_name() + ', '
>> + argstr += memb.c_name() + ', '
>
> I like to use + instead of % in sufficiently simple cases myself. Not
> sure this is one. See also change to gen_params() below.
>> @@ -1555,8 +1567,8 @@ def gen_params(arg_type, extra):
>> ret += sep
>> sep = ', '
>> if memb.optional:
>> - ret += 'bool has_%s, ' % c_name(memb.name)
>> - ret += '%s %s' % (memb.type.c_type(is_param=True), c_name(memb.name))
>> + ret += 'bool has_' + memb.c_name() + sep
>> + ret += '%s %s' % (memb.type.c_type(is_param=True), memb.c_name())
>> if extra:
>> ret += sep + extra
>> return ret
>
> New hunk in v8, to remain consistent with gen_call().
>
> I doubt replacing literal ', ' by sep is making things clearer.
Fair enough - if there is reason for respin, I can undo the changes to
using '+'.
>> @@ -1604,7 +1616,7 @@ def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
>> 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,
>> + c_name=memb.c_name(), name=memb.name,
>> errp=errparg)
>> ret += gen_err_check(skiperr=skiperr)
>
> New hunks in v8. Do you change from function c_name() to method
> .c_name() Just for consistency, or is there a more serious reason?
It matters the most in qapi-type's gen_union(); everywhere else, it is
just for consistency and less typing.
What is the plan of attack on this one - will I need to respin a v9?
--
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] 51+ messages in thread
* Re: [Qemu-devel] [PATCH v8 10/18] qapi: Move union tag quirks into subclass
2015-10-13 14:15 ` Eric Blake
@ 2015-10-13 16:56 ` Markus Armbruster
0 siblings, 0 replies; 51+ messages in thread
From: Markus Armbruster @ 2015-10-13 16:56 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 10/13/2015 06:10 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> Right now, simple unions have a quirk of using 'kind' in the C
>>> struct to match the QMP wire name 'type'. This has resulted in
>>> messy clients each doing special cases. While we plan to
>>> eventually rename things to match, it is better in the meantime
>>> to consolidate the quirks into a special subclass, by adding a
>>> new member.c_name() function. This will also make it easier
>>> for reworking how alternate types are laid out in a future
>>> patch. Use the new c_name() function where possible.
>>
>> Terminology: "the new c_name() method".
>>
>> I don't like having both function c_name() and method c_name(), because
>> it's very easy to use the function when you should use the method, and
>> the mistakes will make things break only rarely, so they can go
>> undetected easily. I'm okay with this patch only because you assure me
>> the whole thing is temporary: "# TODO Drop this class once we no longer
>> have the 'type'/'kind' mismatch".
>
> Hmm. Even after my type/kind fix is in, my local tree doesn't (yet)
> remove uses of c_name(), because of its shorter typing. But you are
> correct that once the rename is in and the temporary
> QAPISchemaObjectTypeUnionTag is deleted, then there is no longer a
> difference between member.c_name() and the longer c_name(member.name).
>
> On the other hand, my patch subset C adds a member.c_type() function
> which is required for use on simplified alternate layout, because it is
> not always the same as member.type.c_type() or c_type(member.type.name).
Can't say how I like it until I reviewed it :)
> As it is, we already have cases where c_name(entity.name) is not the
> same as entity.c_name(), so we already have the confusion of when to use
> the global function vs. the member function.
They are:
* QAPISchemaBuiltinType.c_name() returns its argument instead
* QAPISchemaObjectType.c_name() additionally asserts "not implicit".
* QAPISchemaObjectTypeUnionTag.c_name() returns 'kind' instead, but
that'll go away.
Anything else?
> Is it worth renaming things so that the global function and member
> functions have different names? If so, which one would be renamed, and
> to what?
Renaming one of them can perhaps make misuse of the function stand out a
bit more.
The only way I can see to keep obvious use of the interface correct is
getting rid of the function entirely. Involves passing objects instead
of names around, then calling the objects' method instead of passing the
name to the function. Can't say whether a suitable object always exists
without trying it.
>>> No change to generated code.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>>
>>> ---
>>
>> v8: use + instead of interpolation in a few places, rebase to simpler
>> .is_implicit(), use .c_name() more.
>
> Whoops, forgot to 'git commit --amend' this one. Looks like you are
> also viewing interdiffs, though, which makes me feel better about how
> the series is progressing.
When I expect only small and scattered change, I like to feed patches to
ediff :)
>>> +++ b/scripts/qapi-commands.py
>>> @@ -32,8 +32,8 @@ def gen_call(name, arg_type, ret_type):
>>> if arg_type:
>>> for memb in arg_type.members:
>>> if memb.optional:
>>> - argstr += 'has_%s, ' % c_name(memb.name)
>>> - argstr += '%s, ' % c_name(memb.name)
>>> + argstr += 'has_' + memb.c_name() + ', '
>>> + argstr += memb.c_name() + ', '
>>
>> I like to use + instead of % in sufficiently simple cases myself. Not
>> sure this is one. See also change to gen_params() below.
>
>>> @@ -1555,8 +1567,8 @@ def gen_params(arg_type, extra):
>>> ret += sep
>>> sep = ', '
>>> if memb.optional:
>>> - ret += 'bool has_%s, ' % c_name(memb.name)
>>> - ret += '%s %s' % (memb.type.c_type(is_param=True), c_name(memb.name))
>>> + ret += 'bool has_' + memb.c_name() + sep
>>> + ret += '%s %s' % (memb.type.c_type(is_param=True), memb.c_name())
>>> if extra:
>>> ret += sep + extra
>>> return ret
>>
>> New hunk in v8, to remain consistent with gen_call().
>>
>> I doubt replacing literal ', ' by sep is making things clearer.
>
> Fair enough - if there is reason for respin, I can undo the changes to
> using '+'.
>
>>> @@ -1604,7 +1616,7 @@ def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
>>> 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,
>>> + c_name=memb.c_name(), name=memb.name,
>>> errp=errparg)
>>> ret += gen_err_check(skiperr=skiperr)
>>
>> New hunks in v8. Do you change from function c_name() to method
>> .c_name() Just for consistency, or is there a more serious reason?
>
> It matters the most in qapi-type's gen_union(); everywhere else, it is
> just for consistency and less typing.
>
> What is the plan of attack on this one - will I need to respin a v9?
I'll answer this in my reply to PATCH 15.
^ permalink raw reply [flat|nested] 51+ messages in thread
* [Qemu-devel] [PATCH v8 11/18] qapi: Simplify gen_struct_field()
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
` (9 preceding siblings ...)
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 10/18] qapi: Move union tag quirks into subclass Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-13 12:12 ` Markus Armbruster
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 12/18] qapi: Track location that created an implicit type Eric Blake
` (8 subsequent siblings)
19 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Rather than having all callers pass a name, type, and optional
flag, have them instead pass a QAPISchemaObjectTypeMember which
already has all that information.
This will allow a future patch to simplify alternates to use
a special tag 'qtype_code type'. In the meantime, it requires
a hack to create a temporary member 'base' for struct base
classes; this temporary member will go away in a later patch
that flattens structs in the same way that flat union base
classes were flattened in commit 1e6c1616.
No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: new patch
---
scripts/qapi-types.py | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index e37d529..5ffabf5 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -36,18 +36,18 @@ struct %(c_name)s {
c_name=c_name(name), c_type=element_type.c_type())
-def gen_struct_field(name, typ, optional):
+def gen_struct_field(member):
ret = ''
- if optional:
+ if member.optional:
ret += mcgen('''
bool has_%(c_name)s;
''',
- c_name=c_name(name))
+ c_name=member.c_name())
ret += mcgen('''
%(c_type)s %(c_name)s;
''',
- c_type=typ.c_type(), c_name=c_name(name))
+ c_type=member.type.c_type(), c_name=member.c_name())
return ret
@@ -55,7 +55,7 @@ def gen_struct_fields(members):
ret = ''
for memb in members:
- ret += gen_struct_field(memb.name, memb.type, memb.optional)
+ ret += gen_struct_field(memb)
return ret
@@ -67,7 +67,11 @@ struct %(c_name)s {
c_name=c_name(name))
if base:
- ret += gen_struct_field('base', base, False)
+ # TODO Flatten this struct, similar to flat union bases. Until
+ # then, hack around it with a temporary member.
+ member = QAPISchemaObjectTypeMember('base', base.name, False)
+ member.type = base
+ ret += gen_struct_field(member)
ret += gen_struct_fields(members)
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 11/18] qapi: Simplify gen_struct_field()
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 11/18] qapi: Simplify gen_struct_field() Eric Blake
@ 2015-10-13 12:12 ` Markus Armbruster
2015-10-13 13:11 ` Eric Blake
0 siblings, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2015-10-13 12:12 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Rather than having all callers pass a name, type, and optional
> flag, have them instead pass a QAPISchemaObjectTypeMember which
> already has all that information.
>
> This will allow a future patch to simplify alternates to use
> a special tag 'qtype_code type'. In the meantime, it requires
> a hack to create a temporary member 'base' for struct base
> classes; this temporary member will go away in a later patch
> that flattens structs in the same way that flat union base
> classes were flattened in commit 1e6c1616.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v8: new patch
> ---
> scripts/qapi-types.py | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index e37d529..5ffabf5 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -36,18 +36,18 @@ struct %(c_name)s {
> c_name=c_name(name), c_type=element_type.c_type())
>
>
> -def gen_struct_field(name, typ, optional):
> +def gen_struct_field(member):
> ret = ''
>
> - if optional:
> + if member.optional:
> ret += mcgen('''
> bool has_%(c_name)s;
> ''',
> - c_name=c_name(name))
> + c_name=member.c_name())
> ret += mcgen('''
> %(c_type)s %(c_name)s;
> ''',
> - c_type=typ.c_type(), c_name=c_name(name))
> + c_type=member.type.c_type(), c_name=member.c_name())
> return ret
>
>
> @@ -55,7 +55,7 @@ def gen_struct_fields(members):
> ret = ''
>
> for memb in members:
> - ret += gen_struct_field(memb.name, memb.type, memb.optional)
> + ret += gen_struct_field(memb)
> return ret
>
>
> @@ -67,7 +67,11 @@ struct %(c_name)s {
> c_name=c_name(name))
>
> if base:
> - ret += gen_struct_field('base', base, False)
> + # TODO Flatten this struct, similar to flat union bases. Until
> + # then, hack around it with a temporary member.
> + member = QAPISchemaObjectTypeMember('base', base.name, False)
> + member.type = base
> + ret += gen_struct_field(member)
>
> ret += gen_struct_fields(members)
Uff! Are you really, really sure this is the right spot in the series
for this transformation?
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 11/18] qapi: Simplify gen_struct_field()
2015-10-13 12:12 ` Markus Armbruster
@ 2015-10-13 13:11 ` Eric Blake
0 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2015-10-13 13:11 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1828 bytes --]
On 10/13/2015 06:12 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Rather than having all callers pass a name, type, and optional
>> flag, have them instead pass a QAPISchemaObjectTypeMember which
>> already has all that information.
>>
>> This will allow a future patch to simplify alternates to use
>> a special tag 'qtype_code type'. In the meantime, it requires
>> a hack to create a temporary member 'base' for struct base
>> classes; this temporary member will go away in a later patch
>> that flattens structs in the same way that flat union base
>> classes were flattened in commit 1e6c1616.
>>
>> No change to generated code.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>>
>> if base:
>> - ret += gen_struct_field('base', base, False)
>> + # TODO Flatten this struct, similar to flat union bases. Until
>> + # then, hack around it with a temporary member.
>> + member = QAPISchemaObjectTypeMember('base', base.name, False)
>> + member.type = base
>> + ret += gen_struct_field(member)
>>
>> ret += gen_struct_fields(members)
>
> Uff! Are you really, really sure this is the right spot in the series
> for this transformation?
Serves me right for sending the series late at night. I can probably
delay this transformation to a later subset, after I have unboxed struct
base members, so I won't need this hack. And I just confirmed that
nothing else in subset B depends on this patch, so let's defer it to
subset C or later (dropping from this series is not sufficient reason
for a v9 respin, but if something else turns up that needs a v9, this
patch has been moved out).
--
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] 51+ messages in thread
* [Qemu-devel] [PATCH v8 12/18] qapi: Track location that created an implicit type
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
` (10 preceding siblings ...)
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 11/18] qapi: Simplify gen_struct_field() Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-13 12:19 ` Markus Armbruster
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 13/18] qapi: Track owner of each object member Eric Blake
` (7 subsequent siblings)
19 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
A future patch will move some error checking from the parser
to the various QAPISchema*.check() methods, which run only
after parsing completes. It will thus be possible to create
a python instance representing an implicit QAPI type that
parses fine but will fail validation during check(). Since
all errors have to have an associated 'info' location, we
need a location to be associated with those implicit types.
The intuitive info to use is the location of the enclosing
entity that caused the creation of the implicit type; similar
to what was done for array types in an earlier patch.
Note that we do not anticipate builtin types being used in
an error message (as they are not part of the user's QAPI
input, the user can't cause a semantic error in their
behavior), so we exempt those types from requiring info, by
setting a flag to track the completion of _def_predefineds(),
and tracking that flag in _def_entity().
No change to the generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: rebase to earlier changes, improve comment, rework predefined
flag name
v7: better commit message and comments, fix info assertion to
use instance flag rather than ugly leaky abstraction static flag
v6: improve commit message, track implicit enum info, rebase
on new lazy array handling
---
scripts/qapi.py | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 80c026b..c9ce9ee 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -793,9 +793,9 @@ class QAPISchemaEntity(object):
# For explicitly defined entities, info points to the (explicit)
# definition. For builtins (and their arrays), info is None.
# For other arrays, info points to an explicit place that uses
- # the array (there may be more than one such place).
- # TODO For other implicitly defined entities, it should point to
- # a place that triggers implicit definition.
+ # the array (there may be more than one such place). For other
+ # implicitly defined entities, it points to the place that
+ # triggered the implicit definition.
self.info = info
def c_name(self):
@@ -1146,7 +1146,9 @@ class QAPISchema(object):
try:
self.exprs = check_exprs(QAPISchemaParser(open(fname, "r")).exprs)
self._entity_dict = {}
+ self._predefining = True
self._def_predefineds()
+ self._predefining = False
self._def_exprs()
self.check()
except (QAPISchemaError, QAPIExprError), err:
@@ -1154,6 +1156,8 @@ class QAPISchema(object):
exit(1)
def _def_entity(self, ent):
+ # Only the predefined types are allowed to not have info
+ assert ent.info or self._predefining
assert ent.name not in self._entity_dict
self._entity_dict[ent.name] = ent
@@ -1196,9 +1200,9 @@ class QAPISchema(object):
[], None)
self._def_entity(self.the_empty_object_type)
- def _make_implicit_enum_type(self, name, values):
+ def _make_implicit_enum_type(self, name, info, values):
name = name + 'Kind' # Use namespace reserved by add_name()
- self._def_entity(QAPISchemaEnumType(name, None, values, None))
+ self._def_entity(QAPISchemaEnumType(name, info, values, None))
return name
def _make_array_type(self, element_type, info):
@@ -1209,12 +1213,12 @@ class QAPISchema(object):
self._def_entity(QAPISchemaArrayType(name, info, element_type))
return name
- def _make_implicit_object_type(self, name, role, members):
+ def _make_implicit_object_type(self, name, info, role, members):
if not members:
return None
name = ':obj-%s-%s' % (name, role)
if not self.lookup_entity(name, QAPISchemaObjectType):
- self._def_entity(QAPISchemaObjectType(name, None, None,
+ self._def_entity(QAPISchemaObjectType(name, info, None,
members, None))
return name
@@ -1254,11 +1258,11 @@ class QAPISchema(object):
assert len(typ) == 1
typ = self._make_array_type(typ[0], info)
typ = self._make_implicit_object_type(
- typ, 'wrapper', [self._make_member('data', typ, info)])
+ typ, info, 'wrapper', [self._make_member('data', typ, info)])
return QAPISchemaObjectTypeVariant(case, typ)
- def _make_implicit_tag(self, type_name, variants):
- typ = self._make_implicit_enum_type(type_name,
+ def _make_implicit_tag(self, type_name, info, variants):
+ typ = self._make_implicit_enum_type(type_name, info,
[v.name for v in variants])
return QAPISchemaObjectTypeUnionTag('type', typ, False)
@@ -1274,7 +1278,7 @@ class QAPISchema(object):
else:
variants = [self._make_simple_variant(key, value, info)
for (key, value) in data.iteritems()]
- tag_member = self._make_implicit_tag(name, variants)
+ tag_member = self._make_implicit_tag(name, info, variants)
self._def_entity(
QAPISchemaObjectType(name, info, base,
self._make_members(OrderedDict(), info),
@@ -1287,7 +1291,7 @@ class QAPISchema(object):
data = expr['data']
variants = [self._make_variant(key, value)
for (key, value) in data.iteritems()]
- tag_member = self._make_implicit_tag(name, variants)
+ tag_member = self._make_implicit_tag(name, info, variants)
self._def_entity(
QAPISchemaAlternateType(name, info,
QAPISchemaObjectTypeVariants(None,
@@ -1302,7 +1306,7 @@ class QAPISchema(object):
success_response = expr.get('success-response', True)
if isinstance(data, OrderedDict):
data = self._make_implicit_object_type(
- name, 'arg', self._make_members(data, info))
+ name, info, 'arg', self._make_members(data, info))
if isinstance(rets, list):
assert len(rets) == 1
rets = self._make_array_type(rets[0], info)
@@ -1314,7 +1318,7 @@ class QAPISchema(object):
data = expr.get('data')
if isinstance(data, OrderedDict):
data = self._make_implicit_object_type(
- name, 'arg', self._make_members(data, info))
+ name, info, 'arg', self._make_members(data, info))
self._def_entity(QAPISchemaEvent(name, info, data))
def _def_exprs(self):
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 12/18] qapi: Track location that created an implicit type
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 12/18] qapi: Track location that created an implicit type Eric Blake
@ 2015-10-13 12:19 ` Markus Armbruster
2015-10-13 14:27 ` Eric Blake
0 siblings, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2015-10-13 12:19 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> A future patch will move some error checking from the parser
> to the various QAPISchema*.check() methods, which run only
> after parsing completes. It will thus be possible to create
> a python instance representing an implicit QAPI type that
> parses fine but will fail validation during check(). Since
> all errors have to have an associated 'info' location, we
> need a location to be associated with those implicit types.
> The intuitive info to use is the location of the enclosing
> entity that caused the creation of the implicit type; similar
> to what was done for array types in an earlier patch.
>
> Note that we do not anticipate builtin types being used in
> an error message (as they are not part of the user's QAPI
> input, the user can't cause a semantic error in their
> behavior), so we exempt those types from requiring info, by
> setting a flag to track the completion of _def_predefineds(),
> and tracking that flag in _def_entity().
>
> No change to the generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v8: rebase to earlier changes, improve comment, rework predefined
> flag name
> v7: better commit message and comments, fix info assertion to
> use instance flag rather than ugly leaky abstraction static flag
> v6: improve commit message, track implicit enum info, rebase
> on new lazy array handling
> ---
> scripts/qapi.py | 32 ++++++++++++++++++--------------
> 1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 80c026b..c9ce9ee 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -793,9 +793,9 @@ class QAPISchemaEntity(object):
> # For explicitly defined entities, info points to the (explicit)
> # definition. For builtins (and their arrays), info is None.
> # For other arrays, info points to an explicit place that uses
> - # the array (there may be more than one such place).
> - # TODO For other implicitly defined entities, it should point to
> - # a place that triggers implicit definition.
> + # the array (there may be more than one such place). For other
> + # implicitly defined entities, it points to the place that
> + # triggered the implicit definition.
How does info for implicitly defined arrays differ from info for other
implicitly defined entities? I suspect it doesn't, and this comment
should become simpler:
# For explicitly defined entities, info points to the (explicit)
# definition. For builtins (and their arrays), info is None.
# For implicitly defined entities, info points to a place that
# triggered the implicit definition (there may be more than one
# such place).
> self.info = info
>
> def c_name(self):
[...]
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 12/18] qapi: Track location that created an implicit type
2015-10-13 12:19 ` Markus Armbruster
@ 2015-10-13 14:27 ` Eric Blake
0 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2015-10-13 14:27 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2082 bytes --]
On 10/13/2015 06:19 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> A future patch will move some error checking from the parser
>> to the various QAPISchema*.check() methods, which run only
>> after parsing completes. It will thus be possible to create
>> a python instance representing an implicit QAPI type that
>> parses fine but will fail validation during check(). Since
>> all errors have to have an associated 'info' location, we
>> need a location to be associated with those implicit types.
>> The intuitive info to use is the location of the enclosing
>> entity that caused the creation of the implicit type; similar
>> to what was done for array types in an earlier patch.
>>
>> +++ b/scripts/qapi.py
>> @@ -793,9 +793,9 @@ class QAPISchemaEntity(object):
>> # For explicitly defined entities, info points to the (explicit)
>> # definition. For builtins (and their arrays), info is None.
>> # For other arrays, info points to an explicit place that uses
>> - # the array (there may be more than one such place).
>> - # TODO For other implicitly defined entities, it should point to
>> - # a place that triggers implicit definition.
>> + # the array (there may be more than one such place). For other
>> + # implicitly defined entities, it points to the place that
>> + # triggered the implicit definition.
>
> How does info for implicitly defined arrays differ from info for other
> implicitly defined entities? I suspect it doesn't, and this comment
> should become simpler:
>
> # For explicitly defined entities, info points to the (explicit)
> # definition. For builtins (and their arrays), info is None.
> # For implicitly defined entities, info points to a place that
> # triggered the implicit definition (there may be more than one
> # such place).
>
Works for me.
--
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] 51+ messages in thread
* [Qemu-devel] [PATCH v8 13/18] qapi: Track owner of each object member
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
` (11 preceding siblings ...)
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 12/18] qapi: Track location that created an implicit type Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-13 13:14 ` Markus Armbruster
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 14/18] qapi: Detect collisions in C member names Eric Blake
` (6 subsequent siblings)
19 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Future commits will migrate semantic checking away from parsing
and over to the various QAPISchema*.check() methods. But to
report an error message about an incorrect semantic use of a
member of an object type, it helps to know which type, command,
or event owns the member. In particular, when a member is
inherited from a base type, it is desirable to associate the
member name with the base type (and not the type calling
member.check()).
Rather than packing additional information into the seen array
passed to each member.check() (as in seen[m.name] = {'member':m,
'owner':type}), it is easier to have each member track the name
of the owner type in the first place (keeping things simpler
with the existing seen[m.name] = m). The new member.owner field
is set via a new set_owner() function, called when registering
the members and variants arrays with an object or variant type.
Track only a name, and not the actual type object, to avoid
creating a circular python reference chain.
The source information is intended for human consumption in
error messages, and a new describe() method is added to access
the resulting information. For example, given the qapi:
{ 'command': 'foo', 'data': { 'string': 'str' } }
an implementation of visit_command() that calls
arg_type.members[0].describe()
will see "'string' (argument of foo)".
To make the human-readable name of implicit types work without
duplicating efforts, the describe() method has to reverse the
name of implicit types.
No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: don't munge implicit type names [except for event data], and
instead make describe() create nicer messages. Add set_owner(), and
use field 'role' instead of method _describe()
v7: total rewrite: rework implicit object names, assign owner
when initializing owner type rather than when creating member
python object
v6: rebase on new lazy array creation and simple union 'type'
motion; tweak commit message
---
scripts/qapi.py | 48 +++++++++++++++++++++++++++++++---
tests/qapi-schema/qapi-schema-test.out | 8 +++---
2 files changed, 49 insertions(+), 7 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index c9ce9ee..8b29e11 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -952,8 +952,10 @@ class QAPISchemaObjectType(QAPISchemaType):
assert base is None or isinstance(base, str)
for m in local_members:
assert isinstance(m, QAPISchemaObjectTypeMember)
- assert (variants is None or
- isinstance(variants, QAPISchemaObjectTypeVariants))
+ m.set_owner(name)
+ if variants is not None:
+ assert isinstance(variants, QAPISchemaObjectTypeVariants)
+ variants.set_owner(name)
self._base_name = base
self.base = None
self.local_members = local_members
@@ -1014,8 +1016,14 @@ class QAPISchemaObjectTypeMember(object):
self._type_name = typ
self.type = None
self.optional = optional
+ self.owner = None
+
+ def set_owner(self, name):
+ assert not self.owner
+ self.owner = name
def check(self, schema, all_members, seen):
+ assert self.owner
assert self.name not in seen
self.type = schema.lookup_type(self._type_name)
assert self.type
@@ -1025,6 +1033,25 @@ class QAPISchemaObjectTypeMember(object):
def c_name(self):
return c_name(self.name)
+ def describe(self):
+ owner = self.owner
+ # See QAPISchema._make_implicit_object_type() - reverse the
+ # mapping there to create a nice human-readable description
+ if owner.startswith(':obj-'):
+ owner = owner[5:]
+ if owner.endswith('-arg'):
+ source = '(argument of %s)' % owner[:4]
+ elif owner.endswith('-data'):
+ source = '(data of %s)' % owner[:5]
+ else:
+ assert owner.endswith('-wrapper')
+ source = '(branch of %s)' % owner[:8]
+ else:
+ source = '(%s of %s)' % (self.role, owner)
+ return "'%s' %s" % (self.name, source)
+
+ role = 'member'
+
# TODO Drop this class once we no longer have the 'type'/'kind' mismatch
class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
@@ -1034,6 +1061,11 @@ class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
assert self.type.is_implicit()
return 'kind'
+ def describe(self):
+ # Must override superclass describe() because self.name is 'type'
+ assert self.owner[0] != ':'
+ return "'kind' (implicit tag of %s)" % self.owner
+
class QAPISchemaObjectTypeVariants(object):
def __init__(self, tag_name, tag_member, variants):
@@ -1050,6 +1082,12 @@ class QAPISchemaObjectTypeVariants(object):
self.tag_member = tag_member
self.variants = variants
+ def set_owner(self, name):
+ if self.tag_member:
+ self.tag_member.set_owner(name)
+ for v in self.variants:
+ v.set_owner(name)
+
def check(self, schema, members, seen):
if self.tag_name:
self.tag_member = seen[self.tag_name]
@@ -1079,12 +1117,15 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
return self.type.members[0].type
return None
+ role = 'branch'
+
class QAPISchemaAlternateType(QAPISchemaType):
def __init__(self, name, info, variants):
QAPISchemaType.__init__(self, name, info)
assert isinstance(variants, QAPISchemaObjectTypeVariants)
assert not variants.tag_name
+ variants.set_owner(name)
self.variants = variants
def check(self, schema):
@@ -1216,6 +1257,7 @@ class QAPISchema(object):
def _make_implicit_object_type(self, name, info, role, members):
if not members:
return None
+ # See also QAPISchemaObjectTypeMember.describe()
name = ':obj-%s-%s' % (name, role)
if not self.lookup_entity(name, QAPISchemaObjectType):
self._def_entity(QAPISchemaObjectType(name, info, None,
@@ -1318,7 +1360,7 @@ class QAPISchema(object):
data = expr.get('data')
if isinstance(data, OrderedDict):
data = self._make_implicit_object_type(
- name, info, 'arg', self._make_members(data, info))
+ name, info, 'data', self._make_members(data, info))
self._def_entity(QAPISchemaEvent(name, info, data))
def _def_exprs(self):
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index a6c80e0..d837475 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -1,9 +1,9 @@
object :empty
-object :obj-EVENT_C-arg
+object :obj-EVENT_C-data
member a: int optional=True
member b: UserDefOne optional=True
member c: str optional=False
-object :obj-EVENT_D-arg
+object :obj-EVENT_D-data
member a: EventStructOne optional=False
member b: str optional=False
member c: str optional=True
@@ -79,8 +79,8 @@ alternate AltStrNum
enum AltStrNumKind ['s', 'n']
event EVENT_A None
event EVENT_B None
-event EVENT_C :obj-EVENT_C-arg
-event EVENT_D :obj-EVENT_D-arg
+event EVENT_C :obj-EVENT_C-data
+event EVENT_D :obj-EVENT_D-data
enum EnumOne ['value1', 'value2', 'value3']
object EventStructOne
member struct1: UserDefOne optional=False
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 13/18] qapi: Track owner of each object member
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 13/18] qapi: Track owner of each object member Eric Blake
@ 2015-10-13 13:14 ` Markus Armbruster
2015-10-13 14:38 ` Eric Blake
0 siblings, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2015-10-13 13:14 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Future commits will migrate semantic checking away from parsing
> and over to the various QAPISchema*.check() methods. But to
> report an error message about an incorrect semantic use of a
> member of an object type, it helps to know which type, command,
> or event owns the member. In particular, when a member is
> inherited from a base type, it is desirable to associate the
> member name with the base type (and not the type calling
> member.check()).
>
> Rather than packing additional information into the seen array
> passed to each member.check() (as in seen[m.name] = {'member':m,
> 'owner':type}), it is easier to have each member track the name
> of the owner type in the first place (keeping things simpler
> with the existing seen[m.name] = m). The new member.owner field
> is set via a new set_owner() function, called when registering
method
> the members and variants arrays with an object or variant type.
> Track only a name, and not the actual type object, to avoid
> creating a circular python reference chain.
>
> The source information is intended for human consumption in
> error messages, and a new describe() method is added to access
> the resulting information. For example, given the qapi:
> { 'command': 'foo', 'data': { 'string': 'str' } }
> an implementation of visit_command() that calls
> arg_type.members[0].describe()
> will see "'string' (argument of foo)".
>
> To make the human-readable name of implicit types work without
> duplicating efforts, the describe() method has to reverse the
> name of implicit types.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v8: don't munge implicit type names [except for event data], and
> instead make describe() create nicer messages. Add set_owner(), and
> use field 'role' instead of method _describe()
> v7: total rewrite: rework implicit object names, assign owner
> when initializing owner type rather than when creating member
> python object
> v6: rebase on new lazy array creation and simple union 'type'
> motion; tweak commit message
> ---
> scripts/qapi.py | 48 +++++++++++++++++++++++++++++++---
> tests/qapi-schema/qapi-schema-test.out | 8 +++---
> 2 files changed, 49 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index c9ce9ee..8b29e11 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -952,8 +952,10 @@ class QAPISchemaObjectType(QAPISchemaType):
> assert base is None or isinstance(base, str)
> for m in local_members:
> assert isinstance(m, QAPISchemaObjectTypeMember)
> - assert (variants is None or
> - isinstance(variants, QAPISchemaObjectTypeVariants))
> + m.set_owner(name)
> + if variants is not None:
> + assert isinstance(variants, QAPISchemaObjectTypeVariants)
> + variants.set_owner(name)
> self._base_name = base
> self.base = None
> self.local_members = local_members
> @@ -1014,8 +1016,14 @@ class QAPISchemaObjectTypeMember(object):
> self._type_name = typ
> self.type = None
> self.optional = optional
> + self.owner = None
> +
> + def set_owner(self, name):
> + assert not self.owner
> + self.owner = name
>
> def check(self, schema, all_members, seen):
> + assert self.owner
> assert self.name not in seen
> self.type = schema.lookup_type(self._type_name)
> assert self.type
> @@ -1025,6 +1033,25 @@ class QAPISchemaObjectTypeMember(object):
> def c_name(self):
> return c_name(self.name)
>
> + def describe(self):
> + owner = self.owner
> + # See QAPISchema._make_implicit_object_type() - reverse the
> + # mapping there to create a nice human-readable description
> + if owner.startswith(':obj-'):
> + owner = owner[5:]
> + if owner.endswith('-arg'):
> + source = '(argument of %s)' % owner[:4]
> + elif owner.endswith('-data'):
> + source = '(data of %s)' % owner[:5]
> + else:
> + assert owner.endswith('-wrapper')
> + source = '(branch of %s)' % owner[:8]
> + else:
> + source = '(%s of %s)' % (self.role, owner)
> + return "'%s' %s" % (self.name, source)
Perhaps not exactly pretty, but it gets the job done with minimal fuss.
Sold.
> +
> + role = 'member'
> +
>
Class variables are usually defined first, before methods.
> # TODO Drop this class once we no longer have the 'type'/'kind' mismatch
> class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
> @@ -1034,6 +1061,11 @@ class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
> assert self.type.is_implicit()
> return 'kind'
>
> + def describe(self):
> + # Must override superclass describe() because self.name is 'type'
I don't find this argument convincing. I think 'kind' is exactly as
unhelpful as 'type' is. Specifically, should we report a QMP clash,
calling it 'kind' is confusing (it actually clashes with 'type').
Conversely 'type' is confusing when we report a C clash.
The helpful part...
> + assert self.owner[0] != ':'
> + return "'kind' (implicit tag of %s)" % self.owner
> +
... is (implicit tag of FOO).
Fortunately, you're working towards killing the kind vs. type confusion.
Suggest to either rephrase the comment, or simply drop it.
>
> class QAPISchemaObjectTypeVariants(object):
> def __init__(self, tag_name, tag_member, variants):
> @@ -1050,6 +1082,12 @@ class QAPISchemaObjectTypeVariants(object):
> self.tag_member = tag_member
> self.variants = variants
>
> + def set_owner(self, name):
> + if self.tag_member:
> + self.tag_member.set_owner(name)
> + for v in self.variants:
> + v.set_owner(name)
> +
> def check(self, schema, members, seen):
> if self.tag_name:
> self.tag_member = seen[self.tag_name]
> @@ -1079,12 +1117,15 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
> return self.type.members[0].type
> return None
>
> + role = 'branch'
> +
>
Define this first in the class.
> class QAPISchemaAlternateType(QAPISchemaType):
> def __init__(self, name, info, variants):
> QAPISchemaType.__init__(self, name, info)
> assert isinstance(variants, QAPISchemaObjectTypeVariants)
> assert not variants.tag_name
> + variants.set_owner(name)
> self.variants = variants
>
> def check(self, schema):
> @@ -1216,6 +1257,7 @@ class QAPISchema(object):
> def _make_implicit_object_type(self, name, info, role, members):
> if not members:
> return None
> + # See also QAPISchemaObjectTypeMember.describe()
> name = ':obj-%s-%s' % (name, role)
> if not self.lookup_entity(name, QAPISchemaObjectType):
> self._def_entity(QAPISchemaObjectType(name, info, None,
> @@ -1318,7 +1360,7 @@ class QAPISchema(object):
> data = expr.get('data')
> if isinstance(data, OrderedDict):
> data = self._make_implicit_object_type(
> - name, info, 'arg', self._make_members(data, info))
> + name, info, 'data', self._make_members(data, info))
> self._def_entity(QAPISchemaEvent(name, info, data))
>
> def _def_exprs(self):
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index a6c80e0..d837475 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -1,9 +1,9 @@
> object :empty
> -object :obj-EVENT_C-arg
> +object :obj-EVENT_C-data
> member a: int optional=True
> member b: UserDefOne optional=True
> member c: str optional=False
> -object :obj-EVENT_D-arg
> +object :obj-EVENT_D-data
> member a: EventStructOne optional=False
> member b: str optional=False
> member c: str optional=True
> @@ -79,8 +79,8 @@ alternate AltStrNum
> enum AltStrNumKind ['s', 'n']
> event EVENT_A None
> event EVENT_B None
> -event EVENT_C :obj-EVENT_C-arg
> -event EVENT_D :obj-EVENT_D-arg
> +event EVENT_C :obj-EVENT_C-data
> +event EVENT_D :obj-EVENT_D-data
> enum EnumOne ['value1', 'value2', 'value3']
> object EventStructOne
> member struct1: UserDefOne optional=False
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 13/18] qapi: Track owner of each object member
2015-10-13 13:14 ` Markus Armbruster
@ 2015-10-13 14:38 ` Eric Blake
2015-10-13 16:30 ` Markus Armbruster
0 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2015-10-13 14:38 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 4230 bytes --]
On 10/13/2015 07:14 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Future commits will migrate semantic checking away from parsing
>> and over to the various QAPISchema*.check() methods. But to
>> report an error message about an incorrect semantic use of a
>> member of an object type, it helps to know which type, command,
>> or event owns the member. In particular, when a member is
>> inherited from a base type, it is desirable to associate the
>> member name with the base type (and not the type calling
>> member.check()).
>>
>>
>> + def describe(self):
>> + owner = self.owner
>> + # See QAPISchema._make_implicit_object_type() - reverse the
>> + # mapping there to create a nice human-readable description
>> + if owner.startswith(':obj-'):
>> + owner = owner[5:]
>> + if owner.endswith('-arg'):
>> + source = '(argument of %s)' % owner[:4]
>> + elif owner.endswith('-data'):
>> + source = '(data of %s)' % owner[:5]
>> + else:
>> + assert owner.endswith('-wrapper')
>> + source = '(branch of %s)' % owner[:8]
>> + else:
>> + source = '(%s of %s)' % (self.role, owner)
>> + return "'%s' %s" % (self.name, source)
>
> Perhaps not exactly pretty, but it gets the job done with minimal fuss.
> Sold.
Ouch - I think these should be owner[:-4] (and similar) - I want the
slice that excludes the last four bytes, and not one that is four bytes
long. (It happened to work in my tests because I was testing on
:obj-oops-arg, where owner[:4] and owner[:-4] gave the same 'oops' string).
It's an easy fixup to squash in, so I'm not yet sure if I need a full v9
respin.
>
>> +
>> + role = 'member'
>> +
>>
>
> Class variables are usually defined first, before methods.
Sure, can do. pep8 didn't complain, and I didn't check if pylint warns
about it.
>
>> # TODO Drop this class once we no longer have the 'type'/'kind' mismatch
>> class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
>> @@ -1034,6 +1061,11 @@ class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
>> assert self.type.is_implicit()
>> return 'kind'
>>
>> + def describe(self):
>> + # Must override superclass describe() because self.name is 'type'
>
> I don't find this argument convincing. I think 'kind' is exactly as
> unhelpful as 'type' is. Specifically, should we report a QMP clash,
> calling it 'kind' is confusing (it actually clashes with 'type').
> Conversely 'type' is confusing when we report a C clash.
>
> The helpful part...
>
>> + assert self.owner[0] != ':'
>> + return "'kind' (implicit tag of %s)" % self.owner
>> +
>
> ... is (implicit tag of FOO).
>
> Fortunately, you're working towards killing the kind vs. type confusion.
> Suggest to either rephrase the comment, or simply drop it.
Drop the comment is fine by me.
I personally thought the message needed to report 'kind', because 16/18
ends up reporting:
+tests/qapi-schema/union-clash-type.json:8: 'kind' (branch of TestUnion)
collides with 'kind' (implicit tag of TestUnion)
If I didn't override describe() at all, it would report:
...: 'kind' (branch of TestUnion) collides with 'type' (member of TestUnion)
If all I did was override 'role = "implicit tag"' instead of describe(),
it would report:
...: 'kind' (branch of TestUnion) collides with 'type' (implicit tag of
TestUnion)
Of course when a later subset fixes the kind/type misnomer, the error
message changes:
...: 'type' (branch of TestUnion) collides with 'type' (member of TestUnion)
where we lose the "implicit tag" because we completely lose the subclass.
Any preferences on which error message to prefer, and therefore how much
(or how little) we need to override in this temporary subclass? Or is
this an argument for making the subclass permanent (with a 'role =
"implicit tag"') even after the kind/type rename?
--
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] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 13/18] qapi: Track owner of each object member
2015-10-13 14:38 ` Eric Blake
@ 2015-10-13 16:30 ` Markus Armbruster
0 siblings, 0 replies; 51+ messages in thread
From: Markus Armbruster @ 2015-10-13 16:30 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 10/13/2015 07:14 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> Future commits will migrate semantic checking away from parsing
>>> and over to the various QAPISchema*.check() methods. But to
>>> report an error message about an incorrect semantic use of a
>>> member of an object type, it helps to know which type, command,
>>> or event owns the member. In particular, when a member is
>>> inherited from a base type, it is desirable to associate the
>>> member name with the base type (and not the type calling
>>> member.check()).
>>>
>
>>>
>>> + def describe(self):
>>> + owner = self.owner
>>> + # See QAPISchema._make_implicit_object_type() - reverse the
>>> + # mapping there to create a nice human-readable description
>>> + if owner.startswith(':obj-'):
>>> + owner = owner[5:]
>>> + if owner.endswith('-arg'):
>>> + source = '(argument of %s)' % owner[:4]
>>> + elif owner.endswith('-data'):
>>> + source = '(data of %s)' % owner[:5]
>>> + else:
>>> + assert owner.endswith('-wrapper')
>>> + source = '(branch of %s)' % owner[:8]
>>> + else:
>>> + source = '(%s of %s)' % (self.role, owner)
>>> + return "'%s' %s" % (self.name, source)
>>
>> Perhaps not exactly pretty, but it gets the job done with minimal fuss.
>> Sold.
>
> Ouch - I think these should be owner[:-4] (and similar) - I want the
> slice that excludes the last four bytes, and not one that is four bytes
> long. (It happened to work in my tests because I was testing on
> :obj-oops-arg, where owner[:4] and owner[:-4] gave the same 'oops' string).
>
> It's an easy fixup to squash in, so I'm not yet sure if I need a full v9
> respin.
If we decide we can do without a respin, I'll want the incremental patch
to squash in.
>>> +
>>> + role = 'member'
>>> +
>>>
>>
>> Class variables are usually defined first, before methods.
>
> Sure, can do. pep8 didn't complain, and I didn't check if pylint warns
> about it.
PEP8 (the document) doesn't ask for it, but the examples I found in the
Python tutorial are consistently putting them first.
>>> # TODO Drop this class once we no longer have the 'type'/'kind' mismatch
>>> class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
>>> @@ -1034,6 +1061,11 @@ class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
>>> assert self.type.is_implicit()
>>> return 'kind'
>>>
>>> + def describe(self):
>>> + # Must override superclass describe() because self.name is 'type'
>>
>> I don't find this argument convincing. I think 'kind' is exactly as
>> unhelpful as 'type' is. Specifically, should we report a QMP clash,
>> calling it 'kind' is confusing (it actually clashes with 'type').
>> Conversely 'type' is confusing when we report a C clash.
>>
>> The helpful part...
>>
>>> + assert self.owner[0] != ':'
>>> + return "'kind' (implicit tag of %s)" % self.owner
>>> +
>>
>> ... is (implicit tag of FOO).
>>
>> Fortunately, you're working towards killing the kind vs. type confusion.
>> Suggest to either rephrase the comment, or simply drop it.
>
> Drop the comment is fine by me.
>
> I personally thought the message needed to report 'kind', because 16/18
> ends up reporting:
>
> +tests/qapi-schema/union-clash-type.json:8: 'kind' (branch of TestUnion)
> collides with 'kind' (implicit tag of TestUnion)
>
> If I didn't override describe() at all, it would report:
>
> ...: 'kind' (branch of TestUnion) collides with 'type' (member of TestUnion)
>
> If all I did was override 'role = "implicit tag"' instead of describe(),
> it would report:
>
> ...: 'kind' (branch of TestUnion) collides with 'type' (implicit tag of
> TestUnion)
Good enough for me considering we'll clean up the 'kind' vs. 'type' mess
soon.
> Of course when a later subset fixes the kind/type misnomer, the error
> message changes:
>
> ...: 'type' (branch of TestUnion) collides with 'type' (member of TestUnion)
>
> where we lose the "implicit tag" because we completely lose the subclass.
Still good enough for me.
We could perhaps leverage is_implicit() to make it (implicit member of
TestUnion).
> Any preferences on which error message to prefer, and therefore how much
> (or how little) we need to override in this temporary subclass? Or is
> this an argument for making the subclass permanent (with a 'role =
> "implicit tag"') even after the kind/type rename?
I think I'd prefer the simplest solution that still gives understandable
error messages.
I'd even accept temporary regressions in error message quality if it
simplifies coding or review, or reduces churn.
When a little effort can yield significantly better error messages in
the final state, feel free to go for it, of course.
^ permalink raw reply [flat|nested] 51+ messages in thread
* [Qemu-devel] [PATCH v8 14/18] qapi: Detect collisions in C member names
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
` (12 preceding siblings ...)
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 13/18] qapi: Track owner of each object member Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check() Eric Blake
` (5 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Detect attempts to declare two object members that would result
in the same C member name, by keying the 'seen' dictionary off
of the C name rather than the qapi name. It also requires passing
info through some of the check() methods.
This fixes two previously-broken tests, and the resulting error
messages demonstrate the utility of the .describe() method added
previously. No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: rebase to earlier changes
v7: split out error reporting prep and member.c_name() addition
v6: rebase to earlier testsuite and info improvements
---
scripts/qapi.py | 33 ++++++++++++++++----------
tests/qapi-schema/args-name-clash.err | 1 +
tests/qapi-schema/args-name-clash.exit | 2 +-
tests/qapi-schema/args-name-clash.json | 6 ++---
tests/qapi-schema/args-name-clash.out | 6 -----
tests/qapi-schema/flat-union-clash-branch.err | 1 +
tests/qapi-schema/flat-union-clash-branch.exit | 2 +-
tests/qapi-schema/flat-union-clash-branch.json | 9 +++----
tests/qapi-schema/flat-union-clash-branch.out | 14 -----------
9 files changed, 32 insertions(+), 42 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 8b29e11..58c4bb3 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -978,11 +978,11 @@ class QAPISchemaObjectType(QAPISchemaType):
seen = {}
for m in members:
assert m.c_name() not in seen
- seen[m.name] = m
+ seen[m.c_name()] = m
for m in self.local_members:
- m.check(schema, members, seen)
+ m.check(schema, self.info, members, seen)
if self.variants:
- self.variants.check(schema, members, seen)
+ self.variants.check(schema, self.info, members, seen)
self.members = members
def is_implicit(self):
@@ -1022,13 +1022,19 @@ class QAPISchemaObjectTypeMember(object):
assert not self.owner
self.owner = name
- def check(self, schema, all_members, seen):
+ def check(self, schema, info, all_members, seen):
assert self.owner
- assert self.name not in seen
self.type = schema.lookup_type(self._type_name)
assert self.type
+ # Check that there is no collision in generated C names (which
+ # also ensures no collisions in QMP names)
+ if self.c_name() in seen:
+ raise QAPIExprError(info,
+ "%s collides with %s"
+ % (self.describe(),
+ seen[self.c_name()].describe()))
all_members.append(self)
- seen[self.name] = self
+ seen[self.c_name()] = self
def c_name(self):
return c_name(self.name)
@@ -1088,23 +1094,24 @@ class QAPISchemaObjectTypeVariants(object):
for v in self.variants:
v.set_owner(name)
- def check(self, schema, members, seen):
+ def check(self, schema, info, members, seen):
if self.tag_name:
- self.tag_member = seen[self.tag_name]
+ self.tag_member = seen[c_name(self.tag_name)]
+ assert self.tag_name == self.tag_member.name
else:
- self.tag_member.check(schema, members, seen)
+ self.tag_member.check(schema, info, members, seen)
assert isinstance(self.tag_member.type, QAPISchemaEnumType)
for v in self.variants:
vseen = dict(seen)
- v.check(schema, self.tag_member.type, vseen)
+ v.check(schema, info, self.tag_member.type, vseen)
class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
def __init__(self, name, typ):
QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
- def check(self, schema, tag_type, seen):
- QAPISchemaObjectTypeMember.check(self, schema, [], seen)
+ def check(self, schema, info, tag_type, seen):
+ QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
assert self.name in tag_type.values
# This function exists to support ugly simple union special cases
@@ -1129,7 +1136,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
self.variants = variants
def check(self, schema):
- self.variants.check(schema, [], {})
+ self.variants.check(schema, self.info, [], {})
def json_type(self):
return 'value'
diff --git a/tests/qapi-schema/args-name-clash.err b/tests/qapi-schema/args-name-clash.err
index e69de29..2735217 100644
--- a/tests/qapi-schema/args-name-clash.err
+++ b/tests/qapi-schema/args-name-clash.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-name-clash.json:5: 'a_b' (argument of oops) collides with 'a-b' (argument of oops)
diff --git a/tests/qapi-schema/args-name-clash.exit b/tests/qapi-schema/args-name-clash.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/args-name-clash.exit
+++ b/tests/qapi-schema/args-name-clash.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/args-name-clash.json b/tests/qapi-schema/args-name-clash.json
index 9e8f889..3fe4ea5 100644
--- a/tests/qapi-schema/args-name-clash.json
+++ b/tests/qapi-schema/args-name-clash.json
@@ -1,5 +1,5 @@
# C member name collision
-# FIXME - This parses, but fails to compile, because the C struct is given
-# two 'a_b' members. Either reject this at parse time, or munge the C names
-# to avoid the collision.
+# Reject members that clash when mapped to C names (we would have two 'a_b'
+# members). It would also be possible to munge the C names to avoid the
+# collision, but unlikely to be worth the effort.
{ '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
index 9b2f6e4..e69de29 100644
--- a/tests/qapi-schema/args-name-clash.out
+++ b/tests/qapi-schema/args-name-clash.out
@@ -1,6 +0,0 @@
-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-clash-branch.err b/tests/qapi-schema/flat-union-clash-branch.err
index e69de29..0190d79 100644
--- a/tests/qapi-schema/flat-union-clash-branch.err
+++ b/tests/qapi-schema/flat-union-clash-branch.err
@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-clash-branch.json:15: 'c-d' (branch of TestUnion) collides with 'c_d' (member of Base)
diff --git a/tests/qapi-schema/flat-union-clash-branch.exit b/tests/qapi-schema/flat-union-clash-branch.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/flat-union-clash-branch.exit
+++ b/tests/qapi-schema/flat-union-clash-branch.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/flat-union-clash-branch.json b/tests/qapi-schema/flat-union-clash-branch.json
index e593336..a6c302f 100644
--- a/tests/qapi-schema/flat-union-clash-branch.json
+++ b/tests/qapi-schema/flat-union-clash-branch.json
@@ -1,8 +1,9 @@
# Flat union branch name collision
-# FIXME: this parses, but then fails to compile due to a duplicate 'c_d'
-# (one from the base member, the other from the branch name). We should
-# either reject the collision at parse time, or munge the generated branch
-# name to allow this to compile.
+# Reject attempts to use a branch name that would clash with a non-variant
+# member, when mapped to C names (here, we would have two 'c_d' members,
+# one from the base member, the other from the branch name).
+# TODO: We could munge the generated branch name to avoid the collision and
+# allow this to compile.
{ 'enum': 'TestEnum',
'data': [ 'base', 'c-d' ] }
{ 'struct': 'Base',
diff --git a/tests/qapi-schema/flat-union-clash-branch.out b/tests/qapi-schema/flat-union-clash-branch.out
index 8e0da73..e69de29 100644
--- a/tests/qapi-schema/flat-union-clash-branch.out
+++ b/tests/qapi-schema/flat-union-clash-branch.out
@@ -1,14 +0,0 @@
-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
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check()
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
` (13 preceding siblings ...)
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 14/18] qapi: Detect collisions in C member names Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-13 15:06 ` Markus Armbruster
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 16/18] qapi: Move duplicate enum value " Eric Blake
` (4 subsequent siblings)
19 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
With the previous commit, we have two different locations for
detecting member name clashes - one at parse time, and another
at QAPISchema*.check() time. Consolidate some of the checks
into a single place, which is also in line with our TODO to
eventually move all of the parse time semantic checking into
the newer schema code. The check_member_clash() function is
no longer needed.
Checking variants is tricky: we need to check that the branch
name will not cause a collision (important for C code, but
no bearing on QMP). Then, if the variant belongs to a union
(flat or simple), we need to check that QMP members of that
type will not collide with non-variant QMP members (but do
not care if they collide with C branch names). Each call to
variant.check() has a 'seen' that contains all [*] non-variant
C names (which includes all non-variant QMP names), but does
not add to that array (QMP members of one branch do not cause
collisions with other branches). This means that there is
one form of collision we still miss: when two branch names
collide. However, that will be dealt with in the next patch.
[*] Exception - the 'seen' array doesn't contain 'base', which
is currently a non-variant C member of structs; but since
structs don't contain variants, it doesn't hurt. Besides, a
later patch will be unboxing structs the way flat unions
have already been unboxed.
The wording of several error messages has changed, but in many
cases feels like an improvement rather than a regression. The
recent change (commit 7b2a5c2) to avoid an assertion failure
when a flat union branch name collides with its discriminator
name is also handled nicely by this code; but there is more work
needed before we can detect all collisions in simple union branch
names done by the old code.
No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: decide whether to inline members based on union vs. alternate,
not on flat vs. simple, and fix logic to avoid breaking
union-clash-data in the process; add comments; assumes
pull-qapi-2015-10-12 will go in without modifying commit ids
v7: comment improvements, retitle subject
v6: rebase to earlier testsuite improvements, fold in cleanup
of flat-union-clash-type
---
scripts/qapi.py | 70 ++++++++++++---------------
tests/qapi-schema/flat-union-clash-member.err | 2 +-
tests/qapi-schema/flat-union-clash-type.err | 2 +-
tests/qapi-schema/struct-base-clash-deep.err | 2 +-
tests/qapi-schema/struct-base-clash.err | 2 +-
5 files changed, 36 insertions(+), 42 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 58c4bb3..144dd4a 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -496,21 +496,6 @@ def check_type(expr_info, source, value, allow_array=False,
'enum'])
-def check_member_clash(expr_info, base_name, data, source=""):
- base = find_struct(base_name)
- assert base
- base_members = base['data']
- for key in data.keys():
- if key.startswith('*'):
- key = key[1:]
- if key in base_members or "*" + key in base_members:
- raise QAPIExprError(expr_info,
- "Member name '%s'%s clashes with base '%s'"
- % (key, source, base_name))
- if base.get('base'):
- check_member_clash(expr_info, base['base'], data, source)
-
-
def check_command(expr, expr_info):
name = expr['command']
@@ -589,30 +574,18 @@ def check_union(expr, expr_info):
for (key, value) in members.items():
check_name(expr_info, "Member of union '%s'" % name, key)
- # Each value must name a known type; furthermore, in flat unions,
- # branches must be a struct with no overlapping member names
+ # Each value must name a known type
check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
value, allow_array=not base, allow_metas=allow_metas)
- if base:
- branch_struct = find_struct(value)
- assert branch_struct
- check_member_clash(expr_info, base, branch_struct['data'],
- " of branch '%s'" % key)
# If the discriminator names an enum type, then all members
- # of 'data' must also be members of the enum type, which in turn
- # must not collide with the discriminator name.
+ # of 'data' must also be members of the enum type.
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:
@@ -687,8 +660,6 @@ def check_struct(expr, expr_info):
allow_dict=True, allow_optional=True)
check_type(expr_info, "'base' for struct '%s'" % name, expr.get('base'),
allow_metas=['struct'])
- if expr.get('base'):
- check_member_clash(expr_info, expr['base'], expr['data'])
def check_keys(expr_elem, meta, required, optional=[]):
@@ -982,7 +953,7 @@ class QAPISchemaObjectType(QAPISchemaType):
for m in self.local_members:
m.check(schema, self.info, members, seen)
if self.variants:
- self.variants.check(schema, self.info, members, seen)
+ self.variants.check(schema, self.info, members, seen, True)
self.members = members
def is_implicit(self):
@@ -1094,7 +1065,7 @@ class QAPISchemaObjectTypeVariants(object):
for v in self.variants:
v.set_owner(name)
- def check(self, schema, info, members, seen):
+ def check(self, schema, info, members, seen, union):
if self.tag_name:
self.tag_member = seen[c_name(self.tag_name)]
assert self.tag_name == self.tag_member.name
@@ -1102,18 +1073,41 @@ class QAPISchemaObjectTypeVariants(object):
self.tag_member.check(schema, info, members, seen)
assert isinstance(self.tag_member.type, QAPISchemaEnumType)
for v in self.variants:
- vseen = dict(seen)
- v.check(schema, info, self.tag_member.type, vseen)
+ v.check(schema, info, self.tag_member.type, dict(seen), union)
class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
def __init__(self, name, typ):
QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
- def check(self, schema, info, tag_type, seen):
- QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
+ # TODO remove union argument once alternate types can be distinguished
+ # solely by their different tag_type
+ def check(self, schema, info, tag_type, seen, union):
+ # Check that the branch name does not collide with non-variant C
+ # members, without modifying caller's copy of seen
+ # TODO Detect collisions between branch names in C - easiest
+ # will be to check instead for collisions in the corresponding
+ # enum type
+ QAPISchemaObjectTypeMember.check(self, schema, info, [], dict(seen))
assert self.name in tag_type.values
+ # Additionally, for unions (flat or simple), the QMP members of the
+ # (possibly implicit) variant type are flattened into the owner
+ # object, and must not collide with any non-variant members. For
+ # alternates, no flattening occurs. No type can contain itself
+ # directly as a variant.
+ if union:
+ assert isinstance(self.type, QAPISchemaObjectType)
+ self.type.check(schema)
+ assert not self.type.variants # not implemented
+ for m in self.type.members:
+ if m.c_name() in seen:
+ raise QAPIExprError(info,
+ "Member '%s' of branch '%s' collides "
+ "with %s"
+ % (m.name, self.name,
+ seen[m.c_name()].describe()))
+
# This function exists to support ugly simple union special cases
# TODO get rid of them, and drop the function
def simple_union_type(self):
@@ -1136,7 +1130,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
self.variants = variants
def check(self, schema):
- self.variants.check(schema, self.info, [], {})
+ self.variants.check(schema, self.info, [], {}, False)
def json_type(self):
return 'value'
diff --git a/tests/qapi-schema/flat-union-clash-member.err b/tests/qapi-schema/flat-union-clash-member.err
index 2f0397a..57dd478 100644
--- a/tests/qapi-schema/flat-union-clash-member.err
+++ b/tests/qapi-schema/flat-union-clash-member.err
@@ -1 +1 @@
-tests/qapi-schema/flat-union-clash-member.json:11: Member name 'name' of branch 'value1' clashes with base 'Base'
+tests/qapi-schema/flat-union-clash-member.json:11: Member 'name' of branch 'value1' collides with 'name' (member of Base)
diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err
index b44dd40..3f3248b 100644
--- a/tests/qapi-schema/flat-union-clash-type.err
+++ b/tests/qapi-schema/flat-union-clash-type.err
@@ -1 +1 @@
-tests/qapi-schema/flat-union-clash-type.json:11: Discriminator name 'type' collides with enum value in 'TestEnum'
+tests/qapi-schema/flat-union-clash-type.json:11: 'type' (branch of TestUnion) collides with 'type' (member of Base)
diff --git a/tests/qapi-schema/struct-base-clash-deep.err b/tests/qapi-schema/struct-base-clash-deep.err
index f7a25a3..e2d7943 100644
--- a/tests/qapi-schema/struct-base-clash-deep.err
+++ b/tests/qapi-schema/struct-base-clash-deep.err
@@ -1 +1 @@
-tests/qapi-schema/struct-base-clash-deep.json:10: Member name 'name' clashes with base 'Base'
+tests/qapi-schema/struct-base-clash-deep.json:10: 'name' (member of Sub) collides with 'name' (member of Base)
diff --git a/tests/qapi-schema/struct-base-clash.err b/tests/qapi-schema/struct-base-clash.err
index 3a9f66b..c52f33d 100644
--- a/tests/qapi-schema/struct-base-clash.err
+++ b/tests/qapi-schema/struct-base-clash.err
@@ -1 +1 @@
-tests/qapi-schema/struct-base-clash.json:5: Member name 'name' clashes with base 'Base'
+tests/qapi-schema/struct-base-clash.json:5: 'name' (member of Sub) collides with 'name' (member of Base)
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check()
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check() Eric Blake
@ 2015-10-13 15:06 ` Markus Armbruster
2015-10-13 15:35 ` Eric Blake
0 siblings, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2015-10-13 15:06 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
First reply: commit message review only. Patch review to follow.
Eric Blake <eblake@redhat.com> writes:
> With the previous commit, we have two different locations for
> detecting member name clashes - one at parse time, and another
> at QAPISchema*.check() time. Consolidate some of the checks
> into a single place, which is also in line with our TODO to
> eventually move all of the parse time semantic checking into
> the newer schema code. The check_member_clash() function is
> no longer needed.
>
> Checking variants is tricky:
Indeed.
Struct types aren't as tricky, but tricky enough to warrant spelling
them out, so let me do that.
QMP: The member names of the JSON object are exactly the member names of
the struct type. Thus, members can clash with each other (d'oh!).
C: The member names of the C struct are exactly the C names of the *own*
(non-inherited) member names of the struct type, plus 'base' if it has a
base type, plus a has_FOO flag for each optional local member with C
name FOO. Therefore, local members can clash with each other (d'oh
again!), and additionally with 'base' and the has_FOOs.
The additional clashes are self-inflicted pain:
* Less foolish names for the flags, i.e. ones that cannot occur as
member names, would eliminate the has_FOO clashes.
* Unboxing base types like we do for unions would eliminate the 'base'
clash. Heck, even renaming base to something that cannot occur as
member name would.
If we check for clashes with *inherited* member names, too, then
checking for C clashes suffices, because when the QMP member names
clash, the C member names clash, too.
> we need to check that the branch
> name will not cause a collision (important for C code, but
> no bearing on QMP). Then, if the variant belongs to a union
> (flat or simple), we need to check that QMP members of that
> type will not collide with non-variant QMP members (but do
> not care if they collide with C branch names).
Union types.
QMP: The member names of the JSON object are exactly the names of the
union type's non-variant members (this includes the tag member; a simple
union's implicit tag is named 'type') plus the names of a single case's
variant members. Branch names occurs only as (tag) value, not as key.
Thus, members can clash with each other, except for variant members from
different cases.
C: The member names of the C struct are
* the C names of the non-variant members (this includes the tag member;
a simple union's implicit tag is named 'kind' now, soon 'type')
* a has_FOO for each optional non-variant member with C name FOO
* the branch names, wrapped in an unnamed union
* 'data', wrapped in the same unnamed union
Therefore, non-variant members can clash with each other as for struct
types (except here the inherited members *are* unboxed already), and
additionally
* branch names can clash with each other (but that's caught when
checking the tag enumeration, which has the branch names as values)
* branch names can clash with non-variant members
* 'data' can clash with branch names and non-variant members
The additional clashes are all self-inflicted pain: either give the
union a name that cannot clash with a non-variant member, or unbox the
cases and rename or kill 'data'.
If we check for clashes between the non-variant members and each single
case's variant members, too, then checking for C clashes suffices,
because when the QMP member names clash, the C member names clash, too.
> Each call to
> variant.check() has a 'seen' that contains all [*] non-variant
> C names (which includes all non-variant QMP names), but does
What exactly do you mean by the parenthesis?
> not add to that array (QMP members of one branch do not cause
> collisions with other branches). This means that there is
> one form of collision we still miss: when two branch names
> collide. However, that will be dealt with in the next patch.
Well, it's already dealt with. The next patch merely moves the dealing
into the .check().
> [*] Exception - the 'seen' array doesn't contain 'base', which
> is currently a non-variant C member of structs; but since
> structs don't contain variants, it doesn't hurt. Besides, a
> later patch will be unboxing structs the way flat unions
> have already been unboxed.
>
> The wording of several error messages has changed, but in many
> cases feels like an improvement rather than a regression. The
> recent change (commit 7b2a5c2) to avoid an assertion failure
> when a flat union branch name collides with its discriminator
> name is also handled nicely by this code; but there is more work
> needed before we can detect all collisions in simple union branch
> names done by the old code.
Only simple?
I've come to the conclusion that we should get rid of the self-inflicted
pain before we attempt to detect all collisions.
> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check()
2015-10-13 15:06 ` Markus Armbruster
@ 2015-10-13 15:35 ` Eric Blake
2015-10-13 17:13 ` Markus Armbruster
0 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2015-10-13 15:35 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 8973 bytes --]
On 10/13/2015 09:06 AM, Markus Armbruster wrote:
> First reply: commit message review only. Patch review to follow.
>
> Eric Blake <eblake@redhat.com> writes:
>
>> With the previous commit, we have two different locations for
>> detecting member name clashes - one at parse time, and another
>> at QAPISchema*.check() time. Consolidate some of the checks
>> into a single place, which is also in line with our TODO to
>> eventually move all of the parse time semantic checking into
>> the newer schema code. The check_member_clash() function is
>> no longer needed.
>>
>> Checking variants is tricky:
>
> Indeed.
>
> Struct types aren't as tricky, but tricky enough to warrant spelling
> them out, so let me do that.
Thanks. May be worth cribbing this information into the commit message,
if there is a reason for a v9 respin.
>
> QMP: The member names of the JSON object are exactly the member names of
> the struct type. Thus, members can clash with each other (d'oh!).
>
> C: The member names of the C struct are exactly the C names of the *own*
> (non-inherited) member names of the struct type, plus 'base' if it has a
> base type, plus a has_FOO flag for each optional local member with C
> name FOO. Therefore, local members can clash with each other (d'oh
> again!), and additionally with 'base' and the has_FOOs.
>
> The additional clashes are self-inflicted pain:
>
> * Less foolish names for the flags, i.e. ones that cannot occur as
> member names, would eliminate the has_FOO clashes.
Or even eliminating flags: at least for 'str' and objects, we can use
foo==NULL in place of has_foo==false. I have plans to add a markup to
various structs for when it is safe to eliminate the has_PTR bools, and
then over a series of patches clean up the .json files to take advantage
of it - but not until after the pending patches are flushed.
>
> * Unboxing base types like we do for unions would eliminate the 'base'
> clash. Heck, even renaming base to something that cannot occur as
> member name would.
My unflushed queue already includes this change, and I'm likely to
promote it to the front of the queue after subset B goes in (along with
the kind/type fixes).
>
> If we check for clashes with *inherited* member names, too, then
> checking for C clashes suffices, because when the QMP member names
> clash, the C member names clash, too.
Another clash is when two QMP names map to the same C name, as in 'a-b'
vs. 'a_b'. Checking for C member name clashes rejects things that are
not a QMP collision now, but also leaves the door open in case we later
decide to make QMP key detection looser and allow '-' vs. '_' to be
synonyms (if we allowed both spellings in the same struct now, then we
prevent making them synonyms in the future for anyone that (ab)uses both
names in the same struct).
>
>> we need to check that the branch
>> name will not cause a collision (important for C code, but
>> no bearing on QMP). Then, if the variant belongs to a union
>> (flat or simple), we need to check that QMP members of that
>> type will not collide with non-variant QMP members (but do
>> not care if they collide with C branch names).
>
> Union types.
>
> QMP: The member names of the JSON object are exactly the names of the
> union type's non-variant members (this includes the tag member; a simple
> union's implicit tag is named 'type') plus the names of a single case's
> variant members. Branch names occurs only as (tag) value, not as key.
>
> Thus, members can clash with each other, except for variant members from
> different cases.
>
> C: The member names of the C struct are
>
> * the C names of the non-variant members (this includes the tag member;
> a simple union's implicit tag is named 'kind' now, soon 'type')
>
> * a has_FOO for each optional non-variant member with C name FOO
>
> * the branch names, wrapped in an unnamed union
>
> * 'data', wrapped in the same unnamed union
>
> Therefore, non-variant members can clash with each other as for struct
> types (except here the inherited members *are* unboxed already), and
> additionally
>
> * branch names can clash with each other (but that's caught when
> checking the tag enumeration, which has the branch names as values)
>
> * branch names can clash with non-variant members
>
> * 'data' can clash with branch names and non-variant members
>
> The additional clashes are all self-inflicted pain: either give the
> union a name that cannot clash with a non-variant member, or unbox the
> cases and rename or kill 'data'.
Later patches kill 'data'. I'm not convinced about unboxing cases:
consider this qapi (with abbreviated notation):
{ 'union': 'Foo', 'base': { 'type': 'MyEnum' },
'discriminator': 'type',
'data': { 'b1': { 'm': 'int' },
'b2': { 'm': 'bool' } } }
which, if partially unboxed, would result in:
struct Foo {
MyEnum type;
union { /* tag is type */
struct {
int m;
} b1;
struct {
bool m;
} b2;
};
};
where the case names are still present, or if completely unboxed via
anonymous structs within the anonymous union, would present:
struct Foo {
MyEnum type;
union { /* tag is type */
struct {
int m;
};
struct {
bool m;
};
};
};
Oops - by making m completely anonymous, we have a C collision. But if
we munge things, such as 'b1_m' or 'b2_m', we no longer can use C names
to detect QMP collisions with the non-variant names.
I still haven't played with naming the union, but the more we discuss
this, the more I like the idea of:
struct Foo {
MyEnum type;
union { /* tag is type */
B1 b1;
B2 b2;
} u;
};
where the caller has to do foo.u.b1.m or foo.u.b2.m (instead of the
current foo.b1->m or foo.b2->m). That is, hiding the branch names
behind the union named 'u' (of course, we'd have to forbid a QMP
non-variant named 'u', or else go with a union named '_u') instantly
solves the problem of branches colliding with QMP names, at the price of
slightly more verbose C client code, and while still requiring effort to
track QMP collisions between branch members and non-variant members.
>
> If we check for clashes between the non-variant members and each single
> case's variant members, too, then checking for C clashes suffices,
> because when the QMP member names clash, the C member names clash, too.
>
>> Each call to
>> variant.check() has a 'seen' that contains all [*] non-variant
>> C names (which includes all non-variant QMP names), but does
>
> What exactly do you mean by the parenthesis?
I was trying to get at what you covered: that the set of C names
includes things like 'has_*' that are not QMP names, but that all
non-variant QMP names are at least included in the set of C names in the
'seen' dictionary.
>
>> not add to that array (QMP members of one branch do not cause
>> collisions with other branches). This means that there is
>> one form of collision we still miss: when two branch names
>> collide. However, that will be dealt with in the next patch.
>
> Well, it's already dealt with. The next patch merely moves the dealing
> into the .check().
>
>> [*] Exception - the 'seen' array doesn't contain 'base', which
>> is currently a non-variant C member of structs; but since
>> structs don't contain variants, it doesn't hurt. Besides, a
>> later patch will be unboxing structs the way flat unions
>> have already been unboxed.
>>
>> The wording of several error messages has changed, but in many
>> cases feels like an improvement rather than a regression. The
>> recent change (commit 7b2a5c2) to avoid an assertion failure
>> when a flat union branch name collides with its discriminator
>> name is also handled nicely by this code; but there is more work
>> needed before we can detect all collisions in simple union branch
>> names done by the old code.
>
> Only simple?
Ugh, I guess my wording needs an update here, to match the fact that I
updated the code to cover all unions. I was trying to convey that this
patch cannot revert everything added in 7b2a5c2, because the branch name
collision detection added in that patch is not covered by this move to
check().
>
> I've come to the conclusion that we should get rid of the self-inflicted
> pain before we attempt to detect all collisions.
Then that sounds like I should try harder to get the kind/type naming,
the boxed base naming, and even the anonymous union naming all hoisted
into this subset, and spin a v9?
--
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] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check()
2015-10-13 15:35 ` Eric Blake
@ 2015-10-13 17:13 ` Markus Armbruster
2015-10-13 17:43 ` Eric Blake
0 siblings, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2015-10-13 17:13 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 10/13/2015 09:06 AM, Markus Armbruster wrote:
>> First reply: commit message review only. Patch review to follow.
>>
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> With the previous commit, we have two different locations for
>>> detecting member name clashes - one at parse time, and another
>>> at QAPISchema*.check() time. Consolidate some of the checks
>>> into a single place, which is also in line with our TODO to
>>> eventually move all of the parse time semantic checking into
>>> the newer schema code. The check_member_clash() function is
>>> no longer needed.
>>>
>>> Checking variants is tricky:
>>
>> Indeed.
>>
>> Struct types aren't as tricky, but tricky enough to warrant spelling
>> them out, so let me do that.
>
> Thanks. May be worth cribbing this information into the commit message,
> if there is a reason for a v9 respin.
>
>>
>> QMP: The member names of the JSON object are exactly the member names of
>> the struct type. Thus, members can clash with each other (d'oh!).
>>
>> C: The member names of the C struct are exactly the C names of the *own*
>> (non-inherited) member names of the struct type, plus 'base' if it has a
>> base type, plus a has_FOO flag for each optional local member with C
>> name FOO. Therefore, local members can clash with each other (d'oh
>> again!), and additionally with 'base' and the has_FOOs.
>>
>> The additional clashes are self-inflicted pain:
>>
>> * Less foolish names for the flags, i.e. ones that cannot occur as
>> member names, would eliminate the has_FOO clashes.
>
> Or even eliminating flags: at least for 'str' and objects, we can use
> foo==NULL in place of has_foo==false. I have plans to add a markup to
> various structs for when it is safe to eliminate the has_PTR bools, and
> then over a series of patches clean up the .json files to take advantage
> of it - but not until after the pending patches are flushed.
I've wanted to get rid of the "have flags" for pointers since basically
forever.
But even then we'll still have other "have flags", and we'll want less
foolish names for them.
>>
>> * Unboxing base types like we do for unions would eliminate the 'base'
>> clash. Heck, even renaming base to something that cannot occur as
>> member name would.
>
> My unflushed queue already includes this change, and I'm likely to
> promote it to the front of the queue after subset B goes in (along with
> the kind/type fixes).
Sounds good.
>> If we check for clashes with *inherited* member names, too, then
>> checking for C clashes suffices, because when the QMP member names
>> clash, the C member names clash, too.
>
> Another clash is when two QMP names map to the same C name, as in 'a-b'
> vs. 'a_b'. Checking for C member name clashes rejects things that are
> not a QMP collision now, but also leaves the door open in case we later
> decide to make QMP key detection looser and allow '-' vs. '_' to be
> synonyms (if we allowed both spellings in the same struct now, then we
> prevent making them synonyms in the future for anyone that (ab)uses both
> names in the same struct).
Keeping the door open for cleaning up the '-' vs. '_' mess is a good
point.
>>> we need to check that the branch
>>> name will not cause a collision (important for C code, but
>>> no bearing on QMP). Then, if the variant belongs to a union
>>> (flat or simple), we need to check that QMP members of that
>>> type will not collide with non-variant QMP members (but do
>>> not care if they collide with C branch names).
>>
>> Union types.
>>
>> QMP: The member names of the JSON object are exactly the names of the
>> union type's non-variant members (this includes the tag member; a simple
>> union's implicit tag is named 'type') plus the names of a single case's
>> variant members. Branch names occurs only as (tag) value, not as key.
>>
>> Thus, members can clash with each other, except for variant members from
>> different cases.
>>
>> C: The member names of the C struct are
>>
>> * the C names of the non-variant members (this includes the tag member;
>> a simple union's implicit tag is named 'kind' now, soon 'type')
>>
>> * a has_FOO for each optional non-variant member with C name FOO
>>
>> * the branch names, wrapped in an unnamed union
>>
>> * 'data', wrapped in the same unnamed union
>>
>> Therefore, non-variant members can clash with each other as for struct
>> types (except here the inherited members *are* unboxed already), and
>> additionally
>>
>> * branch names can clash with each other (but that's caught when
>> checking the tag enumeration, which has the branch names as values)
>>
>> * branch names can clash with non-variant members
>>
>> * 'data' can clash with branch names and non-variant members
>>
>> The additional clashes are all self-inflicted pain: either give the
>> union a name that cannot clash with a non-variant member, or unbox the
>> cases and rename or kill 'data'.
>
> Later patches kill 'data'. I'm not convinced about unboxing cases:
> consider this qapi (with abbreviated notation):
>
> { 'union': 'Foo', 'base': { 'type': 'MyEnum' },
> 'discriminator': 'type',
> 'data': { 'b1': { 'm': 'int' },
> 'b2': { 'm': 'bool' } } }
>
> which, if partially unboxed, would result in:
>
> struct Foo {
> MyEnum type;
> union { /* tag is type */
> struct {
> int m;
> } b1;
> struct {
> bool m;
> } b2;
> };
> };
>
> where the case names are still present,
This makes sense, I think.
> or if completely unboxed via
> anonymous structs within the anonymous union, would present:
>
> struct Foo {
> MyEnum type;
> union { /* tag is type */
> struct {
> int m;
> };
> struct {
> bool m;
> };
> };
> };
>
> Oops - by making m completely anonymous, we have a C collision. But if
> we munge things, such as 'b1_m' or 'b2_m', we no longer can use C names
> to detect QMP collisions with the non-variant names.
You're right.
Calling the member b1_m when we could just as well call it b1.m feels
silly.
> I still haven't played with naming the union, but the more we discuss
> this, the more I like the idea of:
>
> struct Foo {
> MyEnum type;
> union { /* tag is type */
> B1 b1;
> B2 b2;
> } u;
> };
>
> where the caller has to do foo.u.b1.m or foo.u.b2.m (instead of the
> current foo.b1->m or foo.b2->m). That is, hiding the branch names
> behind the union named 'u' (of course, we'd have to forbid a QMP
> non-variant named 'u', or else go with a union named '_u') instantly
> solves the problem of branches colliding with QMP names, at the price of
> slightly more verbose C client code, and while still requiring effort to
> track QMP collisions between branch members and non-variant members.
I like the idea to avoid silly clashes by naming the union. The actual
union name is debatable detail.
>> If we check for clashes between the non-variant members and each single
>> case's variant members, too, then checking for C clashes suffices,
>> because when the QMP member names clash, the C member names clash, too.
>>
>>> Each call to
>>> variant.check() has a 'seen' that contains all [*] non-variant
>>> C names (which includes all non-variant QMP names), but does
>>
>> What exactly do you mean by the parenthesis?
>
> I was trying to get at what you covered: that the set of C names
> includes things like 'has_*' that are not QMP names, but that all
> non-variant QMP names are at least included in the set of C names in the
> 'seen' dictionary.
Ah, okay.
>>> not add to that array (QMP members of one branch do not cause
>>> collisions with other branches). This means that there is
>>> one form of collision we still miss: when two branch names
>>> collide. However, that will be dealt with in the next patch.
>>
>> Well, it's already dealt with. The next patch merely moves the dealing
>> into the .check().
>>
>>> [*] Exception - the 'seen' array doesn't contain 'base', which
>>> is currently a non-variant C member of structs; but since
>>> structs don't contain variants, it doesn't hurt. Besides, a
>>> later patch will be unboxing structs the way flat unions
>>> have already been unboxed.
>>>
>>> The wording of several error messages has changed, but in many
>>> cases feels like an improvement rather than a regression. The
>>> recent change (commit 7b2a5c2) to avoid an assertion failure
>>> when a flat union branch name collides with its discriminator
>>> name is also handled nicely by this code; but there is more work
>>> needed before we can detect all collisions in simple union branch
>>> names done by the old code.
>>
>> Only simple?
>
> Ugh, I guess my wording needs an update here, to match the fact that I
> updated the code to cover all unions. I was trying to convey that this
> patch cannot revert everything added in 7b2a5c2, because the branch name
> collision detection added in that patch is not covered by this move to
> check().
>
>>
>> I've come to the conclusion that we should get rid of the self-inflicted
>> pain before we attempt to detect all collisions.
>
> Then that sounds like I should try harder to get the kind/type naming,
> the boxed base naming, and even the anonymous union naming all hoisted
> into this subset, and spin a v9?
I can take PATCH 01-09,12 into my tree right away, with PATCH 07's two
redundant is_implicit() methods dropped, and PATCH 12's comment touched
up.
I could take PATCH 10, but let's at least try to make a plan for
c_name() first. If we fail, I'll take the patch, perhaps less the % to
+ change, and we'll revisit c_name() later when we see more clearly.
You want to move PATCH 11 to later in the queue, and I like that.
PATCH 13 needs a fix squashed in, and a few nits touched up. If you
want me to do that on commit, please propose a patch for me to squash
in. But a respin is probably easier for all.
PATCH 14 is fine, but it depends on 13.
I haven't finished review of PATCH 15-18.
Taken together, I think the easiest way forward is I take 01-09,12, and
you respin the rest after we finish its review. Makes sense?
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check()
2015-10-13 17:13 ` Markus Armbruster
@ 2015-10-13 17:43 ` Eric Blake
2015-10-13 18:32 ` Markus Armbruster
0 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2015-10-13 17:43 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1752 bytes --]
On 10/13/2015 11:13 AM, Markus Armbruster wrote:
>>>
>>> I've come to the conclusion that we should get rid of the self-inflicted
>>> pain before we attempt to detect all collisions.
>>
>> Then that sounds like I should try harder to get the kind/type naming,
>> the boxed base naming, and even the anonymous union naming all hoisted
>> into this subset, and spin a v9?
>
> I can take PATCH 01-09,12 into my tree right away, with PATCH 07's two
> redundant is_implicit() methods dropped, and PATCH 12's comment touched
> up.
Okay.
>
> I could take PATCH 10, but let's at least try to make a plan for
> c_name() first. If we fail, I'll take the patch, perhaps less the % to
> + change, and we'll revisit c_name() later when we see more clearly.
At this point, I'm not sure whether 10 disappears completely after the
type/kind fix, so that alone is a good enough reason to leave 10 out of
your tree for another round.
>
> You want to move PATCH 11 to later in the queue, and I like that.
>
> PATCH 13 needs a fix squashed in, and a few nits touched up. If you
> want me to do that on commit, please propose a patch for me to squash
> in. But a respin is probably easier for all.
>
> PATCH 14 is fine, but it depends on 13.
>
> I haven't finished review of PATCH 15-18.
>
> Taken together, I think the easiest way forward is I take 01-09,12, and
> you respin the rest after we finish its review. Makes sense?
>
Sounds like we're agreed then: take the obvious patches into your tree,
and let me rework the tail of this subset on top of cleanups that reduce
self-inflicted collisions.
--
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] 51+ messages in thread
* Re: [Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check()
2015-10-13 17:43 ` Eric Blake
@ 2015-10-13 18:32 ` Markus Armbruster
2015-10-13 20:17 ` Eric Blake
2015-10-14 7:32 ` Markus Armbruster
0 siblings, 2 replies; 51+ messages in thread
From: Markus Armbruster @ 2015-10-13 18:32 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 10/13/2015 11:13 AM, Markus Armbruster wrote:
>
>>>>
>>>> I've come to the conclusion that we should get rid of the self-inflicted
>>>> pain before we attempt to detect all collisions.
>>>
>>> Then that sounds like I should try harder to get the kind/type naming,
>>> the boxed base naming, and even the anonymous union naming all hoisted
>>> into this subset, and spin a v9?
>>
>> I can take PATCH 01-09,12 into my tree right away, with PATCH 07's two
>> redundant is_implicit() methods dropped, and PATCH 12's comment touched
>> up.
>
> Okay.
Done & pushed to http://repo.or.cz/qemu/armbru.git branch qapi-next.
>>
>> I could take PATCH 10, but let's at least try to make a plan for
>> c_name() first. If we fail, I'll take the patch, perhaps less the % to
>> + change, and we'll revisit c_name() later when we see more clearly.
>
> At this point, I'm not sure whether 10 disappears completely after the
> type/kind fix, so that alone is a good enough reason to leave 10 out of
> your tree for another round.
>
>>
>> You want to move PATCH 11 to later in the queue, and I like that.
>>
>> PATCH 13 needs a fix squashed in, and a few nits touched up. If you
>> want me to do that on commit, please propose a patch for me to squash
>> in. But a respin is probably easier for all.
>>
>> PATCH 14 is fine, but it depends on 13.
>>
>> I haven't finished review of PATCH 15-18.
>>
>> Taken together, I think the easiest way forward is I take 01-09,12, and
>> you respin the rest after we finish its review. Makes sense?
>>
>
> Sounds like we're agreed then: take the obvious patches into your tree,
> and let me rework the tail of this subset on top of cleanups that reduce
> self-inflicted collisions.
Yes, please. I'll try to review v8 16-18 quickly.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check()
2015-10-13 18:32 ` Markus Armbruster
@ 2015-10-13 20:17 ` Eric Blake
2015-10-13 20:20 ` Eric Blake
2015-10-14 7:32 ` Markus Armbruster
1 sibling, 1 reply; 51+ messages in thread
From: Eric Blake @ 2015-10-13 20:17 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2109 bytes --]
On 10/13/2015 12:32 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 10/13/2015 11:13 AM, Markus Armbruster wrote:
>>
>>>>>
>>>>> I've come to the conclusion that we should get rid of the self-inflicted
>>>>> pain before we attempt to detect all collisions.
>>>>
>>>> Then that sounds like I should try harder to get the kind/type naming,
>>>> the boxed base naming, and even the anonymous union naming all hoisted
>>>> into this subset, and spin a v9?
>>>
>>> I can take PATCH 01-09,12 into my tree right away, with PATCH 07's two
>>> redundant is_implicit() methods dropped, and PATCH 12's comment touched
>>> up.
>>
>> Okay.
>
> Done & pushed to http://repo.or.cz/qemu/armbru.git branch qapi-next.
I didn't see any mentioned changes on patch 7, at least not in commit
4ad5066. Last paragraph of the commit message would also need a
massage, if you do want to squash it in:
Instead, add an is_implicit() method to QAPISchemaEntity, and use it.
It can be overridden later for ObjectType and EnumType, when implicit
instances of those classes gain info.
diff --git a/scripts/qapi.py b/scripts/qapi.py
index e263ecf..d7cf0f3 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -903,10 +903,6 @@ class QAPISchemaEnumType(QAPISchemaType):
def check(self, schema):
assert len(set(self.values)) == len(self.values)
- def is_implicit(self):
- # See QAPISchema._make_implicit_enum_type()
- return self.name[-4:] == 'Kind'
-
def c_type(self, is_param=False):
return c_name(self.name)
@@ -977,10 +973,6 @@ class QAPISchemaObjectType(QAPISchemaType):
self.variants.check(schema, members, seen)
self.members = members
- def is_implicit(self):
- # See QAPISchema._make_implicit_object_type()
- return self.name[0] == ':'
-
def c_name(self):
assert not self.is_implicit()
return QAPISchemaType.c_name(self)
--
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 related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check()
2015-10-13 20:17 ` Eric Blake
@ 2015-10-13 20:20 ` Eric Blake
2015-10-14 7:11 ` Markus Armbruster
0 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2015-10-13 20:20 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 790 bytes --]
On 10/13/2015 02:17 PM, Eric Blake wrote:
>>>> I can take PATCH 01-09,12 into my tree right away, with PATCH 07's two
>>>> redundant is_implicit() methods dropped, and PATCH 12's comment touched
>>>> up.
>>>
>>> Okay.
>>
>> Done & pushed to http://repo.or.cz/qemu/armbru.git branch qapi-next.
>
> I didn't see any mentioned changes on patch 7, at least not in commit
> 4ad5066. Last paragraph of the commit message would also need a
> massage, if you do want to squash it in:
And of course, if you move the hunks out of 7, then they DO need to be
added back into 12, since it is 12 that provides non-None info on
implicit types (not seen in commit 20bfea1).
--
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] 51+ messages in thread
* Re: [Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check()
2015-10-13 20:20 ` Eric Blake
@ 2015-10-14 7:11 ` Markus Armbruster
0 siblings, 0 replies; 51+ messages in thread
From: Markus Armbruster @ 2015-10-14 7:11 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 10/13/2015 02:17 PM, Eric Blake wrote:
>
>>>>> I can take PATCH 01-09,12 into my tree right away, with PATCH 07's two
>>>>> redundant is_implicit() methods dropped, and PATCH 12's comment touched
>>>>> up.
>>>>
>>>> Okay.
>>>
>>> Done & pushed to http://repo.or.cz/qemu/armbru.git branch qapi-next.
>>
>> I didn't see any mentioned changes on patch 7, at least not in commit
>> 4ad5066. Last paragraph of the commit message would also need a
>> massage, if you do want to squash it in:
I messed it up, fixing :)
> And of course, if you move the hunks out of 7, then they DO need to be
> added back into 12, since it is 12 that provides non-None info on
> implicit types (not seen in commit 20bfea1).
Yup.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check()
2015-10-13 18:32 ` Markus Armbruster
2015-10-13 20:17 ` Eric Blake
@ 2015-10-14 7:32 ` Markus Armbruster
2015-10-14 12:59 ` Eric Blake
1 sibling, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2015-10-14 7:32 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Markus Armbruster <armbru@redhat.com> writes:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 10/13/2015 11:13 AM, Markus Armbruster wrote:
>>
>>>>>
>>>>> I've come to the conclusion that we should get rid of the self-inflicted
>>>>> pain before we attempt to detect all collisions.
>>>>
>>>> Then that sounds like I should try harder to get the kind/type naming,
>>>> the boxed base naming, and even the anonymous union naming all hoisted
>>>> into this subset, and spin a v9?
>>>
>>> I can take PATCH 01-09,12 into my tree right away, with PATCH 07's two
>>> redundant is_implicit() methods dropped, and PATCH 12's comment touched
>>> up.
>>
>> Okay.
>
> Done & pushed to http://repo.or.cz/qemu/armbru.git branch qapi-next.
Redone & pushed.
Summary of tweaks:
* PATCH 01-06.5: unchanged
* PATCH 07: your fixup to drop is_implicit() squashed in
* PATCH 08: array info plumbing moved to PATCH 12
* PATCH 09: trivially rebased
* PATCH 12: revert your fixup, so we get the move we want, adjust commit
message accordingly, supply missing QAPISchemaArrayType.is_implicit()
The end result is the same except for
diff --git a/scripts/qapi.py b/scripts/qapi.py
index dad381d..8a12b91 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -939,6 +939,9 @@ class QAPISchemaArrayType(QAPISchemaType):
self.element_type = schema.lookup_type(self._element_type_name)
assert self.element_type
+ def is_implicit(self):
+ return True
+
def json_type(self):
return 'array'
[...]
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check()
2015-10-14 7:32 ` Markus Armbruster
@ 2015-10-14 12:59 ` Eric Blake
2015-10-14 13:23 ` Markus Armbruster
0 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2015-10-14 12:59 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1501 bytes --]
On 10/14/2015 01:32 AM, Markus Armbruster wrote:
>> Done & pushed to http://repo.or.cz/qemu/armbru.git branch qapi-next.
>
> Redone & pushed.
>
> Summary of tweaks:
> * PATCH 01-06.5: unchanged
> * PATCH 07: your fixup to drop is_implicit() squashed in
> * PATCH 08: array info plumbing moved to PATCH 12
> * PATCH 09: trivially rebased
> * PATCH 12: revert your fixup, so we get the move we want, adjust commit
> message accordingly, supply missing QAPISchemaArrayType.is_implicit()
You may also want to squash this into 12:
diff --git a/scripts/qapi.py b/scripts/qapi.py
index fd95864..1e01714 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -792,10 +792,9 @@ class QAPISchemaEntity(object):
self.name = name
# For explicitly defined entities, info points to the (explicit)
# definition. For builtins (and their arrays), info is None.
- # For other arrays, info points to an explicit place that uses
- # the array (there may be more than one such place). For other
- # implicitly defined entities, it points to the place that
- # triggered the implicit definition.
+ # For implicitly defined entities, info points to a place that
+ # triggered the implicit definition (there may be more than one
+ # such place).
self.info = info
def c_name(self):
--
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 related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check()
2015-10-14 12:59 ` Eric Blake
@ 2015-10-14 13:23 ` Markus Armbruster
0 siblings, 0 replies; 51+ messages in thread
From: Markus Armbruster @ 2015-10-14 13:23 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 10/14/2015 01:32 AM, Markus Armbruster wrote:
>
>>> Done & pushed to http://repo.or.cz/qemu/armbru.git branch qapi-next.
>>
>> Redone & pushed.
>>
>> Summary of tweaks:
>> * PATCH 01-06.5: unchanged
>> * PATCH 07: your fixup to drop is_implicit() squashed in
>> * PATCH 08: array info plumbing moved to PATCH 12
>> * PATCH 09: trivially rebased
>> * PATCH 12: revert your fixup, so we get the move we want, adjust commit
>> message accordingly, supply missing QAPISchemaArrayType.is_implicit()
>
> You may also want to squash this into 12:
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index fd95864..1e01714 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -792,10 +792,9 @@ class QAPISchemaEntity(object):
> self.name = name
> # For explicitly defined entities, info points to the (explicit)
> # definition. For builtins (and their arrays), info is None.
> - # For other arrays, info points to an explicit place that uses
> - # the array (there may be more than one such place). For other
> - # implicitly defined entities, it points to the place that
> - # triggered the implicit definition.
> + # For implicitly defined entities, info points to a place that
> + # triggered the implicit definition (there may be more than one
> + # such place).
> self.info = info
>
> def c_name(self):
Done. Thanks!
^ permalink raw reply [flat|nested] 51+ messages in thread
* [Qemu-devel] [PATCH v8 16/18] qapi: Move duplicate enum value checks to schema check()
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
` (14 preceding siblings ...)
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 15/18] qapi: Move duplicate member checks to schema check() Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-13 18:35 ` Markus Armbruster
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 17/18] qapi: Add test for alternate branch 'kind' clash Eric Blake
` (3 subsequent siblings)
19 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Similar to the previous commit, move the detection of a collision
in enum values from parse time to QAPISchemaEnumType.check().
This happens to also detect collisions in union branch names,
so for a decent error message, we have to determine if the enum
is implicit (and if so where the real collision lies).
Testing this showed that the test union-bad-branch and
union-clash-branches were basically testing the same thing;
with the minor difference that the former clashes only in the
enum, while the latter also clashes in the C union member
names that would be generated. So delete the weaker test.
No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: rebase to earlier changes; better comments
v7: retitle and improve commit message; earlier subclass patches
avoid problem with detecting 'kind' collision
v6: new patch
---
scripts/qapi.py | 54 +++++++++++++-----------------
tests/Makefile | 1 -
tests/qapi-schema/alternate-clash.err | 2 +-
tests/qapi-schema/enum-clash-member.err | 2 +-
tests/qapi-schema/enum-max-member.err | 2 +-
tests/qapi-schema/union-bad-branch.err | 1 -
tests/qapi-schema/union-bad-branch.exit | 1 -
tests/qapi-schema/union-bad-branch.json | 8 -----
tests/qapi-schema/union-bad-branch.out | 0
tests/qapi-schema/union-clash-branches.err | 2 +-
tests/qapi-schema/union-clash-type.err | 2 +-
tests/qapi-schema/union-max.err | 2 +-
12 files changed, 29 insertions(+), 48 deletions(-)
delete mode 100644 tests/qapi-schema/union-bad-branch.err
delete mode 100644 tests/qapi-schema/union-bad-branch.exit
delete mode 100644 tests/qapi-schema/union-bad-branch.json
delete mode 100644 tests/qapi-schema/union-bad-branch.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 144dd4a..b21e38e 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -527,7 +527,6 @@ def check_union(expr, expr_info):
base = expr.get('base')
discriminator = expr.get('discriminator')
members = expr['data']
- values = {'MAX': '(automatic)', 'KIND': '(automatic)'}
# Two types of unions, determined by discriminator.
@@ -587,34 +586,16 @@ def check_union(expr, expr_info):
"enum '%s'" %
(key, enum_define["enum_name"]))
- # Otherwise, check for conflicts in the generated enum
- else:
- c_key = camel_to_upper(key)
- if c_key in values:
- raise QAPIExprError(expr_info,
- "Union '%s' member '%s' clashes with '%s'"
- % (name, key, values[c_key]))
- values[c_key] = key
-
def check_alternate(expr, expr_info):
name = expr['alternate']
members = expr['data']
- values = {'MAX': '(automatic)'}
types_seen = {}
# Check every branch
for (key, value) in members.items():
check_name(expr_info, "Member of alternate '%s'" % name, key)
- # Check for conflicts in the generated enum
- c_key = camel_to_upper(key)
- if c_key in values:
- raise QAPIExprError(expr_info,
- "Alternate '%s' member '%s' clashes with '%s'"
- % (name, key, values[c_key]))
- values[c_key] = key
-
# Ensure alternates have no type conflicts.
check_type(expr_info, "Member '%s' of alternate '%s'" % (key, name),
value,
@@ -633,7 +614,6 @@ def check_enum(expr, expr_info):
name = expr['enum']
members = expr.get('data')
prefix = expr.get('prefix')
- values = {'MAX': '(automatic)'}
if not isinstance(members, list):
raise QAPIExprError(expr_info,
@@ -644,12 +624,6 @@ def check_enum(expr, expr_info):
for member in members:
check_name(expr_info, "Member of enum '%s'" % name, member,
enum_member=True)
- key = camel_to_upper(member)
- if key in values:
- raise QAPIExprError(expr_info,
- "Enum '%s' member '%s' clashes with '%s'"
- % (name, member, values[key]))
- values[key] = member
def check_struct(expr, expr_info):
@@ -878,7 +852,26 @@ class QAPISchemaEnumType(QAPISchemaType):
self.prefix = prefix
def check(self, schema):
- assert len(set(self.values)) == len(self.values)
+ # Check for collisions on the generated C enum values
+ seen = {c_enum_const(self.name, 'MAX'): '(automatic MAX)'}
+ for value in self.values:
+ c_value = c_enum_const(self.name, value)
+ if c_value in seen:
+ # If the enum is implicit, report the error on behalf of
+ # the union or alternate that triggered the enum
+ if self.is_implicit():
+ owner = schema.lookup_type(self.name[:-4])
+ assert owner
+ if isinstance(owner, QAPISchemaAlternateType):
+ description = "Alternate '%s' branch" % owner.name
+ else:
+ description = "Union '%s' branch" % owner.name
+ else:
+ description = "Enum '%s' value" % self.name
+ raise QAPIExprError(self.info,
+ "%s '%s' clashes with '%s'"
+ % (description, value, seen[c_value]))
+ seen[c_value] = value
def is_implicit(self):
# See QAPISchema._make_implicit_enum_type()
@@ -1084,10 +1077,9 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
# solely by their different tag_type
def check(self, schema, info, tag_type, seen, union):
# Check that the branch name does not collide with non-variant C
- # members, without modifying caller's copy of seen
- # TODO Detect collisions between branch names in C - easiest
- # will be to check instead for collisions in the corresponding
- # enum type
+ # members, without modifying caller's copy of seen. Collisions
+ # between branch names in C is detected elsewhere, when validating
+ # that the correpsonding enum of tag_type has no collision.
QAPISchemaObjectTypeMember.check(self, schema, info, [], dict(seen))
assert self.name in tag_type.values
diff --git a/tests/Makefile b/tests/Makefile
index 35a63f3..2cd5d31 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -331,7 +331,6 @@ 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-clash-branches.json
qapi-schema += union-clash-data.json
diff --git a/tests/qapi-schema/alternate-clash.err b/tests/qapi-schema/alternate-clash.err
index a475ab6..7fd3069 100644
--- a/tests/qapi-schema/alternate-clash.err
+++ b/tests/qapi-schema/alternate-clash.err
@@ -1 +1 @@
-tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' member 'a_b' clashes with 'a-b'
+tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' branch 'a_b' clashes with 'a-b'
diff --git a/tests/qapi-schema/enum-clash-member.err b/tests/qapi-schema/enum-clash-member.err
index 48bd136..84030c5 100644
--- a/tests/qapi-schema/enum-clash-member.err
+++ b/tests/qapi-schema/enum-clash-member.err
@@ -1 +1 @@
-tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' member 'ONE' clashes with 'one'
+tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' value 'ONE' clashes with 'one'
diff --git a/tests/qapi-schema/enum-max-member.err b/tests/qapi-schema/enum-max-member.err
index f77837f..6b9ef9b 100644
--- a/tests/qapi-schema/enum-max-member.err
+++ b/tests/qapi-schema/enum-max-member.err
@@ -1 +1 @@
-tests/qapi-schema/enum-max-member.json:3: Enum 'MyEnum' member 'max' clashes with '(automatic)'
+tests/qapi-schema/enum-max-member.json:3: Enum 'MyEnum' value 'max' clashes with '(automatic MAX)'
diff --git a/tests/qapi-schema/union-bad-branch.err b/tests/qapi-schema/union-bad-branch.err
deleted file mode 100644
index 8822735..0000000
--- a/tests/qapi-schema/union-bad-branch.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/union-bad-branch.json:6: Union 'MyUnion' member 'ONE' clashes with 'one'
diff --git a/tests/qapi-schema/union-bad-branch.exit b/tests/qapi-schema/union-bad-branch.exit
deleted file mode 100644
index d00491f..0000000
--- a/tests/qapi-schema/union-bad-branch.exit
+++ /dev/null
@@ -1 +0,0 @@
-1
diff --git a/tests/qapi-schema/union-bad-branch.json b/tests/qapi-schema/union-bad-branch.json
deleted file mode 100644
index 913aa38..0000000
--- a/tests/qapi-schema/union-bad-branch.json
+++ /dev/null
@@ -1,8 +0,0 @@
-# we reject normal unions where branches would collide in C
-{ 'struct': 'One',
- 'data': { 'string': 'str' } }
-{ 'struct': 'Two',
- 'data': { 'number': 'int' } }
-{ 'union': 'MyUnion',
- 'data': { 'one': 'One',
- 'ONE': 'Two' } }
diff --git a/tests/qapi-schema/union-bad-branch.out b/tests/qapi-schema/union-bad-branch.out
deleted file mode 100644
index e69de29..0000000
diff --git a/tests/qapi-schema/union-clash-branches.err b/tests/qapi-schema/union-clash-branches.err
index 005c48d..d8f1265 100644
--- a/tests/qapi-schema/union-clash-branches.err
+++ b/tests/qapi-schema/union-clash-branches.err
@@ -1 +1 @@
-tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' member 'a_b' clashes with 'a-b'
+tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' branch 'a_b' clashes with 'a-b'
diff --git a/tests/qapi-schema/union-clash-type.err b/tests/qapi-schema/union-clash-type.err
index a5dead1..ce7f8cd 100644
--- a/tests/qapi-schema/union-clash-type.err
+++ b/tests/qapi-schema/union-clash-type.err
@@ -1 +1 @@
-tests/qapi-schema/union-clash-type.json:8: Union 'TestUnion' member 'kind' clashes with '(automatic)'
+tests/qapi-schema/union-clash-type.json:8: 'kind' (branch of TestUnion) collides with 'kind' (implicit tag of TestUnion)
diff --git a/tests/qapi-schema/union-max.err b/tests/qapi-schema/union-max.err
index 55ce439..b93beae 100644
--- a/tests/qapi-schema/union-max.err
+++ b/tests/qapi-schema/union-max.err
@@ -1 +1 @@
-tests/qapi-schema/union-max.json:2: Union 'Union' member 'max' clashes with '(automatic)'
+tests/qapi-schema/union-max.json:2: Union 'Union' branch 'max' clashes with '(automatic MAX)'
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 16/18] qapi: Move duplicate enum value checks to schema check()
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 16/18] qapi: Move duplicate enum value " Eric Blake
@ 2015-10-13 18:35 ` Markus Armbruster
2015-10-13 19:37 ` Eric Blake
0 siblings, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2015-10-13 18:35 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Similar to the previous commit, move the detection of a collision
> in enum values from parse time to QAPISchemaEnumType.check().
> This happens to also detect collisions in union branch names,
> so for a decent error message, we have to determine if the enum
> is implicit (and if so where the real collision lies).
>
> Testing this showed that the test union-bad-branch and
> union-clash-branches were basically testing the same thing;
> with the minor difference that the former clashes only in the
> enum, while the latter also clashes in the C union member
> names that would be generated. So delete the weaker test.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v8: rebase to earlier changes; better comments
> v7: retitle and improve commit message; earlier subclass patches
> avoid problem with detecting 'kind' collision
> v6: new patch
> ---
> scripts/qapi.py | 54 +++++++++++++-----------------
> tests/Makefile | 1 -
> tests/qapi-schema/alternate-clash.err | 2 +-
> tests/qapi-schema/enum-clash-member.err | 2 +-
> tests/qapi-schema/enum-max-member.err | 2 +-
> tests/qapi-schema/union-bad-branch.err | 1 -
> tests/qapi-schema/union-bad-branch.exit | 1 -
> tests/qapi-schema/union-bad-branch.json | 8 -----
> tests/qapi-schema/union-bad-branch.out | 0
> tests/qapi-schema/union-clash-branches.err | 2 +-
> tests/qapi-schema/union-clash-type.err | 2 +-
> tests/qapi-schema/union-max.err | 2 +-
> 12 files changed, 29 insertions(+), 48 deletions(-)
> delete mode 100644 tests/qapi-schema/union-bad-branch.err
> delete mode 100644 tests/qapi-schema/union-bad-branch.exit
> delete mode 100644 tests/qapi-schema/union-bad-branch.json
> delete mode 100644 tests/qapi-schema/union-bad-branch.out
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 144dd4a..b21e38e 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -527,7 +527,6 @@ def check_union(expr, expr_info):
> base = expr.get('base')
> discriminator = expr.get('discriminator')
> members = expr['data']
> - values = {'MAX': '(automatic)', 'KIND': '(automatic)'}
Stupid / tired question: I can see 'MAX' in the new code further down,
but I can't see 'KIND'. Why? Was it perhaps covered by the previous
patch?
>
> # Two types of unions, determined by discriminator.
>
> @@ -587,34 +586,16 @@ def check_union(expr, expr_info):
> "enum '%s'" %
> (key, enum_define["enum_name"]))
>
> - # Otherwise, check for conflicts in the generated enum
> - else:
> - c_key = camel_to_upper(key)
> - if c_key in values:
> - raise QAPIExprError(expr_info,
> - "Union '%s' member '%s' clashes with '%s'"
> - % (name, key, values[c_key]))
> - values[c_key] = key
> -
>
> def check_alternate(expr, expr_info):
> name = expr['alternate']
> members = expr['data']
> - values = {'MAX': '(automatic)'}
> types_seen = {}
>
> # Check every branch
> for (key, value) in members.items():
> check_name(expr_info, "Member of alternate '%s'" % name, key)
>
> - # Check for conflicts in the generated enum
> - c_key = camel_to_upper(key)
> - if c_key in values:
> - raise QAPIExprError(expr_info,
> - "Alternate '%s' member '%s' clashes with '%s'"
> - % (name, key, values[c_key]))
> - values[c_key] = key
> -
> # Ensure alternates have no type conflicts.
> check_type(expr_info, "Member '%s' of alternate '%s'" % (key, name),
> value,
> @@ -633,7 +614,6 @@ def check_enum(expr, expr_info):
> name = expr['enum']
> members = expr.get('data')
> prefix = expr.get('prefix')
> - values = {'MAX': '(automatic)'}
>
> if not isinstance(members, list):
> raise QAPIExprError(expr_info,
> @@ -644,12 +624,6 @@ def check_enum(expr, expr_info):
> for member in members:
> check_name(expr_info, "Member of enum '%s'" % name, member,
> enum_member=True)
> - key = camel_to_upper(member)
> - if key in values:
> - raise QAPIExprError(expr_info,
> - "Enum '%s' member '%s' clashes with '%s'"
> - % (name, member, values[key]))
> - values[key] = member
>
>
> def check_struct(expr, expr_info):
> @@ -878,7 +852,26 @@ class QAPISchemaEnumType(QAPISchemaType):
> self.prefix = prefix
>
> def check(self, schema):
> - assert len(set(self.values)) == len(self.values)
> + # Check for collisions on the generated C enum values
> + seen = {c_enum_const(self.name, 'MAX'): '(automatic MAX)'}
> + for value in self.values:
> + c_value = c_enum_const(self.name, value)
> + if c_value in seen:
> + # If the enum is implicit, report the error on behalf of
> + # the union or alternate that triggered the enum
> + if self.is_implicit():
> + owner = schema.lookup_type(self.name[:-4])
> + assert owner
> + if isinstance(owner, QAPISchemaAlternateType):
> + description = "Alternate '%s' branch" % owner.name
> + else:
> + description = "Union '%s' branch" % owner.name
> + else:
> + description = "Enum '%s' value" % self.name
Computing a reasonable description distracts from the checking job.
Suggest to outline this into a private method.
> + raise QAPIExprError(self.info,
> + "%s '%s' clashes with '%s'"
> + % (description, value, seen[c_value]))
> + seen[c_value] = value
>
> def is_implicit(self):
> # See QAPISchema._make_implicit_enum_type()
> @@ -1084,10 +1077,9 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
> # solely by their different tag_type
> def check(self, schema, info, tag_type, seen, union):
> # Check that the branch name does not collide with non-variant C
> - # members, without modifying caller's copy of seen
> - # TODO Detect collisions between branch names in C - easiest
> - # will be to check instead for collisions in the corresponding
> - # enum type
> + # members, without modifying caller's copy of seen. Collisions
> + # between branch names in C is detected elsewhere, when validating
> + # that the correpsonding enum of tag_type has no collision.
> QAPISchemaObjectTypeMember.check(self, schema, info, [], dict(seen))
> assert self.name in tag_type.values
>
> diff --git a/tests/Makefile b/tests/Makefile
> index 35a63f3..2cd5d31 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -331,7 +331,6 @@ 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-clash-branches.json
> qapi-schema += union-clash-data.json
> diff --git a/tests/qapi-schema/alternate-clash.err b/tests/qapi-schema/alternate-clash.err
> index a475ab6..7fd3069 100644
> --- a/tests/qapi-schema/alternate-clash.err
> +++ b/tests/qapi-schema/alternate-clash.err
> @@ -1 +1 @@
> -tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' member 'a_b' clashes with 'a-b'
> +tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' branch 'a_b' clashes with 'a-b'
Our terminology isn't consistent: we use both "branch" and "case". Not
this patch's problem to fix.
> diff --git a/tests/qapi-schema/enum-clash-member.err b/tests/qapi-schema/enum-clash-member.err
> index 48bd136..84030c5 100644
> --- a/tests/qapi-schema/enum-clash-member.err
> +++ b/tests/qapi-schema/enum-clash-member.err
> @@ -1 +1 @@
> -tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' member 'ONE' clashes with 'one'
> +tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' value 'ONE' clashes with 'one'
I actually prefer calling ONE a member of MyEnum, because that leaves
value for its actual value.
For what it's worth, the C standard also talks about "members of an
enumeration".
> diff --git a/tests/qapi-schema/enum-max-member.err b/tests/qapi-schema/enum-max-member.err
> index f77837f..6b9ef9b 100644
> --- a/tests/qapi-schema/enum-max-member.err
> +++ b/tests/qapi-schema/enum-max-member.err
> @@ -1 +1 @@
> -tests/qapi-schema/enum-max-member.json:3: Enum 'MyEnum' member 'max' clashes with '(automatic)'
> +tests/qapi-schema/enum-max-member.json:3: Enum 'MyEnum' value 'max' clashes with '(automatic MAX)'
(automatic MAX) is a slight improvement over (automatic).
> diff --git a/tests/qapi-schema/union-bad-branch.err b/tests/qapi-schema/union-bad-branch.err
> deleted file mode 100644
> index 8822735..0000000
> --- a/tests/qapi-schema/union-bad-branch.err
> +++ /dev/null
> @@ -1 +0,0 @@
> -tests/qapi-schema/union-bad-branch.json:6: Union 'MyUnion' member 'ONE' clashes with 'one'
> diff --git a/tests/qapi-schema/union-bad-branch.exit b/tests/qapi-schema/union-bad-branch.exit
> deleted file mode 100644
> index d00491f..0000000
> --- a/tests/qapi-schema/union-bad-branch.exit
> +++ /dev/null
> @@ -1 +0,0 @@
> -1
> diff --git a/tests/qapi-schema/union-bad-branch.json b/tests/qapi-schema/union-bad-branch.json
> deleted file mode 100644
> index 913aa38..0000000
> --- a/tests/qapi-schema/union-bad-branch.json
> +++ /dev/null
> @@ -1,8 +0,0 @@
> -# we reject normal unions where branches would collide in C
> -{ 'struct': 'One',
> - 'data': { 'string': 'str' } }
> -{ 'struct': 'Two',
> - 'data': { 'number': 'int' } }
> -{ 'union': 'MyUnion',
> - 'data': { 'one': 'One',
> - 'ONE': 'Two' } }
> diff --git a/tests/qapi-schema/union-bad-branch.out b/tests/qapi-schema/union-bad-branch.out
> deleted file mode 100644
> index e69de29..0000000
> diff --git a/tests/qapi-schema/union-clash-branches.err b/tests/qapi-schema/union-clash-branches.err
> index 005c48d..d8f1265 100644
> --- a/tests/qapi-schema/union-clash-branches.err
> +++ b/tests/qapi-schema/union-clash-branches.err
> @@ -1 +1 @@
> -tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' member 'a_b' clashes with 'a-b'
> +tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' branch 'a_b' clashes with 'a-b'
> diff --git a/tests/qapi-schema/union-clash-type.err b/tests/qapi-schema/union-clash-type.err
> index a5dead1..ce7f8cd 100644
> --- a/tests/qapi-schema/union-clash-type.err
> +++ b/tests/qapi-schema/union-clash-type.err
> @@ -1 +1 @@
> -tests/qapi-schema/union-clash-type.json:8: Union 'TestUnion' member 'kind' clashes with '(automatic)'
> +tests/qapi-schema/union-clash-type.json:8: 'kind' (branch of TestUnion) collides with 'kind' (implicit tag of TestUnion)
> diff --git a/tests/qapi-schema/union-max.err b/tests/qapi-schema/union-max.err
> index 55ce439..b93beae 100644
> --- a/tests/qapi-schema/union-max.err
> +++ b/tests/qapi-schema/union-max.err
> @@ -1 +1 @@
> -tests/qapi-schema/union-max.json:2: Union 'Union' member 'max' clashes with '(automatic)'
> +tests/qapi-schema/union-max.json:2: Union 'Union' branch 'max' clashes with '(automatic MAX)'
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 16/18] qapi: Move duplicate enum value checks to schema check()
2015-10-13 18:35 ` Markus Armbruster
@ 2015-10-13 19:37 ` Eric Blake
0 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2015-10-13 19:37 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 3825 bytes --]
On 10/13/2015 12:35 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Similar to the previous commit, move the detection of a collision
>> in enum values from parse time to QAPISchemaEnumType.check().
>> This happens to also detect collisions in union branch names,
>> so for a decent error message, we have to determine if the enum
>> is implicit (and if so where the real collision lies).
>>
>> Testing this showed that the test union-bad-branch and
>> union-clash-branches were basically testing the same thing;
>> with the minor difference that the former clashes only in the
>> enum, while the latter also clashes in the C union member
>> names that would be generated. So delete the weaker test.
>>
>> No change to generated code.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> +++ b/scripts/qapi.py
>> @@ -527,7 +527,6 @@ def check_union(expr, expr_info):
>> base = expr.get('base')
>> discriminator = expr.get('discriminator')
>> members = expr['data']
>> - values = {'MAX': '(automatic)', 'KIND': '(automatic)'}
>
> Stupid / tired question: I can see 'MAX' in the new code further down,
> but I can't see 'KIND'. Why? Was it perhaps covered by the previous
> patch?
MAX is covered by enum collisions. 'kind' is covered by the previous
patch's non-variant vs. branch-name collisions.
Hmm, maybe when respinning this I can simplify 15/18 to drop KIND from
this spot (because KIND is not part of the enum).
>>
>> def check(self, schema):
>> - assert len(set(self.values)) == len(self.values)
>> + # Check for collisions on the generated C enum values
>> + seen = {c_enum_const(self.name, 'MAX'): '(automatic MAX)'}
>> + for value in self.values:
>> + c_value = c_enum_const(self.name, value)
>> + if c_value in seen:
>> + # If the enum is implicit, report the error on behalf of
>> + # the union or alternate that triggered the enum
>> + if self.is_implicit():
>> + owner = schema.lookup_type(self.name[:-4])
>> + assert owner
>> + if isinstance(owner, QAPISchemaAlternateType):
>> + description = "Alternate '%s' branch" % owner.name
>> + else:
>> + description = "Union '%s' branch" % owner.name
>> + else:
>> + description = "Enum '%s' value" % self.name
>
> Computing a reasonable description distracts from the checking job.
> Suggest to outline this into a private method.
Good idea.
>> +++ b/tests/qapi-schema/alternate-clash.err
>> @@ -1 +1 @@
>> -tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' member 'a_b' clashes with 'a-b'
>> +tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' branch 'a_b' clashes with 'a-b'
>
> Our terminology isn't consistent: we use both "branch" and "case". Not
> this patch's problem to fix.
>
>> diff --git a/tests/qapi-schema/enum-clash-member.err b/tests/qapi-schema/enum-clash-member.err
>> index 48bd136..84030c5 100644
>> --- a/tests/qapi-schema/enum-clash-member.err
>> +++ b/tests/qapi-schema/enum-clash-member.err
>> @@ -1 +1 @@
>> -tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' member 'ONE' clashes with 'one'
>> +tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' value 'ONE' clashes with 'one'
>
> I actually prefer calling ONE a member of MyEnum, because that leaves
> value for its actual value.
>
> For what it's worth, the C standard also talks about "members of an
> enumeration".
Easy enough to fix.
--
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] 51+ messages in thread
* [Qemu-devel] [PATCH v8 17/18] qapi: Add test for alternate branch 'kind' clash
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
` (15 preceding siblings ...)
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 16/18] qapi: Move duplicate enum value " Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-13 18:43 ` Markus Armbruster
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 18/18] qapi: Detect base class loops Eric Blake
` (2 subsequent siblings)
19 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Rename alternate-clash to alternate-clash-members, and add a
new test alternate-clash-type. While similar to the earlier
addition of union-clash-type, we have one major difference: a
future patch will be simplifying alternates to not need an
implict AlternateKind enum, but we still need to detect the
collision with the resulting C 'qtype_code type;' tag.
No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: no change
v7: retitle 10/12 and limit to just testsuite changes
v6: New patch
---
tests/Makefile | 3 ++-
tests/qapi-schema/alternate-clash-members.err | 1 +
.../{alternate-clash.exit => alternate-clash-members.exit} | 0
.../{alternate-clash.json => alternate-clash-members.json} | 0
.../{alternate-clash.out => alternate-clash-members.out} | 0
tests/qapi-schema/alternate-clash-type.err | 1 +
tests/qapi-schema/alternate-clash-type.exit | 1 +
tests/qapi-schema/alternate-clash-type.json | 10 ++++++++++
tests/qapi-schema/alternate-clash-type.out | 0
tests/qapi-schema/alternate-clash.err | 1 -
10 files changed, 15 insertions(+), 2 deletions(-)
create mode 100644 tests/qapi-schema/alternate-clash-members.err
rename tests/qapi-schema/{alternate-clash.exit => alternate-clash-members.exit} (100%)
rename tests/qapi-schema/{alternate-clash.json => alternate-clash-members.json} (100%)
rename tests/qapi-schema/{alternate-clash.out => alternate-clash-members.out} (100%)
create mode 100644 tests/qapi-schema/alternate-clash-type.err
create mode 100644 tests/qapi-schema/alternate-clash-type.exit
create mode 100644 tests/qapi-schema/alternate-clash-type.json
create mode 100644 tests/qapi-schema/alternate-clash-type.out
delete mode 100644 tests/qapi-schema/alternate-clash.err
diff --git a/tests/Makefile b/tests/Makefile
index 2cd5d31..443e345 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -226,7 +226,8 @@ check-qtest-generic-y += tests/qom-test$(EXESUF)
qapi-schema += alternate-array.json
qapi-schema += alternate-base.json
-qapi-schema += alternate-clash.json
+qapi-schema += alternate-clash-members.json
+qapi-schema += alternate-clash-type.json
qapi-schema += alternate-conflict-dict.json
qapi-schema += alternate-conflict-string.json
qapi-schema += alternate-empty.json
diff --git a/tests/qapi-schema/alternate-clash-members.err b/tests/qapi-schema/alternate-clash-members.err
new file mode 100644
index 0000000..0adf737
--- /dev/null
+++ b/tests/qapi-schema/alternate-clash-members.err
@@ -0,0 +1 @@
+tests/qapi-schema/alternate-clash-members.json:7: Alternate 'Alt1' branch 'a_b' clashes with 'a-b'
diff --git a/tests/qapi-schema/alternate-clash.exit b/tests/qapi-schema/alternate-clash-members.exit
similarity index 100%
rename from tests/qapi-schema/alternate-clash.exit
rename to tests/qapi-schema/alternate-clash-members.exit
diff --git a/tests/qapi-schema/alternate-clash.json b/tests/qapi-schema/alternate-clash-members.json
similarity index 100%
rename from tests/qapi-schema/alternate-clash.json
rename to tests/qapi-schema/alternate-clash-members.json
diff --git a/tests/qapi-schema/alternate-clash.out b/tests/qapi-schema/alternate-clash-members.out
similarity index 100%
rename from tests/qapi-schema/alternate-clash.out
rename to tests/qapi-schema/alternate-clash-members.out
diff --git a/tests/qapi-schema/alternate-clash-type.err b/tests/qapi-schema/alternate-clash-type.err
new file mode 100644
index 0000000..cdd2090
--- /dev/null
+++ b/tests/qapi-schema/alternate-clash-type.err
@@ -0,0 +1 @@
+tests/qapi-schema/alternate-clash-type.json:9: 'kind' (branch of Alt1) collides with 'kind' (implicit tag of Alt1)
diff --git a/tests/qapi-schema/alternate-clash-type.exit b/tests/qapi-schema/alternate-clash-type.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/alternate-clash-type.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/alternate-clash-type.json b/tests/qapi-schema/alternate-clash-type.json
new file mode 100644
index 0000000..629584b
--- /dev/null
+++ b/tests/qapi-schema/alternate-clash-type.json
@@ -0,0 +1,10 @@
+# Alternate branch 'type'
+# Reject this, because we would have a clash in generated C, between the
+# alternate's implicit tag member 'kind' and the branch name 'kind'
+# within the alternate.
+# TODO: Even if alternates are simplified in the future to use a simpler
+# 'qtype_code type' tag, rather than a full QAPISchemaObjectTypeMember,
+# we must still flag the collision, or else munge the generated C branch
+# names to allow compilation.
+{ 'alternate': 'Alt1',
+ 'data': { 'kind': 'str', 'type': 'int' } }
diff --git a/tests/qapi-schema/alternate-clash-type.out b/tests/qapi-schema/alternate-clash-type.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/alternate-clash.err b/tests/qapi-schema/alternate-clash.err
deleted file mode 100644
index 7fd3069..0000000
--- a/tests/qapi-schema/alternate-clash.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' branch 'a_b' clashes with 'a-b'
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 17/18] qapi: Add test for alternate branch 'kind' clash
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 17/18] qapi: Add test for alternate branch 'kind' clash Eric Blake
@ 2015-10-13 18:43 ` Markus Armbruster
2015-10-13 19:42 ` Eric Blake
0 siblings, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2015-10-13 18:43 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Rename alternate-clash to alternate-clash-members, and add a
> new test alternate-clash-type. While similar to the earlier
> addition of union-clash-type, we have one major difference: a
> future patch will be simplifying alternates to not need an
> implict AlternateKind enum, but we still need to detect the
> collision with the resulting C 'qtype_code type;' tag.
You're alluding to a future change of the generated code from
struct BlockdevRef {
BlockdevRefKind kind;
union { /* union tag is @kind */
void *data;
BlockdevOptions *definition;
char *reference;
};
};
to
struct BlockdevRef {
qtype_code type;
union { /* union tag is @type */
void *data;
BlockdevOptions *definition; /* QTYPE_QDICT */
char *reference; /* QTYPE_QSTRING */
};
};
right?
I don't think that affects collision checking at all. Both before and
after, we have a tag member, and we need to check for collisions with
its name.
> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v8: no change
> v7: retitle 10/12 and limit to just testsuite changes
> v6: New patch
> ---
> tests/Makefile | 3 ++-
> tests/qapi-schema/alternate-clash-members.err | 1 +
> .../{alternate-clash.exit => alternate-clash-members.exit} | 0
> .../{alternate-clash.json => alternate-clash-members.json} | 0
> .../{alternate-clash.out => alternate-clash-members.out} | 0
> tests/qapi-schema/alternate-clash-type.err | 1 +
> tests/qapi-schema/alternate-clash-type.exit | 1 +
> tests/qapi-schema/alternate-clash-type.json | 10 ++++++++++
> tests/qapi-schema/alternate-clash-type.out | 0
> tests/qapi-schema/alternate-clash.err | 1 -
> 10 files changed, 15 insertions(+), 2 deletions(-)
> create mode 100644 tests/qapi-schema/alternate-clash-members.err
> rename tests/qapi-schema/{alternate-clash.exit =>
> alternate-clash-members.exit} (100%)
> rename tests/qapi-schema/{alternate-clash.json =>
> alternate-clash-members.json} (100%)
> rename tests/qapi-schema/{alternate-clash.out =>
> alternate-clash-members.out} (100%)
> create mode 100644 tests/qapi-schema/alternate-clash-type.err
> create mode 100644 tests/qapi-schema/alternate-clash-type.exit
> create mode 100644 tests/qapi-schema/alternate-clash-type.json
> create mode 100644 tests/qapi-schema/alternate-clash-type.out
> delete mode 100644 tests/qapi-schema/alternate-clash.err
>
> diff --git a/tests/Makefile b/tests/Makefile
> index 2cd5d31..443e345 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -226,7 +226,8 @@ check-qtest-generic-y += tests/qom-test$(EXESUF)
>
> qapi-schema += alternate-array.json
> qapi-schema += alternate-base.json
> -qapi-schema += alternate-clash.json
> +qapi-schema += alternate-clash-members.json
> +qapi-schema += alternate-clash-type.json
> qapi-schema += alternate-conflict-dict.json
> qapi-schema += alternate-conflict-string.json
> qapi-schema += alternate-empty.json
> diff --git a/tests/qapi-schema/alternate-clash-members.err
> b/tests/qapi-schema/alternate-clash-members.err
> new file mode 100644
> index 0000000..0adf737
> --- /dev/null
> +++ b/tests/qapi-schema/alternate-clash-members.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/alternate-clash-members.json:7: Alternate 'Alt1'
> branch 'a_b' clashes with 'a-b'
> diff --git a/tests/qapi-schema/alternate-clash.exit
> b/tests/qapi-schema/alternate-clash-members.exit
> similarity index 100%
> rename from tests/qapi-schema/alternate-clash.exit
> rename to tests/qapi-schema/alternate-clash-members.exit
> diff --git a/tests/qapi-schema/alternate-clash.json
> b/tests/qapi-schema/alternate-clash-members.json
> similarity index 100%
> rename from tests/qapi-schema/alternate-clash.json
> rename to tests/qapi-schema/alternate-clash-members.json
> diff --git a/tests/qapi-schema/alternate-clash.out
> b/tests/qapi-schema/alternate-clash-members.out
> similarity index 100%
> rename from tests/qapi-schema/alternate-clash.out
> rename to tests/qapi-schema/alternate-clash-members.out
> diff --git a/tests/qapi-schema/alternate-clash-type.err
> b/tests/qapi-schema/alternate-clash-type.err
> new file mode 100644
> index 0000000..cdd2090
> --- /dev/null
> +++ b/tests/qapi-schema/alternate-clash-type.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/alternate-clash-type.json:9: 'kind' (branch of
> Alt1) collides with 'kind' (implicit tag of Alt1)
> diff --git a/tests/qapi-schema/alternate-clash-type.exit
> b/tests/qapi-schema/alternate-clash-type.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/alternate-clash-type.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/alternate-clash-type.json
> b/tests/qapi-schema/alternate-clash-type.json
> new file mode 100644
> index 0000000..629584b
> --- /dev/null
> +++ b/tests/qapi-schema/alternate-clash-type.json
> @@ -0,0 +1,10 @@
> +# Alternate branch 'type'
> +# Reject this, because we would have a clash in generated C, between the
> +# alternate's implicit tag member 'kind' and the branch name 'kind'
> +# within the alternate.
> +# TODO: Even if alternates are simplified in the future to use a simpler
> +# 'qtype_code type' tag, rather than a full QAPISchemaObjectTypeMember,
> +# we must still flag the collision, or else munge the generated C branch
> +# names to allow compilation.
I don't think there's a TODO here.
> +{ 'alternate': 'Alt1',
> + 'data': { 'kind': 'str', 'type': 'int' } }
> diff --git a/tests/qapi-schema/alternate-clash-type.out
> b/tests/qapi-schema/alternate-clash-type.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/alternate-clash.err
> b/tests/qapi-schema/alternate-clash.err
> deleted file mode 100644
> index 7fd3069..0000000
> --- a/tests/qapi-schema/alternate-clash.err
> +++ /dev/null
> @@ -1 +0,0 @@
> -tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' branch
> 'a_b' clashes with 'a-b'
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 17/18] qapi: Add test for alternate branch 'kind' clash
2015-10-13 18:43 ` Markus Armbruster
@ 2015-10-13 19:42 ` Eric Blake
0 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2015-10-13 19:42 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2369 bytes --]
On 10/13/2015 12:43 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Rename alternate-clash to alternate-clash-members, and add a
>> new test alternate-clash-type. While similar to the earlier
>> addition of union-clash-type, we have one major difference: a
>> future patch will be simplifying alternates to not need an
>> implict AlternateKind enum, but we still need to detect the
>> collision with the resulting C 'qtype_code type;' tag.
>
> You're alluding to a future change of the generated code from
>
> struct BlockdevRef {
> BlockdevRefKind kind;
> union { /* union tag is @kind */
> void *data;
> BlockdevOptions *definition;
> char *reference;
> };
> };
>
> to
>
> struct BlockdevRef {
> qtype_code type;
> union { /* union tag is @type */
> void *data;
> BlockdevOptions *definition; /* QTYPE_QDICT */
> char *reference; /* QTYPE_QSTRING */
> };
> };
>
> right?
Yes.
>
> I don't think that affects collision checking at all. Both before and
> after, we have a tag member, and we need to check for collisions with
> its name.
I guess I wrote that at one point where I was using
alternate.variants.tag_member = None for alternates; but in the
meantime, I've reworked things to just use a special subclass of
QAPISchemaObjectTypeMember instead, at which point normal collision
checking still works. So yeah, I can probably simplify or drop wording
here.
>> +++ b/tests/qapi-schema/alternate-clash-type.json
>> @@ -0,0 +1,10 @@
>> +# Alternate branch 'type'
>> +# Reject this, because we would have a clash in generated C, between the
>> +# alternate's implicit tag member 'kind' and the branch name 'kind'
>> +# within the alternate.
>> +# TODO: Even if alternates are simplified in the future to use a simpler
>> +# 'qtype_code type' tag, rather than a full QAPISchemaObjectTypeMember,
>> +# we must still flag the collision, or else munge the generated C branch
>> +# names to allow compilation.
>
> I don't think there's a TODO here.
Again, probably leftover text from my first implementation.
--
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] 51+ messages in thread
* [Qemu-devel] [PATCH v8 18/18] qapi: Detect base class loops
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
` (16 preceding siblings ...)
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 17/18] qapi: Add test for alternate branch 'kind' clash Eric Blake
@ 2015-10-13 4:22 ` Eric Blake
2015-10-13 18:26 ` [Qemu-devel] [PATCH v8 06.5/18] qapi: Drop redundant args-member-array test Eric Blake
2015-10-13 18:46 ` [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Markus Armbruster
19 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2015-10-13 4:22 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
It should be fairly obvious that qapi base classes need to
form an acyclic graph, since QMP cannot specify the same
key more than once, while base classes are included as flat
members alongside other members added by the child. But the
old check_member_clash() parser function was not prepared to
check for this, and entered an infinite recursion (at least
until python gives up, complaining about nesting too deep).
Now that check_member_clash() has been recently removed,
attempts at self-inheritance trigger an assertion failure
introduced by commit ac88219a. The obvious fix is to turn
the assertion into a conditional.
This patch includes both the test and the fix, since the .err
file output for the unfixed case is not useful (particularly
when it was warning about unbounded recursion, as that limit
may be platform-specific).
We don't need to worry about cycles in flat unions (neither
the base nor a variant class can be a union) nor in alternates
(alternate branches cannot themselves be an alternate).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: improve commit message
v7: improve commit message
v6: rebase to earlier info changes
---
scripts/qapi.py | 6 +++++-
tests/Makefile | 1 +
tests/qapi-schema/base-cycle.err | 1 +
tests/qapi-schema/base-cycle.exit | 1 +
tests/qapi-schema/base-cycle.json | 3 +++
tests/qapi-schema/base-cycle.out | 0
6 files changed, 11 insertions(+), 1 deletion(-)
create mode 100644 tests/qapi-schema/base-cycle.err
create mode 100644 tests/qapi-schema/base-cycle.exit
create mode 100644 tests/qapi-schema/base-cycle.json
create mode 100644 tests/qapi-schema/base-cycle.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index b21e38e..d6825ce 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -927,7 +927,11 @@ class QAPISchemaObjectType(QAPISchemaType):
self.members = None
def check(self, schema):
- assert self.members is not False # not running in cycles
+ if self.members is False: # check for cycles
+ assert self._base_name
+ raise QAPIExprError(self.info,
+ "Object %s cyclically depends on %s"
+ % (self.name, self._base_name))
if self.members:
return
self.members = False # mark as being checked
diff --git a/tests/Makefile b/tests/Makefile
index 443e345..1ef44aa 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -251,6 +251,7 @@ 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 += base-cycle.json
qapi-schema += command-int.json
qapi-schema += comments.json
qapi-schema += double-data.json
diff --git a/tests/qapi-schema/base-cycle.err b/tests/qapi-schema/base-cycle.err
new file mode 100644
index 0000000..e0221b5
--- /dev/null
+++ b/tests/qapi-schema/base-cycle.err
@@ -0,0 +1 @@
+tests/qapi-schema/base-cycle.json:2: Object Base1 cyclically depends on Base2
diff --git a/tests/qapi-schema/base-cycle.exit b/tests/qapi-schema/base-cycle.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/base-cycle.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/base-cycle.json b/tests/qapi-schema/base-cycle.json
new file mode 100644
index 0000000..2866772
--- /dev/null
+++ b/tests/qapi-schema/base-cycle.json
@@ -0,0 +1,3 @@
+# we reject a loop in base classes
+{ 'struct': 'Base1', 'base': 'Base2', 'data': {} }
+{ 'struct': 'Base2', 'base': 'Base1', 'data': {} }
diff --git a/tests/qapi-schema/base-cycle.out b/tests/qapi-schema/base-cycle.out
new file mode 100644
index 0000000..e69de29
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* [Qemu-devel] [PATCH v8 06.5/18] qapi: Drop redundant args-member-array test
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
` (17 preceding siblings ...)
2015-10-13 4:22 ` [Qemu-devel] [PATCH v8 18/18] qapi: Detect base class loops Eric Blake
@ 2015-10-13 18:26 ` Eric Blake
2015-10-13 18:51 ` Markus Armbruster
2015-10-13 18:46 ` [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Markus Armbruster
19 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2015-10-13 18:26 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru
qapi-schema-test already ensures that we can correctly compile
an array of enums (__org.qemu_x-command), an array of builtins
(UserDefNativeListUnion), and an array of structs (again
__org.qemu_x-command). That means args-member-array is not
adding any additional parse-only test coverage, so drop it.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v8: new patch, but in response to the same review of v7 that created
patches 4-6, so we might as well put it in the qapi tree now
---
tests/Makefile | 1 -
tests/qapi-schema/args-member-array.err | 0
tests/qapi-schema/args-member-array.exit | 1 -
tests/qapi-schema/args-member-array.json | 4 ----
tests/qapi-schema/args-member-array.out | 9 ---------
5 files changed, 15 deletions(-)
delete mode 100644 tests/qapi-schema/args-member-array.err
delete mode 100644 tests/qapi-schema/args-member-array.exit
delete mode 100644 tests/qapi-schema/args-member-array.json
delete mode 100644 tests/qapi-schema/args-member-array.out
diff --git a/tests/Makefile b/tests/Makefile
index 35a63f3..cb221de 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -239,7 +239,6 @@ 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-name-clash.json
qapi-schema += args-union.json
diff --git a/tests/qapi-schema/args-member-array.err b/tests/qapi-schema/args-member-array.err
deleted file mode 100644
index e69de29..0000000
diff --git a/tests/qapi-schema/args-member-array.exit b/tests/qapi-schema/args-member-array.exit
deleted file mode 100644
index 573541a..0000000
--- a/tests/qapi-schema/args-member-array.exit
+++ /dev/null
@@ -1 +0,0 @@
-0
diff --git a/tests/qapi-schema/args-member-array.json b/tests/qapi-schema/args-member-array.json
deleted file mode 100644
index e6f7f5d..0000000
--- a/tests/qapi-schema/args-member-array.json
+++ /dev/null
@@ -1,4 +0,0 @@
-# valid array members
-{ 'enum': 'abc', 'data': [ 'a', 'b', 'c' ] }
-{ 'struct': 'def', 'data': { 'array': [ 'abc' ] } }
-{ 'command': 'okay', 'data': { 'member1': [ 'int' ], 'member2': [ 'def' ] } }
diff --git a/tests/qapi-schema/args-member-array.out b/tests/qapi-schema/args-member-array.out
deleted file mode 100644
index b3b92df..0000000
--- a/tests/qapi-schema/args-member-array.out
+++ /dev/null
@@ -1,9 +0,0 @@
-object :empty
-object :obj-okay-arg
- member member1: intList optional=False
- member member2: defList optional=False
-enum abc ['a', 'b', 'c']
-object def
- member array: abcList optional=False
-command okay :obj-okay-arg -> None
- gen=True success_response=True
--
2.4.3
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [Qemu-devel] [PATCH v8 06.5/18] qapi: Drop redundant args-member-array test
2015-10-13 18:26 ` [Qemu-devel] [PATCH v8 06.5/18] qapi: Drop redundant args-member-array test Eric Blake
@ 2015-10-13 18:51 ` Markus Armbruster
0 siblings, 0 replies; 51+ messages in thread
From: Markus Armbruster @ 2015-10-13 18:51 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
Eric Blake <eblake@redhat.com> writes:
> qapi-schema-test already ensures that we can correctly compile
> an array of enums (__org.qemu_x-command), an array of builtins
> (UserDefNativeListUnion), and an array of structs (again
> __org.qemu_x-command). That means args-member-array is not
> adding any additional parse-only test coverage, so drop it.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v8: new patch, but in response to the same review of v7 that created
> patches 4-6, so we might as well put it in the qapi tree now
Spliced into qapi-next & pushed, thanks!
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B
2015-10-13 4:22 [Qemu-devel] [PATCH v8 00/18] post-introspection cleanups, subset B Eric Blake
` (18 preceding siblings ...)
2015-10-13 18:26 ` [Qemu-devel] [PATCH v8 06.5/18] qapi: Drop redundant args-member-array test Eric Blake
@ 2015-10-13 18:46 ` Markus Armbruster
19 siblings, 0 replies; 51+ messages in thread
From: Markus Armbruster @ 2015-10-13 18:46 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
Eric Blake <eblake@redhat.com> writes:
> Pending prerequisite: Markus' qapi-next branch (which has my
> subset A patches):
> git://repo.or.cz/qemu/armbru.git pull-qapi-2015-10-12
> https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02796.html
>
> Also available as a tag at this location:
> git fetch git://repo.or.cz/qemu/ericb.git qapi-cleanupv8b
>
> and I plan to eventually forcefully update my branch with the rest
> of the v5 series, at:
> http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi
Okay, I got through subset B. First time, actually. Progress!
^ permalink raw reply [flat|nested] 51+ messages in thread