From: Marc-André Lureau <marcandre.lureau@redhat.com>
Modify check_if() to normalize the condition tree.
Add _make_if() to build a QAPISchemaIfCond tree.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Tested-by: John Snow <jsnow@redhat.com>
---
tests/unit/test-qmp-cmds.c | 1 +
scripts/qapi/expr.py | 51 +++++++++------
scripts/qapi/schema.py | 62 +++++++++++++------
tests/qapi-schema/bad-if-empty-list.json | 2 +-
tests/qapi-schema/bad-if-list.json | 2 +-
tests/qapi-schema/bad-if.err | 3 +-
tests/qapi-schema/doc-good.out | 12 ++--
tests/qapi-schema/enum-if-invalid.err | 3 +-
tests/qapi-schema/features-if-invalid.err | 2 +-
tests/qapi-schema/qapi-schema-test.json | 32 ++++++----
tests/qapi-schema/qapi-schema-test.out | 59 ++++++++++--------
.../qapi-schema/struct-member-if-invalid.err | 2 +-
.../qapi-schema/union-branch-if-invalid.json | 2 +-
13 files changed, 143 insertions(+), 90 deletions(-)
diff --git a/tests/unit/test-qmp-cmds.c b/tests/unit/test-qmp-cmds.c
index 1b0b7d99df..83efa39720 100644
--- a/tests/unit/test-qmp-cmds.c
+++ b/tests/unit/test-qmp-cmds.c
@@ -51,6 +51,7 @@ FeatureStruct1 *qmp_test_features0(bool has_fs0, FeatureStruct0 *fs0,
bool has_cfs1, CondFeatureStruct1 *cfs1,
bool has_cfs2, CondFeatureStruct2 *cfs2,
bool has_cfs3, CondFeatureStruct3 *cfs3,
+ bool has_cfs4, CondFeatureStruct4 *cfs4,
Error **errp)
{
return g_new0(FeatureStruct1, 1);
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 496f7e0333..60ffe9ef03 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -261,12 +261,12 @@ def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
"""
Normalize and validate the ``if`` member of an object.
- The ``if`` member may be either a ``str`` or a ``List[str]``.
- A ``str`` value will be normalized to ``List[str]``.
+ The ``if`` field may be either a ``str`` or a dict.
+ A ``str`` element will be normalized to ``{'all': List[str]}``.
:forms:
- :sugared: ``Union[str, List[str]]``
- :canonical: ``List[str]``
+ :sugared: ``Union[str, dict]``
+ :canonical: ``Union[str, dict]``
:param expr: The expression containing the ``if`` member to validate.
:param info: QAPI schema source file information.
@@ -281,25 +281,38 @@ def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
if ifcond is None:
return
- if isinstance(ifcond, list):
- if not ifcond:
- raise QAPISemError(
- info, "'if' condition [] of %s is useless" % source)
- else:
- # Normalize to a list
- ifcond = expr['if'] = [ifcond]
-
- for elt in ifcond:
- if not isinstance(elt, str):
+ def normalize(cond: Union[str, object]) -> Union[str, object]:
+ if isinstance(cond, str):
+ if not cond.strip():
+ raise QAPISemError(
+ info,
+ "'if' condition '%s' of %s makes no sense"
+ % (cond, source))
+ return cond
+ if not isinstance(cond, dict):
raise QAPISemError(
info,
- "'if' condition of %s must be a string or a list of strings"
- % source)
- if not elt.strip():
+ "'if' condition of %s must be a string or a dict" % source)
+ if len(cond) != 1:
raise QAPISemError(
info,
- "'if' condition '%s' of %s makes no sense"
- % (elt, source))
+ "'if' condition dict of %s must have one key: "
+ "'all', 'any' or 'not'" % source)
+ check_keys(cond, info, "'if' condition", [],
+ ["all", "any", "not"])
+ oper, operands = next(iter(cond.items()))
+ if not operands:
+ raise QAPISemError(
+ info, "'if' condition [] of %s is useless" % source)
+ if oper == "not":
+ return {oper: normalize(operands)}
+ if oper in ("all", "any") and not isinstance(operands, list):
+ raise QAPISemError(
+ info, "'%s' condition of %s must be a list" % (oper, source))
+ operands = [normalize(o) for o in operands]
+ return {oper: operands}
+
+ expr['if'] = normalize(ifcond)
def normalize_members(members: object) -> None:
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index f52caaeecc..9864e49c54 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -22,6 +22,8 @@
from .common import (
POINTER_SUFFIX,
IfAll,
+ IfAny,
+ IfNot,
IfOption,
c_name,
)
@@ -31,15 +33,14 @@
class QAPISchemaIfCond:
- def __init__(self, ifcond=None):
- self.ifcond = ifcond or []
- self.pred = IfAll([IfOption(opt) for opt in self.ifcond])
+ def __init__(self, pred=None):
+ self.pred = pred
def docgen(self):
- return self.pred.docgen()
+ return self and self.pred.docgen()
def cgen(self):
- return self.pred.cgen()
+ return self and self.pred.cgen()
# Returns true if the condition is not void
def __bool__(self):
@@ -991,16 +992,41 @@ def _def_predefineds(self):
self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
qtype_values, 'QTYPE'))
+ def _make_if(self, cond):
+ if isinstance(cond, QAPISchemaIfCond):
+ return cond
+ if cond is None:
+ return QAPISchemaIfCond()
+
+ def make_pred(node):
+ if isinstance(node, str):
+ return IfOption(node)
+ oper, operands = next(iter(node.items()))
+ op2cls = {
+ 'all': IfAll,
+ 'any': IfAny,
+ 'not': IfNot,
+ }
+
+ if isinstance(operands, list):
+ operands = [make_pred(o) for o in operands]
+ else:
+ operands = make_pred(operands)
+
+ return op2cls[oper](operands)
+
+ return QAPISchemaIfCond(make_pred(cond))
+
def _make_features(self, features, info):
if features is None:
return []
return [QAPISchemaFeature(f['name'], info,
- QAPISchemaIfCond(f.get('if')))
+ self._make_if(f.get('if')))
for f in features]
def _make_enum_members(self, values, info):
return [QAPISchemaEnumMember(v['name'], info,
- QAPISchemaIfCond(v.get('if')))
+ self._make_if(v.get('if')))
for v in values]
def _make_implicit_enum_type(self, name, info, ifcond, values):
@@ -1046,7 +1072,7 @@ def _def_enum_type(self, expr, info, doc):
name = expr['enum']
data = expr['data']
prefix = expr.get('prefix')
- ifcond = QAPISchemaIfCond(expr.get('if'))
+ ifcond = self._make_if(expr.get('if'))
features = self._make_features(expr.get('features'), info)
self._def_entity(QAPISchemaEnumType(
name, info, doc, ifcond, features,
@@ -1065,7 +1091,7 @@ def _make_member(self, name, typ, ifcond, features, info):
def _make_members(self, data, info):
return [self._make_member(key, value['type'],
- QAPISchemaIfCond(value.get('if')),
+ self._make_if(value.get('if')),
value.get('features'), info)
for (key, value) in data.items()]
@@ -1073,7 +1099,7 @@ def _def_struct_type(self, expr, info, doc):
name = expr['struct']
base = expr.get('base')
data = expr['data']
- ifcond = QAPISchemaIfCond(expr.get('if'))
+ ifcond = self._make_if(expr.get('if'))
features = self._make_features(expr.get('features'), info)
self._def_entity(QAPISchemaObjectType(
name, info, doc, ifcond, features, base,
@@ -1096,7 +1122,7 @@ def _def_union_type(self, expr, info, doc):
name = expr['union']
data = expr['data']
base = expr.get('base')
- ifcond = QAPISchemaIfCond(expr.get('if'))
+ ifcond = self._make_if(expr.get('if'))
features = self._make_features(expr.get('features'), info)
tag_name = expr.get('discriminator')
tag_member = None
@@ -1107,7 +1133,7 @@ def _def_union_type(self, expr, info, doc):
if tag_name:
variants = [
self._make_variant(key, value['type'],
- QAPISchemaIfCond(value.get('if')),
+ self._make_if(value.get('if')),
info)
for (key, value) in data.items()
]
@@ -1115,11 +1141,11 @@ def _def_union_type(self, expr, info, doc):
else:
variants = [
self._make_simple_variant(key, value['type'],
- QAPISchemaIfCond(value.get('if')),
+ self._make_if(value.get('if')),
info)
for (key, value) in data.items()
]
- enum = [{'name': v.name, 'if': v.ifcond.ifcond} for v in variants]
+ enum = [{'name': v.name, 'if': v.ifcond} for v in variants]
typ = self._make_implicit_enum_type(name, info, ifcond, enum)
tag_member = QAPISchemaObjectTypeMember('type', info, typ, False)
members = [tag_member]
@@ -1132,11 +1158,11 @@ def _def_union_type(self, expr, info, doc):
def _def_alternate_type(self, expr, info, doc):
name = expr['alternate']
data = expr['data']
- ifcond = QAPISchemaIfCond(expr.get('if'))
+ ifcond = self._make_if(expr.get('if'))
features = self._make_features(expr.get('features'), info)
variants = [
self._make_variant(key, value['type'],
- QAPISchemaIfCond(value.get('if')),
+ self._make_if(value.get('if')),
info)
for (key, value) in data.items()
]
@@ -1156,7 +1182,7 @@ def _def_command(self, expr, info, doc):
allow_oob = expr.get('allow-oob', False)
allow_preconfig = expr.get('allow-preconfig', False)
coroutine = expr.get('coroutine', False)
- ifcond = QAPISchemaIfCond(expr.get('if'))
+ ifcond = self._make_if(expr.get('if'))
features = self._make_features(expr.get('features'), info)
if isinstance(data, OrderedDict):
data = self._make_implicit_object_type(
@@ -1175,7 +1201,7 @@ def _def_event(self, expr, info, doc):
name = expr['event']
data = expr.get('data')
boxed = expr.get('boxed', False)
- ifcond = QAPISchemaIfCond(expr.get('if'))
+ ifcond = self._make_if(expr.get('if'))
features = self._make_features(expr.get('features'), info)
if isinstance(data, OrderedDict):
data = self._make_implicit_object_type(
diff --git a/tests/qapi-schema/bad-if-empty-list.json b/tests/qapi-schema/bad-if-empty-list.json
index 94f2eb8670..b62b5671df 100644
--- a/tests/qapi-schema/bad-if-empty-list.json
+++ b/tests/qapi-schema/bad-if-empty-list.json
@@ -1,3 +1,3 @@
# check empty 'if' list
{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
- 'if': [] }
+ 'if': { 'all': [] } }
diff --git a/tests/qapi-schema/bad-if-list.json b/tests/qapi-schema/bad-if-list.json
index ea3d95bb6b..1fefef16a7 100644
--- a/tests/qapi-schema/bad-if-list.json
+++ b/tests/qapi-schema/bad-if-list.json
@@ -1,3 +1,3 @@
# check invalid 'if' content
{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
- 'if': ['foo', ' '] }
+ 'if': { 'all': ['foo', ' '] } }
diff --git a/tests/qapi-schema/bad-if.err b/tests/qapi-schema/bad-if.err
index f83dee65da..454fbae387 100644
--- a/tests/qapi-schema/bad-if.err
+++ b/tests/qapi-schema/bad-if.err
@@ -1,2 +1,3 @@
bad-if.json: In struct 'TestIfStruct':
-bad-if.json:2: 'if' condition of struct must be a string or a list of strings
+bad-if.json:2: 'if' condition has unknown key 'value'
+Valid keys are 'all', 'any', 'not'.
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index db1d23c6bf..4d951f97ee 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -12,15 +12,15 @@ enum QType
module doc-good.json
enum Enum
member one
- if IfAll([IfOption('defined(IFONE)')])
+ if IfOption('defined(IFONE)')
member two
- if IfAll([IfOption('defined(IFCOND)')])
+ if IfOption('defined(IFCOND)')
feature enum-feat
object Base
member base1: Enum optional=False
object Variant1
member var1: str optional=False
- if IfAll([IfOption('defined(IFSTR)')])
+ if IfOption('defined(IFSTR)')
feature member-feat
feature variant1-feat
object Variant2
@@ -29,7 +29,7 @@ object Object
tag base1
case one: Variant1
case two: Variant2
- if IfAll([IfOption('IFTWO')])
+ if IfOption('IFTWO')
feature union-feat1
object q_obj_Variant1-wrapper
member data: Variant1 optional=False
@@ -38,13 +38,13 @@ object q_obj_Variant2-wrapper
enum SugaredUnionKind
member one
member two
- if IfAll([IfOption('IFTWO')])
+ if IfOption('IFTWO')
object SugaredUnion
member type: SugaredUnionKind optional=False
tag type
case one: q_obj_Variant1-wrapper
case two: q_obj_Variant2-wrapper
- if IfAll([IfOption('IFTWO')])
+ if IfOption('IFTWO')
feature union-feat2
alternate Alternate
tag type
diff --git a/tests/qapi-schema/enum-if-invalid.err b/tests/qapi-schema/enum-if-invalid.err
index 0556dc967b..3bb84075a9 100644
--- a/tests/qapi-schema/enum-if-invalid.err
+++ b/tests/qapi-schema/enum-if-invalid.err
@@ -1,2 +1,3 @@
enum-if-invalid.json: In enum 'TestIfEnum':
-enum-if-invalid.json:2: 'if' condition of 'data' member 'bar' must be a string or a list of strings
+enum-if-invalid.json:2: 'if' condition has unknown key 'val'
+Valid keys are 'all', 'any', 'not'.
diff --git a/tests/qapi-schema/features-if-invalid.err b/tests/qapi-schema/features-if-invalid.err
index f63b89535e..8b64318df6 100644
--- a/tests/qapi-schema/features-if-invalid.err
+++ b/tests/qapi-schema/features-if-invalid.err
@@ -1,2 +1,2 @@
features-if-invalid.json: In struct 'Stru':
-features-if-invalid.json:2: 'if' condition of 'features' member 'f' must be a string or a list of strings
+features-if-invalid.json:2: 'if' condition of 'features' member 'f' must be a string or a dict
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 84b9d41f15..a0f09b277e 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -231,8 +231,8 @@
{ 'union': 'TestIfUnion', 'data':
{ 'foo': 'TestStruct',
- 'bar': { 'type': 'str', 'if': 'defined(TEST_IF_UNION_BAR)'} },
- 'if': 'defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)' }
+ 'union-bar': { 'type': 'str', 'if': 'defined(TEST_IF_UNION_BAR)'} },
+ 'if': { 'all': ['defined(TEST_IF_UNION)', 'defined(TEST_IF_STRUCT)'] } }
{ 'command': 'test-if-union-cmd',
'data': { 'union-cmd-arg': 'TestIfUnion' },
@@ -241,25 +241,24 @@
{ 'alternate': 'TestIfAlternate', 'data':
{ 'foo': 'int',
'bar': { 'type': 'TestStruct', 'if': 'defined(TEST_IF_ALT_BAR)'} },
- 'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }
+ 'if': { 'all': ['defined(TEST_IF_ALT)', 'defined(TEST_IF_STRUCT)'] } }
-{ 'command': 'test-if-alternate-cmd',
- 'data': { 'alt-cmd-arg': 'TestIfAlternate' },
- 'if': 'defined(TEST_IF_ALT)' }
+{ 'command': 'test-if-alternate-cmd', 'data': { 'alt-cmd-arg': 'TestIfAlternate' },
+ 'if': { 'all': ['defined(TEST_IF_ALT)', {'not': 'defined(TEST_IF_NOT_ALT)'}] } }
{ 'command': 'test-if-cmd',
'data': {
'foo': 'TestIfStruct',
'bar': { 'type': 'TestIfEnum', 'if': 'defined(TEST_IF_CMD_BAR)' } },
'returns': 'UserDefThree',
- 'if': ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'] }
+ 'if': { 'all': ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)'] } }
{ 'command': 'test-cmd-return-def-three', 'returns': 'UserDefThree' }
{ 'event': 'TEST_IF_EVENT', 'data':
{ 'foo': 'TestIfStruct',
'bar': { 'type': ['TestIfEnum'], 'if': 'defined(TEST_IF_EVT_BAR)' } },
- 'if': 'defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)' }
+ 'if': { 'all': ['defined(TEST_IF_EVT)', 'defined(TEST_IF_STRUCT)'] } }
# test 'features'
@@ -288,8 +287,13 @@
{ 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'} ] }
{ 'struct': 'CondFeatureStruct3',
'data': { 'foo': 'int' },
- 'features': [ { 'name': 'feature1', 'if': [ 'defined(TEST_IF_COND_1)',
- 'defined(TEST_IF_COND_2)'] } ] }
+ 'features': [ { 'name': 'feature1',
+ 'if': { 'all': [ 'defined(TEST_IF_COND_1)',
+ 'defined(TEST_IF_COND_2)'] } } ] }
+{ 'struct': 'CondFeatureStruct4',
+ 'data': { 'foo': 'int' },
+ 'features': [ { 'name': 'feature1', 'if': {'any': ['defined(TEST_IF_COND_1)',
+ 'defined(TEST_IF_COND_2)'] } } ] }
{ 'enum': 'FeatureEnum1',
'data': [ 'eins', 'zwei', 'drei' ],
@@ -313,7 +317,8 @@
'*fs4': 'FeatureStruct4',
'*cfs1': 'CondFeatureStruct1',
'*cfs2': 'CondFeatureStruct2',
- '*cfs3': 'CondFeatureStruct3' },
+ '*cfs3': 'CondFeatureStruct3',
+ '*cfs4': 'CondFeatureStruct4' },
'returns': 'FeatureStruct1',
'features': [] }
@@ -328,8 +333,9 @@
'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'},
{ 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'} ] }
{ 'command': 'test-command-cond-features3',
- 'features': [ { 'name': 'feature1', 'if': [ 'defined(TEST_IF_COND_1)',
- 'defined(TEST_IF_COND_2)'] } ] }
+ 'features': [ { 'name': 'feature1',
+ 'if': { 'all': [ 'defined(TEST_IF_COND_1)',
+ 'defined(TEST_IF_COND_2)'] } } ] }
{ 'event': 'TEST_EVENT_FEATURES0',
'data': 'FeatureStruct1' }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index e4e0fb173a..dd2f83fbae 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -298,49 +298,49 @@ command __org.qemu_x-command q_obj___org.qemu_x-command-arg -> __org.qemu_x-Unio
object TestIfStruct
member foo: int optional=False
member bar: int optional=False
- if IfAll([IfOption('defined(TEST_IF_STRUCT_BAR)')])
- if IfAll([IfOption('defined(TEST_IF_STRUCT)')])
+ if IfOption('defined(TEST_IF_STRUCT_BAR)')
+ if IfOption('defined(TEST_IF_STRUCT)')
enum TestIfEnum
member foo
member bar
- if IfAll([IfOption('defined(TEST_IF_ENUM_BAR)')])
- if IfAll([IfOption('defined(TEST_IF_ENUM)')])
+ if IfOption('defined(TEST_IF_ENUM_BAR)')
+ if IfOption('defined(TEST_IF_ENUM)')
object q_obj_TestStruct-wrapper
member data: TestStruct optional=False
enum TestIfUnionKind
member foo
- member bar
- if IfAll([IfOption('defined(TEST_IF_UNION_BAR)')])
- if IfAll([IfOption('defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)')])
+ member union-bar
+ if IfOption('defined(TEST_IF_UNION_BAR)')
+ if IfAll([IfOption('defined(TEST_IF_UNION)'), IfOption('defined(TEST_IF_STRUCT)')])
object TestIfUnion
member type: TestIfUnionKind optional=False
tag type
case foo: q_obj_TestStruct-wrapper
- case bar: q_obj_str-wrapper
- if IfAll([IfOption('defined(TEST_IF_UNION_BAR)')])
- if IfAll([IfOption('defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)')])
+ case union-bar: q_obj_str-wrapper
+ if IfOption('defined(TEST_IF_UNION_BAR)')
+ if IfAll([IfOption('defined(TEST_IF_UNION)'), IfOption('defined(TEST_IF_STRUCT)')])
object q_obj_test-if-union-cmd-arg
member union-cmd-arg: TestIfUnion optional=False
- if IfAll([IfOption('defined(TEST_IF_UNION)')])
+ if IfOption('defined(TEST_IF_UNION)')
command test-if-union-cmd q_obj_test-if-union-cmd-arg -> None
gen=True success_response=True boxed=False oob=False preconfig=False
- if IfAll([IfOption('defined(TEST_IF_UNION)')])
+ if IfOption('defined(TEST_IF_UNION)')
alternate TestIfAlternate
tag type
case foo: int
case bar: TestStruct
- if IfAll([IfOption('defined(TEST_IF_ALT_BAR)')])
- if IfAll([IfOption('defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)')])
+ if IfOption('defined(TEST_IF_ALT_BAR)')
+ if IfAll([IfOption('defined(TEST_IF_ALT)'), IfOption('defined(TEST_IF_STRUCT)')])
object q_obj_test-if-alternate-cmd-arg
member alt-cmd-arg: TestIfAlternate optional=False
- if IfAll([IfOption('defined(TEST_IF_ALT)')])
+ if IfAll([IfOption('defined(TEST_IF_ALT)'), IfNot(IfOption('defined(TEST_IF_NOT_ALT)'))])
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 IfAll([IfOption('defined(TEST_IF_ALT)')])
+ if IfAll([IfOption('defined(TEST_IF_ALT)'), IfNot(IfOption('defined(TEST_IF_NOT_ALT)'))])
object q_obj_test-if-cmd-arg
member foo: TestIfStruct optional=False
member bar: TestIfEnum optional=False
- if IfAll([IfOption('defined(TEST_IF_CMD_BAR)')])
+ if IfOption('defined(TEST_IF_CMD_BAR)')
if IfAll([IfOption('defined(TEST_IF_CMD)'), IfOption('defined(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
@@ -348,15 +348,15 @@ command test-if-cmd q_obj_test-if-cmd-arg -> UserDefThree
command test-cmd-return-def-three None -> UserDefThree
gen=True success_response=True boxed=False oob=False preconfig=False
array TestIfEnumList TestIfEnum
- if IfAll([IfOption('defined(TEST_IF_ENUM)')])
+ if IfOption('defined(TEST_IF_ENUM)')
object q_obj_TEST_IF_EVENT-arg
member foo: TestIfStruct optional=False
member bar: TestIfEnumList optional=False
- if IfAll([IfOption('defined(TEST_IF_EVT_BAR)')])
- if IfAll([IfOption('defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)')])
+ if IfOption('defined(TEST_IF_EVT_BAR)')
+ if IfAll([IfOption('defined(TEST_IF_EVT)'), IfOption('defined(TEST_IF_STRUCT)')])
event TEST_IF_EVENT q_obj_TEST_IF_EVENT-arg
boxed=False
- if IfAll([IfOption('defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)')])
+ if IfAll([IfOption('defined(TEST_IF_EVT)'), IfOption('defined(TEST_IF_STRUCT)')])
object FeatureStruct0
member foo: int optional=False
object FeatureStruct1
@@ -379,17 +379,21 @@ object FeatureStruct4
object CondFeatureStruct1
member foo: int optional=False
feature feature1
- if IfAll([IfOption('defined(TEST_IF_FEATURE_1)')])
+ if IfOption('defined(TEST_IF_FEATURE_1)')
object CondFeatureStruct2
member foo: int optional=False
feature feature1
- if IfAll([IfOption('defined(TEST_IF_FEATURE_1)')])
+ if IfOption('defined(TEST_IF_FEATURE_1)')
feature feature2
- if IfAll([IfOption('defined(TEST_IF_FEATURE_2)')])
+ if IfOption('defined(TEST_IF_FEATURE_2)')
object CondFeatureStruct3
member foo: int optional=False
feature feature1
if IfAll([IfOption('defined(TEST_IF_COND_1)'), IfOption('defined(TEST_IF_COND_2)')])
+object CondFeatureStruct4
+ member foo: int optional=False
+ feature feature1
+ if IfAny([IfOption('defined(TEST_IF_COND_1)'), IfOption('defined(TEST_IF_COND_2)')])
enum FeatureEnum1
member eins
member zwei
@@ -417,6 +421,7 @@ object q_obj_test-features0-arg
member cfs1: CondFeatureStruct1 optional=True
member cfs2: CondFeatureStruct2 optional=True
member cfs3: CondFeatureStruct3 optional=True
+ member cfs4: CondFeatureStruct4 optional=True
command test-features0 q_obj_test-features0-arg -> FeatureStruct1
gen=True success_response=True boxed=False oob=False preconfig=False
command test-command-features1 None -> None
@@ -429,13 +434,13 @@ command test-command-features3 None -> None
command test-command-cond-features1 None -> None
gen=True success_response=True boxed=False oob=False preconfig=False
feature feature1
- if IfAll([IfOption('defined(TEST_IF_FEATURE_1)')])
+ if IfOption('defined(TEST_IF_FEATURE_1)')
command test-command-cond-features2 None -> None
gen=True success_response=True boxed=False oob=False preconfig=False
feature feature1
- if IfAll([IfOption('defined(TEST_IF_FEATURE_1)')])
+ if IfOption('defined(TEST_IF_FEATURE_1)')
feature feature2
- if IfAll([IfOption('defined(TEST_IF_FEATURE_2)')])
+ if IfOption('defined(TEST_IF_FEATURE_2)')
command test-command-cond-features3 None -> None
gen=True success_response=True boxed=False oob=False preconfig=False
feature feature1
diff --git a/tests/qapi-schema/struct-member-if-invalid.err b/tests/qapi-schema/struct-member-if-invalid.err
index 42e7fdae3c..eea4c62aaf 100644
--- a/tests/qapi-schema/struct-member-if-invalid.err
+++ b/tests/qapi-schema/struct-member-if-invalid.err
@@ -1,2 +1,2 @@
struct-member-if-invalid.json: In struct 'Stru':
-struct-member-if-invalid.json:2: 'if' condition of 'data' member 'member' must be a string or a list of strings
+struct-member-if-invalid.json:2: 'if' condition of 'data' member 'member' must be a string or a dict
diff --git a/tests/qapi-schema/union-branch-if-invalid.json b/tests/qapi-schema/union-branch-if-invalid.json
index 46d4239af6..c41633856f 100644
--- a/tests/qapi-schema/union-branch-if-invalid.json
+++ b/tests/qapi-schema/union-branch-if-invalid.json
@@ -3,4 +3,4 @@
{ 'struct': 'Stru', 'data': { 'member': 'str' } }
{ 'union': 'Uni',
'base': { 'tag': 'Branches' }, 'discriminator': 'tag',
- 'data': { 'branch1': { 'type': 'Stru', 'if': [''] } } }
+ 'data': { 'branch1': { 'type': 'Stru', 'if': { 'all': [''] } } } }
--
2.29.0
marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Modify check_if() to normalize the condition tree.
How is it normalized? Let me rephrase my question: how does the IR
change? If the generated code changes, how?
> Add _make_if() to build a QAPISchemaIfCond tree.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Tested-by: John Snow <jsnow@redhat.com>
> ---
> tests/unit/test-qmp-cmds.c | 1 +
> scripts/qapi/expr.py | 51 +++++++++------
> scripts/qapi/schema.py | 62 +++++++++++++------
> tests/qapi-schema/bad-if-empty-list.json | 2 +-
> tests/qapi-schema/bad-if-list.json | 2 +-
> tests/qapi-schema/bad-if.err | 3 +-
> tests/qapi-schema/doc-good.out | 12 ++--
> tests/qapi-schema/enum-if-invalid.err | 3 +-
> tests/qapi-schema/features-if-invalid.err | 2 +-
> tests/qapi-schema/qapi-schema-test.json | 32 ++++++----
> tests/qapi-schema/qapi-schema-test.out | 59 ++++++++++--------
> .../qapi-schema/struct-member-if-invalid.err | 2 +-
> .../qapi-schema/union-branch-if-invalid.json | 2 +-
> 13 files changed, 143 insertions(+), 90 deletions(-)
>
> diff --git a/tests/unit/test-qmp-cmds.c b/tests/unit/test-qmp-cmds.c
> index 1b0b7d99df..83efa39720 100644
> --- a/tests/unit/test-qmp-cmds.c
> +++ b/tests/unit/test-qmp-cmds.c
> @@ -51,6 +51,7 @@ FeatureStruct1 *qmp_test_features0(bool has_fs0, FeatureStruct0 *fs0,
> bool has_cfs1, CondFeatureStruct1 *cfs1,
> bool has_cfs2, CondFeatureStruct2 *cfs2,
> bool has_cfs3, CondFeatureStruct3 *cfs3,
> + bool has_cfs4, CondFeatureStruct4 *cfs4,
> Error **errp)
> {
> return g_new0(FeatureStruct1, 1);
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 496f7e0333..60ffe9ef03 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -261,12 +261,12 @@ def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
> """
> Normalize and validate the ``if`` member of an object.
>
> - The ``if`` member may be either a ``str`` or a ``List[str]``.
> - A ``str`` value will be normalized to ``List[str]``.
Double-checking: is this doc comment correct before this patch?
> + The ``if`` field may be either a ``str`` or a dict.
> + A ``str`` element will be normalized to ``{'all': List[str]}``.
>
> :forms:
> - :sugared: ``Union[str, List[str]]``
> - :canonical: ``List[str]``
> + :sugared: ``Union[str, dict]``
> + :canonical: ``Union[str, dict]``
>
> :param expr: The expression containing the ``if`` member to validate.
> :param info: QAPI schema source file information.
> @@ -281,25 +281,38 @@ def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
> if ifcond is None:
> return
>
> - if isinstance(ifcond, list):
> - if not ifcond:
> - raise QAPISemError(
> - info, "'if' condition [] of %s is useless" % source)
> - else:
> - # Normalize to a list
> - ifcond = expr['if'] = [ifcond]
> -
> - for elt in ifcond:
> - if not isinstance(elt, str):
> + def normalize(cond: Union[str, object]) -> Union[str, object]:
This definition is in the middle of check_if()'s body. Intentional?
> + if isinstance(cond, str):
> + if not cond.strip():
> + raise QAPISemError(
> + info,
> + "'if' condition '%s' of %s makes no sense"
> + % (cond, source))
> + return cond
> + if not isinstance(cond, dict):
> raise QAPISemError(
> info,
> - "'if' condition of %s must be a string or a list of strings"
> - % source)
> - if not elt.strip():
> + "'if' condition of %s must be a string or a dict" % source)
> + if len(cond) != 1:
> raise QAPISemError(
> info,
> - "'if' condition '%s' of %s makes no sense"
> - % (elt, source))
> + "'if' condition dict of %s must have one key: "
Exactly one key, to be precise.
> + "'all', 'any' or 'not'" % source)
> + check_keys(cond, info, "'if' condition", [],
> + ["all", "any", "not"])
Hmmm. Getting members of @cond before check_keys() would be wrong, but
you don't do that. Still, I like to call check_keys() early, just to
reduce the chance for accidents.
If we check_keys() first, we're left with just two possible errors:
empty dict (len(cond)==0), and conflicting keys (len(cond)>1). We could
choose to diagnose them separately, but it's probably not worth the
bother.
> + oper, operands = next(iter(cond.items()))
> + if not operands:
> + raise QAPISemError(
> + info, "'if' condition [] of %s is useless" % source)
> + if oper == "not":
> + return {oper: normalize(operands)}
> + if oper in ("all", "any") and not isinstance(operands, list):
> + raise QAPISemError(
> + info, "'%s' condition of %s must be a list" % (oper, source))
> + operands = [normalize(o) for o in operands]
> + return {oper: operands}
I guess making this a function enables writing
return NE
instead of
expr['if] = NE
return
which is slightly more compact, and factors out the assignment's left
hand side. Feels like a wash, but up to you.
> +
> + expr['if'] = normalize(ifcond)
>
>
> def normalize_members(members: object) -> None:
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index f52caaeecc..9864e49c54 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -22,6 +22,8 @@
> from .common import (
> POINTER_SUFFIX,
> IfAll,
> + IfAny,
> + IfNot,
> IfOption,
> c_name,
> )
> @@ -31,15 +33,14 @@
>
>
> class QAPISchemaIfCond:
> - def __init__(self, ifcond=None):
> - self.ifcond = ifcond or []
> - self.pred = IfAll([IfOption(opt) for opt in self.ifcond])
> + def __init__(self, pred=None):
> + self.pred = pred
>
> def docgen(self):
> - return self.pred.docgen()
> + return self and self.pred.docgen()
The more code relying on your __bool__() methods I see, the less I like
them.
Can we do self.pred and self.pred.docgen()?
>
> def cgen(self):
> - return self.pred.cgen()
> + return self and self.pred.cgen()
Likewise.
>
> # Returns true if the condition is not void
> def __bool__(self):
> @@ -991,16 +992,41 @@ def _def_predefineds(self):
> self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
> qtype_values, 'QTYPE'))
>
> + def _make_if(self, cond):
> + if isinstance(cond, QAPISchemaIfCond):
> + return cond
> + if cond is None:
> + return QAPISchemaIfCond()
> +
> + def make_pred(node):
> + if isinstance(node, str):
> + return IfOption(node)
> + oper, operands = next(iter(node.items()))
> + op2cls = {
> + 'all': IfAll,
> + 'any': IfAny,
> + 'not': IfNot,
> + }
> +
> + if isinstance(operands, list):
> + operands = [make_pred(o) for o in operands]
> + else:
> + operands = make_pred(operands)
> +
> + return op2cls[oper](operands)
> +
> + return QAPISchemaIfCond(make_pred(cond))
Maybe it's the weather, but I find this pretty impenetrable right now.
make_if() appears to accept either QAPISchemaIfCond, None, or a tree
whose inner nodes are {'any': List[tree]}, {'all': List[tree]}, {'not':
tree}, or str. Quite the omnivore.
None of the callers I can see pass QAPISchemaIfCond. Am I confused?
make_pred() appears to accept the tree. The part dealing with str is
obvious.
next(iter(node.items())) appears to map a dict {key: val} to a tuple
(key, val). Too clever by half.
val, and thus @operands then is either a list of trees (all, any), or a
tree (not).
The tree(s) in @operands get recursively processed. Now @operands is
either a List[IfPredicate], or an IfPredicate.
IfAll and IfAny take the former, IfNot takes the latter. Works out
(*quack*), but I'm not sure mypy will be happy with it.
> +
> def _make_features(self, features, info):
> if features is None:
> return []
> return [QAPISchemaFeature(f['name'], info,
> - QAPISchemaIfCond(f.get('if')))
> + self._make_if(f.get('if')))
> for f in features]
>
> def _make_enum_members(self, values, info):
> return [QAPISchemaEnumMember(v['name'], info,
> - QAPISchemaIfCond(v.get('if')))
> + self._make_if(v.get('if')))
> for v in values]
>
> def _make_implicit_enum_type(self, name, info, ifcond, values):
> @@ -1046,7 +1072,7 @@ def _def_enum_type(self, expr, info, doc):
> name = expr['enum']
> data = expr['data']
> prefix = expr.get('prefix')
> - ifcond = QAPISchemaIfCond(expr.get('if'))
> + ifcond = self._make_if(expr.get('if'))
> features = self._make_features(expr.get('features'), info)
> self._def_entity(QAPISchemaEnumType(
> name, info, doc, ifcond, features,
> @@ -1065,7 +1091,7 @@ def _make_member(self, name, typ, ifcond, features, info):
>
> def _make_members(self, data, info):
> return [self._make_member(key, value['type'],
> - QAPISchemaIfCond(value.get('if')),
> + self._make_if(value.get('if')),
> value.get('features'), info)
> for (key, value) in data.items()]
>
> @@ -1073,7 +1099,7 @@ def _def_struct_type(self, expr, info, doc):
> name = expr['struct']
> base = expr.get('base')
> data = expr['data']
> - ifcond = QAPISchemaIfCond(expr.get('if'))
> + ifcond = self._make_if(expr.get('if'))
> features = self._make_features(expr.get('features'), info)
> self._def_entity(QAPISchemaObjectType(
> name, info, doc, ifcond, features, base,
> @@ -1096,7 +1122,7 @@ def _def_union_type(self, expr, info, doc):
> name = expr['union']
> data = expr['data']
> base = expr.get('base')
> - ifcond = QAPISchemaIfCond(expr.get('if'))
> + ifcond = self._make_if(expr.get('if'))
> features = self._make_features(expr.get('features'), info)
> tag_name = expr.get('discriminator')
> tag_member = None
> @@ -1107,7 +1133,7 @@ def _def_union_type(self, expr, info, doc):
> if tag_name:
> variants = [
> self._make_variant(key, value['type'],
> - QAPISchemaIfCond(value.get('if')),
> + self._make_if(value.get('if')),
> info)
> for (key, value) in data.items()
> ]
> @@ -1115,11 +1141,11 @@ def _def_union_type(self, expr, info, doc):
> else:
> variants = [
> self._make_simple_variant(key, value['type'],
> - QAPISchemaIfCond(value.get('if')),
> + self._make_if(value.get('if')),
> info)
> for (key, value) in data.items()
> ]
> - enum = [{'name': v.name, 'if': v.ifcond.ifcond} for v in variants]
> + enum = [{'name': v.name, 'if': v.ifcond} for v in variants]
Another riddle for me to solve?
> typ = self._make_implicit_enum_type(name, info, ifcond, enum)
> tag_member = QAPISchemaObjectTypeMember('type', info, typ, False)
> members = [tag_member]
> @@ -1132,11 +1158,11 @@ def _def_union_type(self, expr, info, doc):
> def _def_alternate_type(self, expr, info, doc):
> name = expr['alternate']
> data = expr['data']
> - ifcond = QAPISchemaIfCond(expr.get('if'))
> + ifcond = self._make_if(expr.get('if'))
> features = self._make_features(expr.get('features'), info)
> variants = [
> self._make_variant(key, value['type'],
> - QAPISchemaIfCond(value.get('if')),
> + self._make_if(value.get('if')),
> info)
> for (key, value) in data.items()
> ]
> @@ -1156,7 +1182,7 @@ def _def_command(self, expr, info, doc):
> allow_oob = expr.get('allow-oob', False)
> allow_preconfig = expr.get('allow-preconfig', False)
> coroutine = expr.get('coroutine', False)
> - ifcond = QAPISchemaIfCond(expr.get('if'))
> + ifcond = self._make_if(expr.get('if'))
> features = self._make_features(expr.get('features'), info)
> if isinstance(data, OrderedDict):
> data = self._make_implicit_object_type(
> @@ -1175,7 +1201,7 @@ def _def_event(self, expr, info, doc):
> name = expr['event']
> data = expr.get('data')
> boxed = expr.get('boxed', False)
> - ifcond = QAPISchemaIfCond(expr.get('if'))
> + ifcond = self._make_if(expr.get('if'))
> features = self._make_features(expr.get('features'), info)
> if isinstance(data, OrderedDict):
> data = self._make_implicit_object_type(
> diff --git a/tests/qapi-schema/bad-if-empty-list.json b/tests/qapi-schema/bad-if-empty-list.json
> index 94f2eb8670..b62b5671df 100644
> --- a/tests/qapi-schema/bad-if-empty-list.json
> +++ b/tests/qapi-schema/bad-if-empty-list.json
> @@ -1,3 +1,3 @@
> # check empty 'if' list
> { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
> - 'if': [] }
> + 'if': { 'all': [] } }
> diff --git a/tests/qapi-schema/bad-if-list.json b/tests/qapi-schema/bad-if-list.json
> index ea3d95bb6b..1fefef16a7 100644
> --- a/tests/qapi-schema/bad-if-list.json
> +++ b/tests/qapi-schema/bad-if-list.json
> @@ -1,3 +1,3 @@
> # check invalid 'if' content
> { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
> - 'if': ['foo', ' '] }
> + 'if': { 'all': ['foo', ' '] } }
> diff --git a/tests/qapi-schema/bad-if.err b/tests/qapi-schema/bad-if.err
> index f83dee65da..454fbae387 100644
> --- a/tests/qapi-schema/bad-if.err
> +++ b/tests/qapi-schema/bad-if.err
> @@ -1,2 +1,3 @@
> bad-if.json: In struct 'TestIfStruct':
> -bad-if.json:2: 'if' condition of struct must be a string or a list of strings
> +bad-if.json:2: 'if' condition has unknown key 'value'
> +Valid keys are 'all', 'any', 'not'.
> diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
> index db1d23c6bf..4d951f97ee 100644
> --- a/tests/qapi-schema/doc-good.out
> +++ b/tests/qapi-schema/doc-good.out
> @@ -12,15 +12,15 @@ enum QType
> module doc-good.json
> enum Enum
> member one
> - if IfAll([IfOption('defined(IFONE)')])
> + if IfOption('defined(IFONE)')
> member two
> - if IfAll([IfOption('defined(IFCOND)')])
> + if IfOption('defined(IFCOND)')
> feature enum-feat
> object Base
> member base1: Enum optional=False
> object Variant1
> member var1: str optional=False
> - if IfAll([IfOption('defined(IFSTR)')])
> + if IfOption('defined(IFSTR)')
> feature member-feat
> feature variant1-feat
> object Variant2
> @@ -29,7 +29,7 @@ object Object
> tag base1
> case one: Variant1
> case two: Variant2
> - if IfAll([IfOption('IFTWO')])
> + if IfOption('IFTWO')
> feature union-feat1
> object q_obj_Variant1-wrapper
> member data: Variant1 optional=False
> @@ -38,13 +38,13 @@ object q_obj_Variant2-wrapper
> enum SugaredUnionKind
> member one
> member two
> - if IfAll([IfOption('IFTWO')])
> + if IfOption('IFTWO')
> object SugaredUnion
> member type: SugaredUnionKind optional=False
> tag type
> case one: q_obj_Variant1-wrapper
> case two: q_obj_Variant2-wrapper
> - if IfAll([IfOption('IFTWO')])
> + if IfOption('IFTWO')
> feature union-feat2
> alternate Alternate
> tag type
[...]
Skipping the tests for now because I'm running out of brain-juice.
Hi
On Tue, Jun 15, 2021 at 3:34 PM Markus Armbruster <armbru@redhat.com> wrote:
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Modify check_if() to normalize the condition tree.
>
> How is it normalized? Let me rephrase my question: how does the IR
> change? If the generated code changes, how?
>
Not anymore, since we dropped the sugar form. Updated in v6.
> > Add _make_if() to build a QAPISchemaIfCond tree.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Tested-by: John Snow <jsnow@redhat.com>
> > ---
> > tests/unit/test-qmp-cmds.c | 1 +
> > scripts/qapi/expr.py | 51 +++++++++------
> > scripts/qapi/schema.py | 62 +++++++++++++------
> > tests/qapi-schema/bad-if-empty-list.json | 2 +-
> > tests/qapi-schema/bad-if-list.json | 2 +-
> > tests/qapi-schema/bad-if.err | 3 +-
> > tests/qapi-schema/doc-good.out | 12 ++--
> > tests/qapi-schema/enum-if-invalid.err | 3 +-
> > tests/qapi-schema/features-if-invalid.err | 2 +-
> > tests/qapi-schema/qapi-schema-test.json | 32 ++++++----
> > tests/qapi-schema/qapi-schema-test.out | 59 ++++++++++--------
> > .../qapi-schema/struct-member-if-invalid.err | 2 +-
> > .../qapi-schema/union-branch-if-invalid.json | 2 +-
> > 13 files changed, 143 insertions(+), 90 deletions(-)
> >
> > diff --git a/tests/unit/test-qmp-cmds.c b/tests/unit/test-qmp-cmds.c
> > index 1b0b7d99df..83efa39720 100644
> > --- a/tests/unit/test-qmp-cmds.c
> > +++ b/tests/unit/test-qmp-cmds.c
> > @@ -51,6 +51,7 @@ FeatureStruct1 *qmp_test_features0(bool has_fs0,
> FeatureStruct0 *fs0,
> > bool has_cfs1, CondFeatureStruct1
> *cfs1,
> > bool has_cfs2, CondFeatureStruct2
> *cfs2,
> > bool has_cfs3, CondFeatureStruct3
> *cfs3,
> > + bool has_cfs4, CondFeatureStruct4
> *cfs4,
> > Error **errp)
> > {
> > return g_new0(FeatureStruct1, 1);
> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > index 496f7e0333..60ffe9ef03 100644
> > --- a/scripts/qapi/expr.py
> > +++ b/scripts/qapi/expr.py
> > @@ -261,12 +261,12 @@ def check_if(expr: _JSONObject, info:
> QAPISourceInfo, source: str) -> None:
> > """
> > Normalize and validate the ``if`` member of an object.
> >
> > - The ``if`` member may be either a ``str`` or a ``List[str]``.
> > - A ``str`` value will be normalized to ``List[str]``.
>
> Double-checking: is this doc comment correct before this patch?
>
I think it was
> > + The ``if`` field may be either a ``str`` or a dict.
> > + A ``str`` element will be normalized to ``{'all': List[str]}``.
> >
> > :forms:
> > - :sugared: ``Union[str, List[str]]``
> > - :canonical: ``List[str]``
> > + :sugared: ``Union[str, dict]``
> > + :canonical: ``Union[str, dict]``
> >
> > :param expr: The expression containing the ``if`` member to
> validate.
> > :param info: QAPI schema source file information.
> > @@ -281,25 +281,38 @@ def check_if(expr: _JSONObject, info:
> QAPISourceInfo, source: str) -> None:
> > if ifcond is None:
> > return
> >
> > - if isinstance(ifcond, list):
> > - if not ifcond:
> > - raise QAPISemError(
> > - info, "'if' condition [] of %s is useless" % source)
> > - else:
> > - # Normalize to a list
> > - ifcond = expr['if'] = [ifcond]
> > -
> > - for elt in ifcond:
> > - if not isinstance(elt, str):
> > + def normalize(cond: Union[str, object]) -> Union[str, object]:
>
> This definition is in the middle of check_if()'s body. Intentional?
>
why not
> > + if isinstance(cond, str):
> > + if not cond.strip():
> > + raise QAPISemError(
> > + info,
> > + "'if' condition '%s' of %s makes no sense"
> > + % (cond, source))
> > + return cond
> > + if not isinstance(cond, dict):
> > raise QAPISemError(
> > info,
> > - "'if' condition of %s must be a string or a list of
> strings"
> > - % source)
> > - if not elt.strip():
> > + "'if' condition of %s must be a string or a dict" %
> source)
> > + if len(cond) != 1:
> > raise QAPISemError(
> > info,
> > - "'if' condition '%s' of %s makes no sense"
> > - % (elt, source))
> > + "'if' condition dict of %s must have one key: "
>
> Exactly one key, to be precise.
>
> > + "'all', 'any' or 'not'" % source)
> > + check_keys(cond, info, "'if' condition", [],
> > + ["all", "any", "not"])
>
> Hmmm. Getting members of @cond before check_keys() would be wrong, but
> you don't do that. Still, I like to call check_keys() early, just to
> reduce the chance for accidents.
>
> If we check_keys() first, we're left with just two possible errors:
> empty dict (len(cond)==0), and conflicting keys (len(cond)>1). We could
> choose to diagnose them separately, but it's probably not worth the
> bother.
>
> > + oper, operands = next(iter(cond.items()))
> > + if not operands:
> > + raise QAPISemError(
> > + info, "'if' condition [] of %s is useless" % source)
> > + if oper == "not":
> > + return {oper: normalize(operands)}
> > + if oper in ("all", "any") and not isinstance(operands, list):
> > + raise QAPISemError(
> > + info, "'%s' condition of %s must be a list" % (oper,
> source))
> > + operands = [normalize(o) for o in operands]
> > + return {oper: operands}
>
> I guess making this a function enables writing
>
> return NE
>
> instead of
>
> expr['if] = NE
> return
>
> which is slightly more compact, and factors out the assignment's left
> hand side. Feels like a wash, but up to you.
>
>
gone in v6
> +
> > + expr['if'] = normalize(ifcond)
> >
> >
> > def normalize_members(members: object) -> None:
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index f52caaeecc..9864e49c54 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -22,6 +22,8 @@
> > from .common import (
> > POINTER_SUFFIX,
> > IfAll,
> > + IfAny,
> > + IfNot,
> > IfOption,
> > c_name,
> > )
> > @@ -31,15 +33,14 @@
> >
> >
> > class QAPISchemaIfCond:
> > - def __init__(self, ifcond=None):
> > - self.ifcond = ifcond or []
> > - self.pred = IfAll([IfOption(opt) for opt in self.ifcond])
> > + def __init__(self, pred=None):
> > + self.pred = pred
> >
> > def docgen(self):
> > - return self.pred.docgen()
> > + return self and self.pred.docgen()
>
> The more code relying on your __bool__() methods I see, the less I like
> them.
>
> Can we do self.pred and self.pred.docgen()?
>
> >
> > def cgen(self):
> > - return self.pred.cgen()
> > + return self and self.pred.cgen()
>
> Likewise.
>
> >
> > # Returns true if the condition is not void
> > def __bool__(self):
> > @@ -991,16 +992,41 @@ def _def_predefineds(self):
> > self._def_entity(QAPISchemaEnumType('QType', None, None, None,
> None,
> > qtype_values, 'QTYPE'))
> >
> > + def _make_if(self, cond):
> > + if isinstance(cond, QAPISchemaIfCond):
> > + return cond
> > + if cond is None:
> > + return QAPISchemaIfCond()
> > +
> > + def make_pred(node):
> > + if isinstance(node, str):
> > + return IfOption(node)
> > + oper, operands = next(iter(node.items()))
> > + op2cls = {
> > + 'all': IfAll,
> > + 'any': IfAny,
> > + 'not': IfNot,
> > + }
> > +
> > + if isinstance(operands, list):
> > + operands = [make_pred(o) for o in operands]
> > + else:
> > + operands = make_pred(operands)
> > +
> > + return op2cls[oper](operands)
> > +
> > + return QAPISchemaIfCond(make_pred(cond))
>
> Maybe it's the weather, but I find this pretty impenetrable right now.
>
> make_if() appears to accept either QAPISchemaIfCond, None, or a tree
> whose inner nodes are {'any': List[tree]}, {'all': List[tree]}, {'not':
> tree}, or str. Quite the omnivore.
>
> None of the callers I can see pass QAPISchemaIfCond. Am I confused?
>
> make_pred() appears to accept the tree. The part dealing with str is
> obvious.
>
> next(iter(node.items())) appears to map a dict {key: val} to a tuple
> (key, val). Too clever by half.
>
> val, and thus @operands then is either a list of trees (all, any), or a
> tree (not).
>
> The tree(s) in @operands get recursively processed. Now @operands is
> either a List[IfPredicate], or an IfPredicate.
>
> IfAll and IfAny take the former, IfNot takes the latter. Works out
> (*quack*), but I'm not sure mypy will be happy with it.
>
I removed the IfCond AST altogether in v6.
> > +
> > def _make_features(self, features, info):
> > if features is None:
> > return []
> > return [QAPISchemaFeature(f['name'], info,
> > - QAPISchemaIfCond(f.get('if')))
> > + self._make_if(f.get('if')))
> > for f in features]
> >
> > def _make_enum_members(self, values, info):
> > return [QAPISchemaEnumMember(v['name'], info,
> > - QAPISchemaIfCond(v.get('if')))
> > + self._make_if(v.get('if')))
> > for v in values]
> >
> > def _make_implicit_enum_type(self, name, info, ifcond, values):
> > @@ -1046,7 +1072,7 @@ def _def_enum_type(self, expr, info, doc):
> > name = expr['enum']
> > data = expr['data']
> > prefix = expr.get('prefix')
> > - ifcond = QAPISchemaIfCond(expr.get('if'))
> > + ifcond = self._make_if(expr.get('if'))
> > features = self._make_features(expr.get('features'), info)
> > self._def_entity(QAPISchemaEnumType(
> > name, info, doc, ifcond, features,
> > @@ -1065,7 +1091,7 @@ def _make_member(self, name, typ, ifcond,
> features, info):
> >
> > def _make_members(self, data, info):
> > return [self._make_member(key, value['type'],
> > - QAPISchemaIfCond(value.get('if')),
> > + self._make_if(value.get('if')),
> > value.get('features'), info)
> > for (key, value) in data.items()]
> >
> > @@ -1073,7 +1099,7 @@ def _def_struct_type(self, expr, info, doc):
> > name = expr['struct']
> > base = expr.get('base')
> > data = expr['data']
> > - ifcond = QAPISchemaIfCond(expr.get('if'))
> > + ifcond = self._make_if(expr.get('if'))
> > features = self._make_features(expr.get('features'), info)
> > self._def_entity(QAPISchemaObjectType(
> > name, info, doc, ifcond, features, base,
> > @@ -1096,7 +1122,7 @@ def _def_union_type(self, expr, info, doc):
> > name = expr['union']
> > data = expr['data']
> > base = expr.get('base')
> > - ifcond = QAPISchemaIfCond(expr.get('if'))
> > + ifcond = self._make_if(expr.get('if'))
> > features = self._make_features(expr.get('features'), info)
> > tag_name = expr.get('discriminator')
> > tag_member = None
> > @@ -1107,7 +1133,7 @@ def _def_union_type(self, expr, info, doc):
> > if tag_name:
> > variants = [
> > self._make_variant(key, value['type'],
> > - QAPISchemaIfCond(value.get('if')),
> > + self._make_if(value.get('if')),
> > info)
> > for (key, value) in data.items()
> > ]
> > @@ -1115,11 +1141,11 @@ def _def_union_type(self, expr, info, doc):
> > else:
> > variants = [
> > self._make_simple_variant(key, value['type'],
> > -
> QAPISchemaIfCond(value.get('if')),
> > +
> self._make_if(value.get('if')),
> > info)
> > for (key, value) in data.items()
> > ]
> > - enum = [{'name': v.name, 'if': v.ifcond.ifcond} for v in
> variants]
> > + enum = [{'name': v.name, 'if': v.ifcond} for v in variants]
>
> Another riddle for me to solve?
>
See [PATCH v6 04/11] qapi: _make_enum_members() to work with pre-built
QAPISchemaIfCond
> > typ = self._make_implicit_enum_type(name, info, ifcond,
> enum)
> > tag_member = QAPISchemaObjectTypeMember('type', info, typ,
> False)
> > members = [tag_member]
> > @@ -1132,11 +1158,11 @@ def _def_union_type(self, expr, info, doc):
> > def _def_alternate_type(self, expr, info, doc):
> > name = expr['alternate']
> > data = expr['data']
> > - ifcond = QAPISchemaIfCond(expr.get('if'))
> > + ifcond = self._make_if(expr.get('if'))
> > features = self._make_features(expr.get('features'), info)
> > variants = [
> > self._make_variant(key, value['type'],
> > - QAPISchemaIfCond(value.get('if')),
> > + self._make_if(value.get('if')),
> > info)
> > for (key, value) in data.items()
> > ]
> > @@ -1156,7 +1182,7 @@ def _def_command(self, expr, info, doc):
> > allow_oob = expr.get('allow-oob', False)
> > allow_preconfig = expr.get('allow-preconfig', False)
> > coroutine = expr.get('coroutine', False)
> > - ifcond = QAPISchemaIfCond(expr.get('if'))
> > + ifcond = self._make_if(expr.get('if'))
> > features = self._make_features(expr.get('features'), info)
> > if isinstance(data, OrderedDict):
> > data = self._make_implicit_object_type(
> > @@ -1175,7 +1201,7 @@ def _def_event(self, expr, info, doc):
> > name = expr['event']
> > data = expr.get('data')
> > boxed = expr.get('boxed', False)
> > - ifcond = QAPISchemaIfCond(expr.get('if'))
> > + ifcond = self._make_if(expr.get('if'))
> > features = self._make_features(expr.get('features'), info)
> > if isinstance(data, OrderedDict):
> > data = self._make_implicit_object_type(
> > diff --git a/tests/qapi-schema/bad-if-empty-list.json
> b/tests/qapi-schema/bad-if-empty-list.json
> > index 94f2eb8670..b62b5671df 100644
> > --- a/tests/qapi-schema/bad-if-empty-list.json
> > +++ b/tests/qapi-schema/bad-if-empty-list.json
> > @@ -1,3 +1,3 @@
> > # check empty 'if' list
> > { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
> > - 'if': [] }
> > + 'if': { 'all': [] } }
> > diff --git a/tests/qapi-schema/bad-if-list.json
> b/tests/qapi-schema/bad-if-list.json
> > index ea3d95bb6b..1fefef16a7 100644
> > --- a/tests/qapi-schema/bad-if-list.json
> > +++ b/tests/qapi-schema/bad-if-list.json
> > @@ -1,3 +1,3 @@
> > # check invalid 'if' content
> > { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
> > - 'if': ['foo', ' '] }
> > + 'if': { 'all': ['foo', ' '] } }
> > diff --git a/tests/qapi-schema/bad-if.err b/tests/qapi-schema/bad-if.err
> > index f83dee65da..454fbae387 100644
> > --- a/tests/qapi-schema/bad-if.err
> > +++ b/tests/qapi-schema/bad-if.err
> > @@ -1,2 +1,3 @@
> > bad-if.json: In struct 'TestIfStruct':
> > -bad-if.json:2: 'if' condition of struct must be a string or a list of
> strings
> > +bad-if.json:2: 'if' condition has unknown key 'value'
> > +Valid keys are 'all', 'any', 'not'.
> > diff --git a/tests/qapi-schema/doc-good.out
> b/tests/qapi-schema/doc-good.out
> > index db1d23c6bf..4d951f97ee 100644
> > --- a/tests/qapi-schema/doc-good.out
> > +++ b/tests/qapi-schema/doc-good.out
> > @@ -12,15 +12,15 @@ enum QType
> > module doc-good.json
> > enum Enum
> > member one
> > - if IfAll([IfOption('defined(IFONE)')])
> > + if IfOption('defined(IFONE)')
> > member two
> > - if IfAll([IfOption('defined(IFCOND)')])
> > + if IfOption('defined(IFCOND)')
> > feature enum-feat
> > object Base
> > member base1: Enum optional=False
> > object Variant1
> > member var1: str optional=False
> > - if IfAll([IfOption('defined(IFSTR)')])
> > + if IfOption('defined(IFSTR)')
> > feature member-feat
> > feature variant1-feat
> > object Variant2
> > @@ -29,7 +29,7 @@ object Object
> > tag base1
> > case one: Variant1
> > case two: Variant2
> > - if IfAll([IfOption('IFTWO')])
> > + if IfOption('IFTWO')
> > feature union-feat1
> > object q_obj_Variant1-wrapper
> > member data: Variant1 optional=False
> > @@ -38,13 +38,13 @@ object q_obj_Variant2-wrapper
> > enum SugaredUnionKind
> > member one
> > member two
> > - if IfAll([IfOption('IFTWO')])
> > + if IfOption('IFTWO')
> > object SugaredUnion
> > member type: SugaredUnionKind optional=False
> > tag type
> > case one: q_obj_Variant1-wrapper
> > case two: q_obj_Variant2-wrapper
> > - if IfAll([IfOption('IFTWO')])
> > + if IfOption('IFTWO')
> > feature union-feat2
> > alternate Alternate
> > tag type
> [...]
>
> Skipping the tests for now because I'm running out of brain-juice.
>
>
>
--
Marc-André Lureau
© 2016 - 2026 Red Hat, Inc.