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
> of them is a copy of the original specialized to a certain set of
> flags.
>
> Signed-off-by: Markus Armbruster <arm...@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 <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
