Message ID | 1392875695-15627-10-git-send-email-xiawenc@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes: > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> > --- > 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);
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: { '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) 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 */ 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 }"); /* 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);
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- 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(-)