Re: [Qemu-devel] [PATCH 4/4] qapi: Reject alternates that can't work with keyval_parse()

[Markus Armbruster]

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!