[PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments

[PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments

[Markus Armbruster]

PATCH 01-08 fix a few minor bugs I found on the way.  Could be a
separate series, but keeping them here seems simpler.

PATCH 09-12 improve tests for 'if' conditionals.

PATCH 13 fixes a code generation regression, and PATCH 14 rejects uses
conditionals that never worked and aren't worth fixing.

Markus Armbruster (14):
  qapi: Fix error message format regression
  qapi/schema: Use super()
  qapi: Clean up after removal of simple unions
  qapi: Split up check_type()
  qapi: Improve error message for unexpected array types
  qapi: Simplify code a bit after previous commit
  qapi: Fix error message when type name or array is expected
  qapi: Fix to reject 'data': 'mumble' in struct
  tests/qapi-schema: Improve union discriminator coverage
  tests/qapi-schema: Rename a few conditionals
  tests/qapi-schema: Clean up positive test for conditionals
  tests/qapi-schema: Cover optional conditional struct member
  qapi: Fix code generated for optional conditional struct member
  qapi: Require boxed for conditional command and event arguments

 docs/devel/qapi-code-gen.rst                  |   5 +-
 scripts/qapi/commands.py                      |   1 +
 scripts/qapi/expr.py                          | 115 ++++++++++--------
 scripts/qapi/gen.py                           |   1 +
 scripts/qapi/main.py                          |   2 +-
 scripts/qapi/schema.py                        |  16 ++-
 scripts/qapi/visit.py                         |   2 +
 tests/qapi-schema/args-if-implicit.err        |   2 +
 tests/qapi-schema/args-if-implicit.json       |   4 +
 tests/qapi-schema/args-if-implicit.out        |   0
 tests/qapi-schema/args-if-unboxed.err         |   2 +
 tests/qapi-schema/args-if-unboxed.json        |   6 +
 tests/qapi-schema/args-if-unboxed.out         |   0
 tests/qapi-schema/bad-data.err                |   2 +-
 tests/qapi-schema/event-args-if-unboxed.err   |   2 +
 tests/qapi-schema/event-args-if-unboxed.json  |   4 +
 tests/qapi-schema/event-args-if-unboxed.out   |   0
 tests/qapi-schema/event-nest-struct.err       |   2 +-
 tests/qapi-schema/meson.build                 |   3 +
 tests/qapi-schema/nested-struct-data.err      |   2 +-
 tests/qapi-schema/qapi-schema-test.json       |  20 +--
 tests/qapi-schema/qapi-schema-test.out        |  32 ++---
 tests/qapi-schema/returns-dict.err            |   2 +-
 tests/qapi-schema/struct-data-typename.err    |   2 +
 tests/qapi-schema/struct-data-typename.json   |   2 +
 tests/qapi-schema/struct-data-typename.out    |   0
 tests/qapi-schema/struct-member-invalid.err   |   2 +-
 tests/qapi-schema/union-array-branch.err      |   2 +-
 .../union-invalid-discriminator.err           |   2 +-
 .../union-invalid-discriminator.json          |   4 +-
 30 files changed, 141 insertions(+), 98 deletions(-)
 create mode 100644 tests/qapi-schema/args-if-implicit.err
 create mode 100644 tests/qapi-schema/args-if-implicit.json
 create mode 100644 tests/qapi-schema/args-if-implicit.out
 create mode 100644 tests/qapi-schema/args-if-unboxed.err
 create mode 100644 tests/qapi-schema/args-if-unboxed.json
 create mode 100644 tests/qapi-schema/args-if-unboxed.out
 create mode 100644 tests/qapi-schema/event-args-if-unboxed.err
 create mode 100644 tests/qapi-schema/event-args-if-unboxed.json
 create mode 100644 tests/qapi-schema/event-args-if-unboxed.out
 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

-- 
2.39.2

[PATCH 01/14] qapi: Fix error message format regression

[Markus Armbruster]

Commit 52a474180ae3 changed reporting of errors connected to a source
location without mentioning it in the commit message.  For instance,

    $ python scripts/qapi-gen.py tests/qapi-schema/unknown-escape.json
    tests/qapi-schema/unknown-escape.json:3:21: unknown escape \x

became

    scripts/qapi-gen.py: tests/qapi-schema/unknown-escape.json:3:21: unknown escape \x

This is not how compilers report such errors, and Emacs doesn't
recognize the format.  Revert this change.

Fixes: 52a474180ae3 (qapi-gen: Separate arg-parsing from generation)
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/main.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index fc216a53d3..316736b6a2 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -98,6 +98,6 @@ def main() -> int:
                  builtins=args.builtins,
                  gen_tracing=not args.suppress_tracing)
     except QAPIError as err:
-        print(f"{sys.argv[0]}: {str(err)}", file=sys.stderr)
+        print(err, file=sys.stderr)
         return 1
     return 0
-- 
2.39.2

Re: [PATCH 01/14] qapi: Fix error message format regression

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:12AM +0100, Markus Armbruster wrote:
> Commit 52a474180ae3 changed reporting of errors connected to a source
> location without mentioning it in the commit message.  For instance,
> 
>     $ python scripts/qapi-gen.py tests/qapi-schema/unknown-escape.json
>     tests/qapi-schema/unknown-escape.json:3:21: unknown escape \x
> 
> became
> 
>     scripts/qapi-gen.py: tests/qapi-schema/unknown-escape.json:3:21: unknown escape \x
> 
> This is not how compilers report such errors, and Emacs doesn't
> recognize the format.  Revert this change.
> 
> Fixes: 52a474180ae3 (qapi-gen: Separate arg-parsing from generation)
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi/main.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

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

[PATCH 02/14] qapi/schema: Use super()

[Markus Armbruster]

Commit 2cae67bcb5e (qapi: Use super() now we have Python 3) converted
the code to super().  Shortly after, commit f965e8fea6a (qapi: New
special feature flag "deprecated") neglected to use super().  Convert
it now.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/schema.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 207e4d71f3..719152fe49 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -259,7 +259,7 @@ def need_has_if_optional(self):
         return not self.c_type().endswith(POINTER_SUFFIX)
 
     def check(self, schema):
-        QAPISchemaEntity.check(self, schema)
+        super().check(schema)
         for feat in self.features:
             if feat.is_special():
                 raise QAPISemError(
-- 
2.39.2

Re: [PATCH 02/14] qapi/schema: Use super()

[Philippe Mathieu-Daudé]

On 16/3/23 08:13, Markus Armbruster wrote:
> Commit 2cae67bcb5e (qapi: Use super() now we have Python 3) converted
> the code to super().  Shortly after, commit f965e8fea6a (qapi: New
> special feature flag "deprecated") neglected to use super().  Convert
> it now.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   scripts/qapi/schema.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 03/14] qapi: Clean up after removal of simple unions

[Markus Armbruster]

Commit 4e99f4b12c0 (qapi: Drop simple unions) missed a bit of code
dealing with simple union branches.  Drop it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/expr.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

Re: [PATCH 03/14] qapi: Clean up after removal of simple unions

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:14AM +0100, Markus Armbruster wrote:
> Commit 4e99f4b12c0 (qapi: Drop simple unions) missed a bit of code
> dealing with simple union branches.  Drop it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

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

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

[PATCH 04/14] qapi: Split up check_type()

[Markus Armbruster]

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

Re: [PATCH 04/14] qapi: Split up check_type()

[Eric Blake]

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

Re: [PATCH 04/14] qapi: Split up check_type()

[Markus Armbruster]

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!

[PATCH 05/14] qapi: Improve error message for unexpected array types

[Markus Armbruster]

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 <armbru@redhat.com>
---
 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(-)

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

Re: [PATCH 05/14] qapi: Improve error message for unexpected array types

[Philippe Mathieu-Daudé]

On 16/3/23 08:13, Markus Armbruster wrote:
> 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 <armbru@redhat.com>
> ---
>   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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 06/14] qapi: Simplify code a bit after previous commit

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/expr.py | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

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

