[PATCH 0/3] qapi-schema: support alternates with array type

[PATCH 0/3] qapi-schema: support alternates with array type

[Paolo Bonzini]

As suggested in the review of the statistics subsystem.

Paolo Bonzini (3):
  qapi-schema: support alternates with array type
  qapi-schema: test: add a qapi-schema-test for array alternates
  qapi-schema: test: add a unit test for parsing array alternates

 scripts/qapi/expr.py                          |  2 +-
 scripts/qapi/schema.py                        |  4 ++
 tests/qapi-schema/alternate-array.err         |  2 -
 tests/qapi-schema/alternate-array.json        |  2 -
 tests/qapi-schema/alternate-array.out         | 18 ++++++++
 .../qapi-schema/alternate-conflict-lists.err  |  2 +
 .../qapi-schema/alternate-conflict-lists.json |  6 +++
 .../qapi-schema/alternate-conflict-lists.out  |  0
 tests/qapi-schema/meson.build                 |  1 +
 tests/qapi-schema/qapi-schema-test.json       |  1 +
 tests/qapi-schema/qapi-schema-test.out        |  4 ++
 tests/unit/test-qobject-input-visitor.c       | 43 +++++++++++++++++++
 12 files changed, 79 insertions(+), 5 deletions(-)
 create mode 100644 tests/qapi-schema/alternate-conflict-lists.err
 create mode 100644 tests/qapi-schema/alternate-conflict-lists.json
 create mode 100644 tests/qapi-schema/alternate-conflict-lists.out

-- 
2.35.1

[PATCH 1/3] qapi-schema: support alternates with array type

[Paolo Bonzini]

Detect array types as alternate branches, and turn the JSON list into
a QAPISchemaArrayType.  Array types in an alternate are represented with
QTYPE_QLIST in the type field.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/qapi/expr.py                   |  2 +-
 scripts/qapi/schema.py                 |  4 ++++
 tests/qapi-schema/alternate-array.err  |  2 --
 tests/qapi-schema/alternate-array.json |  2 --
 tests/qapi-schema/alternate-array.out  | 18 ++++++++++++++++++
 5 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 3cb389e875..48578e1698 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -554,7 +554,7 @@ def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None:
         check_name_lower(key, info, source)
         check_keys(value, info, source, ['type'], ['if'])
         check_if(value, info, source)
-        check_type(value['type'], info, source)
+        check_type(value['type'], info, source, allow_array=True)
 
 
 def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None:
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index b7b3fc0ce4..3728340c37 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -243,6 +243,7 @@ def alternate_qtype(self):
             'number':  'QTYPE_QNUM',
             'int':     'QTYPE_QNUM',
             'boolean': 'QTYPE_QBOOL',
+            'array':   'QTYPE_QLIST',
             'object':  'QTYPE_QDICT'
         }
         return json2qtype.get(self.json_type())
@@ -1069,6 +1070,9 @@ def _def_struct_type(self, expr, info, doc):
             None))
 
     def _make_variant(self, case, typ, ifcond, info):
+        if isinstance(typ, list):
+            assert len(typ) == 1
+            typ = self._make_array_type(typ[0], info)
         return QAPISchemaVariant(case, info, typ, ifcond)
 
     def _def_union_type(self, expr, info, doc):
