From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33045) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFOjh-0004FM-In for qemu-devel@nongnu.org; Mon, 17 Feb 2014 08:59:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WFOjc-0006fp-HR for qemu-devel@nongnu.org; Mon, 17 Feb 2014 08:59:53 -0500 Received: from mx1.redhat.com ([209.132.183.28]:4587) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFOjc-0006ep-9p for qemu-devel@nongnu.org; Mon, 17 Feb 2014 08:59:48 -0500 Date: Mon, 17 Feb 2014 08:59:42 -0500 From: Luiz Capitulino Message-ID: <20140217085942.44ff489d@redhat.com> In-Reply-To: <53016AD2.6050709@linux.vnet.ibm.com> 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> <53016AD2.6050709@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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: Wenchao Xia Cc: kwolf@redhat.com, qemu-devel@nongnu.org, Markus Armbruster , mdroth@linux.vnet.ibm.com On Mon, 17 Feb 2014 09:50:10 +0800 Wenchao Xia wrote: > =E4=BA=8E 2014/2/14 17:23, Markus Armbruster =E5=86=99=E9=81=93: > > Wenchao Xia writes: > > > >> =E4=BA=8E 2014/2/13 23:14, Markus Armbruster =E5=86=99=E9=81=93: > >>> 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 =3D 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 f= ound 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 =3D QAPISchema(fp) > >> except QAPISchemaError, e: > >> print >>sys.stderr, e > >> exit(1) > >> > >> exprs =3D [] > >> > >> 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? Yes, but I have to handle current pull request right now. I'll let you know when I apply it. >=20 >=20 > >>>> + 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 =3D 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=3Dguardname(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 =3D find_struct(base) > >>>> + if not base_struct_define: > >>>> + return None > >>>> + return base_struct_define.get('data') > >>>> + > >>>> +# Return the discriminator enum define, if discriminator is specifi= ed in > >>>> +# @expr and it is a pre-defined enum type > >>>> +def discriminator_find_enum_define(expr): > >>>> + discriminator =3D expr.get('discriminator') > >>>> + base =3D expr.get('base') > >>>> + > >>>> + # Only support discriminator when base present > >>>> + if not (discriminator and base): > >>>> + return None > >>>> + > >>>> + base_fields =3D 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 =3D 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. > >>> > > >=20