Re: [PATCH 06/14] qapi: Simplify code a bit after previous commit

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:17AM +0100, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Looks like 'previous commit' in the subject line actually means 4/14
(two commits ago); a victim of rebasing, I'm sure.

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

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

Re: [PATCH 06/14] qapi: Simplify code a bit after previous commit

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On Thu, Mar 16, 2023 at 08:13:17AM +0100, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Looks like 'previous commit' in the subject line actually means 4/14
> (two commits ago); a victim of rebasing, I'm sure.

Hmm, actually both commits matter.

The first hunk simplifies check_type_name() by contracting its two
conditionals.  It is enabled by the previous commit, which removed the
code between the two conditionals.

The second hunk simplifies check_type_name_or_array() the same way, but
that one has had nothing in between since PATCH 04.

I'll change the title to "after previous commits".

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

Thanks!

[PATCH 07/14] qapi: Fix error message when type name or array is expected

[Markus Armbruster]

We incorrectly report "FOO should be a type name" when it could also
be an array.  Fix that.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 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(-)

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

Re: [PATCH 07/14] qapi: Fix error message when type name or array is expected

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:18AM +0100, Markus Armbruster wrote:
> We incorrectly report "FOO should be a type name" when it could also
> be an array.  Fix that.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  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(-)
>

Doesn't change the set of schemas accepted, but does make it easier to
understand when a schema is rejected.

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

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

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

[Markus Armbruster]

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.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 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

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

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

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

[Markus Armbruster]

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!

[PATCH 09/14] tests/qapi-schema: Improve union discriminator coverage

[Markus Armbruster]

A union's 'discriminator' must name a one of the common members.
QAPISchemaVariants.check() looks it up by its c_name(), then checks
the name matches exactly (because c_name() is not injective).

Tests union-base-empty and union-invalid-discriminator both cover the
case where lookup fails.  Repurpose the latter to cover the case where
it succeeds and the name check fails.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qapi-schema/union-invalid-discriminator.err  | 2 +-
 tests/qapi-schema/union-invalid-discriminator.json | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qapi-schema/union-invalid-discriminator.err b/tests/qapi-schema/union-invalid-discriminator.err
index 38efb24b98..6bd774c156 100644
--- a/tests/qapi-schema/union-invalid-discriminator.err
+++ b/tests/qapi-schema/union-invalid-discriminator.err
@@ -1,2 +1,2 @@
 union-invalid-discriminator.json: In union 'TestUnion':
-union-invalid-discriminator.json:10: discriminator 'enum_wrong' is not a member of 'base'
+union-invalid-discriminator.json:10: discriminator 'type_tag' is not a member of 'base'
diff --git a/tests/qapi-schema/union-invalid-discriminator.json b/tests/qapi-schema/union-invalid-discriminator.json
index c4fce97362..f315f36e37 100644
--- a/tests/qapi-schema/union-invalid-discriminator.json
+++ b/tests/qapi-schema/union-invalid-discriminator.json
@@ -8,7 +8,7 @@
   'data': { 'integer': 'int' } }
 
 { 'union': 'TestUnion',
-  'base': { 'enum1': 'TestEnum' },
-  'discriminator': 'enum_wrong',
+  'base': { 'type-tag': 'TestEnum' },
+  'discriminator': 'type_tag',
   'data': { 'value1': 'TestTypeA',
             'value2': 'TestTypeB' } }
-- 
2.39.2

Re: [PATCH 09/14] tests/qapi-schema: Improve union discriminator coverage

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:20AM +0100, Markus Armbruster wrote:
> A union's 'discriminator' must name a one of the common members.

s/ a//

> QAPISchemaVariants.check() looks it up by its c_name(), then checks
> the name matches exactly (because c_name() is not injective).
> 
> Tests union-base-empty and union-invalid-discriminator both cover the
> case where lookup fails.  Repurpose the latter to cover the case where
> it succeeds and the name check fails.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/qapi-schema/union-invalid-discriminator.err  | 2 +-
>  tests/qapi-schema/union-invalid-discriminator.json | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 

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

(- vs. _ is subtle, especially since I purposefully case-map them to
one another whenever I can...)

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

Re: [PATCH 09/14] tests/qapi-schema: Improve union discriminator coverage

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On Thu, Mar 16, 2023 at 08:13:20AM +0100, Markus Armbruster wrote:
>> A union's 'discriminator' must name a one of the common members.
>
> s/ a//

Yes.

>> QAPISchemaVariants.check() looks it up by its c_name(), then checks
>> the name matches exactly (because c_name() is not injective).
>> 
>> Tests union-base-empty and union-invalid-discriminator both cover the
>> case where lookup fails.  Repurpose the latter to cover the case where
>> it succeeds and the name check fails.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/qapi-schema/union-invalid-discriminator.err  | 2 +-
>>  tests/qapi-schema/union-invalid-discriminator.json | 4 ++--
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>> 
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> (- vs. _ is subtle, especially since I purposefully case-map them to
> one another whenever I can...)

Abusing the clash checking machinery to look up the the tag member is
kind of hacky, and it's why we have this odd case to cover.

Thanks!

[PATCH 10/14] tests/qapi-schema: Rename a few conditionals

[Markus Armbruster]

Positive test case

    { 'enum': 'TestIfEnum',
      'data': [ 'foo', { 'name' : 'bar', 'if': 'TEST_IF_ENUM_BAR' } ],
      'if': 'TEST_IF_ENUM' }

generates

    #if defined(TEST_IF_ENUM)
    typedef enum TestIfEnum {
	TEST_IF_ENUM_FOO,
    #if defined(TEST_IF_ENUM_BAR)
	TEST_IF_ENUM_BAR,
    #endif /* defined(TEST_IF_ENUM_BAR) */
	TEST_IF_ENUM__MAX,
    } TestIfEnum;

Macro TEST_IF_ENUM_BAR clashes with the enumeration constant.
Wouldn't compile with -DTEST_IF_BAR.

Rename the macro to TEST_IF_ENUM_MEMBER.  For consistency, rename
similar macros elsewhere as well.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json | 12 ++++++------
 tests/qapi-schema/qapi-schema-test.out  | 12 ++++++------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index ba7302f42b..5728d4de38 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -220,18 +220,18 @@
 
 { 'struct': 'TestIfStruct',
   'data': { 'foo': 'int',
-            'bar': { 'type': 'int', 'if': 'TEST_IF_STRUCT_BAR'} },
+            'bar': { 'type': 'int', 'if': 'TEST_IF_STRUCT_MEMBER'} },
   'if': 'TEST_IF_STRUCT' }
 
 { 'enum': 'TestIfEnum',
-  'data': [ 'foo', { 'name' : 'bar', 'if': 'TEST_IF_ENUM_BAR' } ],
+  'data': [ 'foo', { 'name' : 'bar', 'if': 'TEST_IF_ENUM_MEMBER' } ],
   'if': 'TEST_IF_ENUM' }
 
 { 'union': 'TestIfUnion',
   'base': { 'type': 'TestIfEnum' },
   'discriminator': 'type',
   'data': { 'foo': 'TestStruct',
-            'bar': { 'type': 'UserDefZero', 'if': 'TEST_IF_ENUM_BAR'} },
+            'bar': { 'type': 'UserDefZero', 'if': 'TEST_IF_ENUM_MEMBER'} },
   'if': { 'all': ['TEST_IF_UNION', 'TEST_IF_STRUCT'] } }
 
 { 'command': 'test-if-union-cmd',
@@ -240,7 +240,7 @@
 
 { 'alternate': 'TestIfAlternate',
   'data': { 'foo': 'int',
-            'bar': { 'type': 'TestStruct', 'if': 'TEST_IF_ALT_BAR'} },
+            'bar': { 'type': 'TestStruct', 'if': 'TEST_IF_ALT_MEMBER'} },
   'if': { 'all': ['TEST_IF_ALT', 'TEST_IF_STRUCT'] } }
 
 { 'command': 'test-if-alternate-cmd',
@@ -250,7 +250,7 @@
 { 'command': 'test-if-cmd',
   'data': {
     'foo': 'TestIfStruct',
-    'bar': { 'type': 'TestIfEnum', 'if': 'TEST_IF_CMD_BAR' } },
+    'bar': { 'type': 'TestIfEnum', 'if': 'TEST_IF_CMD_ARG' } },
   'returns': 'UserDefThree',
   'if': { 'all': ['TEST_IF_CMD', 'TEST_IF_STRUCT'] } }
 
