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

[Qemu-devel] [PATCH V6 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.

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

 docs/qapi-code-gen.txt                  |    8 ++-
 include/qapi/qmp/qerror.h               |    2 +-
 qapi-schema.json                        |   14 ++++-
 scripts/qapi-types.py                   |   34 +++++-----
 scripts/qapi-visit.py                   |   61 ++++++++++++++---
 scripts/qapi.py                         |   84 +++++++++++++++++++++--
 target-i386/cpu.c                       |    2 +-
 tests/qapi-schema/comments.out          |    2 +-
 tests/qapi-schema/qapi-schema-test.json |   22 ++++++
 tests/qapi-schema/qapi-schema-test.out  |   13 +++-
 tests/test-qmp-input-visitor.c          |   93 +++++++++++++++++++++++++
 tests/test-qmp-output-visitor.c         |  116 +++++++++++++++++++++++++++++++
 12 files changed, 411 insertions(+), 40 deletions(-)

[Qemu-devel] [PATCH V6 01/10] qapi script: remember enum values

[Wenchao Xia]

Later other scripts will need to check the enum values.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi.py                        |   18 ++++++++++++++----
 tests/qapi-schema/comments.out         |    2 +-
 tests/qapi-schema/qapi-schema-test.out |    4 +++-
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 9b3de4c..aec6bbb 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,23 @@ 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 find_enum(name):
+    global enum_types
+    for enum in enum_types:
+        if enum['enum_name'] == name:
+            return enum
+    return None
 
 def is_enum(name):
     global enum_types
-    return (name in enum_types)
+    for enum in enum_types:
+        if enum['enum_name'] == name:
+            return True
+    return False
 
 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 3851880..ad74cdb 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -11,7 +11,9 @@
  OrderedDict([('command', 'user_def_cmd1'), ('data', OrderedDict([('ud1a', 'UserDefOne')]))]),
  OrderedDict([('command', 'user_def_cmd2'), ('data', OrderedDict([('ud1a', 'UserDefOne'), ('ud1b', 'UserDefOne')])), ('returns', 'UserDefTwo')]),
  OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
-['EnumOne', 'UserDefUnionKind', 'UserDefNativeListUnionKind']
+[{'enum_name': 'EnumOne', 'enum_values': ['value1', 'value2', 'value3']},
+ {'enum_name': 'UserDefUnionKind', 'enum_values': None},
+ {'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None}]
 [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
  OrderedDict([('type', 'UserDefOne'), ('data', OrderedDict([('integer', 'int'), ('string', 'str'), ('*enum1', 'EnumOne')]))]),
  OrderedDict([('type', 'UserDefTwo'), ('data', OrderedDict([('string', 'str'), ('dict', OrderedDict([('string', 'str'), ('dict', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')])), ('*dict2', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')]))]))]))]),
-- 
1.7.1

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

[Wenchao Xia]

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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi.py |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index aec6bbb..cf34768 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, 'Duplicated key "%s"' % key)
             expr[key] = self.get_expr(True)
             if self.tok == '}':
                 self.accept()
-- 
1.7.1

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

[Markus Armbruster]

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

> 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>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  scripts/qapi.py |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index aec6bbb..cf34768 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, 'Duplicated key "%s"' % key)
>              expr[key] = self.get_expr(True)
>              if self.tok == '}':
>                  self.accept()

All errors should have a test in tests/qapi-schema/.  I can try to add
tests for you when I rebase your 09/10.

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

[Wenchao Xia]

于 2014/2/13 23:14, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 
>> 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>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   scripts/qapi.py |    2 ++
>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index aec6bbb..cf34768 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, 'Duplicated key "%s"' % key)
>>               expr[key] = self.get_expr(True)
>>               if self.tok == '}':
>>                   self.accept()
> 
> All errors should have a test in tests/qapi-schema/.  I can try to add
> tests for you when I rebase your 09/10.
> 
  I considered error path test before but didn't find a good place to
go. It would be great if you can add one.

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

[Markus Armbruster]

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

> 于 2014/2/13 23:14, Markus Armbruster 写道:
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>> 
>>> 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>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>   scripts/qapi.py |    2 ++
>>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>>> index aec6bbb..cf34768 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, 'Duplicated key "%s"' % key)
>>>               expr[key] = self.get_expr(True)
>>>               if self.tok == '}':
>>>                   self.accept()
>> 
>> All errors should have a test in tests/qapi-schema/.  I can try to add
>> tests for you when I rebase your 09/10.
>> 
>   I considered error path test before but didn't find a good place to
> go. It would be great if you can add one.

Here's the test for your commit, feel free to squash it into yours.


>From ce842b83cd999ee76e4221e24313a5c447e40bac Mon Sep 17 00:00:00 2001
From: Markus Armbruster <armbru@redhat.com>
Date: Fri, 14 Feb 2014 13:15:46 +0100
Subject: [PATCH] qapi: Polish error message for duplicate key, and add test

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi.py                      | 2 +-
 tests/Makefile                       | 4 ++--
 tests/qapi-schema/duplicate-key.err  | 1 +
 tests/qapi-schema/duplicate-key.exit | 1 +
 tests/qapi-schema/duplicate-key.json | 2 ++
 tests/qapi-schema/duplicate-key.out  | 0
 6 files changed, 7 insertions(+), 3 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 0663c2e..3732fe1 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -117,7 +117,7 @@ class QAPISchema:
                 raise QAPISchemaError(self, 'Expected ":"')
             self.accept()
             if key in expr:
-                raise QAPISchemaError(self, 'Duplicated key "%s"' % key)
+                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 fd36eee..e3ddbcc 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -119,8 +119,8 @@ check-qtest-xtensa-y += tests/qom-test$(EXESUF)
 check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
 
 check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
-        comments.json empty.json funny-char.json indented-expr.json \
-        missing-colon.json missing-comma-list.json \
+        comments.json duplicate-key.json empty.json funny-char.json \
+        indented-expr.json missing-colon.json missing-comma-list.json \
         missing-comma-object.json non-objects.json \
         qapi-schema-test.json quoted-structural-chars.json \
         trailing-comma-list.json trailing-comma-object.json \
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.8.1.4

[Qemu-devel] [PATCH V6 03/10] qapi script: check correctness of discriminator values in union

[Wenchao Xia]

It will check whether the values specified are written correctly,
and whether all enum values are covered, when discriminator is a
pre-defined enum type

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi-visit.py |   17 +++++++++++++++++
 scripts/qapi.py       |   31 +++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 65f1a54..c0efb5f 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -255,6 +255,23 @@ def generate_visit_union(expr):
         assert not base
         return generate_visit_anon_union(name, members)
 
+    # If discriminator is specified and it is a pre-defined enum in schema,
+    # check its correctness
+    enum_define = discriminator_find_enum_define(expr)
+    if enum_define:
+        for key in members:
+            if not key in enum_define["enum_values"]:
+                sys.stderr.write("Discriminator value '%s' is not found in "
+                                 "enum '%s'\n" %
+                                 (key, enum_define["enum_name"]))
+                sys.exit(1)
+        for key in enum_define["enum_values"]:
+            if not key in members:
+                sys.stderr.write("Enum value '%s' is not covered by a branch "
+                                 "of union '%s'\n" %
+                                 (key, name))
+                sys.exit(1)
+
     ret = generate_visit_enum('%sKind' % name, members.keys())
 
     if base:
diff --git a/scripts/qapi.py b/scripts/qapi.py
index cf34768..0a3ab80 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -385,3 +385,34 @@ def guardend(name):
 
 ''',
                  name=guardname(name))
+
+# This function can be used to check whether "base" is valid
+def find_base_fields(base):
+    base_struct_define = find_struct(base)
+    if not base_struct_define:
+        return None
+    return base_struct_define.get('data')
+
+# Return the discriminator enum define, if discriminator is specified in
+# @expr and it is a pre-defined enum type
+def discriminator_find_enum_define(expr):
+    discriminator = expr.get('discriminator')
+    base = expr.get('base')
+
+    # Only support discriminator when base present
+    if not (discriminator and base):
+        return None
+
+    base_fields = find_base_fields(base)
+
+    if not base_fields:
+        raise StandardError("Base '%s' is not a valid type\n"
+                            % base)
+
+    discriminator_type = base_fields.get(discriminator)
+
+    if not discriminator_type:
+        raise StandardError("Discriminator '%s' not found in schema\n"
+                            % discriminator)
+
+    return find_enum(discriminator_type)
-- 
1.7.1

Re: [Qemu-devel] [PATCH V6 03/10] qapi script: check correctness of discriminator values in union

[Markus Armbruster]

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

> It will check whether the values specified are written correctly,
> and whether all enum values are covered, when discriminator is a
> pre-defined enum type
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  scripts/qapi-visit.py |   17 +++++++++++++++++
>  scripts/qapi.py       |   31 +++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 0 deletions(-)
>
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 65f1a54..c0efb5f 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -255,6 +255,23 @@ def generate_visit_union(expr):
>          assert not base
>          return generate_visit_anon_union(name, members)
>  
> +    # If discriminator is specified and it is a pre-defined enum in schema,
> +    # check its correctness
> +    enum_define = discriminator_find_enum_define(expr)
> +    if enum_define:
> +        for key in members:
> +            if not key in enum_define["enum_values"]:
> +                sys.stderr.write("Discriminator value '%s' is not found in "
> +                                 "enum '%s'\n" %
> +                                 (key, enum_define["enum_name"]))
> +                sys.exit(1)

Can this happen?  If yes, why isn't it diagnosed in qapi.py, like all
the other semantic errors?

> +        for key in enum_define["enum_values"]:
> +            if not key in members:
> +                sys.stderr.write("Enum value '%s' is not covered by a branch "
> +                                 "of union '%s'\n" %
> +                                 (key, name))
> +                sys.exit(1)
> +

Likewise.

>      ret = generate_visit_enum('%sKind' % name, members.keys())
>  
>      if base:
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index cf34768..0a3ab80 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -385,3 +385,34 @@ def guardend(name):
>  
>  ''',
>                   name=guardname(name))
> +
> +# This function can be used to check whether "base" is valid
> +def find_base_fields(base):
> +    base_struct_define = find_struct(base)
> +    if not base_struct_define:
> +        return None
> +    return base_struct_define.get('data')
> +
> +# Return the discriminator enum define, if discriminator is specified in
> +# @expr and it is a pre-defined enum type
> +def discriminator_find_enum_define(expr):
> +    discriminator = expr.get('discriminator')
> +    base = expr.get('base')
> +
> +    # Only support discriminator when base present
> +    if not (discriminator and base):
> +        return None
> +
> +    base_fields = find_base_fields(base)
> +
> +    if not base_fields:
> +        raise StandardError("Base '%s' is not a valid type\n"
> +                            % base)

Why not QAPISchemaError, like for other semantic errors?

> +
> +    discriminator_type = base_fields.get(discriminator)
> +
> +    if not discriminator_type:
> +        raise StandardError("Discriminator '%s' not found in schema\n"
> +                            % discriminator)

Likewise.

> +
> +    return find_enum(discriminator_type)

All errors should have a test in tests/qapi-schema/.  I can try to add
tests for you when I rebase your 09/10.

Re: [Qemu-devel] [PATCH V6 03/10] qapi script: check correctness of discriminator values in union

[Wenchao Xia]

于 2014/2/13 23:14, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 
>> It will check whether the values specified are written correctly,
>> and whether all enum values are covered, when discriminator is a
>> pre-defined enum type
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   scripts/qapi-visit.py |   17 +++++++++++++++++
>>   scripts/qapi.py       |   31 +++++++++++++++++++++++++++++++
>>   2 files changed, 48 insertions(+), 0 deletions(-)
>>
>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>> index 65f1a54..c0efb5f 100644
>> --- a/scripts/qapi-visit.py
>> +++ b/scripts/qapi-visit.py
>> @@ -255,6 +255,23 @@ def generate_visit_union(expr):
>>           assert not base
>>           return generate_visit_anon_union(name, members)
>>   
>> +    # If discriminator is specified and it is a pre-defined enum in schema,
>> +    # check its correctness
>> +    enum_define = discriminator_find_enum_define(expr)
>> +    if enum_define:
>> +        for key in members:
>> +            if not key in enum_define["enum_values"]:
>> +                sys.stderr.write("Discriminator value '%s' is not found in "
>> +                                 "enum '%s'\n" %
>> +                                 (key, enum_define["enum_name"]))
>> +                sys.exit(1)
> 
> Can this happen?  If yes, why isn't it diagnosed in qapi.py, like all
> the other semantic errors?
> 
  I think the parse procedure contains two part:
1 read qapi-schema.json and parse it into exprs.
2 translate exprs into final output.
  Looking at qapi.py, qapi-visit.py, qapi-types.py, it seems qapi.py is
in charge of step 1 handling literal error, and other two script are in
charge of step 2. The above error can be only detected in step 2 after
all enum defines are remembered in step 1, so I didn't add those things
into qapi.py.

  I guess you want to place the check inside parse_schema() to let
test case detect it easier, one way to go is, let qapi.py do checks
for step 2:

def parse_schema(fp):
    try:
        schema = QAPISchema(fp)
    except QAPISchemaError, e:
        print >>sys.stderr, e
        exit(1)

    exprs = []

    for expr in schema.exprs:
        if expr.has_key('enum'):
            add_enum(expr['enum'])
        elif expr.has_key('union'):
            add_union(expr)
            add_enum('%sKind' % expr['union'])
        elif expr.has_key('type'):
            add_struct(expr)
        exprs.append(expr)

+    for expr in schema.exprs:
+        if expr.has_key('union'):
+            #check code

    return exprs

  This way qapi.py can detect such errors. Disadvantage is that,
qapi.py is invloved for step 2 things, so some code in qapi.py
and qapi-visit.py may be dupicated, here the "if .... union...
discriminator" code may appear in both qapi.py and qapi-visit.py.

>> +        for key in enum_define["enum_values"]:
>> +            if not key in members:
>> +                sys.stderr.write("Enum value '%s' is not covered by a branch "
>> +                                 "of union '%s'\n" %
>> +                                 (key, name))
>> +                sys.exit(1)
>> +
> 
> Likewise.
> 
>>       ret = generate_visit_enum('%sKind' % name, members.keys())
>>   
>>       if base:
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index cf34768..0a3ab80 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -385,3 +385,34 @@ def guardend(name):
>>   
>>   ''',
>>                    name=guardname(name))
>> +

  The funtions below are likely helper funtions, I planed to put them
into qapi_helper.py, but they are not much so kepted for easy.

>> +# This function can be used to check whether "base" is valid
>> +def find_base_fields(base):
>> +    base_struct_define = find_struct(base)
>> +    if not base_struct_define:
>> +        return None
>> +    return base_struct_define.get('data')
>> +
>> +# Return the discriminator enum define, if discriminator is specified in
>> +# @expr and it is a pre-defined enum type
>> +def discriminator_find_enum_define(expr):
>> +    discriminator = expr.get('discriminator')
>> +    base = expr.get('base')
>> +
>> +    # Only support discriminator when base present
>> +    if not (discriminator and base):
>> +        return None
>> +
>> +    base_fields = find_base_fields(base)
>> +
>> +    if not base_fields:
>> +        raise StandardError("Base '%s' is not a valid type\n"
>> +                            % base)
> 
> Why not QAPISchemaError, like for other semantic errors?
> 

  I think QAPISchemaError is a literal error of step 1, here
it can't be used unless we record the text/line number belong to
each expr.

>> +
>> +    discriminator_type = base_fields.get(discriminator)
>> +
>> +    if not discriminator_type:
>> +        raise StandardError("Discriminator '%s' not found in schema\n"
>> +                            % discriminator)
> 
> Likewise.
> 
>> +
>> +    return find_enum(discriminator_type)
> 
> All errors should have a test in tests/qapi-schema/.  I can try to add
> tests for you when I rebase your 09/10.
> 

Re: [Qemu-devel] [PATCH V6 03/10] qapi script: check correctness of discriminator values in union

[Markus Armbruster]

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

> 于 2014/2/13 23:14, Markus Armbruster 写道:
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>> 
>>> It will check whether the values specified are written correctly,
>>> and whether all enum values are covered, when discriminator is a
>>> pre-defined enum type
>>>
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>   scripts/qapi-visit.py |   17 +++++++++++++++++
>>>   scripts/qapi.py       |   31 +++++++++++++++++++++++++++++++
>>>   2 files changed, 48 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>>> index 65f1a54..c0efb5f 100644
>>> --- a/scripts/qapi-visit.py
>>> +++ b/scripts/qapi-visit.py
>>> @@ -255,6 +255,23 @@ def generate_visit_union(expr):
>>>           assert not base
>>>           return generate_visit_anon_union(name, members)
>>>   
>>> +    # If discriminator is specified and it is a pre-defined enum in schema,
>>> +    # check its correctness
>>> +    enum_define = discriminator_find_enum_define(expr)
>>> +    if enum_define:
>>> +        for key in members:
>>> +            if not key in enum_define["enum_values"]:
>>> +                sys.stderr.write("Discriminator value '%s' is not found in "
>>> +                                 "enum '%s'\n" %
>>> +                                 (key, enum_define["enum_name"]))
>>> +                sys.exit(1)
>> 
>> Can this happen?  If yes, why isn't it diagnosed in qapi.py, like all
>> the other semantic errors?
>> 
>   I think the parse procedure contains two part:
> 1 read qapi-schema.json and parse it into exprs.
> 2 translate exprs into final output.
>   Looking at qapi.py, qapi-visit.py, qapi-types.py, it seems qapi.py is
> in charge of step 1 handling literal error, and other two script are in
> charge of step 2. The above error can be only detected in step 2 after
> all enum defines are remembered in step 1, so I didn't add those things
> into qapi.py.

The distribution of work between the qapi*py isn't spelled out anywhere,
but my working hypothesis is qapi.py is the frontend, and the
qapi-{commands,types,visit}.py are backends.

The frontend's job is lexical, syntax and semantic analysis.

The backends' job is source code generation.

This isn't the only possible split, but it's the orthodox way to split
compilers.

>   I guess you want to place the check inside parse_schema() to let
> test case detect it easier, one way to go is, let qapi.py do checks
> for step 2:
>
> def parse_schema(fp):
>     try:
>         schema = QAPISchema(fp)
>     except QAPISchemaError, e:
>         print >>sys.stderr, e
>         exit(1)
>
>     exprs = []
>
>     for expr in schema.exprs:
>         if expr.has_key('enum'):
>             add_enum(expr['enum'])
>         elif expr.has_key('union'):
>             add_union(expr)
>             add_enum('%sKind' % expr['union'])
>         elif expr.has_key('type'):
>             add_struct(expr)
>         exprs.append(expr)
>
> +    for expr in schema.exprs:
> +        if expr.has_key('union'):
> +            #check code
>
>     return exprs
>
>   This way qapi.py can detect such errors. Disadvantage is that,
> qapi.py is invloved for step 2 things, so some code in qapi.py
> and qapi-visit.py may be dupicated, here the "if .... union...
> discriminator" code may appear in both qapi.py and qapi-visit.py.

How much code would be duplicated?

>>> +        for key in enum_define["enum_values"]:
>>> +            if not key in members:
>>> + sys.stderr.write("Enum value '%s' is not covered by a branch "
>>> +                                 "of union '%s'\n" %
>>> +                                 (key, name))
>>> +                sys.exit(1)
>>> +
>> 
>> Likewise.
>> 
>>>       ret = generate_visit_enum('%sKind' % name, members.keys())
>>>   
>>>       if base:
>>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>>> index cf34768..0a3ab80 100644
>>> --- a/scripts/qapi.py
>>> +++ b/scripts/qapi.py
>>> @@ -385,3 +385,34 @@ def guardend(name):
>>>   
>>>   ''',
>>>                    name=guardname(name))
>>> +
>
>   The funtions below are likely helper funtions, I planed to put them
> into qapi_helper.py, but they are not much so kepted for easy.

