[PATCH 00/12] qapi: Fixes and cleanups for recent work (mostly)

[PATCH 00/12] qapi: Fixes and cleanups for recent work (mostly)

[Markus Armbruster]

Markus Armbruster (12):
  qapi: Simplify QAPISchemaIfCond's interface for generating C
  qapi: Simplify how QAPISchemaIfCond represents "no condition"
  tests/qapi-schema: Correct two 'if' conditionals
  tests/qapi-schema: Demonstrate broken C code for 'if'
  qapi: Fix C code generation for 'if'
  qapi: Factor common recursion out of cgen_ifcond(), docgen_ifcond()
  qapi: Avoid redundant parens in code generated for conditionals
  qapi: Use "not COND" instead of "!COND" for generated documentation
  qapi: Use re.fullmatch() where appropriate
  tests/qapi-schema: Hide OrderedDict in test output
  qapi: Tweak error messages for missing / conflicting meta-type
  qapi: Tweak error messages for unknown / conflicting 'if' keys

 scripts/qapi/common.py                  | 51 ++++++++++++++-----------
 scripts/qapi/expr.py                    | 32 +++++++---------
 scripts/qapi/gen.py                     |  6 +--
 scripts/qapi/introspect.py              |  6 +--
 scripts/qapi/schema.py                  | 12 +++++-
 scripts/qapi/types.py                   | 22 +++++------
 scripts/qapi/visit.py                   | 14 +++----
 tests/qapi-schema/bad-if-key.err        |  2 +-
 tests/qapi-schema/bad-if-keys.err       |  2 +-
 tests/qapi-schema/doc-good.json         |  2 +-
 tests/qapi-schema/doc-good.out          |  6 +--
 tests/qapi-schema/doc-good.txt          |  8 ++--
 tests/qapi-schema/double-type.err       |  4 +-
 tests/qapi-schema/enum-if-invalid.err   |  2 +-
 tests/qapi-schema/missing-type.err      |  2 +-
 tests/qapi-schema/qapi-schema-test.json |  9 +++--
 tests/qapi-schema/qapi-schema-test.out  | 31 ++++++++-------
 tests/qapi-schema/test-qapi.py          | 11 +++++-
 18 files changed, 118 insertions(+), 104 deletions(-)

-- 
2.31.1

[PATCH 01/12] qapi: Simplify QAPISchemaIfCond's interface for generating C

[Markus Armbruster]

QAPISchemaIfCond.cgen() is only ever used like

    gen_if(ifcond.cgen())

and

    gen_endif(ifcond.cgen())

Simplify to

    ifcond.gen_if()

and

    ifcond.gen_endif()

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/gen.py        |  6 ++----
 scripts/qapi/introspect.py |  6 ++----
 scripts/qapi/schema.py     | 10 +++++++++-
 scripts/qapi/types.py      | 22 ++++++++++------------
 scripts/qapi/visit.py      | 14 ++++++--------
 5 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 51a597a025..ab26d5c937 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -24,8 +24,6 @@
 from .common import (
     c_fname,
     c_name,
-    gen_endif,
-    gen_if,
     guardend,
     guardstart,
     mcgen,
@@ -95,9 +93,9 @@ def _wrap_ifcond(ifcond: QAPISchemaIfCond, before: str, after: str) -> str:
     if added[0] == '\n':
         out += '\n'
         added = added[1:]
-    out += gen_if(ifcond.cgen())
+    out += ifcond.gen_if()
     out += added
-    out += gen_endif(ifcond.cgen())
+    out += ifcond.gen_endif()
     return out
 
 
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index bd4233ecee..548bb29961 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -24,8 +24,6 @@
 
 from .common import (
     c_name,
-    gen_endif,
-    gen_if,
     mcgen,
 )
 from .gen import QAPISchemaMonolithicCVisitor
@@ -124,10 +122,10 @@ def indent(level: int) -> str:
         if obj.comment:
             ret += indent(level) + f"/* {obj.comment} */\n"
         if obj.ifcond.is_present():
-            ret += gen_if(obj.ifcond.cgen())
+            ret += obj.ifcond.gen_if()
         ret += _tree_to_qlit(obj.value, level)
         if obj.ifcond.is_present():
-            ret += '\n' + gen_endif(obj.ifcond.cgen())
+            ret += '\n' + obj.ifcond.gen_endif()
         return ret
 
     ret = ''
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 229d24fce9..1451cdec81 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -24,6 +24,8 @@
     c_name,
     cgen_ifcond,
     docgen_ifcond,
+    gen_endif,
+    gen_if,
 )
 from .error import QAPIError, QAPISemError, QAPISourceError
 from .expr import check_exprs
@@ -34,9 +36,15 @@ class QAPISchemaIfCond:
     def __init__(self, ifcond=None):
         self.ifcond = ifcond or {}
 
-    def cgen(self):
+    def _cgen(self):
         return cgen_ifcond(self.ifcond)
 
+    def gen_if(self):
+        return gen_if(self._cgen())
+
+    def gen_endif(self):
+        return gen_endif(self._cgen())
+
     def docgen(self):
         return docgen_ifcond(self.ifcond)
 
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index db9ff95bd1..b358dbbc8d 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -18,8 +18,6 @@
 from .common import (
     c_enum_const,
     c_name,
-    gen_endif,
-    gen_if,
     mcgen,
 )
 from .gen import QAPISchemaModularCVisitor, ifcontext
@@ -51,13 +49,13 @@ def gen_enum_lookup(name: str,
 ''',
                 c_name=c_name(name))
     for memb in members:
-        ret += gen_if(memb.ifcond.cgen())
+        ret += memb.ifcond.gen_if()
         index = c_enum_const(name, memb.name, prefix)
         ret += mcgen('''
         [%(index)s] = "%(name)s",
 ''',
                      index=index, name=memb.name)
-        ret += gen_endif(memb.ifcond.cgen())
+        ret += memb.ifcond.gen_endif()
 
     ret += mcgen('''
     },
@@ -81,12 +79,12 @@ def gen_enum(name: str,
                 c_name=c_name(name))
 
     for memb in enum_members:
-        ret += gen_if(memb.ifcond.cgen())
+        ret += memb.ifcond.gen_if()
         ret += mcgen('''
     %(c_enum)s,
 ''',
                      c_enum=c_enum_const(name, memb.name, prefix))
-        ret += gen_endif(memb.ifcond.cgen())
+        ret += memb.ifcond.gen_endif()
 
     ret += mcgen('''
 } %(c_name)s;
@@ -126,7 +124,7 @@ def gen_array(name: str, element_type: QAPISchemaType) -> str:
 def gen_struct_members(members: List[QAPISchemaObjectTypeMember]) -> str:
     ret = ''
     for memb in members:
-        ret += gen_if(memb.ifcond.cgen())
+        ret += memb.ifcond.gen_if()
         if memb.optional:
             ret += mcgen('''
     bool has_%(c_name)s;
@@ -136,7 +134,7 @@ def gen_struct_members(members: List[QAPISchemaObjectTypeMember]) -> str:
     %(c_type)s %(c_name)s;
 ''',
                      c_type=memb.type.c_type(), c_name=c_name(memb.name))
-        ret += gen_endif(memb.ifcond.cgen())
+        ret += memb.ifcond.gen_endif()
     return ret
 
 
@@ -159,7 +157,7 @@ def gen_object(name: str, ifcond: QAPISchemaIfCond,
     ret += mcgen('''
 
 ''')
-    ret += gen_if(ifcond.cgen())
+    ret += ifcond.gen_if()
     ret += mcgen('''
 struct %(c_name)s {
 ''',
@@ -193,7 +191,7 @@ def gen_object(name: str, ifcond: QAPISchemaIfCond,
     ret += mcgen('''
 };
 ''')
-    ret += gen_endif(ifcond.cgen())
+    ret += ifcond.gen_endif()
 
     return ret
 
@@ -220,13 +218,13 @@ def gen_variants(variants: QAPISchemaVariants) -> str:
     for var in variants.variants:
         if var.type.name == 'q_empty':
             continue
-        ret += gen_if(var.ifcond.cgen())
+        ret += var.ifcond.gen_if()
         ret += mcgen('''
         %(c_type)s %(c_name)s;
 ''',
                      c_type=var.type.c_unboxed_type(),
                      c_name=c_name(var.name))
-        ret += gen_endif(var.ifcond.cgen())
+        ret += var.ifcond.gen_endif()
 
     ret += mcgen('''
     } u;
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 56ea516399..9d9196a143 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -18,8 +18,6 @@
 from .common import (
     c_enum_const,
     c_name,
-    gen_endif,
-    gen_if,
     indent,
     mcgen,
 )
@@ -79,7 +77,7 @@ def gen_visit_object_members(name: str,
 
     for memb in members:
         deprecated = 'deprecated' in [f.name for f in memb.features]
-        ret += gen_if(memb.ifcond.cgen())
+        ret += memb.ifcond.gen_if()
         if memb.optional:
             ret += mcgen('''
     if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) {
@@ -112,7 +110,7 @@ def gen_visit_object_members(name: str,
             ret += mcgen('''
     }
 ''')
-        ret += gen_endif(memb.ifcond.cgen())
+        ret += memb.ifcond.gen_endif()
 
     if variants:
         tag_member = variants.tag_member
@@ -126,7 +124,7 @@ def gen_visit_object_members(name: str,
         for var in variants.variants:
             case_str = c_enum_const(tag_member.type.name, var.name,
                                     tag_member.type.prefix)
-            ret += gen_if(var.ifcond.cgen())
+            ret += var.ifcond.gen_if()
             if var.type.name == 'q_empty':
                 # valid variant and nothing to do
                 ret += mcgen('''
@@ -142,7 +140,7 @@ def gen_visit_object_members(name: str,
                              case=case_str,
                              c_type=var.type.c_name(), c_name=c_name(var.name))
 
-            ret += gen_endif(var.ifcond.cgen())
+            ret += var.ifcond.gen_endif()
         ret += mcgen('''
     default:
         abort();
@@ -228,7 +226,7 @@ def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str:
                 c_name=c_name(name))
 
     for var in variants.variants:
-        ret += gen_if(var.ifcond.cgen())
+        ret += var.ifcond.gen_if()
         ret += mcgen('''
     case %(case)s:
 ''',
@@ -254,7 +252,7 @@ def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str:
         ret += mcgen('''
         break;
 ''')
-        ret += gen_endif(var.ifcond.cgen())
+        ret += var.ifcond.gen_endif()
 
     ret += mcgen('''
     case QTYPE_NONE:
-- 
2.31.1

[PATCH 02/12] qapi: Simplify how QAPISchemaIfCond represents "no condition"

[Markus Armbruster]

None works fine, there is no need to replace it by {} in .__init__().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/common.py | 4 ++--
 scripts/qapi/schema.py | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 1724ac32db..1c1dc87ccb 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -200,7 +200,7 @@ def guardend(name: str) -> str:
                  name=c_fname(name).upper())
 
 
-def cgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
+def cgen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str:
     if not ifcond:
         return ''
     if isinstance(ifcond, str):
@@ -214,7 +214,7 @@ def cgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
     return '(' + (') ' + oper + ' (').join(operands) + ')'
 
 
-def docgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
+def docgen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str:
     # TODO Doc generated for conditions needs polish
     if not ifcond:
         return ''
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 1451cdec81..3d72c7dfc9 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -34,7 +34,7 @@
 
 class QAPISchemaIfCond:
     def __init__(self, ifcond=None):
-        self.ifcond = ifcond or {}
+        self.ifcond = ifcond
 
     def _cgen(self):
         return cgen_ifcond(self.ifcond)
-- 
2.31.1

[PATCH 03/12] tests/qapi-schema: Correct two 'if' conditionals

[Markus Armbruster]

A definition's conditional should imply the conditionals of types it
uses.  If it doesn't, some configurations won't compile.

Example (from tests/qapi-schema/qapi-schema-test.json):

    { 'union': 'TestIfUnion', 'data':
      { 'foo': 'TestStruct',
	'bar': { 'type': 'str', 'if': 'TEST_IF_UNION_BAR'} },
      'if': { 'all': ['TEST_IF_UNION', 'TEST_IF_STRUCT'] } }

    { 'command': 'test-if-union-cmd',
      'data': { 'union-cmd-arg': 'TestIfUnion' },
      'if': 'TEST_IF_UNION' }

generates

    #if (defined(TEST_IF_UNION)) && (defined(TEST_IF_STRUCT))
    typedef struct TestIfUnion TestIfUnion;
    #endif /* (defined(TEST_IF_UNION)) && (defined(TEST_IF_STRUCT)) */

and

    #if defined(TEST_IF_UNION)
    void qmp_test_if_union_cmd(TestIfUnion *union_cmd_arg, Error **errp);
    void qmp_marshal_test_if_union_cmd(QDict *args, QObject **ret, Error **errp);
    #endif /* defined(TEST_IF_UNION) */

which doesn't compile when !defined(TEST_IF_STRUCT).

Messed up in f8c4fdd6ae "tests/qapi: Cover commands with 'if' and
union / alternate 'data'", v4.0.0.  Harmless, as we don't actually use
this configuration.  Correct it anyway, along with another instance.

This loses coverage for 'not'.  The next commit will bring it back.

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

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index fe028145e4..e20f76d84c 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -236,7 +236,7 @@
 
 { 'command': 'test-if-union-cmd',
   'data': { 'union-cmd-arg': 'TestIfUnion' },
-  'if': 'TEST_IF_UNION' }
+  'if': { 'all': ['TEST_IF_UNION', 'TEST_IF_STRUCT'] } }
 
 { 'alternate': 'TestIfAlternate', 'data':
   { 'foo': 'int',
@@ -245,8 +245,7 @@
 
 { 'command': 'test-if-alternate-cmd',
   'data': { 'alt-cmd-arg': 'TestIfAlternate' },
-  'if': { 'all': ['TEST_IF_ALT',
-                  {'not': 'TEST_IF_NOT_ALT'}] } }
+  'if': { 'all': ['TEST_IF_ALT', 'TEST_IF_STRUCT'] } }
 
 { 'command': 'test-if-cmd',
   'data': {
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 3d0c6a8f28..517d802636 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -321,10 +321,10 @@ object TestIfUnion
     if OrderedDict([('all', ['TEST_IF_UNION', 'TEST_IF_STRUCT'])])
 object q_obj_test-if-union-cmd-arg
     member union-cmd-arg: TestIfUnion optional=False
-    if TEST_IF_UNION
+    if OrderedDict([('all', ['TEST_IF_UNION', 'TEST_IF_STRUCT'])])
 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 TEST_IF_UNION
+    if OrderedDict([('all', ['TEST_IF_UNION', 'TEST_IF_STRUCT'])])
 alternate TestIfAlternate
     tag type
     case foo: int
@@ -333,10 +333,10 @@ alternate TestIfAlternate
     if OrderedDict([('all', ['TEST_IF_ALT', 'TEST_IF_STRUCT'])])
 object q_obj_test-if-alternate-cmd-arg
     member alt-cmd-arg: TestIfAlternate optional=False
-    if OrderedDict([('all', ['TEST_IF_ALT', OrderedDict([('not', 'TEST_IF_NOT_ALT')])])])
+    if OrderedDict([('all', ['TEST_IF_ALT', 'TEST_IF_STRUCT'])])
 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 OrderedDict([('all', ['TEST_IF_ALT', OrderedDict([('not', 'TEST_IF_NOT_ALT')])])])
+    if OrderedDict([('all', ['TEST_IF_ALT', 'TEST_IF_STRUCT'])])
 object q_obj_test-if-cmd-arg
     member foo: TestIfStruct optional=False
     member bar: TestIfEnum optional=False
-- 
2.31.1

[PATCH 04/12] tests/qapi-schema: Demonstrate broken C code for 'if'

[Markus Armbruster]

The C code generated for 'if' conditionals is incorrectly
parenthesized.  For instance,

    'if': { 'not': { 'any': [ { 'not': 'TEST_IF_EVT' },
			      { 'not': 'TEST_IF_STRUCT' } ] } } }

generates

    #if !(!defined(TEST_IF_EVT)) || (!defined(TEST_IF_STRUCT))

This is wrong.  Correct would be:

    #if !(!defined(TEST_IF_EVT) || !defined(TEST_IF_STRUCT))

Cover the issue in qapi-schema-test.json.  This generates bad #if in
tests/test-qapi-events.h and other files.

Add a similar condition to doc-good.json.  The generated documentation
is fine.

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

diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
index 5e30790730..e0027e4cf6 100644
--- a/tests/qapi-schema/doc-good.json
+++ b/tests/qapi-schema/doc-good.json
@@ -127,7 +127,7 @@
 { 'alternate': 'Alternate',
   'features': [ 'alt-feat' ],
   'data': { 'i': 'int', 'b': 'bool' },
-  'if': { 'not': 'IFNOT' } }
+  'if': { 'not': { 'any': [ 'IFONE', 'IFTWO' ] } } }
 
 ##
 # == Another subsection
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 26d1fa5d28..d72f3047e9 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -51,7 +51,7 @@ alternate Alternate
     tag type
     case i: int
     case b: bool
-    if OrderedDict([('not', 'IFNOT')])
+    if OrderedDict([('not', OrderedDict([('any', ['IFONE', 'IFTWO'])]))])
     feature alt-feat
 object q_obj_cmd-arg
     member arg1: int optional=False
diff --git a/tests/qapi-schema/doc-good.txt b/tests/qapi-schema/doc-good.txt
index 5bfe06e14e..85a370831f 100644
--- a/tests/qapi-schema/doc-good.txt
+++ b/tests/qapi-schema/doc-good.txt
@@ -174,7 +174,7 @@ Features
 If
 ~~
 
-"!IFNOT"
+"!(IFONE or IFTWO)"
 
 
 Another subsection
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index e20f76d84c..6e37758280 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -261,6 +261,11 @@
     'bar': { 'type': ['TestIfEnum'], 'if': 'TEST_IF_EVT_BAR' } },
   'if': { 'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT'] } }
 
