[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V7 09/11] qapi script: do not allow string discr
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH V7 09/11] qapi script: do not allow string discriminator |
Date: |
Thu, 20 Feb 2014 17:50:21 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) |
Wenchao Xia <address@hidden> writes:
> Signed-off-by: Wenchao Xia <address@hidden>
> ---
> docs/qapi-code-gen.txt | 8 +++-----
> scripts/qapi.py | 6 ++++++
> tests/qapi-schema/qapi-schema-test.json | 9 ++++++---
> tests/qapi-schema/qapi-schema-test.out | 5 +++--
> tests/test-qmp-input-strict.c | 5 ++++-
> tests/test-qmp-input-visitor.c | 10 +++++++---
> tests/test-qmp-output-visitor.c | 10 ++++++----
> 7 files changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index a2e7921..c92add9 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -123,11 +123,9 @@ And it looks like this on the wire:
>
> Flat union types avoid the nesting on the wire. They are used whenever a
> specific field of the base type is declared as the discriminator ('type' is
> -then no longer generated). The discriminator can be a string field or a
> -predefined enum field. If it is a string field, a hidden enum type will be
> -generated as "[UNION_NAME]Kind". If it is an enum field, a compile time check
> -will be done to verify the correctness. It is recommended to use an enum
> field.
> -The above example can then be modified as follows:
> +then no longer generated). The discriminator should be a predefined enum
> field,
> +and a compile time check will be done to verify the correctness. The above
> +example can then be modified as follows:
Suggest:
The discriminator must be of enumeration type. The above example...
>
> { 'enum': 'BlockdevDriver', 'data': [ 'raw', 'qcow2' ] }
> { 'type': 'BlockdevCommonOptions',
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 2a5eb59..c3c118b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -215,6 +215,7 @@ def check_union(expr_elem):
> enum_define = discriminator_find_enum_define(expr_elem)
> name = expr_elem['expr']['union']
> members = expr_elem['expr']['data']
> + discriminator = expr_elem['expr'].get('discriminator')
> if enum_define:
> for key in members:
> if not key in enum_define['enum_values']:
> @@ -228,6 +229,11 @@ def check_union(expr_elem):
> "Enum value '%s' is not covered by a "
> "branch of union '%s'" %
> (key, name))
> + elif discriminator:
> + # Do not allow string discriminator
> + raise QAPIExprError(expr_elem,
> + "Discriminator '%s' is not a pre-defined enum "
> + "type" % discriminator)
I have no idea what "pre-defined" means here :)
Suggest:
"Discriminator '%s' must be of enumeration type"
>
> def check_exprs(schema):
> for expr_elem in schema.exprs:
> diff --git a/tests/qapi-schema/qapi-schema-test.json
> b/tests/qapi-schema/qapi-schema-test.json
> index 471ba47..818c06d 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -37,10 +37,13 @@
> 'base': 'UserDefZero',
> 'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
>
> +{ 'type': 'UserDefUnionBase',
> + 'data': { 'string': 'str', 'enum1': 'EnumOne' } }
> +
> { 'union': 'UserDefFlatUnion',
> - 'base': 'UserDefOne',
> - 'discriminator': 'string',
> - 'data': { 'a' : 'UserDefA', 'b' : 'UserDefB' } }
> + 'base': 'UserDefUnionBase',
> + 'discriminator': 'enum1',
> + 'data': { 'value1' : 'UserDefA', 'value2' : 'UserDefB', 'value3' :
> 'UserDefB' } }
> # FIXME generated struct UserDefFlatUnion has members for direct base
> # UserDefOne, but lacks members for indirect base UserDefZero
>
> diff --git a/tests/qapi-schema/qapi-schema-test.out
> b/tests/qapi-schema/qapi-schema-test.out
> index 01685d4..6cd03f3 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -7,7 +7,8 @@
> OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean',
> 'bool')]))]),
> OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer',
> 'int')]))]),
> OrderedDict([('union', 'UserDefUnion'), ('base', 'UserDefZero'), ('data',
> OrderedDict([('a', 'UserDefA'), ('b', 'UserDefB')]))]),
> - OrderedDict([('union', 'UserDefFlatUnion'), ('base', 'UserDefOne'),
> ('discriminator', 'string'), ('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', 'UserDefAnonUnion'), ('discriminator',
> OrderedDict()), ('data', OrderedDict([('uda', 'UserDefA'), ('s', 'str'),
> ('i', 'int')]))]),
> OrderedDict([('union', 'UserDefNativeListUnion'), ('data',
> OrderedDict([('integer', ['int']), ('s8', ['int8']), ('s16', ['int16']),
> ('s32', ['int32']), ('s64', ['int64']), ('u8', ['uint8']), ('u16',
> ['uint16']), ('u32', ['uint32']), ('u64', ['uint64']), ('number',
> ['number']), ('boolean', ['bool']), ('string', ['str'])]))]),
> OrderedDict([('command', 'user_def_cmd'), ('data', OrderedDict())]),
> @@ -17,7 +18,6 @@
> OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64',
> ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'),
> ('*u64x', 'uint64')]))])]
> [{'enum_name': 'EnumOne', 'enum_values': ['value1', 'value2', 'value3']},
> {'enum_name': 'UserDefUnionKind', 'enum_values': None},
> - {'enum_name': 'UserDefFlatUnionKind', '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')]))]),
> @@ -27,4 +27,5 @@
> OrderedDict([('type', 'UserDefNested'), ('data', OrderedDict([('string0',
> 'str'), ('dict1', OrderedDict([('string1', 'str'), ('dict2',
> OrderedDict([('userdef1', 'UserDefOne'), ('string2', 'str')])), ('*dict3',
> OrderedDict([('userdef2', 'UserDefOne'), ('string3', 'str')]))]))]))]),
> OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean',
> 'bool')]))]),
> OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer',
> 'int')]))]),
> + OrderedDict([('type', 'UserDefUnionBase'), ('data', OrderedDict([('string',
> 'str'), ('enum1', 'EnumOne')]))]),
> OrderedDict([('type', 'UserDefOptions'), ('data', OrderedDict([('*i64',
> ['int']), ('*u64', ['uint64']), ('*u16', ['uint16']), ('*i64x', 'int'),
> ('*u64x', 'uint64')]))])]
> diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
> index 09fc1ef..c715a50 100644
> --- a/tests/test-qmp-input-strict.c
> +++ b/tests/test-qmp-input-strict.c
> @@ -146,7 +146,10 @@ static void
> test_validate_union_flat(TestInputVisitorData *data,
> Visitor *v;
> Error *errp = NULL;
>
> - v = validate_test_init(data, "{ 'string': 'a', 'boolean': true }");
> + v = validate_test_init(data,
> + "{ 'enum1': 'value1',\
> + 'string': 'str', \
> + 'boolean': true }");
> /* TODO when generator bug is fixed, add 'integer': 41 */
>
Please use string concatenation rather than line continuation to avoid
long lines, like this:
v = validate_test_init(data,
"{ 'enum1': 'value1', "
"'string': 'str', "
"'boolean': true }");
The existing tests don't bother to avoid long lines, but that's not your
fault.
> visit_type_UserDefFlatUnion(v, &tmp, NULL, &errp);
> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index f1ab541..b549746 100644
> --- a/tests/test-qmp-input-visitor.c
> +++ b/tests/test-qmp-input-visitor.c
> @@ -310,14 +310,18 @@ static void
> test_visitor_in_union_flat(TestInputVisitorData *data,
> Error *err = NULL;
> UserDefFlatUnion *tmp;
>
> - v = visitor_input_test_init(data, "{ 'string': 'a', 'boolean': true }");
> + v = visitor_input_test_init(data,
> + "{ 'enum1': 'value1',\
> + 'string': 'str', \
> + 'boolean': true }");
Likewise.
> /* TODO when generator bug is fixed, add 'integer': 41 */
>
> visit_type_UserDefFlatUnion(v, &tmp, NULL, &err);
> g_assert(err == NULL);
> - g_assert_cmpint(tmp->kind, ==, USER_DEF_UNION_KIND_A);
> + g_assert_cmpint(tmp->kind, ==, ENUM_ONE_VALUE1);
> + g_assert_cmpstr(tmp->string, ==, "str");
> /* TODO g_assert_cmpint(tmp->integer, ==, 41); */
> - g_assert_cmpint(tmp->a->boolean, ==, true);
> + g_assert_cmpint(tmp->value1->boolean, ==, true);
> qapi_free_UserDefFlatUnion(tmp);
> }
>
> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index 5613396..6f4abb7 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -449,10 +449,11 @@ static void
> test_visitor_out_union_flat(TestOutputVisitorData *data,
> Error *err = NULL;
>
> UserDefFlatUnion *tmp = g_malloc0(sizeof(UserDefFlatUnion));
> - tmp->kind = USER_DEF_UNION_KIND_A;
> - tmp->a = g_malloc0(sizeof(UserDefA));
> + tmp->kind = ENUM_ONE_VALUE1;
> + tmp->string = g_strdup("str");
> + tmp->value1 = g_malloc0(sizeof(UserDefA));
> /* TODO when generator bug is fixed: tmp->integer = 41; */
> - tmp->a->boolean = true;
> + tmp->value1->boolean = true;
>
> visit_type_UserDefFlatUnion(data->ov, &tmp, NULL, &err);
> g_assert(err == NULL);
> @@ -461,7 +462,8 @@ static void
> test_visitor_out_union_flat(TestOutputVisitorData *data,
> g_assert(qobject_type(arg) == QTYPE_QDICT);
> qdict = qobject_to_qdict(arg);
>
> - g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, "a");
> + g_assert_cmpstr(qdict_get_str(qdict, "enum1"), ==, "value1");
> + g_assert_cmpstr(qdict_get_str(qdict, "string"), ==, "str");
> /* TODO g_assert_cmpint(qdict_get_int(qdict, "integer"), ==, 41); */
> g_assert_cmpint(qdict_get_bool(qdict, "boolean"), ==, true);
- Re: [Qemu-devel] [PATCH V7 03/11] qapi-script: remember line number in schema parsing, (continued)
- [Qemu-devel] [PATCH V7 04/11] qapi script: check correctness of discriminator values in union, Wenchao Xia, 2014/02/20
- [Qemu-devel] [PATCH V7 10/11] qapi script: do not add "_" for every capitalized char in enum, Wenchao Xia, 2014/02/20
- [Qemu-devel] [PATCH V7 08/11] qapi: convert BlockdevOptions to use enum discriminator, Wenchao Xia, 2014/02/20
- [Qemu-devel] [PATCH V7 09/11] qapi script: do not allow string discriminator, Wenchao Xia, 2014/02/20
- Re: [Qemu-devel] [PATCH V7 09/11] qapi script: do not allow string discriminator,
Markus Armbruster <=
- [Qemu-devel] [PATCH V7 11/11] qapi test: add error path test for union, Wenchao Xia, 2014/02/20