diff --git a/tests/qapi-schema/alternate-array.err b/tests/qapi-schema/alternate-array.err
index b1aa1f4e8d..e69de29bb2 100644
--- a/tests/qapi-schema/alternate-array.err
+++ b/tests/qapi-schema/alternate-array.err
@@ -1,2 +0,0 @@
-alternate-array.json: In alternate 'Alt':
-alternate-array.json:5: 'data' member 'two' cannot be an array
diff --git a/tests/qapi-schema/alternate-array.json b/tests/qapi-schema/alternate-array.json
index f241aac122..b878a2db77 100644
--- a/tests/qapi-schema/alternate-array.json
+++ b/tests/qapi-schema/alternate-array.json
@@ -1,5 +1,3 @@
-# we do not allow array branches in alternates
-# TODO: should we support this?
 { 'struct': 'One',
   'data': { 'name': 'str' } }
 { 'alternate': 'Alt',
diff --git a/tests/qapi-schema/alternate-array.out b/tests/qapi-schema/alternate-array.out
index e69de29bb2..a657d85738 100644
--- a/tests/qapi-schema/alternate-array.out
+++ b/tests/qapi-schema/alternate-array.out
@@ -0,0 +1,18 @@
+module ./builtin
+object q_empty
+enum QType
+    prefix QTYPE
+    member none
+    member qnull
+    member qnum
+    member qstring
+    member qdict
+    member qlist
+    member qbool
+module alternate-array.json
+object One
+    member name: str optional=False
+alternate Alt
+    tag type
+    case one: One
+    case two: intList
-- 
2.35.1

[PATCH 2/3] qapi-schema: test: add a qapi-schema-test for array alternates

[Paolo Bonzini]

Check that conflicts among array alternates are detected correctly.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/qapi-schema/alternate-conflict-lists.err  | 2 ++
 tests/qapi-schema/alternate-conflict-lists.json | 6 +++++
 tests/qapi-schema/alternate-conflict-lists.out  | 0
 tests/qapi-schema/meson.build                   | 1 +
 4 files changed, 8 insertions(+)
 create mode 100644 tests/qapi-schema/alternate-conflict-lists.err
 create mode 100644 tests/qapi-schema/alternate-conflict-lists.json
 create mode 100644 tests/qapi-schema/alternate-conflict-lists.out

diff --git a/tests/qapi-schema/alternate-conflict-lists.err b/tests/qapi-schema/alternate-conflict-lists.err
new file mode 100644
index 0000000000..288b75a2ff
--- /dev/null
+++ b/tests/qapi-schema/alternate-conflict-lists.err
@@ -0,0 +1,2 @@
+alternate-conflict-lists.json: In alternate 'Alt':
+alternate-conflict-lists.json:5: branch 'two' can't be distinguished from 'one'
diff --git a/tests/qapi-schema/alternate-conflict-lists.json b/tests/qapi-schema/alternate-conflict-lists.json
new file mode 100644
index 0000000000..90710da115
--- /dev/null
+++ b/tests/qapi-schema/alternate-conflict-lists.json
@@ -0,0 +1,6 @@
+# Two lists conflict even if their inner types would be compatible
+{ 'struct': 'One',
+  'data': { 'name': 'str' } }
+{ 'alternate': 'Alt',
+  'data': { 'one': [ 'int' ],
+            'two': [ 'str' ] } }
diff --git a/tests/qapi-schema/alternate-conflict-lists.out b/tests/qapi-schema/alternate-conflict-lists.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index caf0791ba8..c18dd7d02f 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -11,6 +11,7 @@ schemas = [
   'alternate-conflict-dict.json',
   'alternate-conflict-enum-bool.json',
   'alternate-conflict-enum-int.json',
+  'alternate-conflict-lists.json',
   'alternate-conflict-string.json',
   'alternate-conflict-bool-string.json',
   'alternate-conflict-num-string.json',
-- 
2.35.1

[PATCH 3/3] qapi-schema: test: add a unit test for parsing array alternates

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json |  1 +
 tests/qapi-schema/qapi-schema-test.out  |  4 +++
 tests/unit/test-qobject-input-visitor.c | 43 +++++++++++++++++++++++++
 3 files changed, 48 insertions(+)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 43b8697002..ba7302f42b 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -119,6 +119,7 @@
 { 'alternate': 'AltEnumNum', 'data': { 'e': 'EnumOne', 'n': 'number' } }
 { 'alternate': 'AltNumEnum', 'data': { 'n': 'number', 'e': 'EnumOne' } }
 { 'alternate': 'AltEnumInt', 'data': { 'e': 'EnumOne', 'i': 'int' } }
+{ 'alternate': 'AltListInt', 'data': { 'l': ['int'], 'i': 'int' } }
 
 # for testing use of 'str' within alternates
 { 'alternate': 'AltStrObj', 'data': { 's': 'str', 'o': 'TestStruct' } }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 1f9585fa9b..043d75c655 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -121,6 +121,10 @@ alternate AltEnumInt
     tag type
     case e: EnumOne
     case i: int
+alternate AltListInt
+    tag type
+    case l: intList
+    case i: int
 alternate AltStrObj
     tag type
     case s: str
diff --git a/tests/unit/test-qobject-input-visitor.c b/tests/unit/test-qobject-input-visitor.c
index 6f59a7f432..2af002dd82 100644
--- a/tests/unit/test-qobject-input-visitor.c
+++ b/tests/unit/test-qobject-input-visitor.c
@@ -776,6 +776,7 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
     AltEnumNum *aen;
     AltNumEnum *ans;
     AltEnumInt *asi;
+    AltListInt *ali;
 
     /* Parsing an int */
 
@@ -802,6 +803,12 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
     g_assert_cmpint(asi->u.i, ==, 42);
     qapi_free_AltEnumInt(asi);
 
+    v = visitor_input_test_init(data, "42");
+    visit_type_AltListInt(v, NULL, &ali, &error_abort);
+    g_assert_cmpint(ali->type, ==, QTYPE_QNUM);
+    g_assert_cmpint(ali->u.i, ==, 42);
+    qapi_free_AltListInt(ali);
+
     /* Parsing a double */
 
     v = visitor_input_test_init(data, "42.5");
@@ -827,6 +834,40 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
     qapi_free_AltEnumInt(asi);
 }
 
+static void test_visitor_in_alternate_list(TestInputVisitorData *data,
+                                 const void *unused)
+{
+    intList *item;
+    Visitor *v;
+    AltListInt *ali;
+    int i;
+
+    v = visitor_input_test_init(data, "[ 42, 43, 44 ]");
+    visit_type_AltListInt(v, NULL, &ali, &error_abort);
+    g_assert(ali != NULL);
+
+    g_assert_cmpint(ali->type, ==, QTYPE_QLIST);
+    for (i = 0, item = ali->u.l; item; item = item->next, i++) {
+        char string[12];
+
+        snprintf(string, sizeof(string), "string%d", i);
+        g_assert_cmpint(item->value, ==, 42 + i);
+    }
+
+    qapi_free_AltListInt(ali);
+    ali = NULL;
+
+    /* An empty list is valid */
+    v = visitor_input_test_init(data, "[]");
+    visit_type_AltListInt(v, NULL, &ali, &error_abort);
+    g_assert(ali != NULL);
+
+    g_assert_cmpint(ali->type, ==, QTYPE_QLIST);
+    g_assert(!ali->u.l);
+    qapi_free_AltListInt(ali);
+    ali = NULL;
+}
+
 static void input_visitor_test_add(const char *testpath,
                                    const void *user_data,
                                    void (*test_func)(TestInputVisitorData *data,
@@ -1188,6 +1229,8 @@ int main(int argc, char **argv)
                            NULL, test_visitor_in_wrong_type);
     input_visitor_test_add("/visitor/input/alternate-number",
                            NULL, test_visitor_in_alternate_number);
+    input_visitor_test_add("/visitor/input/alternate-list",
+                           NULL, test_visitor_in_alternate_list);
     input_visitor_test_add("/visitor/input/fail/struct",
                            NULL, test_visitor_in_fail_struct);
     input_visitor_test_add("/visitor/input/fail/struct-nested",
-- 
2.35.1

Re: [PATCH 2/3] qapi-schema: test: add a qapi-schema-test for array alternates

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Check that conflicts among array alternates are detected correctly.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/qapi-schema/alternate-conflict-lists.err  | 2 ++
>  tests/qapi-schema/alternate-conflict-lists.json | 6 +++++
>  tests/qapi-schema/alternate-conflict-lists.out  | 0
>  tests/qapi-schema/meson.build                   | 1 +
>  4 files changed, 8 insertions(+)
>  create mode 100644 tests/qapi-schema/alternate-conflict-lists.err
>  create mode 100644 tests/qapi-schema/alternate-conflict-lists.json
>  create mode 100644 tests/qapi-schema/alternate-conflict-lists.out
>
> diff --git a/tests/qapi-schema/alternate-conflict-lists.err b/tests/qapi-schema/alternate-conflict-lists.err
> new file mode 100644
> index 0000000000..288b75a2ff
> --- /dev/null
> +++ b/tests/qapi-schema/alternate-conflict-lists.err
> @@ -0,0 +1,2 @@
> +alternate-conflict-lists.json: In alternate 'Alt':
> +alternate-conflict-lists.json:5: branch 'two' can't be distinguished from 'one'

Needs a trivial fixup:

-alternate-conflict-lists.json:5: branch 'two' can't be distinguished from 'one'
+alternate-conflict-lists.json:4: branch 'two' can't be distinguished from 'one'

Happy to apply it in my tree.

> diff --git a/tests/qapi-schema/alternate-conflict-lists.json b/tests/qapi-schema/alternate-conflict-lists.json
> new file mode 100644
> index 0000000000..90710da115
> --- /dev/null
> +++ b/tests/qapi-schema/alternate-conflict-lists.json
> @@ -0,0 +1,6 @@
> +# Two lists conflict even if their inner types would be compatible
> +{ 'struct': 'One',
> +  'data': { 'name': 'str' } }
> +{ 'alternate': 'Alt',
> +  'data': { 'one': [ 'int' ],
> +            'two': [ 'str' ] } }
> diff --git a/tests/qapi-schema/alternate-conflict-lists.out b/tests/qapi-schema/alternate-conflict-lists.out
> new file mode 100644
> index 0000000000..e69de29bb2
> diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
> index caf0791ba8..c18dd7d02f 100644
> --- a/tests/qapi-schema/meson.build
> +++ b/tests/qapi-schema/meson.build
> @@ -11,6 +11,7 @@ schemas = [
>    'alternate-conflict-dict.json',
>    'alternate-conflict-enum-bool.json',
>    'alternate-conflict-enum-int.json',
> +  'alternate-conflict-lists.json',
>    'alternate-conflict-string.json',
>    'alternate-conflict-bool-string.json',
>    'alternate-conflict-num-string.json',

With the fixup:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 3/3] qapi-schema: test: add a unit test for parsing array alternates

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/qapi-schema/qapi-schema-test.json |  1 +
>  tests/qapi-schema/qapi-schema-test.out  |  4 +++
>  tests/unit/test-qobject-input-visitor.c | 43 +++++++++++++++++++++++++
>  3 files changed, 48 insertions(+)
>
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index 43b8697002..ba7302f42b 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -119,6 +119,7 @@
>  { 'alternate': 'AltEnumNum', 'data': { 'e': 'EnumOne', 'n': 'number' } }
>  { 'alternate': 'AltNumEnum', 'data': { 'n': 'number', 'e': 'EnumOne' } }
>  { 'alternate': 'AltEnumInt', 'data': { 'e': 'EnumOne', 'i': 'int' } }
> +{ 'alternate': 'AltListInt', 'data': { 'l': ['int'], 'i': 'int' } }
>  
>  # for testing use of 'str' within alternates
>  { 'alternate': 'AltStrObj', 'data': { 's': 'str', 'o': 'TestStruct' } }
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 1f9585fa9b..043d75c655 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -121,6 +121,10 @@ alternate AltEnumInt
>      tag type
>      case e: EnumOne
>      case i: int
> +alternate AltListInt
> +    tag type
> +    case l: intList
> +    case i: int
>  alternate AltStrObj
>      tag type
>      case s: str
> diff --git a/tests/unit/test-qobject-input-visitor.c b/tests/unit/test-qobject-input-visitor.c
> index 6f59a7f432..2af002dd82 100644
> --- a/tests/unit/test-qobject-input-visitor.c
> +++ b/tests/unit/test-qobject-input-visitor.c
> @@ -776,6 +776,7 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
>      AltEnumNum *aen;
>      AltNumEnum *ans;
>      AltEnumInt *asi;
> +    AltListInt *ali;
>  
>      /* Parsing an int */
>  
> @@ -802,6 +803,12 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
>      g_assert_cmpint(asi->u.i, ==, 42);
>      qapi_free_AltEnumInt(asi);
>  
> +    v = visitor_input_test_init(data, "42");
> +    visit_type_AltListInt(v, NULL, &ali, &error_abort);
> +    g_assert_cmpint(ali->type, ==, QTYPE_QNUM);
> +    g_assert_cmpint(ali->u.i, ==, 42);
> +    qapi_free_AltListInt(ali);
> +
>      /* Parsing a double */
>  
>      v = visitor_input_test_init(data, "42.5");
> @@ -827,6 +834,40 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
>      qapi_free_AltEnumInt(asi);
>  }
>  
> +static void test_visitor_in_alternate_list(TestInputVisitorData *data,
> +                                 const void *unused)
> +{
> +    intList *item;
> +    Visitor *v;
> +    AltListInt *ali;
> +    int i;
> +
> +    v = visitor_input_test_init(data, "[ 42, 43, 44 ]");
> +    visit_type_AltListInt(v, NULL, &ali, &error_abort);
> +    g_assert(ali != NULL);
> +
> +    g_assert_cmpint(ali->type, ==, QTYPE_QLIST);
> +    for (i = 0, item = ali->u.l; item; item = item->next, i++) {
> +        char string[12];
> +
> +        snprintf(string, sizeof(string), "string%d", i);

This appears to be unused.  Can drop in my tree.

> +        g_assert_cmpint(item->value, ==, 42 + i);
> +    }
> +
> +    qapi_free_AltListInt(ali);
> +    ali = NULL;
> +
> +    /* An empty list is valid */
> +    v = visitor_input_test_init(data, "[]");
> +    visit_type_AltListInt(v, NULL, &ali, &error_abort);
> +    g_assert(ali != NULL);
> +
> +    g_assert_cmpint(ali->type, ==, QTYPE_QLIST);
> +    g_assert(!ali->u.l);
> +    qapi_free_AltListInt(ali);
> +    ali = NULL;
> +}
> +
>  static void input_visitor_test_add(const char *testpath,
>                                     const void *user_data,
>                                     void (*test_func)(TestInputVisitorData *data,
> @@ -1188,6 +1229,8 @@ int main(int argc, char **argv)
>                             NULL, test_visitor_in_wrong_type);
>      input_visitor_test_add("/visitor/input/alternate-number",
>                             NULL, test_visitor_in_alternate_number);
> +    input_visitor_test_add("/visitor/input/alternate-list",
> +                           NULL, test_visitor_in_alternate_list);
>      input_visitor_test_add("/visitor/input/fail/struct",
>                             NULL, test_visitor_in_fail_struct);
>      input_visitor_test_add("/visitor/input/fail/struct-nested",

Other than that:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 1/3] qapi-schema: support alternates with array type

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Detect array types as alternate branches, and turn the JSON list into
> a QAPISchemaArrayType.  Array types in an alternate are represented with
> QTYPE_QLIST in the type field.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I double-checked the generated code; it looks good to me.

Thanks for implementing this, and extra thanks for the tests!

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 0/3] qapi-schema: support alternates with array type

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> As suggested in the review of the statistics subsystem.

Queued for 7.1, thanks!

Re: [PATCH 0/3] qapi-schema: support alternates with array type

[Paolo Bonzini]

On 4/4/22 14:13, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> As suggested in the review of the statistics subsystem.
> 
> Queued for 7.1, thanks!

Thanks, I will follow up with the stats patches after these land.

Paolo

Re: [PATCH 1/3] qapi-schema: support alternates with array type

[Paolo Bonzini]

On 3/22/22 10:48, Markus Armbruster wrote:
> I double-checked the generated code; it looks good to me.
> 
> Thanks for implementing this, and extra thanks for the tests!

Heh, the problem is having to build the infrastructure for tests.  If 
it's already there, patches like this are actually easier to write with 
tests than without!...

Paolo