+{ 'event': 'TEST_IF_EVENT2', 'data': {},
+  # FIXME C #if generated for this conditional is wrong
+  'if': { 'not': { 'any': [ { 'not': 'TEST_IF_EVT' },
+                            { 'not': 'TEST_IF_STRUCT' } ] } } }
+
 # test 'features'
 
 { 'struct': 'FeatureStruct0',
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 517d802636..5d2e830ba2 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -357,6 +357,9 @@ object q_obj_TEST_IF_EVENT-arg
 event TEST_IF_EVENT q_obj_TEST_IF_EVENT-arg
     boxed=False
     if OrderedDict([('all', ['TEST_IF_EVT', 'TEST_IF_STRUCT'])])
+event TEST_IF_EVENT2 None
+    boxed=False
+    if OrderedDict([('not', OrderedDict([('any', [OrderedDict([('not', 'TEST_IF_EVT')]), OrderedDict([('not', 'TEST_IF_STRUCT')])])]))])
 object FeatureStruct0
     member foo: int optional=False
 object FeatureStruct1
-- 
2.31.1

[PATCH 05/12] qapi: Fix C code generation for 'if'

[Markus Armbruster]

When commit 5d83b9a130 "qapi: replace if condition list with dict
{'all': [...]}" made cgen_ifcond() and docgen_ifcond() recursive, it
messed up parenthesises in the former, and got them right in the
latter, as the previous commit demonstrates.

To fix, adopt the latter's working code for the former.  This
generates the correct code from the previous commit's commit message.

Fixes: 5d83b9a130690f879d5f33e991beabe69cb88bc8
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/common.py                  | 4 ++--
 tests/qapi-schema/qapi-schema-test.json | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 1c1dc87ccb..f31e077d7b 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -209,9 +209,9 @@ def cgen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str:
     oper, operands = next(iter(ifcond.items()))
     if oper == 'not':
         return '!' + cgen_ifcond(operands)
-    oper = {'all': '&&', 'any': '||'}[oper]
+    oper = {'all': ' && ', 'any': ' || '}[oper]
     operands = [cgen_ifcond(o) for o in operands]
-    return '(' + (') ' + oper + ' (').join(operands) + ')'
+    return '(' + oper.join(operands) + ')'
 
 
 def docgen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str:
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 6e37758280..b6c36a9eee 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -262,7 +262,6 @@
   'if': { 'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT'] } }
 
 { 'event': 'TEST_IF_EVENT2', 'data': {},
-  # FIXME C #if generated for this conditional is wrong
   'if': { 'not': { 'any': [ { 'not': 'TEST_IF_EVT' },
                             { 'not': 'TEST_IF_STRUCT' } ] } } }
 
-- 
2.31.1

[PATCH 06/12] qapi: Factor common recursion out of cgen_ifcond(), docgen_ifcond()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/common.py | 45 +++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index f31e077d7b..df92cff852 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -17,6 +17,7 @@
     Dict,
     Match,
     Optional,
+    Sequence,
     Union,
 )
 
@@ -200,33 +201,37 @@ def guardend(name: str) -> str:
                  name=c_fname(name).upper())
 
 
