[Qemu-devel] [PATCH V8 00/10] qapi script: support enum as discriminator and better enum name

[Qemu-devel] [PATCH V8 00/10] qapi script: support enum as discriminator and better enum name

[Wenchao Xia]

This series address two issues:

1. support using enum as discriminator in union.
For example, if we have following define in qapi schema:
{ 'enum': 'EnumOne',
  'data': [ 'value1', 'value2', 'value3' ] }

{ 'type': 'UserDefBase0',
  'data': { 'base-string0': 'str', 'base-enum0': 'EnumOne' } }

Before this series, discriminator in union must be a string, and a
hidden enum type as discriminator is generated. After this series,
qapi schema can directly use predefined enum type:
{ 'union': 'UserDefEnumDiscriminatorUnion',
  'base': 'UserDefBase0',
  'discriminator' : 'base-enum0',
  'data': { 'value1' : 'UserDefA',
            'value2' : 'UserDefInherit',
            'value3' : 'UserDefB' } }

The benefit is that every thing is defined explicitly in schema file,
the discriminator enum type can be used in other API define in schema,
and a compile time check will be put to verify the correctness according
to enum define. Currently BlockdevOptions used discriminator which can
be converted, in the future other union can also use enum discriminator.

The implement is done by:
1.1 remember the enum defines by qapi scripts.(patch 1)
1.2 use the remembered enum define to check correctness at compile
time.(patch 3), more strict check(patch 2)
1.3 use the same enum name generation rule to avoid C code mismatch,
esp for "case [ENUM_VALUE]" in qapi-visit.c.(patch 4,5)
1.4 switch the code path, when pre-defined enum type is used as discriminator,
don't generate a hidden enum type, use the enum type instead, add
docs/qapi-code-gen.txt.(Patch 6)
1.5 test case shows how it looks like.(Patch 7)
1.6 convert BlockdevOptions. (Patch 8)

2. Better enum name generation
Before this patch, AIOContext->A_I_O_CONTEXT, after this patch,
AIOContet->AIO_CONTEXT. Since previous patch has foldered enum
name generation codes into one function, it is done easily by modifying
it.(Patch 9)


Changes from RFC:
  Mainly address Eric's comments: fix typo, add patch 2 to allow partly mapping
enum value in union, add related test case, remove direct inherit support "_base"
and related test case. RFC series at:
http://lists.nongnu.org/archive/html/qemu-devel/2013-11/msg00363.html

v2:
  General:
  3/8: use Raise exception instead of sys.error.write in qapi.py.
  Address Eric's comments:
  2/8,3/8: more check for enum value at compile time, not allow partly mapping.
  8/8: correspond test case change.

v3:
  General:
  move enum name generation patch to last in the series, add convert patch
8/9.
  Address Luiz and Kevin's comments:
  Better introduction.
  6/9: renamed this patch, add docs/qapi-code-gen.txt part.

v4:
  Address Eric's comments:
  5/9: better commit message.
  6/9: typo fix in doc.
  9/9: typo fix, fix indentation, better incode comment.

v5:
  Address Eric's comments:
  6/10: doc typo fix.
  8/10: new patch to remove string discriminator.
  9/10: removed the string discriminator test case.

v6:
  rebased on upstream by adding "blgdebug" and "blkverify" in qapi-schema.json
in patch 7/10.

v7:
  The series is rebased on Markus's tree:
  git://repo.or.cz/qemu/armbru.git branch qapi-scripts

  Address Markus's comments:
  2/11: typo fix in error message.
  3/11: new patch to recording addtional line info in schema parsing.
  4/11: move the check into qapi.py, report better with fp/line info.
  7/11: move the UnionKind adding in qapi.py after 1st parsing and
with better error info.
  9/11: move the check into qapi.py with fp/line info, test case
corresond change.
  11/11: new patch for errpr path test case.

v8:
  The series is ontop of Markus's tree and rebased on upstream:
  Address Markus's comments:
  1/10: better commit title, simplify is_enum().
  2/10: test case squashed into this patch.
  3/10: no change, column computation is not touched.
  4/10: simplify commit title and message, refine the semantic check
logic as comments in v7, check in discriminator_find_enum_define() is moved
out so that the function can be used without error info, squash related test
into this patch, re-orgnize 'expr_elem' with separate 'info' member,
QAPIExprError now takes only info to work, better error message, remove
check of whether all enum values are covered by branch, use expr['key']
instead of expr.get('key') when possible.
  6/10: remove useless comments in generate_enum_full_value(), make line
shorter by change variable name.
  7/10: building 'expr_elem' for discriminator_find_enum_define() is removed,
make line shorter in qapi-visit.py, add a test case that enum is used before
define.
  8/10: rebased on uptream by adding 'quorum' driver type.
  9/10: better doc and error message, add a test case for string discriminator.
use string concatenation rather than line continuation for string in C code.


Wenchao Xia (10):
  1 qapi script: remember explicitly defined enum values
  2 qapi script: add check for duplicated key
  3 qapi script: remember line number in schema parsing
  4 qapi script: check correctness of union
  5 qapi script: code move for generate_enum_name()
  6 qapi script: use same function to generate enum string
  7 qapi script: support enum type as discriminator in union
  8 qapi: convert BlockdevOptions to use enum discriminator
  9 qapi script: do not allow string discriminator
  10 qapi script: do not add "_" for every capitalized char in enum

 docs/qapi-code-gen.txt                             |    5 +-
 include/qapi/qmp/qerror.h                          |    2 +-
 qapi-schema.json                                   |   14 ++-
 scripts/qapi-types.py                              |   34 ++--
 scripts/qapi-visit.py                              |   42 ++++--
 scripts/qapi.py                                    |  183 ++++++++++++++++++--
 target-i386/cpu.c                                  |    2 +-
 tests/Makefile                                     |    6 +-
 tests/qapi-schema/comments.out                     |    2 +-
 tests/qapi-schema/duplicate-key.err                |    1 +
 tests/qapi-schema/duplicate-key.exit               |    1 +
 tests/qapi-schema/duplicate-key.json               |    2 +
 .../qapi-schema/flat-union-invalid-branch-key.err  |    1 +
 .../qapi-schema/flat-union-invalid-branch-key.exit |    1 +
 .../qapi-schema/flat-union-invalid-branch-key.json |   17 ++
 .../flat-union-invalid-discriminator.err           |    1 +
 .../flat-union-invalid-discriminator.exit          |    1 +
 .../flat-union-invalid-discriminator.json          |   17 ++
 tests/qapi-schema/flat-union-no-base.err           |    1 +
 tests/qapi-schema/flat-union-no-base.exit          |    1 +
 tests/qapi-schema/flat-union-no-base.json          |   16 ++
 tests/qapi-schema/flat-union-reverse-define.exit   |    1 +
 tests/qapi-schema/flat-union-reverse-define.json   |   17 ++
 tests/qapi-schema/flat-union-reverse-define.out    |    9 +
 .../flat-union-string-discriminator.err            |    1 +
 .../flat-union-string-discriminator.exit           |    1 +
 .../flat-union-string-discriminator.json           |   17 ++
 tests/qapi-schema/qapi-schema-test.json            |    9 +-
 tests/qapi-schema/qapi-schema-test.out             |   13 +-
 tests/qapi-schema/union-invalid-base.err           |    1 +
 tests/qapi-schema/union-invalid-base.exit          |    1 +
 tests/qapi-schema/union-invalid-base.json          |   10 +
 tests/test-qmp-input-strict.c                      |    5 +-
 tests/test-qmp-input-visitor.c                     |   10 +-
 tests/test-qmp-output-visitor.c                    |   10 +-
 35 files changed, 388 insertions(+), 67 deletions(-)
 create mode 100644 tests/qapi-schema/duplicate-key.err
 create mode 100644 tests/qapi-schema/duplicate-key.exit
 create mode 100644 tests/qapi-schema/duplicate-key.json
 create mode 100644 tests/qapi-schema/duplicate-key.out
 create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.err
 create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.exit
 create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.json
 create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.out
 create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.err
 create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.exit
 create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.json
 create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.out
 create mode 100644 tests/qapi-schema/flat-union-no-base.err
 create mode 100644 tests/qapi-schema/flat-union-no-base.exit
 create mode 100644 tests/qapi-schema/flat-union-no-base.json
 create mode 100644 tests/qapi-schema/flat-union-no-base.out
 create mode 100644 tests/qapi-schema/flat-union-reverse-define.err
 create mode 100644 tests/qapi-schema/flat-union-reverse-define.exit
 create mode 100644 tests/qapi-schema/flat-union-reverse-define.json
 create mode 100644 tests/qapi-schema/flat-union-reverse-define.out
 create mode 100644 tests/qapi-schema/flat-union-string-discriminator.err
 create mode 100644 tests/qapi-schema/flat-union-string-discriminator.exit
 create mode 100644 tests/qapi-schema/flat-union-string-discriminator.json
 create mode 100644 tests/qapi-schema/flat-union-string-discriminator.out
 create mode 100644 tests/qapi-schema/union-invalid-base.err
 create mode 100644 tests/qapi-schema/union-invalid-base.exit
 create mode 100644 tests/qapi-schema/union-invalid-base.json
 create mode 100644 tests/qapi-schema/union-invalid-base.out

[Qemu-devel] [PATCH V8 01/10] qapi script: remember explicitly defined enum values

[Wenchao Xia]

From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Later other scripts will need to check the enum values.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
---
 scripts/qapi.py                        |   16 +++++++++++-----
 tests/qapi-schema/comments.out         |    2 +-
 tests/qapi-schema/qapi-schema-test.out |   10 +++++-----
 3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index f3c2a20..023930e 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -169,7 +169,7 @@ def parse_schema(fp):
 
     for expr in schema.exprs:
         if expr.has_key('enum'):
-            add_enum(expr['enum'])
+            add_enum(expr['enum'], expr['data'])
         elif expr.has_key('union'):
             add_union(expr)
             add_enum('%sKind' % expr['union'])
@@ -289,13 +289,19 @@ def find_union(name):
             return union
     return None
 
-def add_enum(name):
+def add_enum(name, enum_values = None):
     global enum_types
-    enum_types.append(name)
+    enum_types.append({"enum_name": name, "enum_values": enum_values})
 
-def is_enum(name):
+def find_enum(name):
     global enum_types
-    return (name in enum_types)
+    for enum in enum_types:
+        if enum['enum_name'] == name:
+            return enum
+    return None
+
+def is_enum(name):
+    return find_enum(name) != None
 
 def c_type(name):
     if name == 'str':
diff --git a/tests/qapi-schema/comments.out b/tests/qapi-schema/comments.out
index e3bd904..4ce3dcf 100644
--- a/tests/qapi-schema/comments.out
+++ b/tests/qapi-schema/comments.out
@@ -1,3 +1,3 @@
 [OrderedDict([('enum', 'Status'), ('data', ['good', 'bad', 'ugly'])])]
-['Status']
+[{'enum_name': 'Status', 'enum_values': ['good', 'bad', 'ugly']}]
 []
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 89b53d4..01685d4 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -15,11 +15,11 @@
  OrderedDict([('command', 'user_def_cmd2'), ('data', OrderedDict([('ud1a', 'UserDefOne'), ('*ud1b', 'UserDefOne')])), ('returns', 'UserDefTwo')]),
  OrderedDict([('command', 'user_def_cmd3'), ('data', OrderedDict([('a', 'int'), ('*b', 'int')])), ('returns', 'int')]),
  OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
