[Qemu-devel] [PATCH v7 00/14] post-introspection cleanups, subset B

[Qemu-devel] [PATCH v7 00/14] post-introspection cleanups, subset B

[Eric Blake]

Pending prerequisite: Markus' qapi-next branch (which has my
subset A patches):
git://repo.or.cz/qemu/armbru.git qapi-next
http://thread.gmane.org/gmane.comp.emulators.qemu/365827/focus=366351

Also available as a tag at this location:
git fetch git://repo.or.cz/qemu/ericb.git qapi-cleanupv7b

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

v7 notes:
Address comments, including a couple of new commits that made
later patches a bit cleaner.  Backport diff gets a bit confused
by a couple of patch titles changing.

001/14:[0013] [FC] 'qapi: Use predicate callback to determine visit filtering'
002/14:[down] 'qapi: Prepare for errors during check()'
003/14:[down] 'qapi: Drop redundant alternate-good test'
004/14:[0026] [FC] 'qapi: Don't use info as witness of implicit object type'
005/14:[0032] [FC] 'qapi: Lazy creation of array types'
006/14:[0018] [FC] 'qapi: Create simple union type member earlier'
007/14:[down] 'qapi: Move union tag quirks into subclass'
008/14:[0039] [FC] 'qapi: Track location that created an implicit type'
009/14:[0190] [FC] 'qapi: Track owner of each object member'
010/14:[0023] [FC] 'qapi: Detect collisions in C member names'
011/14:[down] 'qapi: Move duplicate member checks to schema check()'
012/14:[down] 'qapi: Move duplicate enum value checks to schema check()'
013/14:[down] 'qapi: Add test for alternate branch 'kind' clash'
014/14:[----] [--] 'qapi: Detect base class loops'

v6 notes:
http://thread.gmane.org/gmane.comp.emulators.qemu/366417
This is patches 11-16 of my v5 series; it has grown a bit with
splitting some patches and adding some others.  I suspect that
12/12 on this series will be discarded, but am including it because
it was split from v5 content.

Not much review comments other than on the original 11/46, but there
is enough churn due to rebasing that it's now easier to review this
version than plowing through v5.

Subset C (and more?) will come later.

In v5:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05410.html
I _did_ rearrange patches to try and group related features:

1-2: Groundwork cleanups
3-5: Add more test cases
6-16: Front-end cleanups
17-18: Introspection output cleanups
19-20: 'alternate' type cleanups
21-29: qapi visitor cleanups
30-45: qapi-ify netdev_add
46: add qapi shorthand for flat unions

Lots of fixes based on additional testing, and rebased to
track other changes that happened in the meantime.  The series
is huge; I can split off smaller portions as requested.

In v4:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02580.html
add some more clean up patches
rebase to Markus' recent work
pull in part of Zoltán's work to make netdev_add a flat union,
further enhancing it to be introspectible

I might be able to rearrange some of these patches, or separate
it into smaller independent series, if requested; but I'm
posting now to get review started.

In v3:
https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02059.html
redo cleanup of dealloc of partial struct
add patches to make all visit_type_*() avoid leaks on failure
add patches to allow boxed command arguments and events

In v2:
https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg00900.html
rebase to Markus' v3 series
rework how comments are emitted for fields inherited from base
additional patches added for deleting colliding 'void *data'
documentation updates to match code changes

v1 was here:
https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg05266.html
https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg05325.html

Eric Blake (14):
  qapi: Use predicate callback to determine visit filtering
  qapi: Prepare for errors during check()
  qapi: Drop redundant alternate-good test
  qapi: Don't use info as witness of implicit object type
  qapi: Lazy creation of array types
  qapi: Create simple union type member earlier
  qapi: Move union tag quirks into subclass
  qapi: Track location that created an implicit type
  qapi: Track owner of each object member
  qapi: Detect collisions in C member names
  qapi: Move duplicate member checks to schema check()
  qapi: Move duplicate enum value checks to schema check()
  qapi: Add test for alternate branch 'kind' clash
  qapi: Detect base class loops

 qapi-schema.json                                   |  11 +
 scripts/qapi-commands.py                           |   8 +-
 scripts/qapi-introspect.py                         |   5 +-
 scripts/qapi-types.py                              |  31 +-
 scripts/qapi-visit.py                              |  34 +--
 scripts/qapi.py                                    | 321 +++++++++++++--------
 tests/Makefile                                     |   6 +-
 tests/qapi-schema/alternate-clash-members.err      |   1 +
 ...ad-branch.exit => alternate-clash-members.exit} |   0
 ...ate-clash.json => alternate-clash-members.json} |   0
 ...-bad-branch.out => alternate-clash-members.out} |   0
 tests/qapi-schema/alternate-clash-type.err         |   1 +
 ...ernate-clash.exit => alternate-clash-type.exit} |   0
 tests/qapi-schema/alternate-clash-type.json        |  10 +
 ...alternate-good.err => alternate-clash-type.out} |   0
 tests/qapi-schema/alternate-clash.err              |   1 -
 tests/qapi-schema/alternate-good.exit              |   1 -
 tests/qapi-schema/alternate-good.json              |   9 -
 tests/qapi-schema/alternate-good.out               |  10 -
 tests/qapi-schema/args-member-array.out            |   4 +-
 tests/qapi-schema/args-name-clash.err              |   1 +
 tests/qapi-schema/args-name-clash.exit             |   2 +-
 tests/qapi-schema/args-name-clash.json             |   6 +-
 tests/qapi-schema/args-name-clash.out              |   6 -
 tests/qapi-schema/base-cycle.err                   |   1 +
 tests/qapi-schema/base-cycle.exit                  |   1 +
 tests/qapi-schema/base-cycle.json                  |   3 +
 .../{alternate-clash.out => base-cycle.out}        |   0
 tests/qapi-schema/enum-clash-member.err            |   2 +-
 tests/qapi-schema/enum-max-member.err              |   2 +-
 tests/qapi-schema/flat-union-clash-branch.err      |   1 +
 tests/qapi-schema/flat-union-clash-branch.exit     |   2 +-
 tests/qapi-schema/flat-union-clash-branch.json     |   9 +-
 tests/qapi-schema/flat-union-clash-branch.out      |  14 -
 tests/qapi-schema/flat-union-clash-member.err      |   2 +-
 tests/qapi-schema/flat-union-clash-type.err        |   2 +-
 tests/qapi-schema/ident-with-escape.out            |   4 +-
 tests/qapi-schema/qapi-schema-test.json            |   4 +
 tests/qapi-schema/qapi-schema-test.out             |  91 +++---
 tests/qapi-schema/struct-base-clash-deep.err       |   2 +-
 tests/qapi-schema/struct-base-clash.err            |   2 +-
 tests/qapi-schema/union-bad-branch.err             |   1 -
 tests/qapi-schema/union-bad-branch.json            |   8 -
 tests/qapi-schema/union-clash-branches.err         |   2 +-
 tests/qapi-schema/union-clash-data.out             |   4 +-
 tests/qapi-schema/union-clash-type.err             |   2 +-
 tests/qapi-schema/union-max.err                    |   2 +-
 47 files changed, 341 insertions(+), 288 deletions(-)
 create mode 100644 tests/qapi-schema/alternate-clash-members.err
 rename tests/qapi-schema/{union-bad-branch.exit => alternate-clash-members.exit} (100%)
 rename tests/qapi-schema/{alternate-clash.json => alternate-clash-members.json} (100%)
 rename tests/qapi-schema/{union-bad-branch.out => alternate-clash-members.out} (100%)
 create mode 100644 tests/qapi-schema/alternate-clash-type.err
 rename tests/qapi-schema/{alternate-clash.exit => alternate-clash-type.exit} (100%)
 create mode 100644 tests/qapi-schema/alternate-clash-type.json
 rename tests/qapi-schema/{alternate-good.err => alternate-clash-type.out} (100%)
 delete mode 100644 tests/qapi-schema/alternate-clash.err
 delete mode 100644 tests/qapi-schema/alternate-good.exit
 delete mode 100644 tests/qapi-schema/alternate-good.json
 delete mode 100644 tests/qapi-schema/alternate-good.out
 create mode 100644 tests/qapi-schema/base-cycle.err
 create mode 100644 tests/qapi-schema/base-cycle.exit
 create mode 100644 tests/qapi-schema/base-cycle.json
 rename tests/qapi-schema/{alternate-clash.out => base-cycle.out} (100%)
 delete mode 100644 tests/qapi-schema/union-bad-branch.err
 delete mode 100644 tests/qapi-schema/union-bad-branch.json

-- 
2.4.3

[Qemu-devel] [PATCH v7 01/14] qapi: Use predicate callback to determine visit filtering

[Eric Blake]

Previously, qapi-types and qapi-visit filtered out implicit
objects during visit_object_type() by using 'info' (works since
implicit objects do not [yet] have associated info); meanwhile
qapi-introspect filtered out all schema types on the first pass
by returning a python type from visit_begin(), which was then
used at a distance in QAPISchema.visit() to do the filtering.

Rather than keeping these ad hoc approaches, add a new visitor
callback visit_needed() which returns False to skip a given
entity, and which defaults to True unless overridden.  Use the
new mechanism to simplify all three filtering visitors.

No change to the generated code.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>

---
v7: rename to visit_needed(), add comment
v6: new patch
---
 scripts/qapi-introspect.py |  5 ++++-
 scripts/qapi-types.py      | 19 +++++++++++--------
 scripts/qapi-visit.py      | 17 ++++++++++-------
 scripts/qapi.py            | 12 ++++++++----
 4 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
index 7d39320..c0dad66 100644
--- a/scripts/qapi-introspect.py
+++ b/scripts/qapi-introspect.py
@@ -54,7 +54,6 @@ class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor):
         self._jsons = []
         self._used_types = []
         self._name_map = {}
-        return QAPISchemaType   # don't visit types for now

     def visit_end(self):
         # visit the types that are actually used
@@ -82,6 +81,10 @@ const char %(c_name)s[] = %(c_string)s;
         self._used_types = None
         self._name_map = None

+    def visit_needed(self, entity):
+        # Ignore types on first pass; visit_end() will pick up used types
+        return not isinstance(entity, QAPISchemaType)
+
     def _name(self, name):
         if self._unmask:
             return name
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index d405f8d..2a29c6e 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -233,6 +233,10 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
         self.decl = self._btin + self.decl
         self._btin = None

+    def visit_needed(self, entity):
+        # Visit everything except implicit objects
+        return not isinstance(entity, QAPISchemaObjectType) or entity.info
+
     def _gen_type_cleanup(self, name):
         self.decl += gen_type_cleanup_decl(name)
         self.defn += gen_type_cleanup(name)
@@ -254,14 +258,13 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
             self._gen_type_cleanup(name)

     def visit_object_type(self, name, info, base, members, variants):
-        if info:
-            self._fwdecl += gen_fwd_object_or_array(name)
-            if variants:
-                assert not members      # not implemented
-                self.decl += gen_union(name, base, variants)
-            else:
-                self.decl += gen_struct(name, base, members)
-            self._gen_type_cleanup(name)
+        self._fwdecl += gen_fwd_object_or_array(name)
+        if variants:
+            assert not members      # not implemented
+            self.decl += gen_union(name, base, variants)
+        else:
+            self.decl += gen_struct(name, base, members)
+        self._gen_type_cleanup(name)

     def visit_alternate_type(self, name, info, variants):
         self._fwdecl += gen_fwd_object_or_array(name)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 4f97781..9fc79a7 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -333,6 +333,10 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
         self.decl = self._btin + self.decl
         self._btin = None

+    def visit_needed(self, entity):
+        # Visit everything except implicit objects
+        return not isinstance(entity, QAPISchemaObjectType) or entity.info
+
     def visit_enum_type(self, name, info, values, prefix):
         self.decl += gen_visit_decl(name, scalar=True)
         self.defn += gen_visit_enum(name)
@@ -349,13 +353,12 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
             self.defn += defn

     def visit_object_type(self, name, info, base, members, variants):
-        if info:
-            self.decl += gen_visit_decl(name)
-            if variants:
-                assert not members      # not implemented
-                self.defn += gen_visit_union(name, base, variants)
-            else:
-                self.defn += gen_visit_struct(name, base, members)
+        self.decl += gen_visit_decl(name)
+        if variants:
+            assert not members      # not implemented
+            self.defn += gen_visit_union(name, base, variants)
+        else:
+            self.defn += gen_visit_struct(name, base, members)

     def visit_alternate_type(self, name, info, variants):
         self.decl += gen_visit_decl(name)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 26cff3f..543b378 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -811,6 +811,10 @@ class QAPISchemaVisitor(object):
     def visit_end(self):
         pass

+    def visit_needed(self, entity):
+        # Default to visiting everything
+        return True
+
     def visit_builtin_type(self, name, info, json_type):
         pass

@@ -1304,10 +1308,10 @@ class QAPISchema(object):
             ent.check(self)

     def visit(self, visitor):
-        ignore = visitor.visit_begin(self)
-        for name in sorted(self._entity_dict.keys()):
-            if not ignore or not isinstance(self._entity_dict[name], ignore):
-                self._entity_dict[name].visit(visitor)
+        visitor.visit_begin(self)
+        for (name, entity) in sorted(self._entity_dict.items()):
+            if visitor.visit_needed(entity):
+                entity.visit(visitor)
         visitor.visit_end()


-- 
2.4.3

[Qemu-devel] [PATCH v7 02/14] qapi: Prepare for errors during check()

[Eric Blake]

The next few patches will start migrating error checking from
ad hoc parse methods into the QAPISchema*.check() methods.  But
for an error message to display, we first have to fix the
overall 'try' to catch those errors.  We also want to enable a
few more assertions, such as making sure every attempt to
raise a semantic error is passed a valid location info, or that
various preconditions hold.

The general approach for moving error checking will then be to
relax an assertion into an if that raises an exception if the
condition does not hold, and removing the counterpart ad hoc
check done during the parse phase.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v7: Split off of v6 7/12, plus other obvious asserts floated up
---
 scripts/qapi.py | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 543b378..4573599 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -103,6 +103,7 @@ class QAPISchemaError(Exception):
 class QAPIExprError(Exception):
     def __init__(self, expr_info, msg):
         Exception.__init__(self)
+        assert expr_info
         self.info = expr_info
         self.msg = msg

@@ -964,6 +965,7 @@ class QAPISchemaObjectType(QAPISchemaType):
             members = []
         seen = {}
         for m in members:
+            assert c_name(m.name) not in seen
             seen[m.name] = m
         for m in self.local_members:
             m.check(schema, members, seen)
@@ -1116,13 +1118,13 @@ class QAPISchema(object):
     def __init__(self, fname):
         try:
             self.exprs = check_exprs(QAPISchemaParser(open(fname, "r")).exprs)
+            self._entity_dict = {}
+            self._def_predefineds()
+            self._def_exprs()
+            self.check()
         except (QAPISchemaError, QAPIExprError), err:
             print >>sys.stderr, err
             exit(1)
-        self._entity_dict = {}
-        self._def_predefineds()
-        self._def_exprs()
-        self.check()

     def _def_entity(self, ent):
         assert ent.name not in self._entity_dict
-- 
2.4.3

[Qemu-devel] [PATCH v7 03/14] qapi: Drop redundant alternate-good test

[Eric Blake]

The alternate-good.json test was already covered by
qapi-schema-test.json.  As future commits will be tweaking
how alternates are laid out, removing the duplicate test now
reduces churn.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v7: new patch
---
 tests/Makefile                        |  1 -
 tests/qapi-schema/alternate-good.err  |  0
 tests/qapi-schema/alternate-good.exit |  1 -
 tests/qapi-schema/alternate-good.json |  9 ---------
 tests/qapi-schema/alternate-good.out  | 10 ----------
 5 files changed, 21 deletions(-)
 delete mode 100644 tests/qapi-schema/alternate-good.err
 delete mode 100644 tests/qapi-schema/alternate-good.exit
 delete mode 100644 tests/qapi-schema/alternate-good.json
 delete mode 100644 tests/qapi-schema/alternate-good.out

diff --git a/tests/Makefile b/tests/Makefile
index edf310d..d18b3b2 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -227,7 +227,6 @@ qapi-schema += alternate-clash.json
 qapi-schema += alternate-conflict-dict.json
 qapi-schema += alternate-conflict-string.json
 qapi-schema += alternate-empty.json
-qapi-schema += alternate-good.json
 qapi-schema += alternate-nested.json
 qapi-schema += alternate-unknown.json
 qapi-schema += args-alternate.json
diff --git a/tests/qapi-schema/alternate-good.err b/tests/qapi-schema/alternate-good.err
deleted file mode 100644
index e69de29..0000000
diff --git a/tests/qapi-schema/alternate-good.exit b/tests/qapi-schema/alternate-good.exit
deleted file mode 100644
index 573541a..0000000
--- a/tests/qapi-schema/alternate-good.exit
+++ /dev/null
@@ -1 +0,0 @@
-0
diff --git a/tests/qapi-schema/alternate-good.json b/tests/qapi-schema/alternate-good.json
deleted file mode 100644
index 3371770..0000000
--- a/tests/qapi-schema/alternate-good.json
+++ /dev/null
@@ -1,9 +0,0 @@
-# Working example of alternate
-{ 'struct': 'Data',
-  'data': { '*number': 'int', '*name': 'str' } }
-{ 'enum': 'Enum',
-  'data': [ 'hello', 'world' ] }
-{ 'alternate': 'Alt',
-  'data': { 'value': 'int',
-            'string': 'Enum',
-            'struct': 'Data' } }
diff --git a/tests/qapi-schema/alternate-good.out b/tests/qapi-schema/alternate-good.out
deleted file mode 100644
index 65af727..0000000
--- a/tests/qapi-schema/alternate-good.out
+++ /dev/null
@@ -1,10 +0,0 @@
-object :empty
-alternate Alt
-    case value: int
-    case string: Enum
-    case struct: Data
-enum AltKind ['value', 'string', 'struct']
-object Data
-    member number: int optional=True
-    member name: str optional=True
-enum Enum ['hello', 'world']
-- 
2.4.3

Re: [Qemu-devel] [PATCH v7 03/14] qapi: Drop redundant alternate-good test

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> The alternate-good.json test was already covered by
> qapi-schema-test.json.  As future commits will be tweaking
> how alternates are laid out, removing the duplicate test now
> reduces churn.

Do we have more positive tests?  They should probably all live in
qapi-schema-test.json, because there their generated code actually gets
compiled.

Patch looks good.

Re: [Qemu-devel] [PATCH v7 03/14] qapi: Drop redundant alternate-good test

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1684 bytes --]

On 10/07/2015 10:15 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> The alternate-good.json test was already covered by
>> qapi-schema-test.json.  As future commits will be tweaking
>> how alternates are laid out, removing the duplicate test now
>> reduces churn.
> 
> Do we have more positive tests?  They should probably all live in
> qapi-schema-test.json, because there their generated code actually gets
> compiled.

Hmm, any test that has an empty .err and non-empty .out, but which does
not also have an TODO/FIXME stating that it is a bug, is worth checking.
 So first, here's the list of non-empty .out files:

empty.out [1]
enum-empty.out
event-case.out [4]
union-empty.out [2]
include-simple.out
include-repetition.out
include-relpath.out
comments.out
alternate-empty.out [2]
returns-int.out
struct-base-clash-base.out [3]
flat-union-empty.out [2]
indented-expr.out
union-clash-data.out [3]
ident-with-escape.out
args-member-array.out
flat-union-reverse-define.out

[1] must stay separate, because we can't really make qapi-schema-test
empty :)
[2] marked FIXME, and turns into proper error message later in the v5 series
[3] marked FIXME, and already removed later in the v5 series once a
positive test is inlined into qapi-schema-test
[4] marked TODO

For the ones I didn't tag with a footnote, we could probably cover
things directly in qapi-schema-test instead; the question then becomes
which ones are already covered, and which ones need content moved.

> 
> Patch looks good.
> 

-- 
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 --]

Re: [Qemu-devel] [PATCH v7 03/14] qapi: Drop redundant alternate-good test

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/07/2015 10:15 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> The alternate-good.json test was already covered by
>>> qapi-schema-test.json.  As future commits will be tweaking
>>> how alternates are laid out, removing the duplicate test now
>>> reduces churn.
>> 
>> Do we have more positive tests?  They should probably all live in
>> qapi-schema-test.json, because there their generated code actually gets
>> compiled.
>
> Hmm, any test that has an empty .err and non-empty .out, but which does
> not also have an TODO/FIXME stating that it is a bug, is worth checking.
>  So first, here's the list of non-empty .out files:
>
> empty.out [1]
> enum-empty.out
> event-case.out [4]
> union-empty.out [2]
> include-simple.out
> include-repetition.out
> include-relpath.out
> comments.out
> alternate-empty.out [2]
> returns-int.out
> struct-base-clash-base.out [3]
> flat-union-empty.out [2]
> indented-expr.out
> union-clash-data.out [3]
> ident-with-escape.out
> args-member-array.out
> flat-union-reverse-define.out
>
> [1] must stay separate, because we can't really make qapi-schema-test
> empty :)
> [2] marked FIXME, and turns into proper error message later in the v5 series
> [3] marked FIXME, and already removed later in the v5 series once a
> positive test is inlined into qapi-schema-test
> [4] marked TODO
>
> For the ones I didn't tag with a footnote, we could probably cover
> things directly in qapi-schema-test instead; the question then becomes
> which ones are already covered, and which ones need content moved.

Actually, the point isn't to move the positive test to
qapi-schema-test.json, the point is to compile-test its generated code.
Moving it to qapi-schema-test.json accomplishes that.  However, we may
not want a single, monolithic positive test.  Should we split up
qapi-schema-test.json instead?  I don't know.  Anyway, let's flush our
queue first.

Third case: the generated code isn't worth compile-testing; comparing
actual to expected .out suffices.

Let's sort your untagged tests into buckets:

Not worth compile-testing:
* comments.json
* include-simple.json
* include-repetition.json
* include-relpath.json
* indented-expr.json
* ident-with-escape.json

Not (completely) covered in qapi-schema-test.json:
* enum-empty.json
  Not covered, but should be.
* flat-union-reverse-define.json
  UserDefOne covers forward reference to struct base, UserDefFlatUnion
  covers forward reference to union base, and UserDefFlatUnion2 covers
  forward reference to member.  We may want to cover forward reference
  to the tag member's type.

Covered:
* returns-int.json
  'user_def_cmd3' does the job.
* args-member-array.json
  '__org.qemu_x-command' seems good enough.

Re: [Qemu-devel] [PATCH v7 03/14] qapi: Drop redundant alternate-good test

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1934 bytes --]

On 10/13/2015 02:12 AM, Markus Armbruster wrote:

>> Hmm, any test that has an empty .err and non-empty .out, but which does
>> not also have an TODO/FIXME stating that it is a bug, is worth checking.
>>  So first, here's the list of non-empty .out files:
>>

> 
> Actually, the point isn't to move the positive test to
> qapi-schema-test.json, the point is to compile-test its generated code.
> Moving it to qapi-schema-test.json accomplishes that.  However, we may
> not want a single, monolithic positive test.  Should we split up
> qapi-schema-test.json instead?  I don't know.  Anyway, let's flush our
> queue first.

If we do split qapi-schema-test, it won't be until after my patches are
flushed :)

> 
> Third case: the generated code isn't worth compile-testing; comparing
> actual to expected .out suffices.
> 
> Let's sort your untagged tests into buckets:
> 
> Not worth compile-testing:
> * comments.json
> * include-simple.json
> * include-repetition.json
> * include-relpath.json
> * indented-expr.json
> * ident-with-escape.json
> 
> Not (completely) covered in qapi-schema-test.json:
> * enum-empty.json
>   Not covered, but should be.

Covered in v8:
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02869.html

> * flat-union-reverse-define.json
>   UserDefOne covers forward reference to struct base, UserDefFlatUnion
>   covers forward reference to union base, and UserDefFlatUnion2 covers
>   forward reference to member.  We may want to cover forward reference
>   to the tag member's type.

Covered in v8

> 
> Covered:
> * returns-int.json
>   'user_def_cmd3' does the job.

Cleaned up in v8

> * args-member-array.json
>   '__org.qemu_x-command' seems good enough.

Not cleaned up yet, so I'll add it to my next round of 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 --]

[Qemu-devel] [PATCH v7 04/14] qapi: Don't use info as witness of implicit object type

[Eric Blake]

A future patch will enable error reporting from the various
QAPISchema*.check() methods.  But to report an error related
to an implicit type, we'll need to associate a location with
the type (the same location as the top-level entity that is
causing the creation of the implicit type), and once we do
that, keying off of whether foo.info exists is no longer a
viable way to determine if foo is an implicit type.

Instead, add an is_implicit() method to QAPISchemaEntity, with
an internal helper overridden for ObjectType and EnumType, and
use that function where needed.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v7: declare at the Entity level, with an optional argument for
filtering by sub-class
v6: split 11/46 into pieces; don't rename _info yet; rework atop
nicer filtering mechanism, including no need to change visitor
signature
---
 scripts/qapi-types.py |  2 +-
 scripts/qapi-visit.py |  2 +-
 scripts/qapi.py       | 24 +++++++++++++++++++++---
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 2a29c6e..227ea5c 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -235,7 +235,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):

     def visit_needed(self, entity):
         # Visit everything except implicit objects
-        return not isinstance(entity, QAPISchemaObjectType) or entity.info
+        return not entity.is_implicit(QAPISchemaObjectType)

     def _gen_type_cleanup(self, name):
         self.decl += gen_type_cleanup_decl(name)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 9fc79a7..56b8390 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -335,7 +335,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):

     def visit_needed(self, entity):
         # Visit everything except implicit objects
-        return not isinstance(entity, QAPISchemaObjectType) or entity.info
+        return not entity.is_implicit(QAPISchemaObjectType)

     def visit_enum_type(self, name, info, values, prefix):
         self.decl += gen_visit_decl(name, scalar=True)
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 4573599..19cc78e 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -801,6 +801,16 @@ class QAPISchemaEntity(object):
     def check(self, schema):
         pass

+    # Return True if this entity is implicit
+    # If @typ, only return True if entity is also an instance of that type
+    def is_implicit(self, typ=None):
+        if typ and not isinstance(self, typ):
+            return False
+        return self._is_implicit()
+
+    def _is_implicit(self):
+        return not self.info
+
     def visit(self, visitor):
         pass

@@ -903,6 +913,10 @@ class QAPISchemaEnumType(QAPISchemaType):
     def check(self, schema):
         assert len(set(self.values)) == len(self.values)

+    def _is_implicit(self):
+        # See QAPISchema._make_implicit_enum_type()
+        return self.name[-4:] == 'Kind'
+
     def c_type(self, is_param=False):
         return c_name(self.name)

@@ -973,12 +987,16 @@ class QAPISchemaObjectType(QAPISchemaType):
             self.variants.check(schema, members, seen)
         self.members = members

+    def _is_implicit(self):
+        # See QAPISchema._make_implicit_object_type()
+        return self.name[0] == ':'
+
     def c_name(self):
-        assert self.info
+        assert not self.is_implicit()
         return QAPISchemaType.c_name(self)

     def c_type(self, is_param=False):
-        assert self.info
+        assert not self.is_implicit()
         return QAPISchemaType.c_type(self)

     def json_type(self):
@@ -1046,7 +1064,7 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
     # This function exists to support ugly simple union special cases
     # TODO get rid of them, and drop the function
     def simple_union_type(self):
-        if isinstance(self.type, QAPISchemaObjectType) and not self.type.info:
+        if self.type.is_implicit(QAPISchemaObjectType):
             assert len(self.type.members) == 1
             assert not self.type.variants
             return self.type.members[0].type
-- 
2.4.3

Re: [Qemu-devel] [PATCH v7 04/14] qapi: Don't use info as witness of implicit object type

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> A future patch will enable error reporting from the various
> QAPISchema*.check() methods.  But to report an error related
> to an implicit type, we'll need to associate a location with
> the type (the same location as the top-level entity that is
> causing the creation of the implicit type), and once we do
> that, keying off of whether foo.info exists is no longer a
> viable way to determine if foo is an implicit type.
>
> Instead, add an is_implicit() method to QAPISchemaEntity, with
> an internal helper overridden for ObjectType and EnumType, and
> use that function where needed.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v7: declare at the Entity level, with an optional argument for
> filtering by sub-class
> v6: split 11/46 into pieces; don't rename _info yet; rework atop
> nicer filtering mechanism, including no need to change visitor
> signature
> ---
>  scripts/qapi-types.py |  2 +-
>  scripts/qapi-visit.py |  2 +-
>  scripts/qapi.py       | 24 +++++++++++++++++++++---
>  3 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 2a29c6e..227ea5c 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -235,7 +235,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>
>      def visit_needed(self, entity):
>          # Visit everything except implicit objects
> -        return not isinstance(entity, QAPISchemaObjectType) or entity.info
> +        return not entity.is_implicit(QAPISchemaObjectType)

