[PATCH v7 06/10] qapi: replace if condition list with dict {'all': [...]}

marcandre.lureau@redhat.com posted 10 patches 4 years, 6 months ago
[PATCH v7 06/10] qapi: replace if condition list with dict {'all': [...]}
Posted by marcandre.lureau@redhat.com 4 years, 6 months ago
From: Marc-André Lureau <marcandre.lureau@redhat.com>

Replace the simple list sugar form with a recursive structure that will
accept other operators in the following commits (all, any or not).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 scripts/qapi/common.py                        | 23 +++++--
 scripts/qapi/expr.py                          | 52 ++++++++++------
 scripts/qapi/schema.py                        |  2 +-
 tests/qapi-schema/bad-if-all.err              |  2 +
 tests/qapi-schema/bad-if-all.json             |  3 +
 tests/qapi-schema/bad-if-all.out              |  0
 tests/qapi-schema/bad-if-empty-list.json      |  2 +-
 tests/qapi-schema/bad-if-key.err              |  3 +
 tests/qapi-schema/bad-if-key.json             |  3 +
 tests/qapi-schema/bad-if-key.out              |  0
 tests/qapi-schema/bad-if-keys.err             |  2 +
 tests/qapi-schema/bad-if-keys.json            |  3 +
 tests/qapi-schema/bad-if-keys.out             |  0
 tests/qapi-schema/bad-if-list.json            |  2 +-
 tests/qapi-schema/bad-if.err                  |  2 +-
 tests/qapi-schema/bad-if.json                 |  2 +-
 tests/qapi-schema/doc-good.json               |  3 +-
 tests/qapi-schema/doc-good.out                | 13 ++--
 tests/qapi-schema/doc-good.txt                |  6 ++
 tests/qapi-schema/enum-if-invalid.err         |  3 +-
 tests/qapi-schema/features-if-invalid.err     |  2 +-
 tests/qapi-schema/meson.build                 |  3 +
 tests/qapi-schema/qapi-schema-test.json       | 25 ++++----
 tests/qapi-schema/qapi-schema-test.out        | 62 +++++++++----------
 .../qapi-schema/struct-member-if-invalid.err  |  2 +-
 .../qapi-schema/union-branch-if-invalid.json  |  2 +-
 26 files changed, 138 insertions(+), 84 deletions(-)
 create mode 100644 tests/qapi-schema/bad-if-all.err
 create mode 100644 tests/qapi-schema/bad-if-all.json
 create mode 100644 tests/qapi-schema/bad-if-all.out
 create mode 100644 tests/qapi-schema/bad-if-key.err
 create mode 100644 tests/qapi-schema/bad-if-key.json
 create mode 100644 tests/qapi-schema/bad-if-key.out
 create mode 100644 tests/qapi-schema/bad-if-keys.err
 create mode 100644 tests/qapi-schema/bad-if-keys.json
 create mode 100644 tests/qapi-schema/bad-if-keys.out

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 5181a0f167..51463510c9 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -13,7 +13,8 @@
 
 import re
 from typing import (
-    List,
+    Any,
+    Dict,
     Match,
     Optional,
     Union,
@@ -199,16 +200,28 @@ def guardend(name: str) -> str:
                  name=c_fname(name).upper())
 
 
-def cgen_ifcond(ifcond: Union[str, List[str]]) -> str:
+def docgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
     if not ifcond:
         return ''
-    return '(' + ') && ('.join(ifcond) + ')'
+    if isinstance(ifcond, str):
+        return ifcond
 
+    oper, operands = next(iter(ifcond.items()))
+    oper = {'all': ' and '}[oper]
+    operands = [docgen_ifcond(o) for o in operands]
+    return '(' + oper.join(operands) + ')'
 
-def docgen_ifcond(ifcond: Union[str, List[str]]) -> str:
+
+def cgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
     if not ifcond:
         return ''
-    return ' and '.join(ifcond)
+    if isinstance(ifcond, str):
+        return ifcond
+
+    oper, operands = next(iter(ifcond.items()))
+    oper = {'all': '&&'}[oper]
+    operands = [cgen_ifcond(o) for o in operands]
+    return '(' + (') ' + oper + ' (').join(operands) + ')'
 
 
 def gen_if(cond: str) -> str:
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index cf98923fa6..b5187bfbca 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -259,14 +259,12 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None:
 
 def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
     """
-    Normalize and validate the ``if`` member of an object.
+    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`` member may be either a ``str`` or a dict.
 
     :forms:
-      :sugared: ``Union[str, List[str]]``
-      :canonical: ``List[str]``
+      :canonical: ``Union[str, dict]``
 
     :param expr: The expression containing the ``if`` member to validate.
     :param info: QAPI schema source file information.
@@ -275,31 +273,45 @@ def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
     :raise QAPISemError:
         When the "if" member fails validation, or when there are no
         non-empty conditions.
-    :return: None, ``expr`` is normalized in-place as needed.
+    :return: None
     """
     ifcond = expr.get('if')
     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]
+    def _check_if(cond: Union[str, object]) -> None:
+        if isinstance(cond, str):
+            if not cond.strip():
+                raise QAPISemError(
+                    info,
+                    "'if' condition '%s' of %s makes no sense"
+                    % (cond, source))
+            return
 
-    for elt in ifcond:
-        if not isinstance(elt, str):
+        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'" % source)
+        check_keys(cond, info, "'if' condition", [],
+                   ["all"])
+
+        oper, operands = next(iter(cond.items()))
+        if not operands:
+            raise QAPISemError(
+                info, "'if' condition [] of %s is useless" % source)
+
+        if oper in ("all") and not isinstance(operands, list):
+            raise QAPISemError(
+                info, "'%s' condition of %s must be a list" % (oper, source))
+        for operand in operands:
+            _check_if(operand)
+
+    _check_if(ifcond)
 
 
 def normalize_members(members: object) -> None:
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index ff9c4f0a17..627735a431 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -32,7 +32,7 @@
 
 class QAPISchemaIfCond:
     def __init__(self, ifcond=None):
-        self.ifcond = ifcond or []
+        self.ifcond = ifcond or {}
 
     def cgen(self):
         return cgen_ifcond(self.ifcond)
