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_implicit(). Each
of them is a copy of the original specialized to a certain set of
flags.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
scripts/qapi/expr.py | 116 +++++++++++++++++++++++++------------------
1 file changed, 67 insertions(+), 49 deletions(-)
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``.
-
- Python types of ``str`` or ``None`` are always allowed.
-
- :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.
-
- :raise QAPISemError: When ``value`` fails validation.
- :return: None, ``value`` is normalized in-place as needed.
- """
+def check_type_name(value: Optional[object],
+ info: QAPISourceInfo, source: str) -> None:
+ if value is None:
+ return
+
+ 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)
+
+
+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)
- if not allow_dict:
- 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.
+
+ 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]:
--
2.39.2On Thu, Mar 16, 2023 at 08:13:15AM +0100, Markus Armbruster wrote:
> 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_implicit(). Each
You omitted check_type_name_or_array() in this summary
> of them is a copy of the original specialized to a certain set of
> flags.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> scripts/qapi/expr.py | 116 +++++++++++++++++++++++++------------------
> 1 file changed, 67 insertions(+), 49 deletions(-)
>
> 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:
There are few enough callers to see that they do indeed have exactly
one of (nearly) three call patterns.
> - """
> - Normalize and validate the QAPI type of ``value``.
> -
> - Python types of ``str`` or ``None`` are always allowed.
> -
> - :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.
> -
> - :raise QAPISemError: When ``value`` fails validation.
> - :return: None, ``value`` is normalized in-place as needed.
> - """
> +def check_type_name(value: Optional[object],
> + info: QAPISourceInfo, source: str) -> None:
check_type_name() replaces callers that relied on the default for
allow_array and allow_dict
> + if value is None:
Loses out on the documentation. Not sure how much that matters to
you?
> + return
> +
> + 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)
> +
> +
> +def check_type_name_or_array(value: Optional[object],
> + info: QAPISourceInfo, source: str) -> None:
check_type_name_or_array() replaces all callers that passed
allow_array=True.
> if value is None:
Another copy without documentation.
> 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)
>
> - if not allow_dict:
> - 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:
And check_type_name_or_implicit replaces all callers that passed
allow_dict=str, where str is now the parent_name. (Wow, that was an
odd overload of the parameter name - I like the split version better).
...
> @@ -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)
And this use of allow_dict was the weirdest, where it really does fit
better as calls into two separate functions.
With the fixed commit message, and with or without more function docs,
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Eric Blake <eblake@redhat.com> writes:
> On Thu, Mar 16, 2023 at 08:13:15AM +0100, Markus Armbruster wrote:
>> 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_implicit(). Each
>
> You omitted check_type_name_or_array() in this summary
Oops!
>> of them is a copy of the original specialized to a certain set of
>> flags.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> scripts/qapi/expr.py | 116 +++++++++++++++++++++++++------------------
>> 1 file changed, 67 insertions(+), 49 deletions(-)
>
>>
>> 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:
>
> There are few enough callers to see that they do indeed have exactly
> one of (nearly) three call patterns.
>
>> - """
>> - Normalize and validate the QAPI type of ``value``.
>> -
>> - Python types of ``str`` or ``None`` are always allowed.
>> -
>> - :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.
>> -
>> - :raise QAPISemError: When ``value`` fails validation.
>> - :return: None, ``value`` is normalized in-place as needed.
>> - """
>> +def check_type_name(value: Optional[object],
>> + info: QAPISourceInfo, source: str) -> None:
>
> check_type_name() replaces callers that relied on the default for
> allow_array and allow_dict
Yes.
>> + if value is None:
>
> Loses out on the documentation. Not sure how much that matters to
> you?
You mean the doc string?
I could copy and specialize it along with the code, but the new function
is so simple... not sure it's worth explaining.
>> + return
>> +
>> + 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)
>> +
>> +
>> +def check_type_name_or_array(value: Optional[object],
>> + info: QAPISourceInfo, source: str) -> None:
>
> check_type_name_or_array() replaces all callers that passed
> allow_array=True.
Yes.
>> if value is None:
>
> Another copy without documentation.
Same doubts.
>> 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)
>>
>> - if not allow_dict:
>> - 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:
>
> And check_type_name_or_implicit replaces all callers that passed
> allow_dict=str, where str is now the parent_name.
Yes.
> (Wow, that was an
> odd overload of the parameter name - I like the split version better).
It was less bad than what it replaced :)
Commit 638c4af9310 (qapi: Clean up member name case checking)
> ...
>> @@ -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)
>
> And this use of allow_dict was the weirdest, where it really does fit
> better as calls into two separate functions.
>
> With the fixed commit message, and with or without more function docs,
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Thanks!
© 2016 - 2025 Red Hat, Inc.