@@ -258,7 +258,7 @@
 
 { 'event': 'TEST_IF_EVENT',
   'data': { 'foo': 'TestIfStruct',
-            'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_BAR' } },
+            'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_ARG' } },
   'if': { 'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT'] } }
 
 { 'event': 'TEST_IF_EVENT2', 'data': {},
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 043d75c655..cbd96f0b24 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -246,12 +246,12 @@ command __org.qemu_x-command q_obj___org.qemu_x-command-arg -> None
 object TestIfStruct
     member foo: int optional=False
     member bar: int optional=False
-        if TEST_IF_STRUCT_BAR
+        if TEST_IF_STRUCT_MEMBER
     if TEST_IF_STRUCT
 enum TestIfEnum
     member foo
     member bar
-        if TEST_IF_ENUM_BAR
+        if TEST_IF_ENUM_MEMBER
     if TEST_IF_ENUM
 object q_obj_TestIfUnion-base
     member type: TestIfEnum optional=False
@@ -261,7 +261,7 @@ object TestIfUnion
     tag type
     case foo: TestStruct
     case bar: UserDefZero
-        if TEST_IF_ENUM_BAR
+        if TEST_IF_ENUM_MEMBER
     if {'all': ['TEST_IF_UNION', 'TEST_IF_STRUCT']}
 object q_obj_test-if-union-cmd-arg
     member union-cmd-arg: TestIfUnion optional=False
@@ -273,7 +273,7 @@ alternate TestIfAlternate
     tag type
     case foo: int
     case bar: TestStruct
-        if TEST_IF_ALT_BAR
+        if TEST_IF_ALT_MEMBER
     if {'all': ['TEST_IF_ALT', 'TEST_IF_STRUCT']}
 object q_obj_test-if-alternate-cmd-arg
     member alt-cmd-arg: TestIfAlternate optional=False
@@ -284,7 +284,7 @@ command test-if-alternate-cmd q_obj_test-if-alternate-cmd-arg -> None
 object q_obj_test-if-cmd-arg
     member foo: TestIfStruct optional=False
     member bar: TestIfEnum optional=False
-        if TEST_IF_CMD_BAR
+        if TEST_IF_CMD_ARG
     if {'all': ['TEST_IF_CMD', 'TEST_IF_STRUCT']}
 command test-if-cmd q_obj_test-if-cmd-arg -> UserDefThree
     gen=True success_response=True boxed=False oob=False preconfig=False
@@ -296,7 +296,7 @@ array TestIfEnumList TestIfEnum
 object q_obj_TEST_IF_EVENT-arg
     member foo: TestIfStruct optional=False
     member bar: TestIfEnumList optional=False
-        if TEST_IF_EVT_BAR
+        if TEST_IF_EVT_ARG
     if {'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT']}
 event TEST_IF_EVENT q_obj_TEST_IF_EVENT-arg
     boxed=False
-- 
2.39.2

Re: [PATCH 10/14] tests/qapi-schema: Rename a few conditionals

[Philippe Mathieu-Daudé]

On 16/3/23 08:13, Markus Armbruster wrote:
> Positive test case
> 
>      { 'enum': 'TestIfEnum',
>        'data': [ 'foo', { 'name' : 'bar', 'if': 'TEST_IF_ENUM_BAR' } ],
>        'if': 'TEST_IF_ENUM' }
> 
> generates
> 
>      #if defined(TEST_IF_ENUM)
>      typedef enum TestIfEnum {
> 	TEST_IF_ENUM_FOO,
>      #if defined(TEST_IF_ENUM_BAR)
> 	TEST_IF_ENUM_BAR,
>      #endif /* defined(TEST_IF_ENUM_BAR) */
> 	TEST_IF_ENUM__MAX,
>      } TestIfEnum;
> 
> Macro TEST_IF_ENUM_BAR clashes with the enumeration constant.
> Wouldn't compile with -DTEST_IF_BAR.
> 
> Rename the macro to TEST_IF_ENUM_MEMBER.  For consistency, rename
> similar macros elsewhere as well.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/qapi-schema/qapi-schema-test.json | 12 ++++++------
>   tests/qapi-schema/qapi-schema-test.out  | 12 ++++++------
>   2 files changed, 12 insertions(+), 12 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 11/14] tests/qapi-schema: Clean up positive test for conditionals

[Markus Armbruster]

Union TestIfUnion is conditional on macros TEST_IF_UNION and
TEST_IF_STRUCT.  It uses TestIfEnum, which is conditional on macro
TEST_IF_ENUM.  If TEST_IF_ENUM and TEST_IF_STRUCT are defined, but
TEST_IF_ENUM isn't, the generated code won't compile.

Command test-if-cmd is conditional an macros TEST_IF_CMD and
TEST_IF_STRUCT, and uses TestIfEnum.  Similar issue.

Event TEST_IF_EVENT is conditional an macros TEST_IF_EVT and
TEST_IF_STRUCT, and uses TestIfEnum.  Similar issue.

Replace the uses of TestIfEnum in the latter two by str.

TestIfUnion is now TestIfEnum's only user.  Change TestIfEnum's
condition to TEST_IF_UNION.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json | 6 +++---
 tests/qapi-schema/qapi-schema-test.out  | 8 +++-----
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 5728d4de38..8f0ee95d23 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -225,7 +225,7 @@
 
 { 'enum': 'TestIfEnum',
   'data': [ 'foo', { 'name' : 'bar', 'if': 'TEST_IF_ENUM_MEMBER' } ],
-  'if': 'TEST_IF_ENUM' }
+  'if': 'TEST_IF_UNION' }
 
 { 'union': 'TestIfUnion',
   'base': { 'type': 'TestIfEnum' },
@@ -250,7 +250,7 @@
 { 'command': 'test-if-cmd',
   'data': {
     'foo': 'TestIfStruct',
-    'bar': { 'type': 'TestIfEnum', 'if': 'TEST_IF_CMD_ARG' } },
+    'bar': { 'type': 'str', 'if': 'TEST_IF_CMD_ARG' } },
   'returns': 'UserDefThree',
   'if': { 'all': ['TEST_IF_CMD', 'TEST_IF_STRUCT'] } }
 
