Merge remote-tracking branch 'remotes/qmp-unstable/queue/qmp' into staging

* remotes/qmp-unstable/queue/qmp:
  tests: test-qmp-commands: Fix double free
  qapi script: do not add "_" for every capitalized char in enum
  qapi script: do not allow string discriminator
  qapi: convert BlockdevOptions to use enum discriminator
  qapi script: support enum type as discriminator in union
  qapi script: use same function to generate enum string
  qapi script: code move for generate_enum_name()
  qapi script: check correctness of union
  qapi script: remember line number in schema parsing
  qapi script: add check for duplicated key
  qapi script: remember explicitly defined enum values

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

docs/qapi-code-gen.txt

@@ -123,11 +123,12 @@ 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 must be of enumeration type.
 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',

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"

qapi-schema.json

@@ -4248,6 +4248,18 @@
             '*direct': 'bool',
             '*no-flush': 'bool' } }
 
+##
+# @BlockdevDriver
+#
+# Drivers that are supported in block device operations.
+#
+# Since: 2.0
+##
+{ 'enum': 'BlockdevDriver',
+  'data': [ 'file', 'http', 'https', 'ftp', 'ftps', 'tftp', 'vvfat', 'blkdebug',
+            'blkverify', 'bochs', 'cloop', 'cow', 'dmg', 'parallels', 'qcow',
+            'qcow2', 'qed', 'raw', 'vdi', 'vhdx', 'vmdk', 'vpc', 'quorum' ] }
+
 ##
 # @BlockdevOptionsBase
 #
