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

[Eric Blake]

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/

> 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/

> 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? Should it also be a problem
for an enum member with value 'true' or 'false'?

> 
> * 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'/

>   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?

> +                        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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature