From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39699) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFDM9-0006ue-9P for qemu-devel@nongnu.org; Sun, 16 Feb 2014 20:50:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WFDM1-0005vv-3V for qemu-devel@nongnu.org; Sun, 16 Feb 2014 20:50:49 -0500 Received: from e28smtp09.in.ibm.com ([122.248.162.9]:41176) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFDM0-0005t7-F7 for qemu-devel@nongnu.org; Sun, 16 Feb 2014 20:50:41 -0500 Received: from /spool/local by e28smtp09.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 17 Feb 2014 07:20:14 +0530 Received: from d28relay04.in.ibm.com (d28relay04.in.ibm.com [9.184.220.61]) by d28dlp03.in.ibm.com (Postfix) with ESMTP id F19691258056 for ; Mon, 17 Feb 2014 07:22:10 +0530 (IST) Received: from d28av03.in.ibm.com (d28av03.in.ibm.com [9.184.220.65]) by d28relay04.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s1H1oEG72359778 for ; Mon, 17 Feb 2014 07:20:14 +0530 Received: from d28av03.in.ibm.com (localhost [127.0.0.1]) by d28av03.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s1H1oCxg005536 for ; Mon, 17 Feb 2014 07:20:12 +0530 Message-ID: <53016AD2.6050709@linux.vnet.ibm.com> Date: Mon, 17 Feb 2014 09:50:10 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1392068921-3327-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1392068921-3327-4-git-send-email-xiawenc@linux.vnet.ibm.com> <8738jnvydi.fsf@blackfin.pond.sub.org> <52FD82CF.8020500@linux.vnet.ibm.com> <87mwhum4jr.fsf@blackfin.pond.sub.org> In-Reply-To: <87mwhum4jr.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V6 03/10] qapi script: check correctness of discriminator values in union List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, lcapitulino@redhat.com, mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org 于 2014/2/14 17:23, Markus Armbruster 写道: > Wenchao Xia writes: > >> 于 2014/2/13 23:14, Markus Armbruster 写道: >>> Wenchao Xia writes: >>> >>>> It will check whether the values specified are written correctly, >>>> and whether all enum values are covered, when discriminator is a >>>> pre-defined enum type >>>> >>>> Signed-off-by: Wenchao Xia >>>> Reviewed-by: Eric Blake >>>> --- >>>> scripts/qapi-visit.py | 17 +++++++++++++++++ >>>> scripts/qapi.py | 31 +++++++++++++++++++++++++++++++ >>>> 2 files changed, 48 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py >>>> index 65f1a54..c0efb5f 100644 >>>> --- a/scripts/qapi-visit.py >>>> +++ b/scripts/qapi-visit.py >>>> @@ -255,6 +255,23 @@ def generate_visit_union(expr): >>>> assert not base >>>> return generate_visit_anon_union(name, members) >>>> >>>> + # If discriminator is specified and it is a pre-defined enum in schema, >>>> + # check its correctness >>>> + enum_define = discriminator_find_enum_define(expr) >>>> + if enum_define: >>>> + for key in members: >>>> + if not key in enum_define["enum_values"]: >>>> + sys.stderr.write("Discriminator value '%s' is not found in " >>>> + "enum '%s'\n" % >>>> + (key, enum_define["enum_name"])) >>>> + sys.exit(1) >>> >>> Can this happen? If yes, why isn't it diagnosed in qapi.py, like all >>> the other semantic errors? >>> >> I think the parse procedure contains two part: >> 1 read qapi-schema.json and parse it into exprs. >> 2 translate exprs into final output. >> Looking at qapi.py, qapi-visit.py, qapi-types.py, it seems qapi.py is >> in charge of step 1 handling literal error, and other two script are in >> charge of step 2. The above error can be only detected in step 2 after >> all enum defines are remembered in step 1, so I didn't add those things >> into qapi.py. > > The distribution of work between the qapi*py isn't spelled out anywhere, > but my working hypothesis is qapi.py is the frontend, and the > qapi-{commands,types,visit}.py are backends. > > The frontend's job is lexical, syntax and semantic analysis. > > The backends' job is source code generation. > > This isn't the only possible split, but it's the orthodox way to split > compilers. > >> I guess you want to place the check inside parse_schema() to let >> test case detect it easier, one way to go is, let qapi.py do checks >> for step 2: >> >> def parse_schema(fp): >> try: >> schema = QAPISchema(fp) >> except QAPISchemaError, e: >> print >>sys.stderr, e >> exit(1) >> >> exprs = [] >> >> for expr in schema.exprs: >> if expr.has_key('enum'): >> add_enum(expr['enum']) >> elif expr.has_key('union'): >> add_union(expr) >> add_enum('%sKind' % expr['union']) >> elif expr.has_key('type'): >> add_struct(expr) >> exprs.append(expr) >> >> + for expr in schema.exprs: >> + if expr.has_key('union'): >> + #check code >> >> return exprs >> >> This way qapi.py can detect such errors. Disadvantage is that, >> qapi.py is invloved for step 2 things, so some code in qapi.py >> and qapi-visit.py may be dupicated, here the "if .... union... >> discriminator" code may appear in both qapi.py and qapi-visit.py. > > How much code would be duplicated? > Not many now, my concern is it may becomes more complex when more check introduced in future. However, your distribution of qapi*.py as complier make sense, so I am OK to respin this series. Luiz, could you apply or push Markus's series, so I can pull it as my working base? >>>> + for key in enum_define["enum_values"]: >>>> + if not key in members: >>>> + sys.stderr.write("Enum value '%s' is not covered by a branch " >>>> + "of union '%s'\n" % >>>> + (key, name)) >>>> + sys.exit(1) >>>> + >>> >>> Likewise. >>> >>>> ret = generate_visit_enum('%sKind' % name, members.keys()) >>>> >>>> if base: >>>> diff --git a/scripts/qapi.py b/scripts/qapi.py >>>> index cf34768..0a3ab80 100644 >>>> --- a/scripts/qapi.py >>>> +++ b/scripts/qapi.py >>>> @@ -385,3 +385,34 @@ def guardend(name): >>>> >>>> ''', >>>> name=guardname(name)) >>>> + >> >> The funtions below are likely helper funtions, I planed to put them >> into qapi_helper.py, but they are not much so kepted for easy. > > That's fine with me. > >>>> +# This function can be used to check whether "base" is valid >>>> +def find_base_fields(base): >>>> + base_struct_define = find_struct(base) >>>> + if not base_struct_define: >>>> + return None >>>> + return base_struct_define.get('data') >>>> + >>>> +# Return the discriminator enum define, if discriminator is specified in >>>> +# @expr and it is a pre-defined enum type >>>> +def discriminator_find_enum_define(expr): >>>> + discriminator = expr.get('discriminator') >>>> + base = expr.get('base') >>>> + >>>> + # Only support discriminator when base present >>>> + if not (discriminator and base): >>>> + return None >>>> + >>>> + base_fields = find_base_fields(base) >>>> + >>>> + if not base_fields: >>>> + raise StandardError("Base '%s' is not a valid type\n" >>>> + % base) >>> >>> Why not QAPISchemaError, like for other semantic errors? >>> >> >> I think QAPISchemaError is a literal error of step 1, here >> it can't be used unless we record the text/line number belong to >> each expr. > > Reporting an error without a location is not nice! > > If decent error messages require recording locations, then we should > record locations. > > A real compiler frontend records full location information, i.e. every > node in the abstract syntax tree (or whatever else it produces) is > decorated with a location. > > Unfortunately, this wasn't done in qapi.py, so we get to retrofit it > now. > > Perhaps recording only locations of top-level expressions would suffice > to improve your error messages to acceptable levels. I'm not saying we > should take this shortcut, just pointing out it exists. > > qapi.py represents locations as character offset in the contents of the > schema file (QAPISchema.cursor), which it converts to line, column on > demand, in QAPISchemaError.__init__. If we keep things working that > way, the location data to record is the offset, not line, column. > >>>> + >>>> + discriminator_type = base_fields.get(discriminator) >>>> + >>>> + if not discriminator_type: >>>> + raise StandardError("Discriminator '%s' not found in schema\n" >>>> + % discriminator) >>> >>> Likewise. >>> >>>> + >>>> + return find_enum(discriminator_type) >>> >>> All errors should have a test in tests/qapi-schema/. I can try to add >>> tests for you when I rebase your 09/10. >>> >