That's fine with me.

>>> +# This function can be used to check whether "base" is valid
>>> +def find_base_fields(base):
>>> +    base_struct_define = find_struct(base)
>>> +    if not base_struct_define:
>>> +        return None
>>> +    return base_struct_define.get('data')
>>> +
>>> +# Return the discriminator enum define, if discriminator is specified in
>>> +# @expr and it is a pre-defined enum type
>>> +def discriminator_find_enum_define(expr):
>>> +    discriminator = expr.get('discriminator')
>>> +    base = expr.get('base')
>>> +
>>> +    # Only support discriminator when base present
>>> +    if not (discriminator and base):
>>> +        return None
>>> +
>>> +    base_fields = find_base_fields(base)
>>> +
>>> +    if not base_fields:
>>> +        raise StandardError("Base '%s' is not a valid type\n"
>>> +                            % base)
>> 
>> Why not QAPISchemaError, like for other semantic errors?
>> 
>
>   I think QAPISchemaError is a literal error of step 1, here
> it can't be used unless we record the text/line number belong to
> each expr.

Reporting an error without a location is not nice!

If decent error messages require recording locations, then we should
record locations.

A real compiler frontend records full location information, i.e. every
node in the abstract syntax tree (or whatever else it produces) is
decorated with a location.

Unfortunately, this wasn't done in qapi.py, so we get to retrofit it
now.

Perhaps recording only locations of top-level expressions would suffice
to improve your error messages to acceptable levels.  I'm not saying we
should take this shortcut, just pointing out it exists.

qapi.py represents locations as character offset in the contents of the
schema file (QAPISchema.cursor), which it converts to line, column on
demand, in QAPISchemaError.__init__.  If we keep things working that
way, the location data to record is the offset, not line, column.

>>> +
>>> +    discriminator_type = base_fields.get(discriminator)
>>> +
>>> +    if not discriminator_type:
>>> +        raise StandardError("Discriminator '%s' not found in schema\n"
>>> +                            % discriminator)
>> 
>> Likewise.
>> 
>>> +
>>> +    return find_enum(discriminator_type)
>> 
>> All errors should have a test in tests/qapi-schema/.  I can try to add
>> tests for you when I rebase your 09/10.
>> 

Re: [Qemu-devel] [PATCH V6 03/10] qapi script: check correctness of discriminator values in union

[Wenchao Xia]

于 2014/2/14 17:23, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>
>> 于 2014/2/13 23:14, Markus Armbruster 写道:
>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>>>
>>>> It will check whether the values specified are written correctly,
>>>> and whether all enum values are covered, when discriminator is a
>>>> pre-defined enum type
>>>>
>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>> ---
>>>>    scripts/qapi-visit.py |   17 +++++++++++++++++
>>>>    scripts/qapi.py       |   31 +++++++++++++++++++++++++++++++
>>>>    2 files changed, 48 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>>>> index 65f1a54..c0efb5f 100644
>>>> --- a/scripts/qapi-visit.py
>>>> +++ b/scripts/qapi-visit.py
>>>> @@ -255,6 +255,23 @@ def generate_visit_union(expr):
>>>>            assert not base
>>>>            return generate_visit_anon_union(name, members)
>>>>
>>>> +    # If discriminator is specified and it is a pre-defined enum in schema,
>>>> +    # check its correctness
>>>> +    enum_define = discriminator_find_enum_define(expr)
>>>> +    if enum_define:
>>>> +        for key in members:
>>>> +            if not key in enum_define["enum_values"]:
>>>> +                sys.stderr.write("Discriminator value '%s' is not found in "
>>>> +                                 "enum '%s'\n" %
>>>> +                                 (key, enum_define["enum_name"]))
>>>> +                sys.exit(1)
>>>
>>> Can this happen?  If yes, why isn't it diagnosed in qapi.py, like all
>>> the other semantic errors?
>>>
>>    I think the parse procedure contains two part:
>> 1 read qapi-schema.json and parse it into exprs.
>> 2 translate exprs into final output.
>>    Looking at qapi.py, qapi-visit.py, qapi-types.py, it seems qapi.py is
>> in charge of step 1 handling literal error, and other two script are in
>> charge of step 2. The above error can be only detected in step 2 after
>> all enum defines are remembered in step 1, so I didn't add those things
>> into qapi.py.
>
> The distribution of work between the qapi*py isn't spelled out anywhere,
> but my working hypothesis is qapi.py is the frontend, and the
> qapi-{commands,types,visit}.py are backends.
>
> The frontend's job is lexical, syntax and semantic analysis.
>
> The backends' job is source code generation.
>
> This isn't the only possible split, but it's the orthodox way to split
> compilers.
>
>>    I guess you want to place the check inside parse_schema() to let
>> test case detect it easier, one way to go is, let qapi.py do checks
>> for step 2:
>>
>> def parse_schema(fp):
>>      try:
>>          schema = QAPISchema(fp)
>>      except QAPISchemaError, e:
>>          print >>sys.stderr, e
>>          exit(1)
>>
>>      exprs = []
>>
>>      for expr in schema.exprs:
>>          if expr.has_key('enum'):
>>              add_enum(expr['enum'])
>>          elif expr.has_key('union'):
>>              add_union(expr)
>>              add_enum('%sKind' % expr['union'])
>>          elif expr.has_key('type'):
>>              add_struct(expr)
>>          exprs.append(expr)
>>
>> +    for expr in schema.exprs:
>> +        if expr.has_key('union'):
>> +            #check code
>>
>>      return exprs
>>
>>    This way qapi.py can detect such errors. Disadvantage is that,
>> qapi.py is invloved for step 2 things, so some code in qapi.py
>> and qapi-visit.py may be dupicated, here the "if .... union...
>> discriminator" code may appear in both qapi.py and qapi-visit.py.
>
> How much code would be duplicated?
>
   Not many now, my concern is it may becomes more complex
when more check introduced in future.
   However, your distribution of qapi*.py as complier make
sense, so I am OK to respin this series.
   Luiz, could you apply or push Markus's series, so I
can pull it as my working base?


>>>> +        for key in enum_define["enum_values"]:
>>>> +            if not key in members:
>>>> + sys.stderr.write("Enum value '%s' is not covered by a branch "
>>>> +                                 "of union '%s'\n" %
>>>> +                                 (key, name))
>>>> +                sys.exit(1)
>>>> +
>>>
>>> Likewise.
>>>
>>>>        ret = generate_visit_enum('%sKind' % name, members.keys())
>>>>
>>>>        if base:
>>>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>>>> index cf34768..0a3ab80 100644
>>>> --- a/scripts/qapi.py
>>>> +++ b/scripts/qapi.py
>>>> @@ -385,3 +385,34 @@ def guardend(name):
>>>>
>>>>    ''',
>>>>                     name=guardname(name))
>>>> +
>>
>>    The funtions below are likely helper funtions, I planed to put them
>> into qapi_helper.py, but they are not much so kepted for easy.
>
> That's fine with me.
>
>>>> +# This function can be used to check whether "base" is valid
>>>> +def find_base_fields(base):
>>>> +    base_struct_define = find_struct(base)
>>>> +    if not base_struct_define:
>>>> +        return None
>>>> +    return base_struct_define.get('data')
>>>> +
>>>> +# Return the discriminator enum define, if discriminator is specified in
>>>> +# @expr and it is a pre-defined enum type
>>>> +def discriminator_find_enum_define(expr):
>>>> +    discriminator = expr.get('discriminator')
>>>> +    base = expr.get('base')
>>>> +
>>>> +    # Only support discriminator when base present
>>>> +    if not (discriminator and base):
>>>> +        return None
>>>> +
>>>> +    base_fields = find_base_fields(base)
>>>> +
>>>> +    if not base_fields:
>>>> +        raise StandardError("Base '%s' is not a valid type\n"
>>>> +                            % base)
>>>
>>> Why not QAPISchemaError, like for other semantic errors?
>>>
>>
>>    I think QAPISchemaError is a literal error of step 1, here
>> it can't be used unless we record the text/line number belong to
>> each expr.
>
> Reporting an error without a location is not nice!
>
> If decent error messages require recording locations, then we should
> record locations.
>
> A real compiler frontend records full location information, i.e. every
> node in the abstract syntax tree (or whatever else it produces) is
> decorated with a location.
>
> Unfortunately, this wasn't done in qapi.py, so we get to retrofit it
> now.
>
> Perhaps recording only locations of top-level expressions would suffice
> to improve your error messages to acceptable levels.  I'm not saying we
> should take this shortcut, just pointing out it exists.
>
> qapi.py represents locations as character offset in the contents of the
> schema file (QAPISchema.cursor), which it converts to line, column on
> demand, in QAPISchemaError.__init__.  If we keep things working that
> way, the location data to record is the offset, not line, column.
>
>>>> +
>>>> +    discriminator_type = base_fields.get(discriminator)
>>>> +
>>>> +    if not discriminator_type:
>>>> +        raise StandardError("Discriminator '%s' not found in schema\n"
>>>> +                            % discriminator)
>>>
>>> Likewise.
>>>
>>>> +
>>>> +    return find_enum(discriminator_type)
>>>
>>> All errors should have a test in tests/qapi-schema/.  I can try to add
>>> tests for you when I rebase your 09/10.
>>>
>

Re: [Qemu-devel] [PATCH V6 03/10] qapi script: check correctness of discriminator values in union

[Markus Armbruster]

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

> 于 2014/2/14 17:23, Markus Armbruster 写道:
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>>
>>> 于 2014/2/13 23:14, Markus Armbruster 写道:
>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>>>>
>>>>> It will check whether the values specified are written correctly,
>>>>> and whether all enum values are covered, when discriminator is a
>>>>> pre-defined enum type
>>>>>
>>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>> ---
>>>>>    scripts/qapi-visit.py |   17 +++++++++++++++++
>>>>>    scripts/qapi.py       |   31 +++++++++++++++++++++++++++++++
>>>>>    2 files changed, 48 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>>>>> index 65f1a54..c0efb5f 100644
>>>>> --- a/scripts/qapi-visit.py
>>>>> +++ b/scripts/qapi-visit.py
>>>>> @@ -255,6 +255,23 @@ def generate_visit_union(expr):
>>>>>            assert not base
>>>>>            return generate_visit_anon_union(name, members)
>>>>>
>>>>> +    # If discriminator is specified and it is a pre-defined enum in schema,
>>>>> +    # check its correctness
>>>>> +    enum_define = discriminator_find_enum_define(expr)
>>>>> +    if enum_define:
>>>>> +        for key in members:
>>>>> +            if not key in enum_define["enum_values"]:
>>>>> +                sys.stderr.write("Discriminator value '%s' is not found in "
>>>>> +                                 "enum '%s'\n" %
>>>>> +                                 (key, enum_define["enum_name"]))
>>>>> +                sys.exit(1)
>>>>
>>>> Can this happen?  If yes, why isn't it diagnosed in qapi.py, like all
>>>> the other semantic errors?
>>>>
>>>    I think the parse procedure contains two part:
>>> 1 read qapi-schema.json and parse it into exprs.
>>> 2 translate exprs into final output.
>>>    Looking at qapi.py, qapi-visit.py, qapi-types.py, it seems qapi.py is
>>> in charge of step 1 handling literal error, and other two script are in
>>> charge of step 2. The above error can be only detected in step 2 after
>>> all enum defines are remembered in step 1, so I didn't add those things
>>> into qapi.py.
>>
>> The distribution of work between the qapi*py isn't spelled out anywhere,
>> but my working hypothesis is qapi.py is the frontend, and the
>> qapi-{commands,types,visit}.py are backends.
>>
>> The frontend's job is lexical, syntax and semantic analysis.
>>
>> The backends' job is source code generation.
>>
>> This isn't the only possible split, but it's the orthodox way to split
>> compilers.
>>
>>>    I guess you want to place the check inside parse_schema() to let
>>> test case detect it easier, one way to go is, let qapi.py do checks
>>> for step 2:
>>>
>>> def parse_schema(fp):
>>>      try:
>>>          schema = QAPISchema(fp)
>>>      except QAPISchemaError, e:
>>>          print >>sys.stderr, e
>>>          exit(1)
>>>
>>>      exprs = []
>>>
>>>      for expr in schema.exprs:
>>>          if expr.has_key('enum'):
>>>              add_enum(expr['enum'])
>>>          elif expr.has_key('union'):
>>>              add_union(expr)
>>>              add_enum('%sKind' % expr['union'])
>>>          elif expr.has_key('type'):
>>>              add_struct(expr)
>>>          exprs.append(expr)
>>>
>>> +    for expr in schema.exprs:
>>> +        if expr.has_key('union'):
>>> +            #check code
>>>
>>>      return exprs
>>>
>>>    This way qapi.py can detect such errors. Disadvantage is that,
>>> qapi.py is invloved for step 2 things, so some code in qapi.py
>>> and qapi-visit.py may be dupicated, here the "if .... union...
>>> discriminator" code may appear in both qapi.py and qapi-visit.py.
>>
>> How much code would be duplicated?
>>
>   Not many now, my concern is it may becomes more complex
> when more check introduced in future.
>   However, your distribution of qapi*.py as complier make
> sense, so I am OK to respin this series.

I'll gladly review your respin.  If you need help rebasing, don't
hesitate to ask me.

>   Luiz, could you apply or push Markus's series, so I
> can pull it as my working base?

I pushed my series trivially rebased to current master to
git://repo.or.cz/qemu/armbru.git branch qapi-scripts.  It applies fine
to Luiz's qmp-unstable branch.

Re: [Qemu-devel] [PATCH V6 03/10] qapi script: check correctness of discriminator values in union

[Luiz Capitulino]

On Mon, 17 Feb 2014 09:50:10 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> 于 2014/2/14 17:23, Markus Armbruster 写道:
> > Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> >
> >> 于 2014/2/13 23:14, Markus Armbruster 写道:
> >>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> >>>
> >>>> It will check whether the values specified are written correctly,
> >>>> and whether all enum values are covered, when discriminator is a
> >>>> pre-defined enum type
> >>>>
> >>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>>> Reviewed-by: Eric Blake <eblake@redhat.com>
> >>>> ---
> >>>>    scripts/qapi-visit.py |   17 +++++++++++++++++
> >>>>    scripts/qapi.py       |   31 +++++++++++++++++++++++++++++++
> >>>>    2 files changed, 48 insertions(+), 0 deletions(-)
> >>>>
> >>>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> >>>> index 65f1a54..c0efb5f 100644
> >>>> --- a/scripts/qapi-visit.py
> >>>> +++ b/scripts/qapi-visit.py
> >>>> @@ -255,6 +255,23 @@ def generate_visit_union(expr):
> >>>>            assert not base
> >>>>            return generate_visit_anon_union(name, members)
> >>>>
> >>>> +    # If discriminator is specified and it is a pre-defined enum in schema,
> >>>> +    # check its correctness
> >>>> +    enum_define = discriminator_find_enum_define(expr)
> >>>> +    if enum_define:
> >>>> +        for key in members:
> >>>> +            if not key in enum_define["enum_values"]:
> >>>> +                sys.stderr.write("Discriminator value '%s' is not found in "
> >>>> +                                 "enum '%s'\n" %
> >>>> +                                 (key, enum_define["enum_name"]))
> >>>> +                sys.exit(1)
> >>>
> >>> Can this happen?  If yes, why isn't it diagnosed in qapi.py, like all
> >>> the other semantic errors?
> >>>
> >>    I think the parse procedure contains two part:
> >> 1 read qapi-schema.json and parse it into exprs.
> >> 2 translate exprs into final output.
> >>    Looking at qapi.py, qapi-visit.py, qapi-types.py, it seems qapi.py is
> >> in charge of step 1 handling literal error, and other two script are in
> >> charge of step 2. The above error can be only detected in step 2 after
> >> all enum defines are remembered in step 1, so I didn't add those things
> >> into qapi.py.
> >
> > The distribution of work between the qapi*py isn't spelled out anywhere,
> > but my working hypothesis is qapi.py is the frontend, and the
> > qapi-{commands,types,visit}.py are backends.
> >
> > The frontend's job is lexical, syntax and semantic analysis.
> >
> > The backends' job is source code generation.
> >
> > This isn't the only possible split, but it's the orthodox way to split
> > compilers.
> >
> >>    I guess you want to place the check inside parse_schema() to let
> >> test case detect it easier, one way to go is, let qapi.py do checks
> >> for step 2:
> >>
> >> def parse_schema(fp):
> >>      try:
> >>          schema = QAPISchema(fp)
> >>      except QAPISchemaError, e:
> >>          print >>sys.stderr, e
> >>          exit(1)
> >>
> >>      exprs = []
> >>
> >>      for expr in schema.exprs:
> >>          if expr.has_key('enum'):
> >>              add_enum(expr['enum'])
> >>          elif expr.has_key('union'):
> >>              add_union(expr)
> >>              add_enum('%sKind' % expr['union'])
> >>          elif expr.has_key('type'):
> >>              add_struct(expr)
> >>          exprs.append(expr)
> >>
> >> +    for expr in schema.exprs:
> >> +        if expr.has_key('union'):
> >> +            #check code
> >>
> >>      return exprs
> >>
> >>    This way qapi.py can detect such errors. Disadvantage is that,
> >> qapi.py is invloved for step 2 things, so some code in qapi.py
> >> and qapi-visit.py may be dupicated, here the "if .... union...
> >> discriminator" code may appear in both qapi.py and qapi-visit.py.
> >
> > How much code would be duplicated?
> >
>    Not many now, my concern is it may becomes more complex
> when more check introduced in future.
>    However, your distribution of qapi*.py as complier make
> sense, so I am OK to respin this series.
>    Luiz, could you apply or push Markus's series, so I
> can pull it as my working base?

Yes, but I have to handle current pull request right now. I'll let you
know when I apply it.

> 
> 
> >>>> +        for key in enum_define["enum_values"]:
> >>>> +            if not key in members:
> >>>> + sys.stderr.write("Enum value '%s' is not covered by a branch "
> >>>> +                                 "of union '%s'\n" %
> >>>> +                                 (key, name))
> >>>> +                sys.exit(1)
> >>>> +
> >>>
> >>> Likewise.
> >>>
> >>>>        ret = generate_visit_enum('%sKind' % name, members.keys())
> >>>>
> >>>>        if base:
> >>>> diff --git a/scripts/qapi.py b/scripts/qapi.py
> >>>> index cf34768..0a3ab80 100644
> >>>> --- a/scripts/qapi.py
> >>>> +++ b/scripts/qapi.py
> >>>> @@ -385,3 +385,34 @@ def guardend(name):
> >>>>
> >>>>    ''',
> >>>>                     name=guardname(name))
> >>>> +
> >>
> >>    The funtions below are likely helper funtions, I planed to put them
> >> into qapi_helper.py, but they are not much so kepted for easy.
> >
> > That's fine with me.
> >
> >>>> +# This function can be used to check whether "base" is valid
> >>>> +def find_base_fields(base):
> >>>> +    base_struct_define = find_struct(base)
> >>>> +    if not base_struct_define:
> >>>> +        return None
> >>>> +    return base_struct_define.get('data')
> >>>> +
> >>>> +# Return the discriminator enum define, if discriminator is specified in
> >>>> +# @expr and it is a pre-defined enum type
> >>>> +def discriminator_find_enum_define(expr):
> >>>> +    discriminator = expr.get('discriminator')
> >>>> +    base = expr.get('base')
> >>>> +
> >>>> +    # Only support discriminator when base present
> >>>> +    if not (discriminator and base):
> >>>> +        return None
> >>>> +
> >>>> +    base_fields = find_base_fields(base)
> >>>> +
> >>>> +    if not base_fields:
> >>>> +        raise StandardError("Base '%s' is not a valid type\n"
> >>>> +                            % base)
> >>>
> >>> Why not QAPISchemaError, like for other semantic errors?
> >>>
> >>
> >>    I think QAPISchemaError is a literal error of step 1, here
> >> it can't be used unless we record the text/line number belong to
> >> each expr.
> >
> > Reporting an error without a location is not nice!
> >
> > If decent error messages require recording locations, then we should
> > record locations.
> >
> > A real compiler frontend records full location information, i.e. every
> > node in the abstract syntax tree (or whatever else it produces) is
> > decorated with a location.
> >
> > Unfortunately, this wasn't done in qapi.py, so we get to retrofit it
> > now.
> >
> > Perhaps recording only locations of top-level expressions would suffice
> > to improve your error messages to acceptable levels.  I'm not saying we
> > should take this shortcut, just pointing out it exists.
> >
> > qapi.py represents locations as character offset in the contents of the
> > schema file (QAPISchema.cursor), which it converts to line, column on
> > demand, in QAPISchemaError.__init__.  If we keep things working that
> > way, the location data to record is the offset, not line, column.
> >
> >>>> +
> >>>> +    discriminator_type = base_fields.get(discriminator)
> >>>> +
> >>>> +    if not discriminator_type:
> >>>> +        raise StandardError("Discriminator '%s' not found in schema\n"
> >>>> +                            % discriminator)
> >>>
> >>> Likewise.
> >>>
> >>>> +
> >>>> +    return find_enum(discriminator_type)
> >>>
> >>> All errors should have a test in tests/qapi-schema/.  I can try to add
> >>> tests for you when I rebase your 09/10.
> >>>
> >
> 

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

[Wenchao Xia]

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>
---
 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 4a1652b..88bf76a 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 0a3ab80..ac126f4 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -416,3 +416,13 @@ def discriminator_find_enum_define(expr):
                             % discriminator)
 
     return find_enum(discriminator_type)
+
+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 V6 05/10] qapi script: use same function to generate enum string

[Wenchao Xia]

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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi-types.py |    6 +++---
 scripts/qapi-visit.py |   21 +++++++++++++++------
 scripts/qapi.py       |   15 +++++++++++----
 3 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 88bf76a..914f055 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_string = generate_enum_full_value_string(name, value)
         enum_decl += mcgen('''
-    %(abbrev)s_%(value)s = %(i)d,
+    %(enum_full_value_string)s = %(i)d,
 ''',
-                     abbrev=de_camel_case(name).upper(),
-                     value=generate_enum_name(value),
+                     enum_full_value_string=enum_full_value_string,
                      i=i)
         i += 1
 
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index c0efb5f..1b55924 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -214,18 +214,23 @@ 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)"
+    discriminator_type_name = '%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_string = \
+                  generate_enum_full_value_string(discriminator_type_name, key)
         ret += mcgen('''
-        case %(abbrev)s_KIND_%(enum)s:
+        case %(enum_full_value_string)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_string = enum_full_value_string,
                 c_type = type_name(members[key]),
                 c_name = c_fun(key))
 
@@ -272,7 +277,10 @@ def generate_visit_union(expr):
                                  (key, name))
                 sys.exit(1)
 
+    # 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())
+    discriminator_type_name = '%sKind' % (name)
 
     if base:
         base_fields = find_struct(base)['data']
@@ -330,13 +338,14 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
                     visit_end_implicit_struct(m, &err);
                 }'''
 
+        enum_full_value_string = \
+                  generate_enum_full_value_string(discriminator_type_name, key)
         ret += mcgen('''
-            case %(abbrev)s_KIND_%(enum)s:
+            case %(enum_full_value_string)s:
                 ''' + fmt + '''
                 break;
 ''',
-                abbrev = de_camel_case(name).upper(),
-                enum = c_fun(de_camel_case(key),False).upper(),
+                enum_full_value_string = enum_full_value_string,
                 c_type=type_name(members[key]),
                 c_name=c_fun(key))
 
diff --git a/scripts/qapi.py b/scripts/qapi.py
index ac126f4..8ada42a 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -417,12 +417,19 @@ def discriminator_find_enum_define(expr):
 
     return find_enum(discriminator_type)
 
-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_string(enum_name, enum_value):
+    # generate abbrev string
+    abbrev_string = de_camel_case(enum_name).upper()
+    # generate value string
+    value_string = _generate_enum_value_string(enum_value)
+    return "%s_%s" % (abbrev_string, value_string)
-- 
1.7.1

[Qemu-devel] [PATCH V6 06/10] qapi script: support pre-defined enum type as discriminator in union

[Wenchao Xia]

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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 docs/qapi-code-gen.txt |    8 ++++++--
 scripts/qapi-types.py  |   18 ++++++++++++++----
 scripts/qapi-visit.py  |   23 ++++++++++++++++-------
 scripts/qapi.py        |    4 +++-
 4 files changed, 39 insertions(+), 14 deletions(-)

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 914f055..5031c4b 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 1b55924..3240921 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -277,10 +277,14 @@ def generate_visit_union(expr):
                                  (key, name))
                 sys.exit(1)
 
-    # 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())
-    discriminator_type_name = '%sKind' % (name)
+        # Use the predefined enum type as discriminator
+        ret = ""
+        discriminator_type_name = 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())
+        discriminator_type_name = '%sKind' % (name)
 
     if base:
         base_fields = find_struct(base)['data']
@@ -320,11 +324,12 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
     else:
         desc_type = discriminator
     ret += mcgen('''
-        visit_type_%(name)sKind(m, &(*obj)->kind, "%(type)s", &err);
+        visit_type_%(discriminator_type_name)s(m, &(*obj)->kind, "%(type)s", &err);
         if (!err) {
             switch ((*obj)->kind) {
 ''',
-                 name=name, type=desc_type)
+                 discriminator_type_name=discriminator_type_name,
+                 type=desc_type)
 
     for key in members:
         if not discriminator:
@@ -538,7 +543,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 8ada42a..62a97c1 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -174,7 +174,9 @@ def parse_schema(fp):
             add_enum(expr['enum'], expr['data'])
         elif expr.has_key('union'):
             add_union(expr)
-            add_enum('%sKind' % expr['union'])
+            enum_define = discriminator_find_enum_define(expr)
+            if not enum_define:
+                add_enum('%sKind' % expr['union'])
         elif expr.has_key('type'):
             add_struct(expr)
         exprs.append(expr)
-- 
1.7.1

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

[Wenchao Xia]

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>
---
 qapi-schema.json |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 05ced9d..3e9b554 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4120,6 +4120,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' ] }
+
+##
 # @BlockdevOptionsBase
 #
 # Options that are available for all block devices, independent of the block
@@ -4143,7 +4155,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 V6 08/10] qapi script: do not allow string discriminator

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 docs/qapi-code-gen.txt |    8 +++-----
 scripts/qapi-visit.py  |    6 ++++++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index a2e7921..c92add9 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -123,11 +123,9 @@ 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.
-The above example can then be modified as follows:
+then no longer generated). The discriminator should be a predefined enum field,
+and a compile time check will be done to verify the correctness. The above
+example can then be modified as follows:
 
  { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] }
  { 'type': 'BlockdevCommonOptions',
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 3240921..efa7ec3 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -280,6 +280,12 @@ def generate_visit_union(expr):
         # Use the predefined enum type as discriminator
         ret = ""
         discriminator_type_name = enum_define['enum_name']
+    elif discriminator:
+        # Do not allow string discriminator
+        sys.stderr.write("Discriminator '%s' is not a pre-defined enum "
+                         "type\n" %
+                         discriminator)
+        sys.exit(1)
     else:
         # There will always be a discriminator in the C switch code, by default it
         # is an enum type generated silently as "'%sKind' % (name)"
-- 
1.7.1

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

[Markus Armbruster]

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

> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  docs/qapi-code-gen.txt |    8 +++-----
>  scripts/qapi-visit.py  |    6 ++++++
>  2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index a2e7921..c92add9 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -123,11 +123,9 @@ 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.
> -The above example can then be modified as follows:
> +then no longer generated). The discriminator should be a predefined enum field,
> +and a compile time check will be done to verify the correctness. The above
> +example can then be modified as follows:
>  
>   { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] }
>   { 'type': 'BlockdevCommonOptions',
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 3240921..efa7ec3 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -280,6 +280,12 @@ def generate_visit_union(expr):
>          # Use the predefined enum type as discriminator
>          ret = ""
>          discriminator_type_name = enum_define['enum_name']
> +    elif discriminator:
> +        # Do not allow string discriminator
> +        sys.stderr.write("Discriminator '%s' is not a pre-defined enum "
> +                         "type\n" %
> +                         discriminator)
> +        sys.exit(1)
>      else:
>          # There will always be a discriminator in the C switch code, by default it
>          # is an enum type generated silently as "'%sKind' % (name)"

Why isn't it diagnosed in qapi.py, like all the other semantic errors?

All errors should have a test in tests/qapi-schema/.  I can try to add
tests for you when I rebase your 09/10.

[Qemu-devel] [PATCH V6 09/10] tests: add cases for inherited struct and union with discriminator

[Wenchao Xia]

Test for inherit and complex union.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 tests/qapi-schema/qapi-schema-test.json |   22 ++++++
 tests/qapi-schema/qapi-schema-test.out  |    9 +++
 tests/test-qmp-input-visitor.c          |   93 +++++++++++++++++++++++++
 tests/test-qmp-output-visitor.c         |  116 +++++++++++++++++++++++++++++++
 4 files changed, 240 insertions(+), 0 deletions(-)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index fe5af75..0bc58ac 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -21,8 +21,18 @@
             'dict1': { 'string1': 'str',
                        'dict2': { 'userdef1': 'UserDefOne', 'string2': 'str' },
                        '*dict3': { 'userdef2': 'UserDefOne', 'string3': 'str' } } } }
+# for testing base
+{ 'type': 'UserDefBase',
+  'data': { 'string': 'str', 'enum1': 'EnumOne' } }
+
+{ 'type': 'UserDefInherit',
+  'base': 'UserDefBase',
+  'data': { 'boolean': 'bool', 'integer': 'int' } }
 
 # for testing unions
+{ 'type': 'UserDefBase0',
+  'data': { 'base-string0': 'str', 'base-enum0': 'EnumOne' } }
+
 { 'type': 'UserDefA',
   'data': { 'boolean': 'bool' } }
 
@@ -32,6 +42,18 @@
 { 'union': 'UserDefUnion',
   'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
 
+{ 'union': 'UserDefBaseUnion',
+  'base': 'UserDefBase0',
+  'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
+
+# A complex type
+{ 'union': 'UserDefEnumDiscriminatorUnion',
+  'base': 'UserDefBase0',
+  'discriminator' : 'base-enum0',
+  'data': { 'value1' : 'UserDefA',
+            'value2' : 'UserDefInherit',
+            'value3' : 'UserDefB' } }
+
 # for testing native lists
 { 'union': 'UserDefNativeListUnion',
   'data': { 'integer': ['int'],
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index ad74cdb..80edf10 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -3,9 +3,14 @@
  OrderedDict([('type', 'UserDefOne'), ('data', OrderedDict([('integer', 'int'), ('string', 'str'), ('*enum1', 'EnumOne')]))]),
  OrderedDict([('type', 'UserDefTwo'), ('data', OrderedDict([('string', 'str'), ('dict', OrderedDict([('string', 'str'), ('dict', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')])), ('*dict2', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')]))]))]))]),
  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', 'UserDefBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
+ OrderedDict([('type', 'UserDefInherit'), ('base', 'UserDefBase'), ('data', OrderedDict([('boolean', 'bool'), ('integer', 'int')]))]),
+ OrderedDict([('type', 'UserDefBase0'), ('data', OrderedDict([('base-string0', 'str'), ('base-enum0', 'EnumOne')]))]),
  OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('union', 'UserDefUnion'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
+ OrderedDict([('union', 'UserDefBaseUnion'), ('base', 'UserDefBase0'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
+ OrderedDict([('union', 'UserDefEnumDiscriminatorUnion'), ('base', 'UserDefBase0'), ('discriminator', 'base-enum0'), ('data', OrderedDict([('value1', 'UserDefA'), ('value2', 'UserDefInherit'), ('value3', 'UserDefB')]))]),
  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())]),
  OrderedDict([('command', 'user_def_cmd1'), ('data', OrderedDict([('ud1a', 'UserDefOne')]))]),
@@ -13,11 +18,15 @@
  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': 'UserDefBaseUnionKind', 'enum_values': None},
  {'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None}]
 [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
  OrderedDict([('type', 'UserDefOne'), ('data', OrderedDict([('integer', 'int'), ('string', 'str'), ('*enum1', 'EnumOne')]))]),
  OrderedDict([('type', 'UserDefTwo'), ('data', OrderedDict([('string', 'str'), ('dict', OrderedDict([('string', 'str'), ('dict', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')])), ('*dict2', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')]))]))]))]),
  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', 'UserDefBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]),
+ OrderedDict([('type', 'UserDefInherit'), ('base', 'UserDefBase'), ('data', OrderedDict([('boolean', 'bool'), ('integer', 'int')]))]),
+ OrderedDict([('type', 'UserDefBase0'), ('data', OrderedDict([('base-string0', 'str'), ('base-enum0', 'EnumOne')]))]),
  OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64', ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'), ('*u64x', 'uint64')]))])]
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 1e1c6fa..1d45e5e 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -286,6 +286,31 @@ static void test_visitor_in_list(TestInputVisitorData *data,
     qapi_free_UserDefOneList(head);
 }
 
