qapi: Better error messages for bad enums

The previous commit demonstrated that the generator had several
flaws with less-than-perfect enums:
- an enum that listed the same string twice (or two variant
strings that map to the same C enumerator) ended up generating
an invalid C enum
- because the generator adds a _MAX terminator to each enum,
the use of an enum member 'max' can also cause this clash
- if an enum omits 'data', the generator left a python stack
trace rather than a graceful message
- an enum that used a non-array 'data' was silently accepted by
the parser
- an enum that used non-string members in the 'data' member
was silently accepted by the parser

Add check_enum to cover these situations, and update testcases
to match.  While valid .json files won't trigger any of these
cases, we might as well be nicer to developers that make a typo
while trying to add new QAPI code.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 20ee505430319d250d4eb0039dc0ddfd3075d675..3ce8c3321b2deb4c9d9fe25112c5a6ab233aac40 100644 (file)
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -311,13 +311,37 @@ def check_union(expr, expr_info):
         # 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_enum(expr, expr_info):
+    name = expr['enum']
+    members = expr.get('data')
+    values = { 'MAX': '(automatic)' }
+
+    if not isinstance(members, list):
+        raise QAPIExprError(expr_info,
+                            "Enum '%s' requires an array for 'data'" % name)
+    for member in members:
+        if not isinstance(member, str):
+            raise QAPIExprError(expr_info,
+                                "Enum '%s' member '%s' is not a string"
+                                % (name, member))
+        key = _generate_enum_string(member)
+        if key in values:
+            raise QAPIExprError(expr_info,
+                                "Enum '%s' member '%s' clashes with '%s'"
+                                % (name, member, values[key]))
+        values[key] = member
+
 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'])
-        if expr.has_key('event'):
-            check_event(expr, expr_elem['info'])
+        info = expr_elem['info']
+
+        if expr.has_key('enum'):
+            check_enum(expr, info)
+        elif expr.has_key('union'):
+            check_union(expr, info)
+        elif expr.has_key('event'):
+            check_event(expr, info)
 
 def parse_schema(input_file):
     try:
@@ -331,7 +355,7 @@ def parse_schema(input_file):
     for expr_elem in schema.exprs:
         expr = expr_elem['expr']
         if expr.has_key('enum'):
-            add_enum(expr['enum'], expr['data'])
+            add_enum(expr['enum'], expr.get('data'))
         elif expr.has_key('union'):
             add_union(expr)
         elif expr.has_key('type'):

diff --git a/tests/qapi-schema/enum-clash-member.err b/tests/qapi-schema/enum-clash-member.err
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..48bd1360e75af38f0a5b4db38d500c6f569f4acd 100644 (file)
--- a/tests/qapi-schema/enum-clash-member.err
+++ b/tests/qapi-schema/enum-clash-member.err
@@ -0,0 +1 @@
+tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' member 'ONE' clashes with 'one'

diff --git a/tests/qapi-schema/enum-clash-member.exit b/tests/qapi-schema/enum-clash-member.exit
index 573541ac9702dd3969c9bc859d2b91ec1f7e6e56..d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 100644 (file)
--- a/tests/qapi-schema/enum-clash-member.exit
+++ b/tests/qapi-schema/enum-clash-member.exit
@@ -1 +1 @@
-0
+1

diff --git a/tests/qapi-schema/enum-clash-member.json b/tests/qapi-schema/enum-clash-member.json
index 99d442a9789eb1b05b8f97514674dcf63454739e..b7dc02a28d67d3145092f2a58e35505f15925fb9 100644 (file)
--- a/tests/qapi-schema/enum-clash-member.json
+++ b/tests/qapi-schema/enum-clash-member.json
@@ -1,2 +1,2 @@
-# FIXME: we should reject enums where members will clash when mapped to C enum
+# we reject enums where members will clash when mapped to C enum
 { 'enum': 'MyEnum', 'data': [ 'one', 'ONE' ] }

diff --git a/tests/qapi-schema/enum-clash-member.out b/tests/qapi-schema/enum-clash-member.out
index 08144597214e54b6ccdab3c4ad12b19b9bab35db..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 100644 (file)
--- a/tests/qapi-schema/enum-clash-member.out
+++ b/tests/qapi-schema/enum-clash-member.out
@@ -1,3 +0,0 @@
-[OrderedDict([('enum', 'MyEnum'), ('data', ['one', 'ONE'])])]
-[{'enum_name': 'MyEnum', 'enum_values': ['one', 'ONE']}]
-[]

diff --git a/tests/qapi-schema/enum-dict-member.err b/tests/qapi-schema/enum-dict-member.err
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..7e966a8aaefbc2c907ea65b0fa28c5ef074a6cc5 100644 (file)
--- a/tests/qapi-schema/enum-dict-member.err
+++ b/tests/qapi-schema/enum-dict-member.err
@@ -0,0 +1 @@
+tests/qapi-schema/enum-dict-member.json:2: Enum 'MyEnum' member 'OrderedDict([('value', 'str')])' is not a string

diff --git a/tests/qapi-schema/enum-dict-member.exit b/tests/qapi-schema/enum-dict-member.exit
index 573541ac9702dd3969c9bc859d2b91ec1f7e6e56..d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 100644 (file)
--- a/tests/qapi-schema/enum-dict-member.exit
+++ b/tests/qapi-schema/enum-dict-member.exit
@@ -1 +1 @@
-0
+1

diff --git a/tests/qapi-schema/enum-dict-member.json b/tests/qapi-schema/enum-dict-member.json
index de4d6bf7773ebf535d087286865d59bde9ebd698..79672e0f091ab56989b189cf8eb5fa7a08bf1e9d 100644 (file)
--- a/tests/qapi-schema/enum-dict-member.json
+++ b/tests/qapi-schema/enum-dict-member.json
@@ -1,2 +1,2 @@
-# FIXME: we should reject any enum member that is not a string
+# we reject any enum member that is not a string
 { 'enum': 'MyEnum', 'data': [ { 'value': 'str' } ] }

diff --git a/tests/qapi-schema/enum-dict-member.out b/tests/qapi-schema/enum-dict-member.out
index 8b293f85c5c9f137b4de8abded6034ac598c9665..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 100644 (file)
--- a/tests/qapi-schema/enum-dict-member.out
+++ b/tests/qapi-schema/enum-dict-member.out
@@ -1,3 +0,0 @@
-[OrderedDict([('enum', 'MyEnum'), ('data', [OrderedDict([('value', 'str')])])])]
-[{'enum_name': 'MyEnum', 'enum_values': [OrderedDict([('value', 'str')])]}]
-[]

diff --git a/tests/qapi-schema/enum-max-member.err b/tests/qapi-schema/enum-max-member.err
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..f77837fb45e11fedfb08a1ee8a9d78bbaf88ab37 100644 (file)
--- a/tests/qapi-schema/enum-max-member.err
+++ b/tests/qapi-schema/enum-max-member.err
@@ -0,0 +1 @@
+tests/qapi-schema/enum-max-member.json:3: Enum 'MyEnum' member 'max' clashes with '(automatic)'

diff --git a/tests/qapi-schema/enum-max-member.exit b/tests/qapi-schema/enum-max-member.exit
index 573541ac9702dd3969c9bc859d2b91ec1f7e6e56..d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 100644 (file)
--- a/tests/qapi-schema/enum-max-member.exit
+++ b/tests/qapi-schema/enum-max-member.exit
@@ -1 +1 @@
-0
+1

diff --git a/tests/qapi-schema/enum-max-member.json b/tests/qapi-schema/enum-max-member.json
index 15195413449eed5b8c506f441bddd1d2cfdeaa8d..4bcda0bf0749274e414d3690c5c9113345114bc1 100644 (file)
--- a/tests/qapi-schema/enum-max-member.json
+++ b/tests/qapi-schema/enum-max-member.json
@@ -1,3 +1,3 @@
-# FIXME: we should reject user-supplied 'max' for clashing with implicit enum end
+# we reject user-supplied 'max' for clashing with implicit enum end
 # TODO: should we instead munge the implicit value to avoid the clash?
 { 'enum': 'MyEnum', 'data': [ 'max' ] }

diff --git a/tests/qapi-schema/enum-max-member.out b/tests/qapi-schema/enum-max-member.out
index c933044c69b5791757fa2605405b85704960b181..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 100644 (file)
--- a/tests/qapi-schema/enum-max-member.out
+++ b/tests/qapi-schema/enum-max-member.out
@@ -1,3 +0,0 @@
-[OrderedDict([('enum', 'MyEnum'), ('data', ['max'])])]
-[{'enum_name': 'MyEnum', 'enum_values': ['max']}]
-[]

diff --git a/tests/qapi-schema/enum-missing-data.err b/tests/qapi-schema/enum-missing-data.err
index 814ab2613db1dba37bb406d9b757cc1ecbfe94fb..b8ccae071b2873aae902b734c09583c4112589cf 100644 (file)
--- a/tests/qapi-schema/enum-missing-data.err
+++ b/tests/qapi-schema/enum-missing-data.err
@@ -1,6 +1 @@
-Traceback (most recent call last):
-  File "tests/qapi-schema/test-qapi.py", line 19, in <module>
-    exprs = parse_schema(sys.argv[1])
-  File "scripts/qapi.py", line 334, in parse_schema
-    add_enum(expr['enum'], expr['data'])
-KeyError: 'data'
+tests/qapi-schema/enum-missing-data.json:2: Enum 'MyEnum' requires an array for 'data'

diff --git a/tests/qapi-schema/enum-missing-data.json b/tests/qapi-schema/enum-missing-data.json
index 01f3f32e70a77a9b1c45f6730e06672e3af5e946..558fd35e93f8a68725ca915c89fa3997f46a51ab 100644 (file)
--- a/tests/qapi-schema/enum-missing-data.json
+++ b/tests/qapi-schema/enum-missing-data.json
@@ -1,2 +1,2 @@
-# FIXME: we should require that all QAPI enums have a data array
+# we require that all QAPI enums have a data array
 { 'enum': 'MyEnum' }

diff --git a/tests/qapi-schema/enum-wrong-data.err b/tests/qapi-schema/enum-wrong-data.err
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..11b43471cf34a1f028e75b3756fa6eda93d1a36b 100644 (file)
--- a/tests/qapi-schema/enum-wrong-data.err
+++ b/tests/qapi-schema/enum-wrong-data.err
@@ -0,0 +1 @@
+tests/qapi-schema/enum-wrong-data.json:2: Enum 'MyEnum' requires an array for 'data'

diff --git a/tests/qapi-schema/enum-wrong-data.exit b/tests/qapi-schema/enum-wrong-data.exit
index 573541ac9702dd3969c9bc859d2b91ec1f7e6e56..d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 100644 (file)
--- a/tests/qapi-schema/enum-wrong-data.exit
+++ b/tests/qapi-schema/enum-wrong-data.exit
@@ -1 +1 @@
-0
+1

diff --git a/tests/qapi-schema/enum-wrong-data.json b/tests/qapi-schema/enum-wrong-data.json
index 61d25ec4270d6bb9b8c87cbb0c5fb2b1f794133b..7b3e255c146e1a5d6d5572afb5e5e7a3b9cb13d4 100644 (file)
--- a/tests/qapi-schema/enum-wrong-data.json
+++ b/tests/qapi-schema/enum-wrong-data.json
@@ -1,2 +1,2 @@
-# FIXME: we should require that all qapi enums have an array for data
+# we require that all qapi enums have an array for data
 { 'enum': 'MyEnum', 'data': { 'value': 'str' } }

diff --git a/tests/qapi-schema/enum-wrong-data.out b/tests/qapi-schema/enum-wrong-data.out
index 28d22116d0c9fbeef16c9332c9be1c32976a78f7..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 100644 (file)
--- a/tests/qapi-schema/enum-wrong-data.out
+++ b/tests/qapi-schema/enum-wrong-data.out
@@ -1,3 +0,0 @@
-[OrderedDict([('enum', 'MyEnum'), ('data', OrderedDict([('value', 'str')]))])]
-[{'enum_name': 'MyEnum', 'enum_values': OrderedDict([('value', 'str')])}]
-[]