The alternative is something like

        return not (isinstance(entity, QAPISchemaObjectType) and
                    entity.is_implicit())

Trades a more verbose "is this an implicit object type" check for a
simpler is_implicit().  Shorter overall, and feels better to me.  But if
you feel strongly about your way of doing it, I can live with it.

>
>      def _gen_type_cleanup(self, name):
>          self.decl += gen_type_cleanup_decl(name)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 9fc79a7..56b8390 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -335,7 +335,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
>
>      def visit_needed(self, entity):
>          # Visit everything except implicit objects
> -        return not isinstance(entity, QAPISchemaObjectType) or entity.info
> +        return not entity.is_implicit(QAPISchemaObjectType)
>
>      def visit_enum_type(self, name, info, values, prefix):
>          self.decl += gen_visit_decl(name, scalar=True)
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4573599..19cc78e 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -801,6 +801,16 @@ class QAPISchemaEntity(object):
>      def check(self, schema):
>          pass
>
> +    # Return True if this entity is implicit
> +    # If @typ, only return True if entity is also an instance of that type
> +    def is_implicit(self, typ=None):
> +        if typ and not isinstance(self, typ):
> +            return False
> +        return self._is_implicit()
> +
> +    def _is_implicit(self):
> +        return not self.info
> +
>      def visit(self, visitor):
>          pass
>
> @@ -903,6 +913,10 @@ class QAPISchemaEnumType(QAPISchemaType):
>      def check(self, schema):
>          assert len(set(self.values)) == len(self.values)
>
> +    def _is_implicit(self):
> +        # See QAPISchema._make_implicit_enum_type()
> +        return self.name[-4:] == 'Kind'
> +
>      def c_type(self, is_param=False):
>          return c_name(self.name)
>

Might want to add a comment to _make_implicit_enum_type() that justifies
name + 'Kind' by pointing out add_name() rejects a 'Kind' suffix.

> @@ -973,12 +987,16 @@ class QAPISchemaObjectType(QAPISchemaType):
>              self.variants.check(schema, members, seen)
>          self.members = members
>
> +    def _is_implicit(self):
> +        # See QAPISchema._make_implicit_object_type()
> +        return self.name[0] == ':'
> +
>      def c_name(self):
> -        assert self.info
> +        assert not self.is_implicit()
>          return QAPISchemaType.c_name(self)
>
>      def c_type(self, is_param=False):
> -        assert self.info
> +        assert not self.is_implicit()
>          return QAPISchemaType.c_type(self)
>
>      def json_type(self):
> @@ -1046,7 +1064,7 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>      # This function exists to support ugly simple union special cases
>      # TODO get rid of them, and drop the function
>      def simple_union_type(self):
> -        if isinstance(self.type, QAPISchemaObjectType) and not self.type.info:
> +        if self.type.is_implicit(QAPISchemaObjectType):
>              assert len(self.type.members) == 1
>              assert not self.type.variants
>              return self.type.members[0].type

Re: [Qemu-devel] [PATCH v7 04/14] qapi: Don't use info as witness of implicit object type

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

On 10/07/2015 10:27 AM, Markus Armbruster wrote:

>>      def visit_needed(self, entity):
>>          # Visit everything except implicit objects
>> -        return not isinstance(entity, QAPISchemaObjectType) or entity.info
>> +        return not entity.is_implicit(QAPISchemaObjectType)
> 
> The alternative is something like
> 
>         return not (isinstance(entity, QAPISchemaObjectType) and
>                     entity.is_implicit())
> 
> Trades a more verbose "is this an implicit object type" check for a
> simpler is_implicit().  Shorter overall, and feels better to me.  But if
> you feel strongly about your way of doing it, I can live with it.

I'm not strongly tied to the concise form enough to stall review, so v8
will use the longer explicit form.

-- 
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 --]

[Qemu-devel] [PATCH v7 05/14] qapi: Lazy creation of array types

[Eric Blake]

Commit ac88219a had several TODO markers about whether we needed
to automatically create the corresponding array type alongside
any other type.  It turns out that most of the time, we don't!

As part of lazy creation of array types, this patch now assigns
an 'info' to array types at their point of first instantiation,
rather than leaving it None.

There are a few exceptions: 1) We have a few situations where we
use an array type in internal code but do not expose that type
through QMP; fix it by declaring a dummy type that forces the
generator to see that we want to use the array type.

2) The builtin arrays (such as intList for QAPI ['int']) must
always be generated, because of the way our QAPI_TYPES_BUILTIN
compile guard works: we have situations (at the very least
tests/test-qmp-output-visitor.c) that include both top-level
"qapi-types.h" (via "error.h") and a secondary
"test-qapi-types.h". If we were to only emit the builtin types
when used locally, then the first .h file would not include all
types, but the second .h does not declare anything at all because
the first .h set QAPI_TYPES_BUILTIN, and we would end up with
compilation error due to things like unknown type 'int8List'.

Actually, we may need to revisit how we do type guards, and
change from a single QAPI_TYPES_BUILTIN over to a different
usage pattern that does one #ifdef per qapi type - right now,
the only types that are declared multiple times between two qapi
.json files for inclusion by a single .c file happen to be the
builtin arrays.  But now that we have QAPI 'include' statements,
it is logical to assume that we will soon reach a point where
we want to reuse non-builtin types (yes, I'm thinking about what
it will take to add introspection to QGA, where we will want to
reuse the SchemaInfo type and friends).  One #ifdef per type
will help ensure that generating the same qapi type into more
than one qapi-types.h won't cause collisions when both are
included in the same .c file; but we also have to solve how to
avoid creating duplicate qapi-types.c entry points.  So that
is a problem left for another day.

Generated code for qapi-types and qapi-visit is drastically
reduced; less than a third of the arrays that were blindly
created were actually needed (a quick grep shows we dropped
from 219 to 69 *List types), and the .o files lost more than
30% of their bulk.  [For best results, diff the generated
files with 'git diff --patience --no-index pre post'.]

Interestingly, the introspection output is unchanged - this is
because we already cull all types that are not indirectly
reachable from a command or event, so introspection was already
using only a subset of array types.  The subset of types
introspected is now a much larger percentage of the overall set
of array types emitted in qapi-types.h (since the larger set
shrunk), but still not 100% (proof that the array types emitted
for our new Dummy structs, and the new struct itself, don't
affect QMP).

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v7: improve commit message, add comments, rename dummy type,
better line wrapping
v6: new patch
---
 qapi-schema.json                        | 11 +++++++
 scripts/qapi.py                         | 52 ++++++++++++++++++---------------
 tests/qapi-schema/qapi-schema-test.json |  4 +++
 tests/qapi-schema/qapi-schema-test.out  |  3 ++
 4 files changed, 47 insertions(+), 23 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 8b0520c..81d2e18 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3396,6 +3396,17 @@
             'features': 'int' } }

 ##
+# @DummyForceArrays
+#
+# Not used by QMP; hack to let us use X86CPUFeatureWordInfoList internally
+#
+# Since 2.5
+##
+{ 'struct': 'DummyForceArrays',
+  'data': { 'unused': ['X86CPUFeatureWordInfo'] } }
+
+
+##
 # @RxState:
 #
 # Packets receiving state
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 19cc78e..b1134b8 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -793,6 +793,11 @@ class QAPISchemaEntity(object):
     def __init__(self, name, info):
         assert isinstance(name, str)
         self.name = name
+        # For explicitly defined entities, info points to the (explicit)
+        # definition.  For builtins (and their arrays), info is None.
+        # TODO For other implicitly defined entities, it should point to
+        # a place that triggers implicit definition; there may be more
+        # than one such place.
         self.info = info

     def c_name(self):
@@ -1160,7 +1165,12 @@ class QAPISchema(object):
     def _def_builtin_type(self, name, json_type, c_type, c_null):
         self._def_entity(QAPISchemaBuiltinType(name, json_type,
                                                c_type, c_null))
-        self._make_array_type(name)     # TODO really needed?
+        # TODO As long as we have QAPI_TYPES_BUILTIN to share multiple
+        # qapi-types.h from a single .c, all arrays of builtins must be
+        # declared in the first file whether or not they are used.  Nicer
+        # would be to use lazy instantiation, while figuring out how to
+        # avoid compilation issues with multiple qapi-types.h.
+        self._make_array_type(name, None)

     def _def_predefineds(self):
         for t in [('str',    'string',  'char' + pointer_suffix, 'NULL'),
@@ -1187,10 +1197,10 @@ class QAPISchema(object):
         self._def_entity(QAPISchemaEnumType(name, None, values, None))
         return name

-    def _make_array_type(self, element_type):
+    def _make_array_type(self, element_type, info):
         name = element_type + 'List'
         if not self.lookup_type(name):
-            self._def_entity(QAPISchemaArrayType(name, None, element_type))
+            self._def_entity(QAPISchemaArrayType(name, info, element_type))
         return name

     def _make_implicit_object_type(self, name, role, members):
@@ -1207,20 +1217,19 @@ class QAPISchema(object):
         data = expr['data']
         prefix = expr.get('prefix')
         self._def_entity(QAPISchemaEnumType(name, info, data, prefix))
-        self._make_array_type(name)     # TODO really needed?

-    def _make_member(self, name, typ):
+    def _make_member(self, name, typ, info):
         optional = False
         if name.startswith('*'):
             name = name[1:]
             optional = True
         if isinstance(typ, list):
             assert len(typ) == 1
-            typ = self._make_array_type(typ[0])
+            typ = self._make_array_type(typ[0], info)
         return QAPISchemaObjectTypeMember(name, typ, optional)

-    def _make_members(self, data):
-        return [self._make_member(key, value)
+    def _make_members(self, data, info):
+        return [self._make_member(key, value, info)
                 for (key, value) in data.iteritems()]

     def _def_struct_type(self, expr, info):
@@ -1228,19 +1237,18 @@ class QAPISchema(object):
         base = expr.get('base')
         data = expr['data']
         self._def_entity(QAPISchemaObjectType(name, info, base,
-                                              self._make_members(data),
+                                              self._make_members(data, info),
                                               None))
-        self._make_array_type(name)     # TODO really needed?

     def _make_variant(self, case, typ):
         return QAPISchemaObjectTypeVariant(case, typ)

-    def _make_simple_variant(self, case, typ):
+    def _make_simple_variant(self, case, typ, info):
         if isinstance(typ, list):
             assert len(typ) == 1
-            typ = self._make_array_type(typ[0])
-        typ = self._make_implicit_object_type(typ, 'wrapper',
-                                              [self._make_member('data', typ)])
+            typ = self._make_array_type(typ[0], info)
+        typ = self._make_implicit_object_type(
+            typ, 'wrapper', [self._make_member('data', typ, info)])
         return QAPISchemaObjectTypeVariant(case, typ)

     def _make_tag_enum(self, type_name, variants):
@@ -1257,16 +1265,15 @@ class QAPISchema(object):
             variants = [self._make_variant(key, value)
                         for (key, value) in data.iteritems()]
         else:
-            variants = [self._make_simple_variant(key, value)
+            variants = [self._make_simple_variant(key, value, info)
                         for (key, value) in data.iteritems()]
             tag_enum = self._make_tag_enum(name, variants)
         self._def_entity(
             QAPISchemaObjectType(name, info, base,
-                                 self._make_members(OrderedDict()),
+                                 self._make_members(OrderedDict(), info),
                                  QAPISchemaObjectTypeVariants(tag_name,
                                                               tag_enum,
                                                               variants)))
-        self._make_array_type(name)     # TODO really needed?

     def _def_alternate_type(self, expr, info):
         name = expr['alternate']
@@ -1279,7 +1286,6 @@ class QAPISchema(object):
                                     QAPISchemaObjectTypeVariants(None,
                                                                  tag_enum,
                                                                  variants)))
-        self._make_array_type(name)     # TODO really needed?

     def _def_command(self, expr, info):
         name = expr['command']
@@ -1288,11 +1294,11 @@ class QAPISchema(object):
         gen = expr.get('gen', True)
         success_response = expr.get('success-response', True)
         if isinstance(data, OrderedDict):
-            data = self._make_implicit_object_type(name, 'arg',
-                                                   self._make_members(data))
+            data = self._make_implicit_object_type(
+                name, 'arg', self._make_members(data, info))
         if isinstance(rets, list):
             assert len(rets) == 1
-            rets = self._make_array_type(rets[0])
+            rets = self._make_array_type(rets[0], info)
         self._def_entity(QAPISchemaCommand(name, info, data, rets, gen,
                                            success_response))

@@ -1300,8 +1306,8 @@ class QAPISchema(object):
         name = expr['event']
         data = expr.get('data')
         if isinstance(data, OrderedDict):
-            data = self._make_implicit_object_type(name, 'arg',
-                                                   self._make_members(data))
+            data = self._make_implicit_object_type(
+                name, 'arg', self._make_members(data, info))
         self._def_entity(QAPISchemaEvent(name, info, data))

     def _def_exprs(self):
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index abe59fd..020ff2e 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -31,6 +31,10 @@
   'data': { 'string0': 'str',
             'dict1': 'UserDefTwoDict' } }

+# dummy struct to force generation of array types not otherwise mentioned
+{ 'struct': 'ForceArrays',
+  'data': { 'unused1':['UserDefOne'], 'unused2':['UserDefTwo'] } }
+
 # for testing unions
 # Among other things, test that a name collision between branches does
 # not cause any problems (since only one branch can be in use at a time),
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 8f81784..2a8c82e 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -86,6 +86,9 @@ object EventStructOne
     member struct1: UserDefOne optional=False
     member string: str optional=False
     member enum2: EnumOne optional=True
+object ForceArrays
+    member unused1: UserDefOneList optional=False
+    member unused2: UserDefTwoList optional=False
 object NestedEnumsOne
     member enum1: EnumOne optional=False
     member enum2: EnumOne optional=True
-- 
2.4.3

Re: [Qemu-devel] [PATCH v7 05/14] qapi: Lazy creation of array types

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Commit ac88219a had several TODO markers about whether we needed
> to automatically create the corresponding array type alongside
> any other type.  It turns out that most of the time, we don't!
>
> As part of lazy creation of array types, this patch now assigns
> an 'info' to array types at their point of first instantiation,
> rather than leaving it None.
>
> There are a few exceptions: 1) We have a few situations where we
> use an array type in internal code but do not expose that type
> through QMP; fix it by declaring a dummy type that forces the
> generator to see that we want to use the array type.
>
> 2) The builtin arrays (such as intList for QAPI ['int']) must
> always be generated, because of the way our QAPI_TYPES_BUILTIN
> compile guard works: we have situations (at the very least
> tests/test-qmp-output-visitor.c) that include both top-level
> "qapi-types.h" (via "error.h") and a secondary
> "test-qapi-types.h". If we were to only emit the builtin types
> when used locally, then the first .h file would not include all
> types, but the second .h does not declare anything at all because
> the first .h set QAPI_TYPES_BUILTIN, and we would end up with
> compilation error due to things like unknown type 'int8List'.
>
> Actually, we may need to revisit how we do type guards, and
> change from a single QAPI_TYPES_BUILTIN over to a different
> usage pattern that does one #ifdef per qapi type - right now,
> the only types that are declared multiple times between two qapi
> .json files for inclusion by a single .c file happen to be the
> builtin arrays.  But now that we have QAPI 'include' statements,
> it is logical to assume that we will soon reach a point where
> we want to reuse non-builtin types (yes, I'm thinking about what
> it will take to add introspection to QGA, where we will want to
> reuse the SchemaInfo type and friends).  One #ifdef per type
> will help ensure that generating the same qapi type into more
> than one qapi-types.h won't cause collisions when both are
> included in the same .c file; but we also have to solve how to
> avoid creating duplicate qapi-types.c entry points.  So that
> is a problem left for another day.
>
> Generated code for qapi-types and qapi-visit is drastically
> reduced; less than a third of the arrays that were blindly
> created were actually needed (a quick grep shows we dropped
> from 219 to 69 *List types), and the .o files lost more than
> 30% of their bulk.  [For best results, diff the generated
> files with 'git diff --patience --no-index pre post'.]
>
> Interestingly, the introspection output is unchanged - this is
> because we already cull all types that are not indirectly
> reachable from a command or event, so introspection was already
> using only a subset of array types.  The subset of types
> introspected is now a much larger percentage of the overall set
> of array types emitted in qapi-types.h (since the larger set
> shrunk), but still not 100% (proof that the array types emitted

Evidence, not proof :)

> for our new Dummy structs, and the new struct itself, don't
> affect QMP).
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v7: improve commit message, add comments, rename dummy type,
> better line wrapping
> v6: new patch
> ---
>  qapi-schema.json                        | 11 +++++++
>  scripts/qapi.py                         | 52 ++++++++++++++++++---------------
>  tests/qapi-schema/qapi-schema-test.json |  4 +++
>  tests/qapi-schema/qapi-schema-test.out  |  3 ++
>  4 files changed, 47 insertions(+), 23 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 8b0520c..81d2e18 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3396,6 +3396,17 @@
>              'features': 'int' } }
>
>  ##
> +# @DummyForceArrays
> +#
> +# Not used by QMP; hack to let us use X86CPUFeatureWordInfoList internally
> +#
> +# Since 2.5
> +##
> +{ 'struct': 'DummyForceArrays',
> +  'data': { 'unused': ['X86CPUFeatureWordInfo'] } }
> +
> +
> +##
>  # @RxState:
>  #
>  # Packets receiving state
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 19cc78e..b1134b8 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -793,6 +793,11 @@ class QAPISchemaEntity(object):
>      def __init__(self, name, info):
>          assert isinstance(name, str)
>          self.name = name
> +        # For explicitly defined entities, info points to the (explicit)
> +        # definition.  For builtins (and their arrays), info is None.
> +        # TODO For other implicitly defined entities, it should point to
> +        # a place that triggers implicit definition; there may be more
> +        # than one such place.

The TODO suggests that info doesn't yet point to a place that triggers
implicit definition.  Yet your patch makes it point at least for arrays.
To fix, you could insert "For other arrays, info points to a place that
implicitly defines the array."

>          self.info = info
>
>      def c_name(self):
> @@ -1160,7 +1165,12 @@ class QAPISchema(object):
>      def _def_builtin_type(self, name, json_type, c_type, c_null):
>          self._def_entity(QAPISchemaBuiltinType(name, json_type,
>                                                 c_type, c_null))
> -        self._make_array_type(name)     # TODO really needed?
> +        # TODO As long as we have QAPI_TYPES_BUILTIN to share multiple
> +        # qapi-types.h from a single .c, all arrays of builtins must be
> +        # declared in the first file whether or not they are used.  Nicer
> +        # would be to use lazy instantiation, while figuring out how to
> +        # avoid compilation issues with multiple qapi-types.h.
> +        self._make_array_type(name, None)
>
>      def _def_predefineds(self):
>          for t in [('str',    'string',  'char' + pointer_suffix, 'NULL'),
> @@ -1187,10 +1197,10 @@ class QAPISchema(object):
>          self._def_entity(QAPISchemaEnumType(name, None, values, None))
>          return name
>
> -    def _make_array_type(self, element_type):
> +    def _make_array_type(self, element_type, info):
>          name = element_type + 'List'
>          if not self.lookup_type(name):
> -            self._def_entity(QAPISchemaArrayType(name, None, element_type))
> +            self._def_entity(QAPISchemaArrayType(name, info, element_type))
>          return name
>
>      def _make_implicit_object_type(self, name, role, members):
> @@ -1207,20 +1217,19 @@ class QAPISchema(object):
>          data = expr['data']
>          prefix = expr.get('prefix')
>          self._def_entity(QAPISchemaEnumType(name, info, data, prefix))
> -        self._make_array_type(name)     # TODO really needed?
>
> -    def _make_member(self, name, typ):
> +    def _make_member(self, name, typ, info):
>          optional = False
>          if name.startswith('*'):
>              name = name[1:]
>              optional = True
>          if isinstance(typ, list):
>              assert len(typ) == 1
> -            typ = self._make_array_type(typ[0])
> +            typ = self._make_array_type(typ[0], info)
>          return QAPISchemaObjectTypeMember(name, typ, optional)
>
> -    def _make_members(self, data):
> -        return [self._make_member(key, value)
> +    def _make_members(self, data, info):
> +        return [self._make_member(key, value, info)
>                  for (key, value) in data.iteritems()]
>
>      def _def_struct_type(self, expr, info):
> @@ -1228,19 +1237,18 @@ class QAPISchema(object):
>          base = expr.get('base')
>          data = expr['data']
>          self._def_entity(QAPISchemaObjectType(name, info, base,
> -                                              self._make_members(data),
> +                                              self._make_members(data, info),
>                                                None))
> -        self._make_array_type(name)     # TODO really needed?
>
>      def _make_variant(self, case, typ):
>          return QAPISchemaObjectTypeVariant(case, typ)
>
> -    def _make_simple_variant(self, case, typ):
> +    def _make_simple_variant(self, case, typ, info):
>          if isinstance(typ, list):
>              assert len(typ) == 1
> -            typ = self._make_array_type(typ[0])
> -        typ = self._make_implicit_object_type(typ, 'wrapper',
> -                                              [self._make_member('data', typ)])
> +            typ = self._make_array_type(typ[0], info)
> +        typ = self._make_implicit_object_type(
> +            typ, 'wrapper', [self._make_member('data', typ, info)])

I'd indent the hanging intent a bit more, to make the = stand out.

>          return QAPISchemaObjectTypeVariant(case, typ)
>
>      def _make_tag_enum(self, type_name, variants):
> @@ -1257,16 +1265,15 @@ class QAPISchema(object):
>              variants = [self._make_variant(key, value)
>                          for (key, value) in data.iteritems()]
>          else:
> -            variants = [self._make_simple_variant(key, value)
> +            variants = [self._make_simple_variant(key, value, info)
>                          for (key, value) in data.iteritems()]
>              tag_enum = self._make_tag_enum(name, variants)
>          self._def_entity(
>              QAPISchemaObjectType(name, info, base,
> -                                 self._make_members(OrderedDict()),
> +                                 self._make_members(OrderedDict(), info),
>                                   QAPISchemaObjectTypeVariants(tag_name,
>                                                                tag_enum,
>                                                                variants)))
> -        self._make_array_type(name)     # TODO really needed?
>
>      def _def_alternate_type(self, expr, info):
>          name = expr['alternate']
> @@ -1279,7 +1286,6 @@ class QAPISchema(object):
>                                      QAPISchemaObjectTypeVariants(None,
>                                                                   tag_enum,
>                                                                   variants)))
> -        self._make_array_type(name)     # TODO really needed?
>
>      def _def_command(self, expr, info):
>          name = expr['command']
> @@ -1288,11 +1294,11 @@ class QAPISchema(object):
>          gen = expr.get('gen', True)
>          success_response = expr.get('success-response', True)
>          if isinstance(data, OrderedDict):
> -            data = self._make_implicit_object_type(name, 'arg',
> -                                                   self._make_members(data))
> +            data = self._make_implicit_object_type(
> +                name, 'arg', self._make_members(data, info))

Likewise.

>          if isinstance(rets, list):
>              assert len(rets) == 1
> -            rets = self._make_array_type(rets[0])
> +            rets = self._make_array_type(rets[0], info)
>          self._def_entity(QAPISchemaCommand(name, info, data, rets, gen,
>                                             success_response))
>
> @@ -1300,8 +1306,8 @@ class QAPISchema(object):
>          name = expr['event']
>          data = expr.get('data')
>          if isinstance(data, OrderedDict):
> -            data = self._make_implicit_object_type(name, 'arg',
> -                                                   self._make_members(data))
> +            data = self._make_implicit_object_type(
> +                name, 'arg', self._make_members(data, info))

Likewise.

>          self._def_entity(QAPISchemaEvent(name, info, data))
>
>      def _def_exprs(self):
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index abe59fd..020ff2e 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -31,6 +31,10 @@
>    'data': { 'string0': 'str',
>              'dict1': 'UserDefTwoDict' } }
>
> +# dummy struct to force generation of array types not otherwise mentioned
> +{ 'struct': 'ForceArrays',
> +  'data': { 'unused1':['UserDefOne'], 'unused2':['UserDefTwo'] } }
> +
>  # for testing unions
>  # Among other things, test that a name collision between branches does
>  # not cause any problems (since only one branch can be in use at a time),
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 8f81784..2a8c82e 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -86,6 +86,9 @@ object EventStructOne
>      member struct1: UserDefOne optional=False
>      member string: str optional=False
>      member enum2: EnumOne optional=True
> +object ForceArrays
> +    member unused1: UserDefOneList optional=False
> +    member unused2: UserDefTwoList optional=False
>  object NestedEnumsOne
>      member enum1: EnumOne optional=False
>      member enum2: EnumOne optional=True

Re: [Qemu-devel] [PATCH v7 05/14] qapi: Lazy creation of array types

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]

On 10/07/2015 10:38 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Commit ac88219a had several TODO markers about whether we needed
>> to automatically create the corresponding array type alongside
>> any other type.  It turns out that most of the time, we don't!
>>

>> +    def _make_simple_variant(self, case, typ, info):
>>          if isinstance(typ, list):
>>              assert len(typ) == 1
>> -            typ = self._make_array_type(typ[0])
>> -        typ = self._make_implicit_object_type(typ, 'wrapper',
>> -                                              [self._make_member('data', typ)])
>> +            typ = self._make_array_type(typ[0], info)
>> +        typ = self._make_implicit_object_type(
>> +            typ, 'wrapper', [self._make_member('data', typ, info)])
> 
> I'd indent the hanging intent a bit more, to make the = stand out.

pep8 doesn't like it:

scripts/qapi.py:1299:17: E126 continuation line over-indented for
hanging indent

but it was okay with:

    type = (self._make_implicit_object_type(
            type, ...))

and that does look a little better.

-- 
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 --]

Re: [Qemu-devel] [PATCH v7 05/14] qapi: Lazy creation of array types

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/07/2015 10:38 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> Commit ac88219a had several TODO markers about whether we needed
>>> to automatically create the corresponding array type alongside
>>> any other type.  It turns out that most of the time, we don't!
>>>
>
>>> +    def _make_simple_variant(self, case, typ, info):
>>>          if isinstance(typ, list):
>>>              assert len(typ) == 1
>>> -            typ = self._make_array_type(typ[0])
>>> -        typ = self._make_implicit_object_type(typ, 'wrapper',
>>> -                                              [self._make_member('data', typ)])
>>> +            typ = self._make_array_type(typ[0], info)
>>> +        typ = self._make_implicit_object_type(
>>> +            typ, 'wrapper', [self._make_member('data', typ, info)])
>> 
>> I'd indent the hanging intent a bit more, to make the = stand out.
>
> pep8 doesn't like it:
>
> scripts/qapi.py:1299:17: E126 continuation line over-indented for
> hanging indent

Meh.

See also https://github.com/PyCQA/pep8/issues/167

> but it was okay with:
>
>     type = (self._make_implicit_object_type(
>             type, ...))
>
> and that does look a little better.

I find it a bit stilted.  Let's stick with your first version.

[Qemu-devel] [PATCH v7 06/14] qapi: Create simple union type member earlier

[Eric Blake]

For simple unions, we were creating the implicit 'type' tag
member during the QAPISchemaObjectTypeVariants constructor.
This is different from every other implicit QAPISchemaEntity
object, which get created by QAPISchema methods.  Hoist the
creation to the caller (renaming _make_tag_enum() to
_make_implicit_tag()), and pass the entity rather than the
string name, so that we have the nice property that no
entities are created as a side effect within a different
entity.  A later patch will then have an easier time of
associating location info with each entity creation.

No change to generated code.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v7: Rework assertions, rename to _make_implicit_tag()
v6: New patch
---
 scripts/qapi.py | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index b1134b8..eaa43b8 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1033,18 +1033,18 @@ class QAPISchemaObjectTypeMember(object):


 class QAPISchemaObjectTypeVariants(object):
-    def __init__(self, tag_name, tag_enum, variants):
-        assert tag_name is None or isinstance(tag_name, str)
-        assert tag_enum is None or isinstance(tag_enum, str)
+    def __init__(self, tag_name, tag_member, variants):
+        # Flat unions pass tag_name but not tag_member.
+        # Simple unions and alternates pass tag_member but not tag_name.
+        # After check(), tag_member is always set, and tag_name remains
+        # a reliable witness of being used by a flat union.
+        assert bool(tag_member) != bool(tag_name)
+        assert (isinstance(tag_name, str) or
+                isinstance(tag_member, QAPISchemaObjectTypeMember))
         for v in variants:
             assert isinstance(v, QAPISchemaObjectTypeVariant)
         self.tag_name = tag_name
-        if tag_name:
-            assert not tag_enum
-            self.tag_member = None
-        else:
-            self.tag_member = QAPISchemaObjectTypeMember('type', tag_enum,
-                                                         False)
+        self.tag_member = tag_member
         self.variants = variants

     def check(self, schema, members, seen):