@@ -4272,7 +4284,7 @@
 # Since: 1.7
 ##
 { 'type': 'BlockdevOptionsBase',
-  'data': { 'driver': 'str',
+  'data': { 'driver': 'BlockdevDriver',
             '*id': 'str',
             '*node-name': 'str',
             '*discard': 'BlockdevDiscardOptions',

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[];
@@ -154,11 +144,11 @@ typedef enum %(name)s
 
     i = 0
     for value in enum_values:
+        enum_full_value = generate_enum_full_value(name, value)
         enum_decl += mcgen('''
-    %(abbrev)s_%(value)s = %(i)d,
+    %(enum_full_value)s = %(i)d,
 ''',
-                     abbrev=de_camel_case(name).upper(),
-                     value=generate_enum_name(value),
+                     enum_full_value = enum_full_value,
                      i=i)
         i += 1
 
@@ -211,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('''
@@ -399,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:

scripts/qapi-visit.py

@@ -214,18 +214,22 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
 ''',
     name=name)
 
+    # For anon union, always use the default enum type automatically generated
+    # as "'%sKind' % (name)"
+    disc_type = '%sKind' % (name)
+
     for key in members:
         assert (members[key] in builtin_types
             or find_struct(members[key])
             or find_union(members[key])), "Invalid anonymous union member"
 
+        enum_full_value = generate_enum_full_value(disc_type, key)
         ret += mcgen('''
-        case %(abbrev)s_KIND_%(enum)s:
+        case %(enum_full_value)s:
             visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err);
             break;
 ''',
-                abbrev = de_camel_case(name).upper(),
-                enum = c_fun(de_camel_case(key),False).upper(),
+                enum_full_value = enum_full_value,
                 c_type = type_name(members[key]),
                 c_name = c_fun(key))
 
@@ -255,7 +259,16 @@ def generate_visit_union(expr):
         assert not base
         return generate_visit_anon_union(name, members)
 
-    ret = generate_visit_enum('%sKind' % name, members.keys())
+    enum_define = discriminator_find_enum_define(expr)
+    if enum_define:
+        # Use the enum type as discriminator
+        ret = ""
+        disc_type = enum_define['enum_name']
+    else:
+        # There will always be a discriminator in the C switch code, by default it
+        # is an enum type generated silently as "'%sKind' % (name)"
+        ret = generate_visit_enum('%sKind' % name, members.keys())
+        disc_type = '%sKind' % (name)
 
     if base:
         base_fields = find_struct(base)['data']
@@ -291,15 +304,16 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
     pop_indent()
 
     if not discriminator:
-        desc_type = "type"
+        disc_key = "type"
     else:
-        desc_type = discriminator
+        disc_key = discriminator
     ret += mcgen('''
-        visit_type_%(name)sKind(m, &(*obj)->kind, "%(type)s", &err);
+        visit_type_%(disc_type)s(m, &(*obj)->kind, "%(disc_key)s", &err);
         if (!err) {
             switch ((*obj)->kind) {
 ''',
-                 name=name, type=desc_type)
+                 disc_type = disc_type,
+                 disc_key = disc_key)
 
     for key in members:
         if not discriminator:
@@ -313,13 +327,13 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
                     visit_end_implicit_struct(m, &err);
                 }'''
 
+        enum_full_value = generate_enum_full_value(disc_type, key)
         ret += mcgen('''
-            case %(abbrev)s_KIND_%(enum)s:
+            case %(enum_full_value)s:
                 ''' + fmt + '''
                 break;
 ''',
-                abbrev = de_camel_case(name).upper(),
-                enum = c_fun(de_camel_case(key),False).upper(),
+                enum_full_value = enum_full_value,
                 c_type=type_name(members[key]),
                 c_name=c_fun(key))
 
@@ -510,7 +524,11 @@ for expr in exprs:
         ret += generate_visit_list(expr['union'], expr['data'])
         fdef.write(ret)
 
-        ret = generate_decl_enum('%sKind' % expr['union'], expr['data'].keys())
+        enum_define = discriminator_find_enum_define(expr)
+        ret = ""
+        if not enum_define:
+            ret = generate_decl_enum('%sKind' % expr['union'],
+                                     expr['data'].keys())
         ret += generate_declaration(expr['union'], expr['data'])
         fdecl.write(ret)
     elif expr.has_key('enum'):

scripts/qapi.py

@@ -39,12 +39,10 @@ class QAPISchemaError(Exception):
     def __init__(self, schema, msg):
         self.fp = schema.fp
         self.msg = msg
-        self.line = self.col = 1
-        for ch in schema.src[0:schema.pos]:
-            if ch == '\n':
-                self.line += 1
-                self.col = 1
-            elif ch == '\t':
+        self.col = 1
+        self.line = schema.line
+        for ch in schema.src[schema.line_pos:schema.pos]:
+            if ch == '\t':
                 self.col = (self.col + 7) % 8 + 1
             else:
                 self.col += 1
@@ -52,6 +50,15 @@ class QAPISchemaError(Exception):
     def __str__(self):
         return "%s:%s:%s: %s" % (self.fp.name, self.line, self.col, self.msg)
 
+class QAPIExprError(Exception):
+    def __init__(self, expr_info, msg):
+        self.fp = expr_info['fp']
+        self.line = expr_info['line']
+        self.msg = msg
+
+    def __str__(self):
+        return "%s:%s: %s" % (self.fp.name, self.line, self.msg)
+
 class QAPISchema:
 
     def __init__(self, fp):
@@ -60,11 +67,16 @@ class QAPISchema:
         if self.src == '' or self.src[-1] != '\n':
             self.src += '\n'
         self.cursor = 0
+        self.line = 1
+        self.line_pos = 0
         self.exprs = []
         self.accept()
 
         while self.tok != None:
-            self.exprs.append(self.get_expr(False))
+            expr_info = {'fp': fp, 'line': self.line}
+            expr_elem = {'expr': self.get_expr(False),
+                         'info': expr_info}
+            self.exprs.append(expr_elem)
 
     def accept(self):
         while True:
@@ -100,6 +112,8 @@ class QAPISchema:
                 if self.cursor == len(self.src):
                     self.tok = None
                     return
+                self.line += 1
+                self.line_pos = self.cursor
             elif not self.tok.isspace():
                 raise QAPISchemaError(self, 'Stray "%s"' % self.tok)
 
@@ -116,6 +130,8 @@ class QAPISchema:
             if self.tok != ':':
                 raise QAPISchemaError(self, 'Expected ":"')
             self.accept()
+            if key in expr:
+                raise QAPISchemaError(self, 'Duplicate key "%s"' % key)
             expr[key] = self.get_expr(True)
             if self.tok == '}':
                 self.accept()
@@ -158,6 +174,95 @@ class QAPISchema:
             raise QAPISchemaError(self, 'Expected "{", "[" or string')
         return expr
 
+def find_base_fields(base):
+    base_struct_define = find_struct(base)
+    if not base_struct_define:
+        return None
+    return base_struct_define['data']
+
+# Return the discriminator enum define if discriminator is specified as an
+# enum type, otherwise return None.
+def discriminator_find_enum_define(expr):
+    base = expr.get('base')
+    discriminator = expr.get('discriminator')
+
+    if not (discriminator and base):
+        return None
+
+    base_fields = find_base_fields(base)
+    if not base_fields:
+        return None
+
+    discriminator_type = base_fields.get(discriminator)
+    if not discriminator_type:
+        return None
+
+    return find_enum(discriminator_type)
+
+def check_union(expr, expr_info):
+    name = expr['union']
+    base = expr.get('base')
+    discriminator = expr.get('discriminator')
+    members = expr['data']
+
+    # If the object has a member 'base', its value must name a complex type.
+    if base:
+        base_fields = find_base_fields(base)
+        if not base_fields:
+            raise QAPIExprError(expr_info,
+                                "Base '%s' is not a valid type"
+                                % base)
+
+    # If the union object has no member 'discriminator', it's an
+    # ordinary union.
+    if not discriminator:
+        enum_define = None
+
+    # Else if the value of member 'discriminator' is {}, it's an
+    # anonymous union.
+    elif discriminator == {}:
+        enum_define = None
+
+    # Else, it's a flat union.
+    else:
+        # The object must have a member 'base'.
+        if not base:
+            raise QAPIExprError(expr_info,
+                                "Flat union '%s' must have a base field"
+                                % name)
+        # The value of member 'discriminator' must name a member of the
+        # base type.
+        discriminator_type = base_fields.get(discriminator)
+        if not discriminator_type:
+            raise QAPIExprError(expr_info,
+                                "Discriminator '%s' is not a member of base "
+                                "type '%s'"
+                                % (discriminator, base))
+        enum_define = find_enum(discriminator_type)
+        # Do not allow string discriminator
+        if not enum_define:
+            raise QAPIExprError(expr_info,
+                                "Discriminator '%s' must be of enumeration "
+                                "type" % discriminator)
+
+    # Check every branch
+    for (key, value) in members.items():
+        # If this named member's value names an enum type, then all members
+        # of 'data' must also be members of the enum type.
+        if enum_define and not key in enum_define['enum_values']:
+            raise QAPIExprError(expr_info,
+                                "Discriminator value '%s' is not found in "
+                                "enum '%s'" %
+                                (key, enum_define["enum_name"]))
+        # Todo: add checking for values. Key is checked as above, value can be
+        # also checked here, but we need more functions to handle array case.
+
+def check_exprs(schema):
+    for expr_elem in schema.exprs:
+        expr = expr_elem['expr']
+        if expr.has_key('union'):
+            check_union(expr, expr_elem['info'])
+
 def parse_schema(fp):
     try:
         schema = QAPISchema(fp)
@@ -167,16 +272,29 @@ def parse_schema(fp):
 
     exprs = []
 
-    for expr in schema.exprs:
+    for expr_elem in schema.exprs:
+        expr = expr_elem['expr']
         if expr.has_key('enum'):
-            add_enum(expr['enum'])
+            add_enum(expr['enum'], expr['data'])
         elif expr.has_key('union'):
             add_union(expr)
-            add_enum('%sKind' % expr['union'])
         elif expr.has_key('type'):
             add_struct(expr)
         exprs.append(expr)
 
+    # Try again for hidden UnionKind enum
+    for expr_elem in schema.exprs:
+        expr = expr_elem['expr']
+        if expr.has_key('union'):
+            if not discriminator_find_enum_define(expr):
+                add_enum('%sKind' % expr['union'])
+
+    try:
+        check_exprs(schema)
+    except QAPIExprError, e:
+        print >>sys.stderr, e
+        exit(1)
+
     return exprs
 
 def parse_args(typeinfo):
@@ -289,13 +407,19 @@ def find_union(name):
             return union
     return None
 
-def add_enum(name):
+def add_enum(name, enum_values = None):
     global enum_types
-    enum_types.append(name)
+    enum_types.append({"enum_name": name, "enum_values": enum_values})
+
+def 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)
+    return find_enum(name) != None
 
 def c_type(name):
     if name == 'str':
@@ -373,3 +497,30 @@ def guardend(name):
 
 ''',
                  name=guardname(name))
