From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47229) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WiUYP-000262-Sa for qemu-devel@nongnu.org; Thu, 08 May 2014 16:04:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WiUYL-0001LN-7Z for qemu-devel@nongnu.org; Thu, 08 May 2014 16:04:29 -0400 Received: from lputeaux-656-01-25-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:58227 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WiUYK-0001LH-Ub for qemu-devel@nongnu.org; Thu, 08 May 2014 16:04:25 -0400 Date: Thu, 8 May 2014 22:04:55 +0200 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140508200455.GA28563@irqsave.net> References: <1399562319-26873-1-git-send-email-benoit.canet@irqsave.net> <87wqdwgm2e.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <87wqdwgm2e.fsf@blackfin.pond.sub.org> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] qapi: Make the include directive idempotent. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: =?iso-8859-1?Q?Beno=EEt?= Canet , qemu-devel@nongnu.org, vilanova@ac.upc.edu, lcapitulino@redhat.com The Thursday 08 May 2014 =E0 20:30:33 (+0200), Markus Armbruster wrote : > Humor me: no period at end of subject. >=20 > Beno=EEt Canet writes: >=20 > > The purpose of this change is to help create a json file containing > > common definitions. >=20 > Please describe the new semantics of the include directive here, so > mathematically challenged readers don't have to loop up "idempotent" in > the dictionary :) >=20 > > The history used to detect multiple inclusions is not a stack anymore > > but a regular list. >=20 > You need a stack to show the actual include cycle. >=20 > There are two reasons to detect cycles. The technical one is preventin= g > infinite expansion. No longer applies with idempotent include. The > other, more abstract one is rejecting nonsensical use of the include > directive. I think that one still applies. >=20 > > The cycle detection check where updated and leaved in place to make > > sure the code will never enter into an infinite recursion loop. >=20 > -ENOPARSE >=20 > Suggest to retry in active voice :) >=20 > > Signed-off-by: Benoit Canet > > --- > > scripts/qapi.py | 13 ++++++------- > > tests/Makefile | 3 ++- > > tests/qapi-schema/include-cycle.err | 3 --- > > tests/qapi-schema/include-cycle.exit | 2 +- > > tests/qapi-schema/include-cycle.out | 3 +++ > > tests/qapi-schema/include-idempotent.exit | 1 + > > tests/qapi-schema/include-idempotent.json | 3 +++ > > tests/qapi-schema/include-idempotent.out | 3 +++ > > tests/qapi-schema/include-self-cycle.err | 1 - > > tests/qapi-schema/include-self-cycle.exit | 2 +- > > tests/qapi-schema/include-self-cycle.out | 3 +++ > > tests/qapi-schema/sub-include-idempotent.json | 3 +++ > > 12 files changed, 26 insertions(+), 14 deletions(-) > > create mode 100644 tests/qapi-schema/include-idempotent.err > > create mode 100644 tests/qapi-schema/include-idempotent.exit > > create mode 100644 tests/qapi-schema/include-idempotent.json > > create mode 100644 tests/qapi-schema/include-idempotent.out > > create mode 100644 tests/qapi-schema/sub-include-idempotent.json >=20 > Is this based on Luiz's queue-qmp? >=20 > > diff --git a/scripts/qapi.py b/scripts/qapi.py > > index ec806aa..0ed44c8 100644 > > --- a/scripts/qapi.py > > +++ b/scripts/qapi.py > > @@ -79,7 +79,7 @@ class QAPISchema: > > input_relname =3D fp.name > > self.input_dir =3D os.path.dirname(input_fname) > > self.input_file =3D input_relname > > - self.include_hist =3D include_hist + [(input_relname, input_= fname)] > > + include_hist.append((input_relname, input_fname)) > > self.parent_info =3D parent_info > > self.src =3D fp.read() > > if self.src =3D=3D '' or self.src[-1] !=3D '\n': >=20 > Looks like you drop self.include_hist and simply keep it in local > variable include_hist. Have you considered doing that in a separate > cleanup patch prior to this one? >=20 > > @@ -102,17 +102,16 @@ class QAPISchema: > > 'Expected a file name (strin= g), got: %s' > > % include) > > include_path =3D os.path.join(self.input_dir, includ= e) > > - if any(include_path =3D=3D elem[1] > > - for elem in self.include_hist): > > - raise QAPIExprError(expr_info, "Inclusion loop f= or %s" > > - % include) > > + # make include idempotent by skipping further includ= es > > + if include_path in [x[1] for x in include_hist]: > > + continue >=20 > Loses cycle detection. It simply also skip cycle includes. If cycle are skipped then cannot do n= o harm. >=20 > Is permitting cycles necessary to solve your problem? Or asking more > directly: do you actually need cyclic includes? >=20 > > try: > > fobj =3D open(include_path, 'r') > > except IOError as e: > > raise QAPIExprError(expr_info, > > '%s: %s' % (e.strerror, incl= ude)) > > - exprs_include =3D QAPISchema(fobj, include, > > - self.include_hist, expr_i= nfo) > > + exprs_include =3D QAPISchema(fobj, include, include_= hist, > > + expr_info) > > self.exprs.extend(exprs_include.exprs) > > else: > > expr_elem =3D {'expr': expr, > > diff --git a/tests/Makefile b/tests/Makefile > > index a8d45be..c587b4a 100644 > > --- a/tests/Makefile > > +++ b/tests/Makefile > > @@ -182,7 +182,8 @@ check-qapi-schema-y :=3D $(addprefix tests/qapi-s= chema/, \ > > flat-union-string-discriminator.json \ > > include-simple.json include-relpath.json include-format-err.= json \ > > include-non-file.json include-no-file.json include-before-er= r.json \ > > - include-nested-err.json include-self-cycle.json include-cycl= e.json) > > + include-nested-err.json include-self-cycle.json include-cycl= e.json \ > > + include-idempotent.json) > > =20 > > GENERATED_HEADERS +=3D tests/test-qapi-types.h tests/test-qapi-visit= .h tests/test-qmp-commands.h > > =20 > > diff --git a/tests/qapi-schema/include-cycle.err b/tests/qapi-schema/= include-cycle.err > > index 602cf62..e69de29 100644 > > --- a/tests/qapi-schema/include-cycle.err > > +++ b/tests/qapi-schema/include-cycle.err > > @@ -1,3 +0,0 @@ > > -In file included from tests/qapi-schema/include-cycle.json:1: > > -In file included from include-cycle-b.json:1: > > -include-cycle-c.json:1: Inclusion loop for include-cycle.json > > diff --git a/tests/qapi-schema/include-cycle.exit b/tests/qapi-schema= /include-cycle.exit > > index d00491f..573541a 100644 > > --- a/tests/qapi-schema/include-cycle.exit > > +++ b/tests/qapi-schema/include-cycle.exit > > @@ -1 +1 @@ > > -1 > > +0 > > diff --git a/tests/qapi-schema/include-cycle.out b/tests/qapi-schema/= include-cycle.out > > index e69de29..b7f89a4 100644 > > --- a/tests/qapi-schema/include-cycle.out > > +++ b/tests/qapi-schema/include-cycle.out > > @@ -0,0 +1,3 @@ > > +[] > > +[] > > +[] >=20 > This test shows the loss of cycle detection. This test shows no real directive is present and cycles are skipped and the code does not go in an infinite loop.: >=20 > > diff --git a/tests/qapi-schema/include-idempotent.err b/tests/qapi-sc= hema/include-idempotent.err > > new file mode 100644 > > index 0000000..e69de29 > > diff --git a/tests/qapi-schema/include-idempotent.exit b/tests/qapi-s= chema/include-idempotent.exit > > new file mode 100644 > > index 0000000..573541a > > --- /dev/null > > +++ b/tests/qapi-schema/include-idempotent.exit > > @@ -0,0 +1 @@ > > +0 > > diff --git a/tests/qapi-schema/include-idempotent.json b/tests/qapi-s= chema/include-idempotent.json > > new file mode 100644 > > index 0000000..94113c6 > > --- /dev/null > > +++ b/tests/qapi-schema/include-idempotent.json > > @@ -0,0 +1,3 @@ > > +{ 'include': 'comments.json' } > > +{ 'include': 'sub-include-idempotent.json' } > > +{ 'include': 'comments.json' } > > diff --git a/tests/qapi-schema/include-idempotent.out b/tests/qapi-sc= hema/include-idempotent.out > > new file mode 100644 > > index 0000000..4ce3dcf > > --- /dev/null > > +++ b/tests/qapi-schema/include-idempotent.out > > @@ -0,0 +1,3 @@ > > +[OrderedDict([('enum', 'Status'), ('data', ['good', 'bad', 'ugly'])]= )] > > +[{'enum_name': 'Status', 'enum_values': ['good', 'bad', 'ugly']}] > > +[] > > diff --git a/tests/qapi-schema/include-self-cycle.err b/tests/qapi-sc= hema/include-self-cycle.err > > index 981742a..e69de29 100644 > > --- a/tests/qapi-schema/include-self-cycle.err > > +++ b/tests/qapi-schema/include-self-cycle.err > > @@ -1 +0,0 @@ > > -tests/qapi-schema/include-self-cycle.json:1: Inclusion loop for incl= ude-self-cycle.json > > diff --git a/tests/qapi-schema/include-self-cycle.exit b/tests/qapi-s= chema/include-self-cycle.exit > > index d00491f..573541a 100644 > > --- a/tests/qapi-schema/include-self-cycle.exit > > +++ b/tests/qapi-schema/include-self-cycle.exit > > @@ -1 +1 @@ > > -1 > > +0 > > diff --git a/tests/qapi-schema/include-self-cycle.out b/tests/qapi-sc= hema/include-self-cycle.out > > index e69de29..b7f89a4 100644 > > --- a/tests/qapi-schema/include-self-cycle.out > > +++ b/tests/qapi-schema/include-self-cycle.out > > @@ -0,0 +1,3 @@ > > +[] > > +[] > > +[] >=20 > This test shows the loss of cycle detection, too. >=20 > > diff --git a/tests/qapi-schema/sub-include-idempotent.json b/tests/qa= pi-schema/sub-include-idempotent.json > > new file mode 100644 > > index 0000000..94113c6 > > --- /dev/null > > +++ b/tests/qapi-schema/sub-include-idempotent.json > > @@ -0,0 +1,3 @@ > > +{ 'include': 'comments.json' } > > +{ 'include': 'sub-include-idempotent.json' } > > +{ 'include': 'comments.json' } >=20 > For what it's worth, your include-idempotent test case has a cycle. >=20