Re: [PATCH 08/14] qapi: Fix to reject 'data': 'mumble' in struct

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:19AM +0100, Markus Armbruster wrote:
> 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, and add a test case.

Nice catch; I see why the split into three functions earlier on
foreshadowed some subtle bug fixes to come.

> +++ 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:

At first I thought this was a straight rename...

>      """
>      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):

...until I got here and saw that you kept the original name, and added
a new helper.  Worth calling out the new function name
check_type_implicit() in the commit message?  It would have spared me
a minute.

As earlier, you lost the doc comment.  I'll leave it to your
discretion if it is important to copy one back in.

> +        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'")
>

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org