@@ -258,7 +258,7 @@
 
 { 'event': 'TEST_IF_EVENT',
   'data': { 'foo': 'TestIfStruct',
-            'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_ARG' } },
+            'bar': { 'type': ['str'], 'if': 'TEST_IF_EVT_ARG' } },
   'if': { 'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT'] } }
 
 { 'event': 'TEST_IF_EVENT2', 'data': {},
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index cbd96f0b24..715f3a3f23 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -252,7 +252,7 @@ enum TestIfEnum
     member foo
     member bar
         if TEST_IF_ENUM_MEMBER
-    if TEST_IF_ENUM
+    if TEST_IF_UNION
 object q_obj_TestIfUnion-base
     member type: TestIfEnum optional=False
     if {'all': ['TEST_IF_UNION', 'TEST_IF_STRUCT']}
@@ -283,7 +283,7 @@ command test-if-alternate-cmd q_obj_test-if-alternate-cmd-arg -> None
     if {'all': ['TEST_IF_ALT', 'TEST_IF_STRUCT']}
 object q_obj_test-if-cmd-arg
     member foo: TestIfStruct optional=False
-    member bar: TestIfEnum optional=False
+    member bar: str optional=False
         if TEST_IF_CMD_ARG
     if {'all': ['TEST_IF_CMD', 'TEST_IF_STRUCT']}
 command test-if-cmd q_obj_test-if-cmd-arg -> UserDefThree
@@ -291,11 +291,9 @@ command test-if-cmd q_obj_test-if-cmd-arg -> UserDefThree
     if {'all': ['TEST_IF_CMD', 'TEST_IF_STRUCT']}
 command test-cmd-return-def-three None -> UserDefThree
     gen=True success_response=True boxed=False oob=False preconfig=False
-array TestIfEnumList TestIfEnum
-    if TEST_IF_ENUM
 object q_obj_TEST_IF_EVENT-arg
     member foo: TestIfStruct optional=False
-    member bar: TestIfEnumList optional=False
+    member bar: strList optional=False
         if TEST_IF_EVT_ARG
     if {'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT']}
 event TEST_IF_EVENT q_obj_TEST_IF_EVENT-arg
-- 
2.39.2

Re: [PATCH 11/14] tests/qapi-schema: Clean up positive test for conditionals

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:22AM +0100, Markus Armbruster wrote:
> Union TestIfUnion is conditional on macros TEST_IF_UNION and
> TEST_IF_STRUCT.  It uses TestIfEnum, which is conditional on macro
> TEST_IF_ENUM.  If TEST_IF_ENUM and TEST_IF_STRUCT are defined, but
> TEST_IF_ENUM isn't, the generated code won't compile.

s/ENUM/UNION/ in one of these two uses in this sentence.

> 
> Command test-if-cmd is conditional an macros TEST_IF_CMD and
> TEST_IF_STRUCT, and uses TestIfEnum.  Similar issue.
> 
> Event TEST_IF_EVENT is conditional an macros TEST_IF_EVT and
> TEST_IF_STRUCT, and uses TestIfEnum.  Similar issue.
> 
> Replace the uses of TestIfEnum in the latter two by str.
> 
> TestIfUnion is now TestIfEnum's only user.  Change TestIfEnum's
> condition to TEST_IF_UNION.

Fair enough, once the commit message doesn't confuse me in the first
paragraph ;)

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/qapi-schema/qapi-schema-test.json | 6 +++---
>  tests/qapi-schema/qapi-schema-test.out  | 8 +++-----
>  2 files changed, 6 insertions(+), 8 deletions(-)
>

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

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

Re: [PATCH 11/14] tests/qapi-schema: Clean up positive test for conditionals

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On Thu, Mar 16, 2023 at 08:13:22AM +0100, Markus Armbruster wrote:
>> Union TestIfUnion is conditional on macros TEST_IF_UNION and
>> TEST_IF_STRUCT.  It uses TestIfEnum, which is conditional on macro
>> TEST_IF_ENUM.  If TEST_IF_ENUM and TEST_IF_STRUCT are defined, but
>> TEST_IF_ENUM isn't, the generated code won't compile.
>
> s/ENUM/UNION/ in one of these two uses in this sentence.

Yes: If TEST_IF_UNION and TEST_IF_UNION are defined, ...

>> Command test-if-cmd is conditional an macros TEST_IF_CMD and
>> TEST_IF_STRUCT, and uses TestIfEnum.  Similar issue.
>> 
>> Event TEST_IF_EVENT is conditional an macros TEST_IF_EVT and
>> TEST_IF_STRUCT, and uses TestIfEnum.  Similar issue.
>> 
>> Replace the uses of TestIfEnum in the latter two by str.
>> 
>> TestIfUnion is now TestIfEnum's only user.  Change TestIfEnum's
>> condition to TEST_IF_UNION.
>
> Fair enough, once the commit message doesn't confuse me in the first
> paragraph ;)
>
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/qapi-schema/qapi-schema-test.json | 6 +++---
>>  tests/qapi-schema/qapi-schema-test.out  | 8 +++-----
>>  2 files changed, 6 insertions(+), 8 deletions(-)
>>
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [PATCH 11/14] tests/qapi-schema: Clean up positive test for conditionals

[Eric Blake]

On Fri, Mar 17, 2023 at 07:10:52AM +0100, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On Thu, Mar 16, 2023 at 08:13:22AM +0100, Markus Armbruster wrote:
> >> Union TestIfUnion is conditional on macros TEST_IF_UNION and
> >> TEST_IF_STRUCT.  It uses TestIfEnum, which is conditional on macro
> >> TEST_IF_ENUM.  If TEST_IF_ENUM and TEST_IF_STRUCT are defined, but
> >> TEST_IF_ENUM isn't, the generated code won't compile.
> >
> > s/ENUM/UNION/ in one of these two uses in this sentence.
> 
> Yes: If TEST_IF_UNION and TEST_IF_UNION are defined, ...

If TEST_IF_UNION and TEST_IF_STRUCT are defined, ...

(you are stuck in a maze of twisty little passages, all alike)

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

Re: [PATCH 11/14] tests/qapi-schema: Clean up positive test for conditionals

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On Fri, Mar 17, 2023 at 07:10:52AM +0100, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>> > On Thu, Mar 16, 2023 at 08:13:22AM +0100, Markus Armbruster wrote:
>> >> Union TestIfUnion is conditional on macros TEST_IF_UNION and
>> >> TEST_IF_STRUCT.  It uses TestIfEnum, which is conditional on macro
>> >> TEST_IF_ENUM.  If TEST_IF_ENUM and TEST_IF_STRUCT are defined, but
>> >> TEST_IF_ENUM isn't, the generated code won't compile.
>> >
>> > s/ENUM/UNION/ in one of these two uses in this sentence.
>> 
>> Yes: If TEST_IF_UNION and TEST_IF_UNION are defined, ...
>
> If TEST_IF_UNION and TEST_IF_STRUCT are defined, ...
>
> (you are stuck in a maze of twisty little passages, all alike)