diff --git a/tests/qapi-schema/bad-if-all.err b/tests/qapi-schema/bad-if-all.err
new file mode 100644
index 0000000000..3d9edd8af9
--- /dev/null
+++ b/tests/qapi-schema/bad-if-all.err
@@ -0,0 +1,2 @@
+bad-if-all.json: In struct 'TestIfStruct':
+bad-if-all.json:2: 'all' condition of struct must be a list
diff --git a/tests/qapi-schema/bad-if-all.json b/tests/qapi-schema/bad-if-all.json
new file mode 100644
index 0000000000..44837d3981
--- /dev/null
+++ b/tests/qapi-schema/bad-if-all.json
@@ -0,0 +1,3 @@
+# check 'if all' is not a list
+{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
+  'if': { 'all': 'ALL' } }
diff --git a/tests/qapi-schema/bad-if-all.out b/tests/qapi-schema/bad-if-all.out
new file mode 100644
index 0000000000..e69de29bb2
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-key.err b/tests/qapi-schema/bad-if-key.err
new file mode 100644
index 0000000000..725d5abae5
--- /dev/null
+++ b/tests/qapi-schema/bad-if-key.err
@@ -0,0 +1,3 @@
+bad-if-key.json: In struct 'TestIfStruct':
+bad-if-key.json:2: 'if' condition has unknown key 'value'
+Valid keys are 'all'.
diff --git a/tests/qapi-schema/bad-if-key.json b/tests/qapi-schema/bad-if-key.json
new file mode 100644
index 0000000000..64c74c13f2
--- /dev/null
+++ b/tests/qapi-schema/bad-if-key.json
@@ -0,0 +1,3 @@
+# check unknown 'if' dict key
+{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
+  'if': { 'value': 'defined(TEST_IF_STRUCT)' } }
diff --git a/tests/qapi-schema/bad-if-key.out b/tests/qapi-schema/bad-if-key.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/bad-if-keys.err b/tests/qapi-schema/bad-if-keys.err
new file mode 100644
index 0000000000..b072db9a6f
--- /dev/null
+++ b/tests/qapi-schema/bad-if-keys.err
@@ -0,0 +1,2 @@
+bad-if-keys.json: In struct 'TestIfStruct':
+bad-if-keys.json:2: 'if' condition dict of struct must have one key: 'all'
diff --git a/tests/qapi-schema/bad-if-keys.json b/tests/qapi-schema/bad-if-keys.json
new file mode 100644
index 0000000000..9e2f39ae21
--- /dev/null
+++ b/tests/qapi-schema/bad-if-keys.json
@@ -0,0 +1,3 @@
+# check multiple 'if' keys
+{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
+  'if': { 'any': ['ANY'], 'all': ['ALL'] } }
diff --git a/tests/qapi-schema/bad-if-keys.out b/tests/qapi-schema/bad-if-keys.out
new file mode 100644
index 0000000000..e69de29bb2
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..7f8f57057a 100644
--- a/tests/qapi-schema/bad-if.err
+++ b/tests/qapi-schema/bad-if.err
@@ -1,2 +1,2 @@
 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 of struct must be a string or a dict
diff --git a/tests/qapi-schema/bad-if.json b/tests/qapi-schema/bad-if.json
index 3edd1a0bf2..fdc0c87bb3 100644
--- a/tests/qapi-schema/bad-if.json
+++ b/tests/qapi-schema/bad-if.json
@@ -1,3 +1,3 @@
 # check invalid 'if' type
 { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
-  'if': { 'value': 'defined(TEST_IF_STRUCT)' } }
+  'if': ['defined(TEST_IF_STRUCT)'] }
diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
index 423ea23e07..25b1053e8a 100644
--- a/tests/qapi-schema/doc-good.json
+++ b/tests/qapi-schema/doc-good.json
@@ -70,7 +70,8 @@
 # @base1:
 # the first member
 ##
-{ 'struct': 'Base', 'data': { 'base1': 'Enum' } }
+{ 'struct': 'Base', 'data': { 'base1': 'Enum' },
+  'if': { 'all': ['IFALL1', 'IFALL2'] } }
 
 ##
 # @Variant1:
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 8f54ceff2e..689d084f3a 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -12,15 +12,16 @@ enum QType
 module doc-good.json
 enum Enum
     member one
-        if ['defined(IFONE)']
+        if defined(IFONE)
     member two
-    if ['defined(IFCOND)']
+    if defined(IFCOND)
     feature enum-feat
 object Base
     member base1: Enum optional=False
+    if OrderedDict([('all', ['IFALL1', 'IFALL2'])])
 object Variant1
     member var1: str optional=False
-        if ['defined(IFSTR)']
+        if defined(IFSTR)
         feature member-feat
     feature variant1-feat
 object Variant2
@@ -29,7 +30,7 @@ object Object
     tag base1
     case one: Variant1
     case two: Variant2
-        if ['IFTWO']
+        if IFTWO
     feature union-feat1
 object q_obj_Variant1-wrapper
     member data: Variant1 optional=False
@@ -38,13 +39,13 @@ object q_obj_Variant2-wrapper
 enum SugaredUnionKind
     member one
     member two
-        if ['IFTWO']
+        if IFTWO
 object SugaredUnion
     member type: SugaredUnionKind optional=False
     tag type
     case one: q_obj_Variant1-wrapper
     case two: q_obj_Variant2-wrapper
-        if ['IFTWO']
+        if IFTWO
     feature union-feat2
 alternate Alternate
     tag type
diff --git a/tests/qapi-schema/doc-good.txt b/tests/qapi-schema/doc-good.txt
index 726727af74..4490108cb7 100644
--- a/tests/qapi-schema/doc-good.txt
+++ b/tests/qapi-schema/doc-good.txt
@@ -76,6 +76,12 @@ Members
    the first member
 
 
+If
+~~
+
+"(IFALL1 and IFALL2)"
+
+
 "Variant1" (Object)
 -------------------
 
