[PATCH v3 0/3] qapi: allow unions to contain further unions

[PATCH v3 0/3] qapi: allow unions to contain further unions

[Daniel P. Berrangé]

Currently it is not possible for a union type to contain a
further union as one (or more) of its branches. This relaxes
that restriction and adds the calls needed to validate field
name uniqueness as unions are flattened.

In v3:

  * Use markus' suggestion for improved error messages
  * Set env var default in argparse directly

In v2:

  * Improve specificity of type/members descriptions for
    error reporting scenarios
  * Make it easier to regenerate qapi test output
  * Move expected "good data" into qapi-schema-test.json
  * Add description to "bad data" test files
  * Add unit tests to cover union-in-union serialization
    / deserialization to/from JSON

Daniel P. Berrangé (3):
  qapi: support updating expected test output via make
  qapi: improve specificity of type/member descriptions
  qapi: allow unions to contain further unions

 scripts/qapi/schema.py                        | 15 +++--
 tests/qapi-schema/meson.build                 |  2 +
 tests/qapi-schema/qapi-schema-test.json       | 32 ++++++++++
 tests/qapi-schema/qapi-schema-test.out        | 29 ++++++++++
 tests/qapi-schema/test-qapi.py                |  1 +
 .../union-invalid-union-subfield.err          |  2 +
 .../union-invalid-union-subfield.json         | 30 ++++++++++
 .../union-invalid-union-subfield.out          |  0
 .../union-invalid-union-subtype.err           |  2 +
 .../union-invalid-union-subtype.json          | 29 ++++++++++
 .../union-invalid-union-subtype.out           |  0
 tests/unit/test-qobject-input-visitor.c       | 47 +++++++++++++++
 tests/unit/test-qobject-output-visitor.c      | 58 +++++++++++++++++++
 13 files changed, 242 insertions(+), 5 deletions(-)
 create mode 100644 tests/qapi-schema/union-invalid-union-subfield.err
 create mode 100644 tests/qapi-schema/union-invalid-union-subfield.json
 create mode 100644 tests/qapi-schema/union-invalid-union-subfield.out
 create mode 100644 tests/qapi-schema/union-invalid-union-subtype.err
 create mode 100644 tests/qapi-schema/union-invalid-union-subtype.json
 create mode 100644 tests/qapi-schema/union-invalid-union-subtype.out

-- 
2.40.0

[PATCH v3 1/3] qapi: support updating expected test output via make

[Daniel P. Berrangé]

It is possible to pass --update to tests/qapi-schema/test-qapi.py
to make it update the output files on error. This is inconvenient
to achieve though when test-qapi.py is run indirectly by make/meson.

Instead simply allow for an env variable to be set:

 $ QAPI_TEST_UPDATE= make check-qapi-schema

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qapi-schema/test-qapi.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 2160cef082..d58c31f539 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -206,6 +206,7 @@ def main(argv):
     parser.add_argument('-d', '--dir', action='store', default='',
                         help="directory containing tests")
     parser.add_argument('-u', '--update', action='store_true',
+                        default='QAPI_TEST_UPDATE' in os.environ,
                         help="update expected test results")
     parser.add_argument('tests', nargs='*', metavar='TEST', action='store')
     args = parser.parse_args()
-- 
2.40.0

[PATCH v3 2/3] qapi: improve specificity of type/member descriptions

[Daniel P. Berrangé]

When describing member types always include the context of the
containing type. Although this is often redundant, in some cases
it will help to reduce ambiguity.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/qapi/schema.py | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 207e4d71f3..da04b97ded 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -697,6 +697,7 @@ def connect_doc(self, doc):
 
     def describe(self, info):
         role = self.role
+        meta = 'type'
         defined_in = self.defined_in
         assert defined_in
 
@@ -708,13 +709,17 @@ def describe(self, info):
                 # Implicit type created for a command's dict 'data'
                 assert role == 'member'
                 role = 'parameter'
+                meta = 'command'
+                defined_in = defined_in[:-4]
             elif defined_in.endswith('-base'):
                 # Implicit type created for a union's dict 'base'
                 role = 'base ' + role
+                defined_in = defined_in[:-5]
             else:
                 assert False
-        elif defined_in != info.defn_name:
-            return "%s '%s' of type '%s'" % (role, self.name, defined_in)
+
+        if defined_in != info.defn_name:
+            return "%s '%s' of %s '%s'" % (role, self.name, meta, defined_in)
         return "%s '%s'" % (role, self.name)
 
 
-- 
2.40.0

[PATCH v3 3/3] qapi: allow unions to contain further unions

[Daniel P. Berrangé]

This extends the QAPI schema validation to permit unions inside unions,
provided the checks for clashing fields pass.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/qapi/schema.py                        |  6 +-
 tests/qapi-schema/meson.build                 |  2 +
 tests/qapi-schema/qapi-schema-test.json       | 32 ++++++++++
 tests/qapi-schema/qapi-schema-test.out        | 29 ++++++++++
 .../union-invalid-union-subfield.err          |  2 +
 .../union-invalid-union-subfield.json         | 30 ++++++++++
 .../union-invalid-union-subfield.out          |  0
 .../union-invalid-union-subtype.err           |  2 +
 .../union-invalid-union-subtype.json          | 29 ++++++++++
 .../union-invalid-union-subtype.out           |  0
 tests/unit/test-qobject-input-visitor.c       | 47 +++++++++++++++
 tests/unit/test-qobject-output-visitor.c      | 58 +++++++++++++++++++
 12 files changed, 234 insertions(+), 3 deletions(-)
 create mode 100644 tests/qapi-schema/union-invalid-union-subfield.err
 create mode 100644 tests/qapi-schema/union-invalid-union-subfield.json
 create mode 100644 tests/qapi-schema/union-invalid-union-subfield.out
 create mode 100644 tests/qapi-schema/union-invalid-union-subtype.err
 create mode 100644 tests/qapi-schema/union-invalid-union-subtype.json
 create mode 100644 tests/qapi-schema/union-invalid-union-subtype.out

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index da04b97ded..edb09f2ed2 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -465,9 +465,10 @@ def check(self, schema):
     # on behalf of info, which is not necessarily self.info
     def check_clash(self, info, seen):
         assert self._checked
-        assert not self.variants       # not implemented
         for m in self.members:
             m.check_clash(info, seen)
+        if self.variants:
+            self.variants.check_clash(info, seen)
 
     def connect_doc(self, doc=None):
         super().connect_doc(doc)
@@ -652,8 +653,7 @@ def check(self, schema, seen):
                         self.info,
                         "branch '%s' is not a value of %s"
                         % (v.name, self.tag_member.type.describe()))