+static void test_visitor_in_inherit(TestInputVisitorData *data,
+                                    const void *unused)
+{
+    Visitor *v;
+    Error *err = NULL;
+    UserDefInherit *tmp;
+    const char *input =
+    "{ \
+        'integer': 2, \
+        'boolean': false, \
+        'enum1': 'value2', \
+        'string': 'test' \
+    }";
+
+    v = visitor_input_test_init_raw(data, input);
+
+    visit_type_UserDefInherit(v, &tmp, NULL, &err);
+    g_assert(err == NULL);
+    g_assert_cmpint(tmp->integer, ==, 2);
+    g_assert_cmpint(tmp->boolean, ==, false);
+    g_assert_cmpint(tmp->base->enum1, ==, ENUM_ONE_VALUE2);
+    g_assert_cmpstr(tmp->base->string, ==, "test");
+    qapi_free_UserDefInherit(tmp);
+}
+
 static void test_visitor_in_union(TestInputVisitorData *data,
                                   const void *unused)
 {
@@ -302,6 +327,67 @@ static void test_visitor_in_union(TestInputVisitorData *data,
     qapi_free_UserDefUnion(tmp);
 }
 
+static void test_visitor_in_base_union(TestInputVisitorData *data,
+                                       const void *unused)
+{
+    Visitor *v;
+    Error *err = NULL;
+    UserDefBaseUnion *tmp;
+    const char *input =
+    "{ \
+        'base-enum0': 'value2', \
+        'base-string0': 'test', \
+        'type': 'b', \
+        'data': { \
+            'integer': 2 \
+        } \
+    }";
+
+    v = visitor_input_test_init_raw(data, input);
+
+    visit_type_UserDefBaseUnion(v, &tmp, NULL, &err);
+    g_assert(err == NULL);
+    g_assert_cmpint(tmp->base_enum0, ==, ENUM_ONE_VALUE2);
+    g_assert_cmpstr(tmp->base_string0, ==, "test");
+    g_assert_cmpint(tmp->kind, ==, USER_DEF_BASE_UNION_KIND_B);
+    g_assert_cmpint(tmp->b->integer, ==, 2);
+
+    qapi_free_UserDefBaseUnion(tmp);
+}
+
+static
+void test_visitor_in_enum_discriminator_union(TestInputVisitorData *data,
+                                              const void *unused)
+{
+    Visitor *v;
+    Error *err = NULL;
+    UserDefEnumDiscriminatorUnion *tmp;
+    const char *input =
+    "{ \
+        'boolean': false, \
+        'integer': 2, \
+        'enum1': 'value2', \
+        'string': 'test', \
+        'base-enum0': 'value2', \
+        'base-string0': 'test' \
+    }";
+
+    v = visitor_input_test_init_raw(data, input);
+
+    visit_type_UserDefEnumDiscriminatorUnion(v, &tmp, NULL, &err);
+    g_assert(err == NULL);
+
+    g_assert_cmpstr(tmp->base_string0, ==, "test");
+    g_assert_cmpint(tmp->kind, ==, ENUM_ONE_VALUE2);
+
+    g_assert_cmpint(tmp->value2->boolean, ==, false);
+    g_assert_cmpint(tmp->value2->integer, ==, 2);
+    g_assert_cmpint(tmp->value2->base->enum1, ==, ENUM_ONE_VALUE2);
+    g_assert_cmpstr(tmp->value2->base->string, ==, "test");
+
+    qapi_free_UserDefEnumDiscriminatorUnion(tmp);
+}
+
 static void test_native_list_integer_helper(TestInputVisitorData *data,
                                             const void *unused,
                                             UserDefNativeListUnionKind kind)
@@ -633,8 +719,15 @@ int main(int argc, char **argv)
                             &in_visitor_data, test_visitor_in_struct_nested);
     input_visitor_test_add("/visitor/input/list",
                             &in_visitor_data, test_visitor_in_list);
+    input_visitor_test_add("/visitor/input/inherit",
+                            &in_visitor_data, test_visitor_in_inherit);
     input_visitor_test_add("/visitor/input/union",
                             &in_visitor_data, test_visitor_in_union);
+    input_visitor_test_add("/visitor/input/base-union",
+                            &in_visitor_data, test_visitor_in_base_union);
+    input_visitor_test_add("/visitor/input/enum-discriminator-union",
+                            &in_visitor_data,
+                            test_visitor_in_enum_discriminator_union);
     input_visitor_test_add("/visitor/input/errors",
                             &in_visitor_data, test_visitor_in_errors);
     input_visitor_test_add("/visitor/input/native_list/int",
diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index e073d83..0e89482 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -402,6 +402,38 @@ static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data,
     qapi_free_UserDefNestedList(head);
 }
 
+static void test_visitor_out_inherit(TestOutputVisitorData *data,
+                                     const void *unused)
+{
+    QObject *arg;
+    QDict *qdict;
+    const char *test_str = "test";
+    Error *err = NULL;
+
+    UserDefInherit *tmp =
+            g_malloc0(sizeof(UserDefInherit));
+    tmp->integer = 2;
+    tmp->boolean = false;
+    tmp->base = g_malloc0(sizeof(UserDefBase));
+    tmp->base->enum1 = ENUM_ONE_VALUE2;
+    tmp->base->string = g_strdup(test_str);
+
+    visit_type_UserDefInherit(data->ov, &tmp, NULL, &err);
+    g_assert(err == NULL);
+    arg = qmp_output_get_qobject(data->qov);
+
+    g_assert(qobject_type(arg) == QTYPE_QDICT);
+    qdict = qobject_to_qdict(arg);
+
+    g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 2);
+    g_assert_cmpint(qdict_get_bool(qdict, "boolean"), ==, false);
+    g_assert_cmpstr(qdict_get_str(qdict, "enum1"), ==, "value2");
+    g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, test_str);
+
+    qapi_free_UserDefInherit(tmp);
+    QDECREF(qdict);
+}
+
 static void test_visitor_out_union(TestOutputVisitorData *data,
                                    const void *unused)
 {
@@ -434,6 +466,83 @@ static void test_visitor_out_union(TestOutputVisitorData *data,
     QDECREF(qdict);
 }
 
+static void test_visitor_out_base_union(TestOutputVisitorData *data,
+                                        const void *unused)
+{
+    QObject *arg;
+    QDict *qdict, *qdict_sub;
+    const char *test_str = "test";
+    Error *err = NULL;
+
+    UserDefBaseUnion *tmp =
+            g_malloc0(sizeof(UserDefBaseUnion));
+    tmp->base_enum0 = ENUM_ONE_VALUE2;
+    tmp->base_string0 = g_strdup(test_str);
+    tmp->kind = USER_DEF_BASE_UNION_KIND_B;
+    tmp->b = g_malloc0(sizeof(UserDefB));
+    tmp->b->integer = 2;
+
+    visit_type_UserDefBaseUnion(data->ov, &tmp, NULL, &err);
+    g_assert(err == NULL);
+    arg = qmp_output_get_qobject(data->qov);
+
+    g_assert(qobject_type(arg) == QTYPE_QDICT);
+    qdict = qobject_to_qdict(arg);
+
+    g_assert_cmpstr(qdict_get_str(qdict, "base-enum0"), ==, "value2");
+    g_assert_cmpstr(qdict_get_str(qdict, "base-string0"), ==, test_str);
+    g_assert_cmpstr(qdict_get_str(qdict, "type"), ==, "b");
+
+    qdict_sub = qobject_to_qdict(qdict_get(qdict, "data"));
+    g_assert(qdict_sub);
+
+    g_assert_cmpint(qdict_get_int(qdict_sub, "integer"), ==, 2);
+
+    qapi_free_UserDefBaseUnion(tmp);
+    QDECREF(qdict);
+}
+
+/* A complex type */
+static
+void test_visitor_out_enum_discriminator_union(TestOutputVisitorData *data,
+                                               const void *unused)
+{
+    QObject *arg;
+    QDict *qdict;
+    const char *test_str = "test";
+    Error *err = NULL;
+
+    UserDefEnumDiscriminatorUnion *tmp =
+            g_malloc0(sizeof(UserDefEnumDiscriminatorUnion));
+    tmp->base_string0 = g_strdup(test_str);
+    tmp->kind = ENUM_ONE_VALUE2;
+    tmp->value2 = g_malloc0(sizeof(UserDefInherit));
+    tmp->value2->integer = 2;
+    tmp->value2->boolean = false;
+    tmp->value2->base = g_malloc0(sizeof(UserDefBase));
+    tmp->value2->base->enum1 = ENUM_ONE_VALUE2;
+    tmp->value2->base->string = g_strdup(test_str);
+
+    visit_type_UserDefEnumDiscriminatorUnion(data->ov, &tmp, NULL, &err);
+    g_assert(err == NULL);
+    arg = qmp_output_get_qobject(data->qov);
+
+    g_assert(qobject_type(arg) == QTYPE_QDICT);
+    qdict = qobject_to_qdict(arg);
+
+    g_assert_cmpstr(qdict_get_str(qdict, "base-enum0"), ==, "value2");
+    g_assert_cmpstr(qdict_get_str(qdict, "base-string0"), ==, test_str);
+    /* check sub type specfic */
+    g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 2);
+    g_assert_cmpint(qdict_get_bool(qdict, "boolean"), ==, false);
+    g_assert_cmpstr(qdict_get_str(qdict, "enum1"), ==, "value2");
+    g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, test_str);
+
+    qapi_free_UserDefEnumDiscriminatorUnion(tmp);
+    QDECREF(qdict);
+
+}
+
 static void init_native_list(UserDefNativeListUnion *cvalue)
 {
     int i;
@@ -780,8 +889,15 @@ int main(int argc, char **argv)
                             &out_visitor_data, test_visitor_out_list);
     output_visitor_test_add("/visitor/output/list-qapi-free",
                             &out_visitor_data, test_visitor_out_list_qapi_free);
