From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38067) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKq9I-0005Yh-IW for qemu-devel@nongnu.org; Thu, 30 Jul 2015 11:53:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKq9E-0006A0-Ad for qemu-devel@nongnu.org; Thu, 30 Jul 2015 11:53:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35853) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKq9E-00069v-3v for qemu-devel@nongnu.org; Thu, 30 Jul 2015 11:53:32 -0400 From: Markus Armbruster References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-27-git-send-email-armbru@redhat.com> <55B95DAA.3040302@redhat.com> <87oaiu5eox.fsf@blackfin.pond.sub.org> <55BA1CAE.8030405@redhat.com> Date: Thu, 30 Jul 2015 17:53:28 +0200 In-Reply-To: <55BA1CAE.8030405@redhat.com> (Eric Blake's message of "Thu, 30 Jul 2015 06:46:38 -0600") Message-ID: <87si85y747.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, berto@igalia.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com Eric Blake writes: > 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, QTYPE_QDICT, but I get what you mean. > 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. Hmm. The schema currently doesn't let you control enumeration values. qapi-code-gen.txt specifies: The enumeration values [...] are encoded as C enum integral values in generated code. While the C code starts numbering at 0, it is better to use explicit comparisons to enum values than implicit comparisons to 0; the C code will also include a generated enum member ending in _MAX for tracking the size of the enum, useful when using common functions for converting between strings and enum values. Strictly speaking, this needn't apply to implicit enums like BlockdevRefKind. But is it a good idea to make them different? Hmm, your new BlockdevRefKind is basically a subset of qtype_code with the members renamed. Could we simply use qtype_code directly? >> 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.