diff --git a/tests/qapi-schema/enum-if-invalid.err b/tests/qapi-schema/enum-if-invalid.err
index 0556dc967b..df305cd79f 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'.
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/meson.build b/tests/qapi-schema/meson.build
index b8de58116a..4697c070bc 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -37,8 +37,11 @@ schemas = [
   'bad-data.json',
   'bad-ident.json',
   'bad-if.json',
+  'bad-if-all.json',
   'bad-if-empty.json',
   'bad-if-empty-list.json',
+  'bad-if-key.json',
+  'bad-if-keys.json',
   'bad-if-list.json',
   'bad-type-bool.json',
   'bad-type-dict.json',
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 84b9d41f15..f2e0fff51f 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)'] } }
 
 { '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,9 @@
                 { '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)'] } } ] }
 
 { 'enum': 'FeatureEnum1',
   'data': [ 'eins', 'zwei', 'drei' ],
@@ -328,8 +328,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 e0b8a5f0b6..6a1b3aa341 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -298,65 +298,65 @@ 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 ['defined(TEST_IF_STRUCT_BAR)']
-    if ['defined(TEST_IF_STRUCT)']
+        if defined(TEST_IF_STRUCT_BAR)
+    if defined(TEST_IF_STRUCT)
 enum TestIfEnum
     member foo
     member bar
-        if ['defined(TEST_IF_ENUM_BAR)']
-    if ['defined(TEST_IF_ENUM)']
+        if defined(TEST_IF_ENUM_BAR)
+    if defined(TEST_IF_ENUM)
 object q_obj_TestStruct-wrapper
     member data: TestStruct optional=False
 enum TestIfUnionKind
     member foo
-    member bar
-        if ['defined(TEST_IF_UNION_BAR)']
-    if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)']
+    member union-bar
+        if defined(TEST_IF_UNION_BAR)
+    if OrderedDict([('all', ['defined(TEST_IF_UNION)', '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 ['defined(TEST_IF_UNION_BAR)']
-    if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)']
+    case union-bar: q_obj_str-wrapper
+        if defined(TEST_IF_UNION_BAR)
+    if OrderedDict([('all', ['defined(TEST_IF_UNION)', 'defined(TEST_IF_STRUCT)'])])
 object q_obj_test-if-union-cmd-arg
     member union-cmd-arg: TestIfUnion optional=False
-    if ['defined(TEST_IF_UNION)']
+    if 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 ['defined(TEST_IF_UNION)']
+    if defined(TEST_IF_UNION)
 alternate TestIfAlternate
     tag type
     case foo: int
     case bar: TestStruct
-        if ['defined(TEST_IF_ALT_BAR)']
-    if ['defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)']
+        if defined(TEST_IF_ALT_BAR)
+    if OrderedDict([('all', ['defined(TEST_IF_ALT)', 'defined(TEST_IF_STRUCT)'])])
 object q_obj_test-if-alternate-cmd-arg
     member alt-cmd-arg: TestIfAlternate optional=False
-    if ['defined(TEST_IF_ALT)']
+    if OrderedDict([('all', ['defined(TEST_IF_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 ['defined(TEST_IF_ALT)']
+    if OrderedDict([('all', ['defined(TEST_IF_ALT)'])])
 object q_obj_test-if-cmd-arg
     member foo: TestIfStruct optional=False
     member bar: TestIfEnum optional=False
-        if ['defined(TEST_IF_CMD_BAR)']
-    if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)']
+        if defined(TEST_IF_CMD_BAR)
+    if OrderedDict([('all', ['defined(TEST_IF_CMD)', '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
-    if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)']
+    if OrderedDict([('all', ['defined(TEST_IF_CMD)', 'defined(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 ['defined(TEST_IF_ENUM)']
+    if defined(TEST_IF_ENUM)
 object q_obj_TEST_IF_EVENT-arg
     member foo: TestIfStruct optional=False
     member bar: TestIfEnumList optional=False
-        if ['defined(TEST_IF_EVT_BAR)']
-    if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)']
+        if defined(TEST_IF_EVT_BAR)
+    if OrderedDict([('all', ['defined(TEST_IF_EVT)', 'defined(TEST_IF_STRUCT)'])])
 event TEST_IF_EVENT q_obj_TEST_IF_EVENT-arg
     boxed=False
-    if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)']
+    if OrderedDict([('all', ['defined(TEST_IF_EVT)', 'defined(TEST_IF_STRUCT)'])])
 object FeatureStruct0
     member foo: int optional=False
 object FeatureStruct1
@@ -379,17 +379,17 @@ object FeatureStruct4
 object CondFeatureStruct1
     member foo: int optional=False
     feature feature1
-        if ['defined(TEST_IF_FEATURE_1)']
+        if defined(TEST_IF_FEATURE_1)
 object CondFeatureStruct2
     member foo: int optional=False
     feature feature1
-        if ['defined(TEST_IF_FEATURE_1)']
+        if defined(TEST_IF_FEATURE_1)
     feature feature2
-        if ['defined(TEST_IF_FEATURE_2)']
+        if defined(TEST_IF_FEATURE_2)
 object CondFeatureStruct3
     member foo: int optional=False
     feature feature1
-        if ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)']
+        if OrderedDict([('all', ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)'])])
 enum FeatureEnum1
     member eins
     member zwei
@@ -429,17 +429,17 @@ 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 ['defined(TEST_IF_FEATURE_1)']
+        if 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 ['defined(TEST_IF_FEATURE_1)']
+        if defined(TEST_IF_FEATURE_1)
     feature feature2
-        if ['defined(TEST_IF_FEATURE_2)']
+        if 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
-        if ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)']
+        if OrderedDict([('all', ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)'])])
 event TEST_EVENT_FEATURES0 FeatureStruct1
     boxed=False
 event TEST_EVENT_FEATURES1 None
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.32.0.264.g75ae10bc75


Re: [PATCH v7 06/10] qapi: replace if condition list with dict {'all': [...]}
Posted by Markus Armbruster 4 years, 6 months ago
marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Replace the simple list sugar form with a recursive structure that will
> accept other operators in the following commits (all, any or not).
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/qapi/common.py                        | 23 +++++--
>  scripts/qapi/expr.py                          | 52 ++++++++++------
>  scripts/qapi/schema.py                        |  2 +-
>  tests/qapi-schema/bad-if-all.err              |  2 +
>  tests/qapi-schema/bad-if-all.json             |  3 +
>  tests/qapi-schema/bad-if-all.out              |  0
>  tests/qapi-schema/bad-if-empty-list.json      |  2 +-
>  tests/qapi-schema/bad-if-key.err              |  3 +
>  tests/qapi-schema/bad-if-key.json             |  3 +
>  tests/qapi-schema/bad-if-key.out              |  0
>  tests/qapi-schema/bad-if-keys.err             |  2 +
>  tests/qapi-schema/bad-if-keys.json            |  3 +
>  tests/qapi-schema/bad-if-keys.out             |  0
>  tests/qapi-schema/bad-if-list.json            |  2 +-
>  tests/qapi-schema/bad-if.err                  |  2 +-
>  tests/qapi-schema/bad-if.json                 |  2 +-
>  tests/qapi-schema/doc-good.json               |  3 +-
>  tests/qapi-schema/doc-good.out                | 13 ++--
>  tests/qapi-schema/doc-good.txt                |  6 ++
>  tests/qapi-schema/enum-if-invalid.err         |  3 +-
>  tests/qapi-schema/features-if-invalid.err     |  2 +-
>  tests/qapi-schema/meson.build                 |  3 +
>  tests/qapi-schema/qapi-schema-test.json       | 25 ++++----
>  tests/qapi-schema/qapi-schema-test.out        | 62 +++++++++----------
>  .../qapi-schema/struct-member-if-invalid.err  |  2 +-
>  .../qapi-schema/union-branch-if-invalid.json  |  2 +-
>  26 files changed, 138 insertions(+), 84 deletions(-)
>  create mode 100644 tests/qapi-schema/bad-if-all.err
>  create mode 100644 tests/qapi-schema/bad-if-all.json
>  create mode 100644 tests/qapi-schema/bad-if-all.out
>  create mode 100644 tests/qapi-schema/bad-if-key.err
>  create mode 100644 tests/qapi-schema/bad-if-key.json
>  create mode 100644 tests/qapi-schema/bad-if-key.out
>  create mode 100644 tests/qapi-schema/bad-if-keys.err
>  create mode 100644 tests/qapi-schema/bad-if-keys.json
>  create mode 100644 tests/qapi-schema/bad-if-keys.out
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 5181a0f167..51463510c9 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -13,7 +13,8 @@
>  
>  import re
>  from typing import (
> -    List,
> +    Any,
> +    Dict,
>      Match,
>      Optional,
>      Union,
> @@ -199,16 +200,28 @@ def guardend(name: str) -> str:
>                   name=c_fname(name).upper())
>  
>  
> -def cgen_ifcond(ifcond: Union[str, List[str]]) -> str:
> +def docgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:

Looks like you forgot to un-swap cgen_ifcond() and docgen_ifcond().  Can
do in my tree.

>      if not ifcond:
>          return ''
> -    return '(' + ') && ('.join(ifcond) + ')'
> +    if isinstance(ifcond, str):
> +        return ifcond
>  
> +    oper, operands = next(iter(ifcond.items()))
> +    oper = {'all': ' and '}[oper]
> +    operands = [docgen_ifcond(o) for o in operands]
> +    return '(' + oper.join(operands) + ')'
>  
> -def docgen_ifcond(ifcond: Union[str, List[str]]) -> str:
> +
> +def cgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
>      if not ifcond:
>          return ''
> -    return ' and '.join(ifcond)
> +    if isinstance(ifcond, str):
> +        return ifcond
> +
> +    oper, operands = next(iter(ifcond.items()))
> +    oper = {'all': '&&'}[oper]
> +    operands = [cgen_ifcond(o) for o in operands]
> +    return '(' + (') ' + oper + ' (').join(operands) + ')'

I suggested a more legible version in review of v6.  Not worth a respin
by itself.

Note to self: try to get rid of redundant parenthesis here.

Note to self: cgen_ifcond() and docgen_ifcond() are almost identical.
Refactor?

>  
>  
>  def gen_if(cond: str) -> str:
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index cf98923fa6..b5187bfbca 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -259,14 +259,12 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None:
>  
>  def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
>      """
> -    Normalize and validate the ``if`` member of an object.
> +    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`` member may be either a ``str`` or a dict.
>  
>      :forms:
> -      :sugared: ``Union[str, List[str]]``
> -      :canonical: ``List[str]``
> +      :canonical: ``Union[str, dict]``

John hasn't answered my question whether :forms: makes sensw without
:sugared:.  If it doesn't, I can drop it in my tree.

>  
>      :param expr: The expression containing the ``if`` member to validate.
>      :param info: QAPI schema source file information.
> @@ -275,31 +273,45 @@ def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
>      :raise QAPISemError:
>          When the "if" member fails validation, or when there are no
>          non-empty conditions.
> -    :return: None, ``expr`` is normalized in-place as needed.
> +    :return: None
>      """
>      ifcond = expr.get('if')
>      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]
> +    def _check_if(cond: Union[str, object]) -> None:
> +        if isinstance(cond, str):
> +            if not cond.strip():
> +                raise QAPISemError(
> +                    info,
> +                    "'if' condition '%s' of %s makes no sense"
> +                    % (cond, source))
> +            return
>  
> -    for elt in ifcond:
> -        if not isinstance(elt, str):
> +        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'" % source)
> +        check_keys(cond, info, "'if' condition", [],
> +                   ["all"])
> +
> +        oper, operands = next(iter(cond.items()))
> +        if not operands:
> +            raise QAPISemError(
> +                info, "'if' condition [] of %s is useless" % source)
> +
> +        if oper in ("all") and not isinstance(operands, list):
> +            raise QAPISemError(
> +                info, "'%s' condition of %s must be a list" % (oper, source))
> +        for operand in operands:
> +            _check_if(operand)
> +
> +    _check_if(ifcond)

Mind if I squash in the move of the helper function to the beginning?

>  
>  
>  def normalize_members(members: object) -> None:
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index ff9c4f0a17..627735a431 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -32,7 +32,7 @@
>  
>  class QAPISchemaIfCond:
>      def __init__(self, ifcond=None):
> -        self.ifcond = ifcond or []
> +        self.ifcond = ifcond or {}

This is slightly subtle.

QAPISchemaIfCond.ifcond can look like one of

* {'all': [COND, ...]}

* {'any': [COND, ...]}          (only after PATCH 07)

* {'not': COND}                 (only after PATCH 08)

* STRING

* {}

This is just like the AST, except "absent" is now {} instead of None.
It can occur only at the root.

cgen_ifcond() and docgen_ifcond() are recursive, which means they
happily accept {} anywhere, and generate crap.

I believe the code works anyway, because it only ever creates {} in
QAPISchemaIfCond.__init__(), i.e. at the root.

Non-local correctness argument.  I'd like to try my hand at tweaking the
code so it's more obviously correct.  Not now.

>  
>      def cgen(self):
>          return cgen_ifcond(self.ifcond)
[Tests skipped for now...]


Re: [PATCH v7 06/10] qapi: replace if condition list with dict {'all': [...]}
Posted by Markus Armbruster 4 years, 6 months ago
Markus Armbruster <armbru@redhat.com> writes:

> marcandre.lureau@redhat.com writes:
>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Replace the simple list sugar form with a recursive structure that will
>> accept other operators in the following commits (all, any or not).
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

[...]

>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index cf98923fa6..b5187bfbca 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -259,14 +259,12 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None:
>>  
>>  def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
>>      """
>> -    Normalize and validate the ``if`` member of an object.
>> +    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`` member may be either a ``str`` or a dict.
>>  
>>      :forms:
>> -      :sugared: ``Union[str, List[str]]``
>> -      :canonical: ``List[str]``
>> +      :canonical: ``Union[str, dict]``
>
> John hasn't answered my question whether :forms: makes sensw without
> :sugared:.  If it doesn't, I can drop it in my tree.

We have a bunch of check_FOO().  Some normalize, and have :forms:.  Some
don't, and don't have :forms:.  This patch changes check_if() not to
normalize.  So far, it leaves a degenerate :forms: behind.  Let's drop
it.  Can do in my tree.

[...]


Re: [PATCH v7 06/10] qapi: replace if condition list with dict {'all': [...]}
Posted by Marc-André Lureau 4 years, 6 months ago
Hi

On Thu, Aug 5, 2021 at 5:42 PM Markus Armbruster <armbru@redhat.com> wrote:

> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Replace the simple list sugar form with a recursive structure that will
> > accept other operators in the following commits (all, any or not).
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  scripts/qapi/common.py                        | 23 +++++--
> >  scripts/qapi/expr.py                          | 52 ++++++++++------
> >  scripts/qapi/schema.py                        |  2 +-
> >  tests/qapi-schema/bad-if-all.err              |  2 +
> >  tests/qapi-schema/bad-if-all.json             |  3 +
> >  tests/qapi-schema/bad-if-all.out              |  0
> >  tests/qapi-schema/bad-if-empty-list.json      |  2 +-
> >  tests/qapi-schema/bad-if-key.err              |  3 +
> >  tests/qapi-schema/bad-if-key.json             |  3 +
> >  tests/qapi-schema/bad-if-key.out              |  0
> >  tests/qapi-schema/bad-if-keys.err             |  2 +
> >  tests/qapi-schema/bad-if-keys.json            |  3 +
> >  tests/qapi-schema/bad-if-keys.out             |  0
> >  tests/qapi-schema/bad-if-list.json            |  2 +-
> >  tests/qapi-schema/bad-if.err                  |  2 +-
> >  tests/qapi-schema/bad-if.json                 |  2 +-
> >  tests/qapi-schema/doc-good.json               |  3 +-
> >  tests/qapi-schema/doc-good.out                | 13 ++--
> >  tests/qapi-schema/doc-good.txt                |  6 ++
> >  tests/qapi-schema/enum-if-invalid.err         |  3 +-
> >  tests/qapi-schema/features-if-invalid.err     |  2 +-
> >  tests/qapi-schema/meson.build                 |  3 +
> >  tests/qapi-schema/qapi-schema-test.json       | 25 ++++----
> >  tests/qapi-schema/qapi-schema-test.out        | 62 +++++++++----------
> >  .../qapi-schema/struct-member-if-invalid.err  |  2 +-
> >  .../qapi-schema/union-branch-if-invalid.json  |  2 +-
> >  26 files changed, 138 insertions(+), 84 deletions(-)
> >  create mode 100644 tests/qapi-schema/bad-if-all.err
> >  create mode 100644 tests/qapi-schema/bad-if-all.json
> >  create mode 100644 tests/qapi-schema/bad-if-all.out
> >  create mode 100644 tests/qapi-schema/bad-if-key.err
> >  create mode 100644 tests/qapi-schema/bad-if-key.json
> >  create mode 100644 tests/qapi-schema/bad-if-key.out
> >  create mode 100644 tests/qapi-schema/bad-if-keys.err
> >  create mode 100644 tests/qapi-schema/bad-if-keys.json
> >  create mode 100644 tests/qapi-schema/bad-if-keys.out
> >
> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> > index 5181a0f167..51463510c9 100644
> > --- a/scripts/qapi/common.py
> > +++ b/scripts/qapi/common.py
> > @@ -13,7 +13,8 @@
> >
> >  import re
> >  from typing import (
> > -    List,
> > +    Any,
> > +    Dict,
> >      Match,
> >      Optional,
> >      Union,
> > @@ -199,16 +200,28 @@ def guardend(name: str) -> str:
> >                   name=c_fname(name).upper())
> >
> >
> > -def cgen_ifcond(ifcond: Union[str, List[str]]) -> str:
> > +def docgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
>
> Looks like you forgot to un-swap cgen_ifcond() and docgen_ifcond().  Can
> do in my tree.
>

Oops, I did it though, I wonder where it went..!


> >      if not ifcond:
> >          return ''
> > -    return '(' + ') && ('.join(ifcond) + ')'
> > +    if isinstance(ifcond, str):
> > +        return ifcond
> >
> > +    oper, operands = next(iter(ifcond.items()))
> > +    oper = {'all': ' and '}[oper]
> > +    operands = [docgen_ifcond(o) for o in operands]
> > +    return '(' + oper.join(operands) + ')'
> >
> > -def docgen_ifcond(ifcond: Union[str, List[str]]) -> str:
> > +
> > +def cgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
> >      if not ifcond:
> >          return ''
> > -    return ' and '.join(ifcond)
> > +    if isinstance(ifcond, str):
> > +        return ifcond
> > +
> > +    oper, operands = next(iter(ifcond.items()))
> > +    oper = {'all': '&&'}[oper]
> > +    operands = [cgen_ifcond(o) for o in operands]
> > +    return '(' + (') ' + oper + ' (').join(operands) + ')'
>
> I suggested a more legible version in review of v6.  Not worth a respin
> by itself.
>
> Note to self: try to get rid of redundant parenthesis here.
>
> Note to self: cgen_ifcond() and docgen_ifcond() are almost identical.
> Refactor?
>
> >
> >
> >  def gen_if(cond: str) -> str:
> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > index cf98923fa6..b5187bfbca 100644
> > --- a/scripts/qapi/expr.py
> > +++ b/scripts/qapi/expr.py
> > @@ -259,14 +259,12 @@ def check_flags(expr: _JSONObject, info:
> QAPISourceInfo) -> None:
> >
> >  def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) ->
> None:
> >      """
> > -    Normalize and validate the ``if`` member of an object.
> > +    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`` member may be either a ``str`` or a dict.
> >
> >      :forms:
> > -      :sugared: ``Union[str, List[str]]``
> > -      :canonical: ``List[str]``
> > +      :canonical: ``Union[str, dict]``
>
> John hasn't answered my question whether :forms: makes sensw without
> :sugared:.  If it doesn't, I can drop it in my tree.
>
> >
> >      :param expr: The expression containing the ``if`` member to
> validate.
> >      :param info: QAPI schema source file information.
> > @@ -275,31 +273,45 @@ def check_if(expr: _JSONObject, info:
> QAPISourceInfo, source: str) -> None:
> >      :raise QAPISemError:
> >          When the "if" member fails validation, or when there are no
> >          non-empty conditions.
> > -    :return: None, ``expr`` is normalized in-place as needed.
> > +    :return: None
> >      """
> >      ifcond = expr.get('if')
> >      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]
> > +    def _check_if(cond: Union[str, object]) -> None:
> > +        if isinstance(cond, str):
> > +            if not cond.strip():
> > +                raise QAPISemError(
> > +                    info,
> > +                    "'if' condition '%s' of %s makes no sense"
> > +                    % (cond, source))
> > +            return
> >
> > -    for elt in ifcond:
> > -        if not isinstance(elt, str):
> > +        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'" % source)
> > +        check_keys(cond, info, "'if' condition", [],
> > +                   ["all"])
> > +
> > +        oper, operands = next(iter(cond.items()))
> > +        if not operands:
> > +            raise QAPISemError(
> > +                info, "'if' condition [] of %s is useless" % source)
> > +
> > +        if oper in ("all") and not isinstance(operands, list):
> > +            raise QAPISemError(
> > +                info, "'%s' condition of %s must be a list" % (oper,
> source))
> > +        for operand in operands:
> > +            _check_if(operand)
> > +
> > +    _check_if(ifcond)
>
> Mind if I squash in the move of the helper function to the beginning?
>
>
No problem

>
> >
> >  def normalize_members(members: object) -> None:
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index ff9c4f0a17..627735a431 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -32,7 +32,7 @@
> >
> >  class QAPISchemaIfCond:
> >      def __init__(self, ifcond=None):
> > -        self.ifcond = ifcond or []
> > +        self.ifcond = ifcond or {}
>
> This is slightly subtle.
>
> QAPISchemaIfCond.ifcond can look like one of
>
> * {'all': [COND, ...]}
>
> * {'any': [COND, ...]}          (only after PATCH 07)
>
> * {'not': COND}                 (only after PATCH 08)
>
> * STRING
>
> * {}
>
> This is just like the AST, except "absent" is now {} instead of None.
> It can occur only at the root.
>
> cgen_ifcond() and docgen_ifcond() are recursive, which means they
> happily accept {} anywhere, and generate crap.
>
> I believe the code works anyway, because it only ever creates {} in
> QAPISchemaIfCond.__init__(), i.e. at the root.
>
> Non-local correctness argument.  I'd like to try my hand at tweaking the
> code so it's more obviously correct.  Not now.
>
> >
> >      def cgen(self):
> >          return cgen_ifcond(self.ifcond)
> [Tests skipped for now...]
>
>
>
thanks

-- 
Marc-André Lureau
Re: [PATCH v7 06/10] qapi: replace if condition list with dict {'all': [...]}
Posted by Markus Armbruster 4 years, 6 months ago
marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Replace the simple list sugar form with a recursive structure that will
> accept other operators in the following commits (all, any or not).
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/qapi/common.py                        | 23 +++++--
>  scripts/qapi/expr.py                          | 52 ++++++++++------
>  scripts/qapi/schema.py                        |  2 +-
>  tests/qapi-schema/bad-if-all.err              |  2 +
>  tests/qapi-schema/bad-if-all.json             |  3 +
>  tests/qapi-schema/bad-if-all.out              |  0
>  tests/qapi-schema/bad-if-empty-list.json      |  2 +-
>  tests/qapi-schema/bad-if-key.err              |  3 +
>  tests/qapi-schema/bad-if-key.json             |  3 +
>  tests/qapi-schema/bad-if-key.out              |  0
>  tests/qapi-schema/bad-if-keys.err             |  2 +
>  tests/qapi-schema/bad-if-keys.json            |  3 +
>  tests/qapi-schema/bad-if-keys.out             |  0
>  tests/qapi-schema/bad-if-list.json            |  2 +-
>  tests/qapi-schema/bad-if.err                  |  2 +-
>  tests/qapi-schema/bad-if.json                 |  2 +-
>  tests/qapi-schema/doc-good.json               |  3 +-
>  tests/qapi-schema/doc-good.out                | 13 ++--
>  tests/qapi-schema/doc-good.txt                |  6 ++
>  tests/qapi-schema/enum-if-invalid.err         |  3 +-
>  tests/qapi-schema/features-if-invalid.err     |  2 +-
>  tests/qapi-schema/meson.build                 |  3 +
>  tests/qapi-schema/qapi-schema-test.json       | 25 ++++----
>  tests/qapi-schema/qapi-schema-test.out        | 62 +++++++++----------
>  .../qapi-schema/struct-member-if-invalid.err  |  2 +-
>  .../qapi-schema/union-branch-if-invalid.json  |  2 +-
>  26 files changed, 138 insertions(+), 84 deletions(-)
>  create mode 100644 tests/qapi-schema/bad-if-all.err
>  create mode 100644 tests/qapi-schema/bad-if-all.json
>  create mode 100644 tests/qapi-schema/bad-if-all.out
>  create mode 100644 tests/qapi-schema/bad-if-key.err
>  create mode 100644 tests/qapi-schema/bad-if-key.json
>  create mode 100644 tests/qapi-schema/bad-if-key.out
>  create mode 100644 tests/qapi-schema/bad-if-keys.err
>  create mode 100644 tests/qapi-schema/bad-if-keys.json
>  create mode 100644 tests/qapi-schema/bad-if-keys.out
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 5181a0f167..51463510c9 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -13,7 +13,8 @@
>  
>  import re
>  from typing import (
> -    List,
> +    Any,
> +    Dict,
>      Match,
>      Optional,
>      Union,
> @@ -199,16 +200,28 @@ def guardend(name: str) -> str:
>                   name=c_fname(name).upper())
>  
>  
> -def cgen_ifcond(ifcond: Union[str, List[str]]) -> str:
> +def docgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
>      if not ifcond:
>          return ''
> -    return '(' + ') && ('.join(ifcond) + ')'
> +    if isinstance(ifcond, str):
> +        return ifcond
>  
> +    oper, operands = next(iter(ifcond.items()))
> +    oper = {'all': ' and '}[oper]
> +    operands = [docgen_ifcond(o) for o in operands]
> +    return '(' + oper.join(operands) + ')'
>  
> -def docgen_ifcond(ifcond: Union[str, List[str]]) -> str:
> +
> +def cgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
>      if not ifcond:
>          return ''
> -    return ' and '.join(ifcond)
> +    if isinstance(ifcond, str):
> +        return ifcond
> +
> +    oper, operands = next(iter(ifcond.items()))
> +    oper = {'all': '&&'}[oper]
> +    operands = [cgen_ifcond(o) for o in operands]
> +    return '(' + (') ' + oper + ' (').join(operands) + ')'
>  
>  
>  def gen_if(cond: str) -> str:
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index cf98923fa6..b5187bfbca 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -259,14 +259,12 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None:
>  
>  def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
>      """
> -    Normalize and validate the ``if`` member of an object.
> +    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`` member may be either a ``str`` or a dict.
>  
>      :forms:
> -      :sugared: ``Union[str, List[str]]``
> -      :canonical: ``List[str]``
> +      :canonical: ``Union[str, dict]``
>  
>      :param expr: The expression containing the ``if`` member to validate.
>      :param info: QAPI schema source file information.
> @@ -275,31 +273,45 @@ def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
>      :raise QAPISemError:
>          When the "if" member fails validation, or when there are no
>          non-empty conditions.
> -    :return: None, ``expr`` is normalized in-place as needed.
> +    :return: None
>      """
>      ifcond = expr.get('if')
>      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]
> +    def _check_if(cond: Union[str, object]) -> None:
> +        if isinstance(cond, str):
> +            if not cond.strip():
> +                raise QAPISemError(
> +                    info,
> +                    "'if' condition '%s' of %s makes no sense"
> +                    % (cond, source))
> +            return
>  
> -    for elt in ifcond:
> -        if not isinstance(elt, str):
> +        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)

s/a dict/an object/, since we're talking about JSON, not Python.

The existing error messages don't get this right 100% either.

> +        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'" % source)
> +        check_keys(cond, info, "'if' condition", [],
> +                   ["all"])
> +
> +        oper, operands = next(iter(cond.items()))
> +        if not operands:
> +            raise QAPISemError(
> +                info, "'if' condition [] of %s is useless" % source)
> +
> +        if oper in ("all") and not isinstance(operands, list):
> +            raise QAPISemError(
> +                info, "'%s' condition of %s must be a list" % (oper, source))

s/a list/an array/

Can do both in my tree.

> +        for operand in operands:
> +            _check_if(operand)
> +
> +    _check_if(ifcond)
>  
>  
>  def normalize_members(members: object) -> None:
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index ff9c4f0a17..627735a431 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -32,7 +32,7 @@
>  
>  class QAPISchemaIfCond:
>      def __init__(self, ifcond=None):
> -        self.ifcond = ifcond or []
> +        self.ifcond = ifcond or {}
>  
>      def cgen(self):
>          return cgen_ifcond(self.ifcond)
> diff --git a/tests/qapi-schema/bad-if-all.err b/tests/qapi-schema/bad-if-all.err
> new file mode 100644
> index 0000000000..3d9edd8af9
> --- /dev/null
> +++ b/tests/qapi-schema/bad-if-all.err
> @@ -0,0 +1,2 @@
> +bad-if-all.json: In struct 'TestIfStruct':
> +bad-if-all.json:2: 'all' condition of struct must be a list
> diff --git a/tests/qapi-schema/bad-if-all.json b/tests/qapi-schema/bad-if-all.json
> new file mode 100644
> index 0000000000..44837d3981
> --- /dev/null
> +++ b/tests/qapi-schema/bad-if-all.json
> @@ -0,0 +1,3 @@
> +# check 'if all' is not a list
> +{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
> +  'if': { 'all': 'ALL' } }
> diff --git a/tests/qapi-schema/bad-if-all.out b/tests/qapi-schema/bad-if-all.out
> new file mode 100644
> index 0000000000..e69de29bb2
> 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': [] } }

Syntax changes, but the error message remains "'if' condition [] of
struct is useless".  The new error message visible in bad-if-all.err
above refers to the same thing as "'all' condition of struct", which is
better.

Not worth a respin by itself.  If improving it is trivial, I'll do it in
my tree.

> diff --git a/tests/qapi-schema/bad-if-key.err b/tests/qapi-schema/bad-if-key.err
> new file mode 100644
> index 0000000000..725d5abae5
> --- /dev/null
> +++ b/tests/qapi-schema/bad-if-key.err
> @@ -0,0 +1,3 @@
> +bad-if-key.json: In struct 'TestIfStruct':
> +bad-if-key.json:2: 'if' condition has unknown key 'value'
> +Valid keys are 'all'.
> diff --git a/tests/qapi-schema/bad-if-key.json b/tests/qapi-schema/bad-if-key.json
> new file mode 100644
> index 0000000000..64c74c13f2
> --- /dev/null
> +++ b/tests/qapi-schema/bad-if-key.json
> @@ -0,0 +1,3 @@
> +# check unknown 'if' dict key
> +{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
> +  'if': { 'value': 'defined(TEST_IF_STRUCT)' } }
> diff --git a/tests/qapi-schema/bad-if-key.out b/tests/qapi-schema/bad-if-key.out
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/tests/qapi-schema/bad-if-keys.err b/tests/qapi-schema/bad-if-keys.err
> new file mode 100644
> index 0000000000..b072db9a6f
> --- /dev/null
> +++ b/tests/qapi-schema/bad-if-keys.err
> @@ -0,0 +1,2 @@
> +bad-if-keys.json: In struct 'TestIfStruct':
> +bad-if-keys.json:2: 'if' condition dict of struct must have one key: 'all'
> diff --git a/tests/qapi-schema/bad-if-keys.json b/tests/qapi-schema/bad-if-keys.json
> new file mode 100644
> index 0000000000..9e2f39ae21
> --- /dev/null
> +++ b/tests/qapi-schema/bad-if-keys.json
> @@ -0,0 +1,3 @@
> +# check multiple 'if' keys
> +{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
> +  'if': { 'any': ['ANY'], 'all': ['ALL'] } }
> diff --git a/tests/qapi-schema/bad-if-keys.out b/tests/qapi-schema/bad-if-keys.out
> new file mode 100644
> index 0000000000..e69de29bb2
> 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', ' '] } }

Likewise.

> diff --git a/tests/qapi-schema/bad-if.err b/tests/qapi-schema/bad-if.err
> index f83dee65da..7f8f57057a 100644
> --- a/tests/qapi-schema/bad-if.err
> +++ b/tests/qapi-schema/bad-if.err
> @@ -1,2 +1,2 @@
>  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 of struct must be a string or a dict
> diff --git a/tests/qapi-schema/bad-if.json b/tests/qapi-schema/bad-if.json
> index 3edd1a0bf2..fdc0c87bb3 100644
> --- a/tests/qapi-schema/bad-if.json
> +++ b/tests/qapi-schema/bad-if.json
> @@ -1,3 +1,3 @@
>  # check invalid 'if' type
>  { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
> -  'if': { 'value': 'defined(TEST_IF_STRUCT)' } }
> +  'if': ['defined(TEST_IF_STRUCT)'] }
> diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
> index 423ea23e07..25b1053e8a 100644
> --- a/tests/qapi-schema/doc-good.json
> +++ b/tests/qapi-schema/doc-good.json
> @@ -70,7 +70,8 @@
>  # @base1:
>  # the first member
>  ##
> -{ 'struct': 'Base', 'data': { 'base1': 'Enum' } }
> +{ 'struct': 'Base', 'data': { 'base1': 'Enum' },
> +  'if': { 'all': ['IFALL1', 'IFALL2'] } }
>  
>  ##
>  # @Variant1:
> diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
> index 8f54ceff2e..689d084f3a 100644
> --- a/tests/qapi-schema/doc-good.out
> +++ b/tests/qapi-schema/doc-good.out
> @@ -12,15 +12,16 @@ enum QType
>  module doc-good.json
>  enum Enum
>      member one
> -        if ['defined(IFONE)']
> +        if defined(IFONE)
>      member two
> -    if ['defined(IFCOND)']
> +    if defined(IFCOND)
>      feature enum-feat
>  object Base
>      member base1: Enum optional=False
> +    if OrderedDict([('all', ['IFALL1', 'IFALL2'])])

This will change (for the better) when we drop OrderedDict in favor of
plain dict.  Tolerable.

>  object Variant1
>      member var1: str optional=False
> -        if ['defined(IFSTR)']
> +        if defined(IFSTR)
>          feature member-feat
>      feature variant1-feat
>  object Variant2
> @@ -29,7 +30,7 @@ object Object
>      tag base1
>      case one: Variant1
>      case two: Variant2
> -        if ['IFTWO']
> +        if IFTWO
>      feature union-feat1
>  object q_obj_Variant1-wrapper
>      member data: Variant1 optional=False
> @@ -38,13 +39,13 @@ object q_obj_Variant2-wrapper
>  enum SugaredUnionKind
>      member one
>      member two
> -        if ['IFTWO']
> +        if IFTWO
>  object SugaredUnion
>      member type: SugaredUnionKind optional=False
>      tag type
>      case one: q_obj_Variant1-wrapper
>      case two: q_obj_Variant2-wrapper
> -        if ['IFTWO']
> +        if IFTWO
>      feature union-feat2
>  alternate Alternate
>      tag type
> diff --git a/tests/qapi-schema/doc-good.txt b/tests/qapi-schema/doc-good.txt
> index 726727af74..4490108cb7 100644
> --- a/tests/qapi-schema/doc-good.txt
> +++ b/tests/qapi-schema/doc-good.txt
> @@ -76,6 +76,12 @@ Members
>     the first member
>  
>  
> +If
> +~~
> +
> +"(IFALL1 and IFALL2)"
> +
> +
>  "Variant1" (Object)
>  -------------------
>  
> diff --git a/tests/qapi-schema/enum-if-invalid.err b/tests/qapi-schema/enum-if-invalid.err
> index 0556dc967b..df305cd79f 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'.
> 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/meson.build b/tests/qapi-schema/meson.build
> index b8de58116a..4697c070bc 100644
> --- a/tests/qapi-schema/meson.build
> +++ b/tests/qapi-schema/meson.build
> @@ -37,8 +37,11 @@ schemas = [
>    'bad-data.json',
>    'bad-ident.json',
>    'bad-if.json',
> +  'bad-if-all.json',
>    'bad-if-empty.json',
>    'bad-if-empty-list.json',
> +  'bad-if-key.json',
> +  'bad-if-keys.json',
>    'bad-if-list.json',
>    'bad-type-bool.json',
>    'bad-type-dict.json',
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index 84b9d41f15..f2e0fff51f 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)'} },

Any particular reason for renaming @bar to @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' },

The joining of lines looks accidental.  Mind if I revert?

> +  'if': { 'all': ['defined(TEST_IF_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,9 @@
>                  { '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)'] } } ] }
>  
>  { 'enum': 'FeatureEnum1',
>    'data': [ 'eins', 'zwei', 'drei' ],
> @@ -328,8 +328,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 e0b8a5f0b6..6a1b3aa341 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -298,65 +298,65 @@ 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 ['defined(TEST_IF_STRUCT_BAR)']
> -    if ['defined(TEST_IF_STRUCT)']
> +        if defined(TEST_IF_STRUCT_BAR)
> +    if defined(TEST_IF_STRUCT)
>  enum TestIfEnum
>      member foo
>      member bar
> -        if ['defined(TEST_IF_ENUM_BAR)']
> -    if ['defined(TEST_IF_ENUM)']
> +        if defined(TEST_IF_ENUM_BAR)
> +    if defined(TEST_IF_ENUM)
>  object q_obj_TestStruct-wrapper
>      member data: TestStruct optional=False
>  enum TestIfUnionKind
>      member foo
> -    member bar
> -        if ['defined(TEST_IF_UNION_BAR)']
> -    if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)']
> +    member union-bar
> +        if defined(TEST_IF_UNION_BAR)
> +    if OrderedDict([('all', ['defined(TEST_IF_UNION)', '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 ['defined(TEST_IF_UNION_BAR)']
> -    if ['defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)']
> +    case union-bar: q_obj_str-wrapper
> +        if defined(TEST_IF_UNION_BAR)
> +    if OrderedDict([('all', ['defined(TEST_IF_UNION)', 'defined(TEST_IF_STRUCT)'])])
>  object q_obj_test-if-union-cmd-arg
>      member union-cmd-arg: TestIfUnion optional=False
> -    if ['defined(TEST_IF_UNION)']
> +    if 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 ['defined(TEST_IF_UNION)']
> +    if defined(TEST_IF_UNION)
>  alternate TestIfAlternate
>      tag type
>      case foo: int
>      case bar: TestStruct
> -        if ['defined(TEST_IF_ALT_BAR)']
> -    if ['defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)']
> +        if defined(TEST_IF_ALT_BAR)
> +    if OrderedDict([('all', ['defined(TEST_IF_ALT)', 'defined(TEST_IF_STRUCT)'])])
>  object q_obj_test-if-alternate-cmd-arg
>      member alt-cmd-arg: TestIfAlternate optional=False
> -    if ['defined(TEST_IF_ALT)']
> +    if OrderedDict([('all', ['defined(TEST_IF_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 ['defined(TEST_IF_ALT)']
> +    if OrderedDict([('all', ['defined(TEST_IF_ALT)'])])
>  object q_obj_test-if-cmd-arg
>      member foo: TestIfStruct optional=False
>      member bar: TestIfEnum optional=False
> -        if ['defined(TEST_IF_CMD_BAR)']
> -    if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)']
> +        if defined(TEST_IF_CMD_BAR)
> +    if OrderedDict([('all', ['defined(TEST_IF_CMD)', '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
> -    if ['defined(TEST_IF_CMD)', 'defined(TEST_IF_STRUCT)']
> +    if OrderedDict([('all', ['defined(TEST_IF_CMD)', 'defined(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 ['defined(TEST_IF_ENUM)']
> +    if defined(TEST_IF_ENUM)
>  object q_obj_TEST_IF_EVENT-arg
>      member foo: TestIfStruct optional=False
>      member bar: TestIfEnumList optional=False
> -        if ['defined(TEST_IF_EVT_BAR)']
> -    if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)']
> +        if defined(TEST_IF_EVT_BAR)
> +    if OrderedDict([('all', ['defined(TEST_IF_EVT)', 'defined(TEST_IF_STRUCT)'])])
>  event TEST_IF_EVENT q_obj_TEST_IF_EVENT-arg
>      boxed=False
> -    if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)']
> +    if OrderedDict([('all', ['defined(TEST_IF_EVT)', 'defined(TEST_IF_STRUCT)'])])
>  object FeatureStruct0
>      member foo: int optional=False
>  object FeatureStruct1
> @@ -379,17 +379,17 @@ object FeatureStruct4
>  object CondFeatureStruct1
>      member foo: int optional=False
>      feature feature1
> -        if ['defined(TEST_IF_FEATURE_1)']
> +        if defined(TEST_IF_FEATURE_1)
>  object CondFeatureStruct2
>      member foo: int optional=False
>      feature feature1
> -        if ['defined(TEST_IF_FEATURE_1)']
> +        if defined(TEST_IF_FEATURE_1)
>      feature feature2
> -        if ['defined(TEST_IF_FEATURE_2)']
> +        if defined(TEST_IF_FEATURE_2)
>  object CondFeatureStruct3
>      member foo: int optional=False
>      feature feature1
> -        if ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)']
> +        if OrderedDict([('all', ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)'])])
>  enum FeatureEnum1
>      member eins
>      member zwei
> @@ -429,17 +429,17 @@ 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 ['defined(TEST_IF_FEATURE_1)']
> +        if 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 ['defined(TEST_IF_FEATURE_1)']
> +        if defined(TEST_IF_FEATURE_1)
>      feature feature2
> -        if ['defined(TEST_IF_FEATURE_2)']
> +        if 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
> -        if ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)']
> +        if OrderedDict([('all', ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)'])])
>  event TEST_EVENT_FEATURES0 FeatureStruct1
>      boxed=False
>  event TEST_EVENT_FEATURES1 None
> 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': [''] } } } }