-['EnumOne',
- 'UserDefUnionKind',
- 'UserDefFlatUnionKind',
- 'UserDefAnonUnionKind',
- 'UserDefNativeListUnionKind']
+[{'enum_name': 'EnumOne', 'enum_values': ['value1', 'value2', 'value3']},
+ {'enum_name': 'UserDefUnionKind', 'enum_values': None},
+ {'enum_name': 'UserDefFlatUnionKind', 'enum_values': None},
+ {'enum_name': 'UserDefAnonUnionKind', 'enum_values': None},
+ {'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None}]
 [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
  OrderedDict([('type', 'UserDefZero'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('type', 'UserDefOne'), ('base', 'UserDefZero'), ('data', OrderedDict([('string', 'str'), ('*enum1', 'EnumOne')]))]),
-- 
1.7.1

[Qemu-devel] [PATCH V8 02/10] qapi script: add check for duplicated key

[Wenchao Xia]

From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

It is bad that same key was specified twice, especially when a union have
two branches with same condition. This patch can prevent it.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
---
 scripts/qapi.py                      |    2 ++
 tests/Makefile                       |    3 ++-
 tests/qapi-schema/duplicate-key.err  |    1 +
 tests/qapi-schema/duplicate-key.exit |    1 +
 tests/qapi-schema/duplicate-key.json |    2 ++
 5 files changed, 8 insertions(+), 1 deletions(-)
 create mode 100644 tests/qapi-schema/duplicate-key.err
 create mode 100644 tests/qapi-schema/duplicate-key.exit
 create mode 100644 tests/qapi-schema/duplicate-key.json
 create mode 100644 tests/qapi-schema/duplicate-key.out

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 023930e..d0e7934 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -116,6 +116,8 @@ class QAPISchema:
             if self.tok != ':':
                 raise QAPISchemaError(self, 'Expected ":"')
             self.accept()
+            if key in expr:
+                raise QAPISchemaError(self, 'Duplicate key "%s"' % key)
             expr[key] = self.get_expr(True)
             if self.tok == '}':
                 self.accept()
diff --git a/tests/Makefile b/tests/Makefile
index b17d41e..dfe06eb 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -142,7 +142,8 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
         missing-comma-object.json non-objects.json \
         qapi-schema-test.json quoted-structural-chars.json \
         trailing-comma-list.json trailing-comma-object.json \
-        unclosed-list.json unclosed-object.json unclosed-string.json)
+        unclosed-list.json unclosed-object.json unclosed-string.json \
+        duplicate-key.json)
 
 GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
 
diff --git a/tests/qapi-schema/duplicate-key.err b/tests/qapi-schema/duplicate-key.err
new file mode 100644
index 0000000..0801c6a
--- /dev/null
+++ b/tests/qapi-schema/duplicate-key.err
@@ -0,0 +1 @@
+<stdin>:2:10: Duplicate key "key"
diff --git a/tests/qapi-schema/duplicate-key.exit b/tests/qapi-schema/duplicate-key.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/duplicate-key.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/duplicate-key.json b/tests/qapi-schema/duplicate-key.json
new file mode 100644
index 0000000..1b55d88
--- /dev/null
+++ b/tests/qapi-schema/duplicate-key.json
@@ -0,0 +1,2 @@
+{ 'key': 'value',
+  'key': 'value' }
diff --git a/tests/qapi-schema/duplicate-key.out b/tests/qapi-schema/duplicate-key.out
new file mode 100644
index 0000000..e69de29
-- 
1.7.1

[Qemu-devel] [PATCH V8 03/10] qapi script: remember line number in schema parsing

[Wenchao Xia]

From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Before this patch, 'QAPISchemaError' scans whole input until 'pos'
to get error line number. After this patch, the scan is avoided since
line number is remembered in schema parsing. This patch also benefits
other error report functions, which would be introduced later.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
---
 scripts/qapi.py |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index d0e7934..1954292 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -39,12 +39,10 @@ class QAPISchemaError(Exception):
     def __init__(self, schema, msg):
         self.fp = schema.fp
         self.msg = msg
-        self.line = self.col = 1
-        for ch in schema.src[0:schema.pos]:
-            if ch == '\n':
-                self.line += 1
-                self.col = 1
-            elif ch == '\t':
+        self.col = 1
+        self.line = schema.line
+        for ch in schema.src[schema.line_pos:schema.pos]:
+            if ch == '\t':
                 self.col = (self.col + 7) % 8 + 1
             else:
                 self.col += 1
@@ -60,6 +58,8 @@ class QAPISchema:
         if self.src == '' or self.src[-1] != '\n':
             self.src += '\n'
         self.cursor = 0
+        self.line = 1
+        self.line_pos = 0
         self.exprs = []
         self.accept()
 
@@ -100,6 +100,8 @@ class QAPISchema:
                 if self.cursor == len(self.src):
                     self.tok = None
                     return
+                self.line += 1
+                self.line_pos = self.cursor
             elif not self.tok.isspace():
                 raise QAPISchemaError(self, 'Stray "%s"' % self.tok)
 
-- 
1.7.1

[Qemu-devel] [PATCH V8 04/10] qapi script: check correctness of union

[Wenchao Xia]

From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
---
 scripts/qapi.py                                    |  106 +++++++++++++++++++-
 tests/Makefile                                     |    4 +-
 .../qapi-schema/flat-union-invalid-branch-key.err  |    1 +
 .../qapi-schema/flat-union-invalid-branch-key.exit |    1 +
 .../qapi-schema/flat-union-invalid-branch-key.json |   17 +++
 .../flat-union-invalid-discriminator.err           |    1 +
 .../flat-union-invalid-discriminator.exit          |    1 +
 .../flat-union-invalid-discriminator.json          |   17 +++
 tests/qapi-schema/flat-union-no-base.err           |    1 +
 tests/qapi-schema/flat-union-no-base.exit          |    1 +
 tests/qapi-schema/flat-union-no-base.json          |   16 +++
 tests/qapi-schema/union-invalid-base.err           |    1 +
 tests/qapi-schema/union-invalid-base.exit          |    1 +
 tests/qapi-schema/union-invalid-base.json          |   10 ++
 14 files changed, 175 insertions(+), 3 deletions(-)
 create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.err
 create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.exit
 create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.json
 create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.out
 create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.err
 create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.exit
 create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.json
 create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.out
 create mode 100644 tests/qapi-schema/flat-union-no-base.err
 create mode 100644 tests/qapi-schema/flat-union-no-base.exit
 create mode 100644 tests/qapi-schema/flat-union-no-base.json
 create mode 100644 tests/qapi-schema/flat-union-no-base.out
 create mode 100644 tests/qapi-schema/union-invalid-base.err
 create mode 100644 tests/qapi-schema/union-invalid-base.exit
 create mode 100644 tests/qapi-schema/union-invalid-base.json
 create mode 100644 tests/qapi-schema/union-invalid-base.out

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 1954292..cea346f 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -50,6 +50,15 @@ class QAPISchemaError(Exception):
     def __str__(self):
         return "%s:%s:%s: %s" % (self.fp.name, self.line, self.col, self.msg)
 
+class QAPIExprError(Exception):
+    def __init__(self, expr_info, msg):
+        self.fp = expr_info['fp']
+        self.line = expr_info['line']
+        self.msg = msg
+
+    def __str__(self):
+        return "%s:%s: %s" % (self.fp.name, self.line, self.msg)
+
 class QAPISchema:
 
     def __init__(self, fp):
@@ -64,7 +73,10 @@ class QAPISchema:
         self.accept()
 
         while self.tok != None:
-            self.exprs.append(self.get_expr(False))
+            expr_info = {'fp': fp, 'line': self.line}
+            expr_elem = {'expr': self.get_expr(False),
+                         'info': expr_info}
+            self.exprs.append(expr_elem)
 
     def accept(self):
         while True:
@@ -162,6 +174,89 @@ class QAPISchema:
             raise QAPISchemaError(self, 'Expected "{", "[" or string')
         return expr
 
+def find_base_fields(base):
+    base_struct_define = find_struct(base)
+    if not base_struct_define:
+        return None
+    return base_struct_define['data']
+
+# Return the discriminator enum define if discrminator is specified as an
+# enum type, otherwise return None.
+def discriminator_find_enum_define(expr):
+    base = expr.get('base')
+    discriminator = expr.get('discriminator')
+
+    if not (discriminator and base):
+        return None
+
+    base_fields = find_base_fields(base)
+    if not base_fields:
+        return None
+
+    discriminator_type = base_fields.get(discriminator)
+    if not discriminator_type:
+        return None
+
+    return find_enum(discriminator_type)
+
+def check_union(expr, expr_info):
+    name = expr['union']
+    base = expr.get('base')
+    discriminator = expr.get('discriminator')
+    members = expr['data']
+
+    # If the object has a member 'base', its value must name a complex type.
+    if base:
+        base_fields = find_base_fields(base)
+        if not base_fields:
+            raise QAPIExprError(expr_info,
+                                "Base '%s' is not a valid type"
+                                % base)
+
+    # If the union object has no member 'discriminator', it's an
+    # ordinary union.
+    if not discriminator:
+        enum_define = None
+
+    # Else if the value of member 'discriminator' is {}, it's an
+    # anonymous union.
+    elif discriminator == {}:
+        enum_define = None
+
+    # Else, it's a flat union.
+    else:
+        # The object must have a member 'base'.
+        if not base:
+            raise QAPIExprError(expr_info,
+                                "Flat union '%s' must have a base field"
+                                % name)
+        # The value of member 'discriminator' must name a member of the
+        # base type.
+        if not base_fields.get(discriminator):
+            raise QAPIExprError(expr_info,
+                                "Discriminator '%s' is not a member of base "
+                                "type '%s'"
+                                % (discriminator, base))
+        enum_define = discriminator_find_enum_define(expr)
+
+    # Check every branch
+    for (key, value) in members.items():
+        # If this named member's value names an enum type, then all members
+        # of 'data' must also be members of the enum type.
+        if enum_define and not key in enum_define['enum_values']:
+            raise QAPIExprError(expr_info,
+                                "Discriminator value '%s' is not found in "
+                                "enum '%s'" %
+                                (key, enum_define["enum_name"]))
+        # Todo: put more check such as whether each value is valid, but it may
+        # need more functions to handle array case, so leave it now.
+
+def check_exprs(schema):
+    for expr_elem in schema.exprs:
+        expr = expr_elem['expr']
+        if expr.has_key('union'):
+            check_union(expr, expr_elem['info'])
+
 def parse_schema(fp):
     try:
         schema = QAPISchema(fp)
@@ -171,7 +266,8 @@ def parse_schema(fp):
 
     exprs = []
 
-    for expr in schema.exprs:
+    for expr_elem in schema.exprs:
+        expr = expr_elem['expr']
         if expr.has_key('enum'):
             add_enum(expr['enum'], expr['data'])
         elif expr.has_key('union'):
@@ -181,6 +277,12 @@ def parse_schema(fp):
             add_struct(expr)
         exprs.append(expr)
 
+    try:
+        check_exprs(schema)
+    except QAPIExprError, e:
+        print >>sys.stderr, e
+        exit(1)
+
     return exprs
 
 def parse_args(typeinfo):
diff --git a/tests/Makefile b/tests/Makefile
index dfe06eb..6ac9889 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -143,7 +143,9 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
         qapi-schema-test.json quoted-structural-chars.json \
         trailing-comma-list.json trailing-comma-object.json \
         unclosed-list.json unclosed-object.json unclosed-string.json \
-        duplicate-key.json)
+        duplicate-key.json union-invalid-base.json flat-union-no-base.json \
+        flat-union-invalid-discriminator.json \
+        flat-union-invalid-branch-key.json)
 
 GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
 
diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.err b/tests/qapi-schema/flat-union-invalid-branch-key.err
new file mode 100644
index 0000000..1125caf
--- /dev/null
+++ b/tests/qapi-schema/flat-union-invalid-branch-key.err
@@ -0,0 +1 @@
+<stdin>:13: Discriminator value 'value_wrong' is not found in enum 'TestEnum'
diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.exit b/tests/qapi-schema/flat-union-invalid-branch-key.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/flat-union-invalid-branch-key.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.json b/tests/qapi-schema/flat-union-invalid-branch-key.json
new file mode 100644
index 0000000..a624282
--- /dev/null
+++ b/tests/qapi-schema/flat-union-invalid-branch-key.json
@@ -0,0 +1,17 @@
+{ 'enum': 'TestEnum',
+  'data': [ 'value1', 'value2' ] }
+
+{ 'type': 'TestBase',
+  'data': { 'enum1': 'TestEnum' } }
+
+{ 'type': 'TestTypeA',
+  'data': { 'string': 'str' } }
+
+{ 'type': 'TestTypeB',
+  'data': { 'integer': 'int' } }
+
+{ 'union': 'TestUnion',
+  'base': 'TestBase',
+  'discriminator': 'enum1',
+  'data': { 'value_wrong': 'TestTypeA',
+            'value2': 'TestTypeB' } }
diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.out b/tests/qapi-schema/flat-union-invalid-branch-key.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.err b/tests/qapi-schema/flat-union-invalid-discriminator.err
new file mode 100644
index 0000000..cad9dbf
--- /dev/null
+++ b/tests/qapi-schema/flat-union-invalid-discriminator.err
@@ -0,0 +1 @@
+<stdin>:13: Discriminator 'enum_wrong' is not a member of base type 'TestBase'
diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.exit b/tests/qapi-schema/flat-union-invalid-discriminator.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/flat-union-invalid-discriminator.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.json b/tests/qapi-schema/flat-union-invalid-discriminator.json
new file mode 100644
index 0000000..887157e
--- /dev/null
+++ b/tests/qapi-schema/flat-union-invalid-discriminator.json
@@ -0,0 +1,17 @@
+{ 'enum': 'TestEnum',
+  'data': [ 'value1', 'value2' ] }
+
+{ 'type': 'TestBase',
+  'data': { 'enum1': 'TestEnum' } }
+
+{ 'type': 'TestTypeA',
+  'data': { 'string': 'str' } }
+
+{ 'type': 'TestTypeB',
+  'data': { 'integer': 'int' } }
+
+{ 'union': 'TestUnion',
+  'base': 'TestBase',
+  'discriminator': 'enum_wrong',
+  'data': { 'value1': 'TestTypeA',
+            'value2': 'TestTypeB' } }
diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.out b/tests/qapi-schema/flat-union-invalid-discriminator.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/flat-union-no-base.err b/tests/qapi-schema/flat-union-no-base.err
new file mode 100644
index 0000000..e695315
--- /dev/null
+++ b/tests/qapi-schema/flat-union-no-base.err
@@ -0,0 +1 @@
+<stdin>:13: Flat union 'TestUnion' must have a base field
diff --git a/tests/qapi-schema/flat-union-no-base.exit b/tests/qapi-schema/flat-union-no-base.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/flat-union-no-base.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/flat-union-no-base.json b/tests/qapi-schema/flat-union-no-base.json
new file mode 100644
index 0000000..e0900d4
--- /dev/null
+++ b/tests/qapi-schema/flat-union-no-base.json
@@ -0,0 +1,16 @@
+{ 'enum': 'TestEnum',
+  'data': [ 'value1', 'value2' ] }
+
+{ 'type': 'TestBase',
+  'data': { 'enum1': 'TestEnum' } }
+
+{ 'type': 'TestTypeA',
+  'data': { 'string': 'str' } }
+
+{ 'type': 'TestTypeB',
+  'data': { 'integer': 'int' } }
+
+{ 'union': 'TestUnion',
+  'discriminator': 'enum1',
+  'data': { 'value1': 'TestTypeA',
+            'value2': 'TestTypeB' } }
diff --git a/tests/qapi-schema/flat-union-no-base.out b/tests/qapi-schema/flat-union-no-base.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/union-invalid-base.err b/tests/qapi-schema/union-invalid-base.err
new file mode 100644
index 0000000..dd8e3d1
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-base.err
@@ -0,0 +1 @@
+<stdin>:7: Base 'TestBaseWrong' is not a valid type
diff --git a/tests/qapi-schema/union-invalid-base.exit b/tests/qapi-schema/union-invalid-base.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-base.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/union-invalid-base.json b/tests/qapi-schema/union-invalid-base.json
new file mode 100644
index 0000000..1fa4930
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-base.json
@@ -0,0 +1,10 @@
+{ 'type': 'TestTypeA',
+  'data': { 'string': 'str' } }
+
+{ 'type': 'TestTypeB',
+  'data': { 'integer': 'int' } }
+
+{ 'union': 'TestUnion',
+  'base': 'TestBaseWrong',
+  'data': { 'value1': 'TestTypeA',
+            'value2': 'TestTypeB' } }
diff --git a/tests/qapi-schema/union-invalid-base.out b/tests/qapi-schema/union-invalid-base.out
new file mode 100644
index 0000000..e69de29
-- 
1.7.1