-def cgen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str:
+def gen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]],
+               cond_fmt: str, not_fmt: str,
+               all_operator: str, any_operator: str) -> str:
+
+    def do_gen(ifcond: Union[str, Dict[str, Any]]):
+        if isinstance(ifcond, str):
+            return cond_fmt % ifcond
+        assert isinstance(ifcond, dict) and len(ifcond) == 1
+        if 'not' in ifcond:
+            return not_fmt % do_gen(ifcond['not'])
+        if 'all' in ifcond:
+            gen = gen_infix(all_operator, ifcond['all'])
+        else:
+            gen = gen_infix(any_operator, ifcond['any'])
+        return gen
+
+    def gen_infix(operator: str, operands: Sequence[Any]) -> str:
+        return '(' + operator.join([do_gen(o) for o in operands]) + ')'
+
     if not ifcond:
         return ''
-    if isinstance(ifcond, str):
-        return 'defined(' + ifcond + ')'
+    return do_gen(ifcond)
 
-    oper, operands = next(iter(ifcond.items()))
-    if oper == 'not':
-        return '!' + cgen_ifcond(operands)
-    oper = {'all': ' && ', 'any': ' || '}[oper]
-    operands = [cgen_ifcond(o) for o in operands]
-    return '(' + oper.join(operands) + ')'
+
+def cgen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str:
+    return gen_ifcond(ifcond, 'defined(%s)', '!%s', ' && ', ' || ')
 
 
 def docgen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str:
     # TODO Doc generated for conditions needs polish
-    if not ifcond:
-        return ''
-    if isinstance(ifcond, str):
-        return ifcond
-
-    oper, operands = next(iter(ifcond.items()))
-    if oper == 'not':
-        return '!' + docgen_ifcond(operands)
-    oper = {'all': ' and ', 'any': ' or '}[oper]
-    operands = [docgen_ifcond(o) for o in operands]
-    return '(' + oper.join(operands) + ')'
+    return gen_ifcond(ifcond, '%s', '!%s', ' and ', ' or ')
 
 
 def gen_if(cond: str) -> str:
-- 
2.31.1

[PATCH 07/12] qapi: Avoid redundant parens in code generated for conditionals

[Markus Armbruster]

Commit 6cc2e4817f "qapi: introduce QAPISchemaIfCond.cgen()" caused a
minor regression: redundant parenthesis.  Subsequent commits
eliminated of many of them, but not all.  Get rid of the rest now.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/common.py         | 10 ++++++----
 tests/qapi-schema/doc-good.txt |  6 +++---
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index df92cff852..c7ccc7cec7 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -205,24 +205,26 @@ def gen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]],
                cond_fmt: str, not_fmt: str,
                all_operator: str, any_operator: str) -> str:
 
-    def do_gen(ifcond: Union[str, Dict[str, Any]]):
+    def do_gen(ifcond: Union[str, Dict[str, Any]], need_parens: bool):
         if isinstance(ifcond, str):
             return cond_fmt % ifcond
         assert isinstance(ifcond, dict) and len(ifcond) == 1
         if 'not' in ifcond:
-            return not_fmt % do_gen(ifcond['not'])
+            return not_fmt % do_gen(ifcond['not'], True)
         if 'all' in ifcond:
             gen = gen_infix(all_operator, ifcond['all'])
         else:
             gen = gen_infix(any_operator, ifcond['any'])
+        if need_parens:
+            gen = '(' + gen + ')'
         return gen
 
     def gen_infix(operator: str, operands: Sequence[Any]) -> str:
-        return '(' + operator.join([do_gen(o) for o in operands]) + ')'
+        return operator.join([do_gen(o, True) for o in operands])
 
     if not ifcond:
         return ''
-    return do_gen(ifcond)
+    return do_gen(ifcond, False)
 
 
 def cgen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str:
diff --git a/tests/qapi-schema/doc-good.txt b/tests/qapi-schema/doc-good.txt
index 85a370831f..75f51a6fc1 100644
--- a/tests/qapi-schema/doc-good.txt
+++ b/tests/qapi-schema/doc-good.txt
@@ -79,7 +79,7 @@ Members
 If
 ~~
 
-"(IFALL1 and IFALL2)"
+"IFALL1 and IFALL2"
 
 
 "Variant1" (Object)
@@ -120,8 +120,8 @@ Members
 
 The members of "Base"
 The members of "Variant1" when "base1" is ""one""
