[Qemu-devel] [PATCH 0/8] qapi script: support enum as discriminator and better enum name

[Qemu-devel] [PATCH 0/8] qapi script: support enum as discriminator and better enum name

[Wenchao Xia]

This series is respined from RFC series at:
http://lists.nongnu.org/archive/html/qemu-devel/2013-11/msg00363.html

Patch 1-6 add support for enum as discriminator.
Patch 7 improve enum name generation, now AIOContext->AIO_CONTEXT, X86CPU->
X86_CPU.
Patch 8 are the test cases.

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.

Wenchao Xia (8):
  1 qapi script: remember enum values
  2 qapi script: report error for default case in union visit
  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: not generate hidden enum type for pre-defined enum discriminator
  7 qapi script: do not add "_" for every capitalized char in enum
  8 tests: add cases for inherited struct and union with discriminator

 include/qapi/qmp/qerror.h               |    2 +-
 scripts/qapi-types.py                   |   34 +++---
 scripts/qapi-visit.py                   |   63 +++++++++--
 scripts/qapi.py                         |   84 ++++++++++++++-
 target-i386/cpu.c                       |    2 +-
 tests/qapi-schema/comments.out          |    2 +-
 tests/qapi-schema/qapi-schema-test.json |   25 +++++
 tests/qapi-schema/qapi-schema-test.out  |   15 +++-
 tests/test-qmp-input-visitor.c          |  152 +++++++++++++++++++++++++++
 tests/test-qmp-output-visitor.c         |  172 +++++++++++++++++++++++++++++++
 10 files changed, 513 insertions(+), 38 deletions(-)

[Qemu-devel] [PATCH 1/8] 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 750e9fb..82f586e 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 2/8] qapi script: report error for default case in union visit

[Wenchao Xia]

It is possible to reach default case, when an union have a enum
discriminator, so don't abort() but report the error message.

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

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index c39e628..b3d3af8 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -223,6 +223,8 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
                 c_type = type_name(members[key]),
                 c_name = c_fun(key))
 
+    # Only support input visitor for an anon union now, and it is not possible
+    # to reach default, so abort() here, see the logic for (*obj)->kind
     ret += mcgen('''
         default:
             abort();
@@ -312,15 +314,23 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
                 c_type=type_name(members[key]),
                 c_name=c_fun(key))
 
+    # Tell caller the value is invalid, since the discriminator value maybe an
+    # unmapped enum value.
     ret += mcgen('''
             default:
-                abort();
+                error_setg(&err,
+                           "Invalid discriminator value %(pi)s for %(name)s",
+                           (*obj)->kind);
+                break;
             }
         }
         error_propagate(errp, err);
         err = NULL;
     }
-''')
+''',
+                pi="%d",
+                name=name)
+
     pop_indent()
     ret += mcgen('''
         /* Always call end_struct if start_struct succeeded.  */
-- 
1.7.1

[Qemu-devel] [PATCH 3/8] qapi script: check correctness of discriminator values in union

[Wenchao Xia]

It will check whether the values specified are written correctly when
discriminator is a pre-defined enum type, which help check whether the
schema is in good form.

It is allowed, that not every value in enum is used, so does not check
that case.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 scripts/qapi-visit.py |   11 +++++++++++
 scripts/qapi.py       |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index b3d3af8..612dc4d 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -251,6 +251,17 @@ 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' not found in "
+                                 "enum '%s'\n" %
+                                 (key, enum_define["enum_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 82f586e..f93bda1 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -383,3 +383,36 @@ 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:
+        sys.stderr.write("Base '%s' is not a valid type\n"
+                         % base)
+        sys.exit(1)
+
+    discriminator_type = base_fields.get(discriminator)
+
+    if not discriminator_type:
+        sys.stderr.write("Discriminator '%s' not found in schema\n"
+                         % discriminator)
+        sys.exit(1)
+
+    return find_enum(discriminator_type)
-- 
1.7.1

[Qemu-devel] [PATCH 4/8] 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>
---
 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 f93bda1..a3be92d 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -416,3 +416,13 @@ def discriminator_find_enum_define(expr):
         sys.exit(1)
 
     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 5/8] qapi script: use same function to generate enum string

[Wenchao Xia]

One function one rule, so the enum string generating have same
behavior for different caller. If multiple caller exist for one
enum define in schema, it is for sure the generated string is
identical.

Note before the patch qapi-visit.py used custom function to
generate the string in union visit, although the patch changes it,
the final string generated is not changed. The custom function used
before will met problem when capitalized discriminator value is
introduced.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.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 612dc4d..787034c 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -208,18 +208,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))
 
@@ -262,7 +267,10 @@ def generate_visit_union(expr):
                                  (key, enum_define["enum_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']
@@ -315,13 +323,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 a3be92d..f14b31f 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 6/8] qapi script: not generate hidden enum type for pre-defined enum discriminator

[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.

In short, enum type as discriminator is fully supported now.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 scripts/qapi-types.py |   18 ++++++++++++++----
 scripts/qapi-visit.py |   23 ++++++++++++++++-------
 scripts/qapi.py       |    4 +++-
 3 files changed, 33 insertions(+), 12 deletions(-)

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 787034c..85f17e1 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -267,10 +267,14 @@ def generate_visit_union(expr):
                                  (key, enum_define["enum_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']
@@ -305,11 +309,12 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
 
     pop_indent()
     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="type" if not discriminator else discriminator)
+                 discriminator_type_name=discriminator_type_name,
+                 type="type" if not discriminator else discriminator)
 
     for key in members:
         if not discriminator:
@@ -531,7 +536,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 f14b31f..462a6ec 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -172,7 +172,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 7/8] 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>
---
 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 c30c2f6..6fa07b4 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -160,7 +160,7 @@ void assert_no_error(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 462a6ec..08a854e 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 "_" appear before, do more check
+        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 864c80e..6ba1945 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

[Qemu-devel] [PATCH 8/8] 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 |   25 +++++
 tests/qapi-schema/qapi-schema-test.out  |   11 ++
 tests/test-qmp-input-visitor.c          |  152 +++++++++++++++++++++++++++
 tests/test-qmp-output-visitor.c         |  172 +++++++++++++++++++++++++++++++
 4 files changed, 360 insertions(+), 0 deletions(-)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index fe5af75..289df68 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,21 @@
 { 'union': 'UserDefUnion',
   'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
 
+{ 'union': 'UserDefBaseUnion',
+  'base': 'UserDefBase0',
+  'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
+
+{ 'union': 'UserDefDiscriminatorUnion',
+  'base': 'UserDefBase0',
+  'discriminator' : 'base-string0',
+  'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
+
+# A complex type
+{ 'union': 'UserDefEnumDiscriminatorUnion',
+  'base': 'UserDefBase0',
+  'discriminator' : 'base-enum0',
+  'data': { 'value1' : 'UserDefA', 'value2' : 'UserDefInherit' } }
+
 # 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..e2e56d8 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -3,9 +3,15 @@
  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', 'UserDefDiscriminatorUnion'), ('base', 'UserDefBase0'), ('discriminator', 'base-string0'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
+ OrderedDict([('union', 'UserDefEnumDiscriminatorUnion'), ('base', 'UserDefBase0'), ('discriminator', 'base-enum0'), ('data', OrderedDict([('value1', 'UserDefA'), ('value2', 'UserDefInherit')]))]),
  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 +19,16 @@
  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': 'UserDefDiscriminatorUnionKind', '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..aeaf0ba 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,120 @@ 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_discriminator_union(TestInputVisitorData *data,
+                                                const void *unused)
+{
+    Visitor *v;
+    Error *err = NULL;
+    UserDefDiscriminatorUnion *tmp;
+    const char *input =
+    "{ \
+        'integer': 2, \
+        'base-enum0': 'value2', \
+        'base-string0': 'b' \
+    }";
+
+    v = visitor_input_test_init_raw(data, input);
+
+    visit_type_UserDefDiscriminatorUnion(v, &tmp, NULL, &err);
+    g_assert(err == NULL);
+    g_assert_cmpint(tmp->base_enum0, ==, ENUM_ONE_VALUE2);
+    g_assert_cmpint(tmp->kind, ==, USER_DEF_DISCRIMINATOR_UNION_KIND_B);
+    g_assert_cmpint(tmp->b->integer, ==, 2);
+
+    qapi_free_UserDefDiscriminatorUnion(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_visitor_in_enum_union_error(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': 'value3', \
+        'base-string0': 'test' \
+    }";
+
+    v = visitor_input_test_init_raw(data, input);
+
+    /* value3 is not mapped, an error should happen, no core dump */
+    visit_type_UserDefEnumDiscriminatorUnion(v, &tmp, NULL, &err);
+    g_assert(err);
+
+    g_assert_cmpstr(tmp->base_string0, ==, "test");
+    g_assert_cmpint(tmp->kind, ==, ENUM_ONE_VALUE3);
+
+    qapi_free_UserDefEnumDiscriminatorUnion(tmp);
+    error_free(err);
+}
+
 static void test_native_list_integer_helper(TestInputVisitorData *data,
                                             const void *unused,
                                             UserDefNativeListUnionKind kind)
@@ -633,8 +772,21 @@ 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/discriminator-union",
+                            &in_visitor_data,
+                            test_visitor_in_discriminator_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/enum-union-error",
+                            &in_visitor_data,
+                            test_visitor_in_enum_union_error);
     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..d7ae597 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,133 @@ 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);
+}
+
+static void test_visitor_out_discriminator_union(TestOutputVisitorData *data,
+                                                 const void *unused)
+{
+    QObject *arg;
+    QDict *qdict;
+
+    Error *err = NULL;
+
+    UserDefDiscriminatorUnion *tmp =
+            g_malloc0(sizeof(UserDefDiscriminatorUnion));
+    tmp->base_enum0 = ENUM_ONE_VALUE2;
+    tmp->kind = USER_DEF_DISCRIMINATOR_UNION_KIND_B;
+    tmp->b = g_malloc0(sizeof(UserDefB));
+    tmp->b->integer = 2;
+
+    visit_type_UserDefDiscriminatorUnion(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"), ==, "b");
+    g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 2);
+
+    qapi_free_UserDefDiscriminatorUnion(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);
+
+}
+
+/* A complex type */
+static void test_visitor_out_enum_union_error(TestOutputVisitorData *data,
+                                              const void *unused)
+{
+    Error *err = NULL;
+    const char *test_str = "test";
+
+    UserDefEnumDiscriminatorUnion *tmp =
+            g_malloc0(sizeof(UserDefEnumDiscriminatorUnion));
+    tmp->base_string0 = g_strdup(test_str);
+    /* Set it to a valid enum value, but not mapped in union */
+    tmp->kind = ENUM_ONE_VALUE3;
+
+    visit_type_UserDefEnumDiscriminatorUnion(data->ov, &tmp, NULL, &err);
+    g_assert(err);
+
+    qapi_free_UserDefEnumDiscriminatorUnion(tmp);
+    error_free(err);
+}
+
 static void init_native_list(UserDefNativeListUnion *cvalue)
 {
     int i;
@@ -780,8 +939,21 @@ 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/discriminator-union",
+                            &out_visitor_data,
+                            test_visitor_out_discriminator_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/enum-union-error",
+                            &out_visitor_data,
+                            test_visitor_out_enum_union_error);
     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 2/8] qapi script: report error for default case in union visit

[Eric Blake]

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

On 11/06/2013 12:33 PM, Wenchao Xia wrote:
> It is possible to reach default case, when an union have a enum
> discriminator, so don't abort() but report the error message.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  scripts/qapi-visit.py |   14 ++++++++++++--
>  1 files changed, 12 insertions(+), 2 deletions(-)

I still think this is not ideal.  You are proposing a runtime error:

> +    # Tell caller the value is invalid, since the discriminator value maybe an
> +    # unmapped enum value.
>      ret += mcgen('''
>              default:
> -                abort();
> +                error_setg(&err,
> +                           "Invalid discriminator value %(pi)s for %(name)s",
> +                           (*obj)->kind);
> +                break;
>              }

