From 607045ba39f6ca845ede3131a902ad785088fea3 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 16 Mar 2023 08:13:14 +0100 Subject: qapi: Clean up after removal of simple unions Commit 4e99f4b12c0 (qapi: Drop simple unions) missed a bit of code dealing with simple union branches. Drop it. Signed-off-by: Markus Armbruster Message-Id: <20230316071325.492471-4-armbru@redhat.com> Reviewed-by: Eric Blake --- scripts/qapi/expr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'scripts/qapi/expr.py') diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index ca01ea6f4a..59bdd86024 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -518,7 +518,7 @@ def check_union(expr: QAPIExpression) -> None: source = "'data' member '%s'" % key check_keys(value, info, source, ['type'], ['if']) check_if(value, info, source) - check_type(value['type'], info, source, allow_array=not base) + check_type(value['type'], info, source) def check_alternate(expr: QAPIExpression) -> None: -- cgit 1.4.1 From 06cc46eeaf3cf5790c85ebbb58e8875719e5eb86 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 16 Mar 2023 08:13:15 +0100 Subject: qapi: Split up check_type() check_type() can check type names, arrays, and implicit struct types. Callers pass flags to select from this menu. This makes the function somewhat hard to read. Moreover, a few minor bugs are hiding in there, as we'll see shortly. Split it into check_type_name(), check_type_name_or_array(), and check_type_name_or_implicit(). Each of them is a copy of the original specialized to a certain set of flags. Signed-off-by: Markus Armbruster Message-Id: <20230316071325.492471-5-armbru@redhat.com> Reviewed-by: Eric Blake [Commit message corrected] --- scripts/qapi/expr.py | 110 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 64 insertions(+), 46 deletions(-) (limited to 'scripts/qapi/expr.py') diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 59bdd86024..bc04bf34c2 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -333,62 +333,74 @@ def normalize_members(members: object) -> None: members[key] = {'type': arg} -def check_type(value: Optional[object], - info: QAPISourceInfo, - source: str, - allow_array: bool = False, - allow_dict: Union[bool, str] = False) -> None: - """ - Normalize and validate the QAPI type of ``value``. +def check_type_name(value: Optional[object], + info: QAPISourceInfo, source: str) -> None: + if value is None: + return - Python types of ``str`` or ``None`` are always allowed. + if isinstance(value, str): + return - :param value: The value to check. - :param info: QAPI schema source file information. - :param source: Error string describing this ``value``. - :param allow_array: - Allow a ``List[str]`` of length 1, which indicates an array of - the type named by the list element. - :param allow_dict: - Allow a dict. Its members can be struct type members or union - branches. When the value of ``allow_dict`` is in pragma - ``member-name-exceptions``, the dict's keys may violate the - member naming rules. The dict members are normalized in place. + if isinstance(value, list): + raise QAPISemError(info, "%s cannot be an array" % source) - :raise QAPISemError: When ``value`` fails validation. - :return: None, ``value`` is normalized in-place as needed. - """ + raise QAPISemError(info, "%s should be a type name" % source) + + +def check_type_name_or_array(value: Optional[object], + info: QAPISourceInfo, source: str) -> None: if value is None: return - # Type name if isinstance(value, str): return - # Array type if isinstance(value, list): - if not allow_array: - raise QAPISemError(info, "%s cannot be an array" % source) if len(value) != 1 or not isinstance(value[0], str): raise QAPISemError(info, "%s: array type must contain single type name" % source) return - # Anonymous type + raise QAPISemError(info, + "%s should be a type name" % source) + + +def check_type_name_or_implicit(value: Optional[object], + info: QAPISourceInfo, source: str, + parent_name: Optional[str]) -> None: + """ + Normalize and validate an optional implicit struct type. - if not allow_dict: - raise QAPISemError(info, "%s should be a type name" % source) + Accept ``None``, ``str``, or a ``dict`` defining an implicit + struct type. The latter is normalized in place. + + :param value: The value to check. + :param info: QAPI schema source file information. + :param source: Error string describing this ``value``. + :param parent_name: + When the value of ``parent_name`` is in pragma + ``member-name-exceptions``, an implicit struct type may + violate the member naming rules. + + :raise QAPISemError: When ``value`` fails validation. + :return: None + """ + if value is None: + return + + if isinstance(value, str): + return + + if isinstance(value, list): + raise QAPISemError(info, "%s cannot be an array" % source) if not isinstance(value, dict): raise QAPISemError(info, "%s should be an object or type name" % source) - permissive = False - if isinstance(allow_dict, str): - permissive = allow_dict in info.pragma.member_name_exceptions + permissive = parent_name in info.pragma.member_name_exceptions - # value is a dictionary, check that each member is okay for (key, arg) in value.items(): key_source = "%s member '%s'" % (source, key) if key.startswith('*'): @@ -401,7 +413,7 @@ def check_type(value: Optional[object], check_keys(arg, info, key_source, ['type'], ['if', 'features']) check_if(arg, info, key_source) check_features(arg.get('features'), info) - check_type(arg['type'], info, key_source, allow_array=True) + check_type_name_or_array(arg['type'], info, key_source) def check_features(features: Optional[object], @@ -489,8 +501,8 @@ def check_struct(expr: QAPIExpression) -> None: name = cast(str, expr['struct']) # Checked in check_exprs members = expr['data'] - check_type(members, expr.info, "'data'", allow_dict=name) - check_type(expr.get('base'), expr.info, "'base'") + check_type_name_or_implicit(members, expr.info, "'data'", name) + check_type_name(expr.get('base'), expr.info, "'base'") def check_union(expr: QAPIExpression) -> None: @@ -508,7 +520,7 @@ def check_union(expr: QAPIExpression) -> None: members = expr['data'] info = expr.info - check_type(base, info, "'base'", allow_dict=name) + check_type_name_or_implicit(base, info, "'base'", name) check_name_is_str(discriminator, info, "'discriminator'") if not isinstance(members, dict): @@ -518,7 +530,7 @@ def check_union(expr: QAPIExpression) -> None: source = "'data' member '%s'" % key check_keys(value, info, source, ['type'], ['if']) check_if(value, info, source) - check_type(value['type'], info, source) + check_type_name(value['type'], info, source) def check_alternate(expr: QAPIExpression) -> None: @@ -544,7 +556,7 @@ def check_alternate(expr: QAPIExpression) -> None: check_name_lower(key, info, source) check_keys(value, info, source, ['type'], ['if']) check_if(value, info, source) - check_type(value['type'], info, source, allow_array=True) + check_type_name_or_array(value['type'], info, source) def check_command(expr: QAPIExpression) -> None: @@ -560,10 +572,13 @@ def check_command(expr: QAPIExpression) -> None: rets = expr.get('returns') boxed = expr.get('boxed', False) - if boxed and args is None: - raise QAPISemError(expr.info, "'boxed': true requires 'data'") - check_type(args, expr.info, "'data'", allow_dict=not boxed) - check_type(rets, expr.info, "'returns'", allow_array=True) + if boxed: + if args is None: + raise QAPISemError(expr.info, "'boxed': true requires 'data'") + check_type_name(args, expr.info, "'data'") + else: + check_type_name_or_implicit(args, expr.info, "'data'", None) + check_type_name_or_array(rets, expr.info, "'returns'") def check_event(expr: QAPIExpression) -> None: @@ -578,9 +593,12 @@ def check_event(expr: QAPIExpression) -> None: args = expr.get('data') boxed = expr.get('boxed', False) - if boxed and args is None: - raise QAPISemError(expr.info, "'boxed': true requires 'data'") - check_type(args, expr.info, "'data'", allow_dict=not boxed) + if boxed: + if args is None: + raise QAPISemError(expr.info, "'boxed': true requires 'data'") + check_type_name(args, expr.info, "'data'") + else: + check_type_name_or_implicit(args, expr.info, "'data'", None) def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]: -- cgit 1.4.1 From 2a0c975f86a24f18b90fc4d65fe8984253fd4562 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 16 Mar 2023 08:13:16 +0100 Subject: qapi: Improve error message for unexpected array types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We reject array types in certain places with "cannot be an array". Deleting this check improves the error message to "should be a type name" or "should be an object or type name", depending on context, so do that. Signed-off-by: Markus Armbruster Message-Id: <20230316071325.492471-6-armbru@redhat.com> Reviewed-by: Philippe Mathieu-Daudé --- scripts/qapi/expr.py | 6 ------ tests/qapi-schema/bad-data.err | 2 +- tests/qapi-schema/union-array-branch.err | 2 +- 3 files changed, 2 insertions(+), 8 deletions(-) (limited to 'scripts/qapi/expr.py') diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index bc04bf34c2..5abeaa19dd 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -341,9 +341,6 @@ def check_type_name(value: Optional[object], if isinstance(value, str): return - if isinstance(value, list): - raise QAPISemError(info, "%s cannot be an array" % source) - raise QAPISemError(info, "%s should be a type name" % source) @@ -392,9 +389,6 @@ def check_type_name_or_implicit(value: Optional[object], if isinstance(value, str): return - if isinstance(value, list): - raise QAPISemError(info, "%s cannot be an array" % source) - if not isinstance(value, dict): raise QAPISemError(info, "%s should be an object or type name" % source) diff --git a/tests/qapi-schema/bad-data.err b/tests/qapi-schema/bad-data.err index 7991c8898d..a987df4108 100644 --- a/tests/qapi-schema/bad-data.err +++ b/tests/qapi-schema/bad-data.err @@ -1,2 +1,2 @@ bad-data.json: In command 'oops': -bad-data.json:2: 'data' cannot be an array +bad-data.json:2: 'data' should be an object or type name diff --git a/tests/qapi-schema/union-array-branch.err b/tests/qapi-schema/union-array-branch.err index 5db9c17481..2aa146261a 100644 --- a/tests/qapi-schema/union-array-branch.err +++ b/tests/qapi-schema/union-array-branch.err @@ -1,2 +1,2 @@ union-array-branch.json: In union 'TestUnion': -union-array-branch.json:8: 'data' member 'value1' cannot be an array +union-array-branch.json:8: 'data' member 'value1' should be a type name -- cgit 1.4.1 From 7c4075190da24a01d9c02f5f59cf0651611bd40f Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 16 Mar 2023 08:13:17 +0100 Subject: qapi: Simplify code a bit after previous commits Signed-off-by: Markus Armbruster Message-Id: <20230316071325.492471-7-armbru@redhat.com> Reviewed-by: Eric Blake [Commit message corrected] --- scripts/qapi/expr.py | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) (limited to 'scripts/qapi/expr.py') diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 5abeaa19dd..8a8de9e3aa 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -335,21 +335,13 @@ def normalize_members(members: object) -> None: def check_type_name(value: Optional[object], info: QAPISourceInfo, source: str) -> None: - if value is None: - return - - if isinstance(value, str): - return - - raise QAPISemError(info, "%s should be a type name" % source) + if value is not None and not isinstance(value, str): + raise QAPISemError(info, "%s should be a type name" % source) def check_type_name_or_array(value: Optional[object], info: QAPISourceInfo, source: str) -> None: - if value is None: - return - - if isinstance(value, str): + if value is None or isinstance(value, str): return if isinstance(value, list): -- cgit 1.4.1 From 6f2ab6090de993988f7345e449852821ffc75f4e Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 16 Mar 2023 08:13:18 +0100 Subject: qapi: Fix error message when type name or array is expected We incorrectly report "FOO should be a type name" when it could also be an array. Fix that. Signed-off-by: Markus Armbruster Message-Id: <20230316071325.492471-8-armbru@redhat.com> Reviewed-by: Eric Blake --- scripts/qapi/expr.py | 15 +++++++-------- tests/qapi-schema/event-nest-struct.err | 2 +- tests/qapi-schema/nested-struct-data.err | 2 +- tests/qapi-schema/returns-dict.err | 2 +- tests/qapi-schema/struct-member-invalid.err | 2 +- 5 files changed, 11 insertions(+), 12 deletions(-) (limited to 'scripts/qapi/expr.py') diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 8a8de9e3aa..9bae500a7d 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -344,15 +344,14 @@ def check_type_name_or_array(value: Optional[object], if value is None or isinstance(value, str): return - if isinstance(value, list): - if len(value) != 1 or not isinstance(value[0], str): - raise QAPISemError(info, - "%s: array type must contain single type name" % - source) - return + if not isinstance(value, list): + raise QAPISemError(info, + "%s should be a type name or array" % source) - raise QAPISemError(info, - "%s should be a type name" % source) + if len(value) != 1 or not isinstance(value[0], str): + raise QAPISemError(info, + "%s: array type must contain single type name" % + source) def check_type_name_or_implicit(value: Optional[object], diff --git a/tests/qapi-schema/event-nest-struct.err b/tests/qapi-schema/event-nest-struct.err index 8c5f6ed311..15fc1406f8 100644 --- a/tests/qapi-schema/event-nest-struct.err +++ b/tests/qapi-schema/event-nest-struct.err @@ -1,2 +1,2 @@ event-nest-struct.json: In event 'EVENT_A': -event-nest-struct.json:1: 'data' member 'a' should be a type name +event-nest-struct.json:1: 'data' member 'a' should be a type name or array diff --git a/tests/qapi-schema/nested-struct-data.err b/tests/qapi-schema/nested-struct-data.err index c7258a0182..7dc5c7cb2d 100644 --- a/tests/qapi-schema/nested-struct-data.err +++ b/tests/qapi-schema/nested-struct-data.err @@ -1,2 +1,2 @@ nested-struct-data.json: In command 'foo': -nested-struct-data.json:2: 'data' member 'a' should be a type name +nested-struct-data.json:2: 'data' member 'a' should be a type name or array diff --git a/tests/qapi-schema/returns-dict.err b/tests/qapi-schema/returns-dict.err index 9b2d90c010..bf160e754b 100644 --- a/tests/qapi-schema/returns-dict.err +++ b/tests/qapi-schema/returns-dict.err @@ -1,2 +1,2 @@ returns-dict.json: In command 'oops': -returns-dict.json:2: 'returns' should be a type name +returns-dict.json:2: 'returns' should be a type name or array diff --git a/tests/qapi-schema/struct-member-invalid.err b/tests/qapi-schema/struct-member-invalid.err index 7e01a41d7c..3130d69d9f 100644 --- a/tests/qapi-schema/struct-member-invalid.err +++ b/tests/qapi-schema/struct-member-invalid.err @@ -1,2 +1,2 @@ struct-member-invalid.json: In struct 'Foo': -struct-member-invalid.json:1: 'data' member 'a' should be a type name +struct-member-invalid.json:1: 'data' member 'a' should be a type name or array -- cgit 1.4.1 From e2050ef633f77781e6b7b3aa04dd736e0ad825e1 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 16 Mar 2023 08:13:19 +0100 Subject: qapi: Fix to reject 'data': 'mumble' in struct A struct's 'data' must be a JSON object defining the struct's members. The QAPI code generator incorrectly accepts a JSON string instead, and then crashes in QAPISchema._make_members() called from ._def_struct_type(). Fix to reject it: factor check_type_implicit() out of check_type_name_or_implicit(), and switch check_struct() to use it instead. Also add a test case. Signed-off-by: Markus Armbruster Message-Id: <20230316071325.492471-9-armbru@redhat.com> Reviewed-by: Eric Blake [More detailed commit message] --- scripts/qapi/expr.py | 24 +++++++++++++++--------- tests/qapi-schema/meson.build | 1 + tests/qapi-schema/struct-data-typename.err | 2 ++ tests/qapi-schema/struct-data-typename.json | 2 ++ tests/qapi-schema/struct-data-typename.out | 0 5 files changed, 20 insertions(+), 9 deletions(-) create mode 100644 tests/qapi-schema/struct-data-typename.err create mode 100644 tests/qapi-schema/struct-data-typename.json create mode 100644 tests/qapi-schema/struct-data-typename.out (limited to 'scripts/qapi/expr.py') diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 9bae500a7d..cae0a08359 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -354,14 +354,14 @@ def check_type_name_or_array(value: Optional[object], source) -def check_type_name_or_implicit(value: Optional[object], - info: QAPISourceInfo, source: str, - parent_name: Optional[str]) -> None: +def check_type_implicit(value: Optional[object], + info: QAPISourceInfo, source: str, + parent_name: Optional[str]) -> None: """ Normalize and validate an optional implicit struct type. - Accept ``None``, ``str``, or a ``dict`` defining an implicit - struct type. The latter is normalized in place. + Accept ``None`` or a ``dict`` defining an implicit struct type. + The latter is normalized in place. :param value: The value to check. :param info: QAPI schema source file information. @@ -377,9 +377,6 @@ def check_type_name_or_implicit(value: Optional[object], if value is None: return - if isinstance(value, str): - return - if not isinstance(value, dict): raise QAPISemError(info, "%s should be an object or type name" % source) @@ -401,6 +398,15 @@ def check_type_name_or_implicit(value: Optional[object], check_type_name_or_array(arg['type'], info, key_source) +def check_type_name_or_implicit(value: Optional[object], + info: QAPISourceInfo, source: str, + parent_name: Optional[str]) -> None: + if value is None or isinstance(value, str): + return + + check_type_implicit(value, info, source, parent_name) + + def check_features(features: Optional[object], info: QAPISourceInfo) -> None: """ @@ -486,7 +492,7 @@ def check_struct(expr: QAPIExpression) -> None: name = cast(str, expr['struct']) # Checked in check_exprs members = expr['data'] - check_type_name_or_implicit(members, expr.info, "'data'", name) + check_type_implicit(members, expr.info, "'data'", name) check_type_name(expr.get('base'), expr.info, "'base'") diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build index d85b14f28c..f88110bddf 100644 --- a/tests/qapi-schema/meson.build +++ b/tests/qapi-schema/meson.build @@ -164,6 +164,7 @@ schemas = [ 'struct-base-clash-deep.json', 'struct-base-clash.json', 'struct-data-invalid.json', + 'struct-data-typename.json', 'struct-member-if-invalid.json', 'struct-member-invalid-dict.json', 'struct-member-invalid.json', diff --git a/tests/qapi-schema/struct-data-typename.err b/tests/qapi-schema/struct-data-typename.err new file mode 100644 index 0000000000..8fbfe99a42 --- /dev/null +++ b/tests/qapi-schema/struct-data-typename.err @@ -0,0 +1,2 @@ +struct-data-typename.json: In struct 'Stru2': +struct-data-typename.json:2: 'data' should be an object or type name diff --git a/tests/qapi-schema/struct-data-typename.json b/tests/qapi-schema/struct-data-typename.json new file mode 100644 index 0000000000..70fbad0ee4 --- /dev/null +++ b/tests/qapi-schema/struct-data-typename.json @@ -0,0 +1,2 @@ +{ 'struct': 'Stru1', 'data': {} } +{ 'struct': 'Stru2', 'data': 'Stru1' } diff --git a/tests/qapi-schema/struct-data-typename.out b/tests/qapi-schema/struct-data-typename.out new file mode 100644 index 0000000000..e69de29bb2 -- cgit 1.4.1