[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/4] qapi: Reject alternates that can't work wit
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 4/4] qapi: Reject alternates that can't work with keyval_parse() |
Date: |
Tue, 23 May 2017 10:45:02 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 05/22/2017 11:42 AM, Markus Armbruster wrote:
>> 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
>
> s/an non/a non/
Will fix.
>> 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
>
> s/district/distinct/
Will fix.
>> 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'.
>
> Does case-(in)sensitivity factor in here?
KEY=VALUE,... syntax is case-sensitive.
> Should it also be a problem
> for an enum member with value 'true' or 'false'?
No, because those are spelled "on" and "off" in KEY=VALUE,... syntax.
If we add synonyms there, we'll have to tighten the restrictions here.
>> * An enumeration member clashes with numeric members when it has a
>> value that starts with '-', '+', '0' or '9'. This is a rather lazy
>
> s/'0' or '9'/or among '0' to '9'/
Will fix.
>> 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 <address@hidden>
>> ---
>
>> +++ 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']:
>
> what about 'true', 'false'?
>
> Do we care about case insensitive?
See above.
>> + 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
>>
>
> Looks good.
Thanks!
- [Qemu-devel] [PATCH 0/4] qapi: Handle some keyval fallout, Markus Armbruster, 2017/05/22
- [Qemu-devel] [PATCH 2/4] qapi: Document visit_type_any() issues with keyval input, Markus Armbruster, 2017/05/22
- [Qemu-devel] [PATCH 1/4] qobject-input-visitor: Reject non-finite numbers with keyval, Markus Armbruster, 2017/05/22
- [Qemu-devel] [PATCH 3/4] tests/qapi-schema: Avoid 'str' in alternate test cases, Markus Armbruster, 2017/05/22
- [Qemu-devel] [PATCH 4/4] qapi: Reject alternates that can't work with keyval_parse(), Markus Armbruster, 2017/05/22
- Re: [Qemu-devel] [PATCH 0/4] qapi: Handle some keyval fallout, Marc-André Lureau, 2017/05/26