[PULL,30/37] qapi: Fix missing 'if' checks in struct, union, alternate 'data'

Commit Message

Markus Armbruster Sept. 24, 2019, 12:33 p.m. UTC

Commit 87adbbffd4..3e270dcacc "qapi: Add 'if' to (implicit
struct|union|alternate) members" (v4.0.0) neglected test coverage, and
promptly failed to check the conditions.  Review fail.

Recent commit "tests/qapi-schema: Demonstrate insufficient 'if'
checking" added test coverage, demonstrating the bug.  Fix it by add
the missing check_if().

Signed-off-by: Markus Armbruster <[e-mail]>
Message-Id: <[e-mail]>
Reviewed-by: Eric Blake <[e-mail]>
---
 scripts/qapi/common.py                        |  3 +++
 .../alternate-branch-if-invalid.err           |  1 +
 .../alternate-branch-if-invalid.exit          |  2 +-
 .../alternate-branch-if-invalid.json          |  1 -
 .../alternate-branch-if-invalid.out           | 16 -------------
 .../qapi-schema/struct-member-if-invalid.err  |  1 +
 .../qapi-schema/struct-member-if-invalid.exit |  2 +-
 .../qapi-schema/struct-member-if-invalid.json |  1 -
 .../qapi-schema/struct-member-if-invalid.out  | 15 ------------
 tests/qapi-schema/union-branch-if-invalid.err |  1 +
 .../qapi-schema/union-branch-if-invalid.exit  |  2 +-
 .../qapi-schema/union-branch-if-invalid.json  |  1 -
 tests/qapi-schema/union-branch-if-invalid.out | 23 -------------------
 13 files changed, 9 insertions(+), 60 deletions(-)