-                if (not isinstance(v.type, QAPISchemaObjectType)
-                        or v.type.variants):
+                if not isinstance(v.type, QAPISchemaObjectType):
                     raise QAPISemError(
                         self.info,
                         "%s cannot use %s"
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index d85b14f28c..1591eb322b 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -194,6 +194,8 @@ schemas = [
   'union-invalid-data.json',
   'union-invalid-discriminator.json',
   'union-invalid-if-discriminator.json',
+  'union-invalid-union-subfield.json',
+  'union-invalid-union-subtype.json',
   'union-no-base.json',
   'union-optional-discriminator.json',
   'union-string-discriminator.json',
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index ba7302f42b..40f1a3d88d 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -114,6 +114,38 @@
 { 'struct': 'UserDefC',
   'data': { 'string1': 'str', 'string2': 'str' } }
 
+# this tests that unions can contain other unions in their branches
+{ 'enum': 'TestUnionEnum',
+  'data': [ 'value-a', 'value-b' ] }
+
+{ 'enum': 'TestUnionEnumA',
+  'data': [ 'value-a1', 'value-a2' ] }
+
+{ 'struct': 'TestUnionTypeA1',
+  'data': { 'integer': 'int',
+            'name': 'str'} }
+
+{ 'struct': 'TestUnionTypeA2',
+  'data': { 'integer': 'int',
+            'size': 'int' } }
+
+{ 'union': 'TestUnionTypeA',
+  'base': { 'type-a': 'TestUnionEnumA' },
+  'discriminator': 'type-a',
+  'data': { 'value-a1': 'TestUnionTypeA1',
+            'value-a2': 'TestUnionTypeA2' } }
+
+{ 'struct': 'TestUnionTypeB',
+  'data': { 'integer': 'int',
+            'onoff': 'bool' } }
+
+{ 'union': 'TestUnionInUnion',
+  'base': { 'type': 'TestUnionEnum' },
+  'discriminator': 'type',
+  'data': { 'value-a': 'TestUnionTypeA',
+            'value-b': 'TestUnionTypeB' } }
+
+
 # for testing use of 'number' within alternates
 { 'alternate': 'AltEnumBool', 'data': { 'e': 'EnumOne', 'b': 'bool' } }
 { 'alternate': 'AltEnumNum', 'data': { 'e': 'EnumOne', 'n': 'number' } }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 043d75c655..9fe1038944 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -105,6 +105,35 @@ alternate UserDefAlternate
 object UserDefC
     member string1: str optional=False
     member string2: str optional=False
+enum TestUnionEnum
+    member value-a
+    member value-b
+enum TestUnionEnumA
+    member value-a1
+    member value-a2
+object TestUnionTypeA1
+    member integer: int optional=False
+    member name: str optional=False
+object TestUnionTypeA2
+    member integer: int optional=False
+    member size: int optional=False
+object q_obj_TestUnionTypeA-base
+    member type-a: TestUnionEnumA optional=False
+object TestUnionTypeA
+    base q_obj_TestUnionTypeA-base
+    tag type-a
+    case value-a1: TestUnionTypeA1
+    case value-a2: TestUnionTypeA2
+object TestUnionTypeB
+    member integer: int optional=False
+    member onoff: bool optional=False
+object q_obj_TestUnionInUnion-base
+    member type: TestUnionEnum optional=False
+object TestUnionInUnion
+    base q_obj_TestUnionInUnion-base
+    tag type
+    case value-a: TestUnionTypeA
+    case value-b: TestUnionTypeB
 alternate AltEnumBool
     tag type
     case e: EnumOne
diff --git a/tests/qapi-schema/union-invalid-union-subfield.err b/tests/qapi-schema/union-invalid-union-subfield.err
new file mode 100644
index 0000000000..91aa87bcd8
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-union-subfield.err
@@ -0,0 +1,2 @@
+union-invalid-union-subfield.json: In union 'TestUnion':
+union-invalid-union-subfield.json:25: member 'teeth' of type 'TestTypeFish' collides with base member 'teeth'
diff --git a/tests/qapi-schema/union-invalid-union-subfield.json b/tests/qapi-schema/union-invalid-union-subfield.json
new file mode 100644
index 0000000000..e1639d3a96
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-union-subfield.json
@@ -0,0 +1,30 @@
+# Clash between common member and union variant's variant member
+# Base's member 'teeth' clashes with TestTypeFish's
+
+{ 'enum': 'TestEnum',
+  'data': [ 'animals', 'plants' ] }
+
+{ 'enum': 'TestAnimals',
+  'data': [ 'fish', 'birds'] }
+
+{ 'struct': 'TestTypeFish',
+  'data': { 'scales': 'int', 'teeth': 'int' } }
+
+{ 'struct': 'TestTypeBirds',
+  'data': { 'feathers': 'int' } }
+
+{ 'union': 'TestTypeAnimals',
+  'base': { 'atype': 'TestAnimals' },
+  'discriminator': 'atype',
+  'data': { 'fish': 'TestTypeFish',
+            'birds': 'TestTypeBirds' } }
+
+{ 'struct': 'TestTypePlants',
+  'data': { 'integer': 'int' } }
+
+{ 'union': 'TestUnion',
+  'base': { 'type': 'TestEnum',
+            'teeth': 'int' },
+  'discriminator': 'type',
+  'data': { 'animals': 'TestTypeAnimals',
+            'plants': 'TestTypePlants' } }
diff --git a/tests/qapi-schema/union-invalid-union-subfield.out b/tests/qapi-schema/union-invalid-union-subfield.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/union-invalid-union-subtype.err b/tests/qapi-schema/union-invalid-union-subtype.err
new file mode 100644
index 0000000000..3538dc2e70
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-union-subtype.err
@@ -0,0 +1,2 @@
+union-invalid-union-subtype.json: In union 'TestUnion':
+union-invalid-union-subtype.json:25: base member 'type' of type 'TestTypeA' collides with base member 'type'
diff --git a/tests/qapi-schema/union-invalid-union-subtype.json b/tests/qapi-schema/union-invalid-union-subtype.json
new file mode 100644
index 0000000000..ce1de51d8d
--- /dev/null
+++ b/tests/qapi-schema/union-invalid-union-subtype.json
@@ -0,0 +1,29 @@
+# Clash between common member and union variant's common member
+# Base's member 'type' clashes with TestTypeA's
+
+{ 'enum': 'TestEnum',
+  'data': [ 'value-a', 'value-b' ] }
+
+{ 'enum': 'TestEnumA',
+  'data': [ 'value-a1', 'value-a2' ] }
+
+{ 'struct': 'TestTypeA1',
+  'data': { 'integer': 'int' } }
+
+{ 'struct': 'TestTypeA2',
+  'data': { 'integer': 'int' } }
+
+{ 'union': 'TestTypeA',
+  'base': { 'type': 'TestEnumA' },
+  'discriminator': 'type',
+  'data': { 'value-a1': 'TestTypeA1',
+            'value-a2': 'TestTypeA2' } }
+
+{ 'struct': 'TestTypeB',
+  'data': { 'integer': 'int' } }
+
+{ 'union': 'TestUnion',
+  'base': { 'type': 'TestEnum' },
+  'discriminator': 'type',
+  'data': { 'value-a': 'TestTypeA',
+            'value-b': 'TestTypeB' } }
diff --git a/tests/qapi-schema/union-invalid-union-subtype.out b/tests/qapi-schema/union-invalid-union-subtype.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/unit/test-qobject-input-visitor.c b/tests/unit/test-qobject-input-visitor.c
index 77fbf985be..9b3e2dbe14 100644
--- a/tests/unit/test-qobject-input-visitor.c
+++ b/tests/unit/test-qobject-input-visitor.c
@@ -706,6 +706,51 @@ static void test_visitor_in_union_flat(TestInputVisitorData *data,
     g_assert(&base->enum1 == &tmp->enum1);
 }
 
+static void test_visitor_in_union_in_union(TestInputVisitorData *data,
+                                           const void *unused)
+{
+    Visitor *v;
+    g_autoptr(TestUnionInUnion) tmp = NULL;
+
+    v = visitor_input_test_init(data,
+                                "{ 'type': 'value-a', "
+                                "  'type-a': 'value-a1', "
+                                "  'integer': 2, "
+                                "  'name': 'fish' }");
+
+    visit_type_TestUnionInUnion(v, NULL, &tmp, &error_abort);
+    g_assert_cmpint(tmp->type, ==, TEST_UNION_ENUM_VALUE_A);
+    g_assert_cmpint(tmp->u.value_a.type_a, ==, TEST_UNION_ENUMA_VALUE_A1);
+    g_assert_cmpint(tmp->u.value_a.u.value_a1.integer, ==, 2);
+    g_assert_cmpint(strcmp(tmp->u.value_a.u.value_a1.name, "fish"), ==, 0);
+
+    qapi_free_TestUnionInUnion(tmp);
+
+    v = visitor_input_test_init(data,
+                                "{ 'type': 'value-a', "
+                                "  'type-a': 'value-a2', "
+                                "  'integer': 1729, "
+                                "  'size': 87539319 }");
+
+    visit_type_TestUnionInUnion(v, NULL, &tmp, &error_abort);
+    g_assert_cmpint(tmp->type, ==, TEST_UNION_ENUM_VALUE_A);
+    g_assert_cmpint(tmp->u.value_a.type_a, ==, TEST_UNION_ENUMA_VALUE_A2);
+    g_assert_cmpint(tmp->u.value_a.u.value_a2.integer, ==, 1729);
+    g_assert_cmpint(tmp->u.value_a.u.value_a2.size, ==, 87539319);
+
+    qapi_free_TestUnionInUnion(tmp);
+
+    v = visitor_input_test_init(data,
+                                "{ 'type': 'value-b', "
+                                "  'integer': 1729, "
+                                "  'onoff': true }");
+
+    visit_type_TestUnionInUnion(v, NULL, &tmp, &error_abort);
+    g_assert_cmpint(tmp->type, ==, TEST_UNION_ENUM_VALUE_B);
+    g_assert_cmpint(tmp->u.value_b.integer, ==, 1729);
+    g_assert_cmpint(tmp->u.value_b.onoff, ==, true);
+}
+
 static void test_visitor_in_alternate(TestInputVisitorData *data,
                                       const void *unused)
 {
@@ -1216,6 +1261,8 @@ int main(int argc, char **argv)
                            NULL, test_visitor_in_null);
     input_visitor_test_add("/visitor/input/union-flat",
                            NULL, test_visitor_in_union_flat);
+    input_visitor_test_add("/visitor/input/union-in-union",
+                           NULL, test_visitor_in_union_in_union);
     input_visitor_test_add("/visitor/input/alternate",
                            NULL, test_visitor_in_alternate);
     input_visitor_test_add("/visitor/input/errors",
diff --git a/tests/unit/test-qobject-output-visitor.c b/tests/unit/test-qobject-output-visitor.c
index 7f054289fe..1535b3ad17 100644
--- a/tests/unit/test-qobject-output-visitor.c
+++ b/tests/unit/test-qobject-output-visitor.c
@@ -352,6 +352,62 @@ static void test_visitor_out_union_flat(TestOutputVisitorData *data,
     qapi_free_UserDefFlatUnion(tmp);
 }
 
+static void test_visitor_out_union_in_union(TestOutputVisitorData *data,
+                                            const void *unused)
+{
+    QDict *qdict;
+
+    TestUnionInUnion *tmp = g_new0(TestUnionInUnion, 1);
+    tmp->type = TEST_UNION_ENUM_VALUE_A;
+    tmp->u.value_a.type_a = TEST_UNION_ENUMA_VALUE_A1;
+    tmp->u.value_a.u.value_a1.integer = 42;
+    tmp->u.value_a.u.value_a1.name = g_strdup("fish");
+
+    visit_type_TestUnionInUnion(data->ov, NULL, &tmp, &error_abort);
+    qdict = qobject_to(QDict, visitor_get(data));
+    g_assert(qdict);
+    g_assert_cmpstr(qdict_get_str(qdict, "type"), ==, "value-a");
+    g_assert_cmpstr(qdict_get_str(qdict, "type-a"), ==, "value-a1");
+    g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 42);
+    g_assert_cmpstr(qdict_get_str(qdict, "name"), ==, "fish");
+
+    qapi_free_TestUnionInUnion(tmp);
+
+
+    visitor_reset(data);
+    tmp = g_new0(TestUnionInUnion, 1);
+    tmp->type = TEST_UNION_ENUM_VALUE_A;
+    tmp->u.value_a.type_a = TEST_UNION_ENUMA_VALUE_A2;
+    tmp->u.value_a.u.value_a2.integer = 1729;
+    tmp->u.value_a.u.value_a2.size = 87539319;
+
+    visit_type_TestUnionInUnion(data->ov, NULL, &tmp, &error_abort);
+    qdict = qobject_to(QDict, visitor_get(data));
+    g_assert(qdict);
+    g_assert_cmpstr(qdict_get_str(qdict, "type"), ==, "value-a");
+    g_assert_cmpstr(qdict_get_str(qdict, "type-a"), ==, "value-a2");
+    g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 1729);
+    g_assert_cmpint(qdict_get_int(qdict, "size"), ==, 87539319);
+
+    qapi_free_TestUnionInUnion(tmp);
+
+
+    visitor_reset(data);
+    tmp = g_new0(TestUnionInUnion, 1);
+    tmp->type = TEST_UNION_ENUM_VALUE_B;
+    tmp->u.value_b.integer = 1729;
+    tmp->u.value_b.onoff = true;
+
+    visit_type_TestUnionInUnion(data->ov, NULL, &tmp, &error_abort);
+    qdict = qobject_to(QDict, visitor_get(data));
+    g_assert(qdict);
+    g_assert_cmpstr(qdict_get_str(qdict, "type"), ==, "value-b");
+    g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 1729);
+    g_assert_cmpint(qdict_get_bool(qdict, "onoff"), ==, true);
+
+    qapi_free_TestUnionInUnion(tmp);
+}
+
 static void test_visitor_out_alternate(TestOutputVisitorData *data,
                                        const void *unused)
 {
@@ -586,6 +642,8 @@ int main(int argc, char **argv)
                             &out_visitor_data, test_visitor_out_list_qapi_free);
     output_visitor_test_add("/visitor/output/union-flat",
                             &out_visitor_data, test_visitor_out_union_flat);
