From: Markus Armbruster <armbru@redhat.com>
To: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: qemu-devel@nongnu.org, Eric Blake <eblake@redhat.com>,
Michael Roth <michael.roth@amd.com>,
Het Gala <het.gala@nutanix.com>
Subject: Re: [PATCH v2 3/3] qapi: allow unions to contain further unions
Date: Fri, 17 Mar 2023 16:48:57 +0100 [thread overview]
Message-ID: <87ilezfkeu.fsf@pond.sub.org> (raw)
In-Reply-To: <20230223134027.2294640-4-berrange@redhat.com> ("Daniel P. Berrangé"'s message of "Thu, 23 Feb 2023 13:40:27 +0000")
Daniel P. Berrangé <berrange@redhat.com> writes:
> This extends the QAPI schema validation to permit unions inside unions,
> provided the checks for clashing fields pass.
>
> 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 6c481ab0c0..5c4457f789 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"
I stared at the code some to convince myself this is complete.
> 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
Looks good to me. I also inspected the generated code; no complaints.
> 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..43574dea79
> --- /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' of type 'TestUnion-base'
Bad error message, see my review of PATCH 1.
> 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..e45f330cec
> --- /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-base' collides with base member 'type' of type 'TestUnion-base'
Likewise.
> 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",
Reviewed-by: Markus Armbruster <armbru@redhat.com>
next prev parent reply other threads:[~2023-03-17 15:50 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-23 13:40 [PATCH v2 0/3] qapi: allow unions to contain further unions Daniel P. Berrangé
2023-02-23 13:40 ` [PATCH v2 1/3] qapi: improve specificity of type/member descriptions Daniel P. Berrangé
2023-03-17 12:08 ` Markus Armbruster
2023-02-23 13:40 ` [PATCH v2 2/3] qapi: use env var to trigger qapi test output updates Daniel P. Berrangé
2023-02-24 19:28 ` Eric Blake
2023-03-17 12:05 ` Markus Armbruster
2023-02-23 13:40 ` [PATCH v2 3/3] qapi: allow unions to contain further unions Daniel P. Berrangé
2023-03-07 3:53 ` Het Gala
2023-03-17 15:48 ` Markus Armbruster [this message]
2023-03-17 15:55 ` [PATCH v2 0/3] " Markus Armbruster
2023-03-31 11:49 ` Het Gala
2023-04-14 6:32 ` Het Gala
2023-04-25 13:29 ` Markus Armbruster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87ilezfkeu.fsf@pond.sub.org \
--to=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=eblake@redhat.com \
--cc=het.gala@nutanix.com \
--cc=michael.roth@amd.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.