Patch

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 2b46164854..cacee9b8bb 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -803,6 +803,7 @@  def check_type(info, source, value,
         # an optional argument.
         check_known_keys(info, "member '%s' of %s" % (key, source),
                          arg, ['type'], ['if'])
+        check_if(arg, info)
         check_type(info, "Member '%s' of %s" % (key, source),
                    arg['type'], allow_array=True,
                    allow_metas=['built-in', 'union', 'alternate', 'struct',
@@ -902,6 +903,7 @@  def check_union(expr, info):
 
         check_known_keys(info, "member '%s' of union '%s'" % (key, name),
                          value, ['type'], ['if'])
+        check_if(value, info)
         # Each value must name a known type
         check_type(info, "Member '%s' of union '%s'" % (key, name),
                    value['type'],
@@ -930,6 +932,7 @@  def check_alternate(expr, info):
         check_known_keys(info,
                          "member '%s' of alternate '%s'" % (key, name),
                          value, ['type'], ['if'])
+        check_if(value, info)
         typ = value['type']
 
         # Ensure alternates have no type conflicts.
diff --git a/tests/qapi-schema/alternate-branch-if-invalid.err b/tests/qapi-schema/alternate-branch-if-invalid.err
index e69de29bb2..f1d6c10e00 100644
--- a/tests/qapi-schema/alternate-branch-if-invalid.err
+++ b/tests/qapi-schema/alternate-branch-if-invalid.err
@@ -0,0 +1 @@ 
+tests/qapi-schema/alternate-branch-if-invalid.json:2: 'if' condition ' ' makes no sense
diff --git a/tests/qapi-schema/alternate-branch-if-invalid.exit b/tests/qapi-schema/alternate-branch-if-invalid.exit
index 573541ac97..d00491fd7e 100644
--- a/tests/qapi-schema/alternate-branch-if-invalid.exit
+++ b/tests/qapi-schema/alternate-branch-if-invalid.exit
@@ -1 +1 @@ 
-0
+1
diff --git a/tests/qapi-schema/alternate-branch-if-invalid.json b/tests/qapi-schema/alternate-branch-if-invalid.json
index 6497f53475..fea6d9080c 100644
--- a/tests/qapi-schema/alternate-branch-if-invalid.json
+++ b/tests/qapi-schema/alternate-branch-if-invalid.json
@@ -1,4 +1,3 @@ 
 # Cover alternative with invalid 'if'
-# FIXME not rejected, would generate '#if  \n'
 { 'alternate': 'Alt',
   'data': { 'branch': { 'type': 'int', 'if': ' ' } } }
diff --git a/tests/qapi-schema/alternate-branch-if-invalid.out b/tests/qapi-schema/alternate-branch-if-invalid.out
index 89305d7f21..e69de29bb2 100644
--- a/tests/qapi-schema/alternate-branch-if-invalid.out
+++ b/tests/qapi-schema/alternate-branch-if-invalid.out
@@ -1,16 +0,0 @@ 
-module None
-object q_empty
-enum QType
-    prefix QTYPE
-    member none
-    member qnull
-    member qnum
-    member qstring
-    member qdict
-    member qlist
-    member qbool
-module alternate-branch-if-invalid.json
-alternate Alt
-    tag type
-    case branch: int
-        if [' ']
diff --git a/tests/qapi-schema/struct-member-if-invalid.err b/tests/qapi-schema/struct-member-if-invalid.err
index e69de29bb2..bfd65db97b 100644
--- a/tests/qapi-schema/struct-member-if-invalid.err
+++ b/tests/qapi-schema/struct-member-if-invalid.err
@@ -0,0 +1 @@ 
+tests/qapi-schema/struct-member-if-invalid.json:2: 'if' condition must be a string or a list of strings
diff --git a/tests/qapi-schema/struct-member-if-invalid.exit b/tests/qapi-schema/struct-member-if-invalid.exit
index 573541ac97..d00491fd7e 100644
--- a/tests/qapi-schema/struct-member-if-invalid.exit
+++ b/tests/qapi-schema/struct-member-if-invalid.exit
@@ -1 +1 @@ 
-0
+1
diff --git a/tests/qapi-schema/struct-member-if-invalid.json b/tests/qapi-schema/struct-member-if-invalid.json
index 73987e04fc..35078bd660 100644
--- a/tests/qapi-schema/struct-member-if-invalid.json
+++ b/tests/qapi-schema/struct-member-if-invalid.json
@@ -1,4 +1,3 @@ 
 # Cover member with invalid 'if'
-# FIXME not rejected, would generate '#if True\n'
 { 'struct': 'Stru',
   'data': { 'member': { 'type': 'int', 'if': true } } }
diff --git a/tests/qapi-schema/struct-member-if-invalid.out b/tests/qapi-schema/struct-member-if-invalid.out
index 8fbb97985c..e69de29bb2 100644
--- a/tests/qapi-schema/struct-member-if-invalid.out
+++ b/tests/qapi-schema/struct-member-if-invalid.out
@@ -1,15 +0,0 @@ 
-module None
-object q_empty
-enum QType
-    prefix QTYPE
-    member none
-    member qnull
-    member qnum
-    member qstring
-    member qdict
-    member qlist
-    member qbool
-module struct-member-if-invalid.json
-object Stru
-    member member: int optional=False
-        if [True]
diff --git a/tests/qapi-schema/union-branch-if-invalid.err b/tests/qapi-schema/union-branch-if-invalid.err
index e69de29bb2..607edee382 100644
--- a/tests/qapi-schema/union-branch-if-invalid.err
+++ b/tests/qapi-schema/union-branch-if-invalid.err
@@ -0,0 +1 @@ 
+tests/qapi-schema/union-branch-if-invalid.json:4: 'if' condition '' makes no sense
diff --git a/tests/qapi-schema/union-branch-if-invalid.exit b/tests/qapi-schema/union-branch-if-invalid.exit
index 573541ac97..d00491fd7e 100644
--- a/tests/qapi-schema/union-branch-if-invalid.exit
+++ b/tests/qapi-schema/union-branch-if-invalid.exit
@@ -1 +1 @@ 
-0
+1
diff --git a/tests/qapi-schema/union-branch-if-invalid.json b/tests/qapi-schema/union-branch-if-invalid.json
index 859b63b610..46d4239af6 100644
--- a/tests/qapi-schema/union-branch-if-invalid.json
+++ b/tests/qapi-schema/union-branch-if-invalid.json
@@ -1,5 +1,4 @@ 
 # Cover branch with invalid 'if'
-# FIXME not rejected, would generate '#if \n'
 { 'enum': 'Branches', 'data': ['branch1'] }
 { 'struct': 'Stru', 'data': { 'member': 'str' } }
 { 'union': 'Uni',
diff --git a/tests/qapi-schema/union-branch-if-invalid.out b/tests/qapi-schema/union-branch-if-invalid.out
index 2ed43218af..e69de29bb2 100644
--- a/tests/qapi-schema/union-branch-if-invalid.out
+++ b/tests/qapi-schema/union-branch-if-invalid.out
@@ -1,23 +0,0 @@ 
-module None
-object q_empty
-enum QType
-    prefix QTYPE
-    member none
-    member qnull
-    member qnum
-    member qstring
-    member qdict
-    member qlist
-    member qbool
-module union-branch-if-invalid.json
-enum Branches
-    member branch1
-object Stru
-    member member: str optional=False
-object q_obj_Uni-base
-    member tag: Branches optional=False
-object Uni
-    base q_obj_Uni-base
-    tag tag
-    case branch1: Stru
-        if ['']