qapi: Require valid names

Previous commits demonstrated that the generator overlooked various
bad naming situations:
- types, commands, and events need a valid name
- enum members must be valid names, when combined with prefix
- union and alternate branches cannot be marked optional

Valid upstream names match [a-zA-Z][a-zA-Z0-9_-]*; valid downstream
names match __[a-zA-Z][a-zA-Z0-9._-]*.  Enumerations match the
weaker [a-zA-Z0-9._-]+ (in part thanks to QKeyCode picking an enum
that starts with a digit, which we can't change now due to
backwards compatibility).  Rather than call out three separate
regex, this patch just uses a broader combination that allows both
upstream and downstream names, as well as a small hack that
realizes that any enum name is merely a suffix to an already valid
name prefix (that is, any enum name is valid if prepending _ fits
the normal rules).

We could reject new enumeration names beginning with a digit by
whitelisting existing exceptions.  We could also be stricter
about the distinction between upstream names (no leading
underscore, no use of dot) and downstream (mandatory leading
double underscore), but it is probably not worth the bother.

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 3c33e4e46c0aa536862ed508c8b1ffeae3f67bdc..5bc32e311d844d8f5e1af4bba039dbec76ffa937 100644 (file)
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -276,8 +276,31 @@ def discriminator_find_enum_define(expr):
 
     return find_enum(discriminator_type)
 
+valid_name = re.compile('^[a-zA-Z_][a-zA-Z0-9_.-]*$')
+def check_name(expr_info, source, name, allow_optional = False,
+               enum_member = False):
+    global valid_name
+    membername = name
+
+    if not isinstance(name, str):
+        raise QAPIExprError(expr_info,
+                            "%s requires a string name" % source)
+    if name.startswith('*'):
+        membername = name[1:]
+        if not allow_optional:
+            raise QAPIExprError(expr_info,
+                                "%s does not allow optional name '%s'"
+                                % (source, name))
+    # Enum members can start with a digit, because the generated C
+    # code always prefixes it with the enum name
+    if enum_member:
+        membername = '_' + membername
+    if not valid_name.match(membername):
+        raise QAPIExprError(expr_info,
+                            "%s uses invalid name '%s'" % (source, name))
+
 def check_type(expr_info, source, value, allow_array = False,
-               allow_dict = False, allow_metas = []):
+               allow_dict = False, allow_optional = False, allow_metas = []):
     global all_names
     orig_value = value
 
@@ -319,18 +342,21 @@ def check_type(expr_info, source, value, allow_array = False,
         raise QAPIExprError(expr_info,
                             "%s should be a type name" % source)
     for (key, arg) in value.items():
+        check_name(expr_info, "Member of %s" % source, key,
+                   allow_optional=allow_optional)
         check_type(expr_info, "Member '%s' of %s" % (key, source), arg,
-                   allow_array=True, allow_dict=True,
+                   allow_array=True, allow_dict=True, allow_optional=True,
                    allow_metas=['built-in', 'union', 'alternate', 'struct',
                                 'enum'])
 
 def check_command(expr, expr_info):
     name = expr['command']
     check_type(expr_info, "'data' for command '%s'" % name,
-               expr.get('data'), allow_dict=True,
+               expr.get('data'), allow_dict=True, allow_optional=True,
                allow_metas=['union', 'struct'])
     check_type(expr_info, "'returns' for command '%s'" % name,
                expr.get('returns'), allow_array=True, allow_dict=True,
+               allow_optional=True,
                allow_metas=['built-in', 'union', 'alternate', 'struct',
                             'enum'])
 
@@ -343,7 +369,7 @@ def check_event(expr, expr_info):
         raise QAPIExprError(expr_info, "Event name 'MAX' cannot be created")
     events.append(name)
     check_type(expr_info, "'data' for event '%s'" % name,
-               expr.get('data'), allow_dict=True,
+               expr.get('data'), allow_dict=True, allow_optional=True,
                allow_metas=['union', 'struct'])
     if params:
         for argname, argentry, optional, structured in parse_args(params):
@@ -392,12 +418,10 @@ def check_union(expr, expr_info):
                                 "Base '%s' is not a valid 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' discriminator must be a string"
-                                % 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,
@@ -414,6 +438,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; furthermore, in flat unions,
         # branches must be a struct
         check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
@@ -445,6 +471,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:
@@ -475,10 +503,8 @@ def check_enum(expr, expr_info):
         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))
+        check_name(expr_info, "Member of enum '%s'" %name, member,
+                   enum_member=True)
         key = _generate_enum_string(member)
         if key in values:
             raise QAPIExprError(expr_info,
@@ -491,7 +517,7 @@ def check_struct(expr, expr_info):
     members = expr['data']
 
     check_type(expr_info, "'data' for type '%s'" % name, members,
-               allow_dict=True)
+               allow_dict=True, allow_optional=True)
     check_type(expr_info, "'base' for type '%s'" % name, expr.get('base'),
                allow_metas=['struct'])
 
