* [Qemu-devel] [PATCH 3/4] tests/qapi-schema: Avoid 'str' in alternate test cases
2017-05-22 16:42 [Qemu-devel] [PATCH 0/4] qapi: Handle some keyval fallout Markus Armbruster
2017-05-22 16:42 ` [Qemu-devel] [PATCH 1/4] qobject-input-visitor: Reject non-finite numbers with keyval Markus Armbruster
2017-05-22 16:42 ` [Qemu-devel] [PATCH 2/4] qapi: Document visit_type_any() issues with keyval input Markus Armbruster
@ 2017-05-22 16:42 ` Markus Armbruster
2017-05-22 17:55 ` Eric Blake
2017-05-22 16:42 ` [Qemu-devel] [PATCH 4/4] qapi: Reject alternates that can't work with keyval_parse() Markus Armbruster
2017-05-26 10:30 ` [Qemu-devel] [PATCH 0/4] qapi: Handle some keyval fallout Marc-André Lureau
4 siblings, 1 reply; 11+ messages in thread
From: Markus Armbruster @ 2017-05-22 16:42 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, mlureau
The next commit is going to make alternate members of type 'str'
conflict with other scalar types. Would break a few test cases that
don't actually require 'str'. Flip them from 'str' to 'bool' or
'EnumOne'.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
tests/qapi-schema/alternate-clash.json | 2 +-
tests/qapi-schema/alternate-nested.json | 2 +-
tests/qapi-schema/args-alternate.json | 2 +-
tests/qapi-schema/doc-bad-alternate-member.json | 2 +-
tests/qapi-schema/qapi-schema-test.json | 9 ++--
tests/qapi-schema/qapi-schema-test.out | 30 +++++++------
tests/qapi-schema/returns-alternate.json | 2 +-
tests/test-clone-visitor.c | 23 +++++-----
tests/test-qobject-input-visitor.c | 56 ++++++++++++-------------
tests/test-qobject-output-visitor.c | 4 +-
10 files changed, 68 insertions(+), 64 deletions(-)
diff --git a/tests/qapi-schema/alternate-clash.json b/tests/qapi-schema/alternate-clash.json
index 6d73bc5..9a59b88 100644
--- a/tests/qapi-schema/alternate-clash.json
+++ b/tests/qapi-schema/alternate-clash.json
@@ -5,4 +5,4 @@
# the implicit Alt1Kind enum, we would still have a collision with the
# resulting C union trying to have two members named 'a_b'.
{ 'alternate': 'Alt1',
- 'data': { 'a-b': 'str', 'a_b': 'int' } }
+ 'data': { 'a-b': 'bool', 'a_b': 'int' } }
diff --git a/tests/qapi-schema/alternate-nested.json b/tests/qapi-schema/alternate-nested.json
index 8e22186..f2b9632 100644
--- a/tests/qapi-schema/alternate-nested.json
+++ b/tests/qapi-schema/alternate-nested.json
@@ -1,5 +1,5 @@
# we reject a nested alternate branch
{ 'alternate': 'Alt1',
- 'data': { 'name': 'str', 'value': 'int' } }
+ 'data': { 'name': 'bool', 'value': 'int' } }
{ 'alternate': 'Alt2',
'data': { 'nested': 'Alt1', 'b': 'bool' } }
diff --git a/tests/qapi-schema/args-alternate.json b/tests/qapi-schema/args-alternate.json
index 69e94d4..824d69c 100644
--- a/tests/qapi-schema/args-alternate.json
+++ b/tests/qapi-schema/args-alternate.json
@@ -1,3 +1,3 @@
# we do not allow alternate arguments
-{ 'alternate': 'Alt', 'data': { 'case1': 'int', 'case2': 'str' } }
+{ 'alternate': 'Alt', 'data': { 'case1': 'int', 'case2': 'bool' } }
{ 'command': 'oops', 'data': 'Alt' }
diff --git a/tests/qapi-schema/doc-bad-alternate-member.json b/tests/qapi-schema/doc-bad-alternate-member.json
index 738635c..fa4143d 100644
--- a/tests/qapi-schema/doc-bad-alternate-member.json
+++ b/tests/qapi-schema/doc-bad-alternate-member.json
@@ -6,4 +6,4 @@
# @bb: b
##
{ 'alternate': 'AorB',
- 'data': { 'a': 'str', 'b': 'int' } }
+ 'data': { 'a': 'bool', 'b': 'int' } }
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 842ea3c..303f2b2 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -93,16 +93,17 @@
{ 'struct': 'WrapAlternate',
'data': { 'alt': 'UserDefAlternate' } }
{ 'alternate': 'UserDefAlternate',
- 'data': { 'udfu': 'UserDefFlatUnion', 's': 'str', 'i': 'int' } }
+ 'data': { 'udfu': 'UserDefFlatUnion', 'e': 'EnumOne', 'i': 'int' } }
{ 'struct': 'UserDefC',
'data': { 'string1': 'str', 'string2': 'str' } }
# for testing use of 'number' within alternates
-{ 'alternate': 'AltStrBool', 'data': { 's': 'str', 'b': 'bool' } }
-{ 'alternate': 'AltStrNum', 'data': { 's': 'str', 'n': 'number' } }
+{ 'alternate': 'AltEnumBool', 'data': { 'e': 'EnumOne', 'b': 'bool' } }
+{ 'alternate': 'AltEnumNum', 'data': { 'e': 'EnumOne', 'n': 'number' } }
{ 'alternate': 'AltNumStr', 'data': { 'n': 'number', 's': 'str' } }
-{ 'alternate': 'AltStrInt', 'data': { 's': 'str', 'i': 'int' } }
+{ 'alternate': 'AltNumEnum', 'data': { 'n': 'number', 'e': 'EnumOne' } }
+{ 'alternate': 'AltEnumInt', 'data': { 'e': 'EnumOne', 'i': 'int' } }
{ 'alternate': 'AltIntNum', 'data': { 'i': 'int', 'n': 'number' } }
{ 'alternate': 'AltNumInt', 'data': { 'n': 'number', 'i': 'int' } }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 9d99c4e..3081091 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -1,7 +1,23 @@
+alternate AltEnumBool
+ tag type
+ case e: EnumOne
+ case b: bool
+alternate AltEnumInt
+ tag type
+ case e: EnumOne
+ case i: int
+alternate AltEnumNum
+ tag type
+ case e: EnumOne
+ case n: number
alternate AltIntNum
tag type
case i: int
case n: number
+alternate AltNumEnum
+ tag type
+ case n: number
+ case e: EnumOne
alternate AltNumInt
tag type
case n: number
@@ -10,18 +26,6 @@ alternate AltNumStr
tag type
case n: number
case s: str
-alternate AltStrBool
- tag type
- case s: str
- case b: bool
-alternate AltStrInt
- tag type
- case s: str
- case i: int
-alternate AltStrNum
- tag type
- case s: str
- case n: number
event EVENT_A None
boxed=False
event EVENT_B None
@@ -66,7 +70,7 @@ object UserDefA
alternate UserDefAlternate
tag type
case udfu: UserDefFlatUnion
- case s: str
+ case e: EnumOne
case i: int
object UserDefB
member intb: int optional=False
diff --git a/tests/qapi-schema/returns-alternate.json b/tests/qapi-schema/returns-alternate.json
index 972390c..f873718 100644
--- a/tests/qapi-schema/returns-alternate.json
+++ b/tests/qapi-schema/returns-alternate.json
@@ -1,3 +1,3 @@
# we reject returns if it is an alternate type
-{ 'alternate': 'Alt', 'data': { 'a': 'int', 'b': 'str' } }
+{ 'alternate': 'Alt', 'data': { 'a': 'int', 'b': 'bool' } }
{ 'command': 'oops', 'returns': 'Alt' }
diff --git a/tests/test-clone-visitor.c b/tests/test-clone-visitor.c
index df0c045..9698216 100644
--- a/tests/test-clone-visitor.c
+++ b/tests/test-clone-visitor.c
@@ -42,29 +42,28 @@ static void test_clone_struct(void)
static void test_clone_alternate(void)
{
- AltStrBool *b_src, *s_src, *b_dst, *s_dst;
+ AltEnumBool *b_src, *s_src, *b_dst, *s_dst;
- b_src = g_new0(AltStrBool, 1);
+ b_src = g_new0(AltEnumBool, 1);
b_src->type = QTYPE_QBOOL;
b_src->u.b = true;
- s_src = g_new0(AltStrBool, 1);
+ s_src = g_new0(AltEnumBool, 1);
s_src->type = QTYPE_QSTRING;
- s_src->u.s = g_strdup("World");
+ s_src->u.e = ENUM_ONE_VALUE1;
- b_dst = QAPI_CLONE(AltStrBool, b_src);
+ b_dst = QAPI_CLONE(AltEnumBool, b_src);
g_assert(b_dst);
g_assert_cmpint(b_dst->type, ==, b_src->type);
g_assert_cmpint(b_dst->u.b, ==, b_src->u.b);
- s_dst = QAPI_CLONE(AltStrBool, s_src);
+ s_dst = QAPI_CLONE(AltEnumBool, s_src);
g_assert(s_dst);
g_assert_cmpint(s_dst->type, ==, s_src->type);
- g_assert_cmpstr(s_dst->u.s, ==, s_src->u.s);
- g_assert(s_dst->u.s != s_src->u.s);
+ g_assert_cmpint(s_dst->u.e, ==, s_src->u.e);
- qapi_free_AltStrBool(b_src);
- qapi_free_AltStrBool(s_src);
- qapi_free_AltStrBool(b_dst);
- qapi_free_AltStrBool(s_dst);
+ qapi_free_AltEnumBool(b_src);
+ qapi_free_AltEnumBool(s_src);
+ qapi_free_AltEnumBool(b_dst);
+ qapi_free_AltEnumBool(s_dst);
}
static void test_clone_native_list(void)
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index e2aabbe..6b997a1 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -537,10 +537,10 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
g_assert_cmpint(tmp->u.i, ==, 42);
qapi_free_UserDefAlternate(tmp);
- v = visitor_input_test_init(data, "'string'");
+ v = visitor_input_test_init(data, "'value1'");
visit_type_UserDefAlternate(v, NULL, &tmp, &error_abort);
g_assert_cmpint(tmp->type, ==, QTYPE_QSTRING);
- g_assert_cmpstr(tmp->u.s, ==, "string");
+ g_assert_cmpint(tmp->u.e, ==, ENUM_ONE_VALUE1);
qapi_free_UserDefAlternate(tmp);
v = visitor_input_test_init(data, "{'integer':1, 'string':'str', "
@@ -565,10 +565,10 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
g_assert_cmpint(wrap->alt->u.i, ==, 42);
qapi_free_WrapAlternate(wrap);
- v = visitor_input_test_init(data, "{ 'alt': 'string' }");
+ v = visitor_input_test_init(data, "{ 'alt': 'value1' }");
visit_type_WrapAlternate(v, NULL, &wrap, &error_abort);
g_assert_cmpint(wrap->alt->type, ==, QTYPE_QSTRING);
- g_assert_cmpstr(wrap->alt->u.s, ==, "string");
+ g_assert_cmpint(wrap->alt->u.e, ==, ENUM_ONE_VALUE1);
qapi_free_WrapAlternate(wrap);
v = visitor_input_test_init(data, "{ 'alt': {'integer':1, 'string':'str', "
@@ -588,37 +588,37 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
{
Visitor *v;
Error *err = NULL;
- AltStrBool *asb;
- AltStrNum *asn;
- AltNumStr *ans;
- AltStrInt *asi;
+ AltEnumBool *aeb;
+ AltEnumNum *aen;
+ AltNumEnum *ans;
+ AltEnumInt *asi;
AltIntNum *ain;
AltNumInt *ani;
/* Parsing an int */
v = visitor_input_test_init(data, "42");
- visit_type_AltStrBool(v, NULL, &asb, &err);
+ visit_type_AltEnumBool(v, NULL, &aeb, &err);
error_free_or_abort(&err);
- qapi_free_AltStrBool(asb);
+ qapi_free_AltEnumBool(aeb);
v = visitor_input_test_init(data, "42");
- visit_type_AltStrNum(v, NULL, &asn, &error_abort);
- g_assert_cmpint(asn->type, ==, QTYPE_QFLOAT);
- g_assert_cmpfloat(asn->u.n, ==, 42);
- qapi_free_AltStrNum(asn);
+ visit_type_AltEnumNum(v, NULL, &aen, &error_abort);
+ g_assert_cmpint(aen->type, ==, QTYPE_QFLOAT);
+ g_assert_cmpfloat(aen->u.n, ==, 42);
+ qapi_free_AltEnumNum(aen);
v = visitor_input_test_init(data, "42");
- visit_type_AltNumStr(v, NULL, &ans, &error_abort);
+ visit_type_AltNumEnum(v, NULL, &ans, &error_abort);
g_assert_cmpint(ans->type, ==, QTYPE_QFLOAT);
g_assert_cmpfloat(ans->u.n, ==, 42);
- qapi_free_AltNumStr(ans);
+ qapi_free_AltNumEnum(ans);
v = visitor_input_test_init(data, "42");
- visit_type_AltStrInt(v, NULL, &asi, &error_abort);
+ visit_type_AltEnumInt(v, NULL, &asi, &error_abort);
g_assert_cmpint(asi->type, ==, QTYPE_QINT);
g_assert_cmpint(asi->u.i, ==, 42);
- qapi_free_AltStrInt(asi);
+ qapi_free_AltEnumInt(asi);
v = visitor_input_test_init(data, "42");
visit_type_AltIntNum(v, NULL, &ain, &error_abort);
@@ -635,26 +635,26 @@ static void test_visitor_in_alternate_number(TestInputVisitorData *data,
/* Parsing a double */
v = visitor_input_test_init(data, "42.5");
- visit_type_AltStrBool(v, NULL, &asb, &err);
+ visit_type_AltEnumBool(v, NULL, &aeb, &err);
error_free_or_abort(&err);
- qapi_free_AltStrBool(asb);
+ qapi_free_AltEnumBool(aeb);
v = visitor_input_test_init(data, "42.5");
- visit_type_AltStrNum(v, NULL, &asn, &error_abort);
- g_assert_cmpint(asn->type, ==, QTYPE_QFLOAT);
- g_assert_cmpfloat(asn->u.n, ==, 42.5);
- qapi_free_AltStrNum(asn);
+ visit_type_AltEnumNum(v, NULL, &aen, &error_abort);
+ g_assert_cmpint(aen->type, ==, QTYPE_QFLOAT);
+ g_assert_cmpfloat(aen->u.n, ==, 42.5);
+ qapi_free_AltEnumNum(aen);
v = visitor_input_test_init(data, "42.5");
- visit_type_AltNumStr(v, NULL, &ans, &error_abort);
+ visit_type_AltNumEnum(v, NULL, &ans, &error_abort);
g_assert_cmpint(ans->type, ==, QTYPE_QFLOAT);
g_assert_cmpfloat(ans->u.n, ==, 42.5);
- qapi_free_AltNumStr(ans);
+ qapi_free_AltNumEnum(ans);
v = visitor_input_test_init(data, "42.5");
- visit_type_AltStrInt(v, NULL, &asi, &err);
+ visit_type_AltEnumInt(v, NULL, &asi, &err);
error_free_or_abort(&err);
- qapi_free_AltStrInt(asi);
+ qapi_free_AltEnumInt(asi);
v = visitor_input_test_init(data, "42.5");
visit_type_AltIntNum(v, NULL, &ain, &error_abort);
diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c
index 94b9518..4e8fdf1 100644
--- a/tests/test-qobject-output-visitor.c
+++ b/tests/test-qobject-output-visitor.c
@@ -406,12 +406,12 @@ static void test_visitor_out_alternate(TestOutputVisitorData *data,
visitor_reset(data);
tmp = g_new0(UserDefAlternate, 1);
tmp->type = QTYPE_QSTRING;
- tmp->u.s = g_strdup("hello");
+ tmp->u.e = ENUM_ONE_VALUE1;
visit_type_UserDefAlternate(data->ov, NULL, &tmp, &error_abort);
qstr = qobject_to_qstring(visitor_get(data));
g_assert(qstr);
- g_assert_cmpstr(qstring_get_str(qstr), ==, "hello");
+ g_assert_cmpstr(qstring_get_str(qstr), ==, "value1");
qapi_free_UserDefAlternate(tmp);
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* [Qemu-devel] [PATCH 4/4] qapi: Reject alternates that can't work with keyval_parse()
2017-05-22 16:42 [Qemu-devel] [PATCH 0/4] qapi: Handle some keyval fallout Markus Armbruster
` (2 preceding siblings ...)
2017-05-22 16:42 ` [Qemu-devel] [PATCH 3/4] tests/qapi-schema: Avoid 'str' in alternate test cases Markus Armbruster
@ 2017-05-22 16:42 ` Markus Armbruster
2017-05-22 18:18 ` Eric Blake
2017-05-26 10:30 ` [Qemu-devel] [PATCH 0/4] qapi: Handle some keyval fallout Marc-André Lureau
4 siblings, 1 reply; 11+ messages in thread
From: Markus Armbruster @ 2017-05-22 16:42 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, mlureau
Alternates are sum types like unions, but use the JSON type on the
wire / QType in QObject instead of an explicit tag. That's why we
require alternate members to have distinct QTypes.
The recently introduced keyval_parse() (commit d454dbe) can only
produce string scalars. The qobject_input_visitor_new_keyval() input
visitor mostly hides the difference, so code using a QObject input
visitor doesn't have to care whether its input was parsed from JSON or
KEY=VALUE,... The difference leaks for alternates, as noted in commit
0ee9ae7: an non-string, non-enum scalar alternate value can't
currently be expressed.
In part, this is just our insufficiently sophisticated implementation.
Consider alternate type 'GuestFileWhence'. It has an integer member
and a 'QGASeek' member. The latter is an enumeration with values
'set', 'cur', 'end'. The meaning of b=set, b=cur, b=end, b=0, b=1 and
so forth is perfectly obvious. However, our current implementation
falls apart at run time for b=0, b=1, and so forth. Fixable, but not
today; add a test case and a TODO comment.
Now consider an alternate type with a string and an integer member.
What's the meaning of a=42? Is it the string "42" or the integer 42?
Whichever meaning you pick makes the other inexpressible. This isn't
just an implementation problem, it's fundamental. Our current
implementation will pick string.
So far, we haven't needed such alternates. To make sure we stop and
think before we add one that cannot sanely work with keyval_parse(),
let's require alternate members to have sufficiently district
representation in KEY=VALUE,... syntax:
* A string member clashes with any other scalar member
* An enumeration member clashes with bool members when it has value
'on' or 'off'.
* An enumeration member clashes with numeric members when it has a
value that starts with '-', '+', '0' or '9'. This is a rather lazy
approximation of the actual number syntax accepted by the visitor.
Note that enumeration values starting with '-' and '+' are rejected
elsewhere already, but better safe than sorry.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
scripts/qapi.py | 19 +++++++++++++++++--
tests/Makefile.include | 2 ++
tests/qapi-schema/alternate-conflict-dict.json | 2 +-
tests/qapi-schema/alternate-conflict-enum-bool.err | 1 +
tests/qapi-schema/alternate-conflict-enum-bool.exit | 1 +
tests/qapi-schema/alternate-conflict-enum-bool.json | 6 ++++++
tests/qapi-schema/alternate-conflict-enum-bool.out | 0
tests/qapi-schema/alternate-conflict-enum-int.err | 1 +
tests/qapi-schema/alternate-conflict-enum-int.exit | 1 +
tests/qapi-schema/alternate-conflict-enum-int.json | 6 ++++++
tests/qapi-schema/alternate-conflict-enum-int.out | 0
tests/qapi-schema/alternate-conflict-string.err | 2 +-
tests/qapi-schema/alternate-conflict-string.json | 6 ++----
tests/qapi-schema/qapi-schema-test.json | 4 +++-
tests/qapi-schema/qapi-schema-test.out | 4 ++--
tests/test-keyval.c | 18 +++++++++++-------
util/keyval.c | 10 +++++-----
17 files changed, 60 insertions(+), 23 deletions(-)
create mode 100644 tests/qapi-schema/alternate-conflict-enum-bool.err
create mode 100644 tests/qapi-schema/alternate-conflict-enum-bool.exit
create mode 100644 tests/qapi-schema/alternate-conflict-enum-bool.json
create mode 100644 tests/qapi-schema/alternate-conflict-enum-bool.out
create mode 100644 tests/qapi-schema/alternate-conflict-enum-int.err
create mode 100644 tests/qapi-schema/alternate-conflict-enum-int.exit
create mode 100644 tests/qapi-schema/alternate-conflict-enum-int.json
create mode 100644 tests/qapi-schema/alternate-conflict-enum-int.out
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 6c4d554..b7a25e4 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -812,11 +812,26 @@ def check_alternate(expr, info):
if not qtype:
raise QAPISemError(info, "Alternate '%s' member '%s' cannot use "
"type '%s'" % (name, key, value))
- if qtype in types_seen:
+ conflicting = set([qtype])
+ if qtype == 'QTYPE_QSTRING':
+ enum_expr = enum_types.get(value)
+ if enum_expr:
+ for v in enum_expr['data']:
+ if v in ['on', 'off']:
+ conflicting.add('QTYPE_QBOOL')
+ if re.match(r'[-+0-9.]', v): # lazy, could be tightened
+ conflicting.add('QTYPE_QINT')
+ conflicting.add('QTYPE_QFLOAT')
+ else:
+ conflicting.add('QTYPE_QINT')
+ conflicting.add('QTYPE_QFLOAT')
+ conflicting.add('QTYPE_QBOOL')
+ if conflicting & set(types_seen):
raise QAPISemError(info, "Alternate '%s' member '%s' can't "
"be distinguished from member '%s'"
% (name, key, types_seen[qtype]))
- types_seen[qtype] = key
+ for qt in conflicting:
+ types_seen[qt] = key
def check_enum(expr, info):
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 4277597..92116de 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -342,6 +342,8 @@ qapi-schema += alternate-array.json
qapi-schema += alternate-base.json
qapi-schema += alternate-clash.json
qapi-schema += alternate-conflict-dict.json
+qapi-schema += alternate-conflict-enum-bool.json
+qapi-schema += alternate-conflict-enum-int.json
qapi-schema += alternate-conflict-string.json
qapi-schema += alternate-empty.json
qapi-schema += alternate-nested.json
diff --git a/tests/qapi-schema/alternate-conflict-dict.json b/tests/qapi-schema/alternate-conflict-dict.json
index d566cca..3d78812 100644
--- a/tests/qapi-schema/alternate-conflict-dict.json
+++ b/tests/qapi-schema/alternate-conflict-dict.json
@@ -1,4 +1,4 @@
-# we reject alternates with multiple object branches
+# alternate branches of object type conflict with each other
{ 'struct': 'One',
'data': { 'name': 'str' } }
{ 'struct': 'Two',
diff --git a/tests/qapi-schema/alternate-conflict-enum-bool.err b/tests/qapi-schema/alternate-conflict-enum-bool.err
new file mode 100644
index 0000000..0dfc002
--- /dev/null
+++ b/tests/qapi-schema/alternate-conflict-enum-bool.err
@@ -0,0 +1 @@
+tests/qapi-schema/alternate-conflict-enum-bool.json:4: Alternate 'Alt' member 'two' can't be distinguished from member 'one'
diff --git a/tests/qapi-schema/alternate-conflict-enum-bool.exit b/tests/qapi-schema/alternate-conflict-enum-bool.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/alternate-conflict-enum-bool.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/alternate-conflict-enum-bool.json b/tests/qapi-schema/alternate-conflict-enum-bool.json
new file mode 100644
index 0000000..bff25c3
--- /dev/null
+++ b/tests/qapi-schema/alternate-conflict-enum-bool.json
@@ -0,0 +1,6 @@
+# alternate branch of 'enum' type that conflicts with bool
+{ 'enum': 'Enum',
+ 'data': [ 'aus', 'off' ] }
+{ 'alternate': 'Alt',
+ 'data': { 'one': 'Enum',
+ 'two': 'bool' } }
diff --git a/tests/qapi-schema/alternate-conflict-enum-bool.out b/tests/qapi-schema/alternate-conflict-enum-bool.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/alternate-conflict-enum-int.err b/tests/qapi-schema/alternate-conflict-enum-int.err
new file mode 100644
index 0000000..2cc8e7b
--- /dev/null
+++ b/tests/qapi-schema/alternate-conflict-enum-int.err
@@ -0,0 +1 @@
+tests/qapi-schema/alternate-conflict-enum-int.json:4: Alternate 'Alt' member 'two' can't be distinguished from member 'one'
diff --git a/tests/qapi-schema/alternate-conflict-enum-int.exit b/tests/qapi-schema/alternate-conflict-enum-int.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/alternate-conflict-enum-int.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/alternate-conflict-enum-int.json b/tests/qapi-schema/alternate-conflict-enum-int.json
new file mode 100644
index 0000000..beb428c
--- /dev/null
+++ b/tests/qapi-schema/alternate-conflict-enum-int.json
@@ -0,0 +1,6 @@
+# alternate branches of 'enum' type that conflicts with numbers
+{ 'enum': 'Enum',
+ 'data': [ '1', '2', '3' ] }
+{ 'alternate': 'Alt',
+ 'data': { 'one': 'Enum',
+ 'two': 'int' } }
diff --git a/tests/qapi-schema/alternate-conflict-enum-int.out b/tests/qapi-schema/alternate-conflict-enum-int.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/alternate-conflict-string.err b/tests/qapi-schema/alternate-conflict-string.err
index fc523b0..fe2f188 100644
--- a/tests/qapi-schema/alternate-conflict-string.err
+++ b/tests/qapi-schema/alternate-conflict-string.err
@@ -1 +1 @@
-tests/qapi-schema/alternate-conflict-string.json:4: Alternate 'Alt' member 'two' can't be distinguished from member 'one'
+tests/qapi-schema/alternate-conflict-string.json:2: Alternate 'Alt' member 'two' can't be distinguished from member 'one'
diff --git a/tests/qapi-schema/alternate-conflict-string.json b/tests/qapi-schema/alternate-conflict-string.json
index 72f04a8..85adbd4 100644
--- a/tests/qapi-schema/alternate-conflict-string.json
+++ b/tests/qapi-schema/alternate-conflict-string.json
@@ -1,6 +1,4 @@
-# we reject alternates with multiple string-like branches
-{ 'enum': 'Enum',
- 'data': [ 'hello', 'world' ] }
+# alternate branches of 'str' type conflict with all scalar types
{ 'alternate': 'Alt',
'data': { 'one': 'str',
- 'two': 'Enum' } }
+ 'two': 'int' } }
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 303f2b2..17649c6 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -101,12 +101,14 @@
# for testing use of 'number' within alternates
{ 'alternate': 'AltEnumBool', 'data': { 'e': 'EnumOne', 'b': 'bool' } }
{ 'alternate': 'AltEnumNum', 'data': { 'e': 'EnumOne', 'n': 'number' } }
-{ 'alternate': 'AltNumStr', 'data': { 'n': 'number', 's': 'str' } }
{ 'alternate': 'AltNumEnum', 'data': { 'n': 'number', 'e': 'EnumOne' } }
{ 'alternate': 'AltEnumInt', 'data': { 'e': 'EnumOne', 'i': 'int' } }
{ 'alternate': 'AltIntNum', 'data': { 'i': 'int', 'n': 'number' } }
{ 'alternate': 'AltNumInt', 'data': { 'n': 'number', 'i': 'int' } }
+# for testing use of 'str' within alternates
+{ 'alternate': 'AltStrObj', 'data': { 's': 'str', 'o': 'TestStruct' } }
+
# for testing native lists
{ 'union': 'UserDefNativeListUnion',
'data': { 'integer': ['int'],
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 3081091..9f68610 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -22,10 +22,10 @@ alternate AltNumInt
tag type
case n: number
case i: int
-alternate AltNumStr
+alternate AltStrObj
tag type
- case n: number
case s: str
+ case o: TestStruct
event EVENT_A None
boxed=False
event EVENT_B None
diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index c556b1b..c3be005 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -614,22 +614,26 @@ static void test_keyval_visit_alternate(void)
Error *err = NULL;
Visitor *v;
QDict *qdict;
- AltNumStr *ans;
+ AltStrObj *aso;
AltNumInt *ani;
+ AltEnumBool *aeb;
/*
* Can't do scalar alternate variants other than string. You get
* the string variant if there is one, else an error.
+ * TODO make it work for unambiguous cases like AltEnumBool below
*/
- qdict = keyval_parse("a=1,b=2", NULL, &error_abort);
+ qdict = keyval_parse("a=1,b=2,c=on", NULL, &error_abort);
v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
QDECREF(qdict);
visit_start_struct(v, NULL, NULL, 0, &error_abort);
- visit_type_AltNumStr(v, "a", &ans, &error_abort);
- g_assert_cmpint(ans->type, ==, QTYPE_QSTRING);
- g_assert_cmpstr(ans->u.s, ==, "1");
- qapi_free_AltNumStr(ans);
- visit_type_AltNumInt(v, "a", &ani, &err);
+ visit_type_AltStrObj(v, "a", &aso, &error_abort);
+ g_assert_cmpint(aso->type, ==, QTYPE_QSTRING);
+ g_assert_cmpstr(aso->u.s, ==, "1");
+ qapi_free_AltStrObj(aso);
+ visit_type_AltNumInt(v, "b", &ani, &err);
+ error_free_or_abort(&err);
+ visit_type_AltEnumBool(v, "c", &aeb, &err);
error_free_or_abort(&err);
visit_end_struct(v, NULL);
visit_free(v);
diff --git a/util/keyval.c b/util/keyval.c
index 93d5db6..7dbda62 100644
--- a/util/keyval.c
+++ b/util/keyval.c
@@ -65,11 +65,11 @@
* denote numbers, true, false or null. The special QObject input
* visitor returned by qobject_input_visitor_new_keyval() mostly hides
* this by automatically converting strings to the type the visitor
- * expects. Breaks down for alternate types and type 'any', where the
- * visitor's expectation isn't clear. Code visiting such types needs
- * to do the conversion itself, but only when using this keyval
- * visitor. Awkward. Alternate types without a string member don't
- * work at all.
+ * expects. Breaks down for type 'any', where the visitor's
+ * expectation isn't clear. Code visiting 'any' needs to do the
+ * conversion itself, but only when using this keyval visitor.
+ * Awkward. Note that we carefully restrict alternate types to avoid
+ * similar ambiguity.
*
* Additional syntax for use with an implied key:
*
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread