* [PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments
@ 2023-03-16 7:13 Markus Armbruster
2023-03-16 7:13 ` [PATCH 01/14] qapi: Fix error message format regression Markus Armbruster
` (13 more replies)
0 siblings, 14 replies; 36+ messages in thread
From: Markus Armbruster @ 2023-03-16 7:13 UTC (permalink / raw)
To: qemu-devel; +Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 01/14] qapi: Fix error message format regression
2023-03-16 7:13 [PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments Markus Armbruster
@ 2023-03-16 7:13 ` Markus Armbruster
2023-03-16 21:56 ` Eric Blake
2023-03-16 7:13 ` [PATCH 02/14] qapi/schema: Use super() Markus Armbruster
` (12 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Markus Armbruster @ 2023-03-16 7:13 UTC (permalink / raw)
To: qemu-devel; +Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 02/14] qapi/schema: Use super()
2023-03-16 7:13 [PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments Markus Armbruster
2023-03-16 7:13 ` [PATCH 01/14] qapi: Fix error message format regression Markus Armbruster
@ 2023-03-16 7:13 ` Markus Armbruster
2023-03-16 7:40 ` Philippe Mathieu-Daudé
2023-03-16 7:13 ` [PATCH 03/14] qapi: Clean up after removal of simple unions Markus Armbruster
` (11 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Markus Armbruster @ 2023-03-16 7:13 UTC (permalink / raw)
To: qemu-devel; +Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 03/14] qapi: Clean up after removal of simple unions
2023-03-16 7:13 [PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments Markus Armbruster
2023-03-16 7:13 ` [PATCH 01/14] qapi: Fix error message format regression Markus Armbruster
2023-03-16 7:13 ` [PATCH 02/14] qapi/schema: Use super() Markus Armbruster
@ 2023-03-16 7:13 ` Markus Armbruster
2023-03-17 0:38 ` Eric Blake
2023-03-16 7:13 ` [PATCH 04/14] qapi: Split up check_type() Markus Armbruster
` (10 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Markus Armbruster @ 2023-03-16 7:13 UTC (permalink / raw)
To: qemu-devel; +Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 04/14] qapi: Split up check_type()
2023-03-16 7:13 [PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments Markus Armbruster
` (2 preceding siblings ...)
2023-03-16 7:13 ` [PATCH 03/14] qapi: Clean up after removal of simple unions Markus Armbruster
@ 2023-03-16 7:13 ` Markus Armbruster
2023-03-17 0:53 ` Eric Blake
2023-03-16 7:13 ` [PATCH 05/14] qapi: Improve error message for unexpected array types Markus Armbruster
` (9 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Markus Armbruster @ 2023-03-16 7:13 UTC (permalink / raw)
To: qemu-devel; +Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 05/14] qapi: Improve error message for unexpected array types
2023-03-16 7:13 [PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments Markus Armbruster
` (3 preceding siblings ...)
2023-03-16 7:13 ` [PATCH 04/14] qapi: Split up check_type() Markus Armbruster
@ 2023-03-16 7:13 ` Markus Armbruster
2023-03-16 7:41 ` Philippe Mathieu-Daudé
2023-03-16 7:13 ` [PATCH 06/14] qapi: Simplify code a bit after previous commit Markus Armbruster
` (8 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Markus Armbruster @ 2023-03-16 7:13 UTC (permalink / raw)
To: qemu-devel; +Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 06/14] qapi: Simplify code a bit after previous commit
2023-03-16 7:13 [PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments Markus Armbruster
` (4 preceding siblings ...)
2023-03-16 7:13 ` [PATCH 05/14] qapi: Improve error message for unexpected array types Markus Armbruster
@ 2023-03-16 7:13 ` Markus Armbruster
2023-03-17 0:55 ` Eric Blake
2023-03-16 7:13 ` [PATCH 07/14] qapi: Fix error message when type name or array is expected Markus Armbruster
` (7 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Markus Armbruster @ 2023-03-16 7:13 UTC (permalink / raw)
To: qemu-devel; +Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 07/14] qapi: Fix error message when type name or array is expected
2023-03-16 7:13 [PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments Markus Armbruster
` (5 preceding siblings ...)
2023-03-16 7:13 ` [PATCH 06/14] qapi: Simplify code a bit after previous commit Markus Armbruster
@ 2023-03-16 7:13 ` Markus Armbruster
2023-03-17 0:57 ` Eric Blake
2023-03-16 7:13 ` [PATCH 08/14] qapi: Fix to reject 'data': 'mumble' in struct Markus Armbruster
` (6 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Markus Armbruster @ 2023-03-16 7:13 UTC (permalink / raw)
To: qemu-devel; +Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 08/14] qapi: Fix to reject 'data': 'mumble' in struct
2023-03-16 7:13 [PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments Markus Armbruster
` (6 preceding siblings ...)
2023-03-16 7:13 ` [PATCH 07/14] qapi: Fix error message when type name or array is expected Markus Armbruster
@ 2023-03-16 7:13 ` Markus Armbruster
2023-03-17 1:02 ` Eric Blake
2023-03-16 7:13 ` [PATCH 09/14] tests/qapi-schema: Improve union discriminator coverage Markus Armbruster
` (5 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Markus Armbruster @ 2023-03-16 7:13 UTC (permalink / raw)
To: qemu-devel; +Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 09/14] tests/qapi-schema: Improve union discriminator coverage
2023-03-16 7:13 [PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments Markus Armbruster
` (7 preceding siblings ...)
2023-03-16 7:13 ` [PATCH 08/14] qapi: Fix to reject 'data': 'mumble' in struct Markus Armbruster
@ 2023-03-16 7:13 ` Markus Armbruster
2023-03-17 1:06 ` Eric Blake
2023-03-16 7:13 ` [PATCH 10/14] tests/qapi-schema: Rename a few conditionals Markus Armbruster
` (4 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Markus Armbruster @ 2023-03-16 7:13 UTC (permalink / raw)
To: qemu-devel; +Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 10/14] tests/qapi-schema: Rename a few conditionals
2023-03-16 7:13 [PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments Markus Armbruster
` (8 preceding siblings ...)
2023-03-16 7:13 ` [PATCH 09/14] tests/qapi-schema: Improve union discriminator coverage Markus Armbruster
@ 2023-03-16 7:13 ` Markus Armbruster
2023-03-16 7:45 ` Philippe Mathieu-Daudé
2023-03-16 7:13 ` [PATCH 11/14] tests/qapi-schema: Clean up positive test for conditionals Markus Armbruster
` (3 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Markus Armbruster @ 2023-03-16 7:13 UTC (permalink / raw)
To: qemu-devel; +Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 11/14] tests/qapi-schema: Clean up positive test for conditionals
2023-03-16 7:13 [PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments Markus Armbruster
` (9 preceding siblings ...)
2023-03-16 7:13 ` [PATCH 10/14] tests/qapi-schema: Rename a few conditionals Markus Armbruster
@ 2023-03-16 7:13 ` Markus Armbruster
2023-03-17 1:09 ` Eric Blake
2023-03-16 7:13 ` [PATCH 12/14] tests/qapi-schema: Cover optional conditional struct member Markus Armbruster
` (2 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Markus Armbruster @ 2023-03-16 7:13 UTC (permalink / raw)
To: qemu-devel; +Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 12/14] tests/qapi-schema: Cover optional conditional struct member
2023-03-16 7:13 [PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments Markus Armbruster
` (10 preceding siblings ...)
2023-03-16 7:13 ` [PATCH 11/14] tests/qapi-schema: Clean up positive test for conditionals Markus Armbruster
@ 2023-03-16 7:13 ` Markus Armbruster
2023-03-16 7:45 ` Philippe Mathieu-Daudé
2023-03-16 7:13 ` [PATCH 13/14] qapi: Fix code generated for " Markus Armbruster
2023-03-16 7:13 ` [PATCH 14/14] qapi: Require boxed for conditional command and event arguments Markus Armbruster
13 siblings, 1 reply; 36+ messages in thread
From: Markus Armbruster @ 2023-03-16 7:13 UTC (permalink / raw)
To: qemu-devel; +Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 13/14] qapi: Fix code generated for optional conditional struct member
2023-03-16 7:13 [PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments Markus Armbruster
` (11 preceding siblings ...)
2023-03-16 7:13 ` [PATCH 12/14] tests/qapi-schema: Cover optional conditional struct member Markus Armbruster
@ 2023-03-16 7:13 ` Markus Armbruster
2023-03-17 1:13 ` Eric Blake
2023-03-16 7:13 ` [PATCH 14/14] qapi: Require boxed for conditional command and event arguments Markus Armbruster
13 siblings, 1 reply; 36+ messages in thread
From: Markus Armbruster @ 2023-03-16 7:13 UTC (permalink / raw)
To: qemu-devel; +Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 14/14] qapi: Require boxed for conditional command and event arguments
2023-03-16 7:13 [PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments Markus Armbruster
` (12 preceding siblings ...)
2023-03-16 7:13 ` [PATCH 13/14] qapi: Fix code generated for " Markus Armbruster
@ 2023-03-16 7:13 ` Markus Armbruster
2023-03-17 1:17 ` Eric Blake
13 siblings, 1 reply; 36+ messages in thread
From: Markus Armbruster @ 2023-03-16 7:13 UTC (permalink / raw)
To: qemu-devel; +Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 02/14] qapi/schema: Use super()
2023-03-16 7:13 ` [PATCH 02/14] qapi/schema: Use super() Markus Armbruster
@ 2023-03-16 7:40 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 36+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-03-16 7:40 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 05/14] qapi: Improve error message for unexpected array types
2023-03-16 7:13 ` [PATCH 05/14] qapi: Improve error message for unexpected array types Markus Armbruster
@ 2023-03-16 7:41 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 36+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-03-16 7:41 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 10/14] tests/qapi-schema: Rename a few conditionals
2023-03-16 7:13 ` [PATCH 10/14] tests/qapi-schema: Rename a few conditionals Markus Armbruster
@ 2023-03-16 7:45 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 36+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-03-16 7:45 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 12/14] tests/qapi-schema: Cover optional conditional struct member
2023-03-16 7:13 ` [PATCH 12/14] tests/qapi-schema: Cover optional conditional struct member Markus Armbruster
@ 2023-03-16 7:45 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 36+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-03-16 7:45 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: michael.roth, marcandre.lureau, berrange, eblake, jsnow
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>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 01/14] qapi: Fix error message format regression
2023-03-16 7:13 ` [PATCH 01/14] qapi: Fix error message format regression Markus Armbruster
@ 2023-03-16 21:56 ` Eric Blake
0 siblings, 0 replies; 36+ messages in thread
From: Eric Blake @ 2023-03-16 21:56 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, michael.roth, marcandre.lureau, berrange, jsnow
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 03/14] qapi: Clean up after removal of simple unions
2023-03-16 7:13 ` [PATCH 03/14] qapi: Clean up after removal of simple unions Markus Armbruster
@ 2023-03-17 0:38 ` Eric Blake
0 siblings, 0 replies; 36+ messages in thread
From: Eric Blake @ 2023-03-17 0:38 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, michael.roth, marcandre.lureau, berrange, jsnow
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 04/14] qapi: Split up check_type()
2023-03-16 7:13 ` [PATCH 04/14] qapi: Split up check_type() Markus Armbruster
@ 2023-03-17 0:53 ` Eric Blake
2023-03-17 5:36 ` Markus Armbruster
0 siblings, 1 reply; 36+ messages in thread
From: Eric Blake @ 2023-03-17 0:53 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, michael.roth, marcandre.lureau, berrange, jsnow
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 06/14] qapi: Simplify code a bit after previous commit
2023-03-16 7:13 ` [PATCH 06/14] qapi: Simplify code a bit after previous commit Markus Armbruster
@ 2023-03-17 0:55 ` Eric Blake
2023-03-17 5:42 ` Markus Armbruster
0 siblings, 1 reply; 36+ messages in thread
From: Eric Blake @ 2023-03-17 0:55 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, michael.roth, marcandre.lureau, berrange, jsnow
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 07/14] qapi: Fix error message when type name or array is expected
2023-03-16 7:13 ` [PATCH 07/14] qapi: Fix error message when type name or array is expected Markus Armbruster
@ 2023-03-17 0:57 ` Eric Blake
0 siblings, 0 replies; 36+ messages in thread
From: Eric Blake @ 2023-03-17 0:57 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, michael.roth, marcandre.lureau, berrange, jsnow
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 08/14] qapi: Fix to reject 'data': 'mumble' in struct
2023-03-16 7:13 ` [PATCH 08/14] qapi: Fix to reject 'data': 'mumble' in struct Markus Armbruster
@ 2023-03-17 1:02 ` Eric Blake
2023-03-17 5:48 ` Markus Armbruster
0 siblings, 1 reply; 36+ messages in thread
From: Eric Blake @ 2023-03-17 1:02 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, michael.roth, marcandre.lureau, berrange, jsnow
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 09/14] tests/qapi-schema: Improve union discriminator coverage
2023-03-16 7:13 ` [PATCH 09/14] tests/qapi-schema: Improve union discriminator coverage Markus Armbruster
@ 2023-03-17 1:06 ` Eric Blake
2023-03-17 5:51 ` Markus Armbruster
0 siblings, 1 reply; 36+ messages in thread
From: Eric Blake @ 2023-03-17 1:06 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, michael.roth, marcandre.lureau, berrange, jsnow
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 11/14] tests/qapi-schema: Clean up positive test for conditionals
2023-03-16 7:13 ` [PATCH 11/14] tests/qapi-schema: Clean up positive test for conditionals Markus Armbruster
@ 2023-03-17 1:09 ` Eric Blake
2023-03-17 6:10 ` Markus Armbruster
0 siblings, 1 reply; 36+ messages in thread
From: Eric Blake @ 2023-03-17 1:09 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, michael.roth, marcandre.lureau, berrange, jsnow
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 13/14] qapi: Fix code generated for optional conditional struct member
2023-03-16 7:13 ` [PATCH 13/14] qapi: Fix code generated for " Markus Armbruster
@ 2023-03-17 1:13 ` Eric Blake
0 siblings, 0 replies; 36+ messages in thread
From: Eric Blake @ 2023-03-17 1:13 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, michael.roth, marcandre.lureau, berrange, jsnow
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 14/14] qapi: Require boxed for conditional command and event arguments
2023-03-16 7:13 ` [PATCH 14/14] qapi: Require boxed for conditional command and event arguments Markus Armbruster
@ 2023-03-17 1:17 ` Eric Blake
0 siblings, 0 replies; 36+ messages in thread
From: Eric Blake @ 2023-03-17 1:17 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, michael.roth, marcandre.lureau, berrange, jsnow
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 04/14] qapi: Split up check_type()
2023-03-17 0:53 ` Eric Blake
@ 2023-03-17 5:36 ` Markus Armbruster
0 siblings, 0 replies; 36+ messages in thread
From: Markus Armbruster @ 2023-03-17 5:36 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, michael.roth, marcandre.lureau, berrange, jsnow
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!
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 06/14] qapi: Simplify code a bit after previous commit
2023-03-17 0:55 ` Eric Blake
@ 2023-03-17 5:42 ` Markus Armbruster
0 siblings, 0 replies; 36+ messages in thread
From: Markus Armbruster @ 2023-03-17 5:42 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, michael.roth, marcandre.lureau, berrange, jsnow
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!
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 08/14] qapi: Fix to reject 'data': 'mumble' in struct
2023-03-17 1:02 ` Eric Blake
@ 2023-03-17 5:48 ` Markus Armbruster
0 siblings, 0 replies; 36+ messages in thread
From: Markus Armbruster @ 2023-03-17 5:48 UTC (permalink / raw)
To: Eric Blake
Cc: Markus Armbruster, qemu-devel, michael.roth, marcandre.lureau,
berrange, jsnow
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!
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 09/14] tests/qapi-schema: Improve union discriminator coverage
2023-03-17 1:06 ` Eric Blake
@ 2023-03-17 5:51 ` Markus Armbruster
0 siblings, 0 replies; 36+ messages in thread
From: Markus Armbruster @ 2023-03-17 5:51 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, michael.roth, marcandre.lureau, berrange, jsnow
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!
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 11/14] tests/qapi-schema: Clean up positive test for conditionals
2023-03-17 1:09 ` Eric Blake
@ 2023-03-17 6:10 ` Markus Armbruster
2023-03-17 12:29 ` Eric Blake
0 siblings, 1 reply; 36+ messages in thread
From: Markus Armbruster @ 2023-03-17 6:10 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, michael.roth, marcandre.lureau, berrange, jsnow
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!
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 11/14] tests/qapi-schema: Clean up positive test for conditionals
2023-03-17 6:10 ` Markus Armbruster
@ 2023-03-17 12:29 ` Eric Blake
2023-03-17 14:14 ` Markus Armbruster
0 siblings, 1 reply; 36+ messages in thread
From: Eric Blake @ 2023-03-17 12:29 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, michael.roth, marcandre.lureau, berrange, jsnow
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 11/14] tests/qapi-schema: Clean up positive test for conditionals
2023-03-17 12:29 ` Eric Blake
@ 2023-03-17 14:14 ` Markus Armbruster
0 siblings, 0 replies; 36+ messages in thread
From: Markus Armbruster @ 2023-03-17 14:14 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, michael.roth, marcandre.lureau, berrange, jsnow
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 :)
^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2023-03-17 14:14 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-16 7:13 [PATCH 00/14] qapi: Fix minor bugs, require boxed for conditional arguments Markus Armbruster
2023-03-16 7:13 ` [PATCH 01/14] qapi: Fix error message format regression Markus Armbruster
2023-03-16 21:56 ` Eric Blake
2023-03-16 7:13 ` [PATCH 02/14] qapi/schema: Use super() Markus Armbruster
2023-03-16 7:40 ` Philippe Mathieu-Daudé
2023-03-16 7:13 ` [PATCH 03/14] qapi: Clean up after removal of simple unions Markus Armbruster
2023-03-17 0:38 ` Eric Blake
2023-03-16 7:13 ` [PATCH 04/14] qapi: Split up check_type() Markus Armbruster
2023-03-17 0:53 ` Eric Blake
2023-03-17 5:36 ` Markus Armbruster
2023-03-16 7:13 ` [PATCH 05/14] qapi: Improve error message for unexpected array types Markus Armbruster
2023-03-16 7:41 ` Philippe Mathieu-Daudé
2023-03-16 7:13 ` [PATCH 06/14] qapi: Simplify code a bit after previous commit Markus Armbruster
2023-03-17 0:55 ` Eric Blake
2023-03-17 5:42 ` Markus Armbruster
2023-03-16 7:13 ` [PATCH 07/14] qapi: Fix error message when type name or array is expected Markus Armbruster
2023-03-17 0:57 ` Eric Blake
2023-03-16 7:13 ` [PATCH 08/14] qapi: Fix to reject 'data': 'mumble' in struct Markus Armbruster
2023-03-17 1:02 ` Eric Blake
2023-03-17 5:48 ` Markus Armbruster
2023-03-16 7:13 ` [PATCH 09/14] tests/qapi-schema: Improve union discriminator coverage Markus Armbruster
2023-03-17 1:06 ` Eric Blake
2023-03-17 5:51 ` Markus Armbruster
2023-03-16 7:13 ` [PATCH 10/14] tests/qapi-schema: Rename a few conditionals Markus Armbruster
2023-03-16 7:45 ` Philippe Mathieu-Daudé
2023-03-16 7:13 ` [PATCH 11/14] tests/qapi-schema: Clean up positive test for conditionals Markus Armbruster
2023-03-17 1:09 ` Eric Blake
2023-03-17 6:10 ` Markus Armbruster
2023-03-17 12:29 ` Eric Blake
2023-03-17 14:14 ` Markus Armbruster
2023-03-16 7:13 ` [PATCH 12/14] tests/qapi-schema: Cover optional conditional struct member Markus Armbruster
2023-03-16 7:45 ` Philippe Mathieu-Daudé
2023-03-16 7:13 ` [PATCH 13/14] qapi: Fix code generated for " Markus Armbruster
2023-03-17 1:13 ` Eric Blake
2023-03-16 7:13 ` [PATCH 14/14] qapi: Require boxed for conditional command and event arguments Markus Armbruster
2023-03-17 1:17 ` Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.