qapi: Reject alternates that can't work with keyval_parse()
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: a 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 distinct
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 '-', '+', or a decimal digit. 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>
Message-Id: <1495471335-23707-5-git-send-email-armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
This commit is contained in:
parent
8168ca8ea3
commit
c0644771eb
|
@ -812,11 +812,26 @@ def check_alternate(expr, info):
|
||||||
if not qtype:
|
if not qtype:
|
||||||
raise QAPISemError(info, "Alternate '%s' member '%s' cannot use "
|
raise QAPISemError(info, "Alternate '%s' member '%s' cannot use "
|
||||||
"type '%s'" % (name, key, value))
|
"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 "
|
raise QAPISemError(info, "Alternate '%s' member '%s' can't "
|
||||||
"be distinguished from member '%s'"
|
"be distinguished from member '%s'"
|
||||||
% (name, key, types_seen[qtype]))
|
% (name, key, types_seen[qtype]))
|
||||||
types_seen[qtype] = key
|
for qt in conflicting:
|
||||||
|
types_seen[qt] = key
|
||||||
|
|
||||||
|
|
||||||
def check_enum(expr, info):
|
def check_enum(expr, info):
|
||||||
|
|
|
@ -342,6 +342,8 @@ qapi-schema += alternate-array.json
|
||||||
qapi-schema += alternate-base.json
|
qapi-schema += alternate-base.json
|
||||||
qapi-schema += alternate-clash.json
|
qapi-schema += alternate-clash.json
|
||||||
qapi-schema += alternate-conflict-dict.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-conflict-string.json
|
||||||
qapi-schema += alternate-empty.json
|
qapi-schema += alternate-empty.json
|
||||||
qapi-schema += alternate-nested.json
|
qapi-schema += alternate-nested.json
|
||||||
|
|
|
@ -1,4 +1,4 @@
|
||||||
# we reject alternates with multiple object branches
|
# alternate branches of object type conflict with each other
|
||||||
{ 'struct': 'One',
|
{ 'struct': 'One',
|
||||||
'data': { 'name': 'str' } }
|
'data': { 'name': 'str' } }
|
||||||
{ 'struct': 'Two',
|
{ 'struct': 'Two',
|
||||||
|
|
1
tests/qapi-schema/alternate-conflict-enum-bool.err
Normal file
1
tests/qapi-schema/alternate-conflict-enum-bool.err
Normal file
|
@ -0,0 +1 @@
|
||||||
|
tests/qapi-schema/alternate-conflict-enum-bool.json:4: Alternate 'Alt' member 'two' can't be distinguished from member 'one'
|
1
tests/qapi-schema/alternate-conflict-enum-bool.exit
Normal file
1
tests/qapi-schema/alternate-conflict-enum-bool.exit
Normal file
|
@ -0,0 +1 @@
|
||||||
|
1
|
6
tests/qapi-schema/alternate-conflict-enum-bool.json
Normal file
6
tests/qapi-schema/alternate-conflict-enum-bool.json
Normal file
|
@ -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' } }
|
0
tests/qapi-schema/alternate-conflict-enum-bool.out
Normal file
0
tests/qapi-schema/alternate-conflict-enum-bool.out
Normal file
1
tests/qapi-schema/alternate-conflict-enum-int.err
Normal file
1
tests/qapi-schema/alternate-conflict-enum-int.err
Normal file
|
@ -0,0 +1 @@
|
||||||
|
tests/qapi-schema/alternate-conflict-enum-int.json:4: Alternate 'Alt' member 'two' can't be distinguished from member 'one'
|
1
tests/qapi-schema/alternate-conflict-enum-int.exit
Normal file
1
tests/qapi-schema/alternate-conflict-enum-int.exit
Normal file
|
@ -0,0 +1 @@
|
||||||
|
1
|
6
tests/qapi-schema/alternate-conflict-enum-int.json
Normal file
6
tests/qapi-schema/alternate-conflict-enum-int.json
Normal file
|
@ -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' } }
|
0
tests/qapi-schema/alternate-conflict-enum-int.out
Normal file
0
tests/qapi-schema/alternate-conflict-enum-int.out
Normal file
|
@ -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'
|
||||||
|
|
|
@ -1,6 +1,4 @@
|
||||||
# we reject alternates with multiple string-like branches
|
# alternate branches of 'str' type conflict with all scalar types
|
||||||
{ 'enum': 'Enum',
|
|
||||||
'data': [ 'hello', 'world' ] }
|
|
||||||
{ 'alternate': 'Alt',
|
{ 'alternate': 'Alt',
|
||||||
'data': { 'one': 'str',
|
'data': { 'one': 'str',
|
||||||
'two': 'Enum' } }
|
'two': 'int' } }
|
||||||
|
|
|
@ -101,12 +101,14 @@
|
||||||
# for testing use of 'number' within alternates
|
# for testing use of 'number' within alternates
|
||||||
{ 'alternate': 'AltEnumBool', 'data': { 'e': 'EnumOne', 'b': 'bool' } }
|
{ 'alternate': 'AltEnumBool', 'data': { 'e': 'EnumOne', 'b': 'bool' } }
|
||||||
{ 'alternate': 'AltEnumNum', 'data': { 'e': 'EnumOne', 'n': 'number' } }
|
{ 'alternate': 'AltEnumNum', 'data': { 'e': 'EnumOne', 'n': 'number' } }
|
||||||
{ 'alternate': 'AltNumStr', 'data': { 'n': 'number', 's': 'str' } }
|
|
||||||
{ 'alternate': 'AltNumEnum', 'data': { 'n': 'number', 'e': 'EnumOne' } }
|
{ 'alternate': 'AltNumEnum', 'data': { 'n': 'number', 'e': 'EnumOne' } }
|
||||||
{ 'alternate': 'AltEnumInt', 'data': { 'e': 'EnumOne', 'i': 'int' } }
|
{ 'alternate': 'AltEnumInt', 'data': { 'e': 'EnumOne', 'i': 'int' } }
|
||||||
{ 'alternate': 'AltIntNum', 'data': { 'i': 'int', 'n': 'number' } }
|
{ 'alternate': 'AltIntNum', 'data': { 'i': 'int', 'n': 'number' } }
|
||||||
{ 'alternate': 'AltNumInt', 'data': { 'n': 'number', 'i': 'int' } }
|
{ '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
|
# for testing native lists
|
||||||
{ 'union': 'UserDefNativeListUnion',
|
{ 'union': 'UserDefNativeListUnion',
|
||||||
'data': { 'integer': ['int'],
|
'data': { 'integer': ['int'],
|
||||||
|
|
|
@ -22,10 +22,10 @@ alternate AltNumInt
|
||||||
tag type
|
tag type
|
||||||
case n: number
|
case n: number
|
||||||
case i: int
|
case i: int
|
||||||
alternate AltNumStr
|
alternate AltStrObj
|
||||||
tag type
|
tag type
|
||||||
case n: number
|
|
||||||
case s: str
|
case s: str
|
||||||
|
case o: TestStruct
|
||||||
event EVENT_A None
|
event EVENT_A None
|
||||||
boxed=False
|
boxed=False
|
||||||
event EVENT_B None
|
event EVENT_B None
|
||||||
|
|
|
@ -614,22 +614,26 @@ static void test_keyval_visit_alternate(void)
|
||||||
Error *err = NULL;
|
Error *err = NULL;
|
||||||
Visitor *v;
|
Visitor *v;
|
||||||
QDict *qdict;
|
QDict *qdict;
|
||||||
AltNumStr *ans;
|
AltStrObj *aso;
|
||||||
AltNumInt *ani;
|
AltNumInt *ani;
|
||||||
|
AltEnumBool *aeb;
|
||||||
|
|
||||||
/*
|
/*
|
||||||
* Can't do scalar alternate variants other than string. You get
|
* Can't do scalar alternate variants other than string. You get
|
||||||
* the string variant if there is one, else an error.
|
* 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));
|
v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
|
||||||
QDECREF(qdict);
|
QDECREF(qdict);
|
||||||
visit_start_struct(v, NULL, NULL, 0, &error_abort);
|
visit_start_struct(v, NULL, NULL, 0, &error_abort);
|
||||||
visit_type_AltNumStr(v, "a", &ans, &error_abort);
|
visit_type_AltStrObj(v, "a", &aso, &error_abort);
|
||||||
g_assert_cmpint(ans->type, ==, QTYPE_QSTRING);
|
g_assert_cmpint(aso->type, ==, QTYPE_QSTRING);
|
||||||
g_assert_cmpstr(ans->u.s, ==, "1");
|
g_assert_cmpstr(aso->u.s, ==, "1");
|
||||||
qapi_free_AltNumStr(ans);
|
qapi_free_AltStrObj(aso);
|
||||||
visit_type_AltNumInt(v, "a", &ani, &err);
|
visit_type_AltNumInt(v, "b", &ani, &err);
|
||||||
|
error_free_or_abort(&err);
|
||||||
|
visit_type_AltEnumBool(v, "c", &aeb, &err);
|
||||||
error_free_or_abort(&err);
|
error_free_or_abort(&err);
|
||||||
visit_end_struct(v, NULL);
|
visit_end_struct(v, NULL);
|
||||||
visit_free(v);
|
visit_free(v);
|
||||||
|
|
|
@ -65,11 +65,11 @@
|
||||||
* denote numbers, true, false or null. The special QObject input
|
* denote numbers, true, false or null. The special QObject input
|
||||||
* visitor returned by qobject_input_visitor_new_keyval() mostly hides
|
* visitor returned by qobject_input_visitor_new_keyval() mostly hides
|
||||||
* this by automatically converting strings to the type the visitor
|
* this by automatically converting strings to the type the visitor
|
||||||
* expects. Breaks down for alternate types and type 'any', where the
|
* expects. Breaks down for type 'any', where the visitor's
|
||||||
* visitor's expectation isn't clear. Code visiting such types needs
|
* expectation isn't clear. Code visiting 'any' needs to do the
|
||||||
* to do the conversion itself, but only when using this keyval
|
* conversion itself, but only when using this keyval visitor.
|
||||||
* visitor. Awkward. Alternate types without a string member don't
|
* Awkward. Note that we carefully restrict alternate types to avoid
|
||||||
* work at all.
|
* similar ambiguity.
|
||||||
*
|
*
|
||||||
* Additional syntax for use with an implied key:
|
* Additional syntax for use with an implied key:
|
||||||
*
|
*
|
||||||
|
|
Loading…
Reference in a new issue