* [Qemu-devel] [PATCH v11 01/24] tests/qapi-schema: Test for reserved names, empty struct
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 02/24] qapi: More idiomatic string operations Eric Blake
` (23 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Add some testsuite coverage to ensure future patches are on
the right track:
Our current C representation of qapi arrays is done by appending
'List' to the element name; but we are not preventing the
creation of an object type with the same name. Add
reserved-type-list.json to test this. Then rename
enum-union-clash.json to reserved-type-kind.json to cover the
reservation that we DO detect, and shorten it to match the fact
that the name is reserved even if there is no clash.
We are failing to detect a collision between a QMP member and
the implicit 'has_*' flag for another optional QMP member. The
easiest fix would be for a future patch to reserve the entire
"has[-_]" namespace for member names (the collision is also
possible for branch names within flat unions, but only as long
as branch names can collide with QMP names; however, since
future patches are about to remove that, it is not worth
testing here). Add reserved-member-has.json to test this.
A similar collision exists between a QMP member where c_name()
munges what might otherwise be a reserved name to start with
'q_', and another member explicitly starts with "q[-_]". Again,
the easiest solution for a future patch will be reserving the
entire namespace, but here for commands as well as members.
Add reserved-member-q.json and reserved-command-q.json to test
this; separate tests since arguably our munging of command
'unix' to 'qmp_q_unix()' could be done without a q_, which is
different than the munging of a member 'unix' to 'foo.q_unix'.
Finally, our testsuite does not have any compilation coverage
of struct inheritance with empty qapi structs. Update
qapi-schema-test.json to test this.
Note that there is currently no technical reason to forbid type
name patterns from member names, or member name patterns from
types, since the two are not in the same namespace in C and
won't collide; but it's not worth adding positive tests of these
corner cases at this time, especially while there is other churn
pending in patches that rearrange which collisions actually
happen.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: wording fixes in tests, updated commit message, rename
test cases to consistent reserved-WHERE-WHAT.json naming
v10: retitle, split off 'u' collisions and positive tests for
later, add 'q_' collisions and empty struct inheritance, improve
commit message, rename args-name-has to args-has-clash
v9: new patch
---
tests/Makefile | 6 +++++-
tests/qapi-schema/enum-union-clash.err | 1 -
tests/qapi-schema/qapi-schema-test.json | 4 ++++
tests/qapi-schema/qapi-schema-test.out | 3 +++
tests/qapi-schema/{enum-union-clash.out => reserved-command-q.err} | 0
tests/qapi-schema/reserved-command-q.exit | 1 +
tests/qapi-schema/reserved-command-q.json | 7 +++++++
tests/qapi-schema/reserved-command-q.out | 5 +++++
tests/qapi-schema/reserved-member-has.err | 0
tests/qapi-schema/reserved-member-has.exit | 1 +
tests/qapi-schema/reserved-member-has.json | 6 ++++++
tests/qapi-schema/reserved-member-has.out | 6 ++++++
tests/qapi-schema/reserved-member-q.err | 0
tests/qapi-schema/reserved-member-q.exit | 1 +
tests/qapi-schema/reserved-member-q.json | 6 ++++++
tests/qapi-schema/reserved-member-q.out | 4 ++++
tests/qapi-schema/reserved-type-kind.err | 1 +
.../qapi-schema/{enum-union-clash.exit => reserved-type-kind.exit} | 0
.../qapi-schema/{enum-union-clash.json => reserved-type-kind.json} | 2 --
tests/qapi-schema/reserved-type-kind.out | 0
tests/qapi-schema/reserved-type-list.err | 0
tests/qapi-schema/reserved-type-list.exit | 1 +
tests/qapi-schema/reserved-type-list.json | 5 +++++
tests/qapi-schema/reserved-type-list.out | 3 +++
24 files changed, 59 insertions(+), 4 deletions(-)
delete mode 100644 tests/qapi-schema/enum-union-clash.err
rename tests/qapi-schema/{enum-union-clash.out => reserved-command-q.err} (100%)
create mode 100644 tests/qapi-schema/reserved-command-q.exit
create mode 100644 tests/qapi-schema/reserved-command-q.json
create mode 100644 tests/qapi-schema/reserved-command-q.out
create mode 100644 tests/qapi-schema/reserved-member-has.err
create mode 100644 tests/qapi-schema/reserved-member-has.exit
create mode 100644 tests/qapi-schema/reserved-member-has.json
create mode 100644 tests/qapi-schema/reserved-member-has.out
create mode 100644 tests/qapi-schema/reserved-member-q.err
create mode 100644 tests/qapi-schema/reserved-member-q.exit
create mode 100644 tests/qapi-schema/reserved-member-q.json
create mode 100644 tests/qapi-schema/reserved-member-q.out
create mode 100644 tests/qapi-schema/reserved-type-kind.err
rename tests/qapi-schema/{enum-union-clash.exit => reserved-type-kind.exit} (100%)
rename tests/qapi-schema/{enum-union-clash.json => reserved-type-kind.json} (69%)
create mode 100644 tests/qapi-schema/reserved-type-kind.out
create mode 100644 tests/qapi-schema/reserved-type-list.err
create mode 100644 tests/qapi-schema/reserved-type-list.exit
create mode 100644 tests/qapi-schema/reserved-type-list.json
create mode 100644 tests/qapi-schema/reserved-type-list.out
diff --git a/tests/Makefile b/tests/Makefile
index 1c57e39..54022d6 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -265,7 +265,6 @@ qapi-schema += enum-dict-member.json
qapi-schema += enum-int-member.json
qapi-schema += enum-max-member.json
qapi-schema += enum-missing-data.json
-qapi-schema += enum-union-clash.json
qapi-schema += enum-wrong-data.json
qapi-schema += escape-outside-string.json
qapi-schema += escape-too-big.json
@@ -316,6 +315,11 @@ qapi-schema += redefined-builtin.json
qapi-schema += redefined-command.json
qapi-schema += redefined-event.json
qapi-schema += redefined-type.json
+qapi-schema += reserved-command-q.json
+qapi-schema += reserved-member-has.json
+qapi-schema += reserved-member-q.json
+qapi-schema += reserved-type-kind.json
+qapi-schema += reserved-type-list.json
qapi-schema += returns-alternate.json
qapi-schema += returns-array-bad.json
qapi-schema += returns-dict.json
diff --git a/tests/qapi-schema/enum-union-clash.err b/tests/qapi-schema/enum-union-clash.err
deleted file mode 100644
index c04e1a8..0000000
--- a/tests/qapi-schema/enum-union-clash.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/enum-union-clash.json:2: enum 'UnionKind' should not end in 'Kind'
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 4e2d7c2..48e104b 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -11,6 +11,10 @@
# An empty enum, although unusual, is currently acceptable
{ 'enum': 'MyEnum', 'data': [ ] }
+# Likewise for an empty struct, including an empty base
+{ 'struct': 'Empty1', 'data': { } }
+{ 'struct': 'Empty2', 'base': 'Empty1', '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 a6c80e0..a7e9aab 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -81,6 +81,9 @@ event EVENT_A None
event EVENT_B None
event EVENT_C :obj-EVENT_C-arg
event EVENT_D :obj-EVENT_D-arg
+object Empty1
+object Empty2
+ base Empty1
enum EnumOne ['value1', 'value2', 'value3']
object EventStructOne
member struct1: UserDefOne optional=False
diff --git a/tests/qapi-schema/enum-union-clash.out b/tests/qapi-schema/reserved-command-q.err
similarity index 100%
rename from tests/qapi-schema/enum-union-clash.out
rename to tests/qapi-schema/reserved-command-q.err
diff --git a/tests/qapi-schema/reserved-command-q.exit b/tests/qapi-schema/reserved-command-q.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/reserved-command-q.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/reserved-command-q.json b/tests/qapi-schema/reserved-command-q.json
new file mode 100644
index 0000000..be9944c
--- /dev/null
+++ b/tests/qapi-schema/reserved-command-q.json
@@ -0,0 +1,7 @@
+# C entity name collision
+# FIXME - This parses, but fails to compile, because it attempts to declare
+# two 'qmp_q_unix' functions (one for 'q-unix', the other because c_name()
+# munges 'unix' to 'q_unix' to avoid reserved word collisions). We should
+# reject attempts to explicitly use 'q_' names, to reserve it for qapi.
+{ 'command': 'unix' }
+{ 'command': 'q-unix' }
diff --git a/tests/qapi-schema/reserved-command-q.out b/tests/qapi-schema/reserved-command-q.out
new file mode 100644
index 0000000..b31b38f
--- /dev/null
+++ b/tests/qapi-schema/reserved-command-q.out
@@ -0,0 +1,5 @@
+object :empty
+command q-unix None -> None
+ gen=True success_response=True
+command unix None -> None
+ gen=True success_response=True
diff --git a/tests/qapi-schema/reserved-member-has.err b/tests/qapi-schema/reserved-member-has.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/reserved-member-has.exit b/tests/qapi-schema/reserved-member-has.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/reserved-member-has.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/reserved-member-has.json b/tests/qapi-schema/reserved-member-has.json
new file mode 100644
index 0000000..a2197de
--- /dev/null
+++ b/tests/qapi-schema/reserved-member-has.json
@@ -0,0 +1,6 @@
+# C member name collision
+# FIXME - This parses, but fails to compile, because the C struct is given
+# two 'has_a' members, one from the flag for optional 'a', and the other
+# from member 'has-a'. Either reject this at parse time, or munge the C
+# names to avoid the collision.
+{ 'command': 'oops', 'data': { '*a': 'str', 'has-a': 'str' } }
diff --git a/tests/qapi-schema/reserved-member-has.out b/tests/qapi-schema/reserved-member-has.out
new file mode 100644
index 0000000..5a18b6b
--- /dev/null
+++ b/tests/qapi-schema/reserved-member-has.out
@@ -0,0 +1,6 @@
+object :empty
+object :obj-oops-arg
+ member a: str optional=True
+ member has-a: str optional=False
+command oops :obj-oops-arg -> None
+ gen=True success_response=True
diff --git a/tests/qapi-schema/reserved-member-q.err b/tests/qapi-schema/reserved-member-q.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/reserved-member-q.exit b/tests/qapi-schema/reserved-member-q.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/reserved-member-q.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/reserved-member-q.json b/tests/qapi-schema/reserved-member-q.json
new file mode 100644
index 0000000..1602ed3
--- /dev/null
+++ b/tests/qapi-schema/reserved-member-q.json
@@ -0,0 +1,6 @@
+# C member name collision
+# FIXME - This parses, but fails to compile, because it attempts to declare
+# two 'q_unix' members (one for 'q-unix', the other because c_name()
+# munges 'unix' to 'q_unix' to avoid reserved word collisions). We should
+# reject attempts to explicitly use 'q_' names, to reserve it for qapi.
+{ 'struct': 'Foo', 'data': { 'unix':'int', 'q-unix':'bool' } }
diff --git a/tests/qapi-schema/reserved-member-q.out b/tests/qapi-schema/reserved-member-q.out
new file mode 100644
index 0000000..0d8685a
--- /dev/null
+++ b/tests/qapi-schema/reserved-member-q.out
@@ -0,0 +1,4 @@
+object :empty
+object Foo
+ member unix: int optional=False
+ member q-unix: bool optional=False
diff --git a/tests/qapi-schema/reserved-type-kind.err b/tests/qapi-schema/reserved-type-kind.err
new file mode 100644
index 0000000..0a38efa
--- /dev/null
+++ b/tests/qapi-schema/reserved-type-kind.err
@@ -0,0 +1 @@
+tests/qapi-schema/reserved-type-kind.json:2: enum 'UnionKind' should not end in 'Kind'
diff --git a/tests/qapi-schema/enum-union-clash.exit b/tests/qapi-schema/reserved-type-kind.exit
similarity index 100%
rename from tests/qapi-schema/enum-union-clash.exit
rename to tests/qapi-schema/reserved-type-kind.exit
diff --git a/tests/qapi-schema/enum-union-clash.json b/tests/qapi-schema/reserved-type-kind.json
similarity index 69%
rename from tests/qapi-schema/enum-union-clash.json
rename to tests/qapi-schema/reserved-type-kind.json
index 593282b..9ecaba1 100644
--- a/tests/qapi-schema/enum-union-clash.json
+++ b/tests/qapi-schema/reserved-type-kind.json
@@ -1,4 +1,2 @@
# we reject types that would conflict with implicit union enum
{ 'enum': 'UnionKind', 'data': [ 'oops' ] }
-{ 'union': 'Union',
- 'data': { 'a': 'int' } }
diff --git a/tests/qapi-schema/reserved-type-kind.out b/tests/qapi-schema/reserved-type-kind.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/reserved-type-list.err b/tests/qapi-schema/reserved-type-list.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/reserved-type-list.exit b/tests/qapi-schema/reserved-type-list.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/reserved-type-list.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/reserved-type-list.json b/tests/qapi-schema/reserved-type-list.json
new file mode 100644
index 0000000..5b7d0f9
--- /dev/null
+++ b/tests/qapi-schema/reserved-type-list.json
@@ -0,0 +1,5 @@
+# Potential C name collision
+# FIXME - This parses and compiles on its own, but prevents the user from
+# creating a type named 'Foo' and using ['Foo'] for an array. We should
+# reject the use of any type names ending in 'List'.
+{ 'struct': 'FooList', 'data': { 's': 'str' } }
diff --git a/tests/qapi-schema/reserved-type-list.out b/tests/qapi-schema/reserved-type-list.out
new file mode 100644
index 0000000..0406bfe
--- /dev/null
+++ b/tests/qapi-schema/reserved-type-list.out
@@ -0,0 +1,3 @@
+object :empty
+object FooList
+ member s: str optional=False
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 02/24] qapi: More idiomatic string operations
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 01/24] tests/qapi-schema: Test for reserved names, empty struct Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 03/24] qapi: More robust conditions for when labels are needed Eric Blake
` (22 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Rather than slicing the end of a string, we can use python's
endswith(). And rather than creating a set of characters,
we can search for a character within a string.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: no change
v10: new patch
---
scripts/qapi.py | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 9d53255..3af4c2c 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -172,7 +172,7 @@ class QAPISchemaParser(object):
if self.tok == '#':
self.cursor = self.src.find('\n', self.cursor)
- elif self.tok in ['{', '}', ':', ',', '[', ']']:
+ elif self.tok in "{}:,[]":
return
elif self.tok == "'":
string = ''
@@ -390,7 +390,7 @@ def add_name(name, info, meta, implicit=False):
raise QAPIExprError(info,
"%s '%s' is already defined"
% (all_names[name], name))
- if not implicit and name[-4:] == 'Kind':
+ if not implicit and name.endswith('Kind'):
raise QAPIExprError(info,
"%s '%s' should not end in 'Kind'"
% (meta, name))
@@ -910,7 +910,7 @@ class QAPISchemaEnumType(QAPISchemaType):
def is_implicit(self):
# See QAPISchema._make_implicit_enum_type()
- return self.name[-4:] == 'Kind'
+ return self.name.endswith('Kind')
def c_type(self, is_param=False):
return c_name(self.name)
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 03/24] qapi: More robust conditions for when labels are needed
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 01/24] tests/qapi-schema: Test for reserved names, empty struct Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 02/24] qapi: More idiomatic string operations Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 04/24] qapi: Reserve '*List' type names for list types Eric Blake
` (21 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
We were using regular expressions to see if ret included
any earlier text that emitted a 'goto out;' line, to decide
whether we needed to output an 'out:' label. But this is
fragile, if the ret text can possibly combine more than one
generated function body, where the first function used a
goto but the second does not. Change the code to just check
for the known conditions which cause an error check to be
needed. Besides, it's slightly more efficient to use plain
checks than regular expression searching.
No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: add comments
v10: new patch, split in part from 6/17
---
scripts/qapi-commands.py | 4 +++-
scripts/qapi-visit.py | 3 ++-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 43a893b..561e47a 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -175,7 +175,9 @@ def gen_marshal(name, arg_type, ret_type):
ret += gen_marshal_input_visit(arg_type)
ret += gen_call(name, arg_type, ret_type)
- if re.search('^ *goto out;', ret, re.MULTILINE):
+ # 'goto out' produced by gen_marshal_input_visit->gen_visit_fields()
+ # for each arg_type member, and by gen_call() for ret_type
+ if (arg_type and arg_type.members) or ret_type:
ret += mcgen('''
out:
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index d0759d7..2dc3aed 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -87,7 +87,8 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
ret += gen_visit_fields(members, prefix='(*obj)->')
- if re.search('^ *goto out;', ret, re.MULTILINE):
+ # 'goto out' produced for base, and by gen_visit_fields() for each member
+ if base or members:
ret += mcgen('''
out:
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 04/24] qapi: Reserve '*List' type names for list types
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (2 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 03/24] qapi: More robust conditions for when labels are needed Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 05/24] qapi: Reserve 'q_*' and 'has_*' member names Eric Blake
` (20 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Type names ending in 'List' can clash with qapi list types in
generated C. We don't currently use such names. It is easier to
outlaw them now than to worry about how to resolve such a clash
in the future. For precedence, see commit 4dc2e69, which did the
same for names ending in 'Kind' versus implicit enum types for
qapi unions.
Update the testsuite to match.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: shorter commit message, rebase to earlier changes
v10: improve commit message, including a retitle (qapi list types
may be JSON arrays, but they are C linked lists, not C arrays)
v9: new patch
---
docs/qapi-code-gen.txt | 3 ++-
scripts/qapi.py | 10 ++++------
tests/qapi-schema/reserved-type-list.err | 1 +
tests/qapi-schema/reserved-type-list.exit | 2 +-
tests/qapi-schema/reserved-type-list.json | 6 +++---
tests/qapi-schema/reserved-type-list.out | 3 ---
6 files changed, 11 insertions(+), 14 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 2afab20..c4264a8 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -106,7 +106,8 @@ Types, commands, and events share a common namespace. Therefore,
generally speaking, type definitions should always use CamelCase for
user-defined type names, while built-in types are lowercase. Type
definitions should not end in 'Kind', as this namespace is used for
-creating implicit C enums for visiting union types. Command names,
+creating implicit C enums for visiting union types, or in 'List', as
+this namespace is used for creating array types. Command names,
and field names within a type, should be all lower case with words
separated by a hyphen. However, some existing older commands and
complex types use underscore; when extending such expressions,
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 3af4c2c..d53b5c4 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -390,10 +390,10 @@ def add_name(name, info, meta, implicit=False):
raise QAPIExprError(info,
"%s '%s' is already defined"
% (all_names[name], name))
- if not implicit and name.endswith('Kind'):
+ if not implicit and (name.endswith('Kind') or name.endswith('List')):
raise QAPIExprError(info,
- "%s '%s' should not end in 'Kind'"
- % (meta, name))
+ "%s '%s' should not end in '%s'"
+ % (meta, name, name[-4:]))
all_names[name] = meta
@@ -1196,9 +1196,7 @@ class QAPISchema(object):
return name
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'
+ name = element_type + 'List' # Use namespace reserved by add_name()
if not self.lookup_type(name):
self._def_entity(QAPISchemaArrayType(name, info, element_type))
return name
diff --git a/tests/qapi-schema/reserved-type-list.err b/tests/qapi-schema/reserved-type-list.err
index e69de29..4510fa6 100644
--- a/tests/qapi-schema/reserved-type-list.err
+++ b/tests/qapi-schema/reserved-type-list.err
@@ -0,0 +1 @@
+tests/qapi-schema/reserved-type-list.json:5: struct 'FooList' should not end in 'List'
diff --git a/tests/qapi-schema/reserved-type-list.exit b/tests/qapi-schema/reserved-type-list.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/reserved-type-list.exit
+++ b/tests/qapi-schema/reserved-type-list.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/reserved-type-list.json b/tests/qapi-schema/reserved-type-list.json
index 5b7d0f9..98d53bf 100644
--- a/tests/qapi-schema/reserved-type-list.json
+++ b/tests/qapi-schema/reserved-type-list.json
@@ -1,5 +1,5 @@
# Potential C name collision
-# FIXME - This parses and compiles on its own, but prevents the user from
-# creating a type named 'Foo' and using ['Foo'] for an array. We should
-# reject the use of any type names ending in 'List'.
+# We reserve names ending in 'List' for use by array types.
+# TODO - we could choose array names to avoid collision with user types,
+# in order to let this compile
{ 'struct': 'FooList', 'data': { 's': 'str' } }
diff --git a/tests/qapi-schema/reserved-type-list.out b/tests/qapi-schema/reserved-type-list.out
index 0406bfe..e69de29 100644
--- a/tests/qapi-schema/reserved-type-list.out
+++ b/tests/qapi-schema/reserved-type-list.out
@@ -1,3 +0,0 @@
-object :empty
-object FooList
- member s: str optional=False
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 05/24] qapi: Reserve 'q_*' and 'has_*' member names
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (3 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 04/24] qapi: Reserve '*List' type names for list types Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 06/24] vnc: Hoist allocation of VncBasicInfo to callers Eric Blake
` (19 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
c_name() produces names starting with 'q_' when protecting
a QMP member name that would fail to directly compile, but
in doing so can cause clashes with any QMP name already
beginning with 'q-' or 'q_'. Likewise, we create a C name
'has_' for any optional member, that can clash with any QMP
name beginning with 'has-' or 'has_'.
Technically, rather than blindly reserving the namespace,
we could try to complain about user names only when an actual
collision occurs, or even teach c_name() how to munge names
to avoid collisions. But it is not trivial, especially when
collisions can occur across multiple types (such as via
inheritance or flat unions). Besides, no existing .json
files are trying to use these names. So it's easier to just
outright forbid the potential for collision. We can always
relax things in the future if a real need arises for QMP to
express member names that have been forbidden here.
'has_' only has to be reserved for struct/union member names,
while 'q_' is reserved everywhere (matching the fact that
only members can be optional, while we use c_name() for munging
both members and entities). Note that we could relax 'q_'
restrictions on entities independently from member names; for
example, c_name('qmp_' + 'unix') would result in a different
function name than our current 'qmp_' + c_name('unix').
Update and add tests to cover the new error messages.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: better comment wording; use of c_name(, False); update
commit message
v10: retitle, improve comments, defer 'u' changes for later, use
c_name(name).startswith('has_') rather than open coding
v9: new patch
---
docs/qapi-code-gen.txt | 11 +++++++----
scripts/qapi.py | 8 +++++++-
tests/qapi-schema/reserved-command-q.err | 1 +
tests/qapi-schema/reserved-command-q.exit | 2 +-
tests/qapi-schema/reserved-command-q.json | 6 ++----
tests/qapi-schema/reserved-command-q.out | 5 -----
tests/qapi-schema/reserved-member-has.err | 1 +
tests/qapi-schema/reserved-member-has.exit | 2 +-
tests/qapi-schema/reserved-member-has.json | 7 +++----
tests/qapi-schema/reserved-member-has.out | 6 ------
tests/qapi-schema/reserved-member-q.err | 1 +
tests/qapi-schema/reserved-member-q.exit | 2 +-
tests/qapi-schema/reserved-member-q.json | 6 ++----
tests/qapi-schema/reserved-member-q.out | 4 ----
14 files changed, 27 insertions(+), 35 deletions(-)
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index c4264a8..4d8c2fc 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -112,7 +112,9 @@ and field names within a type, should be all lower case with words
separated by a hyphen. However, some existing older commands and
complex types use underscore; when extending such expressions,
consistency is preferred over blindly avoiding underscore. Event
-names should be ALL_CAPS with words separated by underscore.
+names should be ALL_CAPS with words separated by underscore. Field
+names cannot start with 'has-' or 'has_', as this is reserved for
+tracking optional fields.
Any name (command, event, type, field, or enum value) beginning with
"x-" is marked experimental, and may be withdrawn or changed
@@ -123,9 +125,10 @@ vendor), even if the rest of the name uses dash (example:
__com.redhat_drive-mirror). Other than downstream extensions (with
leading underscore and the use of dots), all names should begin with a
letter, and contain only ASCII letters, digits, dash, and underscore.
-It is okay to reuse names that match C keywords; the generator will
-rename a field named "default" in the QAPI to "q_default" in the
-generated C code.
+Names beginning with 'q_' are reserved for the generator: QMP names
+that resemble C keywords or other problematic strings will be munged
+in C to use this prefix. For example, a field named "default" in
+qapi becomes "q_default" in the generated C code.
In the rest of this document, usage lines are given for each
expression type, with literal strings written in lower case and
diff --git a/scripts/qapi.py b/scripts/qapi.py
index d53b5c4..3941cd1 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -376,7 +376,9 @@ def check_name(expr_info, source, name, allow_optional=False,
# code always prefixes it with the enum name
if enum_member:
membername = '_' + membername
- if not valid_name.match(membername):
+ # Reserve the entire 'q_' namespace for c_name()
+ if not valid_name.match(membername) or \
+ c_name(membername, False).startswith('q_'):
raise QAPIExprError(expr_info,
"%s uses invalid name '%s'" % (source, name))
@@ -488,6 +490,10 @@ def check_type(expr_info, source, value, allow_array=False,
for (key, arg) in value.items():
check_name(expr_info, "Member of %s" % source, key,
allow_optional=allow_optional)
+ if c_name(key).startswith('has_'):
+ raise QAPIExprError(expr_info,
+ "Member of %s uses reserved name '%s'"
+ % (source, key))
# Todo: allow dictionaries to represent default values of
# an optional argument.
check_type(expr_info, "Member '%s' of %s" % (key, source), arg,
diff --git a/tests/qapi-schema/reserved-command-q.err b/tests/qapi-schema/reserved-command-q.err
index e69de29..f939e04 100644
--- a/tests/qapi-schema/reserved-command-q.err
+++ b/tests/qapi-schema/reserved-command-q.err
@@ -0,0 +1 @@
+tests/qapi-schema/reserved-command-q.json:5: 'command' uses invalid name 'q-unix'
diff --git a/tests/qapi-schema/reserved-command-q.exit b/tests/qapi-schema/reserved-command-q.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/reserved-command-q.exit
+++ b/tests/qapi-schema/reserved-command-q.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/reserved-command-q.json b/tests/qapi-schema/reserved-command-q.json
index be9944c..99f8aae 100644
--- a/tests/qapi-schema/reserved-command-q.json
+++ b/tests/qapi-schema/reserved-command-q.json
@@ -1,7 +1,5 @@
# C entity name collision
-# FIXME - This parses, but fails to compile, because it attempts to declare
-# two 'qmp_q_unix' functions (one for 'q-unix', the other because c_name()
-# munges 'unix' to 'q_unix' to avoid reserved word collisions). We should
-# reject attempts to explicitly use 'q_' names, to reserve it for qapi.
+# We reject names like 'q-unix', because they can collide with the mangled
+# name for 'unix' in generated C.
{ 'command': 'unix' }
{ 'command': 'q-unix' }
diff --git a/tests/qapi-schema/reserved-command-q.out b/tests/qapi-schema/reserved-command-q.out
index b31b38f..e69de29 100644
--- a/tests/qapi-schema/reserved-command-q.out
+++ b/tests/qapi-schema/reserved-command-q.out
@@ -1,5 +0,0 @@
-object :empty
-command q-unix None -> None
- gen=True success_response=True
-command unix None -> None
- gen=True success_response=True
diff --git a/tests/qapi-schema/reserved-member-has.err b/tests/qapi-schema/reserved-member-has.err
index e69de29..e755771 100644
--- a/tests/qapi-schema/reserved-member-has.err
+++ b/tests/qapi-schema/reserved-member-has.err
@@ -0,0 +1 @@
+tests/qapi-schema/reserved-member-has.json:5: Member of 'data' for command 'oops' uses reserved name 'has-a'
diff --git a/tests/qapi-schema/reserved-member-has.exit b/tests/qapi-schema/reserved-member-has.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/reserved-member-has.exit
+++ b/tests/qapi-schema/reserved-member-has.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/reserved-member-has.json b/tests/qapi-schema/reserved-member-has.json
index a2197de..45b9109 100644
--- a/tests/qapi-schema/reserved-member-has.json
+++ b/tests/qapi-schema/reserved-member-has.json
@@ -1,6 +1,5 @@
# C member name collision
-# FIXME - This parses, but fails to compile, because the C struct is given
-# two 'has_a' members, one from the flag for optional 'a', and the other
-# from member 'has-a'. Either reject this at parse time, or munge the C
-# names to avoid the collision.
+# We reject names like 'has-a', because they can collide with the flag
+# for an optional 'a' in generated C.
+# TODO we could munge the optional flag name to avoid the collision.
{ 'command': 'oops', 'data': { '*a': 'str', 'has-a': 'str' } }
diff --git a/tests/qapi-schema/reserved-member-has.out b/tests/qapi-schema/reserved-member-has.out
index 5a18b6b..e69de29 100644
--- a/tests/qapi-schema/reserved-member-has.out
+++ b/tests/qapi-schema/reserved-member-has.out
@@ -1,6 +0,0 @@
-object :empty
-object :obj-oops-arg
- member a: str optional=True
- member has-a: str optional=False
-command oops :obj-oops-arg -> None
- gen=True success_response=True
diff --git a/tests/qapi-schema/reserved-member-q.err b/tests/qapi-schema/reserved-member-q.err
index e69de29..f3d5dd7 100644
--- a/tests/qapi-schema/reserved-member-q.err
+++ b/tests/qapi-schema/reserved-member-q.err
@@ -0,0 +1 @@
+tests/qapi-schema/reserved-member-q.json:4: Member of 'data' for struct 'Foo' uses invalid name 'q-unix'
diff --git a/tests/qapi-schema/reserved-member-q.exit b/tests/qapi-schema/reserved-member-q.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/reserved-member-q.exit
+++ b/tests/qapi-schema/reserved-member-q.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/reserved-member-q.json b/tests/qapi-schema/reserved-member-q.json
index 1602ed3..62fed8f 100644
--- a/tests/qapi-schema/reserved-member-q.json
+++ b/tests/qapi-schema/reserved-member-q.json
@@ -1,6 +1,4 @@
# C member name collision
-# FIXME - This parses, but fails to compile, because it attempts to declare
-# two 'q_unix' members (one for 'q-unix', the other because c_name()
-# munges 'unix' to 'q_unix' to avoid reserved word collisions). We should
-# reject attempts to explicitly use 'q_' names, to reserve it for qapi.
+# We reject names like 'q-unix', because they can collide with the mangled
+# name for 'unix' in generated C.
{ 'struct': 'Foo', 'data': { 'unix':'int', 'q-unix':'bool' } }
diff --git a/tests/qapi-schema/reserved-member-q.out b/tests/qapi-schema/reserved-member-q.out
index 0d8685a..e69de29 100644
--- a/tests/qapi-schema/reserved-member-q.out
+++ b/tests/qapi-schema/reserved-member-q.out
@@ -1,4 +0,0 @@
-object :empty
-object Foo
- member unix: int optional=False
- member q-unix: bool optional=False
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 06/24] vnc: Hoist allocation of VncBasicInfo to callers
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (4 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 05/24] qapi: Reserve 'q_*' and 'has_*' member names Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 07/24] qapi-visit: Split off visit_type_FOO_fields forward decl Eric Blake
` (18 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Gerd Hoffmann
A future qapi patch will rework generated structs with a base
class to be unboxed. In preparation for that, change the code
that allocates then populates an info struct to instead merely
populate the fields of an info field passed in as a parameter
(renaming vnc_basic_info_get* to vnc_init_basic_info*). Add
rudimentary Error handling at the lowest levels for cases
where the old code returned NULL; but rather than plumb Error
all the way through the stack, the callers drop the error and
return NULL as before.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: no change
v10: improve commit message (including a retitle), rename
functions, tweak error message, don't change semantics of
vnc_server_info_get()
v9: (no v6-8): hoist from v5 33/46
---
ui/vnc.c | 57 ++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 34 insertions(+), 23 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index faff054..502a10a 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -156,10 +156,11 @@ char *vnc_socket_remote_addr(const char *format, int fd) {
return addr_to_string(format, &sa, salen);
}
-static VncBasicInfo *vnc_basic_info_get(struct sockaddr_storage *sa,
- socklen_t salen)
+static void vnc_init_basic_info(struct sockaddr_storage *sa,
+ socklen_t salen,
+ VncBasicInfo *info,
+ Error **errp)
{
- VncBasicInfo *info;
char host[NI_MAXHOST];
char serv[NI_MAXSERV];
int err;
@@ -168,42 +169,44 @@ static VncBasicInfo *vnc_basic_info_get(struct sockaddr_storage *sa,
host, sizeof(host),
serv, sizeof(serv),
NI_NUMERICHOST | NI_NUMERICSERV)) != 0) {
- VNC_DEBUG("Cannot resolve address %d: %s\n",
- err, gai_strerror(err));
- return NULL;
+ error_setg(errp, "Cannot resolve address: %s",
+ gai_strerror(err));
+ return;
}
- info = g_malloc0(sizeof(VncBasicInfo));
info->host = g_strdup(host);
info->service = g_strdup(serv);
info->family = inet_netfamily(sa->ss_family);
- return info;
}
-static VncBasicInfo *vnc_basic_info_get_from_server_addr(int fd)
+static void vnc_init_basic_info_from_server_addr(int fd, VncBasicInfo *info,
+ Error **errp)
{
struct sockaddr_storage sa;
socklen_t salen;
salen = sizeof(sa);
if (getsockname(fd, (struct sockaddr*)&sa, &salen) < 0) {
- return NULL;
+ error_setg_errno(errp, errno, "getsockname failed");
+ return;
}
- return vnc_basic_info_get(&sa, salen);
+ vnc_init_basic_info(&sa, salen, info, errp);
}
-static VncBasicInfo *vnc_basic_info_get_from_remote_addr(int fd)
+static void vnc_init_basic_info_from_remote_addr(int fd, VncBasicInfo *info,
+ Error **errp)
{
struct sockaddr_storage sa;
socklen_t salen;
salen = sizeof(sa);
if (getpeername(fd, (struct sockaddr*)&sa, &salen) < 0) {
- return NULL;
+ error_setg_errno(errp, errno, "getpeername failed");
+ return;
}
- return vnc_basic_info_get(&sa, salen);
+ vnc_init_basic_info(&sa, salen, info, errp);
}
static const char *vnc_auth_name(VncDisplay *vd) {
@@ -256,15 +259,18 @@ static const char *vnc_auth_name(VncDisplay *vd) {
static VncServerInfo *vnc_server_info_get(VncDisplay *vd)
{
VncServerInfo *info;
- VncBasicInfo *bi = vnc_basic_info_get_from_server_addr(vd->lsock);
- if (!bi) {
- return NULL;
- }
+ Error *err = NULL;
info = g_malloc(sizeof(*info));
- info->base = bi;
+ info->base = g_malloc(sizeof(*info->base));
+ vnc_init_basic_info_from_server_addr(vd->lsock, info->base, &err);
info->has_auth = true;
info->auth = g_strdup(vnc_auth_name(vd));
+ if (err) {
+ qapi_free_VncServerInfo(info);
+ info = NULL;
+ error_free(err);
+ }
return info;
}
@@ -291,11 +297,16 @@ static void vnc_client_cache_auth(VncState *client)
static void vnc_client_cache_addr(VncState *client)
{
- VncBasicInfo *bi = vnc_basic_info_get_from_remote_addr(client->csock);
+ Error *err = NULL;
- if (bi) {
- client->info = g_malloc0(sizeof(*client->info));
- client->info->base = bi;
+ client->info = g_malloc0(sizeof(*client->info));
+ client->info->base = g_malloc0(sizeof(*client->info->base));
+ vnc_init_basic_info_from_remote_addr(client->csock, client->info->base,
+ &err);
+ if (err) {
+ qapi_free_VncClientInfo(client->info);
+ client->info = NULL;
+ error_free(err);
}
}
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 07/24] qapi-visit: Split off visit_type_FOO_fields forward decl
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (5 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 06/24] vnc: Hoist allocation of VncBasicInfo to callers Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 08/24] qapi-types: Refactor base fields output Eric Blake
` (17 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
We generate a static visit_type_FOO_fields() for every type
FOO. However, sometimes we need a forward declaration. Split
the code to generate the forward declaration out of
gen_visit_implicit_struct() into a new gen_visit_fields_decl(),
and also prepare for a forward declaration to be emitted
during gen_visit_struct(), so that a future patch can switch
from using visit_type_FOO_implicit() to the simpler
visit_type_FOO_fields() as part of unboxing the base class
of a struct.
No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: drop dead hunks, simplify comment
v10: new patch, split from 5/17
---
scripts/qapi-visit.py | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 2dc3aed..d4408f2 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -15,7 +15,12 @@
from qapi import *
import re
+# visit_type_FOO_implicit() is emitted as needed; track if it has already
+# been output.
implicit_structs_seen = set()
+
+# visit_type_FOO_fields() is always emitted; track if a forward declaration
+# or implementation has already been output.
struct_fields_seen = set()
@@ -29,19 +34,24 @@ void visit_type_%(c_name)s(Visitor *v, %(c_type)sobj, const char *name, Error **
c_name=c_name(name), c_type=c_type)
+def gen_visit_fields_decl(typ):
+ ret = ''
+ if typ.name not in struct_fields_seen:
+ ret += mcgen('''
+
+static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error **errp);
+''',
+ c_type=typ.c_name())
+ struct_fields_seen.add(typ.name)
+ return ret
+
+
def gen_visit_implicit_struct(typ):
if typ in implicit_structs_seen:
return ''
implicit_structs_seen.add(typ)
- ret = ''
- if typ.name not in struct_fields_seen:
- # Need a forward declaration
- ret += mcgen('''
-
-static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error **errp);
-''',
- c_type=typ.c_name())
+ ret = gen_visit_fields_decl(typ)
ret += mcgen('''
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 08/24] qapi-types: Refactor base fields output
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (6 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 07/24] qapi-visit: Split off visit_type_FOO_fields forward decl Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 09/24] qapi: Prefer typesafe upcasts to qapi base classes Eric Blake
` (16 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Move code from gen_union() into gen_struct_fields() in order for
a later patch to share code when enumerating inherited fields
for struct types.
No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: rename variable and open-code loop rather than recurse, for
clarity
v10: new patch, split off of 5/17
---
scripts/qapi-types.py | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 4fe618e..40e9f79 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -51,10 +51,21 @@ def gen_struct_field(name, typ, optional):
return ret
-def gen_struct_fields(members):
+def gen_struct_fields(local_members, base=None):
ret = ''
- for memb in members:
+ if base:
+ ret += mcgen('''
+ /* Members inherited from %(c_name)s: */
+''',
+ c_name=base.c_name())
+ for memb in base.members:
+ ret += gen_struct_field(memb.name, memb.type, memb.optional)
+ ret += mcgen('''
+ /* Own members: */
+''')
+
+ for memb in local_members:
ret += gen_struct_field(memb.name, memb.type, memb.optional)
return ret
@@ -126,14 +137,7 @@ struct %(c_name)s {
''',
c_name=c_name(name))
if base:
- ret += mcgen('''
- /* Members inherited from %(c_name)s: */
-''',
- c_name=c_name(base.name))
- ret += gen_struct_fields(base.members)
- ret += mcgen('''
- /* Own members: */
-''')
+ ret += gen_struct_fields([], base)
else:
ret += mcgen('''
%(c_type)s kind;
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 09/24] qapi: Prefer typesafe upcasts to qapi base classes
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (7 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 08/24] qapi-types: Refactor base fields output Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-27 7:46 ` Markus Armbruster
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 10/24] qapi: Unbox base members Eric Blake
` (15 subsequent siblings)
24 siblings, 1 reply; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
A previous patch (commit 1e6c1616) made it possible to
directly cast from a qapi flat union type to its base type.
However, it requires the use of a C cast, which turns off
compiler type-safety checks. Add inline type-safe wrappers
named qapi_FOO_base() for any union type FOO that has a base,
which can be used for a safer upcast, and enhance the
testsuite to cover the new functionality. A future patch
will extend the upcast support to structs.
Note that C makes const-correct upcasts annoying because
it lacks overloads; these functions cast away const so that
they can accept user pointers whether const or not, and the
result in turn can be assigned to normal or const pointers.
Alternatively, this could have been done with macros, but
those tend to not have quite as much type safety.
This patch just adds upcasts. None of our code needed to
downcast from a base qapi class to a child. Also, in the
case of grandchildren (such as BlockdevOptionsQcow2), the
caller will need to call two functions to get to the inner
base (although it wouldn't be too hard to generate a
qapi_FOO_base_base() if desired). If a user changes qapi
to alter the base class hierarchy, such as going from
'A -> C' to 'A -> B -> C', it will change the type of
'qapi_C_base()', and the compiler will point out the places
that are affected by the new base.
One alternative was proposed, but was deemed too ugly to use
in practice: the generators could output redundant
information using anonymous types:
| struct Child {
| union {
| struct {
| Type1 parent_member1;
| Type2 parent_member2;
| };
| Parent base;
| };
| };
With that ugly proposal, for a given qapi type, obj->member
and obj->base.member would refer to the same storage; allowing
convenience in working with members without needing 'base.'
allowing typesafe upcast without needing a C cast by accessing
'&obj->base', and allowing downcasts from the parent back to
the child possible through container_of(obj, Child, base).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: only support unions for this patch, add testsuite coverage
v10: new patch
---
scripts/qapi-types.py | 16 ++++++++++++++++
tests/test-qmp-input-visitor.c | 5 +++++
2 files changed, 21 insertions(+)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 40e9f79..f9fcf15 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -98,6 +98,19 @@ struct %(c_name)s {
return ret
+def gen_upcast(name, base):
+ # C makes const-correctness ugly. We have to cast away const to let
+ # this function work for both const and non-const obj.
+ return mcgen('''
+
+static inline %(base)s *qapi_%(c_name)s_base(const %(c_name)s *obj)
+{
+ return (%(base)s *)obj;
+}
+''',
+ c_name=c_name(name), base=base.c_name())
+
+
def gen_alternate_qtypes_decl(name):
return mcgen('''
@@ -267,6 +280,9 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
if variants:
assert not members # not implemented
self.decl += gen_union(name, base, variants)
+ # TODO Use gen_upcast on structs too, once they have sane layout
+ if base:
+ self.decl += gen_upcast(name, base)
else:
self.decl += gen_struct(name, base, members)
self._gen_type_cleanup(name)
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 8941963..da21709 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -347,6 +347,7 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data,
Visitor *v;
Error *err = NULL;
UserDefFlatUnion *tmp;
+ UserDefUnionBase *base;
v = visitor_input_test_init(data,
"{ 'enum1': 'value1', "
@@ -360,6 +361,10 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data,
g_assert_cmpstr(tmp->string, ==, "str");
g_assert_cmpint(tmp->integer, ==, 41);
g_assert_cmpint(tmp->value1->boolean, ==, true);
+
+ base = qapi_UserDefFlatUnion_base(tmp);
+ g_assert(&base->enum1 == &tmp->enum1);
+
qapi_free_UserDefFlatUnion(tmp);
}
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v11 09/24] qapi: Prefer typesafe upcasts to qapi base classes
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 09/24] qapi: Prefer typesafe upcasts to qapi base classes Eric Blake
@ 2015-10-27 7:46 ` Markus Armbruster
2015-10-27 14:17 ` Eric Blake
0 siblings, 1 reply; 38+ messages in thread
From: Markus Armbruster @ 2015-10-27 7:46 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> A previous patch (commit 1e6c1616) made it possible to
> directly cast from a qapi flat union type to its base type.
> However, it requires the use of a C cast, which turns off
> compiler type-safety checks. Add inline type-safe wrappers
> named qapi_FOO_base() for any union type FOO that has a base,
> which can be used for a safer upcast, and enhance the
> testsuite to cover the new functionality. A future patch
> will extend the upcast support to structs.
>
> Note that C makes const-correct upcasts annoying because
> it lacks overloads; these functions cast away const so that
> they can accept user pointers whether const or not, and the
> result in turn can be assigned to normal or const pointers.
> Alternatively, this could have been done with macros, but
> those tend to not have quite as much type safety.
Well, the macros can be made just as type-safe, but the result is either
somewhat ugly and using gcc-isms, or very ugly and unhygienic.
I'd write something like "type-safe macros are hairy, and not worthwhile
here."
> This patch just adds upcasts. None of our code needed to
> downcast from a base qapi class to a child.
Actually, none of our code needs to upcast unions, either. Only the new
tests do. Code that updasts structs exist, but it doesn't use this
patch's upcasts until later.
Suggest to amend the first paragraph:
A previous patch (commit 1e6c1616) made it possible to directly cast
from a qapi flat union type to its base type. However, it requires
the use of a C cast, which turns off compiler type-safety checks.
Fortunately, no such casts exist just, yet.
Regardless, add inline type-safe wrappers named qapi_FOO_base() for
any union type FOO that has a base, which can be used for a safer
upcast, and enhance the testsuite to cover the new functionality.
A future patch will extend the upcast support to structs, where such
casts do exist already.
> Also, in the
> case of grandchildren (such as BlockdevOptionsQcow2), the
> caller will need to call two functions to get to the inner
> base (although it wouldn't be too hard to generate a
> qapi_FOO_base_base() if desired). If a user changes qapi
> to alter the base class hierarchy, such as going from
> 'A -> C' to 'A -> B -> C', it will change the type of
> 'qapi_C_base()', and the compiler will point out the places
> that are affected by the new base.
>
> One alternative was proposed, but was deemed too ugly to use
> in practice: the generators could output redundant
> information using anonymous types:
> | struct Child {
> | union {
> | struct {
> | Type1 parent_member1;
> | Type2 parent_member2;
> | };
> | Parent base;
> | };
> | };
> With that ugly proposal, for a given qapi type, obj->member
> and obj->base.member would refer to the same storage; allowing
> convenience in working with members without needing 'base.'
> allowing typesafe upcast without needing a C cast by accessing
> '&obj->base', and allowing downcasts from the parent back to
> the child possible through container_of(obj, Child, base).
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v11: only support unions for this patch, add testsuite coverage
> v10: new patch
Patch looks good. I can touch up the commit message in my tree.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v11 09/24] qapi: Prefer typesafe upcasts to qapi base classes
2015-10-27 7:46 ` Markus Armbruster
@ 2015-10-27 14:17 ` Eric Blake
2015-10-27 15:06 ` Markus Armbruster
0 siblings, 1 reply; 38+ messages in thread
From: Eric Blake @ 2015-10-27 14:17 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2658 bytes --]
On 10/27/2015 01:46 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> A previous patch (commit 1e6c1616) made it possible to
>> directly cast from a qapi flat union type to its base type.
>> However, it requires the use of a C cast, which turns off
>> compiler type-safety checks. Add inline type-safe wrappers
>> named qapi_FOO_base() for any union type FOO that has a base,
>> which can be used for a safer upcast, and enhance the
>> testsuite to cover the new functionality. A future patch
>> will extend the upcast support to structs.
>>
>> Note that C makes const-correct upcasts annoying because
>> it lacks overloads; these functions cast away const so that
>> they can accept user pointers whether const or not, and the
>> result in turn can be assigned to normal or const pointers.
>> Alternatively, this could have been done with macros, but
>> those tend to not have quite as much type safety.
>
> Well, the macros can be made just as type-safe, but the result is either
> somewhat ugly and using gcc-isms, or very ugly and unhygienic.
>
> I'd write something like "type-safe macros are hairy, and not worthwhile
> here."
Sure, that works for me.
>
>> This patch just adds upcasts. None of our code needed to
>> downcast from a base qapi class to a child.
>
> Actually, none of our code needs to upcast unions, either. Only the new
> tests do. Code that updasts structs exist, but it doesn't use this
> patch's upcasts until later.
>
> Suggest to amend the first paragraph:
>
> A previous patch (commit 1e6c1616) made it possible to directly cast
> from a qapi flat union type to its base type. However, it requires
> the use of a C cast, which turns off compiler type-safety checks.
> Fortunately, no such casts exist just, yet.
s/ just,/, just/
>
> Regardless, add inline type-safe wrappers named qapi_FOO_base() for
> any union type FOO that has a base, which can be used for a safer
> upcast, and enhance the testsuite to cover the new functionality.
>
> A future patch will extend the upcast support to structs, where such
> casts do exist already.
>
Maybe s/casts/conversions/ - because as of this patch, it is still a
conversion via foo->base rather than (Base *)foo (it's the next patch
that gets rid of base, and therefore needs either the cast or the wrapper).
>
> Patch looks good. I can touch up the commit message in my tree.
Sure, your proposed wording + touchups is fine.
--
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] 38+ messages in thread
* Re: [Qemu-devel] [PATCH v11 09/24] qapi: Prefer typesafe upcasts to qapi base classes
2015-10-27 14:17 ` Eric Blake
@ 2015-10-27 15:06 ` Markus Armbruster
0 siblings, 0 replies; 38+ messages in thread
From: Markus Armbruster @ 2015-10-27 15:06 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 10/27/2015 01:46 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> A previous patch (commit 1e6c1616) made it possible to
>>> directly cast from a qapi flat union type to its base type.
>>> However, it requires the use of a C cast, which turns off
>>> compiler type-safety checks. Add inline type-safe wrappers
>>> named qapi_FOO_base() for any union type FOO that has a base,
>>> which can be used for a safer upcast, and enhance the
>>> testsuite to cover the new functionality. A future patch
>>> will extend the upcast support to structs.
>>>
>>> Note that C makes const-correct upcasts annoying because
>>> it lacks overloads; these functions cast away const so that
>>> they can accept user pointers whether const or not, and the
>>> result in turn can be assigned to normal or const pointers.
>>> Alternatively, this could have been done with macros, but
>>> those tend to not have quite as much type safety.
>>
>> Well, the macros can be made just as type-safe, but the result is either
>> somewhat ugly and using gcc-isms, or very ugly and unhygienic.
>>
>> I'd write something like "type-safe macros are hairy, and not worthwhile
>> here."
>
> Sure, that works for me.
Sold.
>>> This patch just adds upcasts. None of our code needed to
>>> downcast from a base qapi class to a child.
>>
>> Actually, none of our code needs to upcast unions, either. Only the new
>> tests do. Code that updasts structs exist, but it doesn't use this
>> patch's upcasts until later.
>>
>> Suggest to amend the first paragraph:
>>
>> A previous patch (commit 1e6c1616) made it possible to directly cast
>> from a qapi flat union type to its base type. However, it requires
>> the use of a C cast, which turns off compiler type-safety checks.
>> Fortunately, no such casts exist just, yet.
>
> s/ just,/, just/
Yes, thanks.
>>
>> Regardless, add inline type-safe wrappers named qapi_FOO_base() for
>> any union type FOO that has a base, which can be used for a safer
>> upcast, and enhance the testsuite to cover the new functionality.
>>
>> A future patch will extend the upcast support to structs, where such
>> casts do exist already.
>>
>
> Maybe s/casts/conversions/ - because as of this patch, it is still a
> conversion via foo->base rather than (Base *)foo (it's the next patch
> that gets rid of base, and therefore needs either the cast or the wrapper).
Sold.
>> Patch looks good. I can touch up the commit message in my tree.
>
> Sure, your proposed wording + touchups is fine.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v11 10/24] qapi: Unbox base members
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (8 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 09/24] qapi: Prefer typesafe upcasts to qapi base classes Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-27 7:52 ` Markus Armbruster
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 11/24] qapi-visit: Remove redundant functions for flat union base Eric Blake
` (14 subsequent siblings)
24 siblings, 1 reply; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel; +Cc: Michael Roth, Gerd Hoffmann, armbru, Luiz Capitulino
Rather than storing a base class as a pointer to a box, just
store the fields of that base class in the same order, so that
a child struct can be directly cast to its parent. This gives
less malloc overhead, less pointer dereferencing, and even less
generated code. Compare to the earlier commit 1e6c1616a "qapi:
Generate a nicer struct for flat unions" (although that patch
had fewer places to change, as less of qemu was directly using
qapi structs for flat unions). It also allows us to turn on
automatic type-safe wrappers for upcasting to the base class
of a struct.
Changes to the generated code look like this in qapi-types.h:
| struct SpiceChannel {
|- SpiceBasicInfo *base;
|+ /* Members inherited from SpiceBasicInfo: */
|+ char *host;
|+ char *port;
|+ NetworkAddressFamily family;
|+ /* Own members: */
| int64_t connection_id;
as well as added upcast functions like qapi_SpiceChannel_base().
Meanwhile, changes to qapi-visit.c look like:
| static void visit_type_SpiceChannel_fields(Visitor *v, SpiceChannel **obj, Error **errp)
| {
| Error *err = NULL;
|
|- visit_type_implicit_SpiceBasicInfo(v, &(*obj)->base, &err);
|+ visit_type_SpiceBasicInfo_fields(v, (SpiceBasicInfo **)obj, &err);
| if (err) {
(the cast is necessary, since our upcast wrappers only deal with a
single pointer, not pointer-to-pointer); plus the wholesale
elimination of some now-unused visit_type_implicit_FOO() functions.
Without boxing, the corner case of one empty struct having
another empty struct as its base type now requires inserting a
dummy member (previously, the 'Base *base' member sufficed).
And now that we no longer consume a 'base' member in the generated
C struct, we can delete the former negative struct-base-clash-base
test.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: delay gen_upcast for structs until here, rebase to proper use
of gen_visit_fields_decl() in gen_visit_struct_fields(), improve
commit message
v10: split off some of the cleanups into earlier patches, improve
commit message, don't emit visit_type_FOO_fields out of order,
save positive tests in qapi-schema-tests for later
v9: (no v6-8): hoist from v5 34/46, rebase to master
---
hmp.c | 6 +++---
scripts/qapi-types.py | 12 ++++--------
scripts/qapi-visit.py | 15 +++++++--------
tests/Makefile | 1 -
tests/qapi-schema/struct-base-clash-base.err | 0
tests/qapi-schema/struct-base-clash-base.exit | 1 -
tests/qapi-schema/struct-base-clash-base.json | 9 ---------
tests/qapi-schema/struct-base-clash-base.out | 5 -----
tests/test-qmp-commands.c | 15 +++++----------
tests/test-qmp-event.c | 8 ++------
tests/test-qmp-input-visitor.c | 4 ++--
tests/test-qmp-output-visitor.c | 13 +++++--------
tests/test-visitor-serialization.c | 14 ++++++--------
ui/spice-core.c | 23 +++++++++++++----------
ui/vnc.c | 21 ++++++++++-----------
15 files changed, 57 insertions(+), 90 deletions(-)
delete mode 100644 tests/qapi-schema/struct-base-clash-base.err
delete mode 100644 tests/qapi-schema/struct-base-clash-base.exit
delete mode 100644 tests/qapi-schema/struct-base-clash-base.json
delete mode 100644 tests/qapi-schema/struct-base-clash-base.out
diff --git a/hmp.c b/hmp.c
index 5048eee..88fd804 100644
--- a/hmp.c
+++ b/hmp.c
@@ -569,8 +569,8 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
for (client = info->clients; client; client = client->next) {
monitor_printf(mon, "Client:\n");
monitor_printf(mon, " address: %s:%s\n",
- client->value->base->host,
- client->value->base->service);
+ client->value->host,
+ client->value->service);
monitor_printf(mon, " x509_dname: %s\n",
client->value->x509_dname ?
client->value->x509_dname : "none");
@@ -638,7 +638,7 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict)
for (chan = info->channels; chan; chan = chan->next) {
monitor_printf(mon, "Channel:\n");
monitor_printf(mon, " address: %s:%s%s\n",
- chan->value->base->host, chan->value->base->port,
+ chan->value->host, chan->value->port,
chan->value->tls ? " [tls]" : "");
monitor_printf(mon, " session: %" PRId64 "\n",
chan->value->connection_id);
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index f9fcf15..faf7022 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -77,16 +77,13 @@ struct %(c_name)s {
''',
c_name=c_name(name))
- if base:
- ret += gen_struct_field('base', base, False)
-
- ret += gen_struct_fields(members)
+ ret += gen_struct_fields(members, base)
# Make sure that all structs have at least one field; this avoids
# potential issues with attempting to malloc space for zero-length
# structs in C, and also incompatibility with C++ (where an empty
# struct is size 1).
- if not base and not members:
+ if not (base and base.members) and not members:
ret += mcgen('''
char qapi_dummy_field_for_empty_struct;
''')
@@ -280,11 +277,10 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
if variants:
assert not members # not implemented
self.decl += gen_union(name, base, variants)
- # TODO Use gen_upcast on structs too, once they have sane layout
- if base:
- self.decl += gen_upcast(name, base)
else:
self.decl += gen_struct(name, base, members)
+ if base:
+ self.decl += gen_upcast(name, base)
self._gen_type_cleanup(name)
def visit_alternate_type(self, name, info, variants):
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index d4408f2..f711a72 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -72,13 +72,12 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error *
def gen_visit_struct_fields(name, base, members):
+ ret = ''
+
+ if base:
+ ret += gen_visit_fields_decl(base)
+
struct_fields_seen.add(name)
-
- ret = ''
-
- if base:
- ret += gen_visit_implicit_struct(base)
-
ret += mcgen('''
static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **errp)
@@ -90,9 +89,9 @@ static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e
if base:
ret += mcgen('''
- visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
+ visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, &err);
''',
- c_type=base.c_name(), c_name=c_name('base'))
+ c_type=base.c_name())
ret += gen_err_check()
ret += gen_visit_fields(members, prefix='(*obj)->')
diff --git a/tests/Makefile b/tests/Makefile
index 54022d6..6d7c07e 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -325,7 +325,6 @@ qapi-schema += returns-array-bad.json
qapi-schema += returns-dict.json
qapi-schema += returns-unknown.json
qapi-schema += returns-whitelist.json
-qapi-schema += struct-base-clash-base.json
qapi-schema += struct-base-clash-deep.json
qapi-schema += struct-base-clash.json
qapi-schema += struct-data-invalid.json
diff --git a/tests/qapi-schema/struct-base-clash-base.err b/tests/qapi-schema/struct-base-clash-base.err
deleted file mode 100644
index e69de29..0000000
diff --git a/tests/qapi-schema/struct-base-clash-base.exit b/tests/qapi-schema/struct-base-clash-base.exit
deleted file mode 100644
index 573541a..0000000
--- a/tests/qapi-schema/struct-base-clash-base.exit
+++ /dev/null
@@ -1 +0,0 @@
-0
diff --git a/tests/qapi-schema/struct-base-clash-base.json b/tests/qapi-schema/struct-base-clash-base.json
deleted file mode 100644
index 0c84025..0000000
--- a/tests/qapi-schema/struct-base-clash-base.json
+++ /dev/null
@@ -1,9 +0,0 @@
-# Struct member 'base'
-# FIXME: this parses, but then fails to compile due to a duplicate 'base'
-# (one explicit in QMP, the other used to box the base class members).
-# We should either reject the collision at parse time, or change the
-# generated struct to allow this to compile.
-{ 'struct': 'Base', 'data': {} }
-{ 'struct': 'Sub',
- 'base': 'Base',
- 'data': { 'base': 'str' } }
diff --git a/tests/qapi-schema/struct-base-clash-base.out b/tests/qapi-schema/struct-base-clash-base.out
deleted file mode 100644
index e69a416..0000000
--- a/tests/qapi-schema/struct-base-clash-base.out
+++ /dev/null
@@ -1,5 +0,0 @@
-object :empty
-object Base
-object Sub
- base Base
- member base: str optional=False
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index bc59835..ea700d8 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -25,11 +25,9 @@ UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a,
UserDefOne *ud1d = g_malloc0(sizeof(UserDefOne));
ud1c->string = strdup(ud1a->string);
- ud1c->base = g_new0(UserDefZero, 1);
- ud1c->base->integer = ud1a->base->integer;
+ ud1c->integer = ud1a->integer;
ud1d->string = strdup(has_udb1 ? ud1b->string : "blah0");
- ud1d->base = g_new0(UserDefZero, 1);
- ud1d->base->integer = has_udb1 ? ud1b->base->integer : 0;
+ ud1d->integer = has_udb1 ? ud1b->integer : 0;
ret = g_new0(UserDefTwo, 1);
ret->string0 = strdup("blah1");
@@ -176,20 +174,17 @@ static void test_dealloc_types(void)
UserDefOneList *ud1list;
ud1test = g_malloc0(sizeof(UserDefOne));
- ud1test->base = g_new0(UserDefZero, 1);
- ud1test->base->integer = 42;
+ ud1test->integer = 42;
ud1test->string = g_strdup("hi there 42");
qapi_free_UserDefOne(ud1test);
ud1a = g_malloc0(sizeof(UserDefOne));
- ud1a->base = g_new0(UserDefZero, 1);
- ud1a->base->integer = 43;
+ ud1a->integer = 43;
ud1a->string = g_strdup("hi there 43");
ud1b = g_malloc0(sizeof(UserDefOne));
- ud1b->base = g_new0(UserDefZero, 1);
- ud1b->base->integer = 44;
+ ud1b->integer = 44;
ud1b->string = g_strdup("hi there 44");
ud1list = g_malloc0(sizeof(UserDefOneList));
diff --git a/tests/test-qmp-event.c b/tests/test-qmp-event.c
index 28f146d..035c65c 100644
--- a/tests/test-qmp-event.c
+++ b/tests/test-qmp-event.c
@@ -179,9 +179,7 @@ static void test_event_c(TestEventData *data,
QDict *d, *d_data, *d_b;
UserDefOne b;
- UserDefZero z;
- z.integer = 2;
- b.base = &z;
+ b.integer = 2;
b.string = g_strdup("test1");
b.has_enum1 = false;
@@ -209,11 +207,9 @@ static void test_event_d(TestEventData *data,
{
UserDefOne struct1;
EventStructOne a;
- UserDefZero z;
QDict *d, *d_data, *d_a, *d_struct1;
- z.integer = 2;
- struct1.base = &z;
+ struct1.integer = 2;
struct1.string = g_strdup("test1");
struct1.has_enum1 = true;
struct1.enum1 = ENUM_ONE_VALUE1;
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index da21709..2d95db9 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -262,7 +262,7 @@ static void test_visitor_in_struct_nested(TestInputVisitorData *data,
check_and_free_str(udp->string0, "string0");
check_and_free_str(udp->dict1->string1, "string1");
- g_assert_cmpint(udp->dict1->dict2->userdef->base->integer, ==, 42);
+ g_assert_cmpint(udp->dict1->dict2->userdef->integer, ==, 42);
check_and_free_str(udp->dict1->dict2->userdef->string, "string");
check_and_free_str(udp->dict1->dict2->string, "string2");
g_assert(udp->dict1->has_dict3 == false);
@@ -292,7 +292,7 @@ static void test_visitor_in_list(TestInputVisitorData *data,
snprintf(string, sizeof(string), "string%d", i);
g_assert_cmpstr(item->value->string, ==, string);
- g_assert_cmpint(item->value->base->integer, ==, 42 + i);
+ g_assert_cmpint(item->value->integer, ==, 42 + i);
}
qapi_free_UserDefOneList(head);
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index c84002e..cfb06bb 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -250,16 +250,14 @@ static void test_visitor_out_struct_nested(TestOutputVisitorData *data,
ud2->dict1->dict2 = g_malloc0(sizeof(*ud2->dict1->dict2));
ud2->dict1->dict2->userdef = g_new0(UserDefOne, 1);
ud2->dict1->dict2->userdef->string = g_strdup(string);
- ud2->dict1->dict2->userdef->base = g_new0(UserDefZero, 1);
- ud2->dict1->dict2->userdef->base->integer = value;
+ ud2->dict1->dict2->userdef->integer = value;
ud2->dict1->dict2->string = g_strdup(strings[2]);
ud2->dict1->dict3 = g_malloc0(sizeof(*ud2->dict1->dict3));
ud2->dict1->has_dict3 = true;
ud2->dict1->dict3->userdef = g_new0(UserDefOne, 1);
ud2->dict1->dict3->userdef->string = g_strdup(string);
- ud2->dict1->dict3->userdef->base = g_new0(UserDefZero, 1);
- ud2->dict1->dict3->userdef->base->integer = value;
+ ud2->dict1->dict3->userdef->integer = value;
ud2->dict1->dict3->string = g_strdup(strings[3]);
visit_type_UserDefTwo(data->ov, &ud2, "unused", &err);
@@ -301,8 +299,8 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
const void *unused)
{
EnumOne bad_values[] = { ENUM_ONE_MAX, -1 };
- UserDefZero b;
- UserDefOne u = { .base = &b }, *pu = &u;
+ UserDefOne u = {0};
+ UserDefOne *pu = &u;
Error *err;
int i;
@@ -416,8 +414,7 @@ static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data,
p->value->dict1->dict2 = g_new0(UserDefTwoDictDict, 1);
p->value->dict1->dict2->userdef = g_new0(UserDefOne, 1);
p->value->dict1->dict2->userdef->string = g_strdup(string);
- p->value->dict1->dict2->userdef->base = g_new0(UserDefZero, 1);
- p->value->dict1->dict2->userdef->base->integer = 42;
+ p->value->dict1->dict2->userdef->integer = 42;
p->value->dict1->dict2->string = g_strdup(string);
p->value->dict1->has_dict3 = false;
diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index fa86cae..634563b 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -258,15 +258,13 @@ static UserDefTwo *nested_struct_create(void)
udnp->dict1->string1 = strdup("test_string1");
udnp->dict1->dict2 = g_malloc0(sizeof(*udnp->dict1->dict2));
udnp->dict1->dict2->userdef = g_new0(UserDefOne, 1);
- udnp->dict1->dict2->userdef->base = g_new0(UserDefZero, 1);
- udnp->dict1->dict2->userdef->base->integer = 42;
+ udnp->dict1->dict2->userdef->integer = 42;
udnp->dict1->dict2->userdef->string = strdup("test_string");
udnp->dict1->dict2->string = strdup("test_string2");
udnp->dict1->dict3 = g_malloc0(sizeof(*udnp->dict1->dict3));
udnp->dict1->has_dict3 = true;
udnp->dict1->dict3->userdef = g_new0(UserDefOne, 1);
- udnp->dict1->dict3->userdef->base = g_new0(UserDefZero, 1);
- udnp->dict1->dict3->userdef->base->integer = 43;
+ udnp->dict1->dict3->userdef->integer = 43;
udnp->dict1->dict3->userdef->string = strdup("test_string");
udnp->dict1->dict3->string = strdup("test_string3");
return udnp;
@@ -278,15 +276,15 @@ static void nested_struct_compare(UserDefTwo *udnp1, UserDefTwo *udnp2)
g_assert(udnp2);
g_assert_cmpstr(udnp1->string0, ==, udnp2->string0);
g_assert_cmpstr(udnp1->dict1->string1, ==, udnp2->dict1->string1);
- g_assert_cmpint(udnp1->dict1->dict2->userdef->base->integer, ==,
- udnp2->dict1->dict2->userdef->base->integer);
+ g_assert_cmpint(udnp1->dict1->dict2->userdef->integer, ==,
+ udnp2->dict1->dict2->userdef->integer);
g_assert_cmpstr(udnp1->dict1->dict2->userdef->string, ==,
udnp2->dict1->dict2->userdef->string);
g_assert_cmpstr(udnp1->dict1->dict2->string, ==,
udnp2->dict1->dict2->string);
g_assert(udnp1->dict1->has_dict3 == udnp2->dict1->has_dict3);
- g_assert_cmpint(udnp1->dict1->dict3->userdef->base->integer, ==,
- udnp2->dict1->dict3->userdef->base->integer);
+ g_assert_cmpint(udnp1->dict1->dict3->userdef->integer, ==,
+ udnp2->dict1->dict3->userdef->integer);
g_assert_cmpstr(udnp1->dict1->dict3->userdef->string, ==,
udnp2->dict1->dict3->userdef->string);
g_assert_cmpstr(udnp1->dict1->dict3->string, ==,
diff --git a/ui/spice-core.c b/ui/spice-core.c
index bf4fd07..6a62d71 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -200,8 +200,6 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
{
SpiceServerInfo *server = g_malloc0(sizeof(*server));
SpiceChannel *client = g_malloc0(sizeof(*client));
- server->base = g_malloc0(sizeof(*server->base));
- client->base = g_malloc0(sizeof(*client->base));
/*
* Spice server might have called us from spice worker thread
@@ -218,9 +216,11 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
}
if (info->flags & SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT) {
- add_addr_info(client->base, (struct sockaddr *)&info->paddr_ext,
+ add_addr_info(qapi_SpiceChannel_base(client),
+ (struct sockaddr *)&info->paddr_ext,
info->plen_ext);
- add_addr_info(server->base, (struct sockaddr *)&info->laddr_ext,
+ add_addr_info(qapi_SpiceServerInfo_base(server),
+ (struct sockaddr *)&info->laddr_ext,
info->llen_ext);
} else {
error_report("spice: %s, extended address is expected",
@@ -229,7 +229,9 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
switch (event) {
case SPICE_CHANNEL_EVENT_CONNECTED:
- qapi_event_send_spice_connected(server->base, client->base, &error_abort);
+ qapi_event_send_spice_connected(qapi_SpiceServerInfo_base(server),
+ qapi_SpiceChannel_base(client),
+ &error_abort);
break;
case SPICE_CHANNEL_EVENT_INITIALIZED:
if (auth) {
@@ -242,7 +244,9 @@ static void channel_event(int event, SpiceChannelEventInfo *info)
break;
case SPICE_CHANNEL_EVENT_DISCONNECTED:
channel_list_del(info);
- qapi_event_send_spice_disconnected(server->base, client->base, &error_abort);
+ qapi_event_send_spice_disconnected(qapi_SpiceServerInfo_base(server),
+ qapi_SpiceChannel_base(client),
+ &error_abort);
break;
default:
break;
@@ -378,16 +382,15 @@ static SpiceChannelList *qmp_query_spice_channels(void)
chan = g_malloc0(sizeof(*chan));
chan->value = g_malloc0(sizeof(*chan->value));
- chan->value->base = g_malloc0(sizeof(*chan->value->base));
paddr = (struct sockaddr *)&item->info->paddr_ext;
plen = item->info->plen_ext;
getnameinfo(paddr, plen,
host, sizeof(host), port, sizeof(port),
NI_NUMERICHOST | NI_NUMERICSERV);
- chan->value->base->host = g_strdup(host);
- chan->value->base->port = g_strdup(port);
- chan->value->base->family = inet_netfamily(paddr->sa_family);
+ chan->value->host = g_strdup(host);
+ chan->value->port = g_strdup(port);
+ chan->value->family = inet_netfamily(paddr->sa_family);
chan->value->connection_id = item->info->connection_id;
chan->value->channel_type = item->info->type;
diff --git a/ui/vnc.c b/ui/vnc.c
index 502a10a..cec2cee 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -262,8 +262,8 @@ static VncServerInfo *vnc_server_info_get(VncDisplay *vd)
Error *err = NULL;
info = g_malloc(sizeof(*info));
- info->base = g_malloc(sizeof(*info->base));
- vnc_init_basic_info_from_server_addr(vd->lsock, info->base, &err);
+ vnc_init_basic_info_from_server_addr(vd->lsock,
+ qapi_VncServerInfo_base(info), &err);
info->has_auth = true;
info->auth = g_strdup(vnc_auth_name(vd));
if (err) {
@@ -300,8 +300,8 @@ static void vnc_client_cache_addr(VncState *client)
Error *err = NULL;
client->info = g_malloc0(sizeof(*client->info));
- client->info->base = g_malloc0(sizeof(*client->info->base));
- vnc_init_basic_info_from_remote_addr(client->csock, client->info->base,
+ vnc_init_basic_info_from_remote_addr(client->csock,
+ qapi_VncClientInfo_base(client->info),
&err);
if (err) {
qapi_free_VncClientInfo(client->info);
@@ -317,7 +317,6 @@ static void vnc_qmp_event(VncState *vs, QAPIEvent event)
if (!vs->info) {
return;
}
- g_assert(vs->info->base);
si = vnc_server_info_get(vs->vd);
if (!si) {
@@ -326,7 +325,8 @@ static void vnc_qmp_event(VncState *vs, QAPIEvent event)
switch (event) {
case QAPI_EVENT_VNC_CONNECTED:
- qapi_event_send_vnc_connected(si, vs->info->base, &error_abort);
+ qapi_event_send_vnc_connected(si, qapi_VncClientInfo_base(vs->info),
+ &error_abort);
break;
case QAPI_EVENT_VNC_INITIALIZED:
qapi_event_send_vnc_initialized(si, vs->info, &error_abort);
@@ -361,11 +361,10 @@ static VncClientInfo *qmp_query_vnc_client(const VncState *client)
}
info = g_malloc0(sizeof(*info));
- info->base = g_malloc0(sizeof(*info->base));
- info->base->host = g_strdup(host);
- info->base->service = g_strdup(serv);
- info->base->family = inet_netfamily(sa.ss_family);
- info->base->websocket = client->websocket;
+ info->host = g_strdup(host);
+ info->service = g_strdup(serv);
+ info->family = inet_netfamily(sa.ss_family);
+ info->websocket = client->websocket;
if (client->tls) {
info->x509_dname = qcrypto_tls_session_get_peer_name(client->tls);
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v11 10/24] qapi: Unbox base members
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 10/24] qapi: Unbox base members Eric Blake
@ 2015-10-27 7:52 ` Markus Armbruster
2015-10-27 14:20 ` Eric Blake
0 siblings, 1 reply; 38+ messages in thread
From: Markus Armbruster @ 2015-10-27 7:52 UTC (permalink / raw)
To: Eric Blake; +Cc: Luiz Capitulino, Gerd Hoffmann, qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Rather than storing a base class as a pointer to a box, just
> store the fields of that base class in the same order, so that
> a child struct can be directly cast to its parent. This gives
> less malloc overhead, less pointer dereferencing, and even less
> generated code. Compare to the earlier commit 1e6c1616a "qapi:
> Generate a nicer struct for flat unions" (although that patch
> had fewer places to change, as less of qemu was directly using
> qapi structs for flat unions). It also allows us to turn on
> automatic type-safe wrappers for upcasting to the base class
> of a struct.
>
> Changes to the generated code look like this in qapi-types.h:
>
> | struct SpiceChannel {
> |- SpiceBasicInfo *base;
> |+ /* Members inherited from SpiceBasicInfo: */
> |+ char *host;
> |+ char *port;
> |+ NetworkAddressFamily family;
> |+ /* Own members: */
> | int64_t connection_id;
>
> as well as added upcast functions like qapi_SpiceChannel_base().
"additional upcast functions" sounds better to my ears.
> Meanwhile, changes to qapi-visit.c look like:
>
> | static void visit_type_SpiceChannel_fields(Visitor *v, SpiceChannel **obj, Error **errp)
> | {
> | Error *err = NULL;
> |
> |- visit_type_implicit_SpiceBasicInfo(v, &(*obj)->base, &err);
> |+ visit_type_SpiceBasicInfo_fields(v, (SpiceBasicInfo **)obj, &err);
> | if (err) {
>
> (the cast is necessary, since our upcast wrappers only deal with a
> single pointer, not pointer-to-pointer); plus the wholesale
> elimination of some now-unused visit_type_implicit_FOO() functions.
>
> Without boxing, the corner case of one empty struct having
> another empty struct as its base type now requires inserting a
> dummy member (previously, the 'Base *base' member sufficed).
>
> And now that we no longer consume a 'base' member in the generated
> C struct, we can delete the former negative struct-base-clash-base
> test.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v11: delay gen_upcast for structs until here, rebase to proper use
> of gen_visit_fields_decl() in gen_visit_struct_fields(), improve
> commit message
> v10: split off some of the cleanups into earlier patches, improve
> commit message, don't emit visit_type_FOO_fields out of order,
> save positive tests in qapi-schema-tests for later
> v9: (no v6-8): hoist from v5 34/46, rebase to master
Patch looks good. I can touch up the commit message in my tree.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v11 10/24] qapi: Unbox base members
2015-10-27 7:52 ` Markus Armbruster
@ 2015-10-27 14:20 ` Eric Blake
0 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-27 14:20 UTC (permalink / raw)
To: Markus Armbruster
Cc: Luiz Capitulino, Gerd Hoffmann, qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 1418 bytes --]
On 10/27/2015 01:52 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Rather than storing a base class as a pointer to a box, just
>> store the fields of that base class in the same order, so that
>> a child struct can be directly cast to its parent. This gives
>> less malloc overhead, less pointer dereferencing, and even less
>> generated code. Compare to the earlier commit 1e6c1616a "qapi:
>> Generate a nicer struct for flat unions" (although that patch
>> had fewer places to change, as less of qemu was directly using
>> qapi structs for flat unions). It also allows us to turn on
>> automatic type-safe wrappers for upcasting to the base class
>> of a struct.
>>
>> Changes to the generated code look like this in qapi-types.h:
>>
>> | struct SpiceChannel {
>> |- SpiceBasicInfo *base;
>> |+ /* Members inherited from SpiceBasicInfo: */
>> |+ char *host;
>> |+ char *port;
>> |+ NetworkAddressFamily family;
>> |+ /* Own members: */
>> | int64_t connection_id;
>>
>> as well as added upcast functions like qapi_SpiceChannel_base().
>
> "additional upcast functions" sounds better to my ears.
Indeed.
>
> Patch looks good. I can touch up the commit message in my tree.
Yay - looks like we won't need a v12.
--
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] 38+ messages in thread
* [Qemu-devel] [PATCH v11 11/24] qapi-visit: Remove redundant functions for flat union base
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (9 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 10/24] qapi: Unbox base members Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 12/24] qapi: Start converting to new qapi union layout Eric Blake
` (13 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
The code for visiting the base class of a child struct created
visit_type_Base_fields() which covers all fields of Base; while
the code for visiting the base class of a flat union created
visit_type_Union_fields() covering all fields of the base
except the discriminator. But since the base class includes
the discriminator of a flat union, we can just visit the entire
base, without needing a separate visit of the discriminator.
Not only is consistently visiting all fields easier to
understand, it lets us share code.
The generated code in qapi-visit.c loses several now-unused
visit_type_UNION_fields(), along with changes like:
|@@ -1654,11 +1557,7 @@ void visit_type_BlockdevOptions(Visitor
| if (!*obj) {
| goto out_obj;
| }
|- visit_type_BlockdevOptions_fields(v, obj, &err);
|- if (err) {
|- goto out_obj;
|- }
|- visit_type_BlockdevDriver(v, &(*obj)->driver, "driver", &err);
|+ visit_type_BlockdevOptionsBase_fields(v, (BlockdevOptionsBase **)obj, &err);
| if (err) {
| goto out_obj;
| }
and forward declarations where needed. Note that the cast of obj
to BASE ** is necessary to call visit_type_BASE_fields() (and we
can't use our upcast wrappers, because those work on pointers while
we have a pointer-to-pointer).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: touch up commit message
v10: split out regex to earlier commit, rebase to earlier changes,
improve commit message
v9: (no v6-8): hoist from v5 35/46; rebase to master; fix indentation
botch in gen_visit_union(); polish commit message
---
scripts/qapi-visit.py | 32 +++++++++++++++-----------------
1 file changed, 15 insertions(+), 17 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index f711a72..33c013a 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -228,8 +228,7 @@ def gen_visit_union(name, base, variants):
ret = ''
if base:
- members = [m for m in base.members if m != variants.tag_member]
- ret += gen_visit_struct_fields(name, None, members)
+ ret += gen_visit_fields_decl(base)
for var in variants.variants:
# Ugly special case for simple union TODO get rid of it
@@ -254,31 +253,30 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
if base:
ret += mcgen('''
- visit_type_%(c_name)s_fields(v, obj, &err);
+ visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err);
''',
- 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('''
+ c_name=base.c_name())
+ else:
+ ret += mcgen('''
visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
- if (err) {
- goto out_obj;
- }
+''',
+ 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='kind',
+ name=variants.tag_member.name)
+ ret += gen_err_check(label='out_obj')
+ ret += mcgen('''
if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
goto out_obj;
}
switch ((*obj)->%(c_name)s) {
''',
- 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=c_name(variants.tag_name or 'kind'))
for var in variants.variants:
# TODO ugly special case for simple union
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 12/24] qapi: Start converting to new qapi union layout
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (10 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 11/24] qapi-visit: Remove redundant functions for flat union base Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 13/24] qapi-visit: Convert " Eric Blake
` (12 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
This patch is the front end for a series that converts to a
saner qapi union layout. By the end of the series, we will no
longer have the type/kind mismatch, and all tag values will be
under a named union, which requires clients to access
'obj->u.value' instead of 'obj->value'. But since the
conversion touches a number of files, it is easiest if we
temporarily support BOTH layouts simultaneously.
Given a simple union qapi type:
{ 'union':'Foo', 'data': { 'a':'int', 'b':'bool' } }
make the following changes in generated qapi-types.h:
| struct Foo {
|- FooKind kind;
|- union { /* union tag is @kind */
|+ union {
|+ FooKind kind;
|+ FooKind type;
|+ };
|+ union { /* union tag is @type */
| void *data;
| int64_t a;
| bool b;
|+ union { /* union tag is @type */
|+ void *data;
|+ int64_t a;
|+ bool b;
|+ } u;
| };
| };
Flat unions do not need the anonymous union for the tag member,
as we already fixed that to use the member name instead of 'kind'
back in commit 0f61af3e.
One additional change is needed in qapi.py: check_union() now
needs to check for collisions with 'type' in addition to those
with 'kind'.
Later, when the conversions are complete, we will remove the
duplication hacks, and also drop the check_union() restrictions.
Note, however, that we do not rename the generated enum, which
is still 'FooKind'. A further patch could generate implicit
enums as 'FooType', but while the generator already reserved
the '*Kind' namespace (commit 4dc2e69), there are already QMP
constructs with '*Type' naming, which means changing our
reservation namespace would have lots of churn to C code to
deal with a forced name change.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: pull qapi.py change back to this patch, but without removing
'kind' collision detection for less testsuite churn
v10: rebase to earlier changes, improve commit message, defer
qapi-visit and testsuite 'u' changes to later
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
scripts/qapi-types.py | 26 +++++++++++++++++++-------
scripts/qapi.py | 3 ++-
2 files changed, 21 insertions(+), 8 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index faf7022..1420e00 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -149,11 +149,23 @@ struct %(c_name)s {
if base:
ret += gen_struct_fields([], base)
else:
+ # TODO As a hack, we emit both 'kind' and 'type'. Ultimately, we
+ # want to use only 'type', but the conversion is large enough to
+ # require staging over several commits.
ret += mcgen('''
- %(c_type)s kind;
+ union {
+ %(c_type)s kind;
+ %(c_type)s type;
+ };
''',
c_type=c_name(variants.tag_member.type.name))
+ # TODO As a hack, we emit the union twice, once as an anonymous union
+ # and once as a named union. Ultimately, we want to use only the
+ # named union version (as it avoids conflicts between tag values as
+ # branch names competing with non-variant QMP names), but the conversion
+ # is large enough to require staging over several commits.
+ tmp = ''
# 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
# whether to bypass the switch statement if visiting the discriminator
@@ -162,25 +174,25 @@ struct %(c_name)s {
# should not be any data leaks even without a data pointer. Or, if
# 'data' is merely added to guarantee we don't have an empty union,
# shouldn't we enforce that at .json parse time?
- ret += mcgen('''
+ tmp += mcgen('''
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=c_name(variants.tag_member.name))
for var in variants.variants:
# Ugly special case for simple union TODO get rid of it
typ = var.simple_union_type() or var.type
- ret += mcgen('''
+ tmp += mcgen('''
%(c_type)s %(c_name)s;
''',
c_type=typ.c_type(),
c_name=c_name(var.name))
+ ret += tmp
+ ret += ' ' + '\n '.join(tmp.split('\n'))
ret += mcgen('''
+ } u;
};
};
''')
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 3941cd1..3fead04 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -548,7 +548,8 @@ def check_union(expr, expr_info):
base = expr.get('base')
discriminator = expr.get('discriminator')
members = expr['data']
- values = {'MAX': '(automatic)', 'KIND': '(automatic)'}
+ values = {'MAX': '(automatic)', 'KIND': '(automatic)',
+ 'TYPE': '(automatic)'}
# Two types of unions, determined by discriminator.
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 13/24] qapi-visit: Convert to new qapi union layout
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (11 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 12/24] qapi: Start converting to new qapi union layout Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 14/24] tests: " Eric Blake
` (11 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for qapi-visit.py.
Generated code changes look like:
|@@ -4912,16 +4912,16 @@ void visit_type_MemoryDeviceInfo(Visitor
| if (!*obj) {
| goto out_obj;
| }
|- visit_type_MemoryDeviceInfoKind(v, &(*obj)->kind, "type", &err);
|+ visit_type_MemoryDeviceInfoKind(v, &(*obj)->type, "type", &err);
| if (err) {
| goto out_obj;
| }
|- if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
|+ if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
| goto out_obj;
| }
|- switch ((*obj)->kind) {
|+ switch ((*obj)->type) {
| case MEMORY_DEVICE_INFO_KIND_DIMM:
|- visit_type_PCDIMMDeviceInfo(v, &(*obj)->dimm, "data", &err);
|+ visit_type_PCDIMMDeviceInfo(v, &(*obj)->u.dimm, "data", &err);
| break;
| default:
| abort();
|@@ -4930,7 +4930,7 @@ out_obj:
| error_propagate(errp, err);
| err = NULL;
| if (*obj) {
|- visit_end_union(v, !!(*obj)->data, &err);
|+ visit_end_union(v, !!(*obj)->u.data, &err);
| }
| error_propagate(errp, err);
| err = NULL;
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: hoist qapi.py change back to 13/25, and drop testsuite change;
enhance commit message
v10: new patch, split from 7/17 an 8/17
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
scripts/qapi-visit.py | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 33c013a..f40c3c7 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -189,18 +189,18 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
if (err) {
goto out;
}
- visit_get_next_type(v, (int*) &(*obj)->kind, %(c_name)s_qtypes, name, &err);
+ visit_get_next_type(v, (int*) &(*obj)->type, %(c_name)s_qtypes, name, &err);
if (err) {
goto out_obj;
}
- switch ((*obj)->kind) {
+ switch ((*obj)->type) {
''',
c_name=c_name(name))
for var in variants.variants:
ret += mcgen('''
case %(case)s:
- visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, name, &err);
+ visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, name, &err);
break;
''',
case=c_enum_const(variants.tag_member.type.name,
@@ -261,22 +261,16 @@ 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, "%(name)s", &err);
''',
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='kind',
+ c_name=c_name(variants.tag_member.name),
name=variants.tag_member.name)
ret += gen_err_check(label='out_obj')
ret += mcgen('''
- if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
+ if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
goto out_obj;
}
switch ((*obj)->%(c_name)s) {
''',
- # TODO ugly special case for simple union
- # Use same tag name in C as on the wire to get rid of
- # it, then: c_name=c_name(variants.tag_member.name)
- c_name=c_name(variants.tag_name or 'kind'))
+ c_name=c_name(variants.tag_member.name))
for var in variants.variants:
# TODO ugly special case for simple union
@@ -288,13 +282,13 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
var.name))
if simple_union_type:
ret += mcgen('''
- visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "data", &err);
+ visit_type_%(c_type)s(v, &(*obj)->u.%(c_name)s, "data", &err);
''',
c_type=simple_union_type.c_name(),
c_name=c_name(var.name))
else:
ret += mcgen('''
- visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
+ visit_type_implicit_%(c_type)s(v, &(*obj)->u.%(c_name)s, &err);
''',
c_type=var.type.c_name(),
c_name=c_name(var.name))
@@ -310,7 +304,7 @@ out_obj:
error_propagate(errp, err);
err = NULL;
if (*obj) {
- visit_end_union(v, !!(*obj)->data, &err);
+ visit_end_union(v, !!(*obj)->u.data, &err);
}
error_propagate(errp, err);
err = NULL;
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 14/24] tests: Convert to new qapi union layout
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (12 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 13/24] qapi-visit: Convert " Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 15/24] block: " Eric Blake
` (10 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for testsuite code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: rebase to earlier changes
v10: split qapi.py change into earlier less-mechanical patch
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
tests/test-qmp-commands.c | 4 +--
tests/test-qmp-input-visitor.c | 78 ++++++++++++++++++++---------------------
tests/test-qmp-output-visitor.c | 42 +++++++++++-----------
3 files changed, 62 insertions(+), 62 deletions(-)
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index ea700d8..f23d8ea 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -62,8 +62,8 @@ __org_qemu_x_Union1 *qmp___org_qemu_x_command(__org_qemu_x_EnumList *a,
{
__org_qemu_x_Union1 *ret = g_new0(__org_qemu_x_Union1, 1);
- ret->kind = ORG_QEMU_X_UNION1_KIND___ORG_QEMU_X_BRANCH;
- ret->__org_qemu_x_branch = strdup("blah1");
+ ret->type = ORG_QEMU_X_UNION1_KIND___ORG_QEMU_X_BRANCH;
+ ret->u.__org_qemu_x_branch = strdup("blah1");
return ret;
}
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 2d95db9..de65982 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -360,7 +360,7 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data,
g_assert_cmpint(tmp->enum1, ==, ENUM_ONE_VALUE1);
g_assert_cmpstr(tmp->string, ==, "str");
g_assert_cmpint(tmp->integer, ==, 41);
- g_assert_cmpint(tmp->value1->boolean, ==, true);
+ g_assert_cmpint(tmp->u.value1->boolean, ==, true);
base = qapi_UserDefFlatUnion_base(tmp);
g_assert(&base->enum1 == &tmp->enum1);
@@ -377,15 +377,15 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42");
visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
- g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_I);
- g_assert_cmpint(tmp->i, ==, 42);
+ g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_I);
+ g_assert_cmpint(tmp->u.i, ==, 42);
qapi_free_UserDefAlternate(tmp);
visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "'string'");
visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
- g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_S);
- g_assert_cmpstr(tmp->s, ==, "string");
+ g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_S);
+ g_assert_cmpstr(tmp->u.s, ==, "string");
qapi_free_UserDefAlternate(tmp);
visitor_input_teardown(data, NULL);
@@ -424,8 +424,8 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
* parse the same as ans */
v = visitor_input_test_init(data, "42");
visit_type_AltStrNum(v, &asn, NULL, &err);
- /* FIXME g_assert_cmpint(asn->kind, == ALT_STR_NUM_KIND_N); */
- /* FIXME g_assert_cmpfloat(asn->n, ==, 42); */
+ /* FIXME g_assert_cmpint(asn->type, == ALT_STR_NUM_KIND_N); */
+ /* FIXME g_assert_cmpfloat(asn->u.n, ==, 42); */
g_assert(err);
error_free(err);
err = NULL;
@@ -434,29 +434,29 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42");
visit_type_AltNumStr(v, &ans, NULL, &error_abort);
- g_assert_cmpint(ans->kind, ==, ALT_NUM_STR_KIND_N);
- g_assert_cmpfloat(ans->n, ==, 42);
+ g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N);
+ g_assert_cmpfloat(ans->u.n, ==, 42);
qapi_free_AltNumStr(ans);
visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "42");
visit_type_AltStrInt(v, &asi, NULL, &error_abort);
- g_assert_cmpint(asi->kind, ==, ALT_STR_INT_KIND_I);
- g_assert_cmpint(asi->i, ==, 42);
+ g_assert_cmpint(asi->type, ==, ALT_STR_INT_KIND_I);
+ g_assert_cmpint(asi->u.i, ==, 42);
qapi_free_AltStrInt(asi);
visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "42");
visit_type_AltIntNum(v, &ain, NULL, &error_abort);
- g_assert_cmpint(ain->kind, ==, ALT_INT_NUM_KIND_I);
- g_assert_cmpint(ain->i, ==, 42);
+ g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_I);
+ g_assert_cmpint(ain->u.i, ==, 42);
qapi_free_AltIntNum(ain);
visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "42");
visit_type_AltNumInt(v, &ani, NULL, &error_abort);
- g_assert_cmpint(ani->kind, ==, ALT_NUM_INT_KIND_I);
- g_assert_cmpint(ani->i, ==, 42);
+ g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_I);
+ g_assert_cmpint(ani->u.i, ==, 42);
qapi_free_AltNumInt(ani);
visitor_input_teardown(data, NULL);
@@ -472,15 +472,15 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42.5");
visit_type_AltStrNum(v, &asn, NULL, &error_abort);
- g_assert_cmpint(asn->kind, ==, ALT_STR_NUM_KIND_N);
- g_assert_cmpfloat(asn->n, ==, 42.5);
+ g_assert_cmpint(asn->type, ==, ALT_STR_NUM_KIND_N);
+ g_assert_cmpfloat(asn->u.n, ==, 42.5);
qapi_free_AltStrNum(asn);
visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "42.5");
visit_type_AltNumStr(v, &ans, NULL, &error_abort);
- g_assert_cmpint(ans->kind, ==, ALT_NUM_STR_KIND_N);
- g_assert_cmpfloat(ans->n, ==, 42.5);
+ g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N);
+ g_assert_cmpfloat(ans->u.n, ==, 42.5);
qapi_free_AltNumStr(ans);
visitor_input_teardown(data, NULL);
@@ -494,15 +494,15 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
v = visitor_input_test_init(data, "42.5");
visit_type_AltIntNum(v, &ain, NULL, &error_abort);
- g_assert_cmpint(ain->kind, ==, ALT_INT_NUM_KIND_N);
- g_assert_cmpfloat(ain->n, ==, 42.5);
+ g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_N);
+ g_assert_cmpfloat(ain->u.n, ==, 42.5);
qapi_free_AltIntNum(ain);
visitor_input_teardown(data, NULL);
v = visitor_input_test_init(data, "42.5");
visit_type_AltNumInt(v, &ani, NULL, &error_abort);
- g_assert_cmpint(ani->kind, ==, ALT_NUM_INT_KIND_N);
- g_assert_cmpfloat(ani->n, ==, 42.5);
+ g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_N);
+ g_assert_cmpfloat(ani->u.n, ==, 42.5);
qapi_free_AltNumInt(ani);
visitor_input_teardown(data, NULL);
}
@@ -532,68 +532,68 @@ static void test_native_list_integer_helper(TestInputVisitorData *data,
visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &err);
g_assert(err == NULL);
g_assert(cvalue != NULL);
- g_assert_cmpint(cvalue->kind, ==, kind);
+ g_assert_cmpint(cvalue->type, ==, kind);
switch (kind) {
case USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER: {
intList *elem = NULL;
- for (i = 0, elem = cvalue->integer; elem; elem = elem->next, i++) {
+ for (i = 0, elem = cvalue->u.integer; elem; elem = elem->next, i++) {
g_assert_cmpint(elem->value, ==, i);
}
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_S8: {
int8List *elem = NULL;
- for (i = 0, elem = cvalue->s8; elem; elem = elem->next, i++) {
+ for (i = 0, elem = cvalue->u.s8; elem; elem = elem->next, i++) {
g_assert_cmpint(elem->value, ==, i);
}
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_S16: {
int16List *elem = NULL;
- for (i = 0, elem = cvalue->s16; elem; elem = elem->next, i++) {
+ for (i = 0, elem = cvalue->u.s16; elem; elem = elem->next, i++) {
g_assert_cmpint(elem->value, ==, i);
}
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_S32: {
int32List *elem = NULL;
- for (i = 0, elem = cvalue->s32; elem; elem = elem->next, i++) {
+ for (i = 0, elem = cvalue->u.s32; elem; elem = elem->next, i++) {
g_assert_cmpint(elem->value, ==, i);
}
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_S64: {
int64List *elem = NULL;
- for (i = 0, elem = cvalue->s64; elem; elem = elem->next, i++) {
+ for (i = 0, elem = cvalue->u.s64; elem; elem = elem->next, i++) {
g_assert_cmpint(elem->value, ==, i);
}
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_U8: {
uint8List *elem = NULL;
- for (i = 0, elem = cvalue->u8; elem; elem = elem->next, i++) {
+ for (i = 0, elem = cvalue->u.u8; elem; elem = elem->next, i++) {
g_assert_cmpint(elem->value, ==, i);
}
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_U16: {
uint16List *elem = NULL;
- for (i = 0, elem = cvalue->u16; elem; elem = elem->next, i++) {
+ for (i = 0, elem = cvalue->u.u16; elem; elem = elem->next, i++) {
g_assert_cmpint(elem->value, ==, i);
}
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_U32: {
uint32List *elem = NULL;
- for (i = 0, elem = cvalue->u32; elem; elem = elem->next, i++) {
+ for (i = 0, elem = cvalue->u.u32; elem; elem = elem->next, i++) {
g_assert_cmpint(elem->value, ==, i);
}
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_U64: {
uint64List *elem = NULL;
- for (i = 0, elem = cvalue->u64; elem; elem = elem->next, i++) {
+ for (i = 0, elem = cvalue->u.u64; elem; elem = elem->next, i++) {
g_assert_cmpint(elem->value, ==, i);
}
break;
@@ -695,9 +695,9 @@ static void test_visitor_in_native_list_bool(TestInputVisitorData *data,
visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &err);
g_assert(err == NULL);
g_assert(cvalue != NULL);
- g_assert_cmpint(cvalue->kind, ==, USER_DEF_NATIVE_LIST_UNION_KIND_BOOLEAN);
+ g_assert_cmpint(cvalue->type, ==, USER_DEF_NATIVE_LIST_UNION_KIND_BOOLEAN);
- for (i = 0, elem = cvalue->boolean; elem; elem = elem->next, i++) {
+ for (i = 0, elem = cvalue->u.boolean; elem; elem = elem->next, i++) {
g_assert_cmpint(elem->value, ==, (i % 3 == 0) ? 1 : 0);
}
@@ -730,9 +730,9 @@ static void test_visitor_in_native_list_string(TestInputVisitorData *data,
visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &err);
g_assert(err == NULL);
g_assert(cvalue != NULL);
- g_assert_cmpint(cvalue->kind, ==, USER_DEF_NATIVE_LIST_UNION_KIND_STRING);
+ g_assert_cmpint(cvalue->type, ==, USER_DEF_NATIVE_LIST_UNION_KIND_STRING);
- for (i = 0, elem = cvalue->string; elem; elem = elem->next, i++) {
+ for (i = 0, elem = cvalue->u.string; elem; elem = elem->next, i++) {
gchar str[8];
sprintf(str, "%d", i);
g_assert_cmpstr(elem->value, ==, str);
@@ -769,9 +769,9 @@ static void test_visitor_in_native_list_number(TestInputVisitorData *data,
visit_type_UserDefNativeListUnion(v, &cvalue, NULL, &err);
g_assert(err == NULL);
g_assert(cvalue != NULL);
- g_assert_cmpint(cvalue->kind, ==, USER_DEF_NATIVE_LIST_UNION_KIND_NUMBER);
+ g_assert_cmpint(cvalue->type, ==, USER_DEF_NATIVE_LIST_UNION_KIND_NUMBER);
- for (i = 0, elem = cvalue->number; elem; elem = elem->next, i++) {
+ for (i = 0, elem = cvalue->u.number; elem; elem = elem->next, i++) {
GString *double_expected = g_string_new("");
GString *double_actual = g_string_new("");
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index cfb06bb..09d0dd8 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -487,9 +487,9 @@ static void test_visitor_out_union_flat(TestOutputVisitorData *data,
UserDefFlatUnion *tmp = g_malloc0(sizeof(UserDefFlatUnion));
tmp->enum1 = ENUM_ONE_VALUE1;
tmp->string = g_strdup("str");
- tmp->value1 = g_malloc0(sizeof(UserDefA));
- /* TODO when generator bug is fixed: tmp->integer = 41; */
- tmp->value1->boolean = true;
+ tmp->u.value1 = g_malloc0(sizeof(UserDefA));
+ tmp->integer = 41;
+ tmp->u.value1->boolean = true;
visit_type_UserDefFlatUnion(data->ov, &tmp, NULL, &err);
g_assert(err == NULL);
@@ -500,7 +500,7 @@ static void test_visitor_out_union_flat(TestOutputVisitorData *data,
g_assert_cmpstr(qdict_get_str(qdict, "enum1"), ==, "value1");
g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, "str");
- /* TODO g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 41); */
+ g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 41);
g_assert_cmpint(qdict_get_bool(qdict, "boolean"), ==, true);
qapi_free_UserDefFlatUnion(tmp);
@@ -514,8 +514,8 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
Error *err = NULL;
UserDefAlternate *tmp = g_malloc0(sizeof(UserDefAlternate));
- tmp->kind = USER_DEF_ALTERNATE_KIND_I;
- tmp->i = 42;
+ tmp->type = USER_DEF_ALTERNATE_KIND_I;
+ tmp->u.i = 42;
visit_type_UserDefAlternate(data->ov, &tmp, NULL, &err);
g_assert(err == NULL);
@@ -540,9 +540,9 @@ static void test_visitor_out_empty(TestOutputVisitorData *data,
static void init_native_list(UserDefNativeListUnion *cvalue)
{
int i;
- switch (cvalue->kind) {
+ switch (cvalue->type) {
case USER_DEF_NATIVE_LIST_UNION_KIND_INTEGER: {
- intList **list = &cvalue->integer;
+ intList **list = &cvalue->u.integer;
for (i = 0; i < 32; i++) {
*list = g_new0(intList, 1);
(*list)->value = i;
@@ -552,7 +552,7 @@ static void init_native_list(UserDefNativeListUnion *cvalue)
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_S8: {
- int8List **list = &cvalue->s8;
+ int8List **list = &cvalue->u.s8;
for (i = 0; i < 32; i++) {
*list = g_new0(int8List, 1);
(*list)->value = i;
@@ -562,7 +562,7 @@ static void init_native_list(UserDefNativeListUnion *cvalue)
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_S16: {
- int16List **list = &cvalue->s16;
+ int16List **list = &cvalue->u.s16;
for (i = 0; i < 32; i++) {
*list = g_new0(int16List, 1);
(*list)->value = i;
@@ -572,7 +572,7 @@ static void init_native_list(UserDefNativeListUnion *cvalue)
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_S32: {
- int32List **list = &cvalue->s32;
+ int32List **list = &cvalue->u.s32;
for (i = 0; i < 32; i++) {
*list = g_new0(int32List, 1);
(*list)->value = i;
@@ -582,7 +582,7 @@ static void init_native_list(UserDefNativeListUnion *cvalue)
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_S64: {
- int64List **list = &cvalue->s64;
+ int64List **list = &cvalue->u.s64;
for (i = 0; i < 32; i++) {
*list = g_new0(int64List, 1);
(*list)->value = i;
@@ -592,7 +592,7 @@ static void init_native_list(UserDefNativeListUnion *cvalue)
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_U8: {
- uint8List **list = &cvalue->u8;
+ uint8List **list = &cvalue->u.u8;
for (i = 0; i < 32; i++) {
*list = g_new0(uint8List, 1);
(*list)->value = i;
@@ -602,7 +602,7 @@ static void init_native_list(UserDefNativeListUnion *cvalue)
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_U16: {
- uint16List **list = &cvalue->u16;
+ uint16List **list = &cvalue->u.u16;
for (i = 0; i < 32; i++) {
*list = g_new0(uint16List, 1);
(*list)->value = i;
@@ -612,7 +612,7 @@ static void init_native_list(UserDefNativeListUnion *cvalue)
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_U32: {
- uint32List **list = &cvalue->u32;
+ uint32List **list = &cvalue->u.u32;
for (i = 0; i < 32; i++) {
*list = g_new0(uint32List, 1);
(*list)->value = i;
@@ -622,7 +622,7 @@ static void init_native_list(UserDefNativeListUnion *cvalue)
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_U64: {
- uint64List **list = &cvalue->u64;
+ uint64List **list = &cvalue->u.u64;
for (i = 0; i < 32; i++) {
*list = g_new0(uint64List, 1);
(*list)->value = i;
@@ -632,7 +632,7 @@ static void init_native_list(UserDefNativeListUnion *cvalue)
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_BOOLEAN: {
- boolList **list = &cvalue->boolean;
+ boolList **list = &cvalue->u.boolean;
for (i = 0; i < 32; i++) {
*list = g_new0(boolList, 1);
(*list)->value = (i % 3 == 0);
@@ -642,7 +642,7 @@ static void init_native_list(UserDefNativeListUnion *cvalue)
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_STRING: {
- strList **list = &cvalue->string;
+ strList **list = &cvalue->u.string;
for (i = 0; i < 32; i++) {
*list = g_new0(strList, 1);
(*list)->value = g_strdup_printf("%d", i);
@@ -652,7 +652,7 @@ static void init_native_list(UserDefNativeListUnion *cvalue)
break;
}
case USER_DEF_NATIVE_LIST_UNION_KIND_NUMBER: {
- numberList **list = &cvalue->number;
+ numberList **list = &cvalue->u.number;
for (i = 0; i < 32; i++) {
*list = g_new0(numberList, 1);
(*list)->value = (double)i / 3;
@@ -761,14 +761,14 @@ static void test_native_list(TestOutputVisitorData *data,
Error *err = NULL;
QObject *obj;
- cvalue->kind = kind;
+ cvalue->type = kind;
init_native_list(cvalue);
visit_type_UserDefNativeListUnion(data->ov, &cvalue, NULL, &err);
g_assert(err == NULL);
obj = qmp_output_get_qobject(data->qov);
- check_native_list(obj, cvalue->kind);
+ check_native_list(obj, cvalue->type);
qapi_free_UserDefNativeListUnion(cvalue);
qobject_decref(obj);
}
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 15/24] block: Convert to new qapi union layout
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (13 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 14/24] tests: " Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 16/24] sockets: " Eric Blake
` (9 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Paolo Bonzini, Fam Zheng, armbru,
open list:Block layer core
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for block-related code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: no change
v10: no change
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
block/nbd.c | 18 +++++++++---------
block/qcow2.c | 10 ++++------
block/vmdk.c | 6 +++---
blockdev.c | 47 ++++++++++++++++++++++++-----------------------
4 files changed, 40 insertions(+), 41 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index c2a87e9..cd6a587 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -206,24 +206,24 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char **export,
saddr = g_new0(SocketAddress, 1);
if (qdict_haskey(options, "path")) {
- saddr->kind = SOCKET_ADDRESS_KIND_UNIX;
- saddr->q_unix = g_new0(UnixSocketAddress, 1);
- saddr->q_unix->path = g_strdup(qdict_get_str(options, "path"));
+ saddr->type = SOCKET_ADDRESS_KIND_UNIX;
+ saddr->u.q_unix = g_new0(UnixSocketAddress, 1);
+ saddr->u.q_unix->path = g_strdup(qdict_get_str(options, "path"));
qdict_del(options, "path");
} else {
- saddr->kind = SOCKET_ADDRESS_KIND_INET;
- saddr->inet = g_new0(InetSocketAddress, 1);
- saddr->inet->host = g_strdup(qdict_get_str(options, "host"));
+ saddr->type = SOCKET_ADDRESS_KIND_INET;
+ saddr->u.inet = g_new0(InetSocketAddress, 1);
+ saddr->u.inet->host = g_strdup(qdict_get_str(options, "host"));
if (!qdict_get_try_str(options, "port")) {
- saddr->inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
+ saddr->u.inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
} else {
- saddr->inet->port = g_strdup(qdict_get_str(options, "port"));
+ saddr->u.inet->port = g_strdup(qdict_get_str(options, "port"));
}
qdict_del(options, "host");
qdict_del(options, "port");
}
- s->client.is_unix = saddr->kind == SOCKET_ADDRESS_KIND_UNIX;
+ s->client.is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
*export = g_strdup(qdict_get_try_str(options, "export"));
if (*export) {
diff --git a/block/qcow2.c b/block/qcow2.c
index bacc4f2..88f56c8 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2738,18 +2738,16 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
*spec_info = (ImageInfoSpecific){
- .kind = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
- {
- .qcow2 = g_new(ImageInfoSpecificQCow2, 1),
- },
+ .type = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
+ .u.qcow2 = g_new(ImageInfoSpecificQCow2, 1),
};
if (s->qcow_version == 2) {
- *spec_info->qcow2 = (ImageInfoSpecificQCow2){
+ *spec_info->u.qcow2 = (ImageInfoSpecificQCow2){
.compat = g_strdup("0.10"),
.refcount_bits = s->refcount_bits,
};
} else if (s->qcow_version == 3) {
- *spec_info->qcow2 = (ImageInfoSpecificQCow2){
+ *spec_info->u.qcow2 = (ImageInfoSpecificQCow2){
.compat = g_strdup("1.1"),
.lazy_refcounts = s->compatible_features &
QCOW2_COMPAT_LAZY_REFCOUNTS,
diff --git a/block/vmdk.c b/block/vmdk.c
index 0effb7d..6f819e4 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2161,19 +2161,19 @@ static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs)
ImageInfoList **next;
*spec_info = (ImageInfoSpecific){
- .kind = IMAGE_INFO_SPECIFIC_KIND_VMDK,
+ .type = IMAGE_INFO_SPECIFIC_KIND_VMDK,
{
.vmdk = g_new0(ImageInfoSpecificVmdk, 1),
},
};
- *spec_info->vmdk = (ImageInfoSpecificVmdk) {
+ *spec_info->u.vmdk = (ImageInfoSpecificVmdk) {
.create_type = g_strdup(s->create_type),
.cid = s->cid,
.parent_cid = s->parent_cid,
};
- next = &spec_info->vmdk->extents;
+ next = &spec_info->u.vmdk->extents;
for (i = 0; i < s->num_extents; i++) {
*next = g_new0(ImageInfoList, 1);
(*next)->value = vmdk_get_extent_info(&s->extents[i]);
diff --git a/blockdev.c b/blockdev.c
index 18712d2..8b8bfa9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1137,13 +1137,14 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
}
}
-static void blockdev_do_action(int kind, void *data, Error **errp)
+static void blockdev_do_action(TransactionActionKind type, void *data,
+ Error **errp)
{
TransactionAction action;
TransactionActionList list;
- action.kind = kind;
- action.data = data;
+ action.type = type;
+ action.u.data = data;
list.value = &action;
list.next = NULL;
qmp_transaction(&list, errp);
@@ -1388,9 +1389,9 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
InternalSnapshotState *state;
int ret1;
- g_assert(common->action->kind ==
+ g_assert(common->action->type ==
TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC);
- internal = common->action->blockdev_snapshot_internal_sync;
+ internal = common->action->u.blockdev_snapshot_internal_sync;
state = DO_UPCAST(InternalSnapshotState, common, common);
/* 1. parse input */
@@ -1536,22 +1537,22 @@ static void external_snapshot_prepare(BlkTransactionState *common,
TransactionAction *action = common->action;
/* get parameters */
- g_assert(action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
+ g_assert(action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
- has_device = action->blockdev_snapshot_sync->has_device;
- device = action->blockdev_snapshot_sync->device;
- has_node_name = action->blockdev_snapshot_sync->has_node_name;
- node_name = action->blockdev_snapshot_sync->node_name;
+ has_device = action->u.blockdev_snapshot_sync->has_device;
+ device = action->u.blockdev_snapshot_sync->device;
+ has_node_name = action->u.blockdev_snapshot_sync->has_node_name;
+ node_name = action->u.blockdev_snapshot_sync->node_name;
has_snapshot_node_name =
- action->blockdev_snapshot_sync->has_snapshot_node_name;
- snapshot_node_name = action->blockdev_snapshot_sync->snapshot_node_name;
+ action->u.blockdev_snapshot_sync->has_snapshot_node_name;
+ snapshot_node_name = action->u.blockdev_snapshot_sync->snapshot_node_name;
- new_image_file = action->blockdev_snapshot_sync->snapshot_file;
- if (action->blockdev_snapshot_sync->has_format) {
- format = action->blockdev_snapshot_sync->format;
+ new_image_file = action->u.blockdev_snapshot_sync->snapshot_file;
+ if (action->u.blockdev_snapshot_sync->has_format) {
+ format = action->u.blockdev_snapshot_sync->format;
}
- if (action->blockdev_snapshot_sync->has_mode) {
- mode = action->blockdev_snapshot_sync->mode;
+ if (action->u.blockdev_snapshot_sync->has_mode) {
+ mode = action->u.blockdev_snapshot_sync->mode;
}
/* start processing */
@@ -1681,8 +1682,8 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
DriveBackup *backup;
Error *local_err = NULL;
- assert(common->action->kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
- backup = common->action->drive_backup;
+ assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
+ backup = common->action->u.drive_backup;
blk = blk_by_name(backup->device);
if (!blk) {
@@ -1754,8 +1755,8 @@ static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
BlockBackend *blk, *target;
Error *local_err = NULL;
- assert(common->action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
- backup = common->action->blockdev_backup;
+ assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
+ backup = common->action->u.blockdev_backup;
blk = blk_by_name(backup->device);
if (!blk) {
@@ -1887,9 +1888,9 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
dev_info = dev_entry->value;
dev_entry = dev_entry->next;
- assert(dev_info->kind < ARRAY_SIZE(actions));
+ assert(dev_info->type < ARRAY_SIZE(actions));
- ops = &actions[dev_info->kind];
+ ops = &actions[dev_info->type];
assert(ops->instance_size > 0);
state = g_malloc0(ops->instance_size);
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 16/24] sockets: Convert to new qapi union layout
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (14 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 15/24] block: " Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 17/24] net: " Eric Blake
` (8 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, armbru, Gerd Hoffmann
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for socket-related code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: no change
v10: rebase to latest, retitle from nbd to sockets
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
qemu-nbd.c | 16 +++++++-------
ui/vnc.c | 44 ++++++++++++++++++-------------------
util/qemu-sockets.c | 62 ++++++++++++++++++++++++++---------------------------
3 files changed, 61 insertions(+), 61 deletions(-)
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 422a607..3afec76 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -362,17 +362,17 @@ static SocketAddress *nbd_build_socket_address(const char *sockpath,
saddr = g_new0(SocketAddress, 1);
if (sockpath) {
- saddr->kind = SOCKET_ADDRESS_KIND_UNIX;
- saddr->q_unix = g_new0(UnixSocketAddress, 1);
- saddr->q_unix->path = g_strdup(sockpath);
+ saddr->type = SOCKET_ADDRESS_KIND_UNIX;
+ saddr->u.q_unix = g_new0(UnixSocketAddress, 1);
+ saddr->u.q_unix->path = g_strdup(sockpath);
} else {
- saddr->kind = SOCKET_ADDRESS_KIND_INET;
- saddr->inet = g_new0(InetSocketAddress, 1);
- saddr->inet->host = g_strdup(bindto);
+ saddr->type = SOCKET_ADDRESS_KIND_INET;
+ saddr->u.inet = g_new0(InetSocketAddress, 1);
+ saddr->u.inet->host = g_strdup(bindto);
if (port) {
- saddr->inet->port = g_strdup(port);
+ saddr->u.inet->port = g_strdup(port);
} else {
- saddr->inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
+ saddr->u.inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT);
}
}
diff --git a/ui/vnc.c b/ui/vnc.c
index cec2cee..7b37e3b 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3524,9 +3524,9 @@ void vnc_display_open(const char *id, Error **errp)
}
if (strncmp(vnc, "unix:", 5) == 0) {
- saddr->kind = SOCKET_ADDRESS_KIND_UNIX;
- saddr->q_unix = g_new0(UnixSocketAddress, 1);
- saddr->q_unix->path = g_strdup(vnc + 5);
+ saddr->type = SOCKET_ADDRESS_KIND_UNIX;
+ saddr->u.q_unix = g_new0(UnixSocketAddress, 1);
+ saddr->u.q_unix->path = g_strdup(vnc + 5);
if (vs->ws_enabled) {
error_setg(errp, "UNIX sockets not supported with websock");
@@ -3534,12 +3534,12 @@ void vnc_display_open(const char *id, Error **errp)
}
} else {
unsigned long long baseport;
- saddr->kind = SOCKET_ADDRESS_KIND_INET;
- saddr->inet = g_new0(InetSocketAddress, 1);
+ saddr->type = SOCKET_ADDRESS_KIND_INET;
+ saddr->u.inet = g_new0(InetSocketAddress, 1);
if (vnc[0] == '[' && vnc[hlen - 1] == ']') {
- saddr->inet->host = g_strndup(vnc + 1, hlen - 2);
+ saddr->u.inet->host = g_strndup(vnc + 1, hlen - 2);
} else {
- saddr->inet->host = g_strndup(vnc, hlen);
+ saddr->u.inet->host = g_strndup(vnc, hlen);
}
if (parse_uint_full(h + 1, &baseport, 10) < 0) {
error_setg(errp, "can't convert to a number: %s", h + 1);
@@ -3550,28 +3550,28 @@ void vnc_display_open(const char *id, Error **errp)
error_setg(errp, "port %s out of range", h + 1);
goto fail;
}
- saddr->inet->port = g_strdup_printf(
+ saddr->u.inet->port = g_strdup_printf(
"%d", (int)baseport + 5900);
if (to) {
- saddr->inet->has_to = true;
- saddr->inet->to = to;
+ saddr->u.inet->has_to = true;
+ saddr->u.inet->to = to;
}
- saddr->inet->ipv4 = saddr->inet->has_ipv4 = has_ipv4;
- saddr->inet->ipv6 = saddr->inet->has_ipv6 = has_ipv6;
+ saddr->u.inet->ipv4 = saddr->u.inet->has_ipv4 = has_ipv4;
+ saddr->u.inet->ipv6 = saddr->u.inet->has_ipv6 = has_ipv6;
if (vs->ws_enabled) {
- wsaddr->kind = SOCKET_ADDRESS_KIND_INET;
- wsaddr->inet = g_new0(InetSocketAddress, 1);
- wsaddr->inet->host = g_strdup(saddr->inet->host);
- wsaddr->inet->port = g_strdup(websocket);
+ wsaddr->type = SOCKET_ADDRESS_KIND_INET;
+ wsaddr->u.inet = g_new0(InetSocketAddress, 1);
+ wsaddr->u.inet->host = g_strdup(saddr->u.inet->host);
+ wsaddr->u.inet->port = g_strdup(websocket);
if (to) {
- wsaddr->inet->has_to = true;
- wsaddr->inet->to = to;
+ wsaddr->u.inet->has_to = true;
+ wsaddr->u.inet->to = to;
}
- wsaddr->inet->ipv4 = wsaddr->inet->has_ipv4 = has_ipv4;
- wsaddr->inet->ipv6 = wsaddr->inet->has_ipv6 = has_ipv6;
+ wsaddr->u.inet->ipv4 = wsaddr->u.inet->has_ipv4 = has_ipv4;
+ wsaddr->u.inet->ipv6 = wsaddr->u.inet->has_ipv6 = has_ipv6;
}
}
} else {
@@ -3770,7 +3770,7 @@ void vnc_display_open(const char *id, Error **errp)
if (csock < 0) {
goto fail;
}
- vs->is_unix = saddr->kind == SOCKET_ADDRESS_KIND_UNIX;
+ vs->is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
vnc_connect(vs, csock, false, false);
} else {
/* listen for connects */
@@ -3778,7 +3778,7 @@ void vnc_display_open(const char *id, Error **errp)
if (vs->lsock < 0) {
goto fail;
}
- vs->is_unix = saddr->kind == SOCKET_ADDRESS_KIND_UNIX;
+ vs->is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
if (vs->ws_enabled) {
vs->lwebsock = socket_listen(wsaddr, errp);
if (vs->lwebsock < 0) {
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 9142917..dfe4587 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -918,23 +918,23 @@ SocketAddress *socket_parse(const char *str, Error **errp)
error_setg(errp, "invalid Unix socket address");
goto fail;
} else {
- addr->kind = SOCKET_ADDRESS_KIND_UNIX;
- addr->q_unix = g_new(UnixSocketAddress, 1);
- addr->q_unix->path = g_strdup(str + 5);
+ addr->type = SOCKET_ADDRESS_KIND_UNIX;
+ addr->u.q_unix = g_new(UnixSocketAddress, 1);
+ addr->u.q_unix->path = g_strdup(str + 5);
}
} else if (strstart(str, "fd:", NULL)) {
if (str[3] == '\0') {
error_setg(errp, "invalid file descriptor address");
goto fail;
} else {
- addr->kind = SOCKET_ADDRESS_KIND_FD;
- addr->fd = g_new(String, 1);
- addr->fd->str = g_strdup(str + 3);
+ addr->type = SOCKET_ADDRESS_KIND_FD;
+ addr->u.fd = g_new(String, 1);
+ addr->u.fd->str = g_strdup(str + 3);
}
} else {
- addr->kind = SOCKET_ADDRESS_KIND_INET;
- addr->inet = inet_parse(str, errp);
- if (addr->inet == NULL) {
+ addr->type = SOCKET_ADDRESS_KIND_INET;
+ addr->u.inet = inet_parse(str, errp);
+ if (addr->u.inet == NULL) {
goto fail;
}
}
@@ -952,19 +952,19 @@ int socket_connect(SocketAddress *addr, Error **errp,
int fd;
opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
- switch (addr->kind) {
+ switch (addr->type) {
case SOCKET_ADDRESS_KIND_INET:
- inet_addr_to_opts(opts, addr->inet);
+ inet_addr_to_opts(opts, addr->u.inet);
fd = inet_connect_opts(opts, errp, callback, opaque);
break;
case SOCKET_ADDRESS_KIND_UNIX:
- qemu_opt_set(opts, "path", addr->q_unix->path, &error_abort);
+ qemu_opt_set(opts, "path", addr->u.q_unix->path, &error_abort);
fd = unix_connect_opts(opts, errp, callback, opaque);
break;
case SOCKET_ADDRESS_KIND_FD:
- fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
+ fd = monitor_get_fd(cur_mon, addr->u.fd->str, errp);
if (fd >= 0 && callback) {
qemu_set_nonblock(fd);
callback(fd, NULL, opaque);
@@ -984,19 +984,19 @@ int socket_listen(SocketAddress *addr, Error **errp)
int fd;
opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
- switch (addr->kind) {
+ switch (addr->type) {
case SOCKET_ADDRESS_KIND_INET:
- inet_addr_to_opts(opts, addr->inet);
+ inet_addr_to_opts(opts, addr->u.inet);
fd = inet_listen_opts(opts, 0, errp);
break;
case SOCKET_ADDRESS_KIND_UNIX:
- qemu_opt_set(opts, "path", addr->q_unix->path, &error_abort);
+ qemu_opt_set(opts, "path", addr->u.q_unix->path, &error_abort);
fd = unix_listen_opts(opts, errp);
break;
case SOCKET_ADDRESS_KIND_FD:
- fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
+ fd = monitor_get_fd(cur_mon, addr->u.fd->str, errp);
break;
default:
@@ -1012,12 +1012,12 @@ int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp)
int fd;
opts = qemu_opts_create(&socket_optslist, NULL, 0, &error_abort);
- switch (remote->kind) {
+ switch (remote->type) {
case SOCKET_ADDRESS_KIND_INET:
- inet_addr_to_opts(opts, remote->inet);
+ inet_addr_to_opts(opts, remote->u.inet);
if (local) {
- qemu_opt_set(opts, "localaddr", local->inet->host, &error_abort);
- qemu_opt_set(opts, "localport", local->inet->port, &error_abort);
+ qemu_opt_set(opts, "localaddr", local->u.inet->host, &error_abort);
+ qemu_opt_set(opts, "localport", local->u.inet->port, &error_abort);
}
fd = inet_dgram_opts(opts, errp);
break;
@@ -1052,14 +1052,14 @@ socket_sockaddr_to_address_inet(struct sockaddr_storage *sa,
}
addr = g_new0(SocketAddress, 1);
- addr->kind = SOCKET_ADDRESS_KIND_INET;
- addr->inet = g_new0(InetSocketAddress, 1);
- addr->inet->host = g_strdup(host);
- addr->inet->port = g_strdup(serv);
+ addr->type = SOCKET_ADDRESS_KIND_INET;
+ addr->u.inet = g_new0(InetSocketAddress, 1);
+ addr->u.inet->host = g_strdup(host);
+ addr->u.inet->port = g_strdup(serv);
if (sa->ss_family == AF_INET) {
- addr->inet->has_ipv4 = addr->inet->ipv4 = true;
+ addr->u.inet->has_ipv4 = addr->u.inet->ipv4 = true;
} else {
- addr->inet->has_ipv6 = addr->inet->ipv6 = true;
+ addr->u.inet->has_ipv6 = addr->u.inet->ipv6 = true;
}
return addr;
@@ -1076,11 +1076,11 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
struct sockaddr_un *su = (struct sockaddr_un *)sa;
addr = g_new0(SocketAddress, 1);
- addr->kind = SOCKET_ADDRESS_KIND_UNIX;
- addr->q_unix = g_new0(UnixSocketAddress, 1);
+ addr->type = SOCKET_ADDRESS_KIND_UNIX;
+ addr->u.q_unix = g_new0(UnixSocketAddress, 1);
if (su->sun_path[0]) {
- addr->q_unix->path = g_strndup(su->sun_path,
- sizeof(su->sun_path));
+ addr->u.q_unix->path = g_strndup(su->sun_path,
+ sizeof(su->sun_path));
}
return addr;
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 17/24] net: Convert to new qapi union layout
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (15 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 16/24] sockets: " Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 18/24] char: " Eric Blake
` (7 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel; +Cc: Jason Wang, armbru, Michael S. Tsirkin
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for net-related code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: no change
v10: no change
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
net/dump.c | 4 ++--
net/hub.c | 4 ++--
net/l2tpv3.c | 4 ++--
net/net.c | 24 ++++++++++++------------
net/slirp.c | 4 ++--
net/socket.c | 4 ++--
net/tap-win32.c | 4 ++--
net/tap.c | 8 ++++----
net/vde.c | 4 ++--
net/vhost-user.c | 4 ++--
10 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/net/dump.c b/net/dump.c
index 08259af..2812070 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -154,8 +154,8 @@ int net_init_dump(const NetClientOptions *opts, const char *name,
char def_file[128];
const NetdevDumpOptions *dump;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_DUMP);
- dump = opts->dump;
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_DUMP);
+ dump = opts->u.dump;
assert(peer);
diff --git a/net/hub.c b/net/hub.c
index 3047f12..9ae9f01 100644
--- a/net/hub.c
+++ b/net/hub.c
@@ -285,9 +285,9 @@ int net_init_hubport(const NetClientOptions *opts, const char *name,
{
const NetdevHubPortOptions *hubport;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_HUBPORT);
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_HUBPORT);
assert(!peer);
- hubport = opts->hubport;
+ hubport = opts->u.hubport;
net_hub_add_port(hubport->hubid, name);
return 0;
diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index 4f9bcee..8e68e54 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -545,8 +545,8 @@ int net_init_l2tpv3(const NetClientOptions *opts,
s->queue_tail = 0;
s->header_mismatch = false;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_L2TPV3);
- l2tpv3 = opts->l2tpv3;
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_L2TPV3);
+ l2tpv3 = opts->u.l2tpv3;
if (l2tpv3->has_ipv6 && l2tpv3->ipv6) {
s->ipv6 = l2tpv3->ipv6;
diff --git a/net/net.c b/net/net.c
index 3c68f3f..d5aeb31 100644
--- a/net/net.c
+++ b/net/net.c
@@ -882,8 +882,8 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
NICInfo *nd;
const NetLegacyNicOptions *nic;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_NIC);
- nic = opts->nic;
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_NIC);
+ nic = opts->u.nic;
idx = nic_get_free_idx();
if (idx == -1 || nb_nics >= MAX_NICS) {
@@ -984,9 +984,9 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
opts = netdev->opts;
name = netdev->id;
- if (opts->kind == NET_CLIENT_OPTIONS_KIND_DUMP ||
- opts->kind == NET_CLIENT_OPTIONS_KIND_NIC ||
- !net_client_init_fun[opts->kind]) {
+ if (opts->type == NET_CLIENT_OPTIONS_KIND_DUMP ||
+ opts->type == NET_CLIENT_OPTIONS_KIND_NIC ||
+ !net_client_init_fun[opts->type]) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
"a netdev backend type");
return -1;
@@ -997,16 +997,16 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
/* missing optional values have been initialized to "all bits zero" */
name = net->has_id ? net->id : net->name;
- if (opts->kind == NET_CLIENT_OPTIONS_KIND_NONE) {
+ if (opts->type == NET_CLIENT_OPTIONS_KIND_NONE) {
return 0; /* nothing to do */
}
- if (opts->kind == NET_CLIENT_OPTIONS_KIND_HUBPORT) {
+ if (opts->type == NET_CLIENT_OPTIONS_KIND_HUBPORT) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
"a net type");
return -1;
}
- if (!net_client_init_fun[opts->kind]) {
+ if (!net_client_init_fun[opts->type]) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type",
"a net backend type (maybe it is not compiled "
"into this binary)");
@@ -1014,17 +1014,17 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
}
/* Do not add to a vlan if it's a nic with a netdev= parameter. */
- if (opts->kind != NET_CLIENT_OPTIONS_KIND_NIC ||
- !opts->nic->has_netdev) {
+ if (opts->type != NET_CLIENT_OPTIONS_KIND_NIC ||
+ !opts->u.nic->has_netdev) {
peer = net_hub_add_port(net->has_vlan ? net->vlan : 0, NULL);
}
}
- if (net_client_init_fun[opts->kind](opts, name, peer, errp) < 0) {
+ if (net_client_init_fun[opts->type](opts, name, peer, errp) < 0) {
/* FIXME drop when all init functions store an Error */
if (errp && !*errp) {
error_setg(errp, QERR_DEVICE_INIT_FAILED,
- NetClientOptionsKind_lookup[opts->kind]);
+ NetClientOptionsKind_lookup[opts->type]);
}
return -1;
}
diff --git a/net/slirp.c b/net/slirp.c
index 7657b38..f505570 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -746,8 +746,8 @@ int net_init_slirp(const NetClientOptions *opts, const char *name,
const NetdevUserOptions *user;
const char **dnssearch;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_USER);
- user = opts->user;
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_USER);
+ user = opts->u.user;
vnet = user->has_net ? g_strdup(user->net) :
user->has_ip ? g_strdup_printf("%s/24", user->ip) :
diff --git a/net/socket.c b/net/socket.c
index b1e3b1c..e8605d4 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -706,8 +706,8 @@ int net_init_socket(const NetClientOptions *opts, const char *name,
Error *err = NULL;
const NetdevSocketOptions *sock;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_SOCKET);
- sock = opts->socket;
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_SOCKET);
+ sock = opts->u.socket;
if (sock->has_fd + sock->has_listen + sock->has_connect + sock->has_mcast +
sock->has_udp != 1) {
diff --git a/net/tap-win32.c b/net/tap-win32.c
index 625d53c..4e2fa55 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -767,8 +767,8 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
/* FIXME error_setg(errp, ...) on failure */
const NetdevTapOptions *tap;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_TAP);
- tap = opts->tap;
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_TAP);
+ tap = opts->u.tap;
if (!tap->has_ifname) {
error_report("tap: no interface name");
diff --git a/net/tap.c b/net/tap.c
index bd01590..85c4142 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -565,8 +565,8 @@ int net_init_bridge(const NetClientOptions *opts, const char *name,
TAPState *s;
int fd, vnet_hdr;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_BRIDGE);
- bridge = opts->bridge;
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_BRIDGE);
+ bridge = opts->u.bridge;
helper = bridge->has_helper ? bridge->helper : DEFAULT_BRIDGE_HELPER;
br = bridge->has_br ? bridge->br : DEFAULT_BRIDGE_INTERFACE;
@@ -728,8 +728,8 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
const char *vhostfdname;
char ifname[128];
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_TAP);
- tap = opts->tap;
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_TAP);
+ tap = opts->u.tap;
queues = tap->has_queues ? tap->queues : 1;
vhostfdname = tap->has_vhostfd ? tap->vhostfd : NULL;
diff --git a/net/vde.c b/net/vde.c
index dacaa64..4475d92 100644
--- a/net/vde.c
+++ b/net/vde.c
@@ -115,8 +115,8 @@ int net_init_vde(const NetClientOptions *opts, const char *name,
/* FIXME error_setg(errp, ...) on failure */
const NetdevVdeOptions *vde;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_VDE);
- vde = opts->vde;
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_VDE);
+ vde = opts->u.vde;
/* missing optional values have been initialized to "all bits zero" */
if (net_vde_init(peer, "vde", name, vde->sock, vde->port, vde->group,
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 17b5c2a..0ebd7df 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -301,8 +301,8 @@ int net_init_vhost_user(const NetClientOptions *opts, const char *name,
const NetdevVhostUserOptions *vhost_user_opts;
CharDriverState *chr;
- assert(opts->kind == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
- vhost_user_opts = opts->vhost_user;
+ assert(opts->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
+ vhost_user_opts = opts->u.vhost_user;
chr = net_vhost_parse_chardev(vhost_user_opts, errp);
if (!chr) {
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 18/24] char: Convert to new qapi union layout
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (16 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 17/24] net: " Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 19/24] input: " Eric Blake
` (6 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, armbru
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for character-related
code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: no change
v10: no change
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
qemu-char.c | 160 +++++++++++++++++++++++++++---------------------------
spice-qemu-char.c | 12 ++--
2 files changed, 86 insertions(+), 86 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index c4eb4ee..8f04a9d 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -97,18 +97,18 @@ static int SocketAddress_to_str(char *dest, int max_len,
const char *prefix, SocketAddress *addr,
bool is_listen, bool is_telnet)
{
- switch (addr->kind) {
+ switch (addr->type) {
case SOCKET_ADDRESS_KIND_INET:
return snprintf(dest, max_len, "%s%s:%s:%s%s", prefix,
- is_telnet ? "telnet" : "tcp", addr->inet->host,
- addr->inet->port, is_listen ? ",server" : "");
+ is_telnet ? "telnet" : "tcp", addr->u.inet->host,
+ addr->u.inet->port, is_listen ? ",server" : "");
break;
case SOCKET_ADDRESS_KIND_UNIX:
return snprintf(dest, max_len, "%sunix:%s%s", prefix,
- addr->q_unix->path, is_listen ? ",server" : "");
+ addr->u.q_unix->path, is_listen ? ",server" : "");
break;
case SOCKET_ADDRESS_KIND_FD:
- return snprintf(dest, max_len, "%sfd:%s%s", prefix, addr->fd->str,
+ return snprintf(dest, max_len, "%sfd:%s%s", prefix, addr->u.fd->str,
is_listen ? ",server" : "");
break;
default:
@@ -661,7 +661,7 @@ static CharDriverState *qemu_chr_open_mux(const char *id,
ChardevBackend *backend,
ChardevReturn *ret, Error **errp)
{
- ChardevMux *mux = backend->mux;
+ ChardevMux *mux = backend->u.mux;
CharDriverState *chr, *drv;
MuxDriver *d;
@@ -1070,7 +1070,7 @@ static CharDriverState *qemu_chr_open_pipe(const char *id,
ChardevReturn *ret,
Error **errp)
{
- ChardevHostdev *opts = backend->pipe;
+ ChardevHostdev *opts = backend->u.pipe;
int fd_in, fd_out;
char filename_in[CHR_MAX_FILENAME_SIZE];
char filename_out[CHR_MAX_FILENAME_SIZE];
@@ -1148,7 +1148,7 @@ static CharDriverState *qemu_chr_open_stdio(const char *id,
ChardevReturn *ret,
Error **errp)
{
- ChardevStdio *opts = backend->stdio;
+ ChardevStdio *opts = backend->u.stdio;
CharDriverState *chr;
struct sigaction act;
@@ -2147,7 +2147,7 @@ static CharDriverState *qemu_chr_open_pipe(const char *id,
ChardevReturn *ret,
Error **errp)
{
- ChardevHostdev *opts = backend->pipe;
+ ChardevHostdev *opts = backend->u.pipe;
const char *filename = opts->device;
CharDriverState *chr;
WinCharState *s;
@@ -3202,7 +3202,7 @@ static CharDriverState *qemu_chr_open_ringbuf(const char *id,
ChardevReturn *ret,
Error **errp)
{
- ChardevRingbuf *opts = backend->ringbuf;
+ ChardevRingbuf *opts = backend->u.ringbuf;
CharDriverState *chr;
RingBufCharDriver *d;
@@ -3477,16 +3477,16 @@ static void qemu_chr_parse_file_out(QemuOpts *opts, ChardevBackend *backend,
error_setg(errp, "chardev: file: no filename given");
return;
}
- backend->file = g_new0(ChardevFile, 1);
- backend->file->out = g_strdup(path);
+ backend->u.file = g_new0(ChardevFile, 1);
+ backend->u.file->out = g_strdup(path);
}
static void qemu_chr_parse_stdio(QemuOpts *opts, ChardevBackend *backend,
Error **errp)
{
- backend->stdio = g_new0(ChardevStdio, 1);
- backend->stdio->has_signal = true;
- backend->stdio->signal = qemu_opt_get_bool(opts, "signal", true);
+ backend->u.stdio = g_new0(ChardevStdio, 1);
+ backend->u.stdio->has_signal = true;
+ backend->u.stdio->signal = qemu_opt_get_bool(opts, "signal", true);
}
#ifdef HAVE_CHARDEV_SERIAL
@@ -3499,8 +3499,8 @@ static void qemu_chr_parse_serial(QemuOpts *opts, ChardevBackend *backend,
error_setg(errp, "chardev: serial/tty: no device path given");
return;
}
- backend->serial = g_new0(ChardevHostdev, 1);
- backend->serial->device = g_strdup(device);
+ backend->u.serial = g_new0(ChardevHostdev, 1);
+ backend->u.serial->device = g_strdup(device);
}
#endif
@@ -3514,8 +3514,8 @@ static void qemu_chr_parse_parallel(QemuOpts *opts, ChardevBackend *backend,
error_setg(errp, "chardev: parallel: no device path given");
return;
}
- backend->parallel = g_new0(ChardevHostdev, 1);
- backend->parallel->device = g_strdup(device);
+ backend->u.parallel = g_new0(ChardevHostdev, 1);
+ backend->u.parallel->device = g_strdup(device);
}
#endif
@@ -3528,8 +3528,8 @@ static void qemu_chr_parse_pipe(QemuOpts *opts, ChardevBackend *backend,
error_setg(errp, "chardev: pipe: no device path given");
return;
}
- backend->pipe = g_new0(ChardevHostdev, 1);
- backend->pipe->device = g_strdup(device);
+ backend->u.pipe = g_new0(ChardevHostdev, 1);
+ backend->u.pipe->device = g_strdup(device);
}
static void qemu_chr_parse_ringbuf(QemuOpts *opts, ChardevBackend *backend,
@@ -3537,12 +3537,12 @@ static void qemu_chr_parse_ringbuf(QemuOpts *opts, ChardevBackend *backend,
{
int val;
- backend->ringbuf = g_new0(ChardevRingbuf, 1);
+ backend->u.ringbuf = g_new0(ChardevRingbuf, 1);
val = qemu_opt_get_size(opts, "size", 0);
if (val != 0) {
- backend->ringbuf->has_size = true;
- backend->ringbuf->size = val;
+ backend->u.ringbuf->has_size = true;
+ backend->u.ringbuf->size = val;
}
}
@@ -3555,8 +3555,8 @@ static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend *backend,
error_setg(errp, "chardev: mux: no chardev given");
return;
}
- backend->mux = g_new0(ChardevMux, 1);
- backend->mux->chardev = g_strdup(chardev);
+ backend->u.mux = g_new0(ChardevMux, 1);
+ backend->u.mux->chardev = g_strdup(chardev);
}
static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
@@ -3583,37 +3583,37 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
}
}
- backend->socket = g_new0(ChardevSocket, 1);
+ backend->u.socket = g_new0(ChardevSocket, 1);
- backend->socket->has_nodelay = true;
- backend->socket->nodelay = do_nodelay;
- backend->socket->has_server = true;
- backend->socket->server = is_listen;
- backend->socket->has_telnet = true;
- backend->socket->telnet = is_telnet;
- backend->socket->has_wait = true;
- backend->socket->wait = is_waitconnect;
- backend->socket->has_reconnect = true;
- backend->socket->reconnect = reconnect;
+ backend->u.socket->has_nodelay = true;
+ backend->u.socket->nodelay = do_nodelay;
+ backend->u.socket->has_server = true;
+ backend->u.socket->server = is_listen;
+ backend->u.socket->has_telnet = true;
+ backend->u.socket->telnet = is_telnet;
+ backend->u.socket->has_wait = true;
+ backend->u.socket->wait = is_waitconnect;
+ backend->u.socket->has_reconnect = true;
+ backend->u.socket->reconnect = reconnect;
addr = g_new0(SocketAddress, 1);
if (path) {
- addr->kind = SOCKET_ADDRESS_KIND_UNIX;
- addr->q_unix = g_new0(UnixSocketAddress, 1);
- addr->q_unix->path = g_strdup(path);
+ addr->type = SOCKET_ADDRESS_KIND_UNIX;
+ addr->u.q_unix = g_new0(UnixSocketAddress, 1);
+ addr->u.q_unix->path = g_strdup(path);
} else {
- addr->kind = SOCKET_ADDRESS_KIND_INET;
- addr->inet = g_new0(InetSocketAddress, 1);
- addr->inet->host = g_strdup(host);
- addr->inet->port = g_strdup(port);
- addr->inet->has_to = qemu_opt_get(opts, "to");
- addr->inet->to = qemu_opt_get_number(opts, "to", 0);
- addr->inet->has_ipv4 = qemu_opt_get(opts, "ipv4");
- addr->inet->ipv4 = qemu_opt_get_bool(opts, "ipv4", 0);
- addr->inet->has_ipv6 = qemu_opt_get(opts, "ipv6");
- addr->inet->ipv6 = qemu_opt_get_bool(opts, "ipv6", 0);
+ addr->type = SOCKET_ADDRESS_KIND_INET;
+ addr->u.inet = g_new0(InetSocketAddress, 1);
+ addr->u.inet->host = g_strdup(host);
+ addr->u.inet->port = g_strdup(port);
+ addr->u.inet->has_to = qemu_opt_get(opts, "to");
+ addr->u.inet->to = qemu_opt_get_number(opts, "to", 0);
+ addr->u.inet->has_ipv4 = qemu_opt_get(opts, "ipv4");
+ addr->u.inet->ipv4 = qemu_opt_get_bool(opts, "ipv4", 0);
+ addr->u.inet->has_ipv6 = qemu_opt_get(opts, "ipv6");
+ addr->u.inet->ipv6 = qemu_opt_get_bool(opts, "ipv6", 0);
}
- backend->socket->addr = addr;
+ backend->u.socket->addr = addr;
}
static void qemu_chr_parse_udp(QemuOpts *opts, ChardevBackend *backend,
@@ -3644,27 +3644,27 @@ static void qemu_chr_parse_udp(QemuOpts *opts, ChardevBackend *backend,
has_local = true;
}
- backend->udp = g_new0(ChardevUdp, 1);
+ backend->u.udp = g_new0(ChardevUdp, 1);
addr = g_new0(SocketAddress, 1);
- addr->kind = SOCKET_ADDRESS_KIND_INET;
- addr->inet = g_new0(InetSocketAddress, 1);
- addr->inet->host = g_strdup(host);
- addr->inet->port = g_strdup(port);
- addr->inet->has_ipv4 = qemu_opt_get(opts, "ipv4");
- addr->inet->ipv4 = qemu_opt_get_bool(opts, "ipv4", 0);
- addr->inet->has_ipv6 = qemu_opt_get(opts, "ipv6");
- addr->inet->ipv6 = qemu_opt_get_bool(opts, "ipv6", 0);
- backend->udp->remote = addr;
+ addr->type = SOCKET_ADDRESS_KIND_INET;
+ addr->u.inet = g_new0(InetSocketAddress, 1);
+ addr->u.inet->host = g_strdup(host);
+ addr->u.inet->port = g_strdup(port);
+ addr->u.inet->has_ipv4 = qemu_opt_get(opts, "ipv4");
+ addr->u.inet->ipv4 = qemu_opt_get_bool(opts, "ipv4", 0);
+ addr->u.inet->has_ipv6 = qemu_opt_get(opts, "ipv6");
+ addr->u.inet->ipv6 = qemu_opt_get_bool(opts, "ipv6", 0);
+ backend->u.udp->remote = addr;
if (has_local) {
- backend->udp->has_local = true;
+ backend->u.udp->has_local = true;
addr = g_new0(SocketAddress, 1);
- addr->kind = SOCKET_ADDRESS_KIND_INET;
- addr->inet = g_new0(InetSocketAddress, 1);
- addr->inet->host = g_strdup(localaddr);
- addr->inet->port = g_strdup(localport);
- backend->udp->local = addr;
+ addr->type = SOCKET_ADDRESS_KIND_INET;
+ addr->u.inet = g_new0(InetSocketAddress, 1);
+ addr->u.inet->host = g_strdup(localaddr);
+ addr->u.inet->port = g_strdup(localport);
+ backend->u.udp->local = addr;
}
}
@@ -3737,7 +3737,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
}
chr = NULL;
- backend->kind = cd->kind;
+ backend->type = cd->kind;
if (cd->parse) {
cd->parse(opts, backend, &local_err);
if (local_err) {
@@ -3754,9 +3754,9 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
qapi_free_ChardevBackend(backend);
qapi_free_ChardevReturn(ret);
backend = g_new0(ChardevBackend, 1);
- backend->mux = g_new0(ChardevMux, 1);
- backend->kind = CHARDEV_BACKEND_KIND_MUX;
- backend->mux->chardev = g_strdup(bid);
+ backend->u.mux = g_new0(ChardevMux, 1);
+ backend->type = CHARDEV_BACKEND_KIND_MUX;
+ backend->u.mux->chardev = g_strdup(bid);
ret = qmp_chardev_add(id, backend, errp);
if (!ret) {
chr = qemu_chr_find(bid);
@@ -4093,7 +4093,7 @@ static CharDriverState *qmp_chardev_open_file(const char *id,
ChardevReturn *ret,
Error **errp)
{
- ChardevFile *file = backend->file;
+ ChardevFile *file = backend->u.file;
int flags, in = -1, out;
flags = O_WRONLY | O_TRUNC | O_CREAT | O_BINARY;
@@ -4120,7 +4120,7 @@ static CharDriverState *qmp_chardev_open_serial(const char *id,
ChardevReturn *ret,
Error **errp)
{
- ChardevHostdev *serial = backend->serial;
+ ChardevHostdev *serial = backend->u.serial;
int fd;
fd = qmp_chardev_open_file_source(serial->device, O_RDWR, errp);
@@ -4138,7 +4138,7 @@ static CharDriverState *qmp_chardev_open_parallel(const char *id,
ChardevReturn *ret,
Error **errp)
{
- ChardevHostdev *parallel = backend->parallel;
+ ChardevHostdev *parallel = backend->u.parallel;
int fd;
fd = qmp_chardev_open_file_source(parallel->device, O_RDWR, errp);
@@ -4183,7 +4183,7 @@ static CharDriverState *qmp_chardev_open_socket(const char *id,
{
CharDriverState *chr;
TCPCharDriver *s;
- ChardevSocket *sock = backend->socket;
+ ChardevSocket *sock = backend->u.socket;
SocketAddress *addr = sock->addr;
bool do_nodelay = sock->has_nodelay ? sock->nodelay : false;
bool is_listen = sock->has_server ? sock->server : true;
@@ -4196,7 +4196,7 @@ static CharDriverState *qmp_chardev_open_socket(const char *id,
s->fd = -1;
s->listen_fd = -1;
- s->is_unix = addr->kind == SOCKET_ADDRESS_KIND_UNIX;
+ s->is_unix = addr->type == SOCKET_ADDRESS_KIND_UNIX;
s->is_listen = is_listen;
s->is_telnet = is_telnet;
s->do_nodelay = do_nodelay;
@@ -4250,7 +4250,7 @@ static CharDriverState *qmp_chardev_open_udp(const char *id,
ChardevReturn *ret,
Error **errp)
{
- ChardevUdp *udp = backend->udp;
+ ChardevUdp *udp = backend->u.udp;
int fd;
fd = socket_dgram(udp->remote, udp->local, errp);
@@ -4279,7 +4279,7 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
for (i = backends; i; i = i->next) {
cd = i->data;
- if (cd->kind == backend->kind) {
+ if (cd->kind == backend->type) {
chr = cd->create(id, backend, ret, &local_err);
if (local_err) {
error_propagate(errp, local_err);
@@ -4297,9 +4297,9 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
chr->label = g_strdup(id);
chr->avail_connections =
- (backend->kind == CHARDEV_BACKEND_KIND_MUX) ? MAX_MUX : 1;
+ (backend->type == CHARDEV_BACKEND_KIND_MUX) ? MAX_MUX : 1;
if (!chr->filename) {
- chr->filename = g_strdup(ChardevBackendKind_lookup[backend->kind]);
+ chr->filename = g_strdup(ChardevBackendKind_lookup[backend->type]);
}
if (!chr->explicit_be_open) {
qemu_chr_be_event(chr, CHR_EVENT_OPENED);
diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index a20fb5c..e70e0f7 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -301,7 +301,7 @@ static CharDriverState *qemu_chr_open_spice_vmc(const char *id,
ChardevReturn *ret,
Error **errp)
{
- const char *type = backend->spicevmc->type;
+ const char *type = backend->u.spicevmc->type;
const char **psubtype = spice_server_char_device_recognized_subtypes();
for (; *psubtype != NULL; ++psubtype) {
@@ -324,7 +324,7 @@ static CharDriverState *qemu_chr_open_spice_port(const char *id,
ChardevReturn *ret,
Error **errp)
{
- const char *name = backend->spiceport->fqdn;
+ const char *name = backend->u.spiceport->fqdn;
CharDriverState *chr;
SpiceCharDriver *s;
@@ -362,8 +362,8 @@ static void qemu_chr_parse_spice_vmc(QemuOpts *opts, ChardevBackend *backend,
error_setg(errp, "chardev: spice channel: no name given");
return;
}
- backend->spicevmc = g_new0(ChardevSpiceChannel, 1);
- backend->spicevmc->type = g_strdup(name);
+ backend->u.spicevmc = g_new0(ChardevSpiceChannel, 1);
+ backend->u.spicevmc->type = g_strdup(name);
}
static void qemu_chr_parse_spice_port(QemuOpts *opts, ChardevBackend *backend,
@@ -375,8 +375,8 @@ static void qemu_chr_parse_spice_port(QemuOpts *opts, ChardevBackend *backend,
error_setg(errp, "chardev: spice port: no name given");
return;
}
- backend->spiceport = g_new0(ChardevSpicePort, 1);
- backend->spiceport->fqdn = g_strdup(name);
+ backend->u.spiceport = g_new0(ChardevSpicePort, 1);
+ backend->u.spiceport->fqdn = g_strdup(name);
}
static void register_types(void)
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 19/24] input: Convert to new qapi union layout
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (17 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 18/24] char: " Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 20/24] memory: " Eric Blake
` (5 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Michael S. Tsirkin, Gerd Hoffmann, armbru,
Luiz Capitulino
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for input-related code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: no change
v10: no change
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
hmp.c | 8 ++---
hw/char/escc.c | 12 +++----
hw/input/hid.c | 32 ++++++++---------
hw/input/ps2.c | 24 ++++++-------
hw/input/virtio-input-hid.c | 27 ++++++++-------
ui/console.c | 20 +++++------
ui/input-keymap.c | 20 +++++------
ui/input-legacy.c | 21 ++++++------
ui/input.c | 84 ++++++++++++++++++++++-----------------------
9 files changed, 125 insertions(+), 123 deletions(-)
diff --git a/hmp.c b/hmp.c
index 88fd804..cc4946d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1735,15 +1735,15 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
if (*endp != '\0') {
goto err_out;
}
- keylist->value->kind = KEY_VALUE_KIND_NUMBER;
- keylist->value->number = value;
+ keylist->value->type = KEY_VALUE_KIND_NUMBER;
+ keylist->value->u.number = value;
} else {
int idx = index_from_key(keyname_buf);
if (idx == Q_KEY_CODE_MAX) {
goto err_out;
}
- keylist->value->kind = KEY_VALUE_KIND_QCODE;
- keylist->value->qcode = idx;
+ keylist->value->type = KEY_VALUE_KIND_QCODE;
+ keylist->value->u.qcode = idx;
}
if (!separator) {
diff --git a/hw/char/escc.c b/hw/char/escc.c
index 9816154..c9840e1 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -842,13 +842,13 @@ static void sunkbd_handle_event(DeviceState *dev, QemuConsole *src,
ChannelState *s = (ChannelState *)dev;
int qcode, keycode;
- assert(evt->kind == INPUT_EVENT_KIND_KEY);
- qcode = qemu_input_key_value_to_qcode(evt->key->key);
+ assert(evt->type == INPUT_EVENT_KIND_KEY);
+ qcode = qemu_input_key_value_to_qcode(evt->u.key->key);
trace_escc_sunkbd_event_in(qcode, QKeyCode_lookup[qcode],
- evt->key->down);
+ evt->u.key->down);
if (qcode == Q_KEY_CODE_CAPS_LOCK) {
- if (evt->key->down) {
+ if (evt->u.key->down) {
s->caps_lock_mode ^= 1;
if (s->caps_lock_mode == 2) {
return; /* Drop second press */
@@ -862,7 +862,7 @@ static void sunkbd_handle_event(DeviceState *dev, QemuConsole *src,
}
if (qcode == Q_KEY_CODE_NUM_LOCK) {
- if (evt->key->down) {
+ if (evt->u.key->down) {
s->num_lock_mode ^= 1;
if (s->num_lock_mode == 2) {
return; /* Drop second press */
@@ -876,7 +876,7 @@ static void sunkbd_handle_event(DeviceState *dev, QemuConsole *src,
}
keycode = qcode_to_keycode[qcode];
- if (!evt->key->down) {
+ if (!evt->u.key->down) {
keycode |= 0x80;
}
trace_escc_sunkbd_event_out(keycode);
diff --git a/hw/input/hid.c b/hw/input/hid.c
index 21ebd9e..e39269f 100644
--- a/hw/input/hid.c
+++ b/hw/input/hid.c
@@ -119,33 +119,33 @@ static void hid_pointer_event(DeviceState *dev, QemuConsole *src,
assert(hs->n < QUEUE_LENGTH);
e = &hs->ptr.queue[(hs->head + hs->n) & QUEUE_MASK];
- switch (evt->kind) {
+ switch (evt->type) {
case INPUT_EVENT_KIND_REL:
- if (evt->rel->axis == INPUT_AXIS_X) {
- e->xdx += evt->rel->value;
- } else if (evt->rel->axis == INPUT_AXIS_Y) {
- e->ydy += evt->rel->value;
+ if (evt->u.rel->axis == INPUT_AXIS_X) {
+ e->xdx += evt->u.rel->value;
+ } else if (evt->u.rel->axis == INPUT_AXIS_Y) {
+ e->ydy += evt->u.rel->value;
}
break;
case INPUT_EVENT_KIND_ABS:
- if (evt->rel->axis == INPUT_AXIS_X) {
- e->xdx = evt->rel->value;
- } else if (evt->rel->axis == INPUT_AXIS_Y) {
- e->ydy = evt->rel->value;
+ if (evt->u.rel->axis == INPUT_AXIS_X) {
+ e->xdx = evt->u.rel->value;
+ } else if (evt->u.rel->axis == INPUT_AXIS_Y) {
+ e->ydy = evt->u.rel->value;
}
break;
case INPUT_EVENT_KIND_BTN:
- if (evt->btn->down) {
- e->buttons_state |= bmap[evt->btn->button];
- if (evt->btn->button == INPUT_BUTTON_WHEEL_UP) {
+ if (evt->u.btn->down) {
+ e->buttons_state |= bmap[evt->u.btn->button];
+ if (evt->u.btn->button == INPUT_BUTTON_WHEEL_UP) {
e->dz--;
- } else if (evt->btn->button == INPUT_BUTTON_WHEEL_DOWN) {
+ } else if (evt->u.btn->button == INPUT_BUTTON_WHEEL_DOWN) {
e->dz++;
}
} else {
- e->buttons_state &= ~bmap[evt->btn->button];
+ e->buttons_state &= ~bmap[evt->u.btn->button];
}
break;
@@ -223,8 +223,8 @@ static void hid_keyboard_event(DeviceState *dev, QemuConsole *src,
int scancodes[3], i, count;
int slot;
- count = qemu_input_key_value_to_scancode(evt->key->key,
- evt->key->down,
+ count = qemu_input_key_value_to_scancode(evt->u.key->key,
+ evt->u.key->down,
scancodes);
if (hs->n + count > QUEUE_LENGTH) {
fprintf(stderr, "usb-kbd: warning: key event queue full\n");
diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index fdbe565..3d6d496 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -183,8 +183,8 @@ static void ps2_keyboard_event(DeviceState *dev, QemuConsole *src,
int scancodes[3], i, count;
qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
- count = qemu_input_key_value_to_scancode(evt->key->key,
- evt->key->down,
+ count = qemu_input_key_value_to_scancode(evt->u.key->key,
+ evt->u.key->down,
scancodes);
for (i = 0; i < count; i++) {
ps2_put_keycode(s, scancodes[i]);
@@ -393,25 +393,25 @@ static void ps2_mouse_event(DeviceState *dev, QemuConsole *src,
if (!(s->mouse_status & MOUSE_STATUS_ENABLED))
return;
- switch (evt->kind) {
+ switch (evt->type) {
case INPUT_EVENT_KIND_REL:
- if (evt->rel->axis == INPUT_AXIS_X) {
- s->mouse_dx += evt->rel->value;
- } else if (evt->rel->axis == INPUT_AXIS_Y) {
- s->mouse_dy -= evt->rel->value;
+ if (evt->u.rel->axis == INPUT_AXIS_X) {
+ s->mouse_dx += evt->u.rel->value;
+ } else if (evt->u.rel->axis == INPUT_AXIS_Y) {
+ s->mouse_dy -= evt->u.rel->value;
}
break;
case INPUT_EVENT_KIND_BTN:
- if (evt->btn->down) {
- s->mouse_buttons |= bmap[evt->btn->button];
- if (evt->btn->button == INPUT_BUTTON_WHEEL_UP) {
+ if (evt->u.btn->down) {
+ s->mouse_buttons |= bmap[evt->u.btn->button];
+ if (evt->u.btn->button == INPUT_BUTTON_WHEEL_UP) {
s->mouse_dz--;
- } else if (evt->btn->button == INPUT_BUTTON_WHEEL_DOWN) {
+ } else if (evt->u.btn->button == INPUT_BUTTON_WHEEL_DOWN) {
s->mouse_dz++;
}
} else {
- s->mouse_buttons &= ~bmap[evt->btn->button];
+ s->mouse_buttons &= ~bmap[evt->u.btn->button];
}
break;
diff --git a/hw/input/virtio-input-hid.c b/hw/input/virtio-input-hid.c
index 4d85dad..bdd479c 100644
--- a/hw/input/virtio-input-hid.c
+++ b/hw/input/virtio-input-hid.c
@@ -191,44 +191,45 @@ static void virtio_input_handle_event(DeviceState *dev, QemuConsole *src,
virtio_input_event event;
int qcode;
- switch (evt->kind) {
+ switch (evt->type) {
case INPUT_EVENT_KIND_KEY:
- qcode = qemu_input_key_value_to_qcode(evt->key->key);
+ qcode = qemu_input_key_value_to_qcode(evt->u.key->key);
if (qcode && keymap_qcode[qcode]) {
event.type = cpu_to_le16(EV_KEY);
event.code = cpu_to_le16(keymap_qcode[qcode]);
- event.value = cpu_to_le32(evt->key->down ? 1 : 0);
+ event.value = cpu_to_le32(evt->u.key->down ? 1 : 0);
virtio_input_send(vinput, &event);
} else {
- if (evt->key->down) {
+ if (evt->u.key->down) {
fprintf(stderr, "%s: unmapped key: %d [%s]\n", __func__,
qcode, QKeyCode_lookup[qcode]);
}
}
break;
case INPUT_EVENT_KIND_BTN:
- if (keymap_button[evt->btn->button]) {
+ if (keymap_button[evt->u.btn->button]) {
event.type = cpu_to_le16(EV_KEY);
- event.code = cpu_to_le16(keymap_button[evt->btn->button]);
- event.value = cpu_to_le32(evt->btn->down ? 1 : 0);
+ event.code = cpu_to_le16(keymap_button[evt->u.btn->button]);
+ event.value = cpu_to_le32(evt->u.btn->down ? 1 : 0);
virtio_input_send(vinput, &event);
} else {
- if (evt->btn->down) {
+ if (evt->u.btn->down) {
fprintf(stderr, "%s: unmapped button: %d [%s]\n", __func__,
- evt->btn->button, InputButton_lookup[evt->btn->button]);
+ evt->u.btn->button,
+ InputButton_lookup[evt->u.btn->button]);
}
}
break;
case INPUT_EVENT_KIND_REL:
event.type = cpu_to_le16(EV_REL);
- event.code = cpu_to_le16(axismap_rel[evt->rel->axis]);
- event.value = cpu_to_le32(evt->rel->value);
+ event.code = cpu_to_le16(axismap_rel[evt->u.rel->axis]);
+ event.value = cpu_to_le32(evt->u.rel->value);
virtio_input_send(vinput, &event);
break;
case INPUT_EVENT_KIND_ABS:
event.type = cpu_to_le16(EV_ABS);
- event.code = cpu_to_le16(axismap_abs[evt->abs->axis]);
- event.value = cpu_to_le32(evt->abs->value);
+ event.code = cpu_to_le16(axismap_abs[evt->u.abs->axis]);
+ event.value = cpu_to_le32(evt->u.abs->value);
virtio_input_send(vinput, &event);
break;
default:
diff --git a/ui/console.c b/ui/console.c
index cf649b2..f26544e 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2016,7 +2016,7 @@ static VcHandler *vc_handler = text_console_init;
static CharDriverState *vc_init(const char *id, ChardevBackend *backend,
ChardevReturn *ret, Error **errp)
{
- return vc_handler(backend->vc, errp);
+ return vc_handler(backend->u.vc, errp);
}
void register_vc_handler(VcHandler *handler)
@@ -2057,30 +2057,30 @@ static void qemu_chr_parse_vc(QemuOpts *opts, ChardevBackend *backend,
{
int val;
- backend->vc = g_new0(ChardevVC, 1);
+ backend->u.vc = g_new0(ChardevVC, 1);
val = qemu_opt_get_number(opts, "width", 0);
if (val != 0) {
- backend->vc->has_width = true;
- backend->vc->width = val;
+ backend->u.vc->has_width = true;
+ backend->u.vc->width = val;
}
val = qemu_opt_get_number(opts, "height", 0);
if (val != 0) {
- backend->vc->has_height = true;
- backend->vc->height = val;
+ backend->u.vc->has_height = true;
+ backend->u.vc->height = val;
}
val = qemu_opt_get_number(opts, "cols", 0);
if (val != 0) {
- backend->vc->has_cols = true;
- backend->vc->cols = val;
+ backend->u.vc->has_cols = true;
+ backend->u.vc->cols = val;
}
val = qemu_opt_get_number(opts, "rows", 0);
if (val != 0) {
- backend->vc->has_rows = true;
- backend->vc->rows = val;
+ backend->u.vc->has_rows = true;
+ backend->u.vc->rows = val;
}
}
diff --git a/ui/input-keymap.c b/ui/input-keymap.c
index 7635cb0..d36be4b 100644
--- a/ui/input-keymap.c
+++ b/ui/input-keymap.c
@@ -139,11 +139,11 @@ static int number_to_qcode[0x100];
int qemu_input_key_value_to_number(const KeyValue *value)
{
- if (value->kind == KEY_VALUE_KIND_QCODE) {
- return qcode_to_number[value->qcode];
+ if (value->type == KEY_VALUE_KIND_QCODE) {
+ return qcode_to_number[value->u.qcode];
} else {
- assert(value->kind == KEY_VALUE_KIND_NUMBER);
- return value->number;
+ assert(value->type == KEY_VALUE_KIND_NUMBER);
+ return value->u.number;
}
}
@@ -166,11 +166,11 @@ int qemu_input_key_number_to_qcode(uint8_t nr)
int qemu_input_key_value_to_qcode(const KeyValue *value)
{
- if (value->kind == KEY_VALUE_KIND_QCODE) {
- return value->qcode;
+ if (value->type == KEY_VALUE_KIND_QCODE) {
+ return value->u.qcode;
} else {
- assert(value->kind == KEY_VALUE_KIND_NUMBER);
- return qemu_input_key_number_to_qcode(value->number);
+ assert(value->type == KEY_VALUE_KIND_NUMBER);
+ return qemu_input_key_number_to_qcode(value->u.number);
}
}
@@ -180,8 +180,8 @@ int qemu_input_key_value_to_scancode(const KeyValue *value, bool down,
int keycode = qemu_input_key_value_to_number(value);
int count = 0;
- if (value->kind == KEY_VALUE_KIND_QCODE &&
- value->qcode == Q_KEY_CODE_PAUSE) {
+ if (value->type == KEY_VALUE_KIND_QCODE &&
+ value->u.qcode == Q_KEY_CODE_PAUSE) {
/* specific case */
int v = down ? 0 : 0x80;
codes[count++] = 0xe1;
diff --git a/ui/input-legacy.c b/ui/input-legacy.c
index e50f296..a67ed32 100644
--- a/ui/input-legacy.c
+++ b/ui/input-legacy.c
@@ -113,8 +113,8 @@ static void legacy_kbd_event(DeviceState *dev, QemuConsole *src,
if (!entry || !entry->put_kbd) {
return;
}
- count = qemu_input_key_value_to_scancode(evt->key->key,
- evt->key->down,
+ count = qemu_input_key_value_to_scancode(evt->u.key->key,
+ evt->u.key->down,
scancodes);
for (i = 0; i < count; i++) {
entry->put_kbd(entry->opaque, scancodes[i]);
@@ -150,21 +150,22 @@ static void legacy_mouse_event(DeviceState *dev, QemuConsole *src,
};
QEMUPutMouseEntry *s = (QEMUPutMouseEntry *)dev;
- switch (evt->kind) {
+ switch (evt->type) {
case INPUT_EVENT_KIND_BTN:
- if (evt->btn->down) {
- s->buttons |= bmap[evt->btn->button];
+ if (evt->u.btn->down) {
+ s->buttons |= bmap[evt->u.btn->button];
} else {
- s->buttons &= ~bmap[evt->btn->button];
+ s->buttons &= ~bmap[evt->u.btn->button];
}
- if (evt->btn->down && evt->btn->button == INPUT_BUTTON_WHEEL_UP) {
+ if (evt->u.btn->down && evt->u.btn->button == INPUT_BUTTON_WHEEL_UP) {
s->qemu_put_mouse_event(s->qemu_put_mouse_event_opaque,
s->axis[INPUT_AXIS_X],
s->axis[INPUT_AXIS_Y],
-1,
s->buttons);
}
- if (evt->btn->down && evt->btn->button == INPUT_BUTTON_WHEEL_DOWN) {
+ if (evt->u.btn->down &&
+ evt->u.btn->button == INPUT_BUTTON_WHEEL_DOWN) {
s->qemu_put_mouse_event(s->qemu_put_mouse_event_opaque,
s->axis[INPUT_AXIS_X],
s->axis[INPUT_AXIS_Y],
@@ -173,10 +174,10 @@ static void legacy_mouse_event(DeviceState *dev, QemuConsole *src,
}
break;
case INPUT_EVENT_KIND_ABS:
- s->axis[evt->abs->axis] = evt->abs->value;
+ s->axis[evt->u.abs->axis] = evt->u.abs->value;
break;
case INPUT_EVENT_KIND_REL:
- s->axis[evt->rel->axis] += evt->rel->value;
+ s->axis[evt->u.rel->axis] += evt->u.rel->value;
break;
default:
break;
diff --git a/ui/input.c b/ui/input.c
index 1a552d1..643f885 100644
--- a/ui/input.c
+++ b/ui/input.c
@@ -147,10 +147,10 @@ void qmp_x_input_send_event(bool has_console, int64_t console,
for (e = events; e != NULL; e = e->next) {
InputEvent *event = e->value;
- if (!qemu_input_find_handler(1 << event->kind, con)) {
+ if (!qemu_input_find_handler(1 << event->type, con)) {
error_setg(errp, "Input handler not found for "
"event type %s",
- InputEventKind_lookup[event->kind]);
+ InputEventKind_lookup[event->type]);
return;
}
}
@@ -168,22 +168,22 @@ static void qemu_input_transform_abs_rotate(InputEvent *evt)
{
switch (graphic_rotate) {
case 90:
- if (evt->abs->axis == INPUT_AXIS_X) {
- evt->abs->axis = INPUT_AXIS_Y;
- } else if (evt->abs->axis == INPUT_AXIS_Y) {
- evt->abs->axis = INPUT_AXIS_X;
- evt->abs->value = INPUT_EVENT_ABS_SIZE - 1 - evt->abs->value;
+ if (evt->u.abs->axis == INPUT_AXIS_X) {
+ evt->u.abs->axis = INPUT_AXIS_Y;
+ } else if (evt->u.abs->axis == INPUT_AXIS_Y) {
+ evt->u.abs->axis = INPUT_AXIS_X;
+ evt->u.abs->value = INPUT_EVENT_ABS_SIZE - 1 - evt->u.abs->value;
}
break;
case 180:
- evt->abs->value = INPUT_EVENT_ABS_SIZE - 1 - evt->abs->value;
+ evt->u.abs->value = INPUT_EVENT_ABS_SIZE - 1 - evt->u.abs->value;
break;
case 270:
- if (evt->abs->axis == INPUT_AXIS_X) {
- evt->abs->axis = INPUT_AXIS_Y;
- evt->abs->value = INPUT_EVENT_ABS_SIZE - 1 - evt->abs->value;
- } else if (evt->abs->axis == INPUT_AXIS_Y) {
- evt->abs->axis = INPUT_AXIS_X;
+ if (evt->u.abs->axis == INPUT_AXIS_X) {
+ evt->u.abs->axis = INPUT_AXIS_Y;
+ evt->u.abs->value = INPUT_EVENT_ABS_SIZE - 1 - evt->u.abs->value;
+ } else if (evt->u.abs->axis == INPUT_AXIS_Y) {
+ evt->u.abs->axis = INPUT_AXIS_X;
}
break;
}
@@ -197,18 +197,18 @@ static void qemu_input_event_trace(QemuConsole *src, InputEvent *evt)
if (src) {
idx = qemu_console_get_index(src);
}
- switch (evt->kind) {
+ switch (evt->type) {
case INPUT_EVENT_KIND_KEY:
- switch (evt->key->key->kind) {
+ switch (evt->u.key->key->type) {
case KEY_VALUE_KIND_NUMBER:
- qcode = qemu_input_key_number_to_qcode(evt->key->key->number);
+ qcode = qemu_input_key_number_to_qcode(evt->u.key->key->u.number);
name = QKeyCode_lookup[qcode];
- trace_input_event_key_number(idx, evt->key->key->number,
- name, evt->key->down);
+ trace_input_event_key_number(idx, evt->u.key->key->u.number,
+ name, evt->u.key->down);
break;
case KEY_VALUE_KIND_QCODE:
- name = QKeyCode_lookup[evt->key->key->qcode];
- trace_input_event_key_qcode(idx, name, evt->key->down);
+ name = QKeyCode_lookup[evt->u.key->key->u.qcode];
+ trace_input_event_key_qcode(idx, name, evt->u.key->down);
break;
case KEY_VALUE_KIND_MAX:
/* keep gcc happy */
@@ -216,16 +216,16 @@ static void qemu_input_event_trace(QemuConsole *src, InputEvent *evt)
}
break;
case INPUT_EVENT_KIND_BTN:
- name = InputButton_lookup[evt->btn->button];
- trace_input_event_btn(idx, name, evt->btn->down);
+ name = InputButton_lookup[evt->u.btn->button];
+ trace_input_event_btn(idx, name, evt->u.btn->down);
break;
case INPUT_EVENT_KIND_REL:
- name = InputAxis_lookup[evt->rel->axis];
- trace_input_event_rel(idx, name, evt->rel->value);
+ name = InputAxis_lookup[evt->u.rel->axis];
+ trace_input_event_rel(idx, name, evt->u.rel->value);
break;
case INPUT_EVENT_KIND_ABS:
- name = InputAxis_lookup[evt->abs->axis];
- trace_input_event_abs(idx, name, evt->abs->value);
+ name = InputAxis_lookup[evt->u.abs->axis];
+ trace_input_event_abs(idx, name, evt->u.abs->value);
break;
case INPUT_EVENT_KIND_MAX:
/* keep gcc happy */
@@ -311,12 +311,12 @@ void qemu_input_event_send(QemuConsole *src, InputEvent *evt)
qemu_input_event_trace(src, evt);
/* pre processing */
- if (graphic_rotate && (evt->kind == INPUT_EVENT_KIND_ABS)) {
+ if (graphic_rotate && (evt->type == INPUT_EVENT_KIND_ABS)) {
qemu_input_transform_abs_rotate(evt);
}
/* send event */
- s = qemu_input_find_handler(1 << evt->kind, src);
+ s = qemu_input_find_handler(1 << evt->type, src);
if (!s) {
return;
}
@@ -348,10 +348,10 @@ void qemu_input_event_sync(void)
InputEvent *qemu_input_event_new_key(KeyValue *key, bool down)
{
InputEvent *evt = g_new0(InputEvent, 1);
- evt->key = g_new0(InputKeyEvent, 1);
- evt->kind = INPUT_EVENT_KIND_KEY;
- evt->key->key = key;
- evt->key->down = down;
+ evt->u.key = g_new0(InputKeyEvent, 1);
+ evt->type = INPUT_EVENT_KIND_KEY;
+ evt->u.key->key = key;
+ evt->u.key->down = down;
return evt;
}
@@ -372,16 +372,16 @@ void qemu_input_event_send_key(QemuConsole *src, KeyValue *key, bool down)
void qemu_input_event_send_key_number(QemuConsole *src, int num, bool down)
{
KeyValue *key = g_new0(KeyValue, 1);
- key->kind = KEY_VALUE_KIND_NUMBER;
- key->number = num;
+ key->type = KEY_VALUE_KIND_NUMBER;
+ key->u.number = num;
qemu_input_event_send_key(src, key, down);
}
void qemu_input_event_send_key_qcode(QemuConsole *src, QKeyCode q, bool down)
{
KeyValue *key = g_new0(KeyValue, 1);
- key->kind = KEY_VALUE_KIND_QCODE;
- key->qcode = q;
+ key->type = KEY_VALUE_KIND_QCODE;
+ key->u.qcode = q;
qemu_input_event_send_key(src, key, down);
}
@@ -398,10 +398,10 @@ void qemu_input_event_send_key_delay(uint32_t delay_ms)
InputEvent *qemu_input_event_new_btn(InputButton btn, bool down)
{
InputEvent *evt = g_new0(InputEvent, 1);
- evt->btn = g_new0(InputBtnEvent, 1);
- evt->kind = INPUT_EVENT_KIND_BTN;
- evt->btn->button = btn;
- evt->btn->down = down;
+ evt->u.btn = g_new0(InputBtnEvent, 1);
+ evt->type = INPUT_EVENT_KIND_BTN;
+ evt->u.btn->button = btn;
+ evt->u.btn->down = down;
return evt;
}
@@ -451,8 +451,8 @@ InputEvent *qemu_input_event_new_move(InputEventKind kind,
InputEvent *evt = g_new0(InputEvent, 1);
InputMoveEvent *move = g_new0(InputMoveEvent, 1);
- evt->kind = kind;
- evt->data = move;
+ evt->type = kind;
+ evt->u.data = move;
move->axis = axis;
move->value = value;
return evt;
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 20/24] memory: Convert to new qapi union layout
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (18 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 19/24] input: " Eric Blake
@ 2015-10-26 22:34 ` Eric Blake
2015-10-26 22:35 ` [Qemu-devel] [PATCH v11 21/24] tpm: " Eric Blake
` (4 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:34 UTC (permalink / raw)
To: qemu-devel
Cc: Igor Mammedov, Michael S. Tsirkin, armbru, Eduardo Habkost,
Luiz Capitulino
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for memory-related code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: no change
v10: no change
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
hmp.c | 6 +++---
hw/mem/pc-dimm.c | 6 +++---
numa.c | 8 ++++----
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/hmp.c b/hmp.c
index cc4946d..39d5815 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1958,12 +1958,12 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
value = info->value;
if (value) {
- switch (value->kind) {
+ switch (value->type) {
case MEMORY_DEVICE_INFO_KIND_DIMM:
- di = value->dimm;
+ di = value->u.dimm;
monitor_printf(mon, "Memory device [%s]: \"%s\"\n",
- MemoryDeviceInfoKind_lookup[value->kind],
+ MemoryDeviceInfoKind_lookup[value->type],
di->id ? di->id : "");
monitor_printf(mon, " addr: 0x%" PRIx64 "\n", di->addr);
monitor_printf(mon, " slot: %" PRId64 "\n", di->slot);
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 2bae994..fd84a90 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -180,7 +180,7 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
NULL);
di->memdev = object_get_canonical_path(OBJECT(dimm->hostmem));
- info->dimm = di;
+ info->u.dimm = di;
elem->value = info;
elem->next = NULL;
**prev = elem;
@@ -204,9 +204,9 @@ ram_addr_t get_current_ram_size(void)
MemoryDeviceInfo *value = info->value;
if (value) {
- switch (value->kind) {
+ switch (value->type) {
case MEMORY_DEVICE_INFO_KIND_DIMM:
- size += value->dimm->size;
+ size += value->u.dimm->size;
break;
default:
break;
diff --git a/numa.c b/numa.c
index e9b18f5..fdfe294 100644
--- a/numa.c
+++ b/numa.c
@@ -226,9 +226,9 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
goto error;
}
- switch (object->kind) {
+ switch (object->type) {
case NUMA_OPTIONS_KIND_NODE:
- numa_node_parse(object->node, opts, &err);
+ numa_node_parse(object->u.node, opts, &err);
if (err) {
goto error;
}
@@ -487,9 +487,9 @@ static void numa_stat_memory_devices(uint64_t node_mem[])
MemoryDeviceInfo *value = info->value;
if (value) {
- switch (value->kind) {
+ switch (value->type) {
case MEMORY_DEVICE_INFO_KIND_DIMM:
- node_mem[value->dimm->node] += value->dimm->size;
+ node_mem[value->u.dimm->node] += value->u.dimm->size;
break;
default:
break;
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 21/24] tpm: Convert to new qapi union layout
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (19 preceding siblings ...)
2015-10-26 22:34 ` [Qemu-devel] [PATCH v11 20/24] memory: " Eric Blake
@ 2015-10-26 22:35 ` Eric Blake
2015-10-26 22:35 ` [Qemu-devel] [PATCH v11 22/24] qapi: Finish converting " Eric Blake
` (3 subsequent siblings)
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:35 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Luiz Capitulino
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
Make the conversion to the new layout for TPM-related code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: no change
v10: no change
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
hmp.c | 6 +++---
tpm.c | 4 ++--
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/hmp.c b/hmp.c
index 39d5815..a15d00c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -841,11 +841,11 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
c, TpmModel_lookup[ti->model]);
monitor_printf(mon, " \\ %s: type=%s",
- ti->id, TpmTypeOptionsKind_lookup[ti->options->kind]);
+ ti->id, TpmTypeOptionsKind_lookup[ti->options->type]);
- switch (ti->options->kind) {
+ switch (ti->options->type) {
case TPM_TYPE_OPTIONS_KIND_PASSTHROUGH:
- tpo = ti->options->passthrough;
+ tpo = ti->options->u.passthrough;
monitor_printf(mon, "%s%s%s%s",
tpo->has_path ? ",path=" : "",
tpo->has_path ? tpo->path : "",
diff --git a/tpm.c b/tpm.c
index 4e9b109..f2c59d1 100644
--- a/tpm.c
+++ b/tpm.c
@@ -260,9 +260,9 @@ static TPMInfo *qmp_query_tpm_inst(TPMBackend *drv)
switch (drv->ops->type) {
case TPM_TYPE_PASSTHROUGH:
- res->options->kind = TPM_TYPE_OPTIONS_KIND_PASSTHROUGH;
+ res->options->type = TPM_TYPE_OPTIONS_KIND_PASSTHROUGH;
tpo = g_new0(TPMPassthroughOptions, 1);
- res->options->passthrough = tpo;
+ res->options->u.passthrough = tpo;
if (drv->path) {
tpo->path = g_strdup(drv->path);
tpo->has_path = true;
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* [Qemu-devel] [PATCH v11 22/24] qapi: Finish converting to new qapi union layout
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (20 preceding siblings ...)
2015-10-26 22:35 ` [Qemu-devel] [PATCH v11 21/24] tpm: " Eric Blake
@ 2015-10-26 22:35 ` Eric Blake
2015-10-27 14:33 ` Eric Blake
2015-10-26 22:35 ` [Qemu-devel] [PATCH v11 23/24] qapi: Reserve 'u' member name Eric Blake
` (2 subsequent siblings)
24 siblings, 1 reply; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:35 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
We have two issues with our qapi union layout:
1) Even though the QMP wire format spells the tag 'type', the
C code spells it 'kind', requiring some hacks in the generator.
2) The C struct uses an anonymous union, which places all tag
values in the same namespace as all non-variant members. This
leads to spurious collisions if a tag value matches a QMP name.
This patch is the back end for a series that converts to a
saner qapi union layout. Now that all clients have been
converted to use 'type' and 'obj->u.value', we can drop the
temporary parallel support for 'kind' and 'obj->value'.
Given a simple union qapi type:
{ 'union':'Foo', 'data': { 'a':'int', 'b':'bool' } }
this is the overall effect, when compared to the state before
this series of patches:
| struct Foo {
|- FooKind kind;
|- union { /* union tag is @kind */
|+ FooKind type;
|+ union { /* union tag is @type */
| void *data;
| int64_t a;
| bool b;
|- };
|+ } u;
| };
There are still some further changes that can be made to the
testsuite now that there is no longer a collision between
enum tag values and QMP names, as well as adding a reservation
of 'u' to avoid a collision between QMP and our choice of union
naming, but those will be later patches.
Note, however, that we do not rename the generated enum, which
is still 'FooKind'. A further patch could generate implicit
enums as 'FooType', but while the generator already reserved
the '*Kind' namespace (commit 4dc2e69), there are already QMP
constructs with '*Type' naming, which means changing our
reservation namespace would have lots of churn to C code to
deal with a forced name change.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: enhance commit message
v10: rebase to earlier changes, match commit wording
v9: new patch, but incorporates parts of v5 31/46 and Markus' RFC:
http://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02236.html
---
scripts/qapi-types.py | 26 +++++---------------------
1 file changed, 5 insertions(+), 21 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 1420e00..7e35bb6 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -149,23 +149,10 @@ struct %(c_name)s {
if base:
ret += gen_struct_fields([], base)
else:
- # TODO As a hack, we emit both 'kind' and 'type'. Ultimately, we
- # want to use only 'type', but the conversion is large enough to
- # require staging over several commits.
- ret += mcgen('''
- union {
- %(c_type)s kind;
- %(c_type)s type;
- };
-''',
- c_type=c_name(variants.tag_member.type.name))
+ ret += gen_struct_field(variants.tag_member.name,
+ variants.tag_member.type,
+ False)
- # TODO As a hack, we emit the union twice, once as an anonymous union
- # and once as a named union. Ultimately, we want to use only the
- # named union version (as it avoids conflicts between tag values as
- # branch names competing with non-variant QMP names), but the conversion
- # is large enough to require staging over several commits.
- tmp = ''
# 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
# whether to bypass the switch statement if visiting the discriminator
@@ -174,7 +161,7 @@ struct %(c_name)s {
# should not be any data leaks even without a data pointer. Or, if
# 'data' is merely added to guarantee we don't have an empty union,
# shouldn't we enforce that at .json parse time?
- tmp += mcgen('''
+ ret += mcgen('''
union { /* union tag is @%(c_name)s */
void *data;
''',
@@ -183,17 +170,14 @@ struct %(c_name)s {
for var in variants.variants:
# Ugly special case for simple union TODO get rid of it
typ = var.simple_union_type() or var.type
- tmp += mcgen('''
+ ret += mcgen('''
%(c_type)s %(c_name)s;
''',
c_type=typ.c_type(),
c_name=c_name(var.name))
- ret += tmp
- ret += ' ' + '\n '.join(tmp.split('\n'))
ret += mcgen('''
} u;
- };
};
''')
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v11 22/24] qapi: Finish converting to new qapi union layout
2015-10-26 22:35 ` [Qemu-devel] [PATCH v11 22/24] qapi: Finish converting " Eric Blake
@ 2015-10-27 14:33 ` Eric Blake
2015-10-27 16:01 ` Markus Armbruster
0 siblings, 1 reply; 38+ messages in thread
From: Eric Blake @ 2015-10-27 14:33 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 3203 bytes --]
On 10/26/2015 04:35 PM, Eric Blake wrote:
> We have two issues with our qapi union layout:
> 1) Even though the QMP wire format spells the tag 'type', the
> C code spells it 'kind', requiring some hacks in the generator.
> 2) The C struct uses an anonymous union, which places all tag
> values in the same namespace as all non-variant members. This
> leads to spurious collisions if a tag value matches a QMP name.
You asked for an updated commit message (but that request was made
against v10:
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06216.html).
There, you suggested "a non-variant member name" rather than "QMP name"
- it works for me, but you'd want to make that change for all of
13-22/25 (since I copy-pasted the same intro to each). Or decide which
ones you want to squash together.
For your other comment in that mail (mentioning an example test), I
think I already got close to what you asked for, but have one tweak below:
>
> This patch is the back end for a series that converts to a
> saner qapi union layout. Now that all clients have been
> converted to use 'type' and 'obj->u.value', we can drop the
> temporary parallel support for 'kind' and 'obj->value'.
>
> Given a simple union qapi type:
>
> { 'union':'Foo', 'data': { 'a':'int', 'b':'bool' } }
>
> this is the overall effect, when compared to the state before
> this series of patches:
>
> | struct Foo {
> |- FooKind kind;
> |- union { /* union tag is @kind */
> |+ FooKind type;
> |+ union { /* union tag is @type */
> | void *data;
> | int64_t a;
> | bool b;
> |- };
> |+ } u;
> | };
>
> There are still some further changes that can be made to the
> testsuite now that there is no longer a collision between
> enum tag values and QMP names, as well as adding a reservation
> of 'u' to avoid a collision between QMP and our choice of union
> naming, but those will be later patches.
Change this paragraph to something along these lines:
The testsuite still contains some examples of artificial restrictions
(see flat-union-clash-type.json, for example) that are no longer
technically necessary, now that there is no longer a collision between
enum tag values and non-variant member names; but fixing this will be
done in later patches, in part because some further changes are required
to keep QAPISchema*.check() from asserting. Also, a later patch will
add a reservation for the member name 'u' to avoid a collision between a
user's non-variant names and our internal choice of C union name.
>
> Note, however, that we do not rename the generated enum, which
> is still 'FooKind'. A further patch could generate implicit
> enums as 'FooType', but while the generator already reserved
> the '*Kind' namespace (commit 4dc2e69), there are already QMP
> constructs with '*Type' naming, which means changing our
> reservation namespace would have lots of churn to C code to
> deal with a forced name change.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
--
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] 38+ messages in thread* Re: [Qemu-devel] [PATCH v11 22/24] qapi: Finish converting to new qapi union layout
2015-10-27 14:33 ` Eric Blake
@ 2015-10-27 16:01 ` Markus Armbruster
2015-10-27 16:47 ` Eric Blake
0 siblings, 1 reply; 38+ messages in thread
From: Markus Armbruster @ 2015-10-27 16:01 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 10/26/2015 04:35 PM, Eric Blake wrote:
>> We have two issues with our qapi union layout:
>> 1) Even though the QMP wire format spells the tag 'type', the
>> C code spells it 'kind', requiring some hacks in the generator.
>> 2) The C struct uses an anonymous union, which places all tag
>> values in the same namespace as all non-variant members. This
>> leads to spurious collisions if a tag value matches a QMP name.
>
> You asked for an updated commit message (but that request was made
> against v10:
> https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06216.html).
>
> There, you suggested "a non-variant member name" rather than "QMP name"
> - it works for me, but you'd want to make that change for all of
> 13-22/25 (since I copy-pasted the same intro to each). Or decide which
> ones you want to squash together.
I went through all commit messages and tweaked around occurences of QMP.
I pushed the result to qapi-next; please have a look.
> For your other comment in that mail (mentioning an example test), I
> think I already got close to what you asked for, but have one tweak below:
>
>>
>> This patch is the back end for a series that converts to a
>> saner qapi union layout. Now that all clients have been
>> converted to use 'type' and 'obj->u.value', we can drop the
>> temporary parallel support for 'kind' and 'obj->value'.
>>
>> Given a simple union qapi type:
>>
>> { 'union':'Foo', 'data': { 'a':'int', 'b':'bool' } }
>>
>> this is the overall effect, when compared to the state before
>> this series of patches:
>>
>> | struct Foo {
>> |- FooKind kind;
>> |- union { /* union tag is @kind */
>> |+ FooKind type;
>> |+ union { /* union tag is @type */
>> | void *data;
>> | int64_t a;
>> | bool b;
>> |- };
>> |+ } u;
>> | };
>>
>> There are still some further changes that can be made to the
>> testsuite now that there is no longer a collision between
>> enum tag values and QMP names, as well as adding a reservation
>> of 'u' to avoid a collision between QMP and our choice of union
>> naming, but those will be later patches.
>
> Change this paragraph to something along these lines:
>
> The testsuite still contains some examples of artificial restrictions
> (see flat-union-clash-type.json, for example) that are no longer
> technically necessary, now that there is no longer a collision between
> enum tag values and non-variant member names; but fixing this will be
> done in later patches, in part because some further changes are required
> to keep QAPISchema*.check() from asserting. Also, a later patch will
> add a reservation for the member name 'u' to avoid a collision between a
> user's non-variant names and our internal choice of C union name.
Done.
>>
>> Note, however, that we do not rename the generated enum, which
>> is still 'FooKind'. A further patch could generate implicit
>> enums as 'FooType', but while the generator already reserved
>> the '*Kind' namespace (commit 4dc2e69), there are already QMP
>> constructs with '*Type' naming, which means changing our
>> reservation namespace would have lots of churn to C code to
>> deal with a forced name change.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v11 22/24] qapi: Finish converting to new qapi union layout
2015-10-27 16:01 ` Markus Armbruster
@ 2015-10-27 16:47 ` Eric Blake
2015-10-27 16:58 ` Markus Armbruster
0 siblings, 1 reply; 38+ messages in thread
From: Eric Blake @ 2015-10-27 16:47 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 553 bytes --]
On 10/27/2015 10:01 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> I went through all commit messages and tweaked around occurences of QMP.
> I pushed the result to qapi-next; please have a look.
Commit 2858a50 (Prefer typesafe upcasts...) on that branch has too much
text (you pasted in corrected text, but forgot to delete the old).
Otherwise looks good. Thanks for the touchups. Now on to subset C :)
--
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] 38+ messages in thread
* Re: [Qemu-devel] [PATCH v11 22/24] qapi: Finish converting to new qapi union layout
2015-10-27 16:47 ` Eric Blake
@ 2015-10-27 16:58 ` Markus Armbruster
0 siblings, 0 replies; 38+ messages in thread
From: Markus Armbruster @ 2015-10-27 16:58 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 10/27/2015 10:01 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>
>> I went through all commit messages and tweaked around occurences of QMP.
>> I pushed the result to qapi-next; please have a look.
>
> Commit 2858a50 (Prefer typesafe upcasts...) on that branch has too much
> text (you pasted in corrected text, but forgot to delete the old).
> Otherwise looks good. Thanks for the touchups. Now on to subset C :)
Fixed up & pushed.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v11 23/24] qapi: Reserve 'u' member name
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (21 preceding siblings ...)
2015-10-26 22:35 ` [Qemu-devel] [PATCH v11 22/24] qapi: Finish converting " Eric Blake
@ 2015-10-26 22:35 ` Eric Blake
2015-10-27 8:21 ` Markus Armbruster
2015-10-26 22:35 ` [Qemu-devel] [PATCH v11 24/24] qapi: Simplify gen_struct_field() Eric Blake
2015-10-27 8:39 ` [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Markus Armbruster
24 siblings, 1 reply; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:35 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Michael Roth
Now that we have separated union tag values from colliding with
non-variant C names, by naming the union 'u', we should reserve
this name for our use. Note that we want to forbid 'u' even in
a struct with no variants, because it is possible for a future
qemu release to extend QMP in a backwards-compatible manner while
converting from a struct to a flat union. Fortunately, no
existing clients were using this member name. If we ever find
the need for QMP to have a member 'u', we could at that time
relax things, perhaps by having c_name() munge the QMP member to
'q_u'.
Note that we cannot forbid 'u' everywhere (by adding the
rejection code to check_name()), because the existing QKeyCode
enum already uses it; therefore we only reserve it as a struct
type member name.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: commit message tweaks, use c_name(), tweak test names
v10: new patch, split from 1/17 and 3/17; by deferring this until
after the rename was complete, it reduces churn due to the new feature
---
scripts/qapi.py | 2 +-
tests/Makefile | 1 +
tests/qapi-schema/reserved-member-u.err | 1 +
tests/qapi-schema/reserved-member-u.exit | 1 +
tests/qapi-schema/reserved-member-u.json | 7 +++++++
tests/qapi-schema/reserved-member-u.out | 0
6 files changed, 11 insertions(+), 1 deletion(-)
create mode 100644 tests/qapi-schema/reserved-member-u.err
create mode 100644 tests/qapi-schema/reserved-member-u.exit
create mode 100644 tests/qapi-schema/reserved-member-u.json
create mode 100644 tests/qapi-schema/reserved-member-u.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 3fead04..89f3207 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -490,7 +490,7 @@ def check_type(expr_info, source, value, allow_array=False,
for (key, arg) in value.items():
check_name(expr_info, "Member of %s" % source, key,
allow_optional=allow_optional)
- if c_name(key).startswith('has_'):
+ if c_name(key, False) == 'u' or c_name(key).startswith('has_'):
raise QAPIExprError(expr_info,
"Member of %s uses reserved name '%s'"
% (source, key))
diff --git a/tests/Makefile b/tests/Makefile
index 6d7c07e..f448b5a 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -318,6 +318,7 @@ qapi-schema += redefined-type.json
qapi-schema += reserved-command-q.json
qapi-schema += reserved-member-has.json
qapi-schema += reserved-member-q.json
+qapi-schema += reserved-member-u.json
qapi-schema += reserved-type-kind.json
qapi-schema += reserved-type-list.json
qapi-schema += returns-alternate.json
diff --git a/tests/qapi-schema/reserved-member-u.err b/tests/qapi-schema/reserved-member-u.err
new file mode 100644
index 0000000..87d4229
--- /dev/null
+++ b/tests/qapi-schema/reserved-member-u.err
@@ -0,0 +1 @@
+tests/qapi-schema/reserved-member-u.json:7: Member of 'data' for struct 'Oops' uses reserved name 'u'
diff --git a/tests/qapi-schema/reserved-member-u.exit b/tests/qapi-schema/reserved-member-u.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/reserved-member-u.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/reserved-member-u.json b/tests/qapi-schema/reserved-member-u.json
new file mode 100644
index 0000000..1eaf0f3
--- /dev/null
+++ b/tests/qapi-schema/reserved-member-u.json
@@ -0,0 +1,7 @@
+# Potential C member name collision
+# We reject use of 'u' as a member name, to allow it for internal use in
+# putting union branch members in a separate namespace from QMP members.
+# This is true even for non-unions, because it is possible to convert a
+# struct to flat union while remaining backwards compatible in QMP.
+# TODO - we could munge the member name to 'q_u' to avoid the collision
+{ 'struct': 'Oops', 'data': { 'u': 'str' } }
diff --git a/tests/qapi-schema/reserved-member-u.out b/tests/qapi-schema/reserved-member-u.out
new file mode 100644
index 0000000..e69de29
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v11 23/24] qapi: Reserve 'u' member name
2015-10-26 22:35 ` [Qemu-devel] [PATCH v11 23/24] qapi: Reserve 'u' member name Eric Blake
@ 2015-10-27 8:21 ` Markus Armbruster
2015-10-27 14:23 ` Eric Blake
0 siblings, 1 reply; 38+ messages in thread
From: Markus Armbruster @ 2015-10-27 8:21 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> Now that we have separated union tag values from colliding with
> non-variant C names, by naming the union 'u', we should reserve
> this name for our use. Note that we want to forbid 'u' even in
> a struct with no variants, because it is possible for a future
> qemu release to extend QMP in a backwards-compatible manner while
> converting from a struct to a flat union. Fortunately, no
> existing clients were using this member name. If we ever find
> the need for QMP to have a member 'u', we could at that time
> relax things, perhaps by having c_name() munge the QMP member to
> 'q_u'.
>
> Note that we cannot forbid 'u' everywhere (by adding the
> rejection code to check_name()), because the existing QKeyCode
> enum already uses it; therefore we only reserve it as a struct
> type member name.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v11: commit message tweaks, use c_name(), tweak test names
> v10: new patch, split from 1/17 and 3/17; by deferring this until
> after the rename was complete, it reduces churn due to the new feature
> ---
> scripts/qapi.py | 2 +-
> tests/Makefile | 1 +
> tests/qapi-schema/reserved-member-u.err | 1 +
> tests/qapi-schema/reserved-member-u.exit | 1 +
> tests/qapi-schema/reserved-member-u.json | 7 +++++++
> tests/qapi-schema/reserved-member-u.out | 0
> 6 files changed, 11 insertions(+), 1 deletion(-)
> create mode 100644 tests/qapi-schema/reserved-member-u.err
> create mode 100644 tests/qapi-schema/reserved-member-u.exit
> create mode 100644 tests/qapi-schema/reserved-member-u.json
> create mode 100644 tests/qapi-schema/reserved-member-u.out
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 3fead04..89f3207 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -490,7 +490,7 @@ def check_type(expr_info, source, value, allow_array=False,
> for (key, arg) in value.items():
> check_name(expr_info, "Member of %s" % source, key,
> allow_optional=allow_optional)
> - if c_name(key).startswith('has_'):
> + if c_name(key, False) == 'u' or c_name(key).startswith('has_'):
Slightly odd: new c_name() has protect=False, the existing one doesn't.
While we don't really need protect=False, it feels a bit cleaner. If
you like, I can add it to the existing one when it gets added in PATCH
05.
> raise QAPIExprError(expr_info,
> "Member of %s uses reserved name '%s'"
> % (source, key))
> diff --git a/tests/Makefile b/tests/Makefile
> index 6d7c07e..f448b5a 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -318,6 +318,7 @@ qapi-schema += redefined-type.json
> qapi-schema += reserved-command-q.json
> qapi-schema += reserved-member-has.json
> qapi-schema += reserved-member-q.json
> +qapi-schema += reserved-member-u.json
> qapi-schema += reserved-type-kind.json
> qapi-schema += reserved-type-list.json
> qapi-schema += returns-alternate.json
> diff --git a/tests/qapi-schema/reserved-member-u.err b/tests/qapi-schema/reserved-member-u.err
> new file mode 100644
> index 0000000..87d4229
> --- /dev/null
> +++ b/tests/qapi-schema/reserved-member-u.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/reserved-member-u.json:7: Member of 'data' for struct 'Oops' uses reserved name 'u'
> diff --git a/tests/qapi-schema/reserved-member-u.exit b/tests/qapi-schema/reserved-member-u.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/reserved-member-u.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/reserved-member-u.json b/tests/qapi-schema/reserved-member-u.json
> new file mode 100644
> index 0000000..1eaf0f3
> --- /dev/null
> +++ b/tests/qapi-schema/reserved-member-u.json
> @@ -0,0 +1,7 @@
> +# Potential C member name collision
> +# We reject use of 'u' as a member name, to allow it for internal use in
> +# putting union branch members in a separate namespace from QMP members.
> +# This is true even for non-unions, because it is possible to convert a
> +# struct to flat union while remaining backwards compatible in QMP.
> +# TODO - we could munge the member name to 'q_u' to avoid the collision
> +{ 'struct': 'Oops', 'data': { 'u': 'str' } }
> diff --git a/tests/qapi-schema/reserved-member-u.out b/tests/qapi-schema/reserved-member-u.out
> new file mode 100644
> index 0000000..e69de29
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v11 23/24] qapi: Reserve 'u' member name
2015-10-27 8:21 ` Markus Armbruster
@ 2015-10-27 14:23 ` Eric Blake
2015-10-27 15:14 ` Markus Armbruster
0 siblings, 1 reply; 38+ messages in thread
From: Eric Blake @ 2015-10-27 14:23 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Michael Roth
[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]
On 10/27/2015 02:21 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> Now that we have separated union tag values from colliding with
>> non-variant C names, by naming the union 'u', we should reserve
>> this name for our use. Note that we want to forbid 'u' even in
>> a struct with no variants, because it is possible for a future
>> qemu release to extend QMP in a backwards-compatible manner while
>> converting from a struct to a flat union. Fortunately, no
>> existing clients were using this member name. If we ever find
>> the need for QMP to have a member 'u', we could at that time
>> relax things, perhaps by having c_name() munge the QMP member to
>> 'q_u'.
>>
>> Note that we cannot forbid 'u' everywhere (by adding the
>> rejection code to check_name()), because the existing QKeyCode
>> enum already uses it; therefore we only reserve it as a struct
>> type member name.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v11: commit message tweaks, use c_name(), tweak test names
>> - if c_name(key).startswith('has_'):
>> + if c_name(key, False) == 'u' or c_name(key).startswith('has_'):
>
> Slightly odd: new c_name() has protect=False, the existing one doesn't.
> While we don't really need protect=False, it feels a bit cleaner. If
> you like, I can add it to the existing one when it gets added in PATCH
> 05.
You're right - either both places need it, or neither place does.
Argument _for_ using c_name(, False): that's what we do in check_name()
when looking for 'q_', because we have to (if we use the default
c_name(, True), then the name gets munged and starts with q_ even though
the original did not). So even though we don't munge 'u' or 'has_' now,
if c_name() starts munging them in the future, consistently using
c_name(, False) here will protect us. So yes, make the change in patch 5.
--
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] 38+ messages in thread* Re: [Qemu-devel] [PATCH v11 23/24] qapi: Reserve 'u' member name
2015-10-27 14:23 ` Eric Blake
@ 2015-10-27 15:14 ` Markus Armbruster
0 siblings, 0 replies; 38+ messages in thread
From: Markus Armbruster @ 2015-10-27 15:14 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 10/27/2015 02:21 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> Now that we have separated union tag values from colliding with
>>> non-variant C names, by naming the union 'u', we should reserve
>>> this name for our use. Note that we want to forbid 'u' even in
>>> a struct with no variants, because it is possible for a future
>>> qemu release to extend QMP in a backwards-compatible manner while
>>> converting from a struct to a flat union. Fortunately, no
>>> existing clients were using this member name. If we ever find
>>> the need for QMP to have a member 'u', we could at that time
>>> relax things, perhaps by having c_name() munge the QMP member to
>>> 'q_u'.
>>>
>>> Note that we cannot forbid 'u' everywhere (by adding the
>>> rejection code to check_name()), because the existing QKeyCode
>>> enum already uses it; therefore we only reserve it as a struct
>>> type member name.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>>
>>> ---
>>> v11: commit message tweaks, use c_name(), tweak test names
>
>>> - if c_name(key).startswith('has_'):
>>> + if c_name(key, False) == 'u' or c_name(key).startswith('has_'):
>>
>> Slightly odd: new c_name() has protect=False, the existing one doesn't.
>> While we don't really need protect=False, it feels a bit cleaner. If
>> you like, I can add it to the existing one when it gets added in PATCH
>> 05.
>
> You're right - either both places need it, or neither place does.
> Argument _for_ using c_name(, False): that's what we do in check_name()
> when looking for 'q_', because we have to (if we use the default
> c_name(, True), then the name gets munged and starts with q_ even though
> the original did not). So even though we don't munge 'u' or 'has_' now,
> if c_name() starts munging them in the future, consistently using
> c_name(, False) here will protect us. So yes, make the change in patch 5.
Done.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [Qemu-devel] [PATCH v11 24/24] qapi: Simplify gen_struct_field()
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (22 preceding siblings ...)
2015-10-26 22:35 ` [Qemu-devel] [PATCH v11 23/24] qapi: Reserve 'u' member name Eric Blake
@ 2015-10-26 22:35 ` Eric Blake
2015-10-27 8:39 ` [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Markus Armbruster
24 siblings, 0 replies; 38+ messages in thread
From: Eric Blake @ 2015-10-26 22:35 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.
No change to generated code.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
v11: rebase to earlier changes
v10: no change
v9: rebase after kind/base cleanups, don't rely on member.c_name()
v8: new patch
---
scripts/qapi-types.py | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 7e35bb6..b37900f 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=c_name(member.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=c_name(member.name))
return ret
@@ -60,13 +60,13 @@ def gen_struct_fields(local_members, base=None):
''',
c_name=base.c_name())
for memb in base.members:
- ret += gen_struct_field(memb.name, memb.type, memb.optional)
+ ret += gen_struct_field(memb)
ret += mcgen('''
/* Own members: */
''')
for memb in local_members:
- ret += gen_struct_field(memb.name, memb.type, memb.optional)
+ ret += gen_struct_field(memb)
return ret
@@ -149,9 +149,7 @@ struct %(c_name)s {
if base:
ret += gen_struct_fields([], base)
else:
- ret += gen_struct_field(variants.tag_member.name,
- variants.tag_member.type,
- False)
+ ret += gen_struct_field(variants.tag_member)
# 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
--
2.4.3
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B')
2015-10-26 22:34 [Qemu-devel] [PATCH v11 00/24] qapi collision reduction (post-introspection subset B') Eric Blake
` (23 preceding siblings ...)
2015-10-26 22:35 ` [Qemu-devel] [PATCH v11 24/24] qapi: Simplify gen_struct_field() Eric Blake
@ 2015-10-27 8:39 ` Markus Armbruster
24 siblings, 0 replies; 38+ messages in thread
From: Markus Armbruster @ 2015-10-27 8:39 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Eduardo Habkost
Eric Blake <eblake@redhat.com> writes:
> No pending prerequisites (applies to current master)
>
> Also available as a tag at this location:
> git fetch git://repo.or.cz/qemu/ericb.git qapi-cleanupv11b
>
> 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
>
> v11 notes:
> Series is definitely converging. Drop patch v10 24/25, and address
> comments on several other patches. Some tests are renamed for more
> consistent names, but for the most parts, the changes are localized
> and don't have quite the ripple effect that was seen from v9 -> v10.
Yup.
I found a few opportunities for minor improvement, all simple
enough to be done on commit to my tree.
^ permalink raw reply [flat|nested] 38+ messages in thread