I am!

Thanks :)

[PATCH 12/14] tests/qapi-schema: Cover optional conditional struct member

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json | 3 ++-
 tests/qapi-schema/qapi-schema-test.out  | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 8f0ee95d23..f1f742d38c 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -220,7 +220,8 @@
 
 { 'struct': 'TestIfStruct',
   'data': { 'foo': 'int',
-            'bar': { 'type': 'int', 'if': 'TEST_IF_STRUCT_MEMBER'} },
+            'bar': { 'type': 'int', 'if': 'TEST_IF_STRUCT_MEMBER'},
+            '*baz': { 'type': 'str', 'if': 'TEST_IF_STRUCT_MEMBER'} },
   'if': 'TEST_IF_STRUCT' }
 
 { 'enum': 'TestIfEnum',
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 715f3a3f23..cee92c0d2e 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -247,6 +247,8 @@ object TestIfStruct
     member foo: int optional=False
     member bar: int optional=False
         if TEST_IF_STRUCT_MEMBER
+    member baz: str optional=True
+        if TEST_IF_STRUCT_MEMBER
     if TEST_IF_STRUCT
 enum TestIfEnum
     member foo
-- 
2.39.2

Re: [PATCH 12/14] tests/qapi-schema: Cover optional conditional struct member

[Philippe Mathieu-Daudé]

On 16/3/23 08:13, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/qapi-schema/qapi-schema-test.json | 3 ++-
>   tests/qapi-schema/qapi-schema-test.out  | 2 ++
>   2 files changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 13/14] qapi: Fix code generated for optional conditional struct member

[Markus Armbruster]

The generated member visit neglects to emit #if around a conditional
struct member's has_ variable.  For instance,
tests/qapi-schema/qapi-schema-test.json generates

    #if defined(TEST_IF_STRUCT)
    bool visit_type_TestIfStruct_members(Visitor *v, TestIfStruct *obj, Error **errp)
    {
--->	bool has_baz = !!obj->baz;

	if (!visit_type_int(v, "foo", &obj->foo, errp)) {
	    return false;
	}
    #if defined(TEST_IF_STRUCT_MEMBER)
	if (!visit_type_int(v, "bar", &obj->bar, errp)) {
	    return false;
	}
    #endif /* defined(TEST_IF_STRUCT_MEMBER) */
    #if defined(TEST_IF_STRUCT_MEMBER)
	if (visit_optional(v, "baz", &has_baz)) {
	    if (!visit_type_str(v, "baz", &obj->baz, errp)) {
		return false;
	    }
	}
    #endif /* defined(TEST_IF_STRUCT_MEMBER) */
	return true;
    }
    [...]
    #endif /* defined(TEST_IF_STRUCT) */

Won't compile when TEST_IF_STRUCT is defined and TEST_IF_STRUCT_MEMBER
isn't.

Fix that the obvious way:

    #if defined(TEST_IF_STRUCT_MEMBER)
	bool has_baz = !!obj->baz;
    #endif /* defined(TEST_IF_STRUCT_MEMBER) */

