qapi script: check correctness of union

Since line info is remembered as QAPISchema.line now, this patch
uses it as additional info for every expr in QAPISchema inside qapi.py,
then improves error message with it in checking of exprs.

For common union the patch will check whether base is a valid complex
type if specified. For flat union it will check whether base presents,
whether discriminator is found in base, whether the key of every branch
is correct when discriminator is an enum type.

Signed-off-by: Wenchao Xia <wenchaoqemu@gmail.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

18 files changed, 151 insertions, 3 deletions

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 1954292..f1ca5b6 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -50,6 +50,15 @@ class QAPISchemaError(Exception):
     def __str__(self):
         return "%s:%s:%s: %s" % (self.fp.name, self.line, self.col, self.msg)
 
+class QAPIExprError(Exception):
+    def __init__(self, expr_info, msg):
+        self.fp = expr_info['fp']
+        self.line = expr_info['line']
+        self.msg = msg
+
+    def __str__(self):
+        return "%s:%s: %s" % (self.fp.name, self.line, self.msg)
+
 class QAPISchema:
 
     def __init__(self, fp):
@@ -64,7 +73,10 @@ class QAPISchema:
         self.accept()
 
         while self.tok != None:
-            self.exprs.append(self.get_expr(False))
+            expr_info = {'fp': fp, 'line': self.line}
+            expr_elem = {'expr': self.get_expr(False),
+                         'info': expr_info}
+            self.exprs.append(expr_elem)
 
     def accept(self):
         while True:
@@ -162,6 +174,71 @@ 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']
+
+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)
+
+    # 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)
@@ -171,7 +248,8 @@ def parse_schema(fp):
 
     exprs = []
 
-    for expr in schema.exprs:
+    for expr_elem in schema.exprs:
+        expr = expr_elem['expr']
         if expr.has_key('enum'):
             add_enum(expr['enum'], expr['data'])
         elif expr.has_key('union'):
@@ -181,6 +259,12 @@ def parse_schema(fp):
             add_struct(expr)
         exprs.append(expr)
 
+    try:
+        check_exprs(schema)
+    except QAPIExprError, e:
+        print >>sys.stderr, e
+        exit(1)
+
     return exprs
 
 def parse_args(typeinfo):
diff --git a/tests/Makefile b/tests/Makefile
index dfe06eb..6ac9889 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -143,7 +143,9 @@ check-qapi-schema-y := $(addprefix tests/qapi-schema/, \
         qapi-schema-test.json quoted-structural-chars.json \
         trailing-comma-list.json trailing-comma-object.json \
         unclosed-list.json unclosed-object.json unclosed-string.json \
-        duplicate-key.json)
+        duplicate-key.json union-invalid-base.json flat-union-no-base.json \
+        flat-union-invalid-discriminator.json \
+        flat-union-invalid-branch-key.json)
 
 GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
 
diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.err b/tests/qapi-schema/flat-union-invalid-branch-key.err
new file mode 100644
index 0000000..1125caf
--- /dev/null
+++ b/tests/qapi-schema/flat-union-invalid-branch-key.err
@@ -0,0 +1 @@
+<stdin>:13: Discriminator value 'value_wrong' is not found in enum 'TestEnum'
diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.exit b/tests/qapi-schema/flat-union-invalid-branch-key.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/flat-union-invalid-branch-key.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.json b/tests/qapi-schema/flat-union-invalid-branch-key.json
new file mode 100644
index 0000000..a624282
--- /dev/null
+++ b/tests/qapi-schema/flat-union-invalid-branch-key.json
@@ -0,0 +1,17 @@
+{ 'enum': 'TestEnum',
+  'data': [ 'value1', 'value2' ] }
+
+{ 'type': 'TestBase',
+  'data': { 'enum1': 'TestEnum' } }
+
+{ 'type': 'TestTypeA',
+  'data': { 'string': 'str' } }
+
+{ 'type': 'TestTypeB',
+  'data': { 'integer': 'int' } }
+
+{ 'union': 'TestUnion',
+  'base': 'TestBase',
+  'discriminator': 'enum1',
+  'data': { 'value_wrong': 'TestTypeA',
+            'value2': 'TestTypeB' } }
diff --git a/tests/qapi-schema/flat-union-invalid-branch-key.out b/tests/qapi-schema/flat-union-invalid-branch-key.out
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/tests/qapi-schema/flat-union-invalid-branch-key.out
diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.err b/tests/qapi-schema/flat-union-invalid-discriminator.err
new file mode 100644
index 0000000..cad9dbf
--- /dev/null
+++ b/tests/qapi-schema/flat-union-invalid-discriminator.err
@@ -0,0 +1 @@
+<stdin>:13: Discriminator 'enum_wrong' is not a member of base type 'TestBase'
diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.exit b/tests/qapi-schema/flat-union-invalid-discriminator.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/flat-union-invalid-discriminator.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.json b/tests/qapi-schema/flat-union-invalid-discriminator.json
new file mode 100644
index 0000000..887157e
--- /dev/null
+++ b/tests/qapi-schema/flat-union-invalid-discriminator.json
@@ -0,0 +1,17 @@
+{ 'enum': 'TestEnum',
+  'data': [ 'value1', 'value2' ] }
+
+{ 'type': 'TestBase',
+  'data': { 'enum1': 'TestEnum' } }
+
+{ 'type': 'TestTypeA',
+  'data': { 'string': 'str' } }
+
+{ 'type': 'TestTypeB',
+  'data': { 'integer': 'int' } }
+
+{ 'union': 'TestUnion',
+  'base': 'TestBase',
+  'discriminator': 'enum_wrong',
+  'data': { 'value1': 'TestTypeA',
+            'value2': 'TestTypeB' } }
diff --git a/tests/qapi-schema/flat-union-invalid-discriminator.out b/tests/qapi-schema/flat-union-invalid-discriminator.out
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/tests/qapi-schema/flat-union-invalid-discriminator.out
diff --git a/tests/qapi-schema/flat-union-no-base.err b/tests/qapi-schema/flat-union-no-base.err
new file mode 100644
index 0000000..e2d7443
--- /dev/null
+++ b/tests/qapi-schema/flat-union-no-base.err
@@ -0,0 +1 @@
+<stdin>:7: Flat union 'TestUnion' must have a base field
diff --git a/tests/qapi-schema/flat-union-no-base.exit b/tests/qapi-schema/flat-union-no-base.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/flat-union-no-base.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/flat-union-no-base.json b/tests/qapi-schema/flat-union-no-base.json
new file mode 100644
index 0000000..50f2673
--- /dev/null
+++ b/tests/qapi-schema/flat-union-no-base.json
@@ -0,0 +1,10 @@
+{ 'type': 'TestTypeA',
+  'data': { 'string': 'str' } }
+
+{ 'type': 'TestTypeB',
+  'data': { 'integer': 'int' } }
+
+{ 'union': 'TestUnion',
+  'discriminator': 'enum1',
+  'data': { 'value1': 'TestTypeA',
+            'value2': 'TestTypeB' } }
diff --git a/tests/qapi-schema/flat-union-no-base.out b/tests/qapi-schema/flat-union-no-base.out
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/tests/qapi-schema/flat-union-no-base.out
diff --git a/tests/qapi-schema/union-invalid-base.err b/tests/qapi-schema/union-invalid-base.err
new file mode 100644
index 0000000..dd8e3d1
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-base.err
@@ -0,0 +1 @@
+<stdin>:7: Base 'TestBaseWrong' is not a valid type
diff --git a/tests/qapi-schema/union-invalid-base.exit b/tests/qapi-schema/union-invalid-base.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-base.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/union-invalid-base.json b/tests/qapi-schema/union-invalid-base.json
new file mode 100644
index 0000000..1fa4930
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-base.json
@@ -0,0 +1,10 @@
+{ 'type': 'TestTypeA',
+  'data': { 'string': 'str' } }
+
+{ 'type': 'TestTypeB',
+  'data': { 'integer': 'int' } }
+
+{ 'union': 'TestUnion',
+  'base': 'TestBaseWrong',
+  'data': { 'value1': 'TestTypeA',
+            'value2': 'TestTypeB' } }
diff --git a/tests/qapi-schema/union-invalid-base.out b/tests/qapi-schema/union-invalid-base.out
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-base.out