+
+# 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_str
+
+    new_name = ''
+    l = len(c_fun_str)
+    for i in range(l):
+        c = c_fun_str[i]
+        # When c is upper and no "_" appears before, do more checks
+        if c.isupper() and (i > 0) and c_fun_str[i - 1] != "_":
+            # Case 1: next string is lower
+            # Case 2: previous string is digit
+            if (i < (l - 1) and c_fun_str[i + 1].islower()) or \
+            c_fun_str[i - 1].isdigit():
+                new_name += '_'
+        new_name += c
+    return new_name.lstrip('_').upper()
+
+def generate_enum_full_value(enum_name, enum_value):
+    abbrev_string = _generate_enum_string(enum_name)
+    value_string = _generate_enum_string(enum_value)
+    return "%s_%s" % (abbrev_string, value_string)

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

tests/Makefile

@@ -142,7 +142,11 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
         missing-comma-object.json non-objects.json \
         qapi-schema-test.json quoted-structural-chars.json \
         trailing-comma-list.json trailing-comma-object.json \
-        unclosed-list.json unclosed-object.json unclosed-string.json)
+        unclosed-list.json unclosed-object.json unclosed-string.json \
+        duplicate-key.json union-invalid-base.json flat-union-no-base.json \
+        flat-union-invalid-discriminator.json \
+        flat-union-invalid-branch-key.json flat-union-reverse-define.json \
+        flat-union-string-discriminator.json)
 
 GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
 

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']}]
 []