@@ -1251,9 +1251,10 @@ class QAPISchema(object):
             typ, 'wrapper', [self._make_member('data', typ, info)])
         return QAPISchemaObjectTypeVariant(case, typ)

-    def _make_tag_enum(self, type_name, variants):
-        return self._make_implicit_enum_type(type_name,
-                                             [v.name for v in variants])
+    def _make_implicit_tag(self, type_name, variants):
+        typ = self._make_implicit_enum_type(type_name,
+                                            [v.name for v in variants])
+        return QAPISchemaObjectTypeMember('type', typ, False)

     def _def_union_type(self, expr, info):
         name = expr['union']
@@ -1267,7 +1268,7 @@ class QAPISchema(object):
         else:
             variants = [self._make_simple_variant(key, value, info)
                         for (key, value) in data.iteritems()]
-            tag_enum = self._make_tag_enum(name, variants)
+            tag_enum = self._make_implicit_tag(name, variants)
         self._def_entity(
             QAPISchemaObjectType(name, info, base,
                                  self._make_members(OrderedDict(), info),
@@ -1280,7 +1281,7 @@ class QAPISchema(object):
         data = expr['data']
         variants = [self._make_variant(key, value)
                     for (key, value) in data.iteritems()]
-        tag_enum = self._make_tag_enum(name, variants)
+        tag_enum = self._make_implicit_tag(name, variants)
         self._def_entity(
             QAPISchemaAlternateType(name, info,
                                     QAPISchemaObjectTypeVariants(None,
-- 
2.4.3

Re: [Qemu-devel] [PATCH v7 06/14] qapi: Create simple union type member earlier

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> For simple unions, we were creating the implicit 'type' tag
> member during the QAPISchemaObjectTypeVariants constructor.
> This is different from every other implicit QAPISchemaEntity
> object, which get created by QAPISchema methods.  Hoist the
> creation to the caller (renaming _make_tag_enum() to
> _make_implicit_tag()), and pass the entity rather than the
> string name, so that we have the nice property that no
> entities are created as a side effect within a different
> entity.  A later patch will then have an easier time of
> associating location info with each entity creation.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v7: Rework assertions, rename to _make_implicit_tag()
> v6: New patch
> ---
>  scripts/qapi.py | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index b1134b8..eaa43b8 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1033,18 +1033,18 @@ class QAPISchemaObjectTypeMember(object):
>
>
>  class QAPISchemaObjectTypeVariants(object):
> -    def __init__(self, tag_name, tag_enum, variants):
> -        assert tag_name is None or isinstance(tag_name, str)
> -        assert tag_enum is None or isinstance(tag_enum, str)
> +    def __init__(self, tag_name, tag_member, variants):
> +        # Flat unions pass tag_name but not tag_member.
> +        # Simple unions and alternates pass tag_member but not tag_name.
> +        # After check(), tag_member is always set, and tag_name remains
> +        # a reliable witness of being used by a flat union.
> +        assert bool(tag_member) != bool(tag_name)
> +        assert (isinstance(tag_name, str) or
> +                isinstance(tag_member, QAPISchemaObjectTypeMember))
>          for v in variants:
>              assert isinstance(v, QAPISchemaObjectTypeVariant)
>          self.tag_name = tag_name
> -        if tag_name:
> -            assert not tag_enum
> -            self.tag_member = None
> -        else:
> -            self.tag_member = QAPISchemaObjectTypeMember('type', tag_enum,
> -                                                         False)
> +        self.tag_member = tag_member
>          self.variants = variants
>
>      def check(self, schema, members, seen):
> @@ -1251,9 +1251,10 @@ class QAPISchema(object):
>              typ, 'wrapper', [self._make_member('data', typ, info)])
>          return QAPISchemaObjectTypeVariant(case, typ)
>
> -    def _make_tag_enum(self, type_name, variants):
> -        return self._make_implicit_enum_type(type_name,
> -                                             [v.name for v in variants])
> +    def _make_implicit_tag(self, type_name, variants):
> +        typ = self._make_implicit_enum_type(type_name,
> +                                            [v.name for v in variants])
> +        return QAPISchemaObjectTypeMember('type', typ, False)
>
>      def _def_union_type(self, expr, info):
>          name = expr['union']
> @@ -1267,7 +1268,7 @@ class QAPISchema(object):
>          else:
>              variants = [self._make_simple_variant(key, value, info)
>                          for (key, value) in data.iteritems()]
> -            tag_enum = self._make_tag_enum(name, variants)
> +            tag_enum = self._make_implicit_tag(name, variants)

Rename tag_enum to tag_member or tag?

>          self._def_entity(
>              QAPISchemaObjectType(name, info, base,
>                                   self._make_members(OrderedDict(), info),
> @@ -1280,7 +1281,7 @@ class QAPISchema(object):
>          data = expr['data']
>          variants = [self._make_variant(key, value)
>                      for (key, value) in data.iteritems()]
> -        tag_enum = self._make_tag_enum(name, variants)
> +        tag_enum = self._make_implicit_tag(name, variants)
>          self._def_entity(
>              QAPISchemaAlternateType(name, info,
>                                      QAPISchemaObjectTypeVariants(None,

Likewise.

[Qemu-devel] [PATCH v7 07/14] qapi: Move union tag quirks into subclass

[Eric Blake]

Right now, simple unions have a quirk of using 'kind' in the C
struct to match the QMP wire name 'type'.  This has resulted in
messy clients each doing special cases.  While we plan to
eventually rename things to match, it is better in the meantime
to consolidate the quirks into a special subclass, by adding a
new member.c_name() function.  This will also make it easier
for reworking how alternate types are laid out in a future
patch.  Use the new c_name() function where possible.

No change to generated code.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v7: new patch, but borrows idea of subclass from v6 10/12, as
well as c_name() from 7/12
---
 scripts/qapi-commands.py |  8 ++++----
 scripts/qapi-types.py    | 12 +++++-------
 scripts/qapi-visit.py    | 17 +++++------------
 scripts/qapi.py          | 15 +++++++++++++--
 4 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 43a893b..ff52ca9 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -32,8 +32,8 @@ def gen_call(name, arg_type, ret_type):
     if arg_type:
         for memb in arg_type.members:
             if memb.optional:
-                argstr += 'has_%s, ' % c_name(memb.name)
-            argstr += '%s, ' % c_name(memb.name)
+                argstr += 'has_%s, ' % memb.c_name()
+            argstr += '%s, ' % memb.c_name()

     lhs = ''
     if ret_type:
@@ -77,11 +77,11 @@ def gen_marshal_vars(arg_type, ret_type):
                 ret += mcgen('''
     bool has_%(c_name)s = false;
 ''',
-                             c_name=c_name(memb.name))
+                             c_name=memb.c_name())
             ret += mcgen('''
     %(c_type)s %(c_name)s = %(c_null)s;
 ''',
-                         c_name=c_name(memb.name),
+                         c_name=memb.c_name(),
                          c_type=memb.type.c_type(),
                          c_null=memb.type.c_null())
         ret += '\n'
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 227ea5c..34ea318 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -136,9 +136,10 @@ struct %(c_name)s {
 ''')
     else:
         ret += mcgen('''
-    %(c_type)s kind;
+    %(c_type)s %(c_name)s;
 ''',
-                     c_type=c_name(variants.tag_member.type.name))
+                     c_type=variants.tag_member.type.c_name(),
+                     c_name=variants.tag_member.c_name())

     # FIXME: What purpose does data serve, besides preventing a union that
     # has a branch named 'data'? We use it in qapi-visit.py to decide
@@ -152,10 +153,7 @@ struct %(c_name)s {
     union { /* union tag is @%(c_name)s */
         void *data;
 ''',
-                 # TODO ugly special case for simple union
-                 # Use same tag name in C as on the wire to get rid of
-                 # it, then: c_name=c_name(variants.tag_member.name)
-                 c_name=c_name(variants.tag_name or 'kind'))
+                 c_name=variants.tag_member.c_name())

     for var in variants.variants:
         # Ugly special case for simple union TODO get rid of it
@@ -164,7 +162,7 @@ struct %(c_name)s {
         %(c_type)s %(c_name)s;
 ''',
                      c_type=typ.c_type(),
-                     c_name=c_name(var.name))
+                     c_name=var.c_name())

     ret += mcgen('''
     };
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 56b8390..3f74302 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -196,7 +196,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
                      case=c_enum_const(variants.tag_member.type.name,
                                        var.name),
                      c_type=var.type.c_name(),
-                     c_name=c_name(var.name))
+                     c_name=var.c_name())

     ret += mcgen('''
     default:
@@ -249,10 +249,6 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
                      c_name=c_name(name))
         ret += gen_err_check(label='out_obj')

-    tag_key = variants.tag_member.name
-    if not variants.tag_name:
-        # we pointlessly use a different key for simple unions
-        tag_key = 'type'
     ret += mcgen('''
     visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
     if (err) {
@@ -264,11 +260,8 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
     switch ((*obj)->%(c_name)s) {
 ''',
                  c_type=variants.tag_member.type.c_name(),
-                 # TODO ugly special case for simple union
-                 # Use same tag name in C as on the wire to get rid of
-                 # it, then: c_name=c_name(variants.tag_member.name)
-                 c_name=c_name(variants.tag_name or 'kind'),
-                 name=tag_key)
+                 c_name=variants.tag_member.c_name(),
+                 name=variants.tag_member.name)

     for var in variants.variants:
         # TODO ugly special case for simple union
@@ -283,13 +276,13 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
         visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "data", &err);
 ''',
                          c_type=simple_union_type.c_name(),
-                         c_name=c_name(var.name))
+                         c_name=var.c_name())
         else:
             ret += mcgen('''
         visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
 ''',
                          c_type=var.type.c_name(),
-                         c_name=c_name(var.name))
+                         c_name=var.c_name())
         ret += mcgen('''
         break;
 ''')
diff --git a/scripts/qapi.py b/scripts/qapi.py
index eaa43b8..f5040da 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -984,7 +984,7 @@ class QAPISchemaObjectType(QAPISchemaType):
             members = []
         seen = {}
         for m in members:
-            assert c_name(m.name) not in seen
+            assert m.c_name() not in seen
             seen[m.name] = m
         for m in self.local_members:
             m.check(schema, members, seen)
@@ -1031,6 +1031,17 @@ class QAPISchemaObjectTypeMember(object):
         all_members.append(self)
         seen[self.name] = self

+    def c_name(self):
+        return c_name(self.name)
+
+
+# TODO Drop this class once we no longer have the 'type'/'kind' mismatch
+class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
+    def c_name(self):
+        assert self.name == 'type'
+        assert self.type.is_implicit(QAPISchemaEnumType)
+        return 'kind'
+

 class QAPISchemaObjectTypeVariants(object):
     def __init__(self, tag_name, tag_member, variants):
@@ -1254,7 +1265,7 @@ class QAPISchema(object):
     def _make_implicit_tag(self, type_name, variants):
         typ = self._make_implicit_enum_type(type_name,
                                             [v.name for v in variants])
-        return QAPISchemaObjectTypeMember('type', typ, False)
+        return QAPISchemaObjectTypeUnionTag('type', typ, False)

     def _def_union_type(self, expr, info):
         name = expr['union']
-- 
2.4.3

[Qemu-devel] [PATCH] fixup to qapi: Move union tag quirks into subclass

[Eric Blake]

Signed-off-by: Eric Blake <eblake@redhat.com>
---
I noticed a couple more places that could use the c_name() function;
I'll be squashing this into 7/14 if I need to do a v8 spin.

 scripts/qapi-commands.py |  4 ++--
 scripts/qapi.py          | 10 +++++-----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index ff52ca9..53a79ab 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -32,8 +32,8 @@ def gen_call(name, arg_type, ret_type):
     if arg_type:
         for memb in arg_type.members:
             if memb.optional:
-                argstr += 'has_%s, ' % memb.c_name()
-            argstr += '%s, ' % memb.c_name()
+                argstr += 'has_' + memb.c_name() + ', '
+            argstr += memb.c_name() + ', '

     lhs = ''
     if ret_type:
diff --git a/scripts/qapi.py b/scripts/qapi.py
index f5040da..37b38b0 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1572,8 +1572,8 @@ def gen_params(arg_type, extra):
         ret += sep
         sep = ', '
         if memb.optional:
-            ret += 'bool has_%s, ' % c_name(memb.name)
-        ret += '%s %s' % (memb.type.c_type(is_param=True), c_name(memb.name))
+            ret += 'bool has_' + memb.c_name() + sep
+        ret += '%s %s' % (memb.type.c_type(is_param=True), memb.c_name())
     if extra:
         ret += sep + extra
     return ret
@@ -1602,13 +1602,13 @@ def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
             ret += mcgen('''
     visit_optional(v, &%(prefix)shas_%(c_name)s, "%(name)s", %(errp)s);
 ''',
-                         prefix=prefix, c_name=c_name(memb.name),
+                         prefix=prefix, c_name=memb.c_name(),
                          name=memb.name, errp=errparg)
             ret += gen_err_check(skiperr=skiperr)
             ret += mcgen('''
     if (%(prefix)shas_%(c_name)s) {
 ''',
-                         prefix=prefix, c_name=c_name(memb.name))
+                         prefix=prefix, c_name=memb.c_name())
             push_indent()

         # Ugly: sometimes we need to cast away const
@@ -1621,7 +1621,7 @@ def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False):
     visit_type_%(c_type)s(v, %(cast)s&%(prefix)s%(c_name)s, "%(name)s", %(errp)s);
 ''',
                      c_type=memb.type.c_name(), prefix=prefix, cast=cast,
-                     c_name=c_name(memb.name), name=memb.name,
+                     c_name=memb.c_name(), name=memb.name,
                      errp=errparg)
         ret += gen_err_check(skiperr=skiperr)

-- 
2.4.3

Re: [Qemu-devel] [PATCH v7 07/14] qapi: Move union tag quirks into subclass

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Right now, simple unions have a quirk of using 'kind' in the C
> struct to match the QMP wire name 'type'.  This has resulted in
> messy clients each doing special cases.  While we plan to
> eventually rename things to match, it is better in the meantime
> to consolidate the quirks into a special subclass, by adding a
> new member.c_name() function.  This will also make it easier
> for reworking how alternate types are laid out in a future
> patch.  Use the new c_name() function where possible.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v7: new patch, but borrows idea of subclass from v6 10/12, as
> well as c_name() from 7/12
> ---
>  scripts/qapi-commands.py |  8 ++++----
>  scripts/qapi-types.py    | 12 +++++-------
>  scripts/qapi-visit.py    | 17 +++++------------
>  scripts/qapi.py          | 15 +++++++++++++--
>  4 files changed, 27 insertions(+), 25 deletions(-)

My immediate reaction to the subclass idea was "instead of encapsulating
the flaw more nicely, why not fix it?"  So gave that a try, see my other
reply.

That said, the diffstat shows the subclass idea doesn't take much code.
May make sense if we feel we shouldn't fix the flaw now.

> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 43a893b..ff52ca9 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -32,8 +32,8 @@ def gen_call(name, arg_type, ret_type):
>      if arg_type:
>          for memb in arg_type.members:
>              if memb.optional:
> -                argstr += 'has_%s, ' % c_name(memb.name)
> -            argstr += '%s, ' % c_name(memb.name)
> +                argstr += 'has_%s, ' % memb.c_name()
> +            argstr += '%s, ' % memb.c_name()
>
>      lhs = ''
>      if ret_type:

Trap for the unwary: most of the time, c_name(FOO) is just fine.  Except
when FOO is T.name, where T is a simple union's implicit tag member.

> @@ -77,11 +77,11 @@ def gen_marshal_vars(arg_type, ret_type):
>                  ret += mcgen('''
>      bool has_%(c_name)s = false;
>  ''',
> -                             c_name=c_name(memb.name))
> +                             c_name=memb.c_name())
>              ret += mcgen('''
>      %(c_type)s %(c_name)s = %(c_null)s;
>  ''',
> -                         c_name=c_name(memb.name),
> +                         c_name=memb.c_name(),
>                           c_type=memb.type.c_type(),
>                           c_null=memb.type.c_null())
>          ret += '\n'
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 227ea5c..34ea318 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -136,9 +136,10 @@ struct %(c_name)s {
>  ''')
>      else:
>          ret += mcgen('''
> -    %(c_type)s kind;
> +    %(c_type)s %(c_name)s;
>  ''',
> -                     c_type=c_name(variants.tag_member.type.name))
> +                     c_type=variants.tag_member.type.c_name(),
> +                     c_name=variants.tag_member.c_name())

My patch does

        ret += gen_struct_field(variants.tag_member.name,
                                variants.tag_member.type,
                                False);

>
>      # FIXME: What purpose does data serve, besides preventing a union that
>      # has a branch named 'data'? We use it in qapi-visit.py to decide
> @@ -152,10 +153,7 @@ struct %(c_name)s {
>      union { /* union tag is @%(c_name)s */
>          void *data;
>  ''',
> -                 # TODO ugly special case for simple union
> -                 # Use same tag name in C as on the wire to get rid of
> -                 # it, then: c_name=c_name(variants.tag_member.name)
> -                 c_name=c_name(variants.tag_name or 'kind'))
> +                 c_name=variants.tag_member.c_name())
>
>      for var in variants.variants:
>          # Ugly special case for simple union TODO get rid of it
> @@ -164,7 +162,7 @@ struct %(c_name)s {
>          %(c_type)s %(c_name)s;
>  ''',
>                       c_type=typ.c_type(),
> -                     c_name=c_name(var.name))
> +                     c_name=var.c_name())
>
>      ret += mcgen('''
>      };
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 56b8390..3f74302 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -196,7 +196,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
>                       case=c_enum_const(variants.tag_member.type.name,
>                                         var.name),
>                       c_type=var.type.c_name(),
> -                     c_name=c_name(var.name))
> +                     c_name=var.c_name())
>
>      ret += mcgen('''
>      default:
> @@ -249,10 +249,6 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
>                       c_name=c_name(name))
>          ret += gen_err_check(label='out_obj')
>
> -    tag_key = variants.tag_member.name
> -    if not variants.tag_name:
> -        # we pointlessly use a different key for simple unions
> -        tag_key = 'type'
>      ret += mcgen('''
>      visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
>      if (err) {
> @@ -264,11 +260,8 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
>      switch ((*obj)->%(c_name)s) {
>  ''',
>                   c_type=variants.tag_member.type.c_name(),
> -                 # TODO ugly special case for simple union
> -                 # Use same tag name in C as on the wire to get rid of
> -                 # it, then: c_name=c_name(variants.tag_member.name)
> -                 c_name=c_name(variants.tag_name or 'kind'),
> -                 name=tag_key)
> +                 c_name=variants.tag_member.c_name(),
> +                 name=variants.tag_member.name)
>
>      for var in variants.variants:
>          # TODO ugly special case for simple union
> @@ -283,13 +276,13 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
>          visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "data", &err);
>  ''',
>                           c_type=simple_union_type.c_name(),
> -                         c_name=c_name(var.name))
> +                         c_name=var.c_name())
>          else:
>              ret += mcgen('''
>          visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
>  ''',
>                           c_type=var.type.c_name(),
> -                         c_name=c_name(var.name))
> +                         c_name=var.c_name())
>          ret += mcgen('''
>          break;
>  ''')
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index eaa43b8..f5040da 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -984,7 +984,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>              members = []
>          seen = {}
>          for m in members:
> -            assert c_name(m.name) not in seen
> +            assert m.c_name() not in seen
>              seen[m.name] = m
>          for m in self.local_members:
>              m.check(schema, members, seen)
> @@ -1031,6 +1031,17 @@ class QAPISchemaObjectTypeMember(object):
>          all_members.append(self)
>          seen[self.name] = self
>
> +    def c_name(self):
> +        return c_name(self.name)
> +
> +
> +# TODO Drop this class once we no longer have the 'type'/'kind' mismatch
> +class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
> +    def c_name(self):
> +        assert self.name == 'type'
> +        assert self.type.is_implicit(QAPISchemaEnumType)
> +        return 'kind'
> +
>
>  class QAPISchemaObjectTypeVariants(object):
>      def __init__(self, tag_name, tag_member, variants):
> @@ -1254,7 +1265,7 @@ class QAPISchema(object):
>      def _make_implicit_tag(self, type_name, variants):
>          typ = self._make_implicit_enum_type(type_name,
>                                              [v.name for v in variants])
> -        return QAPISchemaObjectTypeMember('type', typ, False)
> +        return QAPISchemaObjectTypeUnionTag('type', typ, False)
>
>      def _def_union_type(self, expr, info):
>          name = expr['union']

Re: [Qemu-devel] [PATCH v7 07/14] qapi: Move union tag quirks into subclass

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2008 bytes --]

On 10/08/2015 06:25 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Right now, simple unions have a quirk of using 'kind' in the C
>> struct to match the QMP wire name 'type'.  This has resulted in
>> messy clients each doing special cases.  While we plan to
>> eventually rename things to match, it is better in the meantime
>> to consolidate the quirks into a special subclass, by adding a
>> new member.c_name() function.  This will also make it easier
>> for reworking how alternate types are laid out in a future
>> patch.  Use the new c_name() function where possible.
>>
>> No change to generated code.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> ---
>> v7: new patch, but borrows idea of subclass from v6 10/12, as
>> well as c_name() from 7/12
>> ---
>>  scripts/qapi-commands.py |  8 ++++----
>>  scripts/qapi-types.py    | 12 +++++-------
>>  scripts/qapi-visit.py    | 17 +++++------------
>>  scripts/qapi.py          | 15 +++++++++++++--
>>  4 files changed, 27 insertions(+), 25 deletions(-)
> 
> My immediate reaction to the subclass idea was "instead of encapsulating
> the flaw more nicely, why not fix it?"  So gave that a try, see my other
> reply.

I had already done the same sort of fix, but it was just sitting later
in my series where you hadn't reached reviewing yet.

> 
> That said, the diffstat shows the subclass idea doesn't take much code.
> May make sense if we feel we shouldn't fix the flaw now.

I also like the subclass idea because it makes simplifying alternates
easier (see my just-posted subset C).

But it sounds like getting rid of the 'type'/'kind' mismatch sooner
rather than later seems like the direction we should be heading.

If I need to spin a v8 of this series, I'll certainly include that
conversion (whether from mine, yours, or a combination of the two).

-- 
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 --]

[Qemu-devel] [RFC PATCH] qapi: Rename simple union's generated tag member to type

[Markus Armbruster]

Struct and union type members are generally named alike in QMP and C,
except for a simple union's implicit tag member, which is "type" in
QMP, and "kind" in C.  Can't change QMP, so rename it in C.

Since the implicit enumeration type is still called *Kind, this
doesn't clean up the type vs. kind mess completely.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/nbd.c                     |  6 +++---
 block/qcow2.c                   |  2 +-
 block/vmdk.c                    |  2 +-
 blockdev.c                      | 17 +++++++++--------
 hmp.c                           | 12 ++++++------
 hw/char/escc.c                  |  2 +-
 hw/input/hid.c                  |  2 +-
 hw/input/ps2.c                  |  2 +-
 hw/input/virtio-input-hid.c     |  2 +-
 hw/mem/pc-dimm.c                |  2 +-
 net/dump.c                      |  2 +-
 net/hub.c                       |  2 +-
 net/l2tpv3.c                    |  2 +-
 net/net.c                       | 20 ++++++++++----------
 net/slirp.c                     |  2 +-
 net/socket.c                    |  2 +-
 net/tap-win32.c                 |  2 +-
 net/tap.c                       |  4 ++--
 net/vde.c                       |  2 +-
 net/vhost-user.c                |  2 +-
 numa.c                          |  4 ++--
 qemu-char.c                     | 24 ++++++++++++------------
 qemu-nbd.c                      |  4 ++--
 scripts/qapi-types.py           | 12 ++++--------
 scripts/qapi-visit.py           | 15 ++++-----------
 tests/test-qmp-commands.c       |  2 +-
 tests/test-qmp-input-visitor.c  | 30 +++++++++++++++---------------
 tests/test-qmp-output-visitor.c |  8 ++++----
 tpm.c                           |  2 +-
 ui/input-keymap.c               | 10 +++++-----
 ui/input-legacy.c               |  2 +-
 ui/input.c                      | 22 +++++++++++-----------
 util/qemu-sockets.c             | 12 ++++++------
 33 files changed, 113 insertions(+), 123 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index c2a87e9..207a83e 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -206,12 +206,12 @@ 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->type = SOCKET_ADDRESS_KIND_UNIX;
         saddr->q_unix = g_new0(UnixSocketAddress, 1);
         saddr->q_unix->path = g_strdup(qdict_get_str(options, "path"));
         qdict_del(options, "path");
     } else {
-        saddr->kind = SOCKET_ADDRESS_KIND_INET;
+        saddr->type = SOCKET_ADDRESS_KIND_INET;
         saddr->inet = g_new0(InetSocketAddress, 1);
         saddr->inet->host = g_strdup(qdict_get_str(options, "host"));
         if (!qdict_get_try_str(options, "port")) {
@@ -223,7 +223,7 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char **export,
         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 56ad808..28aa74d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2736,7 +2736,7 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
     ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
 
     *spec_info = (ImageInfoSpecific){
-        .kind  = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
+        .type  = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
         {
             .qcow2 = g_new(ImageInfoSpecificQCow2, 1),
         },
diff --git a/block/vmdk.c b/block/vmdk.c
index be0d640..695780c 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2156,7 +2156,7 @@ 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),
         },
diff --git a/blockdev.c b/blockdev.c
index 32b04b4..f0479f7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1052,12 +1052,13 @@ 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 kind, void *data,
+                               Error **errp)
 {
     TransactionAction action;
     TransactionActionList list;
 
-    action.kind = kind;
+    action.type = kind;
     action.data = data;
     list.value = &action;
     list.next = NULL;
@@ -1297,7 +1298,7 @@ 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;
     state = DO_UPCAST(InternalSnapshotState, common, common);
@@ -1439,7 +1440,7 @@ 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;
@@ -1579,7 +1580,7 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
     DriveBackup *backup;
     Error *local_err = NULL;
 
-    assert(common->action->kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
+    assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
     backup = common->action->drive_backup;
 
     blk = blk_by_name(backup->device);
@@ -1647,7 +1648,7 @@ static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
     BlockBackend *blk;
     Error *local_err = NULL;
 
-    assert(common->action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
+    assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
     backup = common->action->blockdev_backup;
 
     blk = blk_by_name(backup->device);
@@ -1774,9 +1775,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);
diff --git a/hmp.c b/hmp.c
index 5048eee..1e51a2c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -841,9 +841,9 @@ 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;
             monitor_printf(mon, "%s%s%s%s",
@@ -1735,14 +1735,14 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
             if (*endp != '\0') {
                 goto err_out;
             }
-            keylist->value->kind = KEY_VALUE_KIND_NUMBER;
+            keylist->value->type = KEY_VALUE_KIND_NUMBER;
             keylist->value->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->type = KEY_VALUE_KIND_QCODE;
             keylist->value->qcode = idx;
         }
 
@@ -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;
 
                 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/char/escc.c b/hw/char/escc.c
index ba653ef..2e5b5eb 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -842,7 +842,7 @@ static void sunkbd_handle_event(DeviceState *dev, QemuConsole *src,
     ChannelState *s = (ChannelState *)dev;
     int qcode, keycode;
 
-    assert(evt->kind == INPUT_EVENT_KIND_KEY);
+    assert(evt->type == INPUT_EVENT_KIND_KEY);
     qcode = qemu_input_key_value_to_qcode(evt->key->key);
     trace_escc_sunkbd_event_in(qcode, QKeyCode_lookup[qcode],
                                evt->key->down);
diff --git a/hw/input/hid.c b/hw/input/hid.c
index 21ebd9e..ac02f88 100644
--- a/hw/input/hid.c
+++ b/hw/input/hid.c
@@ -119,7 +119,7 @@ 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;
diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index fdbe565..58decf2 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -393,7 +393,7 @@ 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;
diff --git a/hw/input/virtio-input-hid.c b/hw/input/virtio-input-hid.c
index 4d85dad..362dad3 100644
--- a/hw/input/virtio-input-hid.c
+++ b/hw/input/virtio-input-hid.c
@@ -191,7 +191,7 @@ 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);
         if (qcode && keymap_qcode[qcode]) {
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 6cc6ac3..7a0b8b3 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -197,7 +197,7 @@ 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;
                 break;
diff --git a/net/dump.c b/net/dump.c
index 08259af..4941299 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -154,7 +154,7 @@ 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);
+    assert(opts->type == NET_CLIENT_OPTIONS_KIND_DUMP);
     dump = opts->dump;
 
     assert(peer);
diff --git a/net/hub.c b/net/hub.c
index 3047f12..2dce788 100644
--- a/net/hub.c
+++ b/net/hub.c
@@ -285,7 +285,7 @@ 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;
 
diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index 4f9bcee..7c6c57f 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -545,7 +545,7 @@ int net_init_l2tpv3(const NetClientOptions *opts,
     s->queue_tail = 0;
     s->header_mismatch = false;
 
-    assert(opts->kind == NET_CLIENT_OPTIONS_KIND_L2TPV3);
+    assert(opts->type == NET_CLIENT_OPTIONS_KIND_L2TPV3);
     l2tpv3 = opts->l2tpv3;
 
     if (l2tpv3->has_ipv6 && l2tpv3->ipv6) {
diff --git a/net/net.c b/net/net.c
index 28a5597..36da3f4 100644
--- a/net/net.c
+++ b/net/net.c
@@ -820,7 +820,7 @@ static int net_init_nic(const NetClientOptions *opts, const char *name,
     NICInfo *nd;
     const NetLegacyNicOptions *nic;
 
-    assert(opts->kind == NET_CLIENT_OPTIONS_KIND_NIC);
+    assert(opts->type == NET_CLIENT_OPTIONS_KIND_NIC);
     nic = opts->nic;
 
     idx = nic_get_free_idx();
@@ -922,9 +922,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;
@@ -935,16 +935,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)");
@@ -952,17 +952,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 ||
+        if (opts->type != NET_CLIENT_OPTIONS_KIND_NIC ||
             !opts->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..cd06a30 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -746,7 +746,7 @@ int net_init_slirp(const NetClientOptions *opts, const char *name,
     const NetdevUserOptions *user;
     const char **dnssearch;
 
-    assert(opts->kind == NET_CLIENT_OPTIONS_KIND_USER);
+    assert(opts->type == NET_CLIENT_OPTIONS_KIND_USER);
     user = opts->user;
 
     vnet = user->has_net ? g_strdup(user->net) :
diff --git a/net/socket.c b/net/socket.c
index b1e3b1c..faf2823 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -706,7 +706,7 @@ int net_init_socket(const NetClientOptions *opts, const char *name,
     Error *err = NULL;
     const NetdevSocketOptions *sock;
 
-    assert(opts->kind == NET_CLIENT_OPTIONS_KIND_SOCKET);
+    assert(opts->type == NET_CLIENT_OPTIONS_KIND_SOCKET);
     sock = opts->socket;
 
     if (sock->has_fd + sock->has_listen + sock->has_connect + sock->has_mcast +
diff --git a/net/tap-win32.c b/net/tap-win32.c
index 625d53c..997b314 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -767,7 +767,7 @@ 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);
+    assert(opts->type == NET_CLIENT_OPTIONS_KIND_TAP);
     tap = opts->tap;
 
     if (!tap->has_ifname) {
diff --git a/net/tap.c b/net/tap.c
index bd01590..3cf540b 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -565,7 +565,7 @@ int net_init_bridge(const NetClientOptions *opts, const char *name,
     TAPState *s;
     int fd, vnet_hdr;
 
-    assert(opts->kind == NET_CLIENT_OPTIONS_KIND_BRIDGE);
+    assert(opts->type == NET_CLIENT_OPTIONS_KIND_BRIDGE);
     bridge = opts->bridge;
 
     helper = bridge->has_helper ? bridge->helper : DEFAULT_BRIDGE_HELPER;
@@ -728,7 +728,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
     const char *vhostfdname;
     char ifname[128];
 
-    assert(opts->kind == NET_CLIENT_OPTIONS_KIND_TAP);
+    assert(opts->type == NET_CLIENT_OPTIONS_KIND_TAP);
     tap = opts->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..96c939f 100644
--- a/net/vde.c
+++ b/net/vde.c
@@ -115,7 +115,7 @@ 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);
+    assert(opts->type == NET_CLIENT_OPTIONS_KIND_VDE);
     vde = opts->vde;
 
     /* missing optional values have been initialized to "all bits zero" */
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 8f354eb..756bcb9 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -273,7 +273,7 @@ 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);
+    assert(opts->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
     vhost_user_opts = opts->vhost_user;
 
     chr = net_vhost_parse_chardev(vhost_user_opts, errp);
diff --git a/numa.c b/numa.c
index e9b18f5..16a8c41 100644
--- a/numa.c
+++ b/numa.c
@@ -226,7 +226,7 @@ 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);
         if (err) {
@@ -487,7 +487,7 @@ 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;
                 break;
diff --git a/qemu-char.c b/qemu-char.c
index 653ea10..a0379f2 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -122,7 +122,7 @@ 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,
@@ -3567,11 +3567,11 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
 
     addr = g_new0(SocketAddress, 1);
     if (path) {
-        addr->kind = SOCKET_ADDRESS_KIND_UNIX;
+        addr->type = SOCKET_ADDRESS_KIND_UNIX;
         addr->q_unix = g_new0(UnixSocketAddress, 1);
         addr->q_unix->path = g_strdup(path);
     } else {
-        addr->kind = SOCKET_ADDRESS_KIND_INET;
+        addr->type = SOCKET_ADDRESS_KIND_INET;
         addr->inet = g_new0(InetSocketAddress, 1);
         addr->inet->host = g_strdup(host);
         addr->inet->port = g_strdup(port);
@@ -3616,7 +3616,7 @@ static void qemu_chr_parse_udp(QemuOpts *opts, ChardevBackend *backend,
     backend->udp = g_new0(ChardevUdp, 1);
 
     addr = g_new0(SocketAddress, 1);
-    addr->kind = SOCKET_ADDRESS_KIND_INET;
+    addr->type = SOCKET_ADDRESS_KIND_INET;
     addr->inet = g_new0(InetSocketAddress, 1);
     addr->inet->host = g_strdup(host);
     addr->inet->port = g_strdup(port);
@@ -3629,7 +3629,7 @@ static void qemu_chr_parse_udp(QemuOpts *opts, ChardevBackend *backend,
     if (has_local) {
         backend->udp->has_local = true;
         addr = g_new0(SocketAddress, 1);
-        addr->kind = SOCKET_ADDRESS_KIND_INET;
+        addr->type = SOCKET_ADDRESS_KIND_INET;
         addr->inet = g_new0(InetSocketAddress, 1);
         addr->inet->host = g_strdup(localaddr);
         addr->inet->port = g_strdup(localport);
@@ -3701,7 +3701,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) {
@@ -3719,7 +3719,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
         qapi_free_ChardevReturn(ret);
         backend = g_new0(ChardevBackend, 1);
         backend->mux = g_new0(ChardevMux, 1);
-        backend->kind = CHARDEV_BACKEND_KIND_MUX;
+        backend->type = CHARDEV_BACKEND_KIND_MUX;
         backend->mux->chardev = g_strdup(bid);
         ret = qmp_chardev_add(id, backend, errp);
         if (!ret) {
@@ -4148,7 +4148,7 @@ static CharDriverState *qmp_chardev_open_socket(ChardevSocket *sock,
 
     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;
@@ -4222,7 +4222,7 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
         return NULL;
     }
 
-    switch (backend->kind) {
+    switch (backend->type) {
     case CHARDEV_BACKEND_KIND_FILE:
         chr = qmp_chardev_open_file(backend->file, errp);
         break;
@@ -4293,7 +4293,7 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
         chr = qemu_chr_open_ringbuf(backend->ringbuf, errp);
         break;
     default:
-        error_setg(errp, "unknown chardev backend (%d)", backend->kind);
+        error_setg(errp, "unknown chardev backend (%d)", backend->type);
         break;
     }
 
@@ -4309,9 +4309,9 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
     if (chr) {
         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/qemu-nbd.c b/qemu-nbd.c
index 6428c15..ccf846b 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -364,11 +364,11 @@ static SocketAddress *nbd_build_socket_address(const char *sockpath,
 
     saddr = g_new0(SocketAddress, 1);
     if (sockpath) {
-        saddr->kind = SOCKET_ADDRESS_KIND_UNIX;
+        saddr->type = SOCKET_ADDRESS_KIND_UNIX;
         saddr->q_unix = g_new0(UnixSocketAddress, 1);
         saddr->q_unix->path = g_strdup(sockpath);
     } else {
-        saddr->kind = SOCKET_ADDRESS_KIND_INET;
+        saddr->type = SOCKET_ADDRESS_KIND_INET;
         saddr->inet = g_new0(InetSocketAddress, 1);
         saddr->inet->host = g_strdup(bindto);
         if (port) {
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 227ea5c..10d8ac0 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -135,10 +135,9 @@ struct %(c_name)s {
     /* Own members: */
 ''')
     else:
-        ret += mcgen('''
-    %(c_type)s kind;
-''',
-                     c_type=c_name(variants.tag_member.type.name))
+        ret += gen_struct_field(variants.tag_member.name,
+                                variants.tag_member.type,
+                                False);
 
     # FIXME: What purpose does data serve, besides preventing a union that
     # has a branch named 'data'? We use it in qapi-visit.py to decide
@@ -152,10 +151,7 @@ struct %(c_name)s {
     union { /* union tag is @%(c_name)s */
         void *data;
 ''',
-                 # TODO ugly special case for simple union
-                 # Use same tag name in C as on the wire to get rid of
-                 # it, then: c_name=c_name(variants.tag_member.name)
-                 c_name=c_name(variants.tag_name or 'kind'))
+                 c_name=c_name(variants.tag_member.name))
 
     for var in variants.variants:
         # Ugly special case for simple union TODO get rid of it
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 56b8390..092191c 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -179,11 +179,11 @@ 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))
 
@@ -249,10 +249,6 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
                      c_name=c_name(name))
         ret += gen_err_check(label='out_obj')
 
-    tag_key = variants.tag_member.name
-    if not variants.tag_name:
-        # we pointlessly use a different key for simple unions
-        tag_key = 'type'
     ret += mcgen('''
     visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
     if (err) {
@@ -264,11 +260,8 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error
     switch ((*obj)->%(c_name)s) {
 ''',
                  c_type=variants.tag_member.type.c_name(),
-                 # TODO ugly special case for simple union
-                 # Use same tag name in C as on the wire to get rid of
-                 # it, then: c_name=c_name(variants.tag_member.name)
-                 c_name=c_name(variants.tag_name or 'kind'),
-                 name=tag_key)
+                 c_name=c_name(variants.tag_member.name),
+                 name=variants.tag_member.name)
 
     for var in variants.variants:
         # TODO ugly special case for simple union
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 8d5249e..6a67e73 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -64,7 +64,7 @@ __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->type = ORG_QEMU_X_UNION1_KIND___ORG_QEMU_X_BRANCH;
     ret->__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 183a9ec..54163ad 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -372,14 +372,14 @@ 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->type, ==, USER_DEF_ALTERNATE_KIND_I);
     g_assert_cmpint(tmp->i, ==, 42);
     qapi_free_UserDefAlternate(tmp);
     visitor_input_teardown(data, NULL);
 
     v = visitor_input_test_init(data, "'string'");
     visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
-    g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_S);
+    g_assert_cmpint(tmp->type, ==, USER_DEF_ALTERNATE_KIND_S);
     g_assert_cmpstr(tmp->s, ==, "string");
     qapi_free_UserDefAlternate(tmp);
     visitor_input_teardown(data, NULL);
@@ -419,7 +419,7 @@ 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_cmpint(asn->type, == ALT_STR_NUM_KIND_N); */
     /* FIXME g_assert_cmpfloat(asn->n, ==, 42); */
     g_assert(err);
     error_free(err);
@@ -429,28 +429,28 @@ 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_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N);
     g_assert_cmpfloat(ans->n, ==, 42);
     qapi_free_AltNumStr(ans);
     visitor_input_teardown(data, NULL);
 
     v = visitor_input_test_init(data, "42");
     visit_type_AltStrInt(v, &asi, NULL, &error_abort);
-    g_assert_cmpint(asi->kind, ==, ALT_STR_INT_KIND_I);
+    g_assert_cmpint(asi->type, ==, ALT_STR_INT_KIND_I);
     g_assert_cmpint(asi->i, ==, 42);
     qapi_free_AltStrInt(asi);
     visitor_input_teardown(data, NULL);
 
     v = visitor_input_test_init(data, "42");
     visit_type_AltIntNum(v, &ain, NULL, &error_abort);
-    g_assert_cmpint(ain->kind, ==, ALT_INT_NUM_KIND_I);
+    g_assert_cmpint(ain->type, ==, ALT_INT_NUM_KIND_I);
     g_assert_cmpint(ain->i, ==, 42);
     qapi_free_AltIntNum(ain);
     visitor_input_teardown(data, NULL);
 
     v = visitor_input_test_init(data, "42");
     visit_type_AltNumInt(v, &ani, NULL, &error_abort);
-    g_assert_cmpint(ani->kind, ==, ALT_NUM_INT_KIND_I);
+    g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_I);
     g_assert_cmpint(ani->i, ==, 42);
     qapi_free_AltNumInt(ani);
     visitor_input_teardown(data, NULL);
@@ -467,14 +467,14 @@ 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_cmpint(asn->type, ==, ALT_STR_NUM_KIND_N);
     g_assert_cmpfloat(asn->n, ==, 42.5);
     qapi_free_AltStrNum(asn);
     visitor_input_teardown(data, NULL);
 
     v = visitor_input_test_init(data, "42.5");
     visit_type_AltNumStr(v, &ans, NULL, &error_abort);
-    g_assert_cmpint(ans->kind, ==, ALT_NUM_STR_KIND_N);
+    g_assert_cmpint(ans->type, ==, ALT_NUM_STR_KIND_N);
     g_assert_cmpfloat(ans->n, ==, 42.5);
     qapi_free_AltNumStr(ans);
     visitor_input_teardown(data, NULL);
@@ -489,14 +489,14 @@ 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_cmpint(ain->type, ==, ALT_INT_NUM_KIND_N);
     g_assert_cmpfloat(ain->n, ==, 42.5);
     qapi_free_AltIntNum(ain);
     visitor_input_teardown(data, NULL);
 
     v = visitor_input_test_init(data, "42.5");
     visit_type_AltNumInt(v, &ani, NULL, &error_abort);
-    g_assert_cmpint(ani->kind, ==, ALT_NUM_INT_KIND_N);
+    g_assert_cmpint(ani->type, ==, ALT_NUM_INT_KIND_N);
     g_assert_cmpint(ani->n, ==, 42.5);
     qapi_free_AltNumInt(ani);
     visitor_input_teardown(data, NULL);
@@ -527,7 +527,7 @@ 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: {
@@ -690,7 +690,7 @@ 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++) {
         g_assert_cmpint(elem->value, ==, (i % 3 == 0) ? 1 : 0);
@@ -725,7 +725,7 @@ 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++) {
         gchar str[8];
@@ -764,7 +764,7 @@ 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++) {
         GString *double_expected = g_string_new("");
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index c84002e..bc210b6 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -517,7 +517,7 @@ 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->type = USER_DEF_ALTERNATE_KIND_I;
     tmp->i = 42;
 
     visit_type_UserDefAlternate(data->ov, &tmp, NULL, &err);
@@ -543,7 +543,7 @@ 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;
         for (i = 0; i < 32; i++) {
@@ -764,14 +764,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);
 }
diff --git a/tpm.c b/tpm.c
index 4e9b109..fcab81c 100644
--- a/tpm.c
+++ b/tpm.c
@@ -260,7 +260,7 @@ 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;
         if (drv->path) {
diff --git a/ui/input-keymap.c b/ui/input-keymap.c
index 7635cb0..088523d 100644
--- a/ui/input-keymap.c
+++ b/ui/input-keymap.c
@@ -139,10 +139,10 @@ static int number_to_qcode[0x100];
 
 int qemu_input_key_value_to_number(const KeyValue *value)
 {
-    if (value->kind == KEY_VALUE_KIND_QCODE) {
+    if (value->type == KEY_VALUE_KIND_QCODE) {
         return qcode_to_number[value->qcode];
     } else {
-        assert(value->kind == KEY_VALUE_KIND_NUMBER);
+        assert(value->type == KEY_VALUE_KIND_NUMBER);
         return value->number;
     }
 }
@@ -166,10 +166,10 @@ 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) {
+    if (value->type == KEY_VALUE_KIND_QCODE) {
         return value->qcode;
     } else {
-        assert(value->kind == KEY_VALUE_KIND_NUMBER);
+        assert(value->type == KEY_VALUE_KIND_NUMBER);
         return qemu_input_key_number_to_qcode(value->number);
     }
 }
@@ -180,7 +180,7 @@ 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 &&
+    if (value->type == KEY_VALUE_KIND_QCODE &&
         value->qcode == Q_KEY_CODE_PAUSE) {
         /* specific case */
         int v = down ? 0 : 0x80;
diff --git a/ui/input-legacy.c b/ui/input-legacy.c
index e50f296..6149648 100644
--- a/ui/input-legacy.c
+++ b/ui/input-legacy.c
@@ -150,7 +150,7 @@ 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];
diff --git a/ui/input.c b/ui/input.c
index 1a552d1..fd86571 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;
         }
     }
@@ -197,9 +197,9 @@ 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->key->key->type) {
         case KEY_VALUE_KIND_NUMBER:
             qcode = qemu_input_key_number_to_qcode(evt->key->key->number);
             name = QKeyCode_lookup[qcode];
@@ -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;
     }
@@ -349,7 +349,7 @@ 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->type = INPUT_EVENT_KIND_KEY;
     evt->key->key = key;
     evt->key->down = down;
     return evt;
@@ -372,7 +372,7 @@ 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->type = KEY_VALUE_KIND_NUMBER;
     key->number = num;
     qemu_input_event_send_key(src, key, down);
 }
@@ -380,7 +380,7 @@ void qemu_input_event_send_key_number(QemuConsole *src, int num, bool 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->type = KEY_VALUE_KIND_QCODE;
     key->qcode = q;
     qemu_input_event_send_key(src, key, down);
 }
@@ -399,7 +399,7 @@ 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->type = INPUT_EVENT_KIND_BTN;
     evt->btn->button = btn;
     evt->btn->down = down;
     return evt;
@@ -451,7 +451,7 @@ InputEvent *qemu_input_event_new_move(InputEventKind kind,
     InputEvent *evt = g_new0(InputEvent, 1);
     InputMoveEvent *move = g_new0(InputMoveEvent, 1);
 
-    evt->kind = kind;
+    evt->type = kind;
     evt->data = move;
     move->axis = axis;
     move->value = value;
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 2add83a..277b139 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -904,7 +904,7 @@ 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->type = SOCKET_ADDRESS_KIND_UNIX;
             addr->q_unix = g_new(UnixSocketAddress, 1);
             addr->q_unix->path = g_strdup(str + 5);
         }
@@ -913,12 +913,12 @@ SocketAddress *socket_parse(const char *str, Error **errp)
             error_setg(errp, "invalid file descriptor address");
             goto fail;
         } else {
-            addr->kind = SOCKET_ADDRESS_KIND_FD;
+            addr->type = SOCKET_ADDRESS_KIND_FD;
             addr->fd = g_new(String, 1);
             addr->fd->str = g_strdup(str + 3);
         }
     } else {
-        addr->kind = SOCKET_ADDRESS_KIND_INET;
+        addr->type = SOCKET_ADDRESS_KIND_INET;
         addr->inet = inet_parse(str, errp);
         if (addr->inet == NULL) {
             goto fail;
@@ -938,7 +938,7 @@ 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);
         fd = inet_connect_opts(opts, errp, callback, opaque);
@@ -970,7 +970,7 @@ 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);
         fd = inet_listen_opts(opts, 0, errp);
@@ -998,7 +998,7 @@ 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);
         if (local) {
-- 
2.4.3

Re: [Qemu-devel] [RFC PATCH] qapi: Rename simple union's generated tag member to type

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]

On 10/08/2015 06:26 AM, Markus Armbruster wrote:
> Struct and union type members are generally named alike in QMP and C,
> except for a simple union's implicit tag member, which is "type" in
> QMP, and "kind" in C.  Can't change QMP, so rename it in C.
> 
> Since the implicit enumeration type is still called *Kind, this
> doesn't clean up the type vs. kind mess completely.

Looks very similar to my v5 31/46

https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05445.html

and still waiting for us to get that far into the reviews.

I guess pulling it earlier into the series is worth doing, if it will
help us avoid some hacks.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> +++ b/scripts/qapi-types.py
> @@ -135,10 +135,9 @@ struct %(c_name)s {
>      /* Own members: */
>  ''')
>      else:
> -        ret += mcgen('''
> -    %(c_type)s kind;
> -''',
> -                     c_type=c_name(variants.tag_member.type.name))
> +        ret += gen_struct_field(variants.tag_member.name,
> +                                variants.tag_member.type,
> +                                False);


Hmm, I hadn't even thought to use that. But it would require tweaks to
work with my pending change to alternates to use 'qtype_code', because
in that patch I intentionally left variants.tag_member.type as None (and
instead used variants.tag_member.c_type() to get the string 'qtype_code'):

https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg01985.html


-- 
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 --]

Re: [Qemu-devel] [RFC PATCH] qapi: Rename simple union's generated tag member to type

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3628 bytes --]

On 10/08/2015 06:26 AM, Markus Armbruster wrote:
> Struct and union type members are generally named alike in QMP and C,
> except for a simple union's implicit tag member, which is "type" in
> QMP, and "kind" in C.  Can't change QMP, so rename it in C.
> 
> Since the implicit enumeration type is still called *Kind, this
> doesn't clean up the type vs. kind mess completely.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
...
>  scripts/qapi-types.py           | 12 ++++--------
>  scripts/qapi-visit.py           | 15 ++++-----------
>  tests/test-qmp-commands.c       |  2 +-
>  tests/test-qmp-input-visitor.c  | 30 +++++++++++++++---------------
>  tests/test-qmp-output-visitor.c |  8 ++++----
>  tpm.c                           |  2 +-
>  ui/input-keymap.c               | 10 +++++-----
>  ui/input-legacy.c               |  2 +-
>  ui/input.c                      | 22 +++++++++++-----------
>  util/qemu-sockets.c             | 12 ++++++------
>  33 files changed, 113 insertions(+), 123 deletions(-)

This touches a lot of files all in one commit (both your RFC and my v5
version), and then I get to touch the same files all over again when I
swap to a named rather than anonymous union in the C struct.  So here's
what I'm currently playing with:

first patch: hack _just_ scripts/qapi*py to turn:

{ 'union':'Foo', 'data':{'a':'int','b':'bool'}}

into:

struct Foo {
    union {
        FooKind type;
        FooKind kind; /* temporary hack */
    };
    union { /* union tag is @type */
        void *data; /* will go away much later in series */
        int64_t a;
        bool b;
        union {
            void *data;
            int64_t a;
            int b;
        } u;
    };
};

so that old code accessing foo->kind and foo->a just works, but also
leaving the door open to access foo->type and foo->u.a.  Then a series
of patches grouped by logical file sets (so no one patch is too huge to
review, and spreading the load among maintainers), and a final patch to
scripts/qapi*.py to get to the desired:

struct Foo {
    FooKind type;
    union { /* union tag is @type */
        void *data; /* will go away much later in series */
        int64_t a;
        bool b;
    } u;
};

at which point we've gotten rid of any collisions between tag value
(branch names) and QMP names, at the possible expense of a new collision
with 'u'.  I'm also beefing up the testsuite and check_name() function
(or maybe somewhere else, still haven't actually written that part of my
planned series) to reject 'u' and anything spelled 'has_*' as member
names, to proactively avoid the need to worry about collisions with 'u'
or the added members for optional fields.  I'll probably also reject
'*List' as a user-supplied type name, addressing the comment just barely
added in current qapi-next that we don't have a reserved namespace for
arrays.

The collision with 'data' is harder; I can't remove it until we delete
visit_start_union()/visit_end_union() which starts to get in the mess of
patches that work with qapi visitor interfaces, so that will be in a
later subset.

It may be a day or two before I can post the pending work.  Meanwhile, I
previously posted subset C which is somewhat orthogonal (not sure if it
needs any minor rebasing to apply against current qapi-next), if you
want to dive into reviewing that, instead of waiting:
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg01980.html

-- 
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 --]

Re: [Qemu-devel] [RFC PATCH] qapi: Rename simple union's generated tag member to type

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/08/2015 06:26 AM, Markus Armbruster wrote:
>> Struct and union type members are generally named alike in QMP and C,
>> except for a simple union's implicit tag member, which is "type" in
>> QMP, and "kind" in C.  Can't change QMP, so rename it in C.
>> 
>> Since the implicit enumeration type is still called *Kind, this
>> doesn't clean up the type vs. kind mess completely.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
> ...
>>  scripts/qapi-types.py           | 12 ++++--------
>>  scripts/qapi-visit.py           | 15 ++++-----------
>>  tests/test-qmp-commands.c       |  2 +-
>>  tests/test-qmp-input-visitor.c  | 30 +++++++++++++++---------------
>>  tests/test-qmp-output-visitor.c |  8 ++++----
>>  tpm.c                           |  2 +-
>>  ui/input-keymap.c               | 10 +++++-----
>>  ui/input-legacy.c               |  2 +-
>>  ui/input.c                      | 22 +++++++++++-----------
>>  util/qemu-sockets.c             | 12 ++++++------
>>  33 files changed, 113 insertions(+), 123 deletions(-)
>
> This touches a lot of files all in one commit (both your RFC and my v5
> version), and then I get to touch the same files all over again when I
> swap to a named rather than anonymous union in the C struct.  So here's
> what I'm currently playing with:
>
> first patch: hack _just_ scripts/qapi*py to turn:
>
> { 'union':'Foo', 'data':{'a':'int','b':'bool'}}
>
> into:
>
> struct Foo {
>     union {
>         FooKind type;
>         FooKind kind; /* temporary hack */
>     };
>     union { /* union tag is @type */
>         void *data; /* will go away much later in series */
>         int64_t a;
>         bool b;
>         union {
>             void *data;
>             int64_t a;
>             int b;
>         } u;
>     };
> };

Gross :)

I have to admit the idea occurred to me, too.

> so that old code accessing foo->kind and foo->a just works, but also
> leaving the door open to access foo->type and foo->u.a.  Then a series
> of patches grouped by logical file sets (so no one patch is too huge to
> review, and spreading the load among maintainers), and a final patch to
> scripts/qapi*.py to get to the desired:
>
> struct Foo {
>     FooKind type;
>     union { /* union tag is @type */
>         void *data; /* will go away much later in series */
>         int64_t a;
>         bool b;
>     } u;
> };

The patches for renaming kind to type and for splicing in u. are
mechanical.

If we want to split them up, we need your hack.

We may want to split along maintenance boundaries and route the parts
through the respective maintainers.  Slooow.

We may want to split just for easier review, but still route the whole
set through my tree in one go.

I'm not sure splitting is necessary, however.

> at which point we've gotten rid of any collisions between tag value
> (branch names) and QMP names, at the possible expense of a new collision
> with 'u'.

In other words, we trade C-only collisions between two sets of
user-defined names (branch names / tag values and member names) for
reserving one more fixed name.  Makes sense to me.

>            I'm also beefing up the testsuite and check_name() function
> (or maybe somewhere else, still haven't actually written that part of my
> planned series) to reject 'u' and anything spelled 'has_*' as member
> names, to proactively avoid the need to worry about collisions with 'u'
> or the added members for optional fields.  I'll probably also reject
> '*List' as a user-supplied type name, addressing the comment just barely
> added in current qapi-next that we don't have a reserved namespace for
> arrays.

I think we should consider delaying the work on detecting C collisions
in qapi.py, for a couple of reasons:

* Any collisions qapi.py misses will be detected by the C compiler, thus
  qapi.py missing collisions is just an annoyance.

* Our current plans involve removing some collisions (like branch name
  with member name), and add others (like member name with 'u').

* Doing collision detection late avoids churn, and can hopefully work
  with simpler and more regular code.

If you want to add more tests flagging more of the current
troublemakers, that's okay.  I'd focus on other things, though.  When we
get to collision detection, we'll have to analyze them again anyway.

> The collision with 'data' is harder; I can't remove it until we delete
> visit_start_union()/visit_end_union() which starts to get in the mess of
> patches that work with qapi visitor interfaces, so that will be in a
> later subset.

Delaying removal of 'data' until later feels just fine.

> It may be a day or two before I can post the pending work.  Meanwhile, I
> previously posted subset C which is somewhat orthogonal (not sure if it
> needs any minor rebasing to apply against current qapi-next), if you
> want to dive into reviewing that, instead of waiting:
> https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg01980.html

If I can find time for QAPI before you post again, I'll know where to
find subset C :)

[Qemu-devel] [PATCH v7 08/14] qapi: Track location that created an implicit type

[Eric Blake]

A future patch will move some error checking from the parser
to the various QAPISchema*.check() methods, which run only
after parsing completes.  It will thus be possible to create
a python instance representing an implicit QAPI type that
parses fine but will fail validation during check().  Since
all errors have to have an associated 'info' location, we
need a location to be associated with those implicit types.
The intuitive info to use is the location of the enclosing
entity that caused the creation of the implicit type.

Note that we do not anticipate builtin types being used in
an error message (as they are not part of the user's QAPI
input, the user can't cause a semantic error in their
behavior), so we exempt those types from requiring info, by
setting a flag to track the completion of _def_predefineds(),
and tracking that flag in _def_entity().

No change to the generated code.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v7: better commit message and comments, fix info assertion to
use instance flag rather than ugly leaky abstraction static flag
v6: improve commit message, track implicit enum info, rebase
on new lazy array handling
---
 scripts/qapi.py | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index f5040da..e49f72b 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -795,9 +795,9 @@ class QAPISchemaEntity(object):
         self.name = name
         # For explicitly defined entities, info points to the (explicit)
         # definition.  For builtins (and their arrays), info is None.
-        # TODO For other implicitly defined entities, it should point to
-        # a place that triggers implicit definition; there may be more
-        # than one such place.
+        # For other implicitly defined entities, it points to a place
+        # that triggers implicit definition; there may be more than one
+        # such place.
         self.info = info

     def c_name(self):
@@ -1153,7 +1153,9 @@ class QAPISchema(object):
         try:
             self.exprs = check_exprs(QAPISchemaParser(open(fname, "r")).exprs)
             self._entity_dict = {}
+            self._predefined_done = False
             self._def_predefineds()
+            self._predefined_done = True
             self._def_exprs()
             self.check()
         except (QAPISchemaError, QAPIExprError), err:
@@ -1161,6 +1163,9 @@ class QAPISchema(object):
             exit(1)

     def _def_entity(self, ent):
+        if self._predefined_done:
+            # Only the predefined types are allowed to not have info
+            assert ent.info
         assert ent.name not in self._entity_dict
         self._entity_dict[ent.name] = ent

@@ -1203,9 +1208,9 @@ class QAPISchema(object):
                                                           [], None)
         self._def_entity(self.the_empty_object_type)

-    def _make_implicit_enum_type(self, name, values):
+    def _make_implicit_enum_type(self, name, info, values):
         name = name + 'Kind'
-        self._def_entity(QAPISchemaEnumType(name, None, values, None))
+        self._def_entity(QAPISchemaEnumType(name, info, values, None))
         return name

     def _make_array_type(self, element_type, info):
@@ -1214,12 +1219,12 @@ class QAPISchema(object):
             self._def_entity(QAPISchemaArrayType(name, info, element_type))
         return name

-    def _make_implicit_object_type(self, name, role, members):
+    def _make_implicit_object_type(self, name, info, role, members):
         if not members:
             return None
         name = ':obj-%s-%s' % (name, role)
         if not self.lookup_entity(name, QAPISchemaObjectType):
-            self._def_entity(QAPISchemaObjectType(name, None, None,
+            self._def_entity(QAPISchemaObjectType(name, info, None,
                                                   members, None))
         return name

@@ -1259,11 +1264,11 @@ class QAPISchema(object):
             assert len(typ) == 1
             typ = self._make_array_type(typ[0], info)
         typ = self._make_implicit_object_type(
-            typ, 'wrapper', [self._make_member('data', typ, info)])
+            typ, info, 'wrapper', [self._make_member('data', typ, info)])
         return QAPISchemaObjectTypeVariant(case, typ)

-    def _make_implicit_tag(self, type_name, variants):
-        typ = self._make_implicit_enum_type(type_name,
+    def _make_implicit_tag(self, type_name, info, variants):
+        typ = self._make_implicit_enum_type(type_name, info,
                                             [v.name for v in variants])
         return QAPISchemaObjectTypeUnionTag('type', typ, False)

@@ -1279,7 +1284,7 @@ class QAPISchema(object):
         else:
             variants = [self._make_simple_variant(key, value, info)
                         for (key, value) in data.iteritems()]
-            tag_enum = self._make_implicit_tag(name, variants)
+            tag_enum = self._make_implicit_tag(name, info, variants)
         self._def_entity(
             QAPISchemaObjectType(name, info, base,
                                  self._make_members(OrderedDict(), info),
@@ -1292,7 +1297,7 @@ class QAPISchema(object):
         data = expr['data']
         variants = [self._make_variant(key, value)
                     for (key, value) in data.iteritems()]
-        tag_enum = self._make_implicit_tag(name, variants)
+        tag_enum = self._make_implicit_tag(name, info, variants)
         self._def_entity(
             QAPISchemaAlternateType(name, info,
                                     QAPISchemaObjectTypeVariants(None,
@@ -1307,7 +1312,7 @@ class QAPISchema(object):
         success_response = expr.get('success-response', True)
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
-                name, 'arg', self._make_members(data, info))
+                name, info, 'arg', self._make_members(data, info))
         if isinstance(rets, list):
             assert len(rets) == 1
             rets = self._make_array_type(rets[0], info)
@@ -1319,7 +1324,7 @@ class QAPISchema(object):
         data = expr.get('data')
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
-                name, 'arg', self._make_members(data, info))
+                name, info, 'arg', self._make_members(data, info))
         self._def_entity(QAPISchemaEvent(name, info, data))

     def _def_exprs(self):
-- 
2.4.3

Re: [Qemu-devel] [PATCH v7 08/14] qapi: Track location that created an implicit type

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> A future patch will move some error checking from the parser
> to the various QAPISchema*.check() methods, which run only
> after parsing completes.  It will thus be possible to create
> a python instance representing an implicit QAPI type that
> parses fine but will fail validation during check().  Since
> all errors have to have an associated 'info' location, we
> need a location to be associated with those implicit types.
> The intuitive info to use is the location of the enclosing
> entity that caused the creation of the implicit type.

Would you like to mention you already added info to the implicit array
types a couple of patches ago?

>
> Note that we do not anticipate builtin types being used in
> an error message (as they are not part of the user's QAPI
> input, the user can't cause a semantic error in their
> behavior), so we exempt those types from requiring info, by
> setting a flag to track the completion of _def_predefineds(),
> and tracking that flag in _def_entity().
>
> No change to the generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v7: better commit message and comments, fix info assertion to
> use instance flag rather than ugly leaky abstraction static flag
> v6: improve commit message, track implicit enum info, rebase
> on new lazy array handling
> ---
>  scripts/qapi.py | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index f5040da..e49f72b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -795,9 +795,9 @@ class QAPISchemaEntity(object):
>          self.name = name
>          # For explicitly defined entities, info points to the (explicit)
>          # definition.  For builtins (and their arrays), info is None.
> -        # TODO For other implicitly defined entities, it should point to
> -        # a place that triggers implicit definition; there may be more
> -        # than one such place.
> +        # For other implicitly defined entities, it points to a place
> +        # that triggers implicit definition; there may be more than one
> +        # such place.
>          self.info = info
>
>      def c_name(self):
> @@ -1153,7 +1153,9 @@ class QAPISchema(object):
>          try:
>              self.exprs = check_exprs(QAPISchemaParser(open(fname, "r")).exprs)
>              self._entity_dict = {}
> +            self._predefined_done = False
>              self._def_predefineds()
> +            self._predefined_done = True
>              self._def_exprs()
>              self.check()
>          except (QAPISchemaError, QAPIExprError), err:
> @@ -1161,6 +1163,9 @@ class QAPISchema(object):
>              exit(1)
>
>      def _def_entity(self, ent):
> +        if self._predefined_done:
> +            # Only the predefined types are allowed to not have info
> +            assert ent.info

Or possibly

        assert ent.info or not self._predefined_done

With self._predefined_done replaced by self._predefining, we'd get

        assert ent.info or self._predefining

Pick the bikeshed color you like best :)

>          assert ent.name not in self._entity_dict
>          self._entity_dict[ent.name] = ent
>
> @@ -1203,9 +1208,9 @@ class QAPISchema(object):
>                                                            [], None)
>          self._def_entity(self.the_empty_object_type)
>
> -    def _make_implicit_enum_type(self, name, values):
> +    def _make_implicit_enum_type(self, name, info, values):
>          name = name + 'Kind'
> -        self._def_entity(QAPISchemaEnumType(name, None, values, None))
> +        self._def_entity(QAPISchemaEnumType(name, info, values, None))
>          return name
>
>      def _make_array_type(self, element_type, info):
> @@ -1214,12 +1219,12 @@ class QAPISchema(object):
>              self._def_entity(QAPISchemaArrayType(name, info, element_type))
>          return name
>
> -    def _make_implicit_object_type(self, name, role, members):
> +    def _make_implicit_object_type(self, name, info, role, members):
>          if not members:
>              return None
>          name = ':obj-%s-%s' % (name, role)
>          if not self.lookup_entity(name, QAPISchemaObjectType):
> -            self._def_entity(QAPISchemaObjectType(name, None, None,
> +            self._def_entity(QAPISchemaObjectType(name, info, None,
>                                                    members, None))
>          return name
>
> @@ -1259,11 +1264,11 @@ class QAPISchema(object):
>              assert len(typ) == 1
>              typ = self._make_array_type(typ[0], info)
>          typ = self._make_implicit_object_type(
> -            typ, 'wrapper', [self._make_member('data', typ, info)])
> +            typ, info, 'wrapper', [self._make_member('data', typ, info)])
>          return QAPISchemaObjectTypeVariant(case, typ)
>
> -    def _make_implicit_tag(self, type_name, variants):
> -        typ = self._make_implicit_enum_type(type_name,
> +    def _make_implicit_tag(self, type_name, info, variants):
> +        typ = self._make_implicit_enum_type(type_name, info,
>                                              [v.name for v in variants])
>          return QAPISchemaObjectTypeUnionTag('type', typ, False)
>
> @@ -1279,7 +1284,7 @@ class QAPISchema(object):
>          else:
>              variants = [self._make_simple_variant(key, value, info)
>                          for (key, value) in data.iteritems()]
> -            tag_enum = self._make_implicit_tag(name, variants)
> +            tag_enum = self._make_implicit_tag(name, info, variants)
>          self._def_entity(
>              QAPISchemaObjectType(name, info, base,
>                                   self._make_members(OrderedDict(), info),
> @@ -1292,7 +1297,7 @@ class QAPISchema(object):
>          data = expr['data']
>          variants = [self._make_variant(key, value)
>                      for (key, value) in data.iteritems()]
> -        tag_enum = self._make_implicit_tag(name, variants)
> +        tag_enum = self._make_implicit_tag(name, info, variants)
>          self._def_entity(
>              QAPISchemaAlternateType(name, info,
>                                      QAPISchemaObjectTypeVariants(None,
> @@ -1307,7 +1312,7 @@ class QAPISchema(object):
>          success_response = expr.get('success-response', True)
>          if isinstance(data, OrderedDict):
>              data = self._make_implicit_object_type(
> -                name, 'arg', self._make_members(data, info))
> +                name, info, 'arg', self._make_members(data, info))
>          if isinstance(rets, list):
>              assert len(rets) == 1
>              rets = self._make_array_type(rets[0], info)
> @@ -1319,7 +1324,7 @@ class QAPISchema(object):
>          data = expr.get('data')
>          if isinstance(data, OrderedDict):
>              data = self._make_implicit_object_type(
> -                name, 'arg', self._make_members(data, info))
> +                name, info, 'arg', self._make_members(data, info))
>          self._def_entity(QAPISchemaEvent(name, info, data))
>
>      def _def_exprs(self):

Tedious plumbing work, mostly :)

[Qemu-devel] [PATCH v7 09/14] qapi: Track owner of each object member

[Eric Blake]

Future commits will migrate semantic checking away from parsing
and over to the various QAPISchema*.check() methods.  But to
report an error message about an incorrect semantic use of a
member of an object type, it helps to know which type, command,
or event owns the member.  Rather than making all the check()
methods have to pass around additional information, it is easier
to have each member track the name of the type that owns it in
the first place.  The new member.owner field is set when
registering the members and variants arrays with an object or
variant type.  We track only a name, and not the actual type
object, to avoid creating a circular python reference chain.

The source information is intended for human consumption in
error messages, and a new describe() method is added to access
the resulting information.  For example, given the qapi:
 { 'command': 'foo', 'data': { 'string': 'str' } }
an implementation of visit_command() that calls
 arg_type.members[0].describe()
will see "'string' (member of foo arguments)".

To make the human-readable name of implicit types work without
duplicating efforts, the name of implicit types is tweaked
after the ':obj-' prefix, so that we can just trim off the
prefix.  This required updates to the testsuite.

No change to generated code.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v7: total rewrite: rework implicit object names, assign owner
when initializing owner type rather than when creating member
python object
v6: rebase on new lazy array creation and simple union 'type'
motion; tweak commit message
---
 scripts/qapi.py                         | 41 ++++++++++++---
 tests/qapi-schema/args-member-array.out |  4 +-
 tests/qapi-schema/args-name-clash.out   |  4 +-
 tests/qapi-schema/ident-with-escape.out |  4 +-
 tests/qapi-schema/qapi-schema-test.out  | 88 ++++++++++++++++-----------------
 tests/qapi-schema/union-clash-data.out  |  4 +-
 6 files changed, 87 insertions(+), 58 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index e49f72b..11ffc49 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -961,8 +961,16 @@ class QAPISchemaObjectType(QAPISchemaType):
         assert base is None or isinstance(base, str)
         for m in local_members:
             assert isinstance(m, QAPISchemaObjectTypeMember)
-        assert (variants is None or
-                isinstance(variants, QAPISchemaObjectTypeVariants))
+            assert not m.owner
+            m.owner = name
+        if variants is not None:
+            assert isinstance(variants, QAPISchemaObjectTypeVariants)
+            if variants.tag_member:
+                assert not variants.tag_member.owner
+                variants.tag_member.owner = name
+            for v in variants.variants:
+                assert not v.owner
+                v.owner = name
         self._base_name = base
         self.base = None
         self.local_members = local_members
@@ -1023,8 +1031,10 @@ class QAPISchemaObjectTypeMember(object):
         self._type_name = typ
         self.type = None
         self.optional = optional
+        self.owner = None   # will be filled by owner's init

     def check(self, schema, all_members, seen):
+        assert self.owner
         assert self.name not in seen
         self.type = schema.lookup_type(self._type_name)
         assert self.type
@@ -1034,6 +1044,15 @@ class QAPISchemaObjectTypeMember(object):
     def c_name(self):
         return c_name(self.name)

+    def describe(self):
+        source = self.owner
+        if source.startswith(':obj-'):
+            source = source[5:]
+        return "'%s' (%s of %s)" % (self.name, self._describe(), source)
+
+    def _describe(self):
+        return 'member'
+

 # TODO Drop this class once we no longer have the 'type'/'kind' mismatch
 class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
@@ -1042,6 +1061,9 @@ class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
         assert self.type.is_implicit(QAPISchemaEnumType)
         return 'kind'

+    def describe(self):
+        return "'kind' (implicit tag of %s)" % self.owner
+

 class QAPISchemaObjectTypeVariants(object):
     def __init__(self, tag_name, tag_member, variants):
@@ -1086,12 +1108,19 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
             return self.type.members[0].type
         return None

+    def _describe(self):
+        return 'branch'
+

 class QAPISchemaAlternateType(QAPISchemaType):
     def __init__(self, name, info, variants):
         QAPISchemaType.__init__(self, name, info)
         assert isinstance(variants, QAPISchemaObjectTypeVariants)
-        assert not variants.tag_name
+        assert variants.tag_member and not variants.tag_member.owner
+        variants.tag_member.owner = name
+        for v in variants.variants:
+            assert not v.owner
+            v.owner = name
         self.variants = variants

     def check(self, schema):
@@ -1222,7 +1251,7 @@ class QAPISchema(object):
     def _make_implicit_object_type(self, name, info, role, members):
         if not members:
             return None
-        name = ':obj-%s-%s' % (name, role)
+        name = ':obj-%s %s' % (name, role)
         if not self.lookup_entity(name, QAPISchemaObjectType):
             self._def_entity(QAPISchemaObjectType(name, info, None,
                                                   members, None))
@@ -1312,7 +1341,7 @@ class QAPISchema(object):
         success_response = expr.get('success-response', True)
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
-                name, info, 'arg', self._make_members(data, info))
+                name, info, 'arguments', self._make_members(data, info))
         if isinstance(rets, list):
             assert len(rets) == 1
             rets = self._make_array_type(rets[0], info)
@@ -1324,7 +1353,7 @@ class QAPISchema(object):
         data = expr.get('data')
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
-                name, info, 'arg', self._make_members(data, info))
+                name, info, 'data', self._make_members(data, info))
         self._def_entity(QAPISchemaEvent(name, info, data))

     def _def_exprs(self):
diff --git a/tests/qapi-schema/args-member-array.out b/tests/qapi-schema/args-member-array.out
index b3b92df..c822309 100644
--- a/tests/qapi-schema/args-member-array.out
+++ b/tests/qapi-schema/args-member-array.out
@@ -1,9 +1,9 @@
 object :empty
-object :obj-okay-arg
+object :obj-okay arguments
     member member1: intList optional=False
     member member2: defList optional=False
 enum abc ['a', 'b', 'c']
 object def
     member array: abcList optional=False
-command okay :obj-okay-arg -> None
+command okay :obj-okay arguments -> None
    gen=True success_response=True
diff --git a/tests/qapi-schema/args-name-clash.out b/tests/qapi-schema/args-name-clash.out
index 9b2f6e4..0e986b6 100644
--- a/tests/qapi-schema/args-name-clash.out
+++ b/tests/qapi-schema/args-name-clash.out
@@ -1,6 +1,6 @@
 object :empty
-object :obj-oops-arg
+object :obj-oops arguments
     member a-b: str optional=False
     member a_b: str optional=False
-command oops :obj-oops-arg -> None
+command oops :obj-oops arguments -> None
    gen=True success_response=True
diff --git a/tests/qapi-schema/ident-with-escape.out b/tests/qapi-schema/ident-with-escape.out
index f4542b1..4544777 100644
--- a/tests/qapi-schema/ident-with-escape.out
+++ b/tests/qapi-schema/ident-with-escape.out
@@ -1,5 +1,5 @@
 object :empty
-object :obj-fooA-arg
+object :obj-fooA arguments
     member bar1: str optional=False
-command fooA :obj-fooA-arg -> None
+command fooA :obj-fooA arguments -> None
    gen=True success_response=True
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 2a8c82e..c666481 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -1,56 +1,56 @@
 object :empty
-object :obj-EVENT_C-arg
+object :obj-EVENT_C data
     member a: int optional=True
     member b: UserDefOne optional=True
     member c: str optional=False
-object :obj-EVENT_D-arg
+object :obj-EVENT_D data
     member a: EventStructOne optional=False
     member b: str optional=False
     member c: str optional=True
     member enum3: EnumOne optional=True
-object :obj-__org.qemu_x-command-arg
+object :obj-__org.qemu_x-command arguments
     member a: __org.qemu_x-EnumList optional=False
     member b: __org.qemu_x-StructList optional=False
     member c: __org.qemu_x-Union2 optional=False
     member d: __org.qemu_x-Alt optional=False
-object :obj-anyList-wrapper
+object :obj-anyList wrapper
     member data: anyList optional=False
-object :obj-boolList-wrapper
+object :obj-boolList wrapper
     member data: boolList optional=False
-object :obj-guest-sync-arg
+object :obj-guest-sync arguments
     member arg: any optional=False
-object :obj-int16List-wrapper
+object :obj-int16List wrapper
     member data: int16List optional=False
-object :obj-int32List-wrapper
+object :obj-int32List wrapper
     member data: int32List optional=False
-object :obj-int64List-wrapper
+object :obj-int64List wrapper
     member data: int64List optional=False
-object :obj-int8List-wrapper
+object :obj-int8List wrapper
     member data: int8List optional=False
-object :obj-intList-wrapper
+object :obj-intList wrapper
     member data: intList optional=False
-object :obj-numberList-wrapper
+object :obj-numberList wrapper
     member data: numberList optional=False
-object :obj-sizeList-wrapper
+object :obj-sizeList wrapper
     member data: sizeList optional=False
-object :obj-str-wrapper
+object :obj-str wrapper
     member data: str optional=False
-object :obj-strList-wrapper
+object :obj-strList wrapper
     member data: strList optional=False
-object :obj-uint16List-wrapper
+object :obj-uint16List wrapper
     member data: uint16List optional=False
-object :obj-uint32List-wrapper
+object :obj-uint32List wrapper
     member data: uint32List optional=False
-object :obj-uint64List-wrapper
+object :obj-uint64List wrapper
     member data: uint64List optional=False
-object :obj-uint8List-wrapper
+object :obj-uint8List wrapper
     member data: uint8List optional=False
-object :obj-user_def_cmd1-arg
+object :obj-user_def_cmd1 arguments
     member ud1a: UserDefOne optional=False
-object :obj-user_def_cmd2-arg
+object :obj-user_def_cmd2 arguments
     member ud1a: UserDefOne optional=False
     member ud1b: UserDefOne optional=True
-object :obj-user_def_cmd3-arg
+object :obj-user_def_cmd3 arguments
     member a: int optional=False
     member b: int optional=True
 alternate AltIntNum
@@ -79,8 +79,8 @@ alternate AltStrNum
 enum AltStrNumKind ['s', 'n']
 event EVENT_A None
 event EVENT_B None
-event EVENT_C :obj-EVENT_C-arg
-event EVENT_D :obj-EVENT_D-arg
+event EVENT_C :obj-EVENT_C data
+event EVENT_D :obj-EVENT_D data
 enum EnumOne ['value1', 'value2', 'value3']
 object EventStructOne
     member struct1: UserDefOne optional=False
@@ -123,20 +123,20 @@ object UserDefFlatUnion2
     case value2: UserDefB
     case value3: UserDefA
 object UserDefNativeListUnion
-    case integer: :obj-intList-wrapper
-    case s8: :obj-int8List-wrapper
-    case s16: :obj-int16List-wrapper
-    case s32: :obj-int32List-wrapper
-    case s64: :obj-int64List-wrapper
-    case u8: :obj-uint8List-wrapper
-    case u16: :obj-uint16List-wrapper
-    case u32: :obj-uint32List-wrapper
-    case u64: :obj-uint64List-wrapper
-    case number: :obj-numberList-wrapper
-    case boolean: :obj-boolList-wrapper
-    case string: :obj-strList-wrapper
-    case sizes: :obj-sizeList-wrapper
-    case any: :obj-anyList-wrapper
+    case integer: :obj-intList wrapper
+    case s8: :obj-int8List wrapper
+    case s16: :obj-int16List wrapper
+    case s32: :obj-int32List wrapper
+    case s64: :obj-int64List wrapper
+    case u8: :obj-uint8List wrapper
+    case u16: :obj-uint16List wrapper
+    case u32: :obj-uint32List wrapper
+    case u64: :obj-uint64List wrapper
+    case number: :obj-numberList wrapper
+    case boolean: :obj-boolList wrapper
+    case string: :obj-strList wrapper
+    case sizes: :obj-sizeList wrapper
+    case any: :obj-anyList wrapper
 enum UserDefNativeListUnionKind ['integer', 's8', 's16', 's32', 's64', 'u8', 'u16', 'u32', 'u64', 'number', 'boolean', 'string', 'sizes', 'any']
 object UserDefOne
     base UserDefZero
@@ -178,21 +178,21 @@ object __org.qemu_x-Struct
 object __org.qemu_x-Struct2
     member array: __org.qemu_x-Union1List optional=False
 object __org.qemu_x-Union1
-    case __org.qemu_x-branch: :obj-str-wrapper
+    case __org.qemu_x-branch: :obj-str wrapper
 enum __org.qemu_x-Union1Kind ['__org.qemu_x-branch']
 object __org.qemu_x-Union2
     base __org.qemu_x-Base
     tag __org.qemu_x-member1
     case __org.qemu_x-value: __org.qemu_x-Struct2
-command __org.qemu_x-command :obj-__org.qemu_x-command-arg -> __org.qemu_x-Union1
+command __org.qemu_x-command :obj-__org.qemu_x-command arguments -> __org.qemu_x-Union1
    gen=True success_response=True
-command guest-sync :obj-guest-sync-arg -> any
+command guest-sync :obj-guest-sync arguments -> any
    gen=True success_response=True
 command user_def_cmd None -> None
    gen=True success_response=True
-command user_def_cmd1 :obj-user_def_cmd1-arg -> None
+command user_def_cmd1 :obj-user_def_cmd1 arguments -> None
    gen=True success_response=True
-command user_def_cmd2 :obj-user_def_cmd2-arg -> UserDefTwo
+command user_def_cmd2 :obj-user_def_cmd2 arguments -> UserDefTwo
    gen=True success_response=True
-command user_def_cmd3 :obj-user_def_cmd3-arg -> int
+command user_def_cmd3 :obj-user_def_cmd3 arguments -> int
    gen=True success_response=True
diff --git a/tests/qapi-schema/union-clash-data.out b/tests/qapi-schema/union-clash-data.out
index 6277239..4c5903f 100644
--- a/tests/qapi-schema/union-clash-data.out
+++ b/tests/qapi-schema/union-clash-data.out
@@ -1,6 +1,6 @@
 object :empty
-object :obj-int-wrapper
+object :obj-int wrapper
     member data: int optional=False
 object TestUnion
-    case data: :obj-int-wrapper
+    case data: :obj-int wrapper
 enum TestUnionKind ['data']
-- 
2.4.3

Re: [Qemu-devel] [PATCH v7 09/14] qapi: Track owner of each object member

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Future commits will migrate semantic checking away from parsing
> and over to the various QAPISchema*.check() methods.  But to
> report an error message about an incorrect semantic use of a
> member of an object type, it helps to know which type, command,
> or event owns the member.  Rather than making all the check()
> methods have to pass around additional information, it is easier
> to have each member track the name of the type that owns it in
> the first place.

Making members track their owner is easy enough (your patch is proof),
but asserting it's easier suggests you tried the other approach, too.
Did you?

In fact, passing the owner to the check() methods would be easy enough.
QAPISchemaObjectType.check() passes self to its local members' .check().
Covers non-variant members.  Variant members take a bit more effort,
because more classes (and thus more check() methods) are involved.
QAPISchemaObjectType.check() and QAPISchemaAlternateType.check() pass
self to QAPISchemaObjectTypeVariants.check(), which passes it on to
QAPISchemaObjectTypeVariant.check(), which passes it on to
QAPISchemaObjectTypeMember.check().

I suspect the technique becomes cumbersome only when you start passing
members to helper functions: you have to pass the owner, too.  If
members know their owner, passing a member suffices.

>                   The new member.owner field is set when
> registering the members and variants arrays with an object or
> variant type.  We track only a name, and not the actual type
> object, to avoid creating a circular python reference chain.
>
> The source information is intended for human consumption in
> error messages, and a new describe() method is added to access
> the resulting information.  For example, given the qapi:
>  { 'command': 'foo', 'data': { 'string': 'str' } }
> an implementation of visit_command() that calls
>  arg_type.members[0].describe()
> will see "'string' (member of foo arguments)".
>
> To make the human-readable name of implicit types work without
> duplicating efforts, the name of implicit types is tweaked
> after the ':obj-' prefix, so that we can just trim off the
> prefix.  This required updates to the testsuite.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v7: total rewrite: rework implicit object names, assign owner
> when initializing owner type rather than when creating member
> python object
> v6: rebase on new lazy array creation and simple union 'type'
> motion; tweak commit message
> ---
>  scripts/qapi.py                         | 41 ++++++++++++---
>  tests/qapi-schema/args-member-array.out |  4 +-
>  tests/qapi-schema/args-name-clash.out   |  4 +-
>  tests/qapi-schema/ident-with-escape.out |  4 +-
>  tests/qapi-schema/qapi-schema-test.out  | 88 ++++++++++++++++-----------------
>  tests/qapi-schema/union-clash-data.out  |  4 +-
>  6 files changed, 87 insertions(+), 58 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index e49f72b..11ffc49 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -961,8 +961,16 @@ class QAPISchemaObjectType(QAPISchemaType):
>          assert base is None or isinstance(base, str)
>          for m in local_members:
>              assert isinstance(m, QAPISchemaObjectTypeMember)
> -        assert (variants is None or
> -                isinstance(variants, QAPISchemaObjectTypeVariants))
> +            assert not m.owner
> +            m.owner = name
> +        if variants is not None:
> +            assert isinstance(variants, QAPISchemaObjectTypeVariants)
> +            if variants.tag_member:
> +                assert not variants.tag_member.owner
> +                variants.tag_member.owner = name
> +            for v in variants.variants:
> +                assert not v.owner
> +                v.owner = name

Works, but rummaging in instances of other classes is not so nice.
Could instead do

        for m in local_members:
            m.set_owner(name)
        if variants is not None:
            variants.set_owner(name)

with the obvious set_owner() methods in QAPISchemaObjectTypeMember,
QAPISchemaObjectTypeVariants, QAPISchemaObjectTypeVariant.

>          self._base_name = base
>          self.base = None
>          self.local_members = local_members
> @@ -1023,8 +1031,10 @@ class QAPISchemaObjectTypeMember(object):
>          self._type_name = typ
>          self.type = None
>          self.optional = optional
> +        self.owner = None   # will be filled by owner's init
>
>      def check(self, schema, all_members, seen):
> +        assert self.owner
>          assert self.name not in seen
>          self.type = schema.lookup_type(self._type_name)
>          assert self.type
> @@ -1034,6 +1044,15 @@ class QAPISchemaObjectTypeMember(object):
>      def c_name(self):
>          return c_name(self.name)
>
> +    def describe(self):
> +        source = self.owner
> +        if source.startswith(':obj-'):
> +            source = source[5:]
> +        return "'%s' (%s of %s)" % (self.name, self._describe(), source)
> +
> +    def _describe(self):
> +        return 'member'

A simple class variable would do, wouldn't it?

> +
>
>  # TODO Drop this class once we no longer have the 'type'/'kind' mismatch
>  class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
> @@ -1042,6 +1061,9 @@ class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
>          assert self.type.is_implicit(QAPISchemaEnumType)
>          return 'kind'
>
> +    def describe(self):
> +        return "'kind' (implicit tag of %s)" % self.owner
> +

Let's compare to the inherited describe() with this one.

Why no need to strip a ':obj-' prefix here?

This prints "'kind' (implicit tag of ...)".  The inherited describe()
would print "'type' (member of ...).

The 'kind' vs. 'type' difference doesn't matter, because neither name
occurs in the schema anyway.

"implicit tag of" is an improvement over "member of".

However, the inherited describe() already has a hook to replace the part
before the "of".  Why do we need to override it wholesale anyway?

>
>  class QAPISchemaObjectTypeVariants(object):
>      def __init__(self, tag_name, tag_member, variants):
> @@ -1086,12 +1108,19 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>              return self.type.members[0].type
>          return None
>
> +    def _describe(self):
> +        return 'branch'
> +
>
>  class QAPISchemaAlternateType(QAPISchemaType):
>      def __init__(self, name, info, variants):
>          QAPISchemaType.__init__(self, name, info)
>          assert isinstance(variants, QAPISchemaObjectTypeVariants)
> -        assert not variants.tag_name
> +        assert variants.tag_member and not variants.tag_member.owner
> +        variants.tag_member.owner = name
> +        for v in variants.variants:
> +            assert not v.owner
> +            v.owner = name

With the set_owner() discussed above, this becomes a one-liner:

        variants.set_owner(name)

>          self.variants = variants
>
>      def check(self, schema):
> @@ -1222,7 +1251,7 @@ class QAPISchema(object):
>      def _make_implicit_object_type(self, name, info, role, members):
>          if not members:
>              return None
> -        name = ':obj-%s-%s' % (name, role)
> +        name = ':obj-%s %s' % (name, role)
>          if not self.lookup_entity(name, QAPISchemaObjectType):
>              self._def_entity(QAPISchemaObjectType(name, info, None,
>                                                    members, None))

I know I suggested this, but I'm having second thoughts about spaces in
name.  The test output (visible below) becomes a bit confusing, and
unnecessarily hard to parse by ad hoc scripts (yes, I've done such
things from time to time).

We could keep

        name = ':obj-%s-%s' % (name, role)

here, and replace the '-' by ' ' in describe().  Problematic if name or
role can also contain '-'.  Use a more suitable character to separate
the two then.

> @@ -1312,7 +1341,7 @@ class QAPISchema(object):
>          success_response = expr.get('success-response', True)
>          if isinstance(data, OrderedDict):
>              data = self._make_implicit_object_type(
> -                name, info, 'arg', self._make_members(data, info))
> +                name, info, 'arguments', self._make_members(data, info))
>          if isinstance(rets, list):
>              assert len(rets) == 1
>              rets = self._make_array_type(rets[0], info)
> @@ -1324,7 +1353,7 @@ class QAPISchema(object):
>          data = expr.get('data')
>          if isinstance(data, OrderedDict):
>              data = self._make_implicit_object_type(
> -                name, info, 'arg', self._make_members(data, info))
> +                name, info, 'data', self._make_members(data, info))
>          self._def_entity(QAPISchemaEvent(name, info, data))
>
>      def _def_exprs(self):
> diff --git a/tests/qapi-schema/args-member-array.out b/tests/qapi-schema/args-member-array.out
> index b3b92df..c822309 100644
> --- a/tests/qapi-schema/args-member-array.out
> +++ b/tests/qapi-schema/args-member-array.out
> @@ -1,9 +1,9 @@
>  object :empty
> -object :obj-okay-arg
> +object :obj-okay arguments
>      member member1: intList optional=False
>      member member2: defList optional=False
>  enum abc ['a', 'b', 'c']
>  object def
>      member array: abcList optional=False
> -command okay :obj-okay-arg -> None
> +command okay :obj-okay arguments -> None
>     gen=True success_response=True
> diff --git a/tests/qapi-schema/args-name-clash.out b/tests/qapi-schema/args-name-clash.out
> index 9b2f6e4..0e986b6 100644
> --- a/tests/qapi-schema/args-name-clash.out
> +++ b/tests/qapi-schema/args-name-clash.out
> @@ -1,6 +1,6 @@
>  object :empty
> -object :obj-oops-arg
> +object :obj-oops arguments
>      member a-b: str optional=False
>      member a_b: str optional=False
> -command oops :obj-oops-arg -> None
> +command oops :obj-oops arguments -> None
>     gen=True success_response=True
> diff --git a/tests/qapi-schema/ident-with-escape.out b/tests/qapi-schema/ident-with-escape.out
> index f4542b1..4544777 100644
> --- a/tests/qapi-schema/ident-with-escape.out
> +++ b/tests/qapi-schema/ident-with-escape.out
> @@ -1,5 +1,5 @@
>  object :empty
> -object :obj-fooA-arg
> +object :obj-fooA arguments
>      member bar1: str optional=False
> -command fooA :obj-fooA-arg -> None
> +command fooA :obj-fooA arguments -> None
>     gen=True success_response=True
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 2a8c82e..c666481 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -1,56 +1,56 @@
>  object :empty
> -object :obj-EVENT_C-arg
> +object :obj-EVENT_C data
>      member a: int optional=True
>      member b: UserDefOne optional=True
>      member c: str optional=False
> -object :obj-EVENT_D-arg
> +object :obj-EVENT_D data
>      member a: EventStructOne optional=False
>      member b: str optional=False
>      member c: str optional=True
>      member enum3: EnumOne optional=True
> -object :obj-__org.qemu_x-command-arg
> +object :obj-__org.qemu_x-command arguments
>      member a: __org.qemu_x-EnumList optional=False
>      member b: __org.qemu_x-StructList optional=False
>      member c: __org.qemu_x-Union2 optional=False
>      member d: __org.qemu_x-Alt optional=False
> -object :obj-anyList-wrapper
> +object :obj-anyList wrapper
>      member data: anyList optional=False
> -object :obj-boolList-wrapper
> +object :obj-boolList wrapper
>      member data: boolList optional=False
> -object :obj-guest-sync-arg
> +object :obj-guest-sync arguments
>      member arg: any optional=False
> -object :obj-int16List-wrapper
> +object :obj-int16List wrapper
>      member data: int16List optional=False
> -object :obj-int32List-wrapper
> +object :obj-int32List wrapper
>      member data: int32List optional=False
> -object :obj-int64List-wrapper
> +object :obj-int64List wrapper
>      member data: int64List optional=False
> -object :obj-int8List-wrapper
> +object :obj-int8List wrapper
>      member data: int8List optional=False
> -object :obj-intList-wrapper
> +object :obj-intList wrapper
>      member data: intList optional=False
> -object :obj-numberList-wrapper
> +object :obj-numberList wrapper
>      member data: numberList optional=False
> -object :obj-sizeList-wrapper
> +object :obj-sizeList wrapper
>      member data: sizeList optional=False
> -object :obj-str-wrapper
> +object :obj-str wrapper
>      member data: str optional=False
> -object :obj-strList-wrapper
> +object :obj-strList wrapper
>      member data: strList optional=False
> -object :obj-uint16List-wrapper
> +object :obj-uint16List wrapper
>      member data: uint16List optional=False
> -object :obj-uint32List-wrapper
> +object :obj-uint32List wrapper
>      member data: uint32List optional=False
> -object :obj-uint64List-wrapper
> +object :obj-uint64List wrapper
>      member data: uint64List optional=False
> -object :obj-uint8List-wrapper
> +object :obj-uint8List wrapper
>      member data: uint8List optional=False
> -object :obj-user_def_cmd1-arg
> +object :obj-user_def_cmd1 arguments
>      member ud1a: UserDefOne optional=False
> -object :obj-user_def_cmd2-arg
> +object :obj-user_def_cmd2 arguments
>      member ud1a: UserDefOne optional=False
>      member ud1b: UserDefOne optional=True
> -object :obj-user_def_cmd3-arg
> +object :obj-user_def_cmd3 arguments
>      member a: int optional=False
>      member b: int optional=True
>  alternate AltIntNum
> @@ -79,8 +79,8 @@ alternate AltStrNum
>  enum AltStrNumKind ['s', 'n']
>  event EVENT_A None
>  event EVENT_B None
> -event EVENT_C :obj-EVENT_C-arg
> -event EVENT_D :obj-EVENT_D-arg
> +event EVENT_C :obj-EVENT_C data
> +event EVENT_D :obj-EVENT_D data
>  enum EnumOne ['value1', 'value2', 'value3']
>  object EventStructOne
>      member struct1: UserDefOne optional=False
> @@ -123,20 +123,20 @@ object UserDefFlatUnion2
>      case value2: UserDefB
>      case value3: UserDefA
>  object UserDefNativeListUnion
> -    case integer: :obj-intList-wrapper
> -    case s8: :obj-int8List-wrapper
> -    case s16: :obj-int16List-wrapper
> -    case s32: :obj-int32List-wrapper
> -    case s64: :obj-int64List-wrapper
> -    case u8: :obj-uint8List-wrapper
> -    case u16: :obj-uint16List-wrapper
> -    case u32: :obj-uint32List-wrapper
> -    case u64: :obj-uint64List-wrapper
> -    case number: :obj-numberList-wrapper
> -    case boolean: :obj-boolList-wrapper
> -    case string: :obj-strList-wrapper
> -    case sizes: :obj-sizeList-wrapper
> -    case any: :obj-anyList-wrapper
> +    case integer: :obj-intList wrapper
> +    case s8: :obj-int8List wrapper
> +    case s16: :obj-int16List wrapper
> +    case s32: :obj-int32List wrapper
> +    case s64: :obj-int64List wrapper
> +    case u8: :obj-uint8List wrapper
> +    case u16: :obj-uint16List wrapper
> +    case u32: :obj-uint32List wrapper
> +    case u64: :obj-uint64List wrapper
> +    case number: :obj-numberList wrapper
> +    case boolean: :obj-boolList wrapper
> +    case string: :obj-strList wrapper
> +    case sizes: :obj-sizeList wrapper
> +    case any: :obj-anyList wrapper
>  enum UserDefNativeListUnionKind ['integer', 's8', 's16', 's32', 's64', 'u8', 'u16', 'u32', 'u64', 'number', 'boolean', 'string', 'sizes', 'any']
>  object UserDefOne
>      base UserDefZero
> @@ -178,21 +178,21 @@ object __org.qemu_x-Struct
>  object __org.qemu_x-Struct2
>      member array: __org.qemu_x-Union1List optional=False
>  object __org.qemu_x-Union1
> -    case __org.qemu_x-branch: :obj-str-wrapper
> +    case __org.qemu_x-branch: :obj-str wrapper
>  enum __org.qemu_x-Union1Kind ['__org.qemu_x-branch']
>  object __org.qemu_x-Union2
>      base __org.qemu_x-Base
>      tag __org.qemu_x-member1
>      case __org.qemu_x-value: __org.qemu_x-Struct2
> -command __org.qemu_x-command :obj-__org.qemu_x-command-arg -> __org.qemu_x-Union1
> +command __org.qemu_x-command :obj-__org.qemu_x-command arguments -> __org.qemu_x-Union1
>     gen=True success_response=True
> -command guest-sync :obj-guest-sync-arg -> any
> +command guest-sync :obj-guest-sync arguments -> any
>     gen=True success_response=True
>  command user_def_cmd None -> None
>     gen=True success_response=True
> -command user_def_cmd1 :obj-user_def_cmd1-arg -> None
> +command user_def_cmd1 :obj-user_def_cmd1 arguments -> None
>     gen=True success_response=True
> -command user_def_cmd2 :obj-user_def_cmd2-arg -> UserDefTwo
> +command user_def_cmd2 :obj-user_def_cmd2 arguments -> UserDefTwo
>     gen=True success_response=True
> -command user_def_cmd3 :obj-user_def_cmd3-arg -> int
> +command user_def_cmd3 :obj-user_def_cmd3 arguments -> int
>     gen=True success_response=True
> diff --git a/tests/qapi-schema/union-clash-data.out b/tests/qapi-schema/union-clash-data.out
> index 6277239..4c5903f 100644
> --- a/tests/qapi-schema/union-clash-data.out
> +++ b/tests/qapi-schema/union-clash-data.out
> @@ -1,6 +1,6 @@
>  object :empty
> -object :obj-int-wrapper
> +object :obj-int wrapper
>      member data: int optional=False
>  object TestUnion
> -    case data: :obj-int-wrapper
> +    case data: :obj-int wrapper
>  enum TestUnionKind ['data']

