[Qemu-devel] [PATCH v5 21/28] qapi: Require valid names

[Eric Blake]

Previous commits demonstrated that the generator overlooked various
bad naming situations:
- types, commands, and events need a valid name
- union and alternate branches cannot be marked optional

The set of valid names includes [a-zA-Z0-9._-] (where '.' is
useful only in downstream extensions).

Signed-off-by: Eric Blake <address@hidden>
---
 scripts/qapi.py                                    | 57 ++++++++++++++++------
 tests/qapi-schema/bad-ident.err                    |  1 +
 tests/qapi-schema/bad-ident.exit                   |  2 +-
 tests/qapi-schema/bad-ident.json                   |  2 +-
 tests/qapi-schema/bad-ident.out                    |  3 --
 tests/qapi-schema/flat-union-bad-discriminator.err |  2 +-
 .../flat-union-optional-discriminator.err          |  1 +
 .../flat-union-optional-discriminator.exit         |  2 +-
 .../flat-union-optional-discriminator.json         |  2 +-
 .../flat-union-optional-discriminator.out          |  5 --
 tests/qapi-schema/union-optional-branch.err        |  1 +
 tests/qapi-schema/union-optional-branch.exit       |  2 +-
 tests/qapi-schema/union-optional-branch.json       |  2 +-
 tests/qapi-schema/union-optional-branch.out        |  3 --
 14 files changed, 53 insertions(+), 32 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index c42683b..ed5385a 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -15,6 +15,7 @@ import re
 from ordereddict import OrderedDict
 import os
 import sys
+import string

 builtin_types = {
     'str':      'QTYPE_QSTRING',
@@ -276,8 +277,27 @@ def discriminator_find_enum_define(expr):

     return find_enum(discriminator_type)

+valid_characters = set(string.ascii_letters + string.digits + '.' + '-' + '_')
+def check_name(expr_info, source, name, allow_optional = False):
+    membername = name
+
+    if not isinstance(name, str):
+        raise QAPIExprError(expr_info,
+                            "%s requires a string name" % source)
+    if name == '**':
+        return
+    if name.startswith('*'):
+        membername = name[1:]
+        if not allow_optional:
+            raise QAPIExprError(expr_info,
+                                "%s does not allow optional name '%s'"
+                                % (source, name))
+    if not set(membername) <= valid_characters:
+        raise QAPIExprError(expr_info,
+                            "%s uses invalid name '%s'" % (source, name))
+
 def check_type(expr_info, source, data, allow_array = False,
-               allowed_metas = [], allow_dict = True):
+               allowed_metas = [], allow_dict = True, allow_optional = False):
     global all_names

     if data is None:
@@ -317,21 +337,23 @@ def check_type(expr_info, source, data, allow_array = 
False,
         raise QAPIExprError(expr_info,
                             "%s should be a type name" % source)
     for (key, value) in data.items():
+        check_name(expr_info, "Member of %s" % source, key,
+                   allow_optional=allow_optional)
         check_type(expr_info, "Member '%s' of %s" % (key, source), value,
                    allow_array=True,
                    allowed_metas=['built-in', 'union', 'alternate', 'struct',
                                   'enum'],
-                   allow_dict=True)
+                   allow_dict=True, allow_optional=True)

 def check_command(expr, expr_info):
     name = expr['command']
     check_type(expr_info, "'data' for command '%s'" % name,
                expr.get('data'),
-               allowed_metas=['union', 'struct'])
+               allowed_metas=['union', 'struct'], allow_optional=True)
     check_type(expr_info, "'returns' for command '%s'" % name,
                expr.get('returns'), allow_array=True,
                allowed_metas=['built-in', 'union', 'alternate', 'struct',
-                              'enum'])
+                              'enum'], allow_optional=True)

 def check_event(expr, expr_info):
     global events
@@ -345,7 +367,8 @@ def check_event(expr, expr_info):
                             % name)
     events.append(name)
     check_type(expr_info, "'data' for event '%s'" % name,
-               expr.get('data'), allowed_metas=['union', 'struct'])
+               expr.get('data'), allowed_metas=['union', 'struct'],
+               allow_optional=True)
     if params:
         for argname, argentry, optional, structured in parse_args(params):
             if structured:
@@ -385,12 +408,10 @@ def check_union(expr, expr_info):
                                 "Base '%s' is not a valid base type"
                                 % base)

-        # The value of member 'discriminator' must name a member of the
-        # base type.
-        if not isinstance(discriminator, str):
-            raise QAPIExprError(expr_info,
-                                "Flat union '%s' must have a string "
-                                "discriminator field" % name)
+        # The value of member 'discriminator' must name a non-optional
+        # member of the base type.
+        check_name(expr_info, "Discriminator of flat union '%s'" % name,
+                   discriminator)
         discriminator_type = base_fields.get(discriminator)
         if not discriminator_type:
             raise QAPIExprError(expr_info,
@@ -406,6 +427,8 @@ def check_union(expr, expr_info):

     # Check every branch
     for (key, value) in members.items():
+        check_name(expr_info, "Member of union '%s'" % name, key)
+
         # Each value must name a known type
         check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
                    value, allow_array=True,
@@ -439,6 +462,8 @@ def check_alternate(expr, expr_info):

     # Check every branch
     for (key, value) in members.items():
+        check_name(expr_info, "Member of alternate '%s'" % name, key)
+
         # Check for conflicts in the generated enum
         c_key = _generate_enum_string(key)
         if c_key in values:
@@ -485,7 +510,8 @@ def check_struct(expr, expr_info):
     name = expr['type']
     members = expr['data']

-    check_type(expr_info, "'data' for type '%s'" % name, members)
+    check_type(expr_info, "'data' for type '%s'" % name, members,
+               allow_optional=True)
     check_type(expr_info, "'base' for type '%s'" % name, expr.get('base'),
                allowed_metas=['struct'], allow_dict=False)

@@ -676,8 +702,11 @@ def type_name(name):
         return c_list_type(name[0])
     return name

-def add_name(name, info, meta, implicit = False):
+def add_name(name, info, meta, implicit = False, source = None):
     global all_names
+    if not source:
+        source = "'%s'" % meta
+    check_name(info, source, name)
     if name in all_names:
         raise QAPIExprError(info,
                             "%s '%s' is already defined"
@@ -691,7 +720,7 @@ def add_name(name, info, meta, implicit = False):
 def add_struct(definition, info):
     global struct_types
     name = definition['type']
-    add_name(name, info, 'struct')
+    add_name(name, info, 'struct', source="'type'")
     struct_types.append(definition)

 def find_struct(name):
diff --git a/tests/qapi-schema/bad-ident.err b/tests/qapi-schema/bad-ident.err
index e69de29..42b490c 100644
--- a/tests/qapi-schema/bad-ident.err
+++ b/tests/qapi-schema/bad-ident.err
@@ -0,0 +1 @@
+tests/qapi-schema/bad-ident.json:2: 'type' does not allow optional name '*oops'
diff --git a/tests/qapi-schema/bad-ident.exit b/tests/qapi-schema/bad-ident.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/bad-ident.exit
+++ b/tests/qapi-schema/bad-ident.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/bad-ident.json b/tests/qapi-schema/bad-ident.json
index 66333a7..7418364 100644
--- a/tests/qapi-schema/bad-ident.json
+++ b/tests/qapi-schema/bad-ident.json
@@ -1,3 +1,3 @@
-# FIXME: we should reject creating a type name with bad name
+# we reject creating a type name with bad name
 { 'type': '*oops', 'data': { 'i': 'int' } }

diff --git a/tests/qapi-schema/bad-ident.out b/tests/qapi-schema/bad-ident.out
index 165e346..e69de29 100644
--- a/tests/qapi-schema/bad-ident.out
+++ b/tests/qapi-schema/bad-ident.out
@@ -1,3 +0,0 @@
-[OrderedDict([('type', '*oops'), ('data', OrderedDict([('i', 'int')]))])]
-[]
-[OrderedDict([('type', '*oops'), ('data', OrderedDict([('i', 'int')]))])]
diff --git a/tests/qapi-schema/flat-union-bad-discriminator.err 
b/tests/qapi-schema/flat-union-bad-discriminator.err
index 4f0c798..c38cc8e 100644
--- a/tests/qapi-schema/flat-union-bad-discriminator.err
+++ b/tests/qapi-schema/flat-union-bad-discriminator.err
@@ -1 +1 @@
-tests/qapi-schema/flat-union-bad-discriminator.json:11: Flat union 'TestUnion' 
must have a string discriminator field
+tests/qapi-schema/flat-union-bad-discriminator.json:11: Discriminator of flat 
union 'TestUnion' requires a string name
diff --git a/tests/qapi-schema/flat-union-optional-discriminator.err 
b/tests/qapi-schema/flat-union-optional-discriminator.err
index e69de29..df290ce 100644
--- a/tests/qapi-schema/flat-union-optional-discriminator.err
+++ b/tests/qapi-schema/flat-union-optional-discriminator.err
@@ -0,0 +1 @@
+tests/qapi-schema/flat-union-optional-discriminator.json:5: Discriminator of 
flat union 'MyUnion' does not allow optional name '*switch'
diff --git a/tests/qapi-schema/flat-union-optional-discriminator.exit 
b/tests/qapi-schema/flat-union-optional-discriminator.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/flat-union-optional-discriminator.exit
+++ b/tests/qapi-schema/flat-union-optional-discriminator.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/flat-union-optional-discriminator.json 
b/tests/qapi-schema/flat-union-optional-discriminator.json
index 4957c72..cbf0afa 100644
--- a/tests/qapi-schema/flat-union-optional-discriminator.json
+++ b/tests/qapi-schema/flat-union-optional-discriminator.json
@@ -1,4 +1,4 @@
-# FIXME: we should require the discriminator to be non-optional
+# we require the discriminator to be non-optional
 { 'enum': 'Enum', 'data': [ 'one', 'two' ] }
 { 'type': 'Base',
   'data': { '*switch': 'Enum' } }
diff --git a/tests/qapi-schema/flat-union-optional-discriminator.out 
b/tests/qapi-schema/flat-union-optional-discriminator.out
index f4b6bed..e69de29 100644
--- a/tests/qapi-schema/flat-union-optional-discriminator.out
+++ b/tests/qapi-schema/flat-union-optional-discriminator.out
@@ -1,5 +0,0 @@
-[OrderedDict([('enum', 'Enum'), ('data', ['one', 'two'])]),
- OrderedDict([('type', 'Base'), ('data', OrderedDict([('*switch', 'Enum')]))]),
- OrderedDict([('union', 'MyUnion'), ('base', 'Base'), ('discriminator', 
'*switch'), ('data', OrderedDict([('one', 'int'), ('two', 'str')]))])]
-[{'enum_name': 'Enum', 'enum_values': ['one', 'two']}]
-[OrderedDict([('type', 'Base'), ('data', OrderedDict([('*switch', 'Enum')]))])]
diff --git a/tests/qapi-schema/union-optional-branch.err 
b/tests/qapi-schema/union-optional-branch.err
index e69de29..3ada133 100644
--- a/tests/qapi-schema/union-optional-branch.err
+++ b/tests/qapi-schema/union-optional-branch.err
@@ -0,0 +1 @@
+tests/qapi-schema/union-optional-branch.json:2: Member of union 'Union' does 
not allow optional name '*a'
diff --git a/tests/qapi-schema/union-optional-branch.exit 
b/tests/qapi-schema/union-optional-branch.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/union-optional-branch.exit
+++ b/tests/qapi-schema/union-optional-branch.exit
@@ -1 +1 @@
-0
+1
diff --git a/tests/qapi-schema/union-optional-branch.json 
b/tests/qapi-schema/union-optional-branch.json
index c513db7..591615f 100644
--- a/tests/qapi-schema/union-optional-branch.json
+++ b/tests/qapi-schema/union-optional-branch.json
@@ -1,2 +1,2 @@
-# FIXME: union branches cannot be optional
+# union branches cannot be optional
 { 'union': 'Union', 'data': { '*a': 'int', 'b': 'str' } }
diff --git a/tests/qapi-schema/union-optional-branch.out 
b/tests/qapi-schema/union-optional-branch.out
index b03b5d2..e69de29 100644
--- a/tests/qapi-schema/union-optional-branch.out
+++ b/tests/qapi-schema/union-optional-branch.out
@@ -1,3 +0,0 @@
-[OrderedDict([('union', 'Union'), ('data', OrderedDict([('*a', 'int'), ('b', 
'str')]))])]
-[{'enum_name': 'UnionKind', 'enum_values': None}]
-[]
-- 
2.1.0