[Qemu-devel] [PATCH V8 05/10] qapi script: code move for generate_enum_name()

[Wenchao Xia]

From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Later both qapi-types.py and qapi-visit.py need a common function
for enum name generation.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
---
 scripts/qapi-types.py |   10 ----------
 scripts/qapi.py       |   10 ++++++++++
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 2c6e0dc..35ad993 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -127,16 +127,6 @@ const char *%(name)s_lookup[] = {
 ''')
     return ret
 
-def generate_enum_name(name):
-    if name.isupper():
-        return c_fun(name, False)
-    new_name = ''
-    for c in c_fun(name, False):
-        if c.isupper():
-            new_name += '_'
-        new_name += c
-    return new_name.lstrip('_').upper()
-
 def generate_enum(name, values):
     lookup_decl = mcgen('''
 extern const char *%(name)s_lookup[];
diff --git a/scripts/qapi.py b/scripts/qapi.py
index cea346f..dadf5a5 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -485,3 +485,13 @@ def guardend(name):
 
 ''',
                  name=guardname(name))
+
+def generate_enum_name(name):
+    if name.isupper():
+        return c_fun(name, False)
+    new_name = ''
+    for c in c_fun(name, False):
+        if c.isupper():
+            new_name += '_'
+        new_name += c
+    return new_name.lstrip('_').upper()
-- 
1.7.1

[Qemu-devel] [PATCH V8 06/10] qapi script: use same function to generate enum string

[Wenchao Xia]

From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Prior to this patch, qapi-visit.py used custom code to generate enum
names used for handling a qapi union. Fix it to instead reuse common
code, with identical generated results, and allowing future updates to
generation to only need to touch one place.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
---
 scripts/qapi-types.py |    6 +++---
 scripts/qapi-visit.py |   19 +++++++++++++------
 scripts/qapi.py       |   13 +++++++++----
 3 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 35ad993..5885bac 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -144,11 +144,11 @@ typedef enum %(name)s
 
     i = 0
     for value in enum_values:
+        enum_full_value = generate_enum_full_value(name, value)
         enum_decl += mcgen('''
-    %(abbrev)s_%(value)s = %(i)d,
+    %(enum_full_value)s = %(i)d,
 ''',
-                     abbrev=de_camel_case(name).upper(),
-                     value=generate_enum_name(value),
+                     enum_full_value = enum_full_value,
                      i=i)
         i += 1
 
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index c6de9ae..0baaf60 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -214,18 +214,22 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
 ''',
     name=name)
 
+    # For anon union, always use the default enum type automatically generated
+    # as "'%sKind' % (name)"
+    disc_type = '%sKind' % (name)
+
     for key in members:
         assert (members[key] in builtin_types
             or find_struct(members[key])
             or find_union(members[key])), "Invalid anonymous union member"
 
+        enum_full_value = generate_enum_full_value(disc_type, key)
         ret += mcgen('''
-        case %(abbrev)s_KIND_%(enum)s:
+        case %(enum_full_value)s:
             visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err);
             break;
 ''',
-                abbrev = de_camel_case(name).upper(),
-                enum = c_fun(de_camel_case(key),False).upper(),
+                enum_full_value = enum_full_value,
                 c_type = type_name(members[key]),
                 c_name = c_fun(key))
 
@@ -255,7 +259,10 @@ def generate_visit_union(expr):
         assert not base
         return generate_visit_anon_union(name, members)
 
+    # There will always be a discriminator in the C switch code, by default it
+    # is an enum type generated silently as "'%sKind' % (name)"
     ret = generate_visit_enum('%sKind' % name, members.keys())
+    disc_type = '%sKind' % (name)
 
     if base:
         base_fields = find_struct(base)['data']
@@ -313,13 +320,13 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
                     visit_end_implicit_struct(m, &err);
                 }'''
 
+        enum_full_value = generate_enum_full_value(disc_type, key)
         ret += mcgen('''
-            case %(abbrev)s_KIND_%(enum)s:
+            case %(enum_full_value)s:
                 ''' + fmt + '''
                 break;
 ''',
-                abbrev = de_camel_case(name).upper(),
-                enum = c_fun(de_camel_case(key),False).upper(),
+                enum_full_value = enum_full_value,
                 c_type=type_name(members[key]),
                 c_name=c_fun(key))
 
diff --git a/scripts/qapi.py b/scripts/qapi.py
index dadf5a5..60cc12e 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -486,12 +486,17 @@ def guardend(name):
 ''',
                  name=guardname(name))
 
-def generate_enum_name(name):
-    if name.isupper():
-        return c_fun(name, False)
+def _generate_enum_value_string(value):
+    if value.isupper():
+        return c_fun(value, False)
     new_name = ''
-    for c in c_fun(name, False):
+    for c in c_fun(value, False):
         if c.isupper():
             new_name += '_'
         new_name += c
     return new_name.lstrip('_').upper()
+
+def generate_enum_full_value(enum_name, enum_value):
+    abbrev_string = de_camel_case(enum_name).upper()
+    value_string = _generate_enum_value_string(enum_value)
+    return "%s_%s" % (abbrev_string, value_string)
-- 
1.7.1

[Qemu-devel] [PATCH V8 07/10] qapi script: support enum type as discriminator in union

[Wenchao Xia]

From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

By default, any union will automatically generate a enum type as
"[UnionName]Kind" in C code, and it is duplicated when the discriminator
is specified as a pre-defined enum type in schema. After this patch,
the pre-defined enum type will be really used as the switch case
condition in generated C code, if discriminator is an enum field.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
---
 docs/qapi-code-gen.txt                           |    8 ++++-
 scripts/qapi-types.py                            |   18 ++++++++++---
 scripts/qapi-visit.py                            |   29 +++++++++++++++-------
 scripts/qapi.py                                  |   13 +++++++++-
 tests/Makefile                                   |    2 +-
 tests/qapi-schema/flat-union-reverse-define.exit |    1 +
 tests/qapi-schema/flat-union-reverse-define.json |   17 +++++++++++++
 tests/qapi-schema/flat-union-reverse-define.out  |    9 +++++++
 8 files changed, 80 insertions(+), 17 deletions(-)
 create mode 100644 tests/qapi-schema/flat-union-reverse-define.err
 create mode 100644 tests/qapi-schema/flat-union-reverse-define.exit
 create mode 100644 tests/qapi-schema/flat-union-reverse-define.json
 create mode 100644 tests/qapi-schema/flat-union-reverse-define.out

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 0728f36..a2e7921 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -123,11 +123,15 @@ And it looks like this on the wire:
 
 Flat union types avoid the nesting on the wire. They are used whenever a
 specific field of the base type is declared as the discriminator ('type' is
-then no longer generated). The discriminator must always be a string field.
+then no longer generated). The discriminator can be a string field or a
+predefined enum field. If it is a string field, a hidden enum type will be
+generated as "[UNION_NAME]Kind". If it is an enum field, a compile time check
+will be done to verify the correctness. It is recommended to use an enum field.
 The above example can then be modified as follows:
 
+ { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] }
  { 'type': 'BlockdevCommonOptions',
-   'data': { 'driver': 'str', 'readonly': 'bool' } }
+   'data': { 'driver': 'BlockdevDriver', 'readonly': 'bool' } }
  { 'union': 'BlockdevOptions',
    'base': 'BlockdevCommonOptions',
    'discriminator': 'driver',
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 5885bac..10864ef 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -201,14 +201,21 @@ def generate_union(expr):
     base = expr.get('base')
     discriminator = expr.get('discriminator')
 
+    enum_define = discriminator_find_enum_define(expr)
+    if enum_define:
+        discriminator_type_name = enum_define['enum_name']
+    else:
+        discriminator_type_name = '%sKind' % (name)
+
     ret = mcgen('''
 struct %(name)s
 {
-    %(name)sKind kind;
+    %(discriminator_type_name)s kind;
     union {
         void *data;
 ''',
-                name=name)
+                name=name,
+                discriminator_type_name=discriminator_type_name)
 
     for key in typeinfo:
         ret += mcgen('''
@@ -389,8 +396,11 @@ for expr in exprs:
         fdef.write(generate_enum_lookup(expr['enum'], expr['data']))
     elif expr.has_key('union'):
         ret += generate_fwd_struct(expr['union'], expr['data']) + "\n"
-        ret += generate_enum('%sKind' % expr['union'], expr['data'].keys())
-        fdef.write(generate_enum_lookup('%sKind' % expr['union'], expr['data'].keys()))
+        enum_define = discriminator_find_enum_define(expr)
+        if not enum_define:
+            ret += generate_enum('%sKind' % expr['union'], expr['data'].keys())
+            fdef.write(generate_enum_lookup('%sKind' % expr['union'],
+                                            expr['data'].keys()))
         if expr.get('discriminator') == {}:
             fdef.write(generate_anon_union_qtypes(expr))
     else:
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 0baaf60..45ce3a9 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -259,10 +259,16 @@ def generate_visit_union(expr):
         assert not base
         return generate_visit_anon_union(name, members)
 
-    # There will always be a discriminator in the C switch code, by default it
-    # is an enum type generated silently as "'%sKind' % (name)"
-    ret = generate_visit_enum('%sKind' % name, members.keys())
-    disc_type = '%sKind' % (name)
+    enum_define = discriminator_find_enum_define(expr)
+    if enum_define:
+        # Use the enum type as discriminator
+        ret = ""
+        disc_type = enum_define['enum_name']
+    else:
+        # There will always be a discriminator in the C switch code, by default it
+        # is an enum type generated silently as "'%sKind' % (name)"
+        ret = generate_visit_enum('%sKind' % name, members.keys())
+        disc_type = '%sKind' % (name)
 
     if base:
         base_fields = find_struct(base)['data']
@@ -298,15 +304,16 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
     pop_indent()
 
     if not discriminator:
-        desc_type = "type"
+        disc_key = "type"
     else:
-        desc_type = discriminator
+        disc_key = discriminator
     ret += mcgen('''
-        visit_type_%(name)sKind(m, &(*obj)->kind, "%(type)s", &err);
+        visit_type_%(disc_type)s(m, &(*obj)->kind, "%(disc_key)s", &err);
         if (!err) {
             switch ((*obj)->kind) {
 ''',
-                 name=name, type=desc_type)
+                 disc_type = disc_type,
+                 disc_key = disc_key)
 
     for key in members:
         if not discriminator:
@@ -517,7 +524,11 @@ for expr in exprs:
         ret += generate_visit_list(expr['union'], expr['data'])
         fdef.write(ret)
 
-        ret = generate_decl_enum('%sKind' % expr['union'], expr['data'].keys())
+        enum_define = discriminator_find_enum_define(expr)
+        ret = ""
+        if not enum_define:
+            ret = generate_decl_enum('%sKind' % expr['union'],
+                                     expr['data'].keys())
         ret += generate_declaration(expr['union'], expr['data'])
         fdecl.write(ret)
     elif expr.has_key('enum'):
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 60cc12e..72770c0 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -272,11 +272,22 @@ def parse_schema(fp):
             add_enum(expr['enum'], expr['data'])
         elif expr.has_key('union'):
             add_union(expr)
-            add_enum('%sKind' % expr['union'])
         elif expr.has_key('type'):
             add_struct(expr)
         exprs.append(expr)
 
+    # Try again for hidden UnionKind enum
+    for expr_elem in schema.exprs:
+        expr = expr_elem['expr']
+        if expr.has_key('union'):
+            try:
+                enum_define = discriminator_find_enum_define(expr)
+            except QAPIExprError, e:
+                print >>sys.stderr, e
+                exit(1)
+            if not enum_define:
+                add_enum('%sKind' % expr['union'])
+
     try:
         check_exprs(schema)
     except QAPIExprError, e:
diff --git a/tests/Makefile b/tests/Makefile
index 6ac9889..88fca31 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -145,7 +145,7 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
         unclosed-list.json unclosed-object.json unclosed-string.json \
         duplicate-key.json union-invalid-base.json flat-union-no-base.json \
         flat-union-invalid-discriminator.json \
-        flat-union-invalid-branch-key.json)
+        flat-union-invalid-branch-key.json flat-union-reverse-define.json)
 
 GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
 
diff --git a/tests/qapi-schema/flat-union-reverse-define.err b/tests/qapi-schema/flat-union-reverse-define.err
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/flat-union-reverse-define.exit b/tests/qapi-schema/flat-union-reverse-define.exit
new file mode 100644
index 0000000..573541a
--- /dev/null
+++ b/tests/qapi-schema/flat-union-reverse-define.exit
@@ -0,0 +1 @@
+0
diff --git a/tests/qapi-schema/flat-union-reverse-define.json b/tests/qapi-schema/flat-union-reverse-define.json
new file mode 100644
index 0000000..9ea7e72
--- /dev/null
+++ b/tests/qapi-schema/flat-union-reverse-define.json
@@ -0,0 +1,17 @@
+{ 'union': 'TestUnion',
+  'base': 'TestBase',
+  'discriminator': 'enum1',
+  'data': { 'value1': 'TestTypeA',
+            'value2': 'TestTypeB' } }
+
+{ 'type': 'TestBase',
+  'data': { 'enum1': 'TestEnum' } }
+
+{ 'enum': 'TestEnum',
+  'data': [ 'value1', 'value2' ] }
+
+{ 'type': 'TestTypeA',
+  'data': { 'string': 'str' } }
+
+{ 'type': 'TestTypeB',
+  'data': { 'integer': 'int' } }
diff --git a/tests/qapi-schema/flat-union-reverse-define.out b/tests/qapi-schema/flat-union-reverse-define.out
new file mode 100644
index 0000000..03c952e
--- /dev/null
+++ b/tests/qapi-schema/flat-union-reverse-define.out
@@ -0,0 +1,9 @@
+[OrderedDict([('union', 'TestUnion'), ('base', 'TestBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'TestTypeA'), ('value2', 'TestTypeB')]))]),
+ OrderedDict([('type', 'TestBase'), ('data', OrderedDict([('enum1', 'TestEnum')]))]),
+ OrderedDict([('enum', 'TestEnum'), ('data', ['value1', 'value2'])]),
+ OrderedDict([('type', 'TestTypeA'), ('data', OrderedDict([('string', 'str')]))]),
+ OrderedDict([('type', 'TestTypeB'), ('data', OrderedDict([('integer', 'int')]))])]
+[{'enum_name': 'TestEnum', 'enum_values': ['value1', 'value2']}]
+[OrderedDict([('type', 'TestBase'), ('data', OrderedDict([('enum1', 'TestEnum')]))]),
+ OrderedDict([('type', 'TestTypeA'), ('data', OrderedDict([('string', 'str')]))]),
+ OrderedDict([('type', 'TestTypeB'), ('data', OrderedDict([('integer', 'int')]))])]
-- 
1.7.1

[Qemu-devel] [PATCH V8 08/10] qapi: convert BlockdevOptions to use enum discriminator

[Wenchao Xia]

From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

After this patch, hidden enum type BlockdevOptionsKind will not
be generated, and other API can use enum BlockdevDriver.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
---
 qapi-schema.json |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index fcb2280..7f587b4 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4200,6 +4200,18 @@
             '*no-flush': 'bool' } }
 
 ##
+# @BlockdevDriver
+#
+# Drivers that are supported in block device operations.
+#
+# Since: 2.0
+##
+{ 'enum': 'BlockdevDriver',
+  'data': [ 'file', 'http', 'https', 'ftp', 'ftps', 'tftp', 'vvfat', 'blkdebug',
+            'blkverify', 'bochs', 'cloop', 'cow', 'dmg', 'parallels', 'qcow',
+            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum' ] }
+
+##
 # @BlockdevOptionsBase
 #
 # Options that are available for all block devices, independent of the block
@@ -4223,7 +4235,7 @@
 # Since: 1.7
 ##
 { 'type': 'BlockdevOptionsBase',
-  'data': { 'driver': 'str',
+  'data': { 'driver': 'BlockdevDriver',
             '*id': 'str',
             '*node-name': 'str',
             '*discard': 'BlockdevDiscardOptions',
-- 
1.7.1

[Qemu-devel] [PATCH V8 09/10] qapi script: do not allow string discriminator

[Wenchao Xia]

From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
---
 docs/qapi-code-gen.txt                             |    5 +----
 scripts/qapi.py                                    |    5 +++++
 tests/Makefile                                     |    3 ++-
 .../flat-union-string-discriminator.err            |    1 +
 .../flat-union-string-discriminator.exit           |    1 +
 .../flat-union-string-discriminator.json           |   17 +++++++++++++++++
 tests/qapi-schema/qapi-schema-test.json            |    9 ++++++---
 tests/qapi-schema/qapi-schema-test.out             |    5 +++--
 tests/test-qmp-input-strict.c                      |    5 ++++-
 tests/test-qmp-input-visitor.c                     |   10 +++++++---
 tests/test-qmp-output-visitor.c                    |   10 ++++++----
 11 files changed, 53 insertions(+), 18 deletions(-)
 create mode 100644 tests/qapi-schema/flat-union-string-discriminator.err
 create mode 100644 tests/qapi-schema/flat-union-string-discriminator.exit
 create mode 100644 tests/qapi-schema/flat-union-string-discriminator.json
 create mode 100644 tests/qapi-schema/flat-union-string-discriminator.out

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index a2e7921..d78921f 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -123,10 +123,7 @@ And it looks like this on the wire:
 
 Flat union types avoid the nesting on the wire. They are used whenever a
 specific field of the base type is declared as the discriminator ('type' is
-then no longer generated). The discriminator can be a string field or a
-predefined enum field. If it is a string field, a hidden enum type will be
-generated as "[UNION_NAME]Kind". If it is an enum field, a compile time check
-will be done to verify the correctness. It is recommended to use an enum field.
+then no longer generated). The discriminator must be of enumeration type.
 The above example can then be modified as follows:
 
  { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] }
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 72770c0..fd91f55 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -238,6 +238,11 @@ def check_union(expr, expr_info):
                                 "type '%s'"
                                 % (discriminator, base))
         enum_define = discriminator_find_enum_define(expr)
+        # Do not allow string discriminator
+        if not enum_define:
+            raise QAPIExprError(expr_info,
+                                "Discriminator '%s' must be of enumeration "
+                                "type" % discriminator)
 
     # Check every branch
     for (key, value) in members.items():
diff --git a/tests/Makefile b/tests/Makefile
index 88fca31..e146f81 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -145,7 +145,8 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
         unclosed-list.json unclosed-object.json unclosed-string.json \
         duplicate-key.json union-invalid-base.json flat-union-no-base.json \
         flat-union-invalid-discriminator.json \
-        flat-union-invalid-branch-key.json flat-union-reverse-define.json)
+        flat-union-invalid-branch-key.json flat-union-reverse-define.json \
+        flat-union-string-discriminator.json)
 
 GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
 
diff --git a/tests/qapi-schema/flat-union-string-discriminator.err b/tests/qapi-schema/flat-union-string-discriminator.err
new file mode 100644
index 0000000..8748270
--- /dev/null
+++ b/tests/qapi-schema/flat-union-string-discriminator.err
@@ -0,0 +1 @@
+<stdin>:13: Discriminator 'kind' must be of enumeration type
diff --git a/tests/qapi-schema/flat-union-string-discriminator.exit b/tests/qapi-schema/flat-union-string-discriminator.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/flat-union-string-discriminator.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/flat-union-string-discriminator.json b/tests/qapi-schema/flat-union-string-discriminator.json
new file mode 100644
index 0000000..e966aeb
--- /dev/null
+++ b/tests/qapi-schema/flat-union-string-discriminator.json
@@ -0,0 +1,17 @@
+{ 'enum': 'TestEnum',
+  'data': [ 'value1', 'value2' ] }
+
+{ 'type': 'TestBase',
+  'data': { 'enum1': 'TestEnum', 'kind': 'str' } }
+
+{ 'type': 'TestTypeA',
+  'data': { 'string': 'str' } }
+
+{ 'type': 'TestTypeB',
+  'data': { 'integer': 'int' } }
+
+{ 'union': 'TestUnion',
+  'base': 'TestBase',
+  'discriminator': 'kind',
+  'data': { 'kind1': 'TestTypeA',
+            'kind2': 'TestTypeB' } }
diff --git a/tests/qapi-schema/flat-union-string-discriminator.out b/tests/qapi-schema/flat-union-string-discriminator.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 471ba47..818c06d 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -37,10 +37,13 @@
   'base': 'UserDefZero',
   'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
 
+{ 'type': 'UserDefUnionBase',
+  'data': { 'string': 'str', 'enum1': 'EnumOne' } }
+
 { 'union': 'UserDefFlatUnion',
-  'base': 'UserDefOne',
-  'discriminator': 'string',
-  'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
+  'base': 'UserDefUnionBase',
+  'discriminator': 'enum1',
+  'data': { 'value1' : 'UserDefA', 'value2' : 'UserDefB', 'value3' : 'UserDefB' } }
 # FIXME generated struct UserDefFlatUnion has members for direct base
 # UserDefOne, but lacks members for indirect base UserDefZero
 
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 01685d4..6cd03f3 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -7,7 +7,8 @@
  OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('union', 'UserDefUnion'), ('base', 'UserDefZero'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
- OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefOne'), ('discriminator', 'string'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
+ OrderedDict([('type', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
+ OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefA'), ('value2', 'UserDefB'), ('value3', 'UserDefB')]))]),
  OrderedDict([('union', 'UserDefAnonUnion'), ('discriminator', OrderedDict()), ('data', OrderedDict([('uda', 'UserDefA'), ('s', 'str'), ('i', 'int')]))]),
  OrderedDict([('union', 'UserDefNativeListUnion'), ('data', OrderedDict([('integer', ['int']), ('s8', ['int8']), ('s16', ['int16']), ('s32', ['int32']), ('s64', ['int64']), ('u8', ['uint8']), ('u16', ['uint16']), ('u32', ['uint32']), ('u64', ['uint64']), ('number', ['number']), ('boolean', ['bool']), ('string', ['str'])]))]),
  OrderedDict([('command', 'user_def_cmd'), ('data', OrderedDict())]),
@@ -17,7 +18,6 @@
  OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
 [{'enum_name': 'EnumOne', 'enum_values': ['value1', 'value2', 'value3']},
  {'enum_name': 'UserDefUnionKind', 'enum_values': None},
- {'enum_name': 'UserDefFlatUnionKind', 'enum_values': None},
  {'enum_name': 'UserDefAnonUnionKind', 'enum_values': None},
  {'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None}]
 [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
@@ -27,4 +27,5 @@
  OrderedDict([('type', 'UserDefNested'), ('data', OrderedDict([('string0', 'str'), ('dict1', OrderedDict([('string1', 'str'), ('dict2', OrderedDict([('userdef1', 'UserDefOne'), ('string2', 'str')])), ('*dict3', OrderedDict([('userdef2', 'UserDefOne'), ('string3', 'str')]))]))]))]),
  OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
+ OrderedDict([('type', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
  OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
index 64d72f6..38b5e95 100644
--- a/tests/test-qmp-input-strict.c
+++ b/tests/test-qmp-input-strict.c
@@ -146,7 +146,10 @@ static void test_validate_union_flat(TestInputVisitorData *data,
     Visitor *v;
     Error *errp = NULL;
 
-    v = validate_test_init(data, "{ 'string': 'a', 'boolean': true }");
+    v = validate_test_init(data,
+                           "{ 'enum1': 'value1', "
+                           "'string': 'str', "
+                           "'boolean': true }");
     /* TODO when generator bug is fixed, add 'integer': 41 */
 
     visit_type_UserDefFlatUnion(v, &tmp, NULL, &errp);
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 2dffafc..1729667 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -310,14 +310,18 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data,
     Error *err = NULL;
     UserDefFlatUnion *tmp;
 
-    v = visitor_input_test_init(data, "{ 'string': 'a', 'boolean': true }");
+    v = visitor_input_test_init(data,
+                                "{ 'enum1': 'value1', "
+                                "'string': 'str', "
+                                "'boolean': true }");
     /* TODO when generator bug is fixed, add 'integer': 41 */
 
     visit_type_UserDefFlatUnion(v, &tmp, NULL, &err);
     g_assert(err == NULL);
-    g_assert_cmpint(tmp->kind, ==, USER_DEF_UNION_KIND_A);
+    g_assert_cmpint(tmp->kind, ==, ENUM_ONE_VALUE1);
+    g_assert_cmpstr(tmp->string, ==, "str");
     /* TODO g_assert_cmpint(tmp->integer, ==, 41); */
-    g_assert_cmpint(tmp->a->boolean, ==, true);
+    g_assert_cmpint(tmp->value1->boolean, ==, true);
     qapi_free_UserDefFlatUnion(tmp);
 }
 
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 105f4cf..da27971 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -449,10 +449,11 @@ static void test_visitor_out_union_flat(TestOutputVisitorData *data,
     Error *err = NULL;
 
     UserDefFlatUnion *tmp = g_malloc0(sizeof(UserDefFlatUnion));
-    tmp->kind = USER_DEF_UNION_KIND_A;
-    tmp->a = g_malloc0(sizeof(UserDefA));
+    tmp->kind = ENUM_ONE_VALUE1;
+    tmp->string = g_strdup("str");
+    tmp->value1 = g_malloc0(sizeof(UserDefA));
     /* TODO when generator bug is fixed: tmp->integer = 41; */
-    tmp->a->boolean = true;
+    tmp->value1->boolean = true;
 
     visit_type_UserDefFlatUnion(data->ov, &tmp, NULL, &err);
     g_assert(err == NULL);
@@ -461,7 +462,8 @@ static void test_visitor_out_union_flat(TestOutputVisitorData *data,
     g_assert(qobject_type(arg) == QTYPE_QDICT);
     qdict = qobject_to_qdict(arg);
 
-    g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, "a");
+    g_assert_cmpstr(qdict_get_str(qdict, "enum1"), ==, "value1");
+    g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, "str");
     /* TODO g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 41); */
     g_assert_cmpint(qdict_get_bool(qdict, "boolean"), ==, true);
 
-- 
1.7.1

[Qemu-devel] [PATCH V8 10/10] qapi script: do not add "_" for every capitalized char in enum

[Wenchao Xia]

From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Now "enum AIOContext" will generate AIO_CONTEXT instead of A_I_O_CONTEXT,
"X86CPU" will generate X86_CPU instead of X86_C_P_U.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
---
 include/qapi/qmp/qerror.h |    2 +-
 scripts/qapi.py           |   26 +++++++++++++++++++-------
 target-i386/cpu.c         |    2 +-
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 73c67b7..78db342 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -159,7 +159,7 @@ void qerror_report_err(Error *err);
     ERROR_CLASS_GENERIC_ERROR, "Invalid JSON syntax"
 
 #define QERR_KVM_MISSING_CAP \
-    ERROR_CLASS_K_V_M_MISSING_CAP, "Using KVM without %s, %s unavailable"
+    ERROR_CLASS_KVM_MISSING_CAP, "Using KVM without %s, %s unavailable"
 
 #define QERR_MIGRATION_ACTIVE \
     ERROR_CLASS_GENERIC_ERROR, "There's a migration process in progress"
diff --git a/scripts/qapi.py b/scripts/qapi.py
index fd91f55..7bc4a7c 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -502,17 +502,29 @@ def guardend(name):
 ''',
                  name=guardname(name))
 
-def _generate_enum_value_string(value):
+# ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
+# ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
+# ENUM24_Name -> ENUM24_NAME
+def _generate_enum_string(value):
+    c_fun_str = c_fun(value, False)
     if value.isupper():
-        return c_fun(value, False)
+        return c_fun_str
+
     new_name = ''
-    for c in c_fun(value, False):
-        if c.isupper():
-            new_name += '_'
+    l = len(c_fun_str)
+    for i in range(l):
+        c = c_fun_str[i]
+        # When c is upper and no "_" appears before, do more checks
+        if c.isupper() and (i > 0) and c_fun_str[i - 1] != "_":
+            # Case 1: next string is lower
+            # Case 2: previous string is digit
+            if (i < (l - 1) and c_fun_str[i + 1].islower()) or \
+            c_fun_str[i - 1].isdigit():
+                new_name += '_'
         new_name += c
     return new_name.lstrip('_').upper()
 
 def generate_enum_full_value(enum_name, enum_value):
-    abbrev_string = de_camel_case(enum_name).upper()
-    value_string = _generate_enum_value_string(enum_value)
+    abbrev_string = _generate_enum_string(enum_name)
+    value_string = _generate_enum_string(enum_value)
     return "%s_%s" % (abbrev_string, value_string)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0e8812a..c83ab0f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -315,7 +315,7 @@ typedef struct X86RegisterInfo32 {
 } X86RegisterInfo32;
 
 #define REGISTER(reg) \
-    [R_##reg] = { .name = #reg, .qapi_enum = X86_C_P_U_REGISTER32_##reg }
+    [R_##reg] = { .name = #reg, .qapi_enum = X86_CPU_REGISTER32_##reg }
 X86RegisterInfo32 x86_reg_info_32[CPU_NB_REGS32] = {
     REGISTER(EAX),
     REGISTER(ECX),
-- 
1.7.1

Re: [Qemu-devel] [PATCH V8 01/10] qapi script: remember explicitly defined enum values

[Eric Blake]

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

On 02/27/2014 04:09 AM, Wenchao Xia wrote:
> From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> 
> Later other scripts will need to check the enum values.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>

Having two S-o-B by yourself looks awkward; is there one address we
should be favoring?

> ---
>  scripts/qapi.py                        |   16 +++++++++++-----
>  tests/qapi-schema/comments.out         |    2 +-
>  tests/qapi-schema/qapi-schema-test.out |   10 +++++-----
>  3 files changed, 17 insertions(+), 11 deletions(-)
> 

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V8 02/10] qapi script: add check for duplicated key

[Eric Blake]

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

On 02/27/2014 04:09 AM, Wenchao Xia wrote:
> From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> 
> It is bad that same key was specified twice, especially when a union have

s/have/has/

> two branches with same condition. This patch can prevent it.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>

Again, the double S-o-B is odd.

> ---
>  scripts/qapi.py                      |    2 ++
>  tests/Makefile                       |    3 ++-
>  tests/qapi-schema/duplicate-key.err  |    1 +
>  tests/qapi-schema/duplicate-key.exit |    1 +
>  tests/qapi-schema/duplicate-key.json |    2 ++
>  5 files changed, 8 insertions(+), 1 deletions(-)
>  create mode 100644 tests/qapi-schema/duplicate-key.err
>  create mode 100644 tests/qapi-schema/duplicate-key.exit
>  create mode 100644 tests/qapi-schema/duplicate-key.json
>  create mode 100644 tests/qapi-schema/duplicate-key.out
> 

> +++ b/tests/qapi-schema/duplicate-key.json
> @@ -0,0 +1,2 @@
> +{ 'key': 'value',
> +  'key': 'value' }

This tests a duplicate key in a dictionary.  Since unions use
'data':{dictionary}, that means you caught duplicate branches within a
union.  Based on your test, your patch also has the nice effect of
catching duplicates unrelated to unions!  This test is of a top-level
struct; a bit more realistic might be:

{ 'command': 'foo',
  'command': 'foo', 'data': {} }

Meanwhile, should we also test for duplicates in non-dict locations,
such as:

{ 'enum': 'Foo', 'data': [ 'value', 'value' ] }

or even tests of duplicate top-level items, such as:

{ 'type': 'Dup', 'data': { 'field':'str' } }
{ 'type': 'Dup', 'data': { 'field':'str' } }

But I'm okay with that as a followup, in the interest of getting this
series in.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V8 03/10] qapi script: remember line number in schema parsing

[Eric Blake]

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

On 02/27/2014 04:09 AM, Wenchao Xia wrote:
> From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> 
> Before this patch, 'QAPISchemaError' scans whole input until 'pos'
> to get error line number. After this patch, the scan is avoided since
> line number is remembered in schema parsing. This patch also benefits
> other error report functions, which would be introduced later.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>

Another double S-o-B.

> ---
>  scripts/qapi.py |   14 ++++++++------
>  1 files changed, 8 insertions(+), 6 deletions(-)
> 

My python is not strong, this is a very weak:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V8 04/10] qapi script: check correctness of union

[Eric Blake]

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

On 02/27/2014 04:09 AM, Wenchao Xia wrote:
> From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>

Double-S-o-B. I've also noticed that I'm getting undeliverable mail
rejections from your linux.vnet.ibm.com address:

TCVM.MEGACENTER.DE.IBM.COM unable to deliver following mail to recipient(s):
    <xiawenc@linux.ibm.com>
TCVM.MEGACENTER.DE.IBM.COM received negative reply:
550 5.1.1 <xiawenc@linux.ibm.com>: Recipient address rejected: User
unknown in local recipient table

> ---
>  scripts/qapi.py                                    |  106 +++++++++++++++++++-
>  tests/Makefile                                     |    4 +-

> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 1954292..cea346f 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -50,6 +50,15 @@ class QAPISchemaError(Exception):
>      def __str__(self):
>          return "%s:%s:%s: %s" % (self.fp.name, self.line, self.col, self.msg)
>  
> +class QAPIExprError(Exception):
> +    def __init__(self, expr_info, msg):
> +        self.fp = expr_info['fp']
> +        self.line = expr_info['line']
> +        self.msg = msg
> +
> +    def __str__(self):
> +        return "%s:%s: %s" % (self.fp.name, self.line, self.msg)
> +
>  class QAPISchema:
>  
>      def __init__(self, fp):
> @@ -64,7 +73,10 @@ class QAPISchema:
>          self.accept()
>  
>          while self.tok != None:
> -            self.exprs.append(self.get_expr(False))
> +            expr_info = {'fp': fp, 'line': self.line}
> +            expr_elem = {'expr': self.get_expr(False),
> +                         'info': expr_info}
> +            self.exprs.append(expr_elem)

Should these two hunks be part of 3/10?  Or at least as a separate
patch? Or at least mentioned in the commit message?

> @@ -162,6 +174,89 @@ class QAPISchema:
>              raise QAPISchemaError(self, 'Expected "{", "[" or string')
>          return expr
>  
> +def find_base_fields(base):
> +    base_struct_define = find_struct(base)
> +    if not base_struct_define:
> +        return None
> +    return base_struct_define['data']
> +
> +# Return the discriminator enum define if discrminator is specified as an

s/discrminator/discriminator/


-- 
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 V8 06/10] qapi script: use same function to generate enum string

[Eric Blake]

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

On 02/27/2014 04:09 AM, Wenchao Xia wrote:
> From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> 
> Prior to this patch, qapi-visit.py used custom code to generate enum
> names used for handling a qapi union. Fix it to instead reuse common
> code, with identical generated results, and allowing future updates to
> generation to only need to touch one place.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
> ---

Double S-o-B here and in 5.

>  scripts/qapi-types.py |    6 +++---
>  scripts/qapi-visit.py |   19 +++++++++++++------
>  scripts/qapi.py       |   13 +++++++++----
>  3 files changed, 25 insertions(+), 13 deletions(-)

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


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V8 07/10] qapi script: support enum type as discriminator in union