whereas I'm requesting a compile-time error - it is much easier for
maintenance reasons to have the generator require up-front that all enum
values are covered, and loudly complain if an enum type is extended
without also extending the use of that enum type in a union, than it is
to silently generate a runtime error and wait for the bug reports
several weeks down the road.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH 3/8] qapi script: check correctness of discriminator values in union

[Eric Blake]

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

On 11/06/2013 12:33 PM, Wenchao Xia wrote:
> It will check whether the values specified are written correctly when
> discriminator is a pre-defined enum type, which help check whether the
> schema is in good form.
> 
> It is allowed, that not every value in enum is used, so does not check
> that case.

Again, I think you should require that every value in the enum is used.

> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  scripts/qapi-visit.py |   11 +++++++++++
>  scripts/qapi.py       |   33 +++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 0 deletions(-)
> 
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index b3d3af8..612dc4d 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -251,6 +251,17 @@ 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' not found in "
> +                                 "enum '%s'\n" %
> +                                 (key, enum_define["enum_name"]))
> +                sys.exit(1)

This checks for union branches not covered by the enum, but does not
check for duplicate union branches, nor does it check for enum values
not covered by a union branch.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH 4/8] qapi script: code move for generate_enum_name()

[Eric Blake]

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

On 11/06/2013 12:33 PM, Wenchao Xia wrote:
> 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>
> ---
>  scripts/qapi-types.py |   10 ----------
>  scripts/qapi.py       |   10 ++++++++++
>  2 files changed, 10 insertions(+), 10 deletions(-)

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

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


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

Re: [Qemu-devel] [PATCH 3/8] qapi script: check correctness of discriminator values in union

[Wenchao Xia]

于 2013/11/13 2:12, Eric Blake 写道:
> On 11/06/2013 12:33 PM, Wenchao Xia wrote:
>> It will check whether the values specified are written correctly when
>> discriminator is a pre-defined enum type, which help check whether the
>> schema is in good form.
>>
>> It is allowed, that not every value in enum is used, so does not check
>> that case.
>
> Again, I think you should require that every value in the enum is used.
>
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   scripts/qapi-visit.py |   11 +++++++++++
>>   scripts/qapi.py       |   33 +++++++++++++++++++++++++++++++++
>>   2 files changed, 44 insertions(+), 0 deletions(-)
>>
>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>> index b3d3af8..612dc4d 100644
>> --- a/scripts/qapi-visit.py
>> +++ b/scripts/qapi-visit.py
>> @@ -251,6 +251,17 @@ 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' not found in "
>> +                                 "enum '%s'\n" %
>> +                                 (key, enum_define["enum_name"]))
>> +                sys.exit(1)
>
> This checks for union branches not covered by the enum, but does not
> check for duplicate union branches, nor does it check for enum values
> not covered by a union branch.
>
   I agree that, allowing enum values not covered by union branch,
require more carefully error handling for caller, such as visitors,
will add the check for safty reason.