From a8d4a2e4d7e1a0207699de47142c9bdbf2cc8675 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 4 May 2015 09:05:07 -0600 Subject: [PATCH] qapi: Forbid base without discriminator in unions None of the existing QMP or QGA interfaces uses a union with a base type but no discriminator; it is easier to avoid this in the generator to save room for other future extensions more likely to be useful. An earlier commit added a union-base-no-discriminator test to ensure that we eventually give a decent error message; likewise, removing UserDefUnion outright is okay, because we moved all the tests we wish to keep into the tests of the simple union UserDefNativeListUnion in the previous commit. Now is the time to actually forbid simple union with base, and remove the last vestiges from the testsuite. Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Markus Armbruster --- scripts/qapi-types.py | 7 ++-- scripts/qapi-visit.py | 11 +++--- scripts/qapi.py | 20 +++++------ tests/qapi-schema/qapi-schema-test.json | 4 --- tests/qapi-schema/qapi-schema-test.out | 2 -- .../union-base-no-discriminator.err | 1 + .../union-base-no-discriminator.exit | 2 +- .../union-base-no-discriminator.json | 2 +- .../union-base-no-discriminator.out | 8 ----- tests/test-qmp-input-visitor.c | 19 ---------- tests/test-qmp-output-visitor.c | 36 ------------------- 11 files changed, 21 insertions(+), 91 deletions(-) diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index e400b0347..f6fb93027 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -242,10 +242,9 @@ struct %(name)s ''') if base: - base_fields = find_struct(base)['data'] - if discriminator: - base_fields = base_fields.copy() - del base_fields[discriminator] + assert discriminator + base_fields = find_struct(base)['data'].copy() + del base_fields[discriminator] ret += generate_struct_fields(base_fields) else: assert not discriminator diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 41596bb95..dbf0101cb 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -310,16 +310,15 @@ def generate_visit_union(expr): ret = "" disc_type = enum_define['enum_name'] else: - # There will always be a discriminator in the C switch code, by default it - # is an enum type generated silently as "'%sKind' % (name)" + # There will always be a discriminator in the C switch code, by default + # it is an enum type generated silently as "'%sKind' % (name)" ret = generate_visit_enum('%sKind' % name, members.keys()) disc_type = '%sKind' % (name) if base: - base_fields = find_struct(base)['data'] - if discriminator: - base_fields = base_fields.copy() - del base_fields[discriminator] + assert discriminator + base_fields = find_struct(base)['data'].copy() + del base_fields[discriminator] ret += generate_visit_struct_fields(name, "", "", base_fields) if discriminator: diff --git a/scripts/qapi.py b/scripts/qapi.py index 3ce8c3321..438468e3a 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -259,22 +259,22 @@ def check_union(expr, expr_info): discriminator = expr.get('discriminator') members = expr['data'] - # If the object has a member 'base', its value must name a complex type. - if base: + # If the object has a member 'base', its value must name a complex type, + # and there must be a discriminator. + if base is not None: + if discriminator is None: + raise QAPIExprError(expr_info, + "Union '%s' requires a discriminator to go " + "along with base" %name) base_fields = find_base_fields(base) if not base_fields: raise QAPIExprError(expr_info, "Base '%s' is not a valid type" % base) - # If the union object has no member 'discriminator', it's an - # ordinary union. - if not discriminator: - enum_define = None - - # Else if the value of member 'discriminator' is {}, it's an - # anonymous union. - elif discriminator == {}: + # If the union object has no member 'discriminator', it's a + # simple union. If 'discriminator' is {}, it is an anonymous union. + if not discriminator or discriminator == {}: enum_define = None # Else, it's a flat union. diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 84f0f0763..b134f3fa2 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -36,10 +36,6 @@ { 'type': 'UserDefC', 'data': { 'string1': 'str', 'string2': 'str' } } -{ 'union': 'UserDefUnion', - 'base': 'UserDefZero', - 'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } } - { 'type': 'UserDefUnionBase', 'data': { 'string': 'str', 'enum1': 'EnumOne' } } diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 915a61bcd..664ae7b4b 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -7,7 +7,6 @@ OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]), OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]), OrderedDict([('type', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]), - OrderedDict([('union', 'UserDefUnion'), ('base', 'UserDefZero'), ('data', OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]), OrderedDict([('type', 'UserDefUnionBase'), ('data', OrderedDict([('string', 'str'), ('enum1', 'EnumOne')]))]), OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefA'), ('value2', 'UserDefB'), ('value3', 'UserDefB')]))]), OrderedDict([('union', 'UserDefFlatUnion2'), ('base', 'UserDefUnionBase'), ('discriminator', 'enum1'), ('data', OrderedDict([('value1', 'UserDefC'), ('value2', 'UserDefB'), ('value3', 'UserDefA')]))]), @@ -24,7 +23,6 @@ OrderedDict([('event', 'EVENT_C'), ('data', OrderedDict([('*a', 'int'), ('*b', 'UserDefOne'), ('c', 'str')]))]), OrderedDict([('event', 'EVENT_D'), ('data', OrderedDict([('a', 'EventStructOne'), ('b', 'str'), ('*c', 'str'), ('*enum3', 'EnumOne')]))])] [{'enum_name': 'EnumOne', 'enum_values': ['value1', 'value2', 'value3']}, - {'enum_name': 'UserDefUnionKind', 'enum_values': None}, {'enum_name': 'UserDefAnonUnionKind', 'enum_values': None}, {'enum_name': 'UserDefNativeListUnionKind', 'enum_values': None}] [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]), diff --git a/tests/qapi-schema/union-base-no-discriminator.err b/tests/qapi-schema/union-base-no-discriminator.err index e69de29bb..fc8b79c45 100644 --- a/tests/qapi-schema/union-base-no-discriminator.err +++ b/tests/qapi-schema/union-base-no-discriminator.err @@ -0,0 +1 @@ +tests/qapi-schema/union-base-no-discriminator.json:11: Union 'TestUnion' requires a discriminator to go along with base diff --git a/tests/qapi-schema/union-base-no-discriminator.exit b/tests/qapi-schema/union-base-no-discriminator.exit index 573541ac9..d00491fd7 100644 --- a/tests/qapi-schema/union-base-no-discriminator.exit +++ b/tests/qapi-schema/union-base-no-discriminator.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/union-base-no-discriminator.json b/tests/qapi-schema/union-base-no-discriminator.json index c8cba3ad7..052596c46 100644 --- a/tests/qapi-schema/union-base-no-discriminator.json +++ b/tests/qapi-schema/union-base-no-discriminator.json @@ -1,4 +1,4 @@ -# FIXME: we should reject simple unions with a base +# we reject simple unions with a base (or flat unions without discriminator) { 'type': 'TestTypeA', 'data': { 'string': 'str' } } diff --git a/tests/qapi-schema/union-base-no-discriminator.out b/tests/qapi-schema/union-base-no-discriminator.out index 505fd57dc..e69de29bb 100644 --- a/tests/qapi-schema/union-base-no-discriminator.out +++ b/tests/qapi-schema/union-base-no-discriminator.out @@ -1,8 +0,0 @@ -[OrderedDict([('type', 'TestTypeA'), ('data', OrderedDict([('string', 'str')]))]), - OrderedDict([('type', 'TestTypeB'), ('data', OrderedDict([('integer', 'int')]))]), - OrderedDict([('type', 'Base'), ('data', OrderedDict([('string', 'str')]))]), - OrderedDict([('union', 'TestUnion'), ('base', 'Base'), ('data', OrderedDict([('value1', 'TestTypeA'), ('value2', 'TestTypeB')]))])] -[{'enum_name': 'TestUnionKind', 'enum_values': None}] -[OrderedDict([('type', 'TestTypeA'), ('data', OrderedDict([('string', 'str')]))]), - OrderedDict([('type', 'TestTypeB'), ('data', OrderedDict([('integer', 'int')]))]), - OrderedDict([('type', 'Base'), ('data', OrderedDict([('string', 'str')]))])] diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index 0039ff6f0..cc33f6461 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -293,23 +293,6 @@ static void test_visitor_in_list(TestInputVisitorData *data, qapi_free_UserDefOneList(head); } -static void test_visitor_in_union(TestInputVisitorData *data, - const void *unused) -{ - Visitor *v; - Error *err = NULL; - UserDefUnion *tmp; - - v = visitor_input_test_init(data, "{ 'type': 'b', 'integer': 41, 'data' : { 'integer': 42 } }"); - - visit_type_UserDefUnion(v, &tmp, NULL, &err); - g_assert(err == NULL); - g_assert_cmpint(tmp->kind, ==, USER_DEF_UNION_KIND_B); - g_assert_cmpint(tmp->integer, ==, 41); - g_assert_cmpint(tmp->b->integer, ==, 42); - qapi_free_UserDefUnion(tmp); -} - static void test_visitor_in_union_flat(TestInputVisitorData *data, const void *unused) { @@ -679,8 +662,6 @@ int main(int argc, char **argv) &in_visitor_data, test_visitor_in_struct_nested); input_visitor_test_add("/visitor/input/list", &in_visitor_data, test_visitor_in_list); - input_visitor_test_add("/visitor/input/union", - &in_visitor_data, test_visitor_in_union); input_visitor_test_add("/visitor/input/union-flat", &in_visitor_data, test_visitor_in_union_flat); input_visitor_test_add("/visitor/input/union-anon", diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index e5bf40fab..ebe6ea337 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -422,40 +422,6 @@ static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data, qapi_free_UserDefNestedList(head); } -static void test_visitor_out_union(TestOutputVisitorData *data, - const void *unused) -{ - QObject *arg, *qvalue; - QDict *qdict, *value; - - Error *err = NULL; - - UserDefUnion *tmp = g_malloc0(sizeof(UserDefUnion)); - tmp->kind = USER_DEF_UNION_KIND_A; - tmp->integer = 41; - tmp->a = g_malloc0(sizeof(UserDefA)); - tmp->a->boolean = true; - - visit_type_UserDefUnion(data->ov, &tmp, NULL, &err); - g_assert(err == NULL); - arg = qmp_output_get_qobject(data->qov); - - g_assert(qobject_type(arg) == QTYPE_QDICT); - qdict = qobject_to_qdict(arg); - - g_assert_cmpstr(qdict_get_str(qdict, "type"), ==, "a"); - g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 41); - - qvalue = qdict_get(qdict, "data"); - g_assert(data != NULL); - g_assert(qobject_type(qvalue) == QTYPE_QDICT); - value = qobject_to_qdict(qvalue); - g_assert_cmpint(qdict_get_bool(value, "boolean"), ==, true); - - qapi_free_UserDefUnion(tmp); - QDECREF(qdict); -} - static void test_visitor_out_union_flat(TestOutputVisitorData *data, const void *unused) { @@ -862,8 +828,6 @@ int main(int argc, char **argv) &out_visitor_data, test_visitor_out_list); output_visitor_test_add("/visitor/output/list-qapi-free", &out_visitor_data, test_visitor_out_list_qapi_free); - output_visitor_test_add("/visitor/output/union", - &out_visitor_data, test_visitor_out_union); output_visitor_test_add("/visitor/output/union-flat", &out_visitor_data, test_visitor_out_union_flat); output_visitor_test_add("/visitor/output/union-anon", -- 2.39.5