[Eric Blake]

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

On 02/27/2014 04:09 AM, Wenchao Xia wrote:
> From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> 
> By default, any union will automatically generate a enum type as
> "[UnionName]Kind" in C code, and it is duplicated when the discriminator
> is specified as a pre-defined enum type in schema. After this patch,
> the pre-defined enum type will be really used as the switch case
> condition in generated C code, if discriminator is an enum field.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
> ---

Double S-o-B.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V8 08/10] qapi: convert BlockdevOptions to use enum discriminator

[Eric Blake]

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

On 02/27/2014 04:09 AM, Wenchao Xia wrote:
> From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> 
> After this patch, hidden enum type BlockdevOptionsKind will not
> be generated, and other API can use enum BlockdevDriver.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>

Double S-o-B.

> ---
>  qapi-schema.json |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index fcb2280..7f587b4 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4200,6 +4200,18 @@
>              '*no-flush': 'bool' } }
>  
>  ##
> +# @BlockdevDriver
> +#
> +# Drivers that are supported in block device operations.
> +#
> +# Since: 2.0
> +##
> +{ 'enum': 'BlockdevDriver',
> +  'data': [ 'file', 'http', 'https', 'ftp', 'ftps', 'tftp', 'vvfat', 'blkdebug',
> +            'blkverify', 'bochs', 'cloop', 'cow', 'dmg', 'parallels', 'qcow',
> +            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum' ] }

I see you've picked up 'quorum' since last time :)

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V8 09/10] qapi script: do not allow string discriminator

[Eric Blake]

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

On 02/27/2014 04:09 AM, Wenchao Xia wrote:
> From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>

Double S-o-B.  The commit message is a bit sparse; the one-line summary
mentions what, but for a change like this where you are taking away
functionality, it's also good to mention WHY (hint: enum-based
discriminators provide better type-safety and ensure that future qapi
additions do not forget to adjust dependent unions).

> ---
>  docs/qapi-code-gen.txt                             |    5 +----

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V8 04/10] qapi script: check correctness of union

[Wenchao Xia]

于 2014/2/28 3:21, Eric Blake 写道:
> On 02/27/2014 04:09 AM, Wenchao Xia wrote:
>> From: Wenchao Xia<xiawenc@linux.vnet.ibm.com>
>>
>> Signed-off-by: Wenchao Xia<xiawenc@linux.vnet.ibm.com>
>> Signed-off-by: Wenchao Xia<wenchaoqemu@gmail.com>
> Double-S-o-B. I've also noticed that I'm getting undeliverable mail
> rejections from your linux.vnet.ibm.com address:
>
> TCVM.MEGACENTER.DE.IBM.COM unable to deliver following mail to recipient(s):
>      <xiawenc@linux.ibm.com>
> TCVM.MEGACENTER.DE.IBM.COM received negative reply:
> 550 5.1.1<xiawenc@linux.ibm.com>: Recipient address rejected: User
> unknown in local recipient table
>
   I am using wenchaoqemu@gmail.com now, yep I should fix the SoB problem.