+    output_visitor_test_add("/visitor/output/union-in-union",
+                            &out_visitor_data, test_visitor_out_union_in_union);
     output_visitor_test_add("/visitor/output/alternate",
                             &out_visitor_data, test_visitor_out_alternate);
     output_visitor_test_add("/visitor/output/null",
-- 
2.40.0

Re: [PATCH v3 2/3] qapi: improve specificity of type/member descriptions

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> When describing member types always include the context of the
> containing type. Although this is often redundant, in some cases
> it will help to reduce ambiguity.

This is no longer true.  It was in v2.  Suggest:

  Error messages describe object members, enumeration values, features,
  and variants like ROLE 'NAME', where ROLE is "member", "value",
  "feature", or "branch", respectively.  When the member is defined in
  another type, e.g. inherited from a base type, we add "of type
  'TYPE'".  Example: test case struct-base-clash-deep reports a member
  of type 'Sub' clashing with a member of its base type 'Base' as

      struct-base-clash-deep.json: In struct 'Sub':
      struct-base-clash-deep.json:10: member 'name' collides with member 'name' of type 'Base'

  Members of implicitly defined types need special treatment.  We don't
  want to add "of type 'TYPE'" for them, because their named are made up
  and mean nothing to the user.  Instead, we describe members of an
  implicitly defined base type as "base member 'NAME'", and command and
  event parameters as "parameter 'NAME'".  Example: test case
  union-bad-base reports member of a variant's type clashing with a
  member of its implicitly defined base type as

      union-bad-base.json: In union 'TestUnion':
      union-bad-base.json:8: member 'string' of type 'TestTypeA' collides with base member 'string'

  The next commit will permit unions as variant types.  "base member
  'NAME' would then be ambigious: is it the union's base, or is it the
  union's variant's base?  One of its test cases would report a clash
  between two such bases as "base member 'type' collides with base
  member 'type'".  Confusing.

  Refine the special treatment: add "of TYPE" even for implicitly
  defined types, but massage TYPE and ROLE so they make sense for the
  user.

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  scripts/qapi/schema.py | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 207e4d71f3..da04b97ded 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -697,6 +697,7 @@ def connect_doc(self, doc):
>  
>      def describe(self, info):
>          role = self.role
> +        meta = 'type'
>          defined_in = self.defined_in
>          assert defined_in
>  
> @@ -708,13 +709,17 @@ def describe(self, info):
>                  # Implicit type created for a command's dict 'data'
>                  assert role == 'member'
>                  role = 'parameter'
> +                meta = 'command'
> +                defined_in = defined_in[:-4]
>              elif defined_in.endswith('-base'):
>                  # Implicit type created for a union's dict 'base'
>                  role = 'base ' + role
> +                defined_in = defined_in[:-5]
>              else:
>                  assert False
> -        elif defined_in != info.defn_name:
> -            return "%s '%s' of type '%s'" % (role, self.name, defined_in)
> +
> +        if defined_in != info.defn_name:
> +            return "%s '%s' of %s '%s'" % (role, self.name, meta, defined_in)
>          return "%s '%s'" % (role, self.name)

Since I rewrote both the patch and the commit message, would you like me
to take the blame and claim authorship?

Re: [PATCH v3 1/3] qapi: support updating expected test output via make

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> It is possible to pass --update to tests/qapi-schema/test-qapi.py
> to make it update the output files on error. This is inconvenient
> to achieve though when test-qapi.py is run indirectly by make/meson.
>
> Instead simply allow for an env variable to be set:
>
>  $ QAPI_TEST_UPDATE= make check-qapi-schema
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  tests/qapi-schema/test-qapi.py | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 2160cef082..d58c31f539 100755
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -206,6 +206,7 @@ def main(argv):
>      parser.add_argument('-d', '--dir', action='store', default='',
>                          help="directory containing tests")
>      parser.add_argument('-u', '--update', action='store_true',
> +                        default='QAPI_TEST_UPDATE' in os.environ,
>                          help="update expected test results")
>      parser.add_argument('tests', nargs='*', metavar='TEST', action='store')
>      args = parser.parse_args()

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

Re: [PATCH v3 2/3] qapi: improve specificity of type/member descriptions

[Daniel P. Berrangé]

On Mon, Apr 24, 2023 at 01:38:21PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > When describing member types always include the context of the
> > containing type. Although this is often redundant, in some cases
> > it will help to reduce ambiguity.
> 
> This is no longer true.  It was in v2.  Suggest:
> 
>   Error messages describe object members, enumeration values, features,
>   and variants like ROLE 'NAME', where ROLE is "member", "value",
>   "feature", or "branch", respectively.  When the member is defined in
>   another type, e.g. inherited from a base type, we add "of type
>   'TYPE'".  Example: test case struct-base-clash-deep reports a member
>   of type 'Sub' clashing with a member of its base type 'Base' as
> 
>       struct-base-clash-deep.json: In struct 'Sub':
>       struct-base-clash-deep.json:10: member 'name' collides with member 'name' of type 'Base'
> 
>   Members of implicitly defined types need special treatment.  We don't
>   want to add "of type 'TYPE'" for them, because their named are made up
>   and mean nothing to the user.  Instead, we describe members of an
>   implicitly defined base type as "base member 'NAME'", and command and
>   event parameters as "parameter 'NAME'".  Example: test case
>   union-bad-base reports member of a variant's type clashing with a
>   member of its implicitly defined base type as
> 
>       union-bad-base.json: In union 'TestUnion':
>       union-bad-base.json:8: member 'string' of type 'TestTypeA' collides with base member 'string'
> 
>   The next commit will permit unions as variant types.  "base member
>   'NAME' would then be ambigious: is it the union's base, or is it the
>   union's variant's base?  One of its test cases would report a clash
>   between two such bases as "base member 'type' collides with base
>   member 'type'".  Confusing.
> 
>   Refine the special treatment: add "of TYPE" even for implicitly
>   defined types, but massage TYPE and ROLE so they make sense for the
>   user.
> 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  scripts/qapi/schema.py | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index 207e4d71f3..da04b97ded 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -697,6 +697,7 @@ def connect_doc(self, doc):
> >  
> >      def describe(self, info):
> >          role = self.role
> > +        meta = 'type'
> >          defined_in = self.defined_in
> >          assert defined_in
> >  
> > @@ -708,13 +709,17 @@ def describe(self, info):
> >                  # Implicit type created for a command's dict 'data'
> >                  assert role == 'member'
> >                  role = 'parameter'
> > +                meta = 'command'
> > +                defined_in = defined_in[:-4]
> >              elif defined_in.endswith('-base'):
> >                  # Implicit type created for a union's dict 'base'
> >                  role = 'base ' + role
> > +                defined_in = defined_in[:-5]
> >              else:
> >                  assert False
> > -        elif defined_in != info.defn_name:
> > -            return "%s '%s' of type '%s'" % (role, self.name, defined_in)
> > +
> > +        if defined_in != info.defn_name:
> > +            return "%s '%s' of %s '%s'" % (role, self.name, meta, defined_in)
> >          return "%s '%s'" % (role, self.name)
> 
> Since I rewrote both the patch and the commit message, would you like me
> to take the blame and claim authorship?

Yes, I should have credited you as the author here since it was just
taking your proposed code. The suggested commit message looks fine too

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 2/3] qapi: improve specificity of type/member descriptions

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Apr 24, 2023 at 01:38:21PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > When describing member types always include the context of the
>> > containing type. Although this is often redundant, in some cases
>> > it will help to reduce ambiguity.
>> 
>> This is no longer true.  It was in v2.  Suggest:
>> 
>>   Error messages describe object members, enumeration values, features,
>>   and variants like ROLE 'NAME', where ROLE is "member", "value",
>>   "feature", or "branch", respectively.  When the member is defined in
>>   another type, e.g. inherited from a base type, we add "of type
>>   'TYPE'".  Example: test case struct-base-clash-deep reports a member
>>   of type 'Sub' clashing with a member of its base type 'Base' as
>> 
>>       struct-base-clash-deep.json: In struct 'Sub':
>>       struct-base-clash-deep.json:10: member 'name' collides with member 'name' of type 'Base'
>> 
>>   Members of implicitly defined types need special treatment.  We don't
>>   want to add "of type 'TYPE'" for them, because their named are made up
>>   and mean nothing to the user.  Instead, we describe members of an
>>   implicitly defined base type as "base member 'NAME'", and command and
>>   event parameters as "parameter 'NAME'".  Example: test case
>>   union-bad-base reports member of a variant's type clashing with a
>>   member of its implicitly defined base type as
>> 
>>       union-bad-base.json: In union 'TestUnion':
>>       union-bad-base.json:8: member 'string' of type 'TestTypeA' collides with base member 'string'
>> 
>>   The next commit will permit unions as variant types.  "base member
>>   'NAME' would then be ambigious: is it the union's base, or is it the
>>   union's variant's base?  One of its test cases would report a clash
>>   between two such bases as "base member 'type' collides with base
>>   member 'type'".  Confusing.
>> 
>>   Refine the special treatment: add "of TYPE" even for implicitly
>>   defined types, but massage TYPE and ROLE so they make sense for the
>>   user.
>> 
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> > ---
>> >  scripts/qapi/schema.py | 9 +++++++--
>> >  1 file changed, 7 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> > index 207e4d71f3..da04b97ded 100644
>> > --- a/scripts/qapi/schema.py
>> > +++ b/scripts/qapi/schema.py
>> > @@ -697,6 +697,7 @@ def connect_doc(self, doc):
>> >  
>> >      def describe(self, info):
>> >          role = self.role
>> > +        meta = 'type'
>> >          defined_in = self.defined_in
>> >          assert defined_in
>> >  
>> > @@ -708,13 +709,17 @@ def describe(self, info):
>> >                  # Implicit type created for a command's dict 'data'
>> >                  assert role == 'member'
>> >                  role = 'parameter'
>> > +                meta = 'command'
>> > +                defined_in = defined_in[:-4]
>> >              elif defined_in.endswith('-base'):
>> >                  # Implicit type created for a union's dict 'base'
>> >                  role = 'base ' + role
>> > +                defined_in = defined_in[:-5]
>> >              else:
>> >                  assert False
>> > -        elif defined_in != info.defn_name:
>> > -            return "%s '%s' of type '%s'" % (role, self.name, defined_in)
>> > +
>> > +        if defined_in != info.defn_name:
>> > +            return "%s '%s' of %s '%s'" % (role, self.name, meta, defined_in)
>> >          return "%s '%s'" % (role, self.name)
>> 
>> Since I rewrote both the patch and the commit message, would you like me
>> to take the blame and claim authorship?
>
> Yes, I should have credited you as the author here since it was just
> taking your proposed code. The suggested commit message looks fine too

Thanks!  May I add your R-by in my tree?

Re: [PATCH v3 2/3] qapi: improve specificity of type/member descriptions

[Daniel P. Berrangé]

On Tue, Apr 25, 2023 at 03:17:52PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Mon, Apr 24, 2023 at 01:38:21PM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > When describing member types always include the context of the
> >> > containing type. Although this is often redundant, in some cases
> >> > it will help to reduce ambiguity.
> >> 
> >> This is no longer true.  It was in v2.  Suggest:
> >> 
> >>   Error messages describe object members, enumeration values, features,
> >>   and variants like ROLE 'NAME', where ROLE is "member", "value",
> >>   "feature", or "branch", respectively.  When the member is defined in
> >>   another type, e.g. inherited from a base type, we add "of type
> >>   'TYPE'".  Example: test case struct-base-clash-deep reports a member
> >>   of type 'Sub' clashing with a member of its base type 'Base' as
> >> 
> >>       struct-base-clash-deep.json: In struct 'Sub':
> >>       struct-base-clash-deep.json:10: member 'name' collides with member 'name' of type 'Base'
> >> 
> >>   Members of implicitly defined types need special treatment.  We don't
> >>   want to add "of type 'TYPE'" for them, because their named are made up
> >>   and mean nothing to the user.  Instead, we describe members of an
> >>   implicitly defined base type as "base member 'NAME'", and command and
> >>   event parameters as "parameter 'NAME'".  Example: test case
> >>   union-bad-base reports member of a variant's type clashing with a
> >>   member of its implicitly defined base type as
> >> 
> >>       union-bad-base.json: In union 'TestUnion':
> >>       union-bad-base.json:8: member 'string' of type 'TestTypeA' collides with base member 'string'
> >> 
> >>   The next commit will permit unions as variant types.  "base member
> >>   'NAME' would then be ambigious: is it the union's base, or is it the
> >>   union's variant's base?  One of its test cases would report a clash
> >>   between two such bases as "base member 'type' collides with base
> >>   member 'type'".  Confusing.
> >> 
> >>   Refine the special treatment: add "of TYPE" even for implicitly
> >>   defined types, but massage TYPE and ROLE so they make sense for the
> >>   user.
> >> 
> >> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> >> > ---
> >> >  scripts/qapi/schema.py | 9 +++++++--
> >> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> >> > index 207e4d71f3..da04b97ded 100644
> >> > --- a/scripts/qapi/schema.py
> >> > +++ b/scripts/qapi/schema.py
> >> > @@ -697,6 +697,7 @@ def connect_doc(self, doc):
> >> >  
> >> >      def describe(self, info):
> >> >          role = self.role
> >> > +        meta = 'type'
> >> >          defined_in = self.defined_in
> >> >          assert defined_in
> >> >  
> >> > @@ -708,13 +709,17 @@ def describe(self, info):
> >> >                  # Implicit type created for a command's dict 'data'
> >> >                  assert role == 'member'
> >> >                  role = 'parameter'
> >> > +                meta = 'command'
> >> > +                defined_in = defined_in[:-4]
> >> >              elif defined_in.endswith('-base'):
> >> >                  # Implicit type created for a union's dict 'base'
> >> >                  role = 'base ' + role
> >> > +                defined_in = defined_in[:-5]
> >> >              else:
> >> >                  assert False
> >> > -        elif defined_in != info.defn_name:
> >> > -            return "%s '%s' of type '%s'" % (role, self.name, defined_in)
> >> > +
> >> > +        if defined_in != info.defn_name:
> >> > +            return "%s '%s' of %s '%s'" % (role, self.name, meta, defined_in)
> >> >          return "%s '%s'" % (role, self.name)
> >> 
> >> Since I rewrote both the patch and the commit message, would you like me
> >> to take the blame and claim authorship?
> >
> > Yes, I should have credited you as the author here since it was just
> > taking your proposed code. The suggested commit message looks fine too
> 
> Thanks!  May I add your R-by in my tree?

Certainly

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 0/3] qapi: allow unions to contain further unions

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> Currently it is not possible for a union type to contain a
> further union as one (or more) of its branches. This relaxes
> that restriction and adds the calls needed to validate field
> name uniqueness as unions are flattened.

Queued.  Thanks!