[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaV
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions |
Date: |
Thu, 30 Jul 2015 06:46:38 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 |
On 07/30/2015 12:42 AM, Markus Armbruster wrote:
>> But what happens if the 0th branch is mapped to a different parser, as
>> would be the case if one of the alternate's branches is 'number'? In
>> particular, qmp_input_type_number() accepts BOTH QFloat and QInt types.
>> So, if we have this qapi:
>> { 'alternate': 'Foo', 'data': { 'a': 'str', 'b': 'number' } }
>> but pass in an integer, visit_get_next_type() will see a qtype of QInt,
>> but Foo_qtypes[QTYPE_QINT] will be 0 (due to default initialization) and
>> we will wrongly try to visit the 0th branch (FOO_KIND_A) and fail (the
>> string parser doesn't like ints) even though the parse should succeed by
>> using the FOO_KIND_B branch.
>
> Yup, bug.
And it's an order-dependent bug - merely declaring 'b' first makes it
appear to work correctly.
>
>> Interestingly, this means that if we ever write an alternate type that
>> accepts both 'int' and 'number' (we have not attempted that so far),
>> then the number branch will only be taken for inputs that don't also
>> look like ints (normally, 'number' accepts anything numeric). Maybe that
>> means we should document and enforce that 'number' and 'int' cannot be
>> mixed in the same alternate?
>
> Even if we outlaw mixing the two, I'm afraid we still have this bug: an
> alternate with a 'number' member rejects input that gets parsed as
> QTYPE_QINT.
>
> Let's simply make alternates behave sanely:
>
> alternate has case selected for
> 'int' 'number' QTYPE_QINT QTYPE_QFLOAT
> no no error error
> no yes 'number' 'number'
> yes no 'int' error
> yes yes 'int' 'number'
Works for me.
>
> + 1 works, because the element type is int, not BlockdevRefKind. It's
> int so it can serve as argument for visit_get_next_type()'s parameter
> const int *qtypes.
>
> The + 1, - 1 business could be mildly confusing. We could set all
> unused elements to -1 instead:
Or, we could ditch the qtypes lookup altogether, and merely create the
alternate enum as a non-consecutive QTYPE mapping, for one less level of
indirection, as in:
typedef enum BlockdevRefKind {
BLOCKDEV_REF_DEFINITION = QTYPE_QOBJECT,
BLOCKDEV_REF_REFERENCE = QTYPE_QSTRING,
};
then rewrite visit_get_next_type() to directly return the qtype, as well
as rewrite the generated switch statement in visit_type_BlockdevRef() to
directly inspect the qtypes it cares about. In fact, that's the
approach I'm currently playing with.
> Add test eight test cases from my table above, then fix the generator to
> make them pass.
I hope to post an RFC followup patch along those lines later today.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions, (continued)
- Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions, Eric Blake, 2015/07/22
- Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions, Eric Blake, 2015/07/22
- Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions, Eric Blake, 2015/07/22
- Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions, Markus Armbruster, 2015/07/27
- Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions, Eric Blake, 2015/07/27
- Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions, Markus Armbruster, 2015/07/28
Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions, Eric Blake, 2015/07/29
- Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions, Markus Armbruster, 2015/07/30
- Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions,
Eric Blake <=
- Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions, Markus Armbruster, 2015/07/30
- Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions, Eric Blake, 2015/07/30
- Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions, Eric Blake, 2015/07/30
[Qemu-devel] [PATCH RFC v2 32/47] qapi-event: Convert to QAPISchemaVisitor, fixing data with base, Markus Armbruster, 2015/07/01
[Qemu-devel] [PATCH RFC v2 37/47] qapi: De-duplicate parameter list generation, Markus Armbruster, 2015/07/01