@@ -682,8 +708,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"
@@ -697,7 +726,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 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..42b490ce0b26e1f9cabe56012481fb1e0eb0b18b 100644 (file)
--- 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 573541ac9702dd3969c9bc859d2b91ec1f7e6e56..d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 100644 (file)
--- 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 f139110e89e46fd6c9db8a6a55da4ff3519f4f48..da949e8903554f1aa4232ba4e2977e0f0a095e89 100644 (file)
--- a/tests/qapi-schema/bad-ident.json
+++ b/tests/qapi-schema/bad-ident.json
@@ -1,2 +1,2 @@
-# 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 165e34645d7e5b0c8406c8e8f11f7af37e00e0fc..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 100644 (file)
--- 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/enum-bad-name.err b/tests/qapi-schema/enum-bad-name.err
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..9c3c1002b782c8f5d7ac6252cfe3b76bafdb3174 100644 (file)
--- a/tests/qapi-schema/enum-bad-name.err
+++ b/tests/qapi-schema/enum-bad-name.err
@@ -0,0 +1 @@
+tests/qapi-schema/enum-bad-name.json:2: Member of enum 'MyEnum' uses invalid name 'not^possible'

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

diff --git a/tests/qapi-schema/enum-bad-name.json b/tests/qapi-schema/enum-bad-name.json
index 0c32448a3eed53161050cfaa5260ecabe5594efd..8506562b313fdc9d2e9e5651230b3f81555e3788 100644 (file)
--- a/tests/qapi-schema/enum-bad-name.json
+++ b/tests/qapi-schema/enum-bad-name.json
@@ -1,2 +1,2 @@
-# FIXME: we should ensure all enum names can map to C
+# we ensure all enum names can map to C
 { 'enum': 'MyEnum', 'data': [ 'not^possible' ] }

diff --git a/tests/qapi-schema/enum-bad-name.out b/tests/qapi-schema/enum-bad-name.out
index d24ea49a0f6a981a6069318885c3ce5ff876ddc2..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 100644 (file)
--- a/tests/qapi-schema/enum-bad-name.out
+++ b/tests/qapi-schema/enum-bad-name.out
@@ -1,3 +0,0 @@
-[OrderedDict([('enum', 'MyEnum'), ('data', ['not^possible'])])]
-[{'enum_name': 'MyEnum', 'enum_values': ['not^possible']}]
-[]

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

diff --git a/tests/qapi-schema/flat-union-bad-discriminator.err b/tests/qapi-schema/flat-union-bad-discriminator.err
index 507e2bab4a1fe4cb3d4385df4b470cc424717bb1..c38cc8e4dfd6ced3d9a73ed40f86782b01e813b2 100644 (file)
--- 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' discriminator must be a string
+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 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..aaabedb3bd628078b64e8ad72c744c8b24e65ca9 100644 (file)
--- 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:6: 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 573541ac9702dd3969c9bc859d2b91ec1f7e6e56..d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 100644 (file)
--- 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 ece0d31fb3a7b16489f91cbcba2607e09be3cfaf..25ce0e661247aa55aaac9a045e4d1c3961ad2f91 100644 (file)
--- 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 bb7db0090208a51dcd4c7e2423e0415aa47081d0..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 100644 (file)
--- a/tests/qapi-schema/flat-union-optional-discriminator.out
+++ b/tests/qapi-schema/flat-union-optional-discriminator.out
@@ -1,7 +0,0 @@
-[OrderedDict([('enum', 'Enum'), ('data', ['one', 'two'])]),
- OrderedDict([('type', 'Base'), ('data', OrderedDict([('*switch', 'Enum')]))]),
- OrderedDict([('type', 'Branch'), ('data', OrderedDict([('name', 'str')]))]),
- OrderedDict([('union', 'MyUnion'), ('base', 'Base'), ('discriminator', '*switch'), ('data', OrderedDict([('one', 'Branch'), ('two', 'Branch')]))])]
-[{'enum_name': 'Enum', 'enum_values': ['one', 'two']}]
-[OrderedDict([('type', 'Base'), ('data', OrderedDict([('*switch', 'Enum')]))]),
- OrderedDict([('type', 'Branch'), ('data', OrderedDict([('name', 'str')]))])]

diff --git a/tests/qapi-schema/union-optional-branch.err b/tests/qapi-schema/union-optional-branch.err
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..3ada1334dc6317df276ad28648941c29f3826563 100644 (file)
--- 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 573541ac9702dd3969c9bc859d2b91ec1f7e6e56..d00491fd7e5bb6fa28c517a0bb32b8b506539d4d 100644 (file)
--- 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 c513db72d4b8442164ec936e0cbd702fea43b0ce..591615fc689bb8757c8e99f1cf76f6ad8c95063b 100644 (file)
--- 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 b03b5d2b4f45c2194cb81b4894bb0a36f0d795ce..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 100644 (file)
--- 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}]
-[]