+    output_visitor_test_add("/visitor/output/inherit",
+                            &out_visitor_data, test_visitor_out_inherit);
     output_visitor_test_add("/visitor/output/union",
                             &out_visitor_data, test_visitor_out_union);
+    output_visitor_test_add("/visitor/output/base-union",
+                            &out_visitor_data, test_visitor_out_base_union);
+    output_visitor_test_add("/visitor/output/enum-discriminator-union",
+                            &out_visitor_data,
+                            test_visitor_out_enum_discriminator_union);
     output_visitor_test_add("/visitor/output/native_list/int",
                             &out_visitor_data, test_visitor_out_native_list_int);
     output_visitor_test_add("/visitor/output/native_list/int8",
-- 
1.7.1

Re: [Qemu-devel] [PATCH V6 09/10] tests: add cases for inherited struct and union with discriminator

[Markus Armbruster]

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

> Test for inherit and complex union.

This patch conflicts badly with my test coverage work in "[PATCH v2
00/13] qapi: Test coverage & clean up generated code".  My series
systematically covers code generation in scripts/qapi*py, and that takes
me seven patches.

If I put your patch first, mine all explode, and I get to start over.

Putting my series first looks far easier to resolve, because only this
one patch conflicts.  Would you mind me rebasing your series on top of
mine?

Re: [Qemu-devel] [PATCH V6 09/10] tests: add cases for inherited struct and union with discriminator

[Luiz Capitulino]

On Thu, 13 Feb 2014 15:53:30 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 
> > Test for inherit and complex union.
> 
> This patch conflicts badly with my test coverage work in "[PATCH v2
> 00/13] qapi: Test coverage & clean up generated code".  My series
> systematically covers code generation in scripts/qapi*py, and that takes
> me seven patches.
> 
> If I put your patch first, mine all explode, and I get to start over.
> 
> Putting my series first looks far easier to resolve, because only this
> one patch conflicts.  Would you mind me rebasing your series on top of
> mine?

If it's only this patch that causes conflicts, what about the
following:

 1. I apply patches 1-8 plus patch 10
 2. Markus rebases patch 9 and adds it to his series
 3. Markus post a new version

Does this work for everyone?

The only drawback is that both series will miss today's boat, but this
would happen at this point anyway.

Re: [Qemu-devel] [PATCH V6 09/10] tests: add cases for inherited struct and union with discriminator

[Wenchao Xia]

于 2014/2/13 23:11, Luiz Capitulino 写道:
> On Thu, 13 Feb 2014 15:53:30 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>>
>>> Test for inherit and complex union.
>>
>> This patch conflicts badly with my test coverage work in "[PATCH v2
>> 00/13] qapi: Test coverage & clean up generated code".  My series
>> systematically covers code generation in scripts/qapi*py, and that takes
>> me seven patches.
>>
>> If I put your patch first, mine all explode, and I get to start over.
>>
>> Putting my series first looks far easier to resolve, because only this
>> one patch conflicts.  Would you mind me rebasing your series on top of
>> mine?
>
> If it's only this patch that causes conflicts, what about the
> following:
>
>   1. I apply patches 1-8 plus patch 10
>   2. Markus rebases patch 9 and adds it to his series
>   3. Markus post a new version
>
> Does this work for everyone?
>
> The only drawback is that both series will miss today's boat, but this
> would happen at this point anyway.
>
   I am fine with it.

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

[Wenchao Xia]

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>
---
 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 62a97c1..4bea3a4 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -419,19 +419,31 @@ def discriminator_find_enum_define(expr):
 
     return find_enum(discriminator_type)
 
-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_string(enum_name, enum_value):
     # generate abbrev string
-    abbrev_string = de_camel_case(enum_name).upper()
+    abbrev_string = _generate_enum_string(enum_name)
     # generate value string
-    value_string = _generate_enum_value_string(enum_value)
+    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 V6 00/10] qapi script: support enum as discriminator and better enum name

[Eric Blake]

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

On 02/10/2014 02:48 PM, Wenchao Xia wrote:
> 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' } }

See also:
https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg01663.html

There, I argued that even unions without a 'base':'TypeName' would
benefit from having a discriminator that calls out an enum type listing
the valid branches of the union.  I think that could be a followup to
this series.

-- 
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 V6 00/10] qapi script: support enum as discriminator and better enum name

[Luiz Capitulino]

On Tue, 11 Feb 2014 05:48:31 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

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

Applied to the qmp branch, thanks.

> 
> 
> 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.
> 
> Wenchao Xia (10):
>   1 qapi script: remember enum values
>   2 qapi script: add check for duplicated key
>   3 qapi script: check correctness of discriminator values in union
>   4 qapi script: code move for generate_enum_name()
>   5 qapi script: use same function to generate enum string
>   6 qapi script: support pre-defined enum type as discriminator in union
>   7 qapi: convert BlockdevOptions to use enum discriminator
>   8 qapi script: do not allow string discriminator
>   9 tests: add cases for inherited struct and union with discriminator
>   10 qapi script: do not add "_" for every capitalized char in enum
> 
>  docs/qapi-code-gen.txt                  |    8 ++-
>  include/qapi/qmp/qerror.h               |    2 +-
>  qapi-schema.json                        |   14 ++++-
>  scripts/qapi-types.py                   |   34 +++++-----
>  scripts/qapi-visit.py                   |   61 ++++++++++++++---
>  scripts/qapi.py                         |   84 +++++++++++++++++++++--
>  target-i386/cpu.c                       |    2 +-
>  tests/qapi-schema/comments.out          |    2 +-
>  tests/qapi-schema/qapi-schema-test.json |   22 ++++++
>  tests/qapi-schema/qapi-schema-test.out  |   13 +++-
>  tests/test-qmp-input-visitor.c          |   93 +++++++++++++++++++++++++
>  tests/test-qmp-output-visitor.c         |  116 +++++++++++++++++++++++++++++++
>  12 files changed, 411 insertions(+), 40 deletions(-)
> 

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

[Luiz Capitulino]

On Tue, 11 Feb 2014 14:13:19 -0500
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Tue, 11 Feb 2014 05:48:31 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> 
> > 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)
> 
> Applied to the qmp branch, thanks.

Just to make it clear, I'm not including this series in today's pull
request so that you and Markus can coordinate your work.

> 
> > 
> > 
> > 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.
> > 
> > Wenchao Xia (10):
> >   1 qapi script: remember enum values
> >   2 qapi script: add check for duplicated key
> >   3 qapi script: check correctness of discriminator values in union
> >   4 qapi script: code move for generate_enum_name()
> >   5 qapi script: use same function to generate enum string
> >   6 qapi script: support pre-defined enum type as discriminator in union
> >   7 qapi: convert BlockdevOptions to use enum discriminator
> >   8 qapi script: do not allow string discriminator
> >   9 tests: add cases for inherited struct and union with discriminator
> >   10 qapi script: do not add "_" for every capitalized char in enum
> > 
> >  docs/qapi-code-gen.txt                  |    8 ++-
> >  include/qapi/qmp/qerror.h               |    2 +-
> >  qapi-schema.json                        |   14 ++++-
> >  scripts/qapi-types.py                   |   34 +++++-----
> >  scripts/qapi-visit.py                   |   61 ++++++++++++++---
> >  scripts/qapi.py                         |   84 +++++++++++++++++++++--
> >  target-i386/cpu.c                       |    2 +-
> >  tests/qapi-schema/comments.out          |    2 +-
> >  tests/qapi-schema/qapi-schema-test.json |   22 ++++++
> >  tests/qapi-schema/qapi-schema-test.out  |   13 +++-
> >  tests/test-qmp-input-visitor.c          |   93 +++++++++++++++++++++++++
> >  tests/test-qmp-output-visitor.c         |  116 +++++++++++++++++++++++++++++++
> >  12 files changed, 411 insertions(+), 40 deletions(-)
> > 
> 

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

[Markus Armbruster]

Wenchao Xia <xiawenc@linux.vnet.ibm.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)

Sorry for the lateness of my review.  I like this series, but I had a
few questions related to error reporting.  Also, the new errors lack
tests.

My offer to rebase it on top of my "[PATCH v2 00/13] qapi: Test coverage
& clean up generated code" stands.

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

[Wenchao Xia]

于 2014/2/13 23:23, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.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)
> 
> Sorry for the lateness of my review.  I like this series, but I had a
> few questions related to error reporting.  Also, the new errors lack
> tests.
> 
> My offer to rebase it on top of my "[PATCH v2 00/13] qapi: Test coverage
> & clean up generated code" stands.
> 
  As patch 9 is the conflict patch and easy to rebase, I am fine if you
want to apply yours first and rebase mine.
  About the error reporting, I am not sure which way is better. If you
like qapi.py do it, I can send a patch later moving the code, since it
is easier than modify multiple patches in this series.
s