[Qemu-devel] [PATCH v9 0/5] qapi collision checking (post-introspection cleanups, subset D)

[Qemu-devel] [PATCH v9 0/5] qapi collision checking (post-introspection cleanups, subset D)

[Eric Blake]

Pending prerequisite: Markus' qapi-next branch (subset B)
git://repo.or.cz/qemu/armbru.git qapi-next
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06417.html

Pending prerequisite: Subset C
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06674.html

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

as well as part my qapi branch subject to rebasing:
http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi

v9:
Pick up the remaining patches that were part of Subset B v8 but
dropped in later spins of that subset.  Rebase things on top of
the work to redo the layout of unions and alternates.  Definitely
simpler now; here's a comparison to the earlier v8 tree:

001/5:[0024] [FC] 'qapi: Track owner of each object member'
002/5:[0090] [FC] 'qapi: Detect collisions in C member names'
003/5:[0060] [FC] 'qapi: Move duplicate member checks to schema check()'
004/5:[0053] [FC] 'qapi: Move duplicate enum value checks to schema check()'
005/5:[----] [-C] 'qapi: Detect base class loops'

v8 notes:
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02879.html
Address review comments, including fixing a bug with tracking
branch name collisions. Include a few more simple test cleanups
(4-6), and add another patch (11) that will make converting from
kind=>type easier in a later subset.  More details in per-patch
changelogs.

v7 notes:
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg01387.html
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.

v6 notes:
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg00562.html
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 (5):
  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: Detect base class loops

 scripts/qapi.py                                    | 170 ++++++++++++---------
 tests/Makefile                                     |   2 +-
 tests/qapi-schema/alternate-clash.err              |   2 +-
 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 +
 .../{union-bad-branch.exit => base-cycle.exit}     |   0
 tests/qapi-schema/base-cycle.json                  |   3 +
 .../{union-bad-branch.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-member.err      |   2 +-
 tests/qapi-schema/qapi-schema-test.out             |   8 +-
 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-branches.json        |   2 +-
 tests/qapi-schema/union-max.err                    |   2 +-
 22 files changed, 119 insertions(+), 107 deletions(-)
 create mode 100644 tests/qapi-schema/base-cycle.err
 rename tests/qapi-schema/{union-bad-branch.exit => base-cycle.exit} (100%)
 create mode 100644 tests/qapi-schema/base-cycle.json
 rename tests/qapi-schema/{union-bad-branch.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 v9 1/5] 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.  In particular, when a member is
inherited from a base type, it is desirable to associate the
member name with the base type (and not the type calling
member.check()).

Rather than packing additional information into the seen array
passed to each member.check() (as in seen[m.name] = {'member':m,
'owner':type}), it is easier to have each member track the name
of the owner type in the first place (keeping things simpler
with the existing seen[m.name] = m).  The new member.owner field
is set via a new set_owner() method, called when registering
the members and variants arrays with an object or variant type.
Track only a name, and not the actual type object, to avoid
creating a circular python reference chain.

Note that the set_owner() method for variants has to know
whether the tag_member field is owned elsewhere (by the base of
flat unions, and the local_members of simple unions) or must be
set directly (for alternates); it decides this based on the
subclass of the tag_member field.

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

To make the human-readable name of implicit types work without
duplicating efforts, the describe() method has to reverse the
name of implicit types, via the helper _pretty_owner(), plus a
tweak to report event data separately from command arguments.

No change to generated code.

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

---
v9 (now in subset D): rebase to earlier changes, hoist 'role' to top
of class, split out _pretty_helper(), manage owner when tag_member
appears as part of local_members for unions
v8: don't munge implicit type names [except for event data], and
instead make describe() create nicer messages. Add set_owner(), and
use field 'role' instead of method _describe()
v7: total rewrite: rework implicit object names, assign owner
when initializing owner type rather than when creating member
python object
v6: rebase on new lazy array creation and simple union 'type'
motion; tweak commit message
---
 scripts/qapi.py                        | 46 +++++++++++++++++++++++++++++++---
 tests/qapi-schema/qapi-schema-test.out |  8 +++---
 2 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 0eb5d9a..2877e44 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -957,8 +957,10 @@ class QAPISchemaObjectType(QAPISchemaType):
         assert base is None or isinstance(base, str)
         for m in local_members:
             assert isinstance(m, QAPISchemaObjectTypeMember)
-        assert (variants is None or
-                isinstance(variants, QAPISchemaObjectTypeVariants))
+            m.set_owner(name)
+        if variants is not None:
+            assert isinstance(variants, QAPISchemaObjectTypeVariants)
+            variants.set_owner(name)
         self._base_name = base
         self.base = None
         self.local_members = local_members
@@ -1011,6 +1013,8 @@ class QAPISchemaObjectType(QAPISchemaType):


 class QAPISchemaObjectTypeMember(object):
+    role = 'member'
+
     def __init__(self, name, typ, optional):
         assert isinstance(name, str)
         assert isinstance(typ, str)
@@ -1019,11 +1023,17 @@ class QAPISchemaObjectTypeMember(object):
         self._type_name = typ
         self.type = None
         self.optional = optional
+        self.owner = None
+
+    def set_owner(self, name):
+        assert not self.owner
+        self.owner = name

     def check(self, schema, seen):
         # seen is a map of names we must not collide with (either QMP
         # names, when called by ObjectType, or case names, when called
         # by Variant). This method is safe to call over multiple 'seen'.
+        assert self.owner
         assert self.name not in seen
         self.type = schema.lookup_type(self._type_name)
         assert self.type
@@ -1032,6 +1042,25 @@ class QAPISchemaObjectTypeMember(object):
     def c_type(self):
         return self.type.c_type()

+    def _pretty_owner(self):
+        # See QAPISchema._make_implicit_object_type() - reverse the
+        # mapping there to create a nice human-readable description
+        owner = self.owner
+        if owner.startswith(':obj-'):
+            owner = owner[5:]
+            if owner.endswith('-arg'):
+                return '(argument of %s)' % owner[:-4]
+            elif owner.endswith('-data'):
+                return '(data of %s)' % owner[:-5]
+            else:
+                assert owner.endswith('-wrapper')
+                return '(branch of %s)' % owner[:-8]
+        else:
+            return '(%s of %s)' % (self.role, owner)
+
+    def describe(self):
+        return "'%s' %s" % (self.name, self._pretty_owner())
+

 class QAPISchemaObjectTypeVariants(object):
     def __init__(self, tag_name, tag_member, variants):
@@ -1049,6 +1078,12 @@ class QAPISchemaObjectTypeVariants(object):
         self.tag_member = tag_member
         self.variants = variants

+    def set_owner(self, name):
+        if isinstance(self.tag_member, QAPISchemaAlternateTypeTag):
+            self.tag_member.set_owner(name)
+        for v in self.variants:
+            v.set_owner(name)
+
     def check(self, schema, seen):
         if self.tag_name:    # flat union
             self.tag_member = seen[self.tag_name]
@@ -1067,6 +1102,8 @@ class QAPISchemaObjectTypeVariants(object):


 class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
+    role = 'branch'
+
     def __init__(self, name, typ):
         QAPISchemaObjectTypeMember.__init__(self, name, typ, False)

@@ -1094,6 +1131,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
         QAPISchemaType.__init__(self, name, info)
         assert isinstance(variants, QAPISchemaObjectTypeVariants)
         assert not variants.tag_name
+        variants.set_owner(name)
         self.variants = variants

     def check(self, schema):
@@ -1111,6 +1149,7 @@ class QAPISchemaAlternateTypeTag(QAPISchemaObjectTypeMember):
         QAPISchemaObjectTypeMember.__init__(self, 'type', '', False)

     def check(self, schema, seen):
+        assert self.owner
         assert len(seen) == 0
         seen[self.name] = self

@@ -1235,6 +1274,7 @@ class QAPISchema(object):
     def _make_implicit_object_type(self, name, info, role, members):
         if not members:
             return None
+        # See also QAPISchemaObjectTypeMember.describe()
         name = ':obj-%s-%s' % (name, role)
         if not self.lookup_entity(name, QAPISchemaObjectType):
             self._def_entity(QAPISchemaObjectType(name, info, None,
@@ -1338,7 +1378,7 @@ class QAPISchema(object):
         data = expr.get('data')
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
-                name, info, 'arg', self._make_members(data, info))
+                name, info, 'data', self._make_members(data, info))
         self._def_entity(QAPISchemaEvent(name, info, data))

     def _def_exprs(self):
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index eda71b9..0b85dee 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -1,9 +1,9 @@
 object :empty
-object :obj-EVENT_C-arg
+object :obj-EVENT_C-data
     member a: int optional=True
     member b: UserDefOne optional=True
     member c: str optional=False
-object :obj-EVENT_D-arg
+object :obj-EVENT_D-data
     member a: EventStructOne optional=False
     member b: str optional=False
     member c: str optional=True
@@ -82,8 +82,8 @@ alternate AltStrNum
     case n: number
 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
 object Empty1
 object Empty2
     base Empty1
-- 
2.4.3

[Qemu-devel] [PATCH v9 2/5] 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 a previously-broken test, and the resulting error
message demonstrates the utility of the .describe() method added
previously.  No change to generated code.

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

---
v9 (now in subset D): rebase to earlier changes, now only one test
affected
v8: rebase to earlier changes
v7: split out error reporting prep and member.c_name() addition
v6: rebase to earlier testsuite and info improvements
---
 scripts/qapi.py                        | 43 +++++++++++++++++++---------------
 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 -----
 5 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 2877e44..00e8452 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -976,19 +976,20 @@ class QAPISchemaObjectType(QAPISchemaType):
         seen = OrderedDict()
         if self._base_name:
             self.base = schema.lookup_type(self._base_name)
-            self.base.check_qmp(schema, seen)
+            self.base.check_qmp(schema, self.info, seen)
         for m in self.local_members:
-            m.check(schema, seen)
+            m.check(schema, self.info, seen)
         if self.variants:
-            self.variants.check(schema, seen)
+            self.variants.check(schema, self.info, seen)
         self.members = seen.values()

     # Check that this type does not introduce QMP collisions into seen
-    def check_qmp(self, schema, seen):
+    # info is the location providing seen, which is not necessarily self.info
+    def check_qmp(self, schema, info, seen):
         self.check(schema)
         assert not self.variants       # not implemented
         for m in self.members:
-            m.check(schema, seen)
+            m.check(schema, info, seen)

     def is_implicit(self):
         # See QAPISchema._make_implicit_object_type()
@@ -1029,15 +1030,19 @@ class QAPISchemaObjectTypeMember(object):
         assert not self.owner
         self.owner = name

-    def check(self, schema, seen):
+    def check(self, schema, info, seen):
         # seen is a map of names we must not collide with (either QMP
         # names, when called by ObjectType, or case names, when called
         # by Variant). This method is safe to call over multiple 'seen'.
         assert self.owner
-        assert self.name not in seen
         self.type = schema.lookup_type(self._type_name)
         assert self.type
-        seen[self.name] = self
+        name = c_name(self.name)
+        if name in seen:
+            raise QAPIExprError(info,
+                                "%s collides with %s"
+                                % (self.describe(), seen[name].describe()))
+        seen[name] = self

     def c_type(self):
         return self.type.c_type()
@@ -1084,21 +1089,21 @@ class QAPISchemaObjectTypeVariants(object):
         for v in self.variants:
             v.set_owner(name)

-    def check(self, schema, seen):
+    def check(self, schema, info, seen):
         if self.tag_name:    # flat union
-            self.tag_member = seen[self.tag_name]
-            assert self.tag_member
+            self.tag_member = seen[c_name(self.tag_name)]
+            assert self.tag_name == self.tag_member.name
         elif seen:           # simple union
             assert self.tag_member in seen.itervalues()
         else:                # alternate
-            self.tag_member.check(schema, seen)
+            self.tag_member.check(schema, info, seen)
         if not isinstance(self.tag_member, QAPISchemaAlternateTypeTag):
             assert isinstance(self.tag_member.type, QAPISchemaEnumType)
         cases = OrderedDict()
         for v in self.variants:
             # Reset seen array for each variant, since QMP names from one
             # branch do not affect another branch, nor add to all_members
-            v.check(schema, self.tag_member.type, dict(seen), cases)
+            v.check(schema, info, self.tag_member.type, dict(seen), cases)


 class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
@@ -1107,13 +1112,13 @@ class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
     def __init__(self, name, typ):
         QAPISchemaObjectTypeMember.__init__(self, name, typ, False)

-    def check(self, schema, tag_type, seen, cases):
+    def check(self, schema, info, tag_type, seen, cases):
         # cases is case names we must not collide with
-        QAPISchemaObjectTypeMember.check(self, schema, cases)
+        QAPISchemaObjectTypeMember.check(self, schema, info, cases)
         if tag_type:
             # seen is QMP names our members must not collide with
             assert self.name in tag_type.values
-            self.type.check_qmp(schema, seen)
+            self.type.check_qmp(schema, info, seen)

     # This function exists to support ugly simple union special cases
     # TODO get rid of them, and drop the function
@@ -1135,7 +1140,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'
@@ -1148,10 +1153,10 @@ class QAPISchemaAlternateTypeTag(QAPISchemaObjectTypeMember):
     def __init__(self):
         QAPISchemaObjectTypeMember.__init__(self, 'type', '', False)

-    def check(self, schema, seen):
+    def check(self, schema, info, seen):
         assert self.owner
         assert len(seen) == 0
-        seen[self.name] = self
+        seen[c_name(self.name)] = self

     def c_type(self):
         return 'qtype_code'
diff --git a/tests/qapi-schema/args-name-clash.err b/tests/qapi-schema/args-name-clash.err
index e69de29..2735217 100644
--- a/tests/qapi-schema/args-name-clash.err
+++ b/tests/qapi-schema/args-name-clash.err
@@ -0,0 +1 @@
+tests/qapi-schema/args-name-clash.json:5: 'a_b' (argument of oops) collides with 'a-b' (argument of oops)
diff --git a/tests/qapi-schema/args-name-clash.exit b/tests/qapi-schema/args-name-clash.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/args-name-clash.exit
+++ b/tests/qapi-schema/args-name-clash.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/args-name-clash.json b/tests/qapi-schema/args-name-clash.json
index 9e8f889..3fe4ea5 100644
--- a/tests/qapi-schema/args-name-clash.json
+++ b/tests/qapi-schema/args-name-clash.json
@@ -1,5 +1,5 @@
 # C member name collision
-# FIXME - This parses, but fails to compile, because the C struct is given
-# two 'a_b' members.  Either reject this at parse time, or munge the C names
-# to avoid the collision.
+# Reject members that clash when mapped to C names (we would have two 'a_b'
+# members). It would also be possible to munge the C names to avoid the
+# collision, but unlikely to be worth the effort.
 { 'command': 'oops', 'data': { 'a-b': 'str', 'a_b': 'str' } }
diff --git a/tests/qapi-schema/args-name-clash.out b/tests/qapi-schema/args-name-clash.out
index 9b2f6e4..e69de29 100644
--- a/tests/qapi-schema/args-name-clash.out
+++ b/tests/qapi-schema/args-name-clash.out
@@ -1,6 +0,0 @@
-object :empty
-object :obj-oops-arg
-    member a-b: str optional=False
-    member a_b: str optional=False
-command oops :obj-oops-arg -> None
-   gen=True success_response=True
-- 
2.4.3

[Qemu-devel] [PATCH v9 3/5] 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 (commit 7b2a5c2) to avoid an assertion failure
when a flat union branch name collides with its discriminator
name is also handled nicely by this code; but there is more work
needed before we can detect all collisions in the generated enum
associated with simple union branch names.

No change to generated code.

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

---
v9: simplify on top of earlier check() improvements
v8: decide whether to inline members based on union vs. alternate,
not on flat vs. simple, and fix logic to avoid breaking
union-clash-data in the process; add comments; assumes
pull-qapi-2015-10-12 will go in without modifying commit ids
v7: comment improvements, retitle subject
v6: rebase to earlier testsuite improvements, fold in cleanup
of flat-union-clash-type
---
 scripts/qapi.py                               | 34 +--------------------------
 tests/qapi-schema/alternate-clash.err         |  2 +-
 tests/qapi-schema/flat-union-clash-member.err |  2 +-
 tests/qapi-schema/struct-base-clash-deep.err  |  2 +-
 tests/qapi-schema/struct-base-clash.err       |  2 +-
 5 files changed, 5 insertions(+), 37 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 00e8452..8e927f7 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -502,21 +502,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']

@@ -595,15 +580,9 @@ 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.
@@ -627,21 +606,12 @@ def check_union(expr, expr_info):
 def check_alternate(expr, expr_info):
     name = expr['alternate']
     members = expr['data']
-    values = {}
     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 branch names
-        c_key = c_name(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,
@@ -687,8 +657,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=[]):
diff --git a/tests/qapi-schema/alternate-clash.err b/tests/qapi-schema/alternate-clash.err
index a475ab6..604d849 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: 'a_b' (branch of Alt1) collides with 'a-b' (branch of Alt1)
diff --git a/tests/qapi-schema/flat-union-clash-member.err b/tests/qapi-schema/flat-union-clash-member.err
index 2f0397a..2adf697 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: 'name' (member of Branch1) collides with 'name' (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

[Qemu-devel] [PATCH v9 4/5] 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
mapping to the same enum value, even when the names do not
collide case-wise.  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 wasn't adding
much: union-clash-branches exposes the error message when branches
directly collide, and union-max exposes the error message when
branches cause an enum collision (union-bad-branch basically
causes an enum collision that would not be a C collision).  Maybe
we should require ALL names to be case-insensitively unique, but
that would be the topic for a different patch.

No change to generated code.

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

---
v9: rebase to earlier changes, update commit message, break out
helper _describe() method
v8: rebase to earlier changes; better comments
v7: retitle and improve commit message; earlier subclass patches
avoid problem with detecting 'kind' collision
v6: new patch
---
 scripts/qapi.py                             | 41 ++++++++++++++++-------------
 tests/Makefile                              |  1 -
 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-branches.json |  2 +-
 tests/qapi-schema/union-max.err             |  2 +-
 11 files changed, 28 insertions(+), 34 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 8e927f7..b5a0fde 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -533,7 +533,6 @@ def check_union(expr, expr_info):
     base = expr.get('base')
     discriminator = expr.get('discriminator')
     members = expr['data']
-    values = {'MAX': '(automatic)'}

     # Two types of unions, determined by discriminator.

@@ -593,15 +592,6 @@ 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']
@@ -630,7 +620,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,
@@ -641,12 +630,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):
@@ -873,8 +856,30 @@ class QAPISchemaEnumType(QAPISchemaType):
         self.values = values
         self.prefix = prefix

+    def _describe(self, schema):
+        # 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):
+                return "Alternate '%s' branch" % owner.name
+            else:
+                return "Union '%s' branch" % owner.name
+        else:
+            return "Enum '%s' value" % self.name
+
     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:
+                raise QAPIExprError(self.info,
+                                    "%s '%s' clashes with '%s'"
+                                    % (self._describe(schema), 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 30879ba..fc6643a 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -335,7 +335,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/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..e5b2113 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: 'a_b' (branch of TestUnion) collides with 'a-b' (branch of TestUnion)
diff --git a/tests/qapi-schema/union-clash-branches.json b/tests/qapi-schema/union-clash-branches.json
index 31d135f..3bece8c 100644
--- a/tests/qapi-schema/union-clash-branches.json
+++ b/tests/qapi-schema/union-clash-branches.json
@@ -1,5 +1,5 @@
 # Union branch name collision
 # Reject a union that would result in a collision in generated C names (this
-# would try to generate two enum values 'TEST_UNION_KIND_A_B').
+# would try to generate two members 'a_b').
 { 'union': 'TestUnion',
   'data': { 'a-b': 'int', 'a_b': 'str' } }
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 v9 5/5] 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, complaining about nesting too deep).

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

This patch includes both the test and the fix, since the .err
file output for the unfixed case is not useful (particularly
when it was warning about unbounded recursion, as that limit
may be platform-specific).

We don't need to worry about cycles in flat unions (neither
the base nor a variant class can be a union) nor in alternates
(alternate branches cannot themselves be an alternate).

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

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

diff --git a/scripts/qapi.py b/scripts/qapi.py
index b5a0fde..f2f7c27 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -942,7 +942,11 @@ class QAPISchemaObjectType(QAPISchemaType):

     # Finish construction, and validate that all members are usable
     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 fc6643a..5927e26 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -252,6 +252,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