tests/qapi-schema/duplicate-key.err

@@ -0,0 +1 @@
+<stdin>:2:10: Duplicate key "key"

tests/qapi-schema/duplicate-key.exit

@@ -0,0 +1 @@
+1

tests/qapi-schema/duplicate-key.json

@@ -0,0 +1,2 @@
+{ 'key': 'value',
+  'key': 'value' }

tests/qapi-schema/flat-union-invalid-branch-key.err

@@ -0,0 +1 @@
+<stdin>:13: Discriminator value 'value_wrong' is not found in enum 'TestEnum'

tests/qapi-schema/flat-union-invalid-branch-key.exit

@@ -0,0 +1 @@
+1

tests/qapi-schema/flat-union-invalid-branch-key.json

@@ -0,0 +1,17 @@
+{ 'enum': 'TestEnum',
+  'data': [ 'value1', 'value2' ] }
+
+{ 'type': 'TestBase',
+  'data': { 'enum1': 'TestEnum' } }
+
+{ 'type': 'TestTypeA',
+  'data': { 'string': 'str' } }
+
+{ 'type': 'TestTypeB',
+  'data': { 'integer': 'int' } }
+
+{ 'union': 'TestUnion',
+  'base': 'TestBase',
+  'discriminator': 'enum1',
+  'data': { 'value_wrong': 'TestTypeA',
+            'value2': 'TestTypeB' } }

tests/qapi-schema/flat-union-invalid-discriminator.err

@@ -0,0 +1 @@
+<stdin>:13: Discriminator 'enum_wrong' is not a member of base type 'TestBase'

tests/qapi-schema/flat-union-invalid-discriminator.exit

@@ -0,0 +1 @@
+1

tests/qapi-schema/flat-union-invalid-discriminator.json

@@ -0,0 +1,17 @@
+{ 'enum': 'TestEnum',
+  'data': [ 'value1', 'value2' ] }
+
+{ 'type': 'TestBase',
+  'data': { 'enum1': 'TestEnum' } }
+
+{ 'type': 'TestTypeA',
+  'data': { 'string': 'str' } }
+
+{ 'type': 'TestTypeB',
+  'data': { 'integer': 'int' } }
+
+{ 'union': 'TestUnion',
+  'base': 'TestBase',
+  'discriminator': 'enum_wrong',
+  'data': { 'value1': 'TestTypeA',
+            'value2': 'TestTypeB' } }