-The members of "Variant2" when "base1" is ""two"" (**If: **"(IFONE or
-IFTWO)")
+The members of "Variant2" when "base1" is ""two"" (**If: **"IFONE or
+IFTWO")
 
 Features
 ~~~~~~~~
-- 
2.31.1

[PATCH 08/12] qapi: Use "not COND" instead of "!COND" for generated documentation

[Markus Armbruster]

Generated documentation uses operators "and", "or", and "!".  Change
the latter to "not".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/common.py         | 2 +-
 tests/qapi-schema/doc-good.txt | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index c7ccc7cec7..5f8f76e5b2 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -233,7 +233,7 @@ def cgen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str:
 
 def docgen_ifcond(ifcond: Optional[Union[str, Dict[str, Any]]]) -> str:
     # TODO Doc generated for conditions needs polish
-    return gen_ifcond(ifcond, '%s', '!%s', ' and ', ' or ')
+    return gen_ifcond(ifcond, '%s', 'not %s', ' and ', ' or ')
 
 
 def gen_if(cond: str) -> str:
diff --git a/tests/qapi-schema/doc-good.txt b/tests/qapi-schema/doc-good.txt
index 75f51a6fc1..0c59d75964 100644
--- a/tests/qapi-schema/doc-good.txt
+++ b/tests/qapi-schema/doc-good.txt
@@ -174,7 +174,7 @@ Features
 If
 ~~
 
-"!(IFONE or IFTWO)"
+"not (IFONE or IFTWO)"
 
 
 Another subsection
-- 
2.31.1

[PATCH 09/12] qapi: Use re.fullmatch() where appropriate

[Markus Armbruster]

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

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 019f4c97aa..9e2aa1d43a 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -275,7 +275,7 @@ def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
 
     def _check_if(cond: Union[str, object]) -> None:
         if isinstance(cond, str):
-            if not re.match(r'^[A-Z][A-Z0-9_]*$', cond):
+            if not re.fullmatch(r'[A-Z][A-Z0-9_]*', cond):
                 raise QAPISemError(
                     info,
                     "'if' condition '%s' of %s is not a valid identifier"
-- 
2.31.1

[PATCH 10/12] tests/qapi-schema: Hide OrderedDict in test output

[Markus Armbruster]

Since commit 5d83b9a130 "qapi: replace if condition list with dict
{'all': [...]}", we represent if conditionals as trees consisting of
OrderedDict, list and str.  This results in less than legible test
output.  For instance:

    if OrderedDict([('not', OrderedDict([('any', [OrderedDict([('not', 'TEST_IF_EVT')]), OrderedDict([('not', 'TEST_IF_STRUCT')])])]))])

We intend to replace OrderedDict by dict when we get Python 3.7, which
will result in more legible output:

    if {'not': {'any': [{'not': 'TEST_IF_EVT'}, {'not': 'TEST_IF_STRUCT'}]}}

Can't wait: put in a hack to get that now, with a comment to revert it
when we replace OrderedDict.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qapi-schema/doc-good.out         |  6 +++---
 tests/qapi-schema/qapi-schema-test.out | 30 +++++++++++++-------------
 tests/qapi-schema/test-qapi.py         | 11 +++++++++-
 3 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index d72f3047e9..478fe6f82e 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -18,7 +18,7 @@ enum Enum
     feature enum-feat
 object Base
     member base1: Enum optional=False
-    if OrderedDict([('all', ['IFALL1', 'IFALL2'])])
+    if {'all': ['IFALL1', 'IFALL2']}
 object Variant1
     member var1: str optional=False
         if IFSTR
@@ -30,7 +30,7 @@ object Object
     tag base1
     case one: Variant1
     case two: Variant2
-        if OrderedDict([('any', ['IFONE', 'IFTWO'])])
+        if {'any': ['IFONE', 'IFTWO']}
     feature union-feat1
 object q_obj_Variant1-wrapper
     member data: Variant1 optional=False
@@ -51,7 +51,7 @@ alternate Alternate
     tag type
     case i: int
     case b: bool
-    if OrderedDict([('not', OrderedDict([('any', ['IFONE', 'IFTWO'])]))])
+    if {'not': {'any': ['IFONE', 'IFTWO']}}
     feature alt-feat
 object q_obj_cmd-arg
     member arg1: int optional=False
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 5d2e830ba2..d557fe2d89 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -311,40 +311,40 @@ enum TestIfUnionKind
     member foo
     member bar
         if TEST_IF_UNION_BAR
-    if OrderedDict([('all', ['TEST_IF_UNION', 'TEST_IF_STRUCT'])])
+    if {'all': ['TEST_IF_UNION', '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 TEST_IF_UNION_BAR
-    if OrderedDict([('all', ['TEST_IF_UNION', 'TEST_IF_STRUCT'])])
+    if {'all': ['TEST_IF_UNION', 'TEST_IF_STRUCT']}
 object q_obj_test-if-union-cmd-arg
     member union-cmd-arg: TestIfUnion optional=False
-    if OrderedDict([('all', ['TEST_IF_UNION', 'TEST_IF_STRUCT'])])
+    if {'all': ['TEST_IF_UNION', 'TEST_IF_STRUCT']}
 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 OrderedDict([('all', ['TEST_IF_UNION', 'TEST_IF_STRUCT'])])
+    if {'all': ['TEST_IF_UNION', 'TEST_IF_STRUCT']}
 alternate TestIfAlternate
     tag type
     case foo: int
     case bar: TestStruct
         if TEST_IF_ALT_BAR
-    if OrderedDict([('all', ['TEST_IF_ALT', 'TEST_IF_STRUCT'])])
+    if {'all': ['TEST_IF_ALT', 'TEST_IF_STRUCT']}
 object q_obj_test-if-alternate-cmd-arg
     member alt-cmd-arg: TestIfAlternate optional=False
-    if OrderedDict([('all', ['TEST_IF_ALT', 'TEST_IF_STRUCT'])])
+    if {'all': ['TEST_IF_ALT', 'TEST_IF_STRUCT']}
 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 OrderedDict([('all', ['TEST_IF_ALT', 'TEST_IF_STRUCT'])])
+    if {'all': ['TEST_IF_ALT', 'TEST_IF_STRUCT']}
 object q_obj_test-if-cmd-arg
     member foo: TestIfStruct optional=False
     member bar: TestIfEnum optional=False
         if TEST_IF_CMD_BAR
-    if OrderedDict([('all', ['TEST_IF_CMD', 'TEST_IF_STRUCT'])])
+    if {'all': ['TEST_IF_CMD', 'TEST_IF_STRUCT']}
 command test-if-cmd q_obj_test-if-cmd-arg -> UserDefThree
     gen=True success_response=True boxed=False oob=False preconfig=False
-    if OrderedDict([('all', ['TEST_IF_CMD', 'TEST_IF_STRUCT'])])
+    if {'all': ['TEST_IF_CMD', 'TEST_IF_STRUCT']}
 command test-cmd-return-def-three None -> UserDefThree
     gen=True success_response=True boxed=False oob=False preconfig=False
 array TestIfEnumList TestIfEnum
@@ -353,13 +353,13 @@ object q_obj_TEST_IF_EVENT-arg
     member foo: TestIfStruct optional=False
     member bar: TestIfEnumList optional=False
         if TEST_IF_EVT_BAR
-    if OrderedDict([('all', ['TEST_IF_EVT', 'TEST_IF_STRUCT'])])
+    if {'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT']}
 event TEST_IF_EVENT q_obj_TEST_IF_EVENT-arg
     boxed=False
-    if OrderedDict([('all', ['TEST_IF_EVT', 'TEST_IF_STRUCT'])])
+    if {'all': ['TEST_IF_EVT', 'TEST_IF_STRUCT']}
 event TEST_IF_EVENT2 None
     boxed=False
-    if OrderedDict([('not', OrderedDict([('any', [OrderedDict([('not', 'TEST_IF_EVT')]), OrderedDict([('not', 'TEST_IF_STRUCT')])])]))])
+    if {'not': {'any': [{'not': 'TEST_IF_EVT'}, {'not': 'TEST_IF_STRUCT'}]}}
 object FeatureStruct0
     member foo: int optional=False
 object FeatureStruct1
@@ -392,11 +392,11 @@ object CondFeatureStruct2
 object CondFeatureStruct3
     member foo: int optional=False
     feature feature1
-        if OrderedDict([('all', ['TEST_IF_COND_1', 'TEST_IF_COND_2'])])
+        if {'all': ['TEST_IF_COND_1', 'TEST_IF_COND_2']}
 object CondFeatureStruct4
     member foo: int optional=False
     feature feature1
-        if OrderedDict([('any', ['TEST_IF_COND_1', 'TEST_IF_COND_2'])])
+        if {'any': ['TEST_IF_COND_1', 'TEST_IF_COND_2']}
 enum FeatureEnum1
     member eins
     member zwei
@@ -447,7 +447,7 @@ command test-command-cond-features2 None -> None
 command test-command-cond-features3 None -> None
     gen=True success_response=True boxed=False oob=False preconfig=False
     feature feature1
-        if OrderedDict([('all', ['TEST_IF_COND_1', 'TEST_IF_COND_2'])])
+        if {'all': ['TEST_IF_COND_1', 'TEST_IF_COND_2']}
 event TEST_EVENT_FEATURES0 FeatureStruct1
     boxed=False
 event TEST_EVENT_FEATURES1 None
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index c92be2d086..73cffae2b6 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -94,8 +94,17 @@ def _print_variants(variants):
 
     @staticmethod
     def _print_if(ifcond, indent=4):
+        # TODO Drop this hack after replacing OrderedDict by plain
+        # dict (requires Python 3.7)
+        def _massage(subcond):
+            if isinstance(subcond, str):
+                return subcond
+            if isinstance(subcond, list):
+                return [_massage(val) for val in subcond]
+            return {key: _massage(val) for key, val in subcond.items()}
+
         if ifcond.is_present():
-            print('%sif %s' % (' ' * indent, ifcond.ifcond))
+            print('%sif %s' % (' ' * indent, _massage(ifcond.ifcond)))
 
     @classmethod
     def _print_features(cls, features, indent=4):
-- 
2.31.1

[PATCH 11/12] qapi: Tweak error messages for missing / conflicting meta-type

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/expr.py               | 23 +++++++++--------------
 tests/qapi-schema/double-type.err  |  4 +---
 tests/qapi-schema/missing-type.err |  2 +-
 3 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 9e2aa1d43a..ae4437ba08 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -630,20 +630,15 @@ def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
         if 'include' in expr:
             continue
 
-        if 'enum' in expr:
-            meta = 'enum'
-        elif 'union' in expr:
-            meta = 'union'
-        elif 'alternate' in expr:
-            meta = 'alternate'
-        elif 'struct' in expr:
-            meta = 'struct'
-        elif 'command' in expr:
-            meta = 'command'
-        elif 'event' in expr:
-            meta = 'event'
-        else:
-            raise QAPISemError(info, "expression is missing metatype")
+        metas = expr.keys() & {'enum', 'struct', 'union', 'alternate',
+                               'command', 'event'}
+        if len(metas) != 1:
+            raise QAPISemError(
+                info,
+                "expression must have exactly one key"
+                " 'enum', 'struct', 'union', 'alternate',"
+                " 'command', 'event'")
+        meta = metas.pop()
 
         check_name_is_str(expr[meta], info, "'%s'" % meta)
         name = cast(str, expr[meta])
diff --git a/tests/qapi-schema/double-type.err b/tests/qapi-schema/double-type.err
index 576e716197..6a1e8a5990 100644
--- a/tests/qapi-schema/double-type.err
+++ b/tests/qapi-schema/double-type.err
@@ -1,3 +1 @@
-double-type.json: In struct 'Bar':
-double-type.json:2: struct has unknown key 'command'
-Valid keys are 'base', 'data', 'features', 'if', 'struct'.
+double-type.json:2: expression must have exactly one key 'enum', 'struct', 'union', 'alternate', 'command', 'event'
diff --git a/tests/qapi-schema/missing-type.err b/tests/qapi-schema/missing-type.err
index 5755386a18..cb39569e49 100644
--- a/tests/qapi-schema/missing-type.err
+++ b/tests/qapi-schema/missing-type.err
@@ -1 +1 @@
-missing-type.json:2: expression is missing metatype
+missing-type.json:2: expression must have exactly one key 'enum', 'struct', 'union', 'alternate', 'command', 'event'
-- 
2.31.1

[PATCH 12/12] qapi: Tweak error messages for unknown / conflicting 'if' keys

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/expr.py                  | 7 +++----
 tests/qapi-schema/bad-if-key.err      | 2 +-
 tests/qapi-schema/bad-if-keys.err     | 2 +-
 tests/qapi-schema/enum-if-invalid.err | 2 +-
 4 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index ae4437ba08..b62f0a3640 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -286,13 +286,12 @@ def _check_if(cond: Union[str, object]) -> None:
             raise QAPISemError(
                 info,
                 "'if' condition of %s must be a string or an object" % source)
+        check_keys(cond, info, "'if' condition of %s" % source, [],
+                   ["all", "any", "not"])
         if len(cond) != 1:
             raise QAPISemError(
                 info,
-                "'if' condition dict of %s must have one key: "
-                "'all', 'any' or 'not'" % source)
-        check_keys(cond, info, "'if' condition", [],
-                   ["all", "any", "not"])
+                "'if' condition of %s has conflicting keys" % source)
 
         oper, operands = next(iter(cond.items()))
         if not operands:
diff --git a/tests/qapi-schema/bad-if-key.err b/tests/qapi-schema/bad-if-key.err
index a69dc9ee86..38cf44b687 100644
--- a/tests/qapi-schema/bad-if-key.err
+++ b/tests/qapi-schema/bad-if-key.err
@@ -1,3 +1,3 @@
 bad-if-key.json: In struct 'TestIfStruct':
-bad-if-key.json:2: 'if' condition has unknown key 'value'
+bad-if-key.json:2: 'if' condition of struct has unknown key 'value'
 Valid keys are 'all', 'any', 'not'.
diff --git a/tests/qapi-schema/bad-if-keys.err b/tests/qapi-schema/bad-if-keys.err
index aceb31dc6d..fe87bd30ac 100644
--- a/tests/qapi-schema/bad-if-keys.err
+++ b/tests/qapi-schema/bad-if-keys.err
@@ -1,2 +1,2 @@
 bad-if-keys.json: In struct 'TestIfStruct':
-bad-if-keys.json:2: 'if' condition dict of struct must have one key: 'all', 'any' or 'not'
+bad-if-keys.json:2: 'if' condition of struct has conflicting keys
diff --git a/tests/qapi-schema/enum-if-invalid.err b/tests/qapi-schema/enum-if-invalid.err
index 3bb84075a9..2b2bbffb65 100644
--- a/tests/qapi-schema/enum-if-invalid.err
+++ b/tests/qapi-schema/enum-if-invalid.err
@@ -1,3 +1,3 @@
 enum-if-invalid.json: In enum 'TestIfEnum':
-enum-if-invalid.json:2: 'if' condition has unknown key 'val'
+enum-if-invalid.json:2: 'if' condition of 'data' member 'bar' has unknown key 'val'
 Valid keys are 'all', 'any', 'not'.
-- 
2.31.1

Re: [PATCH 00/12] qapi: Fixes and cleanups for recent work (mostly)

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]

On Tue, Aug 31, 2021 at 4:38 PM Markus Armbruster <armbru@redhat.com> wrote:

> Markus Armbruster (12):
>   qapi: Simplify QAPISchemaIfCond's interface for generating C
>   qapi: Simplify how QAPISchemaIfCond represents "no condition"
>   tests/qapi-schema: Correct two 'if' conditionals
>   tests/qapi-schema: Demonstrate broken C code for 'if'
>   qapi: Fix C code generation for 'if'
>   qapi: Factor common recursion out of cgen_ifcond(), docgen_ifcond()
>   qapi: Avoid redundant parens in code generated for conditionals
>   qapi: Use "not COND" instead of "!COND" for generated documentation
>   qapi: Use re.fullmatch() where appropriate
>   tests/qapi-schema: Hide OrderedDict in test output
>   qapi: Tweak error messages for missing / conflicting meta-type
>   qapi: Tweak error messages for unknown / conflicting 'if' keys
>
>  scripts/qapi/common.py                  | 51 ++++++++++++++-----------
>  scripts/qapi/expr.py                    | 32 +++++++---------
>  scripts/qapi/gen.py                     |  6 +--
>  scripts/qapi/introspect.py              |  6 +--
>  scripts/qapi/schema.py                  | 12 +++++-
>  scripts/qapi/types.py                   | 22 +++++------
>  scripts/qapi/visit.py                   | 14 +++----
>  tests/qapi-schema/bad-if-key.err        |  2 +-
>  tests/qapi-schema/bad-if-keys.err       |  2 +-
>  tests/qapi-schema/doc-good.json         |  2 +-
>  tests/qapi-schema/doc-good.out          |  6 +--
>  tests/qapi-schema/doc-good.txt          |  8 ++--
>  tests/qapi-schema/double-type.err       |  4 +-
>  tests/qapi-schema/enum-if-invalid.err   |  2 +-
>  tests/qapi-schema/missing-type.err      |  2 +-
>  tests/qapi-schema/qapi-schema-test.json |  9 +++--
>  tests/qapi-schema/qapi-schema-test.out  | 31 ++++++++-------
>  tests/qapi-schema/test-qapi.py          | 11 +++++-
>  18 files changed, 118 insertions(+), 104 deletions(-)
>
>
The first patch, you should apply isort (we should have a check for that
and linters I suppose).

Series:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
> 2.31.1
>
>

[-- Attachment #2: Type: text/html, Size: 2972 bytes --]

Re: [PATCH 00/12] qapi: Fixes and cleanups for recent work (mostly)

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> On Tue, Aug 31, 2021 at 4:38 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> Markus Armbruster (12):
>>   qapi: Simplify QAPISchemaIfCond's interface for generating C
>>   qapi: Simplify how QAPISchemaIfCond represents "no condition"
>>   tests/qapi-schema: Correct two 'if' conditionals
>>   tests/qapi-schema: Demonstrate broken C code for 'if'
>>   qapi: Fix C code generation for 'if'
>>   qapi: Factor common recursion out of cgen_ifcond(), docgen_ifcond()
>>   qapi: Avoid redundant parens in code generated for conditionals
>>   qapi: Use "not COND" instead of "!COND" for generated documentation
>>   qapi: Use re.fullmatch() where appropriate
>>   tests/qapi-schema: Hide OrderedDict in test output
>>   qapi: Tweak error messages for missing / conflicting meta-type
>>   qapi: Tweak error messages for unknown / conflicting 'if' keys
>>
>>  scripts/qapi/common.py                  | 51 ++++++++++++++-----------
>>  scripts/qapi/expr.py                    | 32 +++++++---------
>>  scripts/qapi/gen.py                     |  6 +--
>>  scripts/qapi/introspect.py              |  6 +--
>>  scripts/qapi/schema.py                  | 12 +++++-
>>  scripts/qapi/types.py                   | 22 +++++------
>>  scripts/qapi/visit.py                   | 14 +++----
>>  tests/qapi-schema/bad-if-key.err        |  2 +-
>>  tests/qapi-schema/bad-if-keys.err       |  2 +-
>>  tests/qapi-schema/doc-good.json         |  2 +-
>>  tests/qapi-schema/doc-good.out          |  6 +--
>>  tests/qapi-schema/doc-good.txt          |  8 ++--
>>  tests/qapi-schema/double-type.err       |  4 +-
>>  tests/qapi-schema/enum-if-invalid.err   |  2 +-
>>  tests/qapi-schema/missing-type.err      |  2 +-
>>  tests/qapi-schema/qapi-schema-test.json |  9 +++--
>>  tests/qapi-schema/qapi-schema-test.out  | 31 ++++++++-------
>>  tests/qapi-schema/test-qapi.py          | 11 +++++-
>>  18 files changed, 118 insertions(+), 104 deletions(-)
>>
>>
> The first patch, you should apply isort

Will fix.

> The first patch, you should apply isort (we should have a check for that
> and linters I suppose).

John will get us there.

> Series:
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Thanks!

Re: [PATCH 00/12] qapi: Fixes and cleanups for recent work (mostly)

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 3105 bytes --]

On Tue, Aug 31, 2021 at 10:28 AM Markus Armbruster <armbru@redhat.com>
wrote:

> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > On Tue, Aug 31, 2021 at 4:38 PM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> Markus Armbruster (12):
> >>   qapi: Simplify QAPISchemaIfCond's interface for generating C
> >>   qapi: Simplify how QAPISchemaIfCond represents "no condition"
> >>   tests/qapi-schema: Correct two 'if' conditionals
> >>   tests/qapi-schema: Demonstrate broken C code for 'if'
> >>   qapi: Fix C code generation for 'if'
> >>   qapi: Factor common recursion out of cgen_ifcond(), docgen_ifcond()
> >>   qapi: Avoid redundant parens in code generated for conditionals
> >>   qapi: Use "not COND" instead of "!COND" for generated documentation
> >>   qapi: Use re.fullmatch() where appropriate
> >>   tests/qapi-schema: Hide OrderedDict in test output
> >>   qapi: Tweak error messages for missing / conflicting meta-type
> >>   qapi: Tweak error messages for unknown / conflicting 'if' keys
> >>
> >>  scripts/qapi/common.py                  | 51 ++++++++++++++-----------
> >>  scripts/qapi/expr.py                    | 32 +++++++---------
> >>  scripts/qapi/gen.py                     |  6 +--
> >>  scripts/qapi/introspect.py              |  6 +--
> >>  scripts/qapi/schema.py                  | 12 +++++-
> >>  scripts/qapi/types.py                   | 22 +++++------
> >>  scripts/qapi/visit.py                   | 14 +++----
> >>  tests/qapi-schema/bad-if-key.err        |  2 +-
> >>  tests/qapi-schema/bad-if-keys.err       |  2 +-
> >>  tests/qapi-schema/doc-good.json         |  2 +-
> >>  tests/qapi-schema/doc-good.out          |  6 +--
> >>  tests/qapi-schema/doc-good.txt          |  8 ++--
> >>  tests/qapi-schema/double-type.err       |  4 +-
> >>  tests/qapi-schema/enum-if-invalid.err   |  2 +-
> >>  tests/qapi-schema/missing-type.err      |  2 +-
> >>  tests/qapi-schema/qapi-schema-test.json |  9 +++--
> >>  tests/qapi-schema/qapi-schema-test.out  | 31 ++++++++-------
> >>  tests/qapi-schema/test-qapi.py          | 11 +++++-
> >>  18 files changed, 118 insertions(+), 104 deletions(-)
> >>
> >>
> > The first patch, you should apply isort
>
> Will fix.
>
> > The first patch, you should apply isort (we should have a check for that
> > and linters I suppose).
>
> John will get us there.
>
>
The goal is to move scripts/qapi to python/qemu/qapi where it will be
covered by the tests that exist there.
Try going to the python/ directory and run 'make' to see help output on
what tests are available and how to invoke them, and what they actually
test.

"make check-dev" is the target that requires the least amount of
dependencies and environment setup to run, and should be nearly push-button
in most environments.
'make check-tox' and 'make check-pipenv' are executed by GitlabCI as
check-python-tox and check-python-pipenv, respectively.

In the meantime, I've been running tests against qapi manually -- with the
scripts I included in my 'python-qapi-cleanup-pt0' branch.

--js

[-- Attachment #2: Type: text/html, Size: 4257 bytes --]

Re: [PATCH 00/12] qapi: Fixes and cleanups for recent work (mostly)

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 3313 bytes --]

Hi

On Wed, Sep 15, 2021 at 6:51 PM John Snow <jsnow@redhat.com> wrote:

>
>
> On Tue, Aug 31, 2021 at 10:28 AM Markus Armbruster <armbru@redhat.com>
> wrote:
>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > On Tue, Aug 31, 2021 at 4:38 PM Markus Armbruster <armbru@redhat.com>
>> wrote:
>> >
>> >> Markus Armbruster (12):
>> >>   qapi: Simplify QAPISchemaIfCond's interface for generating C
>> >>   qapi: Simplify how QAPISchemaIfCond represents "no condition"
>> >>   tests/qapi-schema: Correct two 'if' conditionals
>> >>   tests/qapi-schema: Demonstrate broken C code for 'if'
>> >>   qapi: Fix C code generation for 'if'
>> >>   qapi: Factor common recursion out of cgen_ifcond(), docgen_ifcond()
>> >>   qapi: Avoid redundant parens in code generated for conditionals
>> >>   qapi: Use "not COND" instead of "!COND" for generated documentation
>> >>   qapi: Use re.fullmatch() where appropriate
>> >>   tests/qapi-schema: Hide OrderedDict in test output
>> >>   qapi: Tweak error messages for missing / conflicting meta-type
>> >>   qapi: Tweak error messages for unknown / conflicting 'if' keys
>> >>
>> >>  scripts/qapi/common.py                  | 51 ++++++++++++++-----------
>> >>  scripts/qapi/expr.py                    | 32 +++++++---------
>> >>  scripts/qapi/gen.py                     |  6 +--
>> >>  scripts/qapi/introspect.py              |  6 +--
>> >>  scripts/qapi/schema.py                  | 12 +++++-
>> >>  scripts/qapi/types.py                   | 22 +++++------
>> >>  scripts/qapi/visit.py                   | 14 +++----
>> >>  tests/qapi-schema/bad-if-key.err        |  2 +-
>> >>  tests/qapi-schema/bad-if-keys.err       |  2 +-
>> >>  tests/qapi-schema/doc-good.json         |  2 +-
>> >>  tests/qapi-schema/doc-good.out          |  6 +--
>> >>  tests/qapi-schema/doc-good.txt          |  8 ++--
>> >>  tests/qapi-schema/double-type.err       |  4 +-
>> >>  tests/qapi-schema/enum-if-invalid.err   |  2 +-
>> >>  tests/qapi-schema/missing-type.err      |  2 +-
>> >>  tests/qapi-schema/qapi-schema-test.json |  9 +++--
>> >>  tests/qapi-schema/qapi-schema-test.out  | 31 ++++++++-------
>> >>  tests/qapi-schema/test-qapi.py          | 11 +++++-
>> >>  18 files changed, 118 insertions(+), 104 deletions(-)
>> >>
>> >>
>> > The first patch, you should apply isort
>>
>> Will fix.
>>
>> > The first patch, you should apply isort (we should have a check for that
>> > and linters I suppose).
>>
>> John will get us there.
>>
>>
> The goal is to move scripts/qapi to python/qemu/qapi where it will be
> covered by the tests that exist there.
> Try going to the python/ directory and run 'make' to see help output on
> what tests are available and how to invoke them, and what they actually
> test.
>
> "make check-dev" is the target that requires the least amount of
> dependencies and environment setup to run, and should be nearly push-button
> in most environments.
> 'make check-tox' and 'make check-pipenv' are executed by GitlabCI as
> check-python-tox and check-python-pipenv, respectively.
>
>
What about a pre-commit hook?


> In the meantime, I've been running tests against qapi manually -- with the
> scripts I included in my 'python-qapi-cleanup-pt0' branch.
>
> --js
>

[-- Attachment #2: Type: text/html, Size: 4925 bytes --]

Re: [PATCH 00/12] qapi: Fixes and cleanups for recent work (mostly)

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 3493 bytes --]

On Wed, Sep 15, 2021 at 11:08 AM Marc-André Lureau <
marcandre.lureau@redhat.com> wrote:

> Hi
>
> On Wed, Sep 15, 2021 at 6:51 PM John Snow <jsnow@redhat.com> wrote:
>
>>
>>
>> On Tue, Aug 31, 2021 at 10:28 AM Markus Armbruster <armbru@redhat.com>
>> wrote:
>>
>>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>>
>>> > On Tue, Aug 31, 2021 at 4:38 PM Markus Armbruster <armbru@redhat.com>
>>> wrote:
>>> >
>>> >> Markus Armbruster (12):
>>> >>   qapi: Simplify QAPISchemaIfCond's interface for generating C
>>> >>   qapi: Simplify how QAPISchemaIfCond represents "no condition"
>>> >>   tests/qapi-schema: Correct two 'if' conditionals
>>> >>   tests/qapi-schema: Demonstrate broken C code for 'if'
>>> >>   qapi: Fix C code generation for 'if'
>>> >>   qapi: Factor common recursion out of cgen_ifcond(), docgen_ifcond()
>>> >>   qapi: Avoid redundant parens in code generated for conditionals
>>> >>   qapi: Use "not COND" instead of "!COND" for generated documentation
>>> >>   qapi: Use re.fullmatch() where appropriate
>>> >>   tests/qapi-schema: Hide OrderedDict in test output
>>> >>   qapi: Tweak error messages for missing / conflicting meta-type
>>> >>   qapi: Tweak error messages for unknown / conflicting 'if' keys
>>> >>
>>> >>  scripts/qapi/common.py                  | 51
>>> ++++++++++++++-----------
>>> >>  scripts/qapi/expr.py                    | 32 +++++++---------
>>> >>  scripts/qapi/gen.py                     |  6 +--
>>> >>  scripts/qapi/introspect.py              |  6 +--
>>> >>  scripts/qapi/schema.py                  | 12 +++++-
>>> >>  scripts/qapi/types.py                   | 22 +++++------
>>> >>  scripts/qapi/visit.py                   | 14 +++----
>>> >>  tests/qapi-schema/bad-if-key.err        |  2 +-
>>> >>  tests/qapi-schema/bad-if-keys.err       |  2 +-
>>> >>  tests/qapi-schema/doc-good.json         |  2 +-
>>> >>  tests/qapi-schema/doc-good.out          |  6 +--
>>> >>  tests/qapi-schema/doc-good.txt          |  8 ++--
>>> >>  tests/qapi-schema/double-type.err       |  4 +-
>>> >>  tests/qapi-schema/enum-if-invalid.err   |  2 +-
>>> >>  tests/qapi-schema/missing-type.err      |  2 +-
>>> >>  tests/qapi-schema/qapi-schema-test.json |  9 +++--
>>> >>  tests/qapi-schema/qapi-schema-test.out  | 31 ++++++++-------
>>> >>  tests/qapi-schema/test-qapi.py          | 11 +++++-
>>> >>  18 files changed, 118 insertions(+), 104 deletions(-)
>>> >>
>>> >>
>>> > The first patch, you should apply isort
>>>
>>> Will fix.
>>>
>>> > The first patch, you should apply isort (we should have a check for
>>> that
>>> > and linters I suppose).
>>>
>>> John will get us there.
>>>
>>>
>> The goal is to move scripts/qapi to python/qemu/qapi where it will be
>> covered by the tests that exist there.
>> Try going to the python/ directory and run 'make' to see help output on
>> what tests are available and how to invoke them, and what they actually
>> test.
>>
>> "make check-dev" is the target that requires the least amount of
>> dependencies and environment setup to run, and should be nearly push-button
>> in most environments.
>> 'make check-tox' and 'make check-pipenv' are executed by GitlabCI as
>> check-python-tox and check-python-pipenv, respectively.
>>
>>
> What about a pre-commit hook?
>

"patches welcome" ? There are other python tests likely to go here in the
future too, so it might be too costly to run as a commit hook. YMMV.

--js

[-- Attachment #2: Type: text/html, Size: 5195 bytes --]

Re: [PATCH 00/12] qapi: Fixes and cleanups for recent work (mostly)

[Markus Armbruster]

Queued.