>> ---
>>   scripts/qapi.py                                    |  106 +++++++++++++++++++-
>>   tests/Makefile                                     |    4 +-
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 1954292..cea346f 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -50,6 +50,15 @@ class QAPISchemaError(Exception):
>>       def __str__(self):
>>           return "%s:%s:%s: %s" % (self.fp.name, self.line, self.col, self.msg)
>>
>> +class QAPIExprError(Exception):
>> +    def __init__(self, expr_info, msg):
>> +        self.fp = expr_info['fp']
>> +        self.line = expr_info['line']
>> +        self.msg = msg
>> +
>> +    def __str__(self):
>> +        return "%s:%s: %s" % (self.fp.name, self.line, self.msg)
>> +
>>   class QAPISchema:
>>
>>       def __init__(self, fp):
>> @@ -64,7 +73,10 @@ class QAPISchema:
>>           self.accept()
>>
>>           while self.tok != None:
>> -            self.exprs.append(self.get_expr(False))
>> +            expr_info = {'fp': fp, 'line': self.line}
>> +            expr_elem = {'expr': self.get_expr(False),
>> +                         'info': expr_info}
>> +            self.exprs.append(expr_elem)
> Should these two hunks be part of 3/10?  Or at least as a separate
> patch? Or at least mentioned in the commit message?
>
   Patch 3/10 only remember line number in class QAPISchema, as well
as its member "cursor". The above code add line info in exprs, and
if they are moved to last patch, the caller code should also go, which
is a main part of patch. I'd like to improve the commit messages instead.

>> @@ -162,6 +174,89 @@ class QAPISchema:
>>               raise QAPISchemaError(self, 'Expected "{", "[" or string')
>>           return expr
>>
>> +def find_base_fields(base):
>> +    base_struct_define = find_struct(base)
>> +    if not base_struct_define:
>> +        return None
>> +    return base_struct_define['data']
>> +
>> +# Return the discriminator enum define if discrminator is specified as an
> s/discrminator/discriminator/
>
>

Re: [Qemu-devel] [PATCH V8 00/10] qapi script: support enum as discriminator and better enum name

[Wenchao Xia]

Markus, do you like the way this version works?

Re: [Qemu-devel] [PATCH V8 00/10] qapi script: support enum as discriminator and better enum name

[Markus Armbruster]

Wenchao Xia <wenchaoqemu@gmail.com> writes:

> Markus, do you like the way this version works?

I hope to review it early next week.

Re: [Qemu-devel] [PATCH V8 04/10] qapi script: check correctness of union

[Markus Armbruster]

Wenchao Xia <wenchaoqemu@gmail.com> writes:

> From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Commit message lost detail since v7.  Intentional?

> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
> ---
>  scripts/qapi.py                                    |  106 +++++++++++++++++++-
>  tests/Makefile                                     |    4 +-
>  .../qapi-schema/flat-union-invalid-branch-key.err  |    1 +
>  .../qapi-schema/flat-union-invalid-branch-key.exit |    1 +
>  .../qapi-schema/flat-union-invalid-branch-key.json |   17 +++
>  .../flat-union-invalid-discriminator.err           |    1 +
>  .../flat-union-invalid-discriminator.exit          |    1 +
>  .../flat-union-invalid-discriminator.json          |   17 +++
>  tests/qapi-schema/flat-union-no-base.err           |    1 +
>  tests/qapi-schema/flat-union-no-base.exit          |    1 +
>  tests/qapi-schema/flat-union-no-base.json          |   16 +++
>  tests/qapi-schema/union-invalid-base.err           |    1 +
>  tests/qapi-schema/union-invalid-base.exit          |    1 +
>  tests/qapi-schema/union-invalid-base.json          |   10 ++
>  14 files changed, 175 insertions(+), 3 deletions(-)
>  create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.err
>  create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.exit
>  create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.json
>  create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.out
>  create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.err
>  create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.exit
>  create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.json
>  create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.out
>  create mode 100644 tests/qapi-schema/flat-union-no-base.err
>  create mode 100644 tests/qapi-schema/flat-union-no-base.exit
>  create mode 100644 tests/qapi-schema/flat-union-no-base.json
>  create mode 100644 tests/qapi-schema/flat-union-no-base.out
>  create mode 100644 tests/qapi-schema/union-invalid-base.err
>  create mode 100644 tests/qapi-schema/union-invalid-base.exit
>  create mode 100644 tests/qapi-schema/union-invalid-base.json
>  create mode 100644 tests/qapi-schema/union-invalid-base.out
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 1954292..cea346f 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -50,6 +50,15 @@ class QAPISchemaError(Exception):
>      def __str__(self):
>          return "%s:%s:%s: %s" % (self.fp.name, self.line, self.col, self.msg)
>  
> +class QAPIExprError(Exception):
> +    def __init__(self, expr_info, msg):
> +        self.fp = expr_info['fp']
> +        self.line = expr_info['line']
> +        self.msg = msg
> +
> +    def __str__(self):
> +        return "%s:%s: %s" % (self.fp.name, self.line, self.msg)
> +
>  class QAPISchema:
>  
>      def __init__(self, fp):
> @@ -64,7 +73,10 @@ class QAPISchema:
>          self.accept()
>  
>          while self.tok != None:
> -            self.exprs.append(self.get_expr(False))
> +            expr_info = {'fp': fp, 'line': self.line}
> +            expr_elem = {'expr': self.get_expr(False),
> +                         'info': expr_info}
> +            self.exprs.append(expr_elem)
>  
>      def accept(self):
>          while True:
> @@ -162,6 +174,89 @@ class QAPISchema:
>              raise QAPISchemaError(self, 'Expected "{", "[" or string')
>          return expr
>  
> +def find_base_fields(base):
> +    base_struct_define = find_struct(base)
> +    if not base_struct_define:
> +        return None
> +    return base_struct_define['data']
> +
> +# Return the discriminator enum define if discrminator is specified as an
> +# enum type, otherwise return None.
> +def discriminator_find_enum_define(expr):
> +    base = expr.get('base')
> +    discriminator = expr.get('discriminator')

Why not expr['base'] and expr['discriminator']?

More of the same below.  No need to respin just for that; we can clean
it up on top.

> +
> +    if not (discriminator and base):
> +        return None
> +
> +    base_fields = find_base_fields(base)
> +    if not base_fields:
> +        return None
> +
> +    discriminator_type = base_fields.get(discriminator)
> +    if not discriminator_type:
> +        return None
> +
> +    return find_enum(discriminator_type)
> +
> +def check_union(expr, expr_info):
> +    name = expr['union']
> +    base = expr.get('base')
> +    discriminator = expr.get('discriminator')
> +    members = expr['data']
> +
> +    # If the object has a member 'base', its value must name a complex type.
> +    if base:
> +        base_fields = find_base_fields(base)
> +        if not base_fields:
> +            raise QAPIExprError(expr_info,
> +                                "Base '%s' is not a valid type"
> +                                % base)
> +
> +    # If the union object has no member 'discriminator', it's an
> +    # ordinary union.
> +    if not discriminator:
> +        enum_define = None
> +
> +    # Else if the value of member 'discriminator' is {}, it's an
> +    # anonymous union.
> +    elif discriminator == {}:
> +        enum_define = None
> +
> +    # Else, it's a flat union.
> +    else:
> +        # The object must have a member 'base'.
> +        if not base:
> +            raise QAPIExprError(expr_info,
> +                                "Flat union '%s' must have a base field"
> +                                % name)
> +        # The value of member 'discriminator' must name a member of the
> +        # base type.
> +        if not base_fields.get(discriminator):
> +            raise QAPIExprError(expr_info,
> +                                "Discriminator '%s' is not a member of base "
> +                                "type '%s'"
> +                                % (discriminator, base))
> +        enum_define = discriminator_find_enum_define(expr)

Could this be simplified?  Like this:

           # The value of member 'discriminator' must name a member of the
           # base type.
           discriminator_type = base_fields.get(discriminator):
           if not discriminator_type
               raise QAPIExprError(expr_info,
                                   "Discriminator '%s' is not a member of base "
                                   "type '%s'"
                                   % (discriminator, base))
           enum_define = find_enum(discriminator_type)

It's the only use of discriminator_find_enum_define()...

Hmm, later patches add more uses, so my simplification is probably not
worthwhile.  Anyway, we can simplify on top.

> +
> +    # Check every branch
> +    for (key, value) in members.items():
> +        # If this named member's value names an enum type, then all members
> +        # of 'data' must also be members of the enum type.
> +        if enum_define and not key in enum_define['enum_values']:
> +            raise QAPIExprError(expr_info,
> +                                "Discriminator value '%s' is not found in "
> +                                "enum '%s'" %
> +                                (key, enum_define["enum_name"]))
> +        # Todo: put more check such as whether each value is valid, but it may
> +        # need more functions to handle array case, so leave it now.

I'm not sure I get your comment.

> +
> +def check_exprs(schema):
> +    for expr_elem in schema.exprs:
> +        expr = expr_elem['expr']
> +        if expr.has_key('union'):
> +            check_union(expr, expr_elem['info'])
> +
>  def parse_schema(fp):
>      try:
>          schema = QAPISchema(fp)
> @@ -171,7 +266,8 @@ def parse_schema(fp):
>  
>      exprs = []
>  
> -    for expr in schema.exprs:
> +    for expr_elem in schema.exprs:
> +        expr = expr_elem['expr']
>          if expr.has_key('enum'):
>              add_enum(expr['enum'], expr['data'])
>          elif expr.has_key('union'):
> @@ -181,6 +277,12 @@ def parse_schema(fp):
>              add_struct(expr)
>          exprs.append(expr)
>  
> +    try:
> +        check_exprs(schema)
> +    except QAPIExprError, e:
> +        print >>sys.stderr, e
> +        exit(1)
> +
>      return exprs
>  
>  def parse_args(typeinfo):
> diff --git a/tests/Makefile b/tests/Makefile
> index dfe06eb..6ac9889 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -143,7 +143,9 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
>          qapi-schema-test.json quoted-structural-chars.json \
>          trailing-comma-list.json trailing-comma-object.json \
>          unclosed-list.json unclosed-object.json unclosed-string.json \
> -        duplicate-key.json)
> +        duplicate-key.json union-invalid-base.json flat-union-no-base.json \
> +        flat-union-invalid-discriminator.json \
> +        flat-union-invalid-branch-key.json)
>  
>  GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
>  
> diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.err b/tests/qapi-schema/flat-union-invalid-branch-key.err
> new file mode 100644
> index 0000000..1125caf
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-invalid-branch-key.err
> @@ -0,0 +1 @@
> +<stdin>:13: Discriminator value 'value_wrong' is not found in enum 'TestEnum'
> diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.exit b/tests/qapi-schema/flat-union-invalid-branch-key.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-invalid-branch-key.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.json b/tests/qapi-schema/flat-union-invalid-branch-key.json
> new file mode 100644
> index 0000000..a624282
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-invalid-branch-key.json
> @@ -0,0 +1,17 @@
> +{ 'enum': 'TestEnum',
> +  'data': [ 'value1', 'value2' ] }
> +
> +{ 'type': 'TestBase',
> +  'data': { 'enum1': 'TestEnum' } }
> +
> +{ 'type': 'TestTypeA',
> +  'data': { 'string': 'str' } }
> +
> +{ 'type': 'TestTypeB',
> +  'data': { 'integer': 'int' } }
> +
> +{ 'union': 'TestUnion',
> +  'base': 'TestBase',
> +  'discriminator': 'enum1',
> +  'data': { 'value_wrong': 'TestTypeA',
> +            'value2': 'TestTypeB' } }
> diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.out b/tests/qapi-schema/flat-union-invalid-branch-key.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.err b/tests/qapi-schema/flat-union-invalid-discriminator.err
> new file mode 100644
> index 0000000..cad9dbf
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-invalid-discriminator.err
> @@ -0,0 +1 @@
> +<stdin>:13: Discriminator 'enum_wrong' is not a member of base type 'TestBase'
> diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.exit b/tests/qapi-schema/flat-union-invalid-discriminator.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-invalid-discriminator.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.json b/tests/qapi-schema/flat-union-invalid-discriminator.json
> new file mode 100644
> index 0000000..887157e
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-invalid-discriminator.json
> @@ -0,0 +1,17 @@
> +{ 'enum': 'TestEnum',
> +  'data': [ 'value1', 'value2' ] }
> +
> +{ 'type': 'TestBase',
> +  'data': { 'enum1': 'TestEnum' } }
> +
> +{ 'type': 'TestTypeA',
> +  'data': { 'string': 'str' } }
> +
> +{ 'type': 'TestTypeB',
> +  'data': { 'integer': 'int' } }
> +
> +{ 'union': 'TestUnion',
> +  'base': 'TestBase',
> +  'discriminator': 'enum_wrong',
> +  'data': { 'value1': 'TestTypeA',
> +            'value2': 'TestTypeB' } }
> diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.out b/tests/qapi-schema/flat-union-invalid-discriminator.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/flat-union-no-base.err b/tests/qapi-schema/flat-union-no-base.err
> new file mode 100644
> index 0000000..e695315
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-no-base.err
> @@ -0,0 +1 @@
> +<stdin>:13: Flat union 'TestUnion' must have a base field
> diff --git a/tests/qapi-schema/flat-union-no-base.exit b/tests/qapi-schema/flat-union-no-base.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-no-base.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/flat-union-no-base.json b/tests/qapi-schema/flat-union-no-base.json
> new file mode 100644
> index 0000000..e0900d4
> --- /dev/null
> +++ b/tests/qapi-schema/flat-union-no-base.json
> @@ -0,0 +1,16 @@
> +{ 'enum': 'TestEnum',
> +  'data': [ 'value1', 'value2' ] }
> +
> +{ 'type': 'TestBase',
> +  'data': { 'enum1': 'TestEnum' } }
> +

TestEnum and TestBase aren't used.

> +{ 'type': 'TestTypeA',
> +  'data': { 'string': 'str' } }
> +
> +{ 'type': 'TestTypeB',
> +  'data': { 'integer': 'int' } }
> +
> +{ 'union': 'TestUnion',
> +  'discriminator': 'enum1',
> +  'data': { 'value1': 'TestTypeA',
> +            'value2': 'TestTypeB' } }
> diff --git a/tests/qapi-schema/flat-union-no-base.out b/tests/qapi-schema/flat-union-no-base.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/union-invalid-base.err b/tests/qapi-schema/union-invalid-base.err
> new file mode 100644
> index 0000000..dd8e3d1
> --- /dev/null
> +++ b/tests/qapi-schema/union-invalid-base.err
> @@ -0,0 +1 @@
> +<stdin>:7: Base 'TestBaseWrong' is not a valid type
> diff --git a/tests/qapi-schema/union-invalid-base.exit b/tests/qapi-schema/union-invalid-base.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/union-invalid-base.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/union-invalid-base.json b/tests/qapi-schema/union-invalid-base.json
> new file mode 100644
> index 0000000..1fa4930
> --- /dev/null
> +++ b/tests/qapi-schema/union-invalid-base.json
> @@ -0,0 +1,10 @@
> +{ 'type': 'TestTypeA',
> +  'data': { 'string': 'str' } }
> +
> +{ 'type': 'TestTypeB',
> +  'data': { 'integer': 'int' } }
> +
> +{ 'union': 'TestUnion',
> +  'base': 'TestBaseWrong',
> +  'data': { 'value1': 'TestTypeA',
> +            'value2': 'TestTypeB' } }
> diff --git a/tests/qapi-schema/union-invalid-base.out b/tests/qapi-schema/union-invalid-base.out
> new file mode 100644
> index 0000000..e69de29

Tests cover all the new errors.  Good.

Re: [Qemu-devel] [PATCH V8 00/10] qapi script: support enum as discriminator and better enum name

[Markus Armbruster]

Wenchao Xia <wenchaoqemu@gmail.com> writes:

> This series address two issues:
>
> 1. support using enum as discriminator in union.
> For example, if we have following define in qapi schema:
> { 'enum': 'EnumOne',
>   'data': [ 'value1', 'value2', 'value3' ] }
>
> { 'type': 'UserDefBase0',
>   'data': { 'base-string0': 'str', 'base-enum0': 'EnumOne' } }
>
> Before this series, discriminator in union must be a string, and a
> hidden enum type as discriminator is generated. After this series,
> qapi schema can directly use predefined enum type:
> { 'union': 'UserDefEnumDiscriminatorUnion',
>   'base': 'UserDefBase0',
>   'discriminator' : 'base-enum0',
>   'data': { 'value1' : 'UserDefA',
>             'value2' : 'UserDefInherit',
>             'value3' : 'UserDefB' } }
>
> The benefit is that every thing is defined explicitly in schema file,
> the discriminator enum type can be used in other API define in schema,
> and a compile time check will be put to verify the correctness according
> to enum define. Currently BlockdevOptions used discriminator which can
> be converted, in the future other union can also use enum discriminator.
>
> The implement is done by:
> 1.1 remember the enum defines by qapi scripts.(patch 1)
> 1.2 use the remembered enum define to check correctness at compile
> time.(patch 3), more strict check(patch 2)
> 1.3 use the same enum name generation rule to avoid C code mismatch,
> esp for "case [ENUM_VALUE]" in qapi-visit.c.(patch 4,5)
> 1.4 switch the code path, when pre-defined enum type is used as discriminator,
> don't generate a hidden enum type, use the enum type instead, add
> docs/qapi-code-gen.txt.(Patch 6)
> 1.5 test case shows how it looks like.(Patch 7)
> 1.6 convert BlockdevOptions. (Patch 8)
>
> 2. Better enum name generation
> Before this patch, AIOContext->A_I_O_CONTEXT, after this patch,
> AIOContet->AIO_CONTEXT. Since previous patch has foldered enum
> name generation codes into one function, it is done easily by modifying
> it.(Patch 9)
[...]
> v8:
>   The series is ontop of Markus's tree and rebased on upstream:
>   Address Markus's comments:
>   1/10: better commit title, simplify is_enum().
>   2/10: test case squashed into this patch.
>   3/10: no change, column computation is not touched.
>   4/10: simplify commit title and message, refine the semantic check
> logic as comments in v7, check in discriminator_find_enum_define() is moved
> out so that the function can be used without error info, squash related test
> into this patch, re-orgnize 'expr_elem' with separate 'info' member,
> QAPIExprError now takes only info to work, better error message, remove
> check of whether all enum values are covered by branch, use expr['key']
> instead of expr.get('key') when possible.
>   6/10: remove useless comments in generate_enum_full_value(), make line
> shorter by change variable name.
>   7/10: building 'expr_elem' for discriminator_find_enum_define() is removed,
> make line shorter in qapi-visit.py, add a test case that enum is used before
> define.
>   8/10: rebased on uptream by adding 'quorum' driver type.
>   9/10: better doc and error message, add a test case for string discriminator.
> use string concatenation rather than line continuation for string in C code.

Applies cleanly on Luiz's queue/qmp branch.

I got a few questions on 04/10, but nothing major.  Perhaps you'd like
to respin to address the nits picked by Eric and me.  If not, we can ask
Luiz to clean up commit messages on commit as per Eric's review.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH V8 00/10] qapi script: support enum as discriminator and better enum name

[Luiz Capitulino]

On Tue, 04 Mar 2014 14:03:34 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Wenchao Xia <wenchaoqemu@gmail.com> writes:
> 
> > This series address two issues:
> >
> > 1. support using enum as discriminator in union.
> > For example, if we have following define in qapi schema:
> > { 'enum': 'EnumOne',
> >   'data': [ 'value1', 'value2', 'value3' ] }
> >
> > { 'type': 'UserDefBase0',
> >   'data': { 'base-string0': 'str', 'base-enum0': 'EnumOne' } }
> >
> > Before this series, discriminator in union must be a string, and a
> > hidden enum type as discriminator is generated. After this series,
> > qapi schema can directly use predefined enum type:
> > { 'union': 'UserDefEnumDiscriminatorUnion',
> >   'base': 'UserDefBase0',
> >   'discriminator' : 'base-enum0',
> >   'data': { 'value1' : 'UserDefA',
> >             'value2' : 'UserDefInherit',
> >             'value3' : 'UserDefB' } }
> >
> > The benefit is that every thing is defined explicitly in schema file,
> > the discriminator enum type can be used in other API define in schema,
> > and a compile time check will be put to verify the correctness according
> > to enum define. Currently BlockdevOptions used discriminator which can
> > be converted, in the future other union can also use enum discriminator.
> >
> > The implement is done by:
> > 1.1 remember the enum defines by qapi scripts.(patch 1)
> > 1.2 use the remembered enum define to check correctness at compile
> > time.(patch 3), more strict check(patch 2)
> > 1.3 use the same enum name generation rule to avoid C code mismatch,
> > esp for "case [ENUM_VALUE]" in qapi-visit.c.(patch 4,5)
> > 1.4 switch the code path, when pre-defined enum type is used as discriminator,
> > don't generate a hidden enum type, use the enum type instead, add
> > docs/qapi-code-gen.txt.(Patch 6)
> > 1.5 test case shows how it looks like.(Patch 7)
> > 1.6 convert BlockdevOptions. (Patch 8)
> >
> > 2. Better enum name generation
> > Before this patch, AIOContext->A_I_O_CONTEXT, after this patch,
> > AIOContet->AIO_CONTEXT. Since previous patch has foldered enum
> > name generation codes into one function, it is done easily by modifying
> > it.(Patch 9)
> [...]
> > v8:
> >   The series is ontop of Markus's tree and rebased on upstream:
> >   Address Markus's comments:
> >   1/10: better commit title, simplify is_enum().
> >   2/10: test case squashed into this patch.
> >   3/10: no change, column computation is not touched.
> >   4/10: simplify commit title and message, refine the semantic check
> > logic as comments in v7, check in discriminator_find_enum_define() is moved
> > out so that the function can be used without error info, squash related test
> > into this patch, re-orgnize 'expr_elem' with separate 'info' member,
> > QAPIExprError now takes only info to work, better error message, remove
> > check of whether all enum values are covered by branch, use expr['key']
> > instead of expr.get('key') when possible.
> >   6/10: remove useless comments in generate_enum_full_value(), make line
> > shorter by change variable name.
> >   7/10: building 'expr_elem' for discriminator_find_enum_define() is removed,
> > make line shorter in qapi-visit.py, add a test case that enum is used before
> > define.
> >   8/10: rebased on uptream by adding 'quorum' driver type.
> >   9/10: better doc and error message, add a test case for string discriminator.
> > use string concatenation rather than line continuation for string in C code.
> 
> Applies cleanly on Luiz's queue/qmp branch.
> 
> I got a few questions on 04/10, but nothing major.  Perhaps you'd like
> to respin to address the nits picked by Eric and me.  If not, we can ask
> Luiz to clean up commit messages on commit as per Eric's review.

Not sure if that's what you meant, but Wenchao could respin only 04/10.

> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Re: [Qemu-devel] [PATCH V8 00/10] qapi script: support enum as discriminator and better enum name

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 04 Mar 2014 14:03:34 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
[....]
>> Applies cleanly on Luiz's queue/qmp branch.
>> 
>> I got a few questions on 04/10, but nothing major.  Perhaps you'd like
>> to respin to address the nits picked by Eric and me.  If not, we can ask
>> Luiz to clean up commit messages on commit as per Eric's review.
>
> Not sure if that's what you meant, but Wenchao could respin only 04/10.

Eric pointed out double S-o-B and a misspelling or two.

[...]

Re: [Qemu-devel] [PATCH V8 04/10] qapi script: check correctness of union

[Wenchao Xia]

于 2014/3/4 20:47, Markus Armbruster 写道:
> Wenchao Xia <wenchaoqemu@gmail.com> writes:
>
>> From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Commit message lost detail since v7.  Intentional?
>
I thought you don't want the message in V7, maybe I misunderstood it.


>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
>> ---
>>  scripts/qapi.py                                    |  106 +++++++++++++++++++-
>>  tests/Makefile                                     |    4 +-
>>  .../qapi-schema/flat-union-invalid-branch-key.err  |    1 +
>>  .../qapi-schema/flat-union-invalid-branch-key.exit |    1 +
>>  .../qapi-schema/flat-union-invalid-branch-key.json |   17 +++
>>  .../flat-union-invalid-discriminator.err           |    1 +
>>  .../flat-union-invalid-discriminator.exit          |    1 +
>>  .../flat-union-invalid-discriminator.json          |   17 +++
>>  tests/qapi-schema/flat-union-no-base.err           |    1 +
>>  tests/qapi-schema/flat-union-no-base.exit          |    1 +
>>  tests/qapi-schema/flat-union-no-base.json          |   16 +++
>>  tests/qapi-schema/union-invalid-base.err           |    1 +
>>  tests/qapi-schema/union-invalid-base.exit          |    1 +
>>  tests/qapi-schema/union-invalid-base.json          |   10 ++
>>  14 files changed, 175 insertions(+), 3 deletions(-)
>>  create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.err
>>  create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.exit
>>  create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.json
>>  create mode 100644 tests/qapi-schema/flat-union-invalid-branch-key.out
>>  create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.err
>>  create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.exit
>>  create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.json
>>  create mode 100644 tests/qapi-schema/flat-union-invalid-discriminator.out
>>  create mode 100644 tests/qapi-schema/flat-union-no-base.err
>>  create mode 100644 tests/qapi-schema/flat-union-no-base.exit
>>  create mode 100644 tests/qapi-schema/flat-union-no-base.json
>>  create mode 100644 tests/qapi-schema/flat-union-no-base.out
>>  create mode 100644 tests/qapi-schema/union-invalid-base.err
>>  create mode 100644 tests/qapi-schema/union-invalid-base.exit
>>  create mode 100644 tests/qapi-schema/union-invalid-base.json
>>  create mode 100644 tests/qapi-schema/union-invalid-base.out
>>
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 1954292..cea346f 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -50,6 +50,15 @@ class QAPISchemaError(Exception):
>>      def __str__(self):
>>          return "%s:%s:%s: %s" % (self.fp.name, self.line, self.col, self.msg)
>>  
>> +class QAPIExprError(Exception):
>> +    def __init__(self, expr_info, msg):
>> +        self.fp = expr_info['fp']
>> +        self.line = expr_info['line']
>> +        self.msg = msg
>> +
>> +    def __str__(self):
>> +        return "%s:%s: %s" % (self.fp.name, self.line, self.msg)
>> +
>>  class QAPISchema:
>>  
>>      def __init__(self, fp):
>> @@ -64,7 +73,10 @@ class QAPISchema:
>>          self.accept()
>>  
>>          while self.tok != None:
>> -            self.exprs.append(self.get_expr(False))
>> +            expr_info = {'fp': fp, 'line': self.line}
>> +            expr_elem = {'expr': self.get_expr(False),
>> +                         'info': expr_info}
>> +            self.exprs.append(expr_elem)
>>  
>>      def accept(self):
>>          while True:
>> @@ -162,6 +174,89 @@ class QAPISchema:
>>              raise QAPISchemaError(self, 'Expected "{", "[" or string')
>>          return expr
>>  
>> +def find_base_fields(base):
>> +    base_struct_define = find_struct(base)
>> +    if not base_struct_define:
>> +        return None
>> +    return base_struct_define['data']
>> +
>> +# Return the discriminator enum define if discrminator is specified as an
>> +# enum type, otherwise return None.
>> +def discriminator_find_enum_define(expr):
>> +    base = expr.get('base')
>> +    discriminator = expr.get('discriminator')
> Why not expr['base'] and expr['discriminator']?
>
> More of the same below.  No need to respin just for that; we can clean
> it up on top.
>
It is possible 'base' not present in some caller, so use .get to
avoid python error.

