From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48774) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKhYF-0007tH-9w for qemu-devel@nongnu.org; Thu, 30 Jul 2015 02:42:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKhYA-000299-4q for qemu-devel@nongnu.org; Thu, 30 Jul 2015 02:42:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33854) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKhY9-00028o-Qn for qemu-devel@nongnu.org; Thu, 30 Jul 2015 02:42:42 -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> Date: Thu, 30 Jul 2015 08:42:38 +0200 In-Reply-To: <55B95DAA.3040302@redhat.com> (Eric Blake's message of "Wed, 29 Jul 2015 17:11:38 -0600") Message-ID: <87oaiu5eox.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/01/2015 02:22 PM, Markus Armbruster wrote: >> Fixes flat unions to get the base's base members. Test case is from >> commit 2fc0043, in qapi-schema-test.json: >> > >> -def generate_alternate_qtypes(expr): >> +def gen_alternate_qtypes_decl(name): >> + return mcgen(''' >> >> - name = expr['alternate'] >> - members = expr['data'] >> +extern const int %(c_name)s_qtypes[]; >> +''', >> + c_name=c_name(name)) >> >> +def gen_alternate_qtypes(name, variants): >> ret = mcgen(''' >> >> const int %(name)s_qtypes[QTYPE_MAX] = { >> ''', >> name=c_name(name)) >> >> - for key in members: >> - qtype = find_alternate_member_qtype(members[key]) >> - assert qtype, "Invalid alternate member" >> + for var in variants.variants: >> + qtype = var.type.alternate_qtype() >> + assert qtype > > I think I found a couple more corner case bugs here. We are using C99 > initialization of the array; for example: > > const int BlockdevRef_qtypes[QTYPE_MAX] = { > [QTYPE_QDICT] = BLOCKDEV_REF_KIND_DEFINITION, > [QTYPE_QSTRING] = BLOCKDEV_REF_KIND_REFERENCE, > }; > > but paired with an enum that starts at 0: > > typedef enum BlockdevRefKind { > BLOCKDEV_REF_KIND_DEFINITION = 0, > BLOCKDEV_REF_KIND_REFERENCE = 1, > BLOCKDEV_REF_KIND_MAX = 2, > } BlockdevRefKind; > > > and that means that every QTYPE_ constant that we don't specify in > _qtypes[] is also assigned the value 0 (aka BLOCKDEV_REF_KIND_DEFINITION I see where this is going... > in this example). In operation, calling something like: > > {"execute":"blockdev-add","arguments":{"options": > {"driver":"raw","id":"a","file":true}}} > > which is invalid per the .json description ("file" must be string or > object, not boolean), still manages to get past visit_get_next_type() > with success, and fall through to the 0th branch of the switch. If that > 0th branch happens to be a struct (as it is for BlockdevRef), then we > fortunately catch the error on the very next parse call, where > qmp_input_start_struct() complains: > > {"error": {"class": "GenericError", "desc": "Invalid parameter type for > 'file', expected: QDict"}} A lucky case. > 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. > 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' > So, the bugs are: visit_get_next_type() can't tell the difference > between a *_qtypes[] lookup that was explicitly initialized to 0 from > one that was accidentally left that way, and therefore can't report > failure for an unexpected type (but mostly mitigated by the fact that > always returning 0 means the parser will attempt to parse the first > branch of the alternate and gracefully fail); Yes. > and that we don't properly > handle QInt for an alternate that accepts 'number' but not 'int'. Yes. > I don't think either bug has to be fixed in your series, although you > may want to add tests. Agree. > The first bug could be resolved by guaranteeing that the _qtypes[] array > has non-zero values for the explicitly initialized lookups, and teaching > visit_get_next_type() that a lookup that produces 0 meant that an > unexpected type was encountered. Perhaps by changing the creation of > _qtypes[] in qapi-types.c to list: > > const int BlockdevRef_qtypes[QTYPE_MAX] = { > [QTYPE_QDICT] = BLOCKDEV_REF_KIND_DEFINITION + 1, > [QTYPE_QSTRING] = BLOCKDEV_REF_KIND_REFERENCE + 1, > }; > > and then having visit_get_next_type() subtract one after verifying a > non-zero value was looked up. + 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: const int BlockdevRef_qtypes[QTYPE_MAX] = { [QTYPE_NONE] = -1, [QTYPE_QNULL] = -1, [QTYPE_QINT] = -1, [QTYPE_QSTRING] = BLOCKDEV_REF_KIND_REFERENCE + 1, [QTYPE_QDICT] = BLOCKDEV_REF_KIND_DEFINITION + 1, [QTYPE_QLIST] = -1, [QTYPE_QFLOAT] = -1, [QTYPE_QBOOL] = -1, }; I wouldn't recommend that in hand-written code, but generating it should be fine. > Or perhaps leave _qtypes alone, and > instead change the alternate enum to have a placeholder at 0: > > typedef enum BlockdevRefKind { > BLOCKDEV_REF_KIND_INVALID = 0, > BLOCKDEV_REF_KIND_DEFINITION = 1, > BLOCKDEV_REF_KIND_REFERENCE = 2, > BLOCKDEV_REF_KIND_MAX = 3, > } BlockdevRefKind; > > and then teaching the generator for visit_type_BlockdevRef() to emit an > error if branch 0 is hit. BLOCKDEV_REF_KIND_INVALID could get in the way elsewhere, e.g. with -Wswitch. > Fixing the second bug probably entails teaching the generator that if an > alternate contains 'number' but not 'int', then we need [QTYPE_QINT] to > map to the same lookup value as [QTYPE_QNUMBER]. Add test eight test cases from my table above, then fix the generator to make them pass.