Fixes: 44ea9d9be33c (qapi: Start to elide redundant has_FOO in generated C)
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/visit.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 26a584ee4c..c56ea4d724 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -74,11 +74,13 @@ def gen_visit_object_members(name: str,
     sep = ''
     for memb in members:
         if memb.optional and not memb.need_has():
+            ret += memb.ifcond.gen_if()
             ret += mcgen('''
     bool has_%(c_name)s = !!obj->%(c_name)s;
 ''',
                          c_name=c_name(memb.name))
             sep = '\n'
+            ret += memb.ifcond.gen_endif()
     ret += sep
 
     if base:
-- 
2.39.2

Re: [PATCH 13/14] qapi: Fix code generated for optional conditional struct member

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:24AM +0100, Markus Armbruster wrote:
> The generated member visit neglects to emit #if around a conditional
> struct member's has_ variable.  For instance,
> tests/qapi-schema/qapi-schema-test.json generates
> 
>     #if defined(TEST_IF_STRUCT)
>     bool visit_type_TestIfStruct_members(Visitor *v, TestIfStruct *obj, Error **errp)
>     {
> --->	bool has_baz = !!obj->baz;
> 
...
> 
> Won't compile when TEST_IF_STRUCT is defined and TEST_IF_STRUCT_MEMBER
> isn't.
> 
> Fix that the obvious way:
> 
>     #if defined(TEST_IF_STRUCT_MEMBER)
> 	bool has_baz = !!obj->baz;
>     #endif /* defined(TEST_IF_STRUCT_MEMBER) */
> 
> Fixes: 44ea9d9be33c (qapi: Start to elide redundant has_FOO in generated C)
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi/visit.py | 2 ++
>  1 file changed, 2 insertions(+)

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

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

[PATCH 14/14] qapi: Require boxed for conditional command and event arguments

[Markus Armbruster]

The C code generator fails to honor 'if' conditions of command and
event arguments.

For instance, tests/qapi-schema/qapi-schema-test.json has

    { 'event': 'TEST_IF_EVENT',
      'data': { 'foo': 'TestIfStruct',
		'bar': { 'type': ['str'], 'if': 'TEST_IF_EVT_ARG' } },
      'if': { 'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT'] } }

Generated tests/test-qapi-events.h fails to honor the TEST_IF_EVT_ARG
condition:

    #if defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)
    void qapi_event_send_test_if_event(TestIfStruct *foo, strList *bar);
    #endif /* defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT) */

Only uses so far are in tests/.

We could fix the generator to emit something like

    #if defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)
    void qapi_event_send_test_if_event(TestIfStruct *foo
    #if defined(TEST_IF_EVT_ARG)
                    , strList *bar
    #endif
                    );
    #endif /* defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT) */

Ugly.  Calls become similarly ugly.  Not worth fixing.

Conditional arguments work fine with 'boxed': true, simply because
complex types with conditional members work fine.  Not worth breaking.

Reject conditional arguments unless boxed.

Move the tests cases covering unboxed conditional arguments out of
tests/qapi-schema/qapi-schema-test.json.  Cover boxed conditional
arguments there instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/devel/qapi-code-gen.rst                 |  5 ++---
 scripts/qapi/commands.py                     |  1 +
 scripts/qapi/gen.py                          |  1 +
 scripts/qapi/schema.py                       | 14 ++++++++++++++
 tests/qapi-schema/args-if-implicit.err       |  2 ++
 tests/qapi-schema/args-if-implicit.json      |  4 ++++
 tests/qapi-schema/args-if-implicit.out       |  0
 tests/qapi-schema/args-if-unboxed.err        |  2 ++
 tests/qapi-schema/args-if-unboxed.json       |  6 ++++++
 tests/qapi-schema/args-if-unboxed.out        |  0
 tests/qapi-schema/event-args-if-unboxed.err  |  2 ++
 tests/qapi-schema/event-args-if-unboxed.json |  4 ++++
 tests/qapi-schema/event-args-if-unboxed.out  |  0
 tests/qapi-schema/meson.build                |  2 ++
 tests/qapi-schema/qapi-schema-test.json      |  9 ++++-----
 tests/qapi-schema/qapi-schema-test.out       | 18 ++++--------------
 16 files changed, 48 insertions(+), 22 deletions(-)
 create mode 100644 tests/qapi-schema/args-if-implicit.err
 create mode 100644 tests/qapi-schema/args-if-implicit.json
 create mode 100644 tests/qapi-schema/args-if-implicit.out
 create mode 100644 tests/qapi-schema/args-if-unboxed.err
 create mode 100644 tests/qapi-schema/args-if-unboxed.json
 create mode 100644 tests/qapi-schema/args-if-unboxed.out
 create mode 100644 tests/qapi-schema/event-args-if-unboxed.err
 create mode 100644 tests/qapi-schema/event-args-if-unboxed.json
 create mode 100644 tests/qapi-schema/event-args-if-unboxed.out

diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index 23e7f2fb1c..879a649e8c 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -805,9 +805,8 @@ gets its generated code guarded like this::
  ... generated code ...
  #endif /* defined(HAVE_BAR) && defined(CONFIG_FOO) */
 
-Individual members of complex types, commands arguments, and
-event-specific data can also be made conditional.  This requires the
-longhand form of MEMBER.
+Individual members of complex types can also be made conditional.
+This requires the longhand form of MEMBER.
 
 Example: a struct type with unconditional member 'foo' and conditional
 member 'bar' ::
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 79c5e5c3a9..bda6896076 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -64,6 +64,7 @@ def gen_call(name: str,
     elif arg_type:
         assert not arg_type.variants
         for memb in arg_type.members:
+            assert not memb.ifcond.is_present()
             if memb.need_has():
                 argstr += 'arg.has_%s, ' % c_name(memb.name)
             argstr += 'arg.%s, ' % c_name(memb.name)
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index b5a8d03e8e..8f8f784f4a 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -119,6 +119,7 @@ def build_params(arg_type: Optional[QAPISchemaObjectType],
     elif arg_type:
         assert not arg_type.variants
         for memb in arg_type.members:
+            assert not memb.ifcond.is_present()
             ret += sep
             sep = ', '
             if memb.need_has():
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 719152fe49..8f31f8832f 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -486,6 +486,10 @@ def is_empty(self):
         assert self.members is not None
         return not self.members and not self.variants
 
+    def has_conditional_members(self):
+        assert self.members is not None
+        return any(m.ifcond.is_present() for m in self.members)
+
     def c_name(self):
         assert self.name != 'q_empty'
         return super().c_name()
@@ -817,6 +821,11 @@ def check(self, schema):
                     self.info,
                     "command's 'data' can take %s only with 'boxed': true"
                     % self.arg_type.describe())
+            self.arg_type.check(schema)
+            if self.arg_type.has_conditional_members() and not self.boxed:
+                raise QAPISemError(
+                    self.info,
+                    "conditional command arguments require 'boxed': true")
         if self._ret_type_name:
             self.ret_type = schema.resolve_type(
                 self._ret_type_name, self.info, "command's 'returns'")
@@ -872,6 +881,11 @@ def check(self, schema):
                     self.info,
                     "event's 'data' can take %s only with 'boxed': true"
                     % self.arg_type.describe())
+            self.arg_type.check(schema)
+            if self.arg_type.has_conditional_members() and not self.boxed:
+                raise QAPISemError(
+                    self.info,
+                    "conditional event arguments require 'boxed': true")
 
     def connect_doc(self, doc=None):
         super().connect_doc(doc)
diff --git a/tests/qapi-schema/args-if-implicit.err b/tests/qapi-schema/args-if-implicit.err
new file mode 100644
index 0000000000..da2447d397
--- /dev/null
+++ b/tests/qapi-schema/args-if-implicit.err
@@ -0,0 +1,2 @@
+args-if-implicit.json: In command 'test-if-cmd':
+args-if-implicit.json:1: conditional command arguments require 'boxed': true
diff --git a/tests/qapi-schema/args-if-implicit.json b/tests/qapi-schema/args-if-implicit.json
new file mode 100644
index 0000000000..1eda39cb1e
--- /dev/null
+++ b/tests/qapi-schema/args-if-implicit.json
@@ -0,0 +1,4 @@
+{ 'command': 'test-if-cmd',
+  'data': {
+    'foo': 'int',
+    'bar': { 'type': 'str', 'if': 'TEST_IF_CMD_ARG' } } }
diff --git a/tests/qapi-schema/args-if-implicit.out b/tests/qapi-schema/args-if-implicit.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/args-if-unboxed.err b/tests/qapi-schema/args-if-unboxed.err
new file mode 100644
index 0000000000..3d2fc836ef
--- /dev/null
+++ b/tests/qapi-schema/args-if-unboxed.err
@@ -0,0 +1,2 @@
+args-if-unboxed.json: In command 'test-if-cmd':
+args-if-unboxed.json:5: conditional command arguments require 'boxed': true
diff --git a/tests/qapi-schema/args-if-unboxed.json b/tests/qapi-schema/args-if-unboxed.json
new file mode 100644
index 0000000000..6e04c13e72
--- /dev/null
+++ b/tests/qapi-schema/args-if-unboxed.json
@@ -0,0 +1,6 @@
+{ 'struct': 'TestIfCmdArgs',
+  'data': {
+    'foo': 'int',
+    'bar': { 'type': 'str', 'if': 'TEST_IF_CMD_ARG' } } }
+{ 'command': 'test-if-cmd',
+  'data': 'TestIfCmdArgs' }
diff --git a/tests/qapi-schema/args-if-unboxed.out b/tests/qapi-schema/args-if-unboxed.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/event-args-if-unboxed.err b/tests/qapi-schema/event-args-if-unboxed.err
new file mode 100644
index 0000000000..41ac64c6f3
--- /dev/null
+++ b/tests/qapi-schema/event-args-if-unboxed.err
@@ -0,0 +1,2 @@
+tests/qapi-schema/event-args-if-unboxed.json: In event 'TEST_IF_EVENT':
+tests/qapi-schema/event-args-if-unboxed.json:1: event's 'data' members may have 'if' conditions only with 'boxed': true
diff --git a/tests/qapi-schema/event-args-if-unboxed.json b/tests/qapi-schema/event-args-if-unboxed.json
new file mode 100644
index 0000000000..ca42a74e3a
--- /dev/null
+++ b/tests/qapi-schema/event-args-if-unboxed.json
@@ -0,0 +1,4 @@
+ { 'event': 'TEST_IF_EVENT',
+  'data': {
+    'foo': 'int',
+    'bar': { 'type': 'str', 'if': 'TEST_IF_CMD_ARG' } } }
diff --git a/tests/qapi-schema/event-args-if-unboxed.out b/tests/qapi-schema/event-args-if-unboxed.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index f88110bddf..a06515ca17 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -27,6 +27,8 @@ schemas = [
   'args-bad-boxed.json',
   'args-boxed-anon.json',
   'args-boxed-string.json',
+  'args-if-implicit.json',
+  'args-if-unboxed.json',
   'args-int.json',
   'args-invalid.json',
   'args-member-array-bad.json',
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index f1f742d38c..8bbf94834a 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -249,17 +249,16 @@
   'if': { 'all': ['TEST_IF_ALT', 'TEST_IF_STRUCT'] } }
 
 { 'command': 'test-if-cmd',
-  'data': {
-    'foo': 'TestIfStruct',
-    'bar': { 'type': 'str', 'if': 'TEST_IF_CMD_ARG' } },
+  'boxed': true,
+  'data': 'TestIfStruct',
   'returns': 'UserDefThree',
   'if': { 'all': ['TEST_IF_CMD', 'TEST_IF_STRUCT'] } }
 
 { 'command': 'test-cmd-return-def-three', 'returns': 'UserDefThree' }
 
 { 'event': 'TEST_IF_EVENT',
-  'data': { 'foo': 'TestIfStruct',
-            'bar': { 'type': ['str'], 'if': 'TEST_IF_EVT_ARG' } },
+  'boxed': true,
+  'data': 'TestIfStruct',
   'if': { 'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT'] } }
 
 { 'event': 'TEST_IF_EVENT2', 'data': {},
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index cee92c0d2e..cc34b422e6 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -283,23 +283,13 @@ object q_obj_test-if-alternate-cmd-arg
 command test-if-alternate-cmd q_obj_test-if-alternate-cmd-arg -> None
     gen=True success_response=True boxed=False oob=False preconfig=False
     if {'all': ['TEST_IF_ALT', 'TEST_IF_STRUCT']}
-object q_obj_test-if-cmd-arg
-    member foo: TestIfStruct optional=False
-    member bar: str optional=False
-        if TEST_IF_CMD_ARG
-    if {'all': ['TEST_IF_CMD', 'TEST_IF_STRUCT']}
-command test-if-cmd q_obj_test-if-cmd-arg -> UserDefThree
-    gen=True success_response=True boxed=False oob=False preconfig=False
+command test-if-cmd TestIfStruct -> UserDefThree
+    gen=True success_response=True boxed=True oob=False preconfig=False
     if {'all': ['TEST_IF_CMD', 'TEST_IF_STRUCT']}
 command test-cmd-return-def-three None -> UserDefThree
     gen=True success_response=True boxed=False oob=False preconfig=False
-object q_obj_TEST_IF_EVENT-arg
-    member foo: TestIfStruct optional=False
-    member bar: strList optional=False
-        if TEST_IF_EVT_ARG
-    if {'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT']}
-event TEST_IF_EVENT q_obj_TEST_IF_EVENT-arg
-    boxed=False
+event TEST_IF_EVENT TestIfStruct
+    boxed=True
     if {'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT']}
 event TEST_IF_EVENT2 None
     boxed=False
-- 
2.39.2

Re: [PATCH 14/14] qapi: Require boxed for conditional command and event arguments

[Eric Blake]

On Thu, Mar 16, 2023 at 08:13:25AM +0100, Markus Armbruster wrote:
> The C code generator fails to honor 'if' conditions of command and
> event arguments.
>
...
> 
> Conditional arguments work fine with 'boxed': true, simply because
> complex types with conditional members work fine.  Not worth breaking.
> 
> Reject conditional arguments unless boxed.

Yay - matches my earlier suggestion at how to avoid #if in the middle
of a parameter list.

> 
> Move the tests cases covering unboxed conditional arguments out of
> tests/qapi-schema/qapi-schema-test.json.  Cover boxed conditional
> arguments there instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

A big end to the series, but I'm glad we got here.

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

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