Re: [Qemu-devel] [PATCH v7 09/14] qapi: Track owner of each object member

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 7206 bytes --]

On 10/09/2015 07:17 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Future commits will migrate semantic checking away from parsing
>> and over to the various QAPISchema*.check() methods.  But to
>> report an error message about an incorrect semantic use of a
>> member of an object type, it helps to know which type, command,
>> or event owns the member.  Rather than making all the check()
>> methods have to pass around additional information, it is easier
>> to have each member track the name of the type that owns it in
>> the first place.
> 
> Making members track their owner is easy enough (your patch is proof),
> but asserting it's easier suggests you tried the other approach, too.
> Did you?

Yes, my first attempt was indeed to pass the QAPISchemaEntity being
checked to each QAPISchemaObjectTypeMember.check() call...

> 
> In fact, passing the owner to the check() methods would be easy enough.
> QAPISchemaObjectType.check() passes self to its local members' .check().
> Covers non-variant members.  Variant members take a bit more effort,
> because more classes (and thus more check() methods) are involved.
> QAPISchemaObjectType.check() and QAPISchemaAlternateType.check() pass
> self to QAPISchemaObjectTypeVariants.check(), which passes it on to
> QAPISchemaObjectTypeVariant.check(), which passes it on to
> QAPISchemaObjectTypeMember.check().
> 
> I suspect the technique becomes cumbersome only when you start passing
> members to helper functions: you have to pass the owner, too.  If
> members know their owner, passing a member suffices.

...One of the other advantages of my approach that you don't get when
passing the owner to each check() call is that when it comes to base
classes, the owner of a member inherited from a base class should be the
base class, but the ObjectType that is calling the .check() method is
instead the derived class.  As soon as I realized that in my first
approach, my next attempt was to pass more information through each
element of the seen[] array, as in:
  seen[m.name] = {'member':m, 'owner':type}
until I  realized that it really was easier to just keep things with:
  seen[m.name] = m
and have m.owner do the right thing.  Having the member track the base
class owner makes the error messages much nicer (as in "'name' (member
of Sub) collides with 'name' (member of Base)" in a subsequent patch),
which was not possible when the only thing being passed down was the
current ObjectType being checked.

>> +++ b/scripts/qapi.py
>> @@ -961,8 +961,16 @@ class QAPISchemaObjectType(QAPISchemaType):
>>          assert base is None or isinstance(base, str)
>>          for m in local_members:
>>              assert isinstance(m, QAPISchemaObjectTypeMember)
>> -        assert (variants is None or
>> -                isinstance(variants, QAPISchemaObjectTypeVariants))
>> +            assert not m.owner
>> +            m.owner = name
>> +        if variants is not None:
>> +            assert isinstance(variants, QAPISchemaObjectTypeVariants)
>> +            if variants.tag_member:
>> +                assert not variants.tag_member.owner
>> +                variants.tag_member.owner = name
>> +            for v in variants.variants:
>> +                assert not v.owner
>> +                v.owner = name
> 
> Works, but rummaging in instances of other classes is not so nice.
> Could instead do
> 
>         for m in local_members:
>             m.set_owner(name)
>         if variants is not None:
>             variants.set_owner(name)
> 
> with the obvious set_owner() methods in QAPISchemaObjectTypeMember,
> QAPISchemaObjectTypeVariants, QAPISchemaObjectTypeVariant.

Yeah, I was debating about a set_owner() method - sounds like it is the
way to go, and that I ought to post a v8 spin of this series.

>> @@ -1034,6 +1044,15 @@ class QAPISchemaObjectTypeMember(object):
>>      def c_name(self):
>>          return c_name(self.name)
>>
>> +    def describe(self):
>> +        source = self.owner
>> +        if source.startswith(':obj-'):
>> +            source = source[5:]
>> +        return "'%s' (%s of %s)" % (self.name, self._describe(), source)
>> +
>> +    def _describe(self):
>> +        return 'member'
> 
> A simple class variable would do, wouldn't it?

Are class variables polymorphic?  (I guess I have to go figure out the
python lookup rules for self.variable, to see if they differ from
self.method())

>> @@ -1042,6 +1061,9 @@ class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
>>          assert self.type.is_implicit(QAPISchemaEnumType)
>>          return 'kind'
>>
>> +    def describe(self):
>> +        return "'kind' (implicit tag of %s)" % self.owner
>> +
> 
> Let's compare to the inherited describe() with this one.
> 
> Why no need to strip a ':obj-' prefix here?

Because no union type is implicit, so there is no ':obj-' prefix to strip.

> 
> This prints "'kind' (implicit tag of ...)".  The inherited describe()
> would print "'type' (member of ...).
> 
> The 'kind' vs. 'type' difference doesn't matter, because neither name
> occurs in the schema anyway.
> 
> "implicit tag of" is an improvement over "member of".
> 
> However, the inherited describe() already has a hook to replace the part
> before the "of".  Why do we need to override it wholesale anyway?

The parent version prints self.name ('type'), but as long as we have the
type/kind mismatch, we want to print the C name that is conflicting
('kind') and not self.name.  But once I override describe() for that, it
was easier to do a wholesale override.  It all goes away later in the
patch that renames kind to type.  Maybe some comments here would help?


>> @@ -1222,7 +1251,7 @@ class QAPISchema(object):
>>      def _make_implicit_object_type(self, name, info, role, members):
>>          if not members:
>>              return None
>> -        name = ':obj-%s-%s' % (name, role)
>> +        name = ':obj-%s %s' % (name, role)
>>          if not self.lookup_entity(name, QAPISchemaObjectType):
>>              self._def_entity(QAPISchemaObjectType(name, info, None,
>>                                                    members, None))
> 
> I know I suggested this, but I'm having second thoughts about spaces in
> name.  The test output (visible below) becomes a bit confusing, and
> unnecessarily hard to parse by ad hoc scripts (yes, I've done such
> things from time to time).
> 
> We could keep
> 
>         name = ':obj-%s-%s' % (name, role)
> 
> here, and replace the '-' by ' ' in describe().  Problematic if name or
> role can also contain '-'.  Use a more suitable character to separate
> the two then.

Name can contain '-', role does not. Can always search for the last '-'.
Or we could reorder things to put role first.  Or use ':obj:%s:%s'.  But
yes, I agree that we can keep the implicit name space-free, and make
describe() smarter at reverse-engineering it back into a human-readable
description.

-- 
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 --]

[Qemu-devel] [PATCH v7 10/14] qapi: Detect collisions in C member names

[Eric Blake]

Detect attempts to declare two object members that would result
in the same C member name, by keying the 'seen' dictionary off
of the C name rather than the qapi name.  It also requires passing
info through some of the check() methods.

This fixes two previously-broken tests, and the resulting error
messages demonstrate the utility of the .describe() method added
previously.  No change to generated code.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v7: split out error reporting prep and member.c_name() addition
v6: rebase to earlier testsuite and info improvements
---
 scripts/qapi.py                                | 33 ++++++++++++++++----------
 tests/qapi-schema/args-name-clash.err          |  1 +
 tests/qapi-schema/args-name-clash.exit         |  2 +-
 tests/qapi-schema/args-name-clash.json         |  6 ++---
 tests/qapi-schema/args-name-clash.out          |  6 -----
 tests/qapi-schema/flat-union-clash-branch.err  |  1 +
 tests/qapi-schema/flat-union-clash-branch.exit |  2 +-
 tests/qapi-schema/flat-union-clash-branch.json |  9 +++----
 tests/qapi-schema/flat-union-clash-branch.out  | 14 -----------
 9 files changed, 32 insertions(+), 42 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 11ffc49..30f1483 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -993,11 +993,11 @@ class QAPISchemaObjectType(QAPISchemaType):
         seen = {}
         for m in members:
             assert m.c_name() not in seen
-            seen[m.name] = m
+            seen[m.c_name()] = m
         for m in self.local_members:
-            m.check(schema, members, seen)
+            m.check(schema, self.info, members, seen)
         if self.variants:
-            self.variants.check(schema, members, seen)
+            self.variants.check(schema, self.info, members, seen)
         self.members = members

     def _is_implicit(self):
@@ -1033,13 +1033,19 @@ class QAPISchemaObjectTypeMember(object):
         self.optional = optional
         self.owner = None   # will be filled by owner's init

-    def check(self, schema, all_members, seen):
+    def check(self, schema, info, all_members, seen):
         assert self.owner
-        assert self.name not in seen
         self.type = schema.lookup_type(self._type_name)
         assert self.type
+        # Check that there is no collision in generated C names (which
+        # also ensures no collisions in QMP names)
+        if self.c_name() in seen:
+            raise QAPIExprError(info,
+                                "%s collides with %s"
+                                % (self.describe(),
+                                   seen[self.c_name()].describe()))
         all_members.append(self)
-        seen[self.name] = self
+        seen[self.c_name()] = self

     def c_name(self):
         return c_name(self.name)
@@ -1080,23 +1086,24 @@ class QAPISchemaObjectTypeVariants(object):
         self.tag_member = tag_member
         self.variants = variants

-    def check(self, schema, members, seen):
+    def check(self, schema, info, members, seen):
         if self.tag_name:
-            self.tag_member = seen[self.tag_name]
+            self.tag_member = seen[c_name(self.tag_name)]
+            assert self.tag_name == self.tag_member.name
         else:
-            self.tag_member.check(schema, members, seen)
+            self.tag_member.check(schema, info, members, seen)
         assert isinstance(self.tag_member.type, QAPISchemaEnumType)
         for v in self.variants:
             vseen = dict(seen)
-            v.check(schema, self.tag_member.type, vseen)
+            v.check(schema, info, self.tag_member.type, vseen)


 class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
     def __init__(self, name, typ):
         QAPISchemaObjectTypeMember.__init__(self, name, typ, False)

-    def check(self, schema, tag_type, seen):
-        QAPISchemaObjectTypeMember.check(self, schema, [], seen)
+    def check(self, schema, info, tag_type, seen):
+        QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
         assert self.name in tag_type.values

     # This function exists to support ugly simple union special cases
@@ -1124,7 +1131,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
         self.variants = variants

     def check(self, schema):
-        self.variants.check(schema, [], {})
+        self.variants.check(schema, self.info, [], {})

     def json_type(self):
         return 'value'
diff --git a/tests/qapi-schema/args-name-clash.err b/tests/qapi-schema/args-name-clash.err
index e69de29..66f609c 100644
--- a/tests/qapi-schema/args-name-clash.err
+++ b/tests/qapi-schema/args-name-clash.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-name-clash.json:5: 'a_b' (member of oops arguments) collides with 'a-b' (member of oops arguments)
diff --git a/tests/qapi-schema/args-name-clash.exit b/tests/qapi-schema/args-name-clash.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/args-name-clash.exit
+++ b/tests/qapi-schema/args-name-clash.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/args-name-clash.json b/tests/qapi-schema/args-name-clash.json
index 9e8f889..3fe4ea5 100644
--- a/tests/qapi-schema/args-name-clash.json
+++ b/tests/qapi-schema/args-name-clash.json
@@ -1,5 +1,5 @@
 # C member name collision
-# FIXME - This parses, but fails to compile, because the C struct is given
-# two 'a_b' members.  Either reject this at parse time, or munge the C names
-# to avoid the collision.
+# Reject members that clash when mapped to C names (we would have two 'a_b'
+# members). It would also be possible to munge the C names to avoid the
+# collision, but unlikely to be worth the effort.
 { 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }
diff --git a/tests/qapi-schema/args-name-clash.out b/tests/qapi-schema/args-name-clash.out
index 0e986b6..e69de29 100644
--- a/tests/qapi-schema/args-name-clash.out
+++ b/tests/qapi-schema/args-name-clash.out
@@ -1,6 +0,0 @@
-object :empty
-object :obj-oops arguments
-    member a-b: str optional=False
-    member a_b: str optional=False
-command oops :obj-oops arguments -> None
-   gen=True success_response=True
diff --git a/tests/qapi-schema/flat-union-clash-branch.err b/tests/qapi-schema/flat-union-clash-branch.err
index e69de29..0190d79 100644
--- a/tests/qapi-schema/flat-union-clash-branch.err
+++ b/tests/qapi-schema/flat-union-clash-branch.err
@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-clash-branch.json:15: 'c-d' (branch of TestUnion) collides with 'c_d' (member of Base)
diff --git a/tests/qapi-schema/flat-union-clash-branch.exit b/tests/qapi-schema/flat-union-clash-branch.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/flat-union-clash-branch.exit
+++ b/tests/qapi-schema/flat-union-clash-branch.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/flat-union-clash-branch.json b/tests/qapi-schema/flat-union-clash-branch.json
index e593336..a6c302f 100644
--- a/tests/qapi-schema/flat-union-clash-branch.json
+++ b/tests/qapi-schema/flat-union-clash-branch.json
@@ -1,8 +1,9 @@
 # Flat union branch name collision
-# FIXME: this parses, but then fails to compile due to a duplicate 'c_d'
-# (one from the base member, the other from the branch name).  We should
-# either reject the collision at parse time, or munge the generated branch
-# name to allow this to compile.
+# Reject attempts to use a branch name that would clash with a non-variant
+# member, when mapped to C names (here, we would have two 'c_d' members,
+# one from the base member, the other from the branch name).
+# TODO: We could munge the generated branch name to avoid the collision and
+# allow this to compile.
 { 'enum': 'TestEnum',
   'data': [ 'base', 'c-d' ] }
 { 'struct': 'Base',
diff --git a/tests/qapi-schema/flat-union-clash-branch.out b/tests/qapi-schema/flat-union-clash-branch.out
index 8e0da73..e69de29 100644
--- a/tests/qapi-schema/flat-union-clash-branch.out
+++ b/tests/qapi-schema/flat-union-clash-branch.out
@@ -1,14 +0,0 @@
-object :empty
-object Base
-    member enum1: TestEnum optional=False
-    member c_d: str optional=True
-object Branch1
-    member string: str optional=False
-object Branch2
-    member value: int optional=False
-enum TestEnum ['base', 'c-d']
-object TestUnion
-    base Base
-    tag enum1
-    case base: Branch1
-    case c-d: Branch2
-- 
2.4.3

Re: [Qemu-devel] [PATCH v7 10/14] qapi: Detect collisions in C member names

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Detect attempts to declare two object members that would result
> in the same C member name, by keying the 'seen' dictionary off
> of the C name rather than the qapi name.  It also requires passing
> info through some of the check() methods.
>
> This fixes two previously-broken tests, and the resulting error
> messages demonstrate the utility of the .describe() method added
> previously.  No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v7: split out error reporting prep and member.c_name() addition
> v6: rebase to earlier testsuite and info improvements
> ---
>  scripts/qapi.py                                | 33 ++++++++++++++++----------
>  tests/qapi-schema/args-name-clash.err          |  1 +
>  tests/qapi-schema/args-name-clash.exit         |  2 +-
>  tests/qapi-schema/args-name-clash.json         |  6 ++---
>  tests/qapi-schema/args-name-clash.out          |  6 -----
>  tests/qapi-schema/flat-union-clash-branch.err  |  1 +
>  tests/qapi-schema/flat-union-clash-branch.exit |  2 +-
>  tests/qapi-schema/flat-union-clash-branch.json |  9 +++----
>  tests/qapi-schema/flat-union-clash-branch.out  | 14 -----------
>  9 files changed, 32 insertions(+), 42 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 11ffc49..30f1483 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -993,11 +993,11 @@ class QAPISchemaObjectType(QAPISchemaType):
>          seen = {}
>          for m in members:
>              assert m.c_name() not in seen
> -            seen[m.name] = m
> +            seen[m.c_name()] = m
>          for m in self.local_members:
> -            m.check(schema, members, seen)
> +            m.check(schema, self.info, members, seen)
>          if self.variants:
> -            self.variants.check(schema, members, seen)
> +            self.variants.check(schema, self.info, members, seen)
>          self.members = members
>
>      def _is_implicit(self):
> @@ -1033,13 +1033,19 @@ class QAPISchemaObjectTypeMember(object):
>          self.optional = optional
>          self.owner = None   # will be filled by owner's init
>
> -    def check(self, schema, all_members, seen):
> +    def check(self, schema, info, all_members, seen):
>          assert self.owner
> -        assert self.name not in seen
>          self.type = schema.lookup_type(self._type_name)
>          assert self.type
> +        # Check that there is no collision in generated C names (which
> +        # also ensures no collisions in QMP names)
> +        if self.c_name() in seen:
> +            raise QAPIExprError(info,
> +                                "%s collides with %s"
> +                                % (self.describe(),
> +                                   seen[self.c_name()].describe()))
>          all_members.append(self)
> -        seen[self.name] = self
> +        seen[self.c_name()] = self
>
>      def c_name(self):
>          return c_name(self.name)
> @@ -1080,23 +1086,24 @@ class QAPISchemaObjectTypeVariants(object):
>          self.tag_member = tag_member
>          self.variants = variants
>
> -    def check(self, schema, members, seen):
> +    def check(self, schema, info, members, seen):
>          if self.tag_name:
> -            self.tag_member = seen[self.tag_name]
> +            self.tag_member = seen[c_name(self.tag_name)]
> +            assert self.tag_name == self.tag_member.name
>          else:
> -            self.tag_member.check(schema, members, seen)
> +            self.tag_member.check(schema, info, members, seen)
>          assert isinstance(self.tag_member.type, QAPISchemaEnumType)
>          for v in self.variants:
>              vseen = dict(seen)
> -            v.check(schema, self.tag_member.type, vseen)
> +            v.check(schema, info, self.tag_member.type, vseen)
>
>
>  class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>      def __init__(self, name, typ):
>          QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
>
> -    def check(self, schema, tag_type, seen):
> -        QAPISchemaObjectTypeMember.check(self, schema, [], seen)
> +    def check(self, schema, info, tag_type, seen):
> +        QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
>          assert self.name in tag_type.values
>
>      # This function exists to support ugly simple union special cases
> @@ -1124,7 +1131,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
>          self.variants = variants
>
>      def check(self, schema):
> -        self.variants.check(schema, [], {})
> +        self.variants.check(schema, self.info, [], {})
>
>      def json_type(self):
>          return 'value'
> diff --git a/tests/qapi-schema/args-name-clash.err b/tests/qapi-schema/args-name-clash.err
> index e69de29..66f609c 100644
> --- a/tests/qapi-schema/args-name-clash.err
> +++ b/tests/qapi-schema/args-name-clash.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/args-name-clash.json:5: 'a_b' (member of oops arguments) collides with 'a-b' (member of oops arguments)

"(argument of oops)" would be better, but "(member of oops arguments)"
will do.

> diff --git a/tests/qapi-schema/args-name-clash.exit b/tests/qapi-schema/args-name-clash.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/args-name-clash.exit
> +++ b/tests/qapi-schema/args-name-clash.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/args-name-clash.json b/tests/qapi-schema/args-name-clash.json
> index 9e8f889..3fe4ea5 100644
> --- a/tests/qapi-schema/args-name-clash.json
> +++ b/tests/qapi-schema/args-name-clash.json
> @@ -1,5 +1,5 @@
>  # C member name collision
> -# FIXME - This parses, but fails to compile, because the C struct is given
> -# two 'a_b' members.  Either reject this at parse time, or munge the C names
> -# to avoid the collision.
> +# Reject members that clash when mapped to C names (we would have two 'a_b'
> +# members). It would also be possible to munge the C names to avoid the
> +# collision, but unlikely to be worth the effort.
>  { 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }
> diff --git a/tests/qapi-schema/args-name-clash.out b/tests/qapi-schema/args-name-clash.out
> index 0e986b6..e69de29 100644
> --- a/tests/qapi-schema/args-name-clash.out
> +++ b/tests/qapi-schema/args-name-clash.out
> @@ -1,6 +0,0 @@
> -object :empty
> -object :obj-oops arguments
> -    member a-b: str optional=False
> -    member a_b: str optional=False
> -command oops :obj-oops arguments -> None
> -   gen=True success_response=True
> diff --git a/tests/qapi-schema/flat-union-clash-branch.err b/tests/qapi-schema/flat-union-clash-branch.err
> index e69de29..0190d79 100644
> --- a/tests/qapi-schema/flat-union-clash-branch.err
> +++ b/tests/qapi-schema/flat-union-clash-branch.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/flat-union-clash-branch.json:15: 'c-d' (branch of TestUnion) collides with 'c_d' (member of Base)
> diff --git a/tests/qapi-schema/flat-union-clash-branch.exit b/tests/qapi-schema/flat-union-clash-branch.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/flat-union-clash-branch.exit
> +++ b/tests/qapi-schema/flat-union-clash-branch.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/flat-union-clash-branch.json b/tests/qapi-schema/flat-union-clash-branch.json
> index e593336..a6c302f 100644
> --- a/tests/qapi-schema/flat-union-clash-branch.json
> +++ b/tests/qapi-schema/flat-union-clash-branch.json
> @@ -1,8 +1,9 @@
>  # Flat union branch name collision
> -# FIXME: this parses, but then fails to compile due to a duplicate 'c_d'
> -# (one from the base member, the other from the branch name).  We should
> -# either reject the collision at parse time, or munge the generated branch
> -# name to allow this to compile.
> +# Reject attempts to use a branch name that would clash with a non-variant
> +# member, when mapped to C names (here, we would have two 'c_d' members,
> +# one from the base member, the other from the branch name).
> +# TODO: We could munge the generated branch name to avoid the collision and
> +# allow this to compile.
>  { 'enum': 'TestEnum',
>    'data': [ 'base', 'c-d' ] }
>  { 'struct': 'Base',
> diff --git a/tests/qapi-schema/flat-union-clash-branch.out b/tests/qapi-schema/flat-union-clash-branch.out
> index 8e0da73..e69de29 100644
> --- a/tests/qapi-schema/flat-union-clash-branch.out
> +++ b/tests/qapi-schema/flat-union-clash-branch.out
> @@ -1,14 +0,0 @@
> -object :empty
> -object Base
> -    member enum1: TestEnum optional=False
> -    member c_d: str optional=True
> -object Branch1
> -    member string: str optional=False
> -object Branch2
> -    member value: int optional=False
> -enum TestEnum ['base', 'c-d']
> -object TestUnion
> -    base Base
> -    tag enum1
> -    case base: Branch1
> -    case c-d: Branch2

Re: [Qemu-devel] [PATCH v7 10/14] qapi: Detect collisions in C member names

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]

On 10/09/2015 08:11 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Detect attempts to declare two object members that would result
>> in the same C member name, by keying the 'seen' dictionary off
>> of the C name rather than the qapi name.  It also requires passing
>> info through some of the check() methods.
>>
>> This fixes two previously-broken tests, and the resulting error
>> messages demonstrate the utility of the .describe() method added
>> previously.  No change to generated code.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>

>> +++ b/tests/qapi-schema/args-name-clash.err
>> @@ -0,0 +1 @@
>> +tests/qapi-schema/args-name-clash.json:5: 'a_b' (member of oops arguments) collides with 'a-b' (member of oops arguments)
> 
> "(argument of oops)" would be better, but "(member of oops arguments)"
> will do.

May be possible to tweak the describe() function in the previous commit
along those lines, especially since I'm already tweaking it to not have
space in the generated name.  I'll have to play with it.

-- 
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 --]

Re: [Qemu-devel] [PATCH v7 10/14] qapi: Detect collisions in C member names

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/09/2015 08:11 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> Detect attempts to declare two object members that would result
>>> in the same C member name, by keying the 'seen' dictionary off
>>> of the C name rather than the qapi name.  It also requires passing
>>> info through some of the check() methods.
>>>
>>> This fixes two previously-broken tests, and the resulting error
>>> messages demonstrate the utility of the .describe() method added
>>> previously.  No change to generated code.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>>
>
>>> +++ b/tests/qapi-schema/args-name-clash.err
>>> @@ -0,0 +1 @@
>>> +tests/qapi-schema/args-name-clash.json:5: 'a_b' (member of oops
>>> arguments) collides with 'a-b' (member of oops arguments)
>> 
>> "(argument of oops)" would be better, but "(member of oops arguments)"
>> will do.
>
> May be possible to tweak the describe() function in the previous commit
> along those lines, especially since I'm already tweaking it to not have
> space in the generated name.  I'll have to play with it.

At some point, implementation simplicity trumps error message polish.
Use your judgement.

[Qemu-devel] [PATCH v7 11/14] qapi: Move duplicate member checks to schema check()

[Eric Blake]

With the previous commit, we have two different locations for
detecting member name clashes - one at parse time, and another
at QAPISchema*.check() time.  Consolidate some of the checks
into a single place, which is also in line with our TODO to
eventually move all of the parse time semantic checking into
the newer schema code.  The check_member_clash() function is
no longer needed.

The wording of several error messages has changed, but in many
cases feels like an improvement rather than a regression.  The
recent change to avoid an assertion failure when a flat union
branch name collides with its discriminator name is also
handled nicely by this code; but there is more work needed
before we can detect all collisions in simple union branch
names done by the old code.

No change to generated code.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v7: comment improvements, retitle subject
v6: rebase to earlier testsuite improvements, fold in cleanup
of flat-union-clash-type
---
 scripts/qapi.py                               | 51 ++++++++++-----------------
 tests/qapi-schema/flat-union-clash-member.err |  2 +-
 tests/qapi-schema/flat-union-clash-type.err   |  2 +-
 tests/qapi-schema/struct-base-clash-deep.err  |  2 +-
 tests/qapi-schema/struct-base-clash.err       |  2 +-
 5 files changed, 22 insertions(+), 37 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 30f1483..9f98413 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -499,21 +499,6 @@ def check_type(expr_info, source, value, allow_array=False,
                                 'enum'])


-def check_member_clash(expr_info, base_name, data, source=""):
-    base = find_struct(base_name)
-    assert base
-    base_members = base['data']
-    for key in data.keys():
-        if key.startswith('*'):
-            key = key[1:]
-        if key in base_members or "*" + key in base_members:
-            raise QAPIExprError(expr_info,
-                                "Member name '%s'%s clashes with base '%s'"
-                                % (key, source, base_name))
-    if base.get('base'):
-        check_member_clash(expr_info, base['base'], data, source)
-
-
 def check_command(expr, expr_info):
     name = expr['command']

@@ -592,30 +577,18 @@ def check_union(expr, expr_info):
     for (key, value) in members.items():
         check_name(expr_info, "Member of union '%s'" % name, key)

-        # Each value must name a known type; furthermore, in flat unions,
-        # branches must be a struct with no overlapping member names
+        # Each value must name a known type
         check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
                    value, allow_array=not base, allow_metas=allow_metas)
-        if base:
-            branch_struct = find_struct(value)
-            assert branch_struct
-            check_member_clash(expr_info, base, branch_struct['data'],
-                               " of branch '%s'" % key)

         # If the discriminator names an enum type, then all members
-        # of 'data' must also be members of the enum type, which in turn
-        # must not collide with the discriminator name.
+        # of 'data' must also be members of the enum type.
         if enum_define:
             if key not in enum_define['enum_values']:
                 raise QAPIExprError(expr_info,
                                     "Discriminator value '%s' is not found in "
                                     "enum '%s'" %
                                     (key, enum_define["enum_name"]))
-            if discriminator in enum_define['enum_values']:
-                raise QAPIExprError(expr_info,
-                                    "Discriminator name '%s' collides with "
-                                    "enum value in '%s'" %
-                                    (discriminator, enum_define["enum_name"]))

         # Otherwise, check for conflicts in the generated enum
         else:
@@ -690,8 +663,6 @@ def check_struct(expr, expr_info):
                allow_dict=True, allow_optional=True)
     check_type(expr_info, "'base' for struct '%s'" % name, expr.get('base'),
                allow_metas=['struct'])
-    if expr.get('base'):
-        check_member_clash(expr_info, expr['base'], expr['data'])


 def check_keys(expr_elem, meta, required, optional=[]):
@@ -1095,16 +1066,30 @@ class QAPISchemaObjectTypeVariants(object):
         assert isinstance(self.tag_member.type, QAPISchemaEnumType)
         for v in self.variants:
             vseen = dict(seen)
-            v.check(schema, info, self.tag_member.type, vseen)
+            v.check(schema, info, self.tag_member.type, vseen,
+                    bool(self.tag_name))


 class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
     def __init__(self, name, typ):
         QAPISchemaObjectTypeMember.__init__(self, name, typ, False)

-    def check(self, schema, info, tag_type, seen):
+    def check(self, schema, info, tag_type, seen, flat):
         QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
         assert self.name in tag_type.values
+        if flat:
+            # For flat unions, check that each QMP member does not
+            # collide with any non-variant members. No type can
+            # contain itself as a flat variant
+            self.type.check(schema)
+            assert not self.type.variants    # not implemented
+            for m in self.type.members:
+                if m.c_name() in seen:
+                    raise QAPIExprError(info,
+                                        "Member '%s' of branch '%s' collides "
+                                        "with %s"
+                                        % (m.name, self.name,
+                                           seen[m.c_name()].describe()))

     # This function exists to support ugly simple union special cases
     # TODO get rid of them, and drop the function
diff --git a/tests/qapi-schema/flat-union-clash-member.err b/tests/qapi-schema/flat-union-clash-member.err
index 2f0397a..57dd478 100644
--- a/tests/qapi-schema/flat-union-clash-member.err
+++ b/tests/qapi-schema/flat-union-clash-member.err
@@ -1 +1 @@
-tests/qapi-schema/flat-union-clash-member.json:11: Member name 'name' of branch 'value1' clashes with base 'Base'
+tests/qapi-schema/flat-union-clash-member.json:11: Member 'name' of branch 'value1' collides with 'name' (member of Base)
diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err
index b44dd40..3f3248b 100644
--- a/tests/qapi-schema/flat-union-clash-type.err
+++ b/tests/qapi-schema/flat-union-clash-type.err
@@ -1 +1 @@
-tests/qapi-schema/flat-union-clash-type.json:11: Discriminator name 'type' collides with enum value in 'TestEnum'
+tests/qapi-schema/flat-union-clash-type.json:11: 'type' (branch of TestUnion) collides with 'type' (member of Base)
diff --git a/tests/qapi-schema/struct-base-clash-deep.err b/tests/qapi-schema/struct-base-clash-deep.err
index f7a25a3..e2d7943 100644
--- a/tests/qapi-schema/struct-base-clash-deep.err
+++ b/tests/qapi-schema/struct-base-clash-deep.err
@@ -1 +1 @@
-tests/qapi-schema/struct-base-clash-deep.json:10: Member name 'name' clashes with base 'Base'
+tests/qapi-schema/struct-base-clash-deep.json:10: 'name' (member of Sub) collides with 'name' (member of Base)
diff --git a/tests/qapi-schema/struct-base-clash.err b/tests/qapi-schema/struct-base-clash.err
index 3a9f66b..c52f33d 100644
--- a/tests/qapi-schema/struct-base-clash.err
+++ b/tests/qapi-schema/struct-base-clash.err
@@ -1 +1 @@
-tests/qapi-schema/struct-base-clash.json:5: Member name 'name' clashes with base 'Base'
+tests/qapi-schema/struct-base-clash.json:5: 'name' (member of Sub) collides with 'name' (member of Base)
-- 
2.4.3

Re: [Qemu-devel] [PATCH v7 11/14] qapi: Move duplicate member checks to schema check()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> With the previous commit, we have two different locations for
> detecting member name clashes - one at parse time, and another
> at QAPISchema*.check() time.  Consolidate some of the checks
> into a single place, which is also in line with our TODO to
> eventually move all of the parse time semantic checking into
> the newer schema code.  The check_member_clash() function is
> no longer needed.
>
> The wording of several error messages has changed, but in many
> cases feels like an improvement rather than a regression.  The
> recent change to avoid an assertion failure when a flat union
> branch name collides with its discriminator name is also
> handled nicely by this code; but there is more work needed
> before we can detect all collisions in simple union branch
> names done by the old code.
>
> No change to generated code.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v7: comment improvements, retitle subject
> v6: rebase to earlier testsuite improvements, fold in cleanup
> of flat-union-clash-type
> ---
>  scripts/qapi.py                               | 51 ++++++++++-----------------
>  tests/qapi-schema/flat-union-clash-member.err |  2 +-
>  tests/qapi-schema/flat-union-clash-type.err   |  2 +-
>  tests/qapi-schema/struct-base-clash-deep.err  |  2 +-
>  tests/qapi-schema/struct-base-clash.err       |  2 +-
>  5 files changed, 22 insertions(+), 37 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 30f1483..9f98413 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -499,21 +499,6 @@ def check_type(expr_info, source, value, allow_array=False,
>                                  'enum'])
>
>
> -def check_member_clash(expr_info, base_name, data, source=""):
> -    base = find_struct(base_name)
> -    assert base
> -    base_members = base['data']
> -    for key in data.keys():
> -        if key.startswith('*'):
> -            key = key[1:]
> -        if key in base_members or "*" + key in base_members:
> -            raise QAPIExprError(expr_info,
> -                                "Member name '%s'%s clashes with base '%s'"
> -                                % (key, source, base_name))
> -    if base.get('base'):
> -        check_member_clash(expr_info, base['base'], data, source)
> -
> -
>  def check_command(expr, expr_info):
>      name = expr['command']
>
> @@ -592,30 +577,18 @@ def check_union(expr, expr_info):
>      for (key, value) in members.items():
>          check_name(expr_info, "Member of union '%s'" % name, key)
>
> -        # Each value must name a known type; furthermore, in flat unions,
> -        # branches must be a struct with no overlapping member names
> +        # Each value must name a known type
>          check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
>                     value, allow_array=not base, allow_metas=allow_metas)
> -        if base:
> -            branch_struct = find_struct(value)
> -            assert branch_struct
> -            check_member_clash(expr_info, base, branch_struct['data'],
> -                               " of branch '%s'" % key)
>
>          # If the discriminator names an enum type, then all members
> -        # of 'data' must also be members of the enum type, which in turn
> -        # must not collide with the discriminator name.
> +        # of 'data' must also be members of the enum type.
>          if enum_define:
>              if key not in enum_define['enum_values']:
>                  raise QAPIExprError(expr_info,
>                                      "Discriminator value '%s' is not found in "
>                                      "enum '%s'" %
>                                      (key, enum_define["enum_name"]))
> -            if discriminator in enum_define['enum_values']:
> -                raise QAPIExprError(expr_info,
> -                                    "Discriminator name '%s' collides with "
> -                                    "enum value in '%s'" %
> -                                    (discriminator, enum_define["enum_name"]))
>
>          # Otherwise, check for conflicts in the generated enum
>          else:
> @@ -690,8 +663,6 @@ def check_struct(expr, expr_info):
>                 allow_dict=True, allow_optional=True)
>      check_type(expr_info, "'base' for struct '%s'" % name, expr.get('base'),
>                 allow_metas=['struct'])
> -    if expr.get('base'):
> -        check_member_clash(expr_info, expr['base'], expr['data'])
>
>
>  def check_keys(expr_elem, meta, required, optional=[]):
> @@ -1095,16 +1066,30 @@ class QAPISchemaObjectTypeVariants(object):
>          assert isinstance(self.tag_member.type, QAPISchemaEnumType)
>          for v in self.variants:
>              vseen = dict(seen)
> -            v.check(schema, info, self.tag_member.type, vseen)
> +            v.check(schema, info, self.tag_member.type, vseen,
> +                    bool(self.tag_name))
>
>
>  class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
>      def __init__(self, name, typ):
>          QAPISchemaObjectTypeMember.__init__(self, name, typ, False)
>
> -    def check(self, schema, info, tag_type, seen):
> +    def check(self, schema, info, tag_type, seen, flat):
>          QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
>          assert self.name in tag_type.values
> +        if flat:
> +            # For flat unions, check that each QMP member does not
> +            # collide with any non-variant members. No type can
> +            # contain itself as a flat variant
> +            self.type.check(schema)
> +            assert not self.type.variants    # not implemented
> +            for m in self.type.members:
> +                if m.c_name() in seen:
> +                    raise QAPIExprError(info,
> +                                        "Member '%s' of branch '%s' collides "
> +                                        "with %s"
> +                                        % (m.name, self.name,
> +                                           seen[m.c_name()].describe()))

If our data structures were right, we wouldn't need the conditional.

Three cases:

1. Flat union

   self is a flat union object type's variant case.  self.name is a tag
   value, and self.type is an object type providing the members for this
   tag value.  It has no variants.

   Example: UserDefFlatUnion's first case, self.name is 'value1',
   self.type is type UserDefA.

   We need to check that the case's variant members (i.e. self.type's
   members) don't clash with non-variant members, because in QMP, they
   become members of the same JSON object.

2. Simple union

   self is a simple union object type's variant case.  self.name is a
   tag value, and self.type is an object type providing the members for
   this tag value.  It has no variants.  Exactly the same as 1.  The
   fact that the tag member, its type and all the variant types are
   implicitly defined is immaterial.

   Example: __org.qemu_x-Union1's first case, self.name is
   '__org.qemu_x-branch', self.type is the implicitly defined type
   :obj-str-wrapper.  It's only member is named 'data'.

   The case's variant members (i.e. self.type's members) don't clash
   with non-variant members by construction.  Since simple unions can't
   have a base, the only non-variant member is the implicitly defined
   tag.  Its name is 'type'.  Each case has exactly one variant member
   named 'data'.

   Therefore, there should be no need to special-case simple unions
   here: the check() for flat unions should work fine, it just shouldn't
   find clashes.

3. Alternate

   self is an alternate type's case.  self.name names the case.  It is
   not a tag value in QMP, because alternates are implicitly tagged in
   QMP.  It is a tag value in C.  self.type is a type.  It needn't be an
   object type.

   Example: UserDefAlternate's first case, self.name is 'uda', self.type
   is type UserDefA.

   The case's members (if any) can't clash with anything.  However, the
   check() for flat unions doesn't quite work.

   First, it assumes self.type is an object type without variants.
   That's not true for alternate cases.

   Second, it puts the implicit tag member into seen, even though it's
   not actually on the wire.

So far the theory, now practice: if I hack the code to test "not
alternate" (i.e. case 1 and 2) instead of "flat union" (just case 1),
"make check" passes except for union-clash-data.json.  There, we now
report a clash we didn't report before:

    union-clash-data.json:6: Member 'data' of branch 'data' collides with 'data' (branch of TestUnion)

The FIXME in union-clash-data.json actually asks for an error, because
'data' clashes with our stupid filler to avoid empty unions.  But that's
not what this error message reports!  I think what happens is this:
QAPISchemaObjectTypeVariant.check() adds the case name self.name (here:
'data') to seen.  When we then check the case's 'data': 'int' member, it
finds the with the case name, and reports a clash.  I can't see why it
should.

This clashing business is awfully confusing, because we have to consider
(flat unions, simple unions, alternates) times (QMP, C).

It would be simpler if we could make C clash only when QMP clashes.

I've been trying to make simple unions a genuine special case of flat
unions in part to get shrink this matrix.

We might want to separate out alternates.  Dunno.

>      # This function exists to support ugly simple union special cases
>      # TODO get rid of them, and drop the function
> diff --git a/tests/qapi-schema/flat-union-clash-member.err b/tests/qapi-schema/flat-union-clash-member.err
> index 2f0397a..57dd478 100644
> --- a/tests/qapi-schema/flat-union-clash-member.err
> +++ b/tests/qapi-schema/flat-union-clash-member.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-clash-member.json:11: Member name 'name' of branch 'value1' clashes with base 'Base'
> +tests/qapi-schema/flat-union-clash-member.json:11: Member 'name' of branch 'value1' collides with 'name' (member of Base)
> diff --git a/tests/qapi-schema/flat-union-clash-type.err b/tests/qapi-schema/flat-union-clash-type.err
> index b44dd40..3f3248b 100644
> --- a/tests/qapi-schema/flat-union-clash-type.err
> +++ b/tests/qapi-schema/flat-union-clash-type.err
> @@ -1 +1 @@
> -tests/qapi-schema/flat-union-clash-type.json:11: Discriminator name 'type' collides with enum value in 'TestEnum'
> +tests/qapi-schema/flat-union-clash-type.json:11: 'type' (branch of TestUnion) collides with 'type' (member of Base)
> diff --git a/tests/qapi-schema/struct-base-clash-deep.err b/tests/qapi-schema/struct-base-clash-deep.err
> index f7a25a3..e2d7943 100644
> --- a/tests/qapi-schema/struct-base-clash-deep.err
> +++ b/tests/qapi-schema/struct-base-clash-deep.err
> @@ -1 +1 @@
> -tests/qapi-schema/struct-base-clash-deep.json:10: Member name 'name' clashes with base 'Base'
> +tests/qapi-schema/struct-base-clash-deep.json:10: 'name' (member of Sub) collides with 'name' (member of Base)
> diff --git a/tests/qapi-schema/struct-base-clash.err b/tests/qapi-schema/struct-base-clash.err
> index 3a9f66b..c52f33d 100644
> --- a/tests/qapi-schema/struct-base-clash.err
> +++ b/tests/qapi-schema/struct-base-clash.err
> @@ -1 +1 @@
> -tests/qapi-schema/struct-base-clash.json:5: Member name 'name' clashes with base 'Base'
> +tests/qapi-schema/struct-base-clash.json:5: 'name' (member of Sub) collides with 'name' (member of Base)

Re: [Qemu-devel] [PATCH v7 11/14] qapi: Move duplicate member checks to schema check()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4381 bytes --]

On 10/12/2015 09:53 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> With the previous commit, we have two different locations for
>> detecting member name clashes - one at parse time, and another
>> at QAPISchema*.check() time.  Consolidate some of the checks
>> into a single place, which is also in line with our TODO to
>> eventually move all of the parse time semantic checking into
>> the newer schema code.  The check_member_clash() function is
>> no longer needed.
>>

>> +    def check(self, schema, info, tag_type, seen, flat):
>>          QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)
>>          assert self.name in tag_type.values
>> +        if flat:
>> +            # For flat unions, check that each QMP member does not
>> +            # collide with any non-variant members. No type can
>> +            # contain itself as a flat variant
>> +            self.type.check(schema)
>> +            assert not self.type.variants    # not implemented
>> +            for m in self.type.members:
>> +                if m.c_name() in seen:
>> +                    raise QAPIExprError(info,
>> +                                        "Member '%s' of branch '%s' collides "
>> +                                        "with %s"
>> +                                        % (m.name, self.name,
>> +                                           seen[m.c_name()].describe()))
> 
> If our data structures were right, we wouldn't need the conditional.
> 

Okay, you've made a good point. The only conditional that is appropriate
here is 'if not alternate', because you are right that flat and simple
unions should behave identically (and merely that simple unions won't
detect any collisions, because flattening their implicit struct with a
single 'data' member shouldn't introduce a conflict in QMP).


> So far the theory, now practice: if I hack the code to test "not
> alternate" (i.e. case 1 and 2) instead of "flat union" (just case 1),
> "make check" passes except for union-clash-data.json.  There, we now
> report a clash we didn't report before:
> 
>     union-clash-data.json:6: Member 'data' of branch 'data' collides with 'data' (branch of TestUnion)

Hmm, I'll have to investigate before posting v8. At one point, I wired
in debug statements into the generator to show the contents of seen[]
through each call to check(), to see where collisions are coming from;
looks like I get to play with that again.

> 
> The FIXME in union-clash-data.json actually asks for an error, because
> 'data' clashes with our stupid filler to avoid empty unions.  But that's
> not what this error message reports!  I think what happens is this:
> QAPISchemaObjectTypeVariant.check() adds the case name self.name (here:
> 'data') to seen.  When we then check the case's 'data': 'int' member, it
> finds the with the case name, and reports a clash.  I can't see why it
> should.
> 
> This clashing business is awfully confusing, because we have to consider
> (flat unions, simple unions, alternates) times (QMP, C).
> 
> It would be simpler if we could make C clash only when QMP clashes.

If a QMP clash always caused a C clash, life would be a bit simpler. We
aren't going to get there (because in a flat union, hiding the members
of the branch type behind a single pointer means those members don't
clash with any C names of the overall union), but I can certainly try to
make the comments explain what is going on.

> 
> We might want to separate out alternates.  Dunno.

And to some extent, subset C already does some of that separation
(because I try to convert from 'FooKind type' to 'qtype_code type'
without even creating an implicit 'FooKind' enum).  It sounds like
you're okay with any well-documented TODO warts related to alternates
for the purposes of this subset B, especially if I can then clean those
warts back up in subset C.  But what v8 of subset B needs to avoid is
any warts based on simple vs. flat unions.  I can live with that.

I'm still trying to figure out if hoisting the kind=>type rename into
subset B makes life easier or harder (it certainly causes me more rebase
churn, but that's not necessarily a bad thing).

-- 
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 --]

Re: [Qemu-devel] [PATCH v7 11/14] qapi: Move duplicate member checks to schema check()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3184 bytes --]

On 10/12/2015 10:22 AM, Eric Blake wrote:

> 
>>> +    def check(self, schema, info, tag_type, seen, flat):
>>>          QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)

seen gets modified here...

>>>          assert self.name in tag_type.values
>>> +        if flat:
>>> +            # For flat unions, check that each QMP member does not
>>> +            # collide with any non-variant members. No type can
>>> +            # contain itself as a flat variant
>>> +            self.type.check(schema)
>>> +            assert not self.type.variants    # not implemented
>>> +            for m in self.type.members:
>>> +                if m.c_name() in seen:

...and it is the modified seen here that causes...

>>  union-clash-data.json:6: Member 'data' of branch 'data' collides with 'data' (branch of TestUnion)

...the incorrect error.

>>
>> The FIXME in union-clash-data.json actually asks for an error, because
>> 'data' clashes with our stupid filler to avoid empty unions.  But that's
>> not what this error message reports!  I think what happens is this:
>> QAPISchemaObjectTypeVariant.check() adds the case name self.name (here:
>> 'data') to seen.  When we then check the case's 'data': 'int' member, it
>> finds the with the case name, and reports a clash.  I can't see why it
>> should.
>>
>> This clashing business is awfully confusing, because we have to consider
>> (flat unions, simple unions, alternates) times (QMP, C).
>>
>> It would be simpler if we could make C clash only when QMP clashes.
> 
> If a QMP clash always caused a C clash, life would be a bit simpler. We
> aren't going to get there (because in a flat union, hiding the members
> of the branch type behind a single pointer means those members don't
> clash with any C names of the overall union), but I can certainly try to
> make the comments explain what is going on.

I've managed to fix it with a dict(seen) and some comments, and will
post a v8.  Great catch, by the way.

> 
>>
>> We might want to separate out alternates.  Dunno.
> 
> And to some extent, subset C already does some of that separation
> (because I try to convert from 'FooKind type' to 'qtype_code type'
> without even creating an implicit 'FooKind' enum).  It sounds like
> you're okay with any well-documented TODO warts related to alternates
> for the purposes of this subset B, especially if I can then clean those
> warts back up in subset C.  But what v8 of subset B needs to avoid is
> any warts based on simple vs. flat unions.  I can live with that.
> 
> I'm still trying to figure out if hoisting the kind=>type rename into
> subset B makes life easier or harder (it certainly causes me more rebase
> churn, but that's not necessarily a bad thing).

At this point, it was easier to live with a temporary hack for
QAPISchemaObjectTypeUnionTag than it was to rebase my kind=>type rename
patch to be this early in the overall series. We'll get there, but I
agree with the approach we've been taking of one subset at a time.

-- 
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 --]

Re: [Qemu-devel] [PATCH v7 11/14] qapi: Move duplicate member checks to schema check()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/12/2015 09:53 AM, Markus Armbruster wrote:
[...]
>> It would be simpler if we could make C clash only when QMP clashes.
>
> If a QMP clash always caused a C clash, life would be a bit simpler. We
> aren't going to get there (because in a flat union, hiding the members
> of the branch type behind a single pointer means those members don't
> clash with any C names of the overall union), but I can certainly try to
> make the comments explain what is going on.

(1) If QMP clashed exactly when C clashed, we'd have to think only about
one of them, and the C compiler would check for clashes statically.

(2) If a QMP clash implied a C clash, we'd only have to think about C,
and the C compiler would check statically.

(3) If a C clash implied a QMP clash, we'd only have to think about QMP.

(4) If they can clash independently, we need to think about both
independently.

We currently have (4), and having to juggle both QMP and C in my mind
has been taxing.

My remark was about (3): C clashes only when QMP clashes <=> C clash
implies QMP clash.

Your remark was about (2).  More useful than (3), but as you say not
feasible.

Do you think we can get to (3)?

>> We might want to separate out alternates.  Dunno.
>
> And to some extent, subset C already does some of that separation
> (because I try to convert from 'FooKind type' to 'qtype_code type'
> without even creating an implicit 'FooKind' enum).  It sounds like
> you're okay with any well-documented TODO warts related to alternates
> for the purposes of this subset B, especially if I can then clean those
> warts back up in subset C.  But what v8 of subset B needs to avoid is
> any warts based on simple vs. flat unions.  I can live with that.

I'm prepared to accept rephrased existing warts and additional temporary
warts when I understand why simply fixing them isn't practical or
economical.  A certain amount of hand-waving is okay.

> I'm still trying to figure out if hoisting the kind=>type rename into
> subset B makes life easier or harder (it certainly causes me more rebase
> churn, but that's not necessarily a bad thing).

Thanks!

Re: [Qemu-devel] [PATCH v7 11/14] qapi: Move duplicate member checks to schema check()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2989 bytes --]

On 10/13/2015 01:08 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 10/12/2015 09:53 AM, Markus Armbruster wrote:
> [...]
>>> It would be simpler if we could make C clash only when QMP clashes.
>>
>> If a QMP clash always caused a C clash, life would be a bit simpler. We
>> aren't going to get there (because in a flat union, hiding the members
>> of the branch type behind a single pointer means those members don't
>> clash with any C names of the overall union), but I can certainly try to
>> make the comments explain what is going on.
> 
> (1) If QMP clashed exactly when C clashed, we'd have to think only about
> one of them, and the C compiler would check for clashes statically.

Although deferring the error about the clash until compile time is a bit
long, compared to reporting the clash at generator time.

> 
> (2) If a QMP clash implied a C clash, we'd only have to think about C,
> and the C compiler would check statically.
> 
> (3) If a C clash implied a QMP clash, we'd only have to think about QMP.
> 
> (4) If they can clash independently, we need to think about both
> independently.
> 
> We currently have (4), and having to juggle both QMP and C in my mind
> has been taxing.
> 
> My remark was about (3): C clashes only when QMP clashes <=> C clash
> implies QMP clash.
> 
> Your remark was about (2).  More useful than (3), but as you say not
> feasible.
> 
> Do you think we can get to (3)?

C clash that does not imply a QMP clash:

{ 'union':'U', 'data':{ 'a':'int', 'A':'str' } }

C clash (the implicit enum has two U_KIND_A values) but not a QMP clash
('a' and 'A' don't even appear in QMP; and even if they did, they are
distinct in C as branch names).  Might be possible to avoid the C clash
by munging case labels of the generated enum differently, but I'm not
sure it is worth the effort.

QMP clash that does not (currently) imply a C clash (using abbreviated
notation):

{ 'union':'U', 'base':{ 'type':'Enum', 'member':'int' },
  'discriminator':'type',
  'data':{ 'branch':{ 'member':'str' } } }

QMP clash (the field 'member' is part of the base type, and also part of
the 'branch' variant type), but not a C clash (because the C layout
accesses the branch through a box named 'branch').

But we cannot just remove the boxing, either, because then we'd have a
clash in C that is NOT a clash in QMP:

{ 'union':'U', 'base':{ 'type':'Enum' }, 'discriminator':'type',
'data':{ 'branch1':{ 'member':'str' }, 'branch2': { 'member':'int' } } }

If the two 'member' fields were at the same C level as 'type', then we
could use C collisions to detect a QMP clash between a variant's members
and the non-variant fields - but it also has the drawback of declaring a
C collision between the two branches.

So no, I don't think we can get to (3).

-- 
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 --]

Re: [Qemu-devel] [PATCH v7 11/14] qapi: Move duplicate member checks to schema check()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/13/2015 01:08 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> On 10/12/2015 09:53 AM, Markus Armbruster wrote:
>> [...]
>>>> It would be simpler if we could make C clash only when QMP clashes.
>>>
>>> If a QMP clash always caused a C clash, life would be a bit simpler. We
>>> aren't going to get there (because in a flat union, hiding the members
>>> of the branch type behind a single pointer means those members don't
>>> clash with any C names of the overall union), but I can certainly try to
>>> make the comments explain what is going on.
>> 
>> (1) If QMP clashed exactly when C clashed, we'd have to think only about
>> one of them, and the C compiler would check for clashes statically.
>
> Although deferring the error about the clash until compile time is a bit
> long, compared to reporting the clash at generator time.

I'd view the C compiler as insurance.  The generators should still
check.

>> (2) If a QMP clash implied a C clash, we'd only have to think about C,
>> and the C compiler would check statically.
>> 
>> (3) If a C clash implied a QMP clash, we'd only have to think about QMP.
>> 
>> (4) If they can clash independently, we need to think about both
>> independently.
>> 
>> We currently have (4), and having to juggle both QMP and C in my mind
>> has been taxing.
>> 
>> My remark was about (3): C clashes only when QMP clashes <=> C clash
>> implies QMP clash.
>> 
>> Your remark was about (2).  More useful than (3), but as you say not
>> feasible.
>> 
>> Do you think we can get to (3)?
>
> C clash that does not imply a QMP clash:
>
> { 'union':'U', 'data':{ 'a':'int', 'A':'str' } }
>
> C clash (the implicit enum has two U_KIND_A values) but not a QMP clash
> ('a' and 'A' don't even appear in QMP; and even if they did, they are
> distinct in C as branch names).  Might be possible to avoid the C clash
> by munging case labels of the generated enum differently, but I'm not
> sure it is worth the effort.
>
> QMP clash that does not (currently) imply a C clash (using abbreviated
> notation):
>
> { 'union':'U', 'base':{ 'type':'Enum', 'member':'int' },
>   'discriminator':'type',
>   'data':{ 'branch':{ 'member':'str' } } }
>
> QMP clash (the field 'member' is part of the base type, and also part of
> the 'branch' variant type), but not a C clash (because the C layout
> accesses the branch through a box named 'branch').
>
> But we cannot just remove the boxing, either, because then we'd have a
> clash in C that is NOT a clash in QMP:
>
> { 'union':'U', 'base':{ 'type':'Enum' }, 'discriminator':'type',
> 'data':{ 'branch1':{ 'member':'str' }, 'branch2': { 'member':'int' } } }
>
> If the two 'member' fields were at the same C level as 'type', then we
> could use C collisions to detect a QMP clash between a variant's members
> and the non-variant fields - but it also has the drawback of declaring a
> C collision between the two branches.
>
> So no, I don't think we can get to (3).

Scratch that then.  My head hurts.

[Qemu-devel] [PATCH v7 12/14] qapi: Move duplicate enum value checks to schema check()

[Eric Blake]

Similar to the previous commit, move the detection of a collision
in enum values from parse time to QAPISchemaEnumType.check().
This happens to also detect collisions in union branch names,
so for a decent error message, we have to determine if the enum
is implicit (and if so where the real collision lies).

Testing this showed that the test union-bad-branch and
union-clash-branches were basically testing the same thing;
with the minor difference that the former clashes only in the
enum, while the latter also clashes in the C union member
names that would be generated. So delete the weaker test.

No change to generated code.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v7: retitle and improve commit message; earlier subclass patches
avoid problem with detecting 'kind' collision
v6: new patch
---
 scripts/qapi.py                            | 47 +++++++++++++-----------------
 tests/Makefile                             |  1 -
 tests/qapi-schema/alternate-clash.err      |  2 +-
 tests/qapi-schema/enum-clash-member.err    |  2 +-
 tests/qapi-schema/enum-max-member.err      |  2 +-
 tests/qapi-schema/union-bad-branch.err     |  1 -
 tests/qapi-schema/union-bad-branch.exit    |  1 -
 tests/qapi-schema/union-bad-branch.json    |  8 -----
 tests/qapi-schema/union-bad-branch.out     |  0
 tests/qapi-schema/union-clash-branches.err |  2 +-
 tests/qapi-schema/union-clash-type.err     |  2 +-
 tests/qapi-schema/union-max.err            |  2 +-
 12 files changed, 26 insertions(+), 44 deletions(-)
 delete mode 100644 tests/qapi-schema/union-bad-branch.err
 delete mode 100644 tests/qapi-schema/union-bad-branch.exit
 delete mode 100644 tests/qapi-schema/union-bad-branch.json
 delete mode 100644 tests/qapi-schema/union-bad-branch.out

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 9f98413..55aca40 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -530,7 +530,6 @@ def check_union(expr, expr_info):
     base = expr.get('base')
     discriminator = expr.get('discriminator')
     members = expr['data']
-    values = {'MAX': '(automatic)', 'KIND': '(automatic)'}

     # Two types of unions, determined by discriminator.

@@ -590,34 +589,16 @@ def check_union(expr, expr_info):
                                     "enum '%s'" %
                                     (key, enum_define["enum_name"]))

-        # Otherwise, check for conflicts in the generated enum
-        else:
-            c_key = camel_to_upper(key)
-            if c_key in values:
-                raise QAPIExprError(expr_info,
-                                    "Union '%s' member '%s' clashes with '%s'"
-                                    % (name, key, values[c_key]))
-            values[c_key] = key
-

 def check_alternate(expr, expr_info):
     name = expr['alternate']
     members = expr['data']
-    values = {'MAX': '(automatic)'}
     types_seen = {}

     # Check every branch
     for (key, value) in members.items():
         check_name(expr_info, "Member of alternate '%s'" % name, key)

-        # Check for conflicts in the generated enum
-        c_key = camel_to_upper(key)
-        if c_key in values:
-            raise QAPIExprError(expr_info,
-                                "Alternate '%s' member '%s' clashes with '%s'"
-                                % (name, key, values[c_key]))
-        values[c_key] = key
-
         # Ensure alternates have no type conflicts.
         check_type(expr_info, "Member '%s' of alternate '%s'" % (key, name),
                    value,
@@ -636,7 +617,6 @@ def check_enum(expr, expr_info):
     name = expr['enum']
     members = expr.get('data')
     prefix = expr.get('prefix')
-    values = {'MAX': '(automatic)'}

     if not isinstance(members, list):
         raise QAPIExprError(expr_info,
@@ -647,12 +627,6 @@ def check_enum(expr, expr_info):
     for member in members:
         check_name(expr_info, "Member of enum '%s'" % name, member,
                    enum_member=True)
-        key = camel_to_upper(member)
-        if key in values:
-            raise QAPIExprError(expr_info,
-                                "Enum '%s' member '%s' clashes with '%s'"
-                                % (name, member, values[key]))
-        values[key] = member


 def check_struct(expr, expr_info):
@@ -887,7 +861,26 @@ class QAPISchemaEnumType(QAPISchemaType):
         self.prefix = prefix

     def check(self, schema):
-        assert len(set(self.values)) == len(self.values)
+        # Check for collisions on the generated C enum values
+        seen = {c_enum_const(self.name, 'MAX'): '(automatic MAX)'}
+        for value in self.values:
+            c_value = c_enum_const(self.name, value)
+            if c_value in seen:
+                # If the enum is implicit, report the error on behalf of
+                # the union or alternate that triggered the enum
+                if self.is_implicit():
+                    owner = schema.lookup_type(self.name[:-4])
+                    assert owner
+                    if isinstance(owner, QAPISchemaAlternateType):
+                        description = "Alternate '%s' branch" % owner.name
+                    else:
+                        description = "Union '%s' branch" % owner.name
+                else:
+                    description = "Enum '%s' value" % self.name
+                raise QAPIExprError(self.info,
+                                    "%s '%s' clashes with '%s'"
+                                    % (description, value, seen[c_value]))
+            seen[c_value] = value

     def _is_implicit(self):
         # See QAPISchema._make_implicit_enum_type()
diff --git a/tests/Makefile b/tests/Makefile
index d18b3b2..b85ce48 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -331,7 +331,6 @@ qapi-schema += unclosed-list.json
 qapi-schema += unclosed-object.json
 qapi-schema += unclosed-string.json
 qapi-schema += unicode-str.json
-qapi-schema += union-bad-branch.json
 qapi-schema += union-base-no-discriminator.json
 qapi-schema += union-clash-branches.json
 qapi-schema += union-clash-data.json
diff --git a/tests/qapi-schema/alternate-clash.err b/tests/qapi-schema/alternate-clash.err
index a475ab6..7fd3069 100644
--- a/tests/qapi-schema/alternate-clash.err
+++ b/tests/qapi-schema/alternate-clash.err
@@ -1 +1 @@
-tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' member 'a_b' clashes with 'a-b'
+tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' branch 'a_b' clashes with 'a-b'
diff --git a/tests/qapi-schema/enum-clash-member.err b/tests/qapi-schema/enum-clash-member.err
index 48bd136..84030c5 100644
--- a/tests/qapi-schema/enum-clash-member.err
+++ b/tests/qapi-schema/enum-clash-member.err
@@ -1 +1 @@
-tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' member 'ONE' clashes with 'one'
+tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' value 'ONE' clashes with 'one'
diff --git a/tests/qapi-schema/enum-max-member.err b/tests/qapi-schema/enum-max-member.err
index f77837f..6b9ef9b 100644
--- a/tests/qapi-schema/enum-max-member.err
+++ b/tests/qapi-schema/enum-max-member.err
@@ -1 +1 @@
-tests/qapi-schema/enum-max-member.json:3: Enum 'MyEnum' member 'max' clashes with '(automatic)'
+tests/qapi-schema/enum-max-member.json:3: Enum 'MyEnum' value 'max' clashes with '(automatic MAX)'
diff --git a/tests/qapi-schema/union-bad-branch.err b/tests/qapi-schema/union-bad-branch.err
deleted file mode 100644
index 8822735..0000000
--- a/tests/qapi-schema/union-bad-branch.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/union-bad-branch.json:6: Union 'MyUnion' member 'ONE' clashes with 'one'
diff --git a/tests/qapi-schema/union-bad-branch.exit b/tests/qapi-schema/union-bad-branch.exit
deleted file mode 100644
index d00491f..0000000
--- a/tests/qapi-schema/union-bad-branch.exit
+++ /dev/null
@@ -1 +0,0 @@
-1
diff --git a/tests/qapi-schema/union-bad-branch.json b/tests/qapi-schema/union-bad-branch.json
deleted file mode 100644
index 913aa38..0000000
--- a/tests/qapi-schema/union-bad-branch.json
+++ /dev/null
@@ -1,8 +0,0 @@
-# we reject normal unions where branches would collide in C
-{ 'struct': 'One',
-  'data': { 'string': 'str' } }
-{ 'struct': 'Two',
-  'data': { 'number': 'int' } }
-{ 'union': 'MyUnion',
-  'data': { 'one': 'One',
-            'ONE': 'Two' } }
diff --git a/tests/qapi-schema/union-bad-branch.out b/tests/qapi-schema/union-bad-branch.out
deleted file mode 100644
index e69de29..0000000
diff --git a/tests/qapi-schema/union-clash-branches.err b/tests/qapi-schema/union-clash-branches.err
index 005c48d..d8f1265 100644
--- a/tests/qapi-schema/union-clash-branches.err
+++ b/tests/qapi-schema/union-clash-branches.err
@@ -1 +1 @@
-tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' member 'a_b' clashes with 'a-b'
+tests/qapi-schema/union-clash-branches.json:4: Union 'TestUnion' branch 'a_b' clashes with 'a-b'
diff --git a/tests/qapi-schema/union-clash-type.err b/tests/qapi-schema/union-clash-type.err
index a5dead1..ce7f8cd 100644
--- a/tests/qapi-schema/union-clash-type.err
+++ b/tests/qapi-schema/union-clash-type.err
@@ -1 +1 @@
-tests/qapi-schema/union-clash-type.json:8: Union 'TestUnion' member 'kind' clashes with '(automatic)'
+tests/qapi-schema/union-clash-type.json:8: 'kind' (branch of TestUnion) collides with 'kind' (implicit tag of TestUnion)
diff --git a/tests/qapi-schema/union-max.err b/tests/qapi-schema/union-max.err
index 55ce439..b93beae 100644
--- a/tests/qapi-schema/union-max.err
+++ b/tests/qapi-schema/union-max.err
@@ -1 +1 @@
-tests/qapi-schema/union-max.json:2: Union 'Union' member 'max' clashes with '(automatic)'
+tests/qapi-schema/union-max.json:2: Union 'Union' branch 'max' clashes with '(automatic MAX)'
-- 
2.4.3

[Qemu-devel] [PATCH v7 13/14] qapi: Add test for alternate branch 'kind' clash

[Eric Blake]

Rename alternate-clash to alternate-clash-members, and add a
new test alternate-clash-type.  While similar to the earlier
addition of union-clash-type, we have one major difference: a
future patch will be simplifying alternates to not need an
implict AlternateKind enum, but we still need to detect the
collision with the resulting C 'qtype_code type;' tag.

No change to generated code.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v7: retitle 10/12 and limit to just testsuite changes
v6: New patch
---
 tests/Makefile                                                 |  3 ++-
 tests/qapi-schema/alternate-clash-members.err                  |  1 +
 .../{alternate-clash.exit => alternate-clash-members.exit}     |  0
 .../{alternate-clash.json => alternate-clash-members.json}     |  0
 .../{alternate-clash.out => alternate-clash-members.out}       |  0
 tests/qapi-schema/alternate-clash-type.err                     |  1 +
 tests/qapi-schema/alternate-clash-type.exit                    |  1 +
 tests/qapi-schema/alternate-clash-type.json                    | 10 ++++++++++
 tests/qapi-schema/alternate-clash-type.out                     |  0
 tests/qapi-schema/alternate-clash.err                          |  1 -
 10 files changed, 15 insertions(+), 2 deletions(-)
 create mode 100644 tests/qapi-schema/alternate-clash-members.err
 rename tests/qapi-schema/{alternate-clash.exit => alternate-clash-members.exit} (100%)
 rename tests/qapi-schema/{alternate-clash.json => alternate-clash-members.json} (100%)
 rename tests/qapi-schema/{alternate-clash.out => alternate-clash-members.out} (100%)
 create mode 100644 tests/qapi-schema/alternate-clash-type.err
 create mode 100644 tests/qapi-schema/alternate-clash-type.exit
 create mode 100644 tests/qapi-schema/alternate-clash-type.json
 create mode 100644 tests/qapi-schema/alternate-clash-type.out
 delete mode 100644 tests/qapi-schema/alternate-clash.err

diff --git a/tests/Makefile b/tests/Makefile
index b85ce48..b721a18 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -223,7 +223,8 @@ $(foreach target,$(SYSEMU_TARGET_LIST), \

 qapi-schema += alternate-array.json
 qapi-schema += alternate-base.json
-qapi-schema += alternate-clash.json
+qapi-schema += alternate-clash-members.json
+qapi-schema += alternate-clash-type.json
 qapi-schema += alternate-conflict-dict.json
 qapi-schema += alternate-conflict-string.json
 qapi-schema += alternate-empty.json
diff --git a/tests/qapi-schema/alternate-clash-members.err b/tests/qapi-schema/alternate-clash-members.err
new file mode 100644
index 0000000..0adf737
--- /dev/null
+++ b/tests/qapi-schema/alternate-clash-members.err
@@ -0,0 +1 @@
+tests/qapi-schema/alternate-clash-members.json:7: Alternate 'Alt1' branch 'a_b' clashes with 'a-b'
diff --git a/tests/qapi-schema/alternate-clash.exit b/tests/qapi-schema/alternate-clash-members.exit
similarity index 100%
rename from tests/qapi-schema/alternate-clash.exit
rename to tests/qapi-schema/alternate-clash-members.exit
diff --git a/tests/qapi-schema/alternate-clash.json b/tests/qapi-schema/alternate-clash-members.json
similarity index 100%
rename from tests/qapi-schema/alternate-clash.json
rename to tests/qapi-schema/alternate-clash-members.json
diff --git a/tests/qapi-schema/alternate-clash.out b/tests/qapi-schema/alternate-clash-members.out
similarity index 100%
rename from tests/qapi-schema/alternate-clash.out
rename to tests/qapi-schema/alternate-clash-members.out
diff --git a/tests/qapi-schema/alternate-clash-type.err b/tests/qapi-schema/alternate-clash-type.err
new file mode 100644
index 0000000..cdd2090
--- /dev/null
+++ b/tests/qapi-schema/alternate-clash-type.err
@@ -0,0 +1 @@
+tests/qapi-schema/alternate-clash-type.json:9: 'kind' (branch of Alt1) collides with 'kind' (implicit tag of Alt1)
diff --git a/tests/qapi-schema/alternate-clash-type.exit b/tests/qapi-schema/alternate-clash-type.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/alternate-clash-type.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/alternate-clash-type.json b/tests/qapi-schema/alternate-clash-type.json
new file mode 100644
index 0000000..629584b
--- /dev/null
+++ b/tests/qapi-schema/alternate-clash-type.json
@@ -0,0 +1,10 @@
+# Alternate branch 'type'
+# Reject this, because we would have a clash in generated C, between the
+# alternate's implicit tag member 'kind' and the branch name 'kind'
+# within the alternate.
+# TODO: Even if alternates are simplified in the future to use a simpler
+# 'qtype_code type' tag, rather than a full QAPISchemaObjectTypeMember,
+# we must still flag the collision, or else munge the generated C branch
+# names to allow compilation.
+{ 'alternate': 'Alt1',
+  'data': { 'kind': 'str', 'type': 'int' } }
diff --git a/tests/qapi-schema/alternate-clash-type.out b/tests/qapi-schema/alternate-clash-type.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/alternate-clash.err b/tests/qapi-schema/alternate-clash.err
deleted file mode 100644
index 7fd3069..0000000
--- a/tests/qapi-schema/alternate-clash.err
+++ /dev/null
@@ -1 +0,0 @@
-tests/qapi-schema/alternate-clash.json:7: Alternate 'Alt1' branch 'a_b' clashes with 'a-b'
-- 
2.4.3

[Qemu-devel] [PATCH v7 14/14] qapi: Detect base class loops

[Eric Blake]

It should be fairly obvious that qapi base classes need to
form an acyclic graph, since QMP cannot specify the same
key more than once, while base classes are included as flat
members alongside other members added by the child.  But the
old check_member_clash() parser function was not prepared to
check for this, and entered an infinite recursion (at least
until python gives up about too deep of nesting).

Now that check_member_clash() has been recently removed,
attempts at self-inheritance trigger an assertion failure
introduced by commit ac88219a.  The obvious fix is to turn
the assertion into a conditional.

This patch includes both the test and the fix, since the .err
file output for unbounded recursion is platform-specific (no
guarantees at what limit it will quit trying to recurse) and
rather large for how little useful content it provides.

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v7: improve commit message
v6: rebase to earlier info changes
---
 scripts/qapi.py                   | 6 +++++-
 tests/Makefile                    | 1 +
 tests/qapi-schema/base-cycle.err  | 1 +
 tests/qapi-schema/base-cycle.exit | 1 +
 tests/qapi-schema/base-cycle.json | 3 +++
 tests/qapi-schema/base-cycle.out  | 0
 6 files changed, 11 insertions(+), 1 deletion(-)
 create mode 100644 tests/qapi-schema/base-cycle.err
 create mode 100644 tests/qapi-schema/base-cycle.exit
 create mode 100644 tests/qapi-schema/base-cycle.json
 create mode 100644 tests/qapi-schema/base-cycle.out

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 55aca40..13deba5 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -942,7 +942,11 @@ class QAPISchemaObjectType(QAPISchemaType):
         self.members = None

     def check(self, schema):
-        assert self.members is not False        # not running in cycles
+        if self.members is False:               # check for cycles
+            assert self._base_name
+            raise QAPIExprError(self.info,
+                                "Object %s cyclically depends on %s"
+                                % (self.name, self._base_name))
         if self.members:
             return
         self.members = False                    # mark as being checked
diff --git a/tests/Makefile b/tests/Makefile
index b721a18..c11ee78 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -248,6 +248,7 @@ qapi-schema += bad-ident.json
 qapi-schema += bad-type-bool.json
 qapi-schema += bad-type-dict.json
 qapi-schema += bad-type-int.json
+qapi-schema += base-cycle.json
 qapi-schema += command-int.json
 qapi-schema += comments.json
 qapi-schema += double-data.json
diff --git a/tests/qapi-schema/base-cycle.err b/tests/qapi-schema/base-cycle.err
new file mode 100644
index 0000000..e0221b5
--- /dev/null
+++ b/tests/qapi-schema/base-cycle.err
@@ -0,0 +1 @@
+tests/qapi-schema/base-cycle.json:2: Object Base1 cyclically depends on Base2
diff --git a/tests/qapi-schema/base-cycle.exit b/tests/qapi-schema/base-cycle.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/base-cycle.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/base-cycle.json b/tests/qapi-schema/base-cycle.json
new file mode 100644
index 0000000..2866772
--- /dev/null
+++ b/tests/qapi-schema/base-cycle.json
@@ -0,0 +1,3 @@
+# we reject a loop in base classes
+{ 'struct': 'Base1', 'base': 'Base2', 'data': {} }
+{ 'struct': 'Base2', 'base': 'Base1', 'data': {} }
diff --git a/tests/qapi-schema/base-cycle.out b/tests/qapi-schema/base-cycle.out
new file mode 100644
index 0000000..e69de29
-- 
2.4.3