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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> 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.

Can do.

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

I didn't duplicate the doc string, which means it moves from
check_type_name_or_implicit() to check_type_implicit(), where the actual
meat is.

John, you added the doc string in commit a48653638fa (qapi/expr.py: Add
docstrings).  Do you have an opinion?

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

Thanks!