>> +
>> +    if not (discriminator and base):
>> +        return None
>> +
>> +    base_fields = find_base_fields(base)
>> +    if not base_fields:
>> +        return None
>> +
>> +    discriminator_type = base_fields.get(discriminator)
>> +    if not discriminator_type:
>> +        return None
>> +
>> +    return find_enum(discriminator_type)
>> +
>> +def check_union(expr, expr_info):
>> +    name = expr['union']
>> +    base = expr.get('base')
>> +    discriminator = expr.get('discriminator')
>> +    members = expr['data']
>> +
>> +    # If the object has a member 'base', its value must name a complex type.
>> +    if base:
>> +        base_fields = find_base_fields(base)
>> +        if not base_fields:
>> +            raise QAPIExprError(expr_info,
>> +                                "Base '%s' is not a valid type"
>> +                                % base)
>> +
>> +    # If the union object has no member 'discriminator', it's an
>> +    # ordinary union.
>> +    if not discriminator:
>> +        enum_define = None
>> +
>> +    # Else if the value of member 'discriminator' is {}, it's an
>> +    # anonymous union.
>> +    elif discriminator == {}:
>> +        enum_define = None
>> +
>> +    # Else, it's a flat union.
>> +    else:
>> +        # The object must have a member 'base'.
>> +        if not base:
>> +            raise QAPIExprError(expr_info,
>> +                                "Flat union '%s' must have a base field"
>> +                                % name)
>> +        # The value of member 'discriminator' must name a member of the
>> +        # base type.
>> +        if not base_fields.get(discriminator):
>> +            raise QAPIExprError(expr_info,
>> +                                "Discriminator '%s' is not a member of base "
>> +                                "type '%s'"
>> +                                % (discriminator, base))
>> +        enum_define = discriminator_find_enum_define(expr)
> Could this be simplified?  Like this:
>
>            # The value of member 'discriminator' must name a member of the
>            # base type.
>            discriminator_type = base_fields.get(discriminator):
>            if not discriminator_type
>                raise QAPIExprError(expr_info,
>                                    "Discriminator '%s' is not a member of base "
>                                    "type '%s'"
>                                    % (discriminator, base))
>            enum_define = find_enum(discriminator_type)
>
> It's the only use of discriminator_find_enum_define()...
>
> Hmm, later patches add more uses, so my simplification is probably not
> worthwhile.  Anyway, we can simplify on top.
>
>> +
>> +    # Check every branch
>> +    for (key, value) in members.items():
>> +        # If this named member's value names an enum type, then all members
>> +        # of 'data' must also be members of the enum type.
>> +        if enum_define and not key in enum_define['enum_values']:
>> +            raise QAPIExprError(expr_info,
>> +                                "Discriminator value '%s' is not found in "
>> +                                "enum '%s'" %
>> +                                (key, enum_define["enum_name"]))
>> +        # Todo: put more check such as whether each value is valid, but it may
>> +        # need more functions to handle array case, so leave it now.
> I'm not sure I get your comment.
>
Above only checks key, and I found it is possible to check every
branch's value,
so leaves a comments here.

>> +
>> +def check_exprs(schema):
>> +    for expr_elem in schema.exprs:
>> +        expr = expr_elem['expr']
>> +        if expr.has_key('union'):
>> +            check_union(expr, expr_elem['info'])
>> +
>>  def parse_schema(fp):
>>      try:
>>          schema = QAPISchema(fp)
>> @@ -171,7 +266,8 @@ def parse_schema(fp):
>>  
>>      exprs = []
>>  
>> -    for expr in schema.exprs:
>> +    for expr_elem in schema.exprs:
>> +        expr = expr_elem['expr']
>>          if expr.has_key('enum'):
>>              add_enum(expr['enum'], expr['data'])
>>          elif expr.has_key('union'):
>> @@ -181,6 +277,12 @@ def parse_schema(fp):
>>              add_struct(expr)
>>          exprs.append(expr)
>>  
>> +    try:
>> +        check_exprs(schema)
>> +    except QAPIExprError, e:
>> +        print >>sys.stderr, e
>> +        exit(1)
>> +
>>      return exprs
>>  
>>  def parse_args(typeinfo):
>> diff --git a/tests/Makefile b/tests/Makefile
>> index dfe06eb..6ac9889 100644
>> --- a/tests/Makefile
>> +++ b/tests/Makefile
>> @@ -143,7 +143,9 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
>>          qapi-schema-test.json quoted-structural-chars.json \
>>          trailing-comma-list.json trailing-comma-object.json \
>>          unclosed-list.json unclosed-object.json unclosed-string.json \
>> -        duplicate-key.json)
>> +        duplicate-key.json union-invalid-base.json flat-union-no-base.json \
>> +        flat-union-invalid-discriminator.json \
>> +        flat-union-invalid-branch-key.json)
>>  
>>  GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
>>  
>> diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.err b/tests/qapi-schema/flat-union-invalid-branch-key.err
>> new file mode 100644
>> index 0000000..1125caf
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-invalid-branch-key.err
>> @@ -0,0 +1 @@
>> +<stdin>:13: Discriminator value 'value_wrong' is not found in enum 'TestEnum'
>> diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.exit b/tests/qapi-schema/flat-union-invalid-branch-key.exit
>> new file mode 100644
>> index 0000000..d00491f
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-invalid-branch-key.exit
>> @@ -0,0 +1 @@
>> +1
>> diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.json b/tests/qapi-schema/flat-union-invalid-branch-key.json
>> new file mode 100644
>> index 0000000..a624282
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-invalid-branch-key.json
>> @@ -0,0 +1,17 @@
>> +{ 'enum': 'TestEnum',
>> +  'data': [ 'value1', 'value2' ] }
>> +
>> +{ 'type': 'TestBase',
>> +  'data': { 'enum1': 'TestEnum' } }
>> +
>> +{ 'type': 'TestTypeA',
>> +  'data': { 'string': 'str' } }
>> +
>> +{ 'type': 'TestTypeB',
>> +  'data': { 'integer': 'int' } }
>> +
>> +{ 'union': 'TestUnion',
>> +  'base': 'TestBase',
>> +  'discriminator': 'enum1',
>> +  'data': { 'value_wrong': 'TestTypeA',
>> +            'value2': 'TestTypeB' } }
>> diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.out b/tests/qapi-schema/flat-union-invalid-branch-key.out
>> new file mode 100644
>> index 0000000..e69de29
>> diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.err b/tests/qapi-schema/flat-union-invalid-discriminator.err
>> new file mode 100644
>> index 0000000..cad9dbf
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-invalid-discriminator.err
>> @@ -0,0 +1 @@
>> +<stdin>:13: Discriminator 'enum_wrong' is not a member of base type 'TestBase'
>> diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.exit b/tests/qapi-schema/flat-union-invalid-discriminator.exit
>> new file mode 100644
>> index 0000000..d00491f
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-invalid-discriminator.exit
>> @@ -0,0 +1 @@
>> +1
>> diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.json b/tests/qapi-schema/flat-union-invalid-discriminator.json
>> new file mode 100644
>> index 0000000..887157e
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-invalid-discriminator.json
>> @@ -0,0 +1,17 @@
>> +{ 'enum': 'TestEnum',
>> +  'data': [ 'value1', 'value2' ] }
>> +
>> +{ 'type': 'TestBase',
>> +  'data': { 'enum1': 'TestEnum' } }
>> +
>> +{ 'type': 'TestTypeA',
>> +  'data': { 'string': 'str' } }
>> +
>> +{ 'type': 'TestTypeB',
>> +  'data': { 'integer': 'int' } }
>> +
>> +{ 'union': 'TestUnion',
>> +  'base': 'TestBase',
>> +  'discriminator': 'enum_wrong',
>> +  'data': { 'value1': 'TestTypeA',
>> +            'value2': 'TestTypeB' } }
>> diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.out b/tests/qapi-schema/flat-union-invalid-discriminator.out
>> new file mode 100644
>> index 0000000..e69de29
>> diff --git a/tests/qapi-schema/flat-union-no-base.err b/tests/qapi-schema/flat-union-no-base.err
>> new file mode 100644
>> index 0000000..e695315
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-no-base.err
>> @@ -0,0 +1 @@
>> +<stdin>:13: Flat union 'TestUnion' must have a base field
>> diff --git a/tests/qapi-schema/flat-union-no-base.exit b/tests/qapi-schema/flat-union-no-base.exit
>> new file mode 100644
>> index 0000000..d00491f
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-no-base.exit
>> @@ -0,0 +1 @@
>> +1
>> diff --git a/tests/qapi-schema/flat-union-no-base.json b/tests/qapi-schema/flat-union-no-base.json
>> new file mode 100644
>> index 0000000..e0900d4
>> --- /dev/null
>> +++ b/tests/qapi-schema/flat-union-no-base.json
>> @@ -0,0 +1,16 @@
>> +{ 'enum': 'TestEnum',
>> +  'data': [ 'value1', 'value2' ] }
>> +
>> +{ 'type': 'TestBase',
>> +  'data': { 'enum1': 'TestEnum' } }
>> +
> TestEnum and TestBase aren't used.
>
will remove.

>> +{ 'type': 'TestTypeA',
>> +  'data': { 'string': 'str' } }
>> +
>> +{ 'type': 'TestTypeB',
>> +  'data': { 'integer': 'int' } }
>> +
>> +{ 'union': 'TestUnion',
>> +  'discriminator': 'enum1',
>> +  'data': { 'value1': 'TestTypeA',
>> +            'value2': 'TestTypeB' } }
>> diff --git a/tests/qapi-schema/flat-union-no-base.out b/tests/qapi-schema/flat-union-no-base.out
>> new file mode 100644
>> index 0000000..e69de29
>> diff --git a/tests/qapi-schema/union-invalid-base.err b/tests/qapi-schema/union-invalid-base.err
>> new file mode 100644
>> index 0000000..dd8e3d1
>> --- /dev/null
>> +++ b/tests/qapi-schema/union-invalid-base.err
>> @@ -0,0 +1 @@
>> +<stdin>:7: Base 'TestBaseWrong' is not a valid type
>> diff --git a/tests/qapi-schema/union-invalid-base.exit b/tests/qapi-schema/union-invalid-base.exit
>> new file mode 100644
>> index 0000000..d00491f
>> --- /dev/null
>> +++ b/tests/qapi-schema/union-invalid-base.exit
>> @@ -0,0 +1 @@
>> +1
>> diff --git a/tests/qapi-schema/union-invalid-base.json b/tests/qapi-schema/union-invalid-base.json
>> new file mode 100644
>> index 0000000..1fa4930
>> --- /dev/null
>> +++ b/tests/qapi-schema/union-invalid-base.json
>> @@ -0,0 +1,10 @@
>> +{ 'type': 'TestTypeA',
>> +  'data': { 'string': 'str' } }
>> +
>> +{ 'type': 'TestTypeB',
>> +  'data': { 'integer': 'int' } }
>> +
>> +{ 'union': 'TestUnion',
>> +  'base': 'TestBaseWrong',
>> +  'data': { 'value1': 'TestTypeA',
>> +            'value2': 'TestTypeB' } }
>> diff --git a/tests/qapi-schema/union-invalid-base.out b/tests/qapi-schema/union-invalid-base.out
>> new file mode 100644
>> index 0000000..e69de29
> Tests cover all the new errors.  Good.

Re: [Qemu-devel] [PATCH V8 00/10] qapi script: support enum as discriminator and better enum name

[Wenchao Xia]

于 2014/3/4 21:53, Markus Armbruster 写道:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
>> On Tue, 04 Mar 2014 14:03:34 +0100
>> Markus Armbruster <armbru@redhat.com> wrote:
> [....]
>>> Applies cleanly on Luiz's queue/qmp branch.
>>>
>>> I got a few questions on 04/10, but nothing major.  Perhaps you'd like
>>> to respin to address the nits picked by Eric and me.  If not, we can ask
>>> Luiz to clean up commit messages on commit as per Eric's review.
>> Not sure if that's what you meant, but Wenchao could respin only 04/10.
> Eric pointed out double S-o-B and a misspelling or two.
>
> [...]
I will respin it tommorrow, thanks for your review.