The Thursday 08 May 2014 à 20:30:33 (+0200), Markus Armbruster wrote :
> Humor me: no period at end of subject.
>
> Benoît Canet <benoit.ca...@irqsave.net> writes:
>
> > The purpose of this change is to help create a json file containing
> > common definitions.
>
> Please describe the new semantics of the include directive here, so
> mathematically challenged readers don't have to loop up "idempotent" in
> the dictionary :)
>
> > The history used to detect multiple inclusions is not a stack anymore
> > but a regular list.
>
> You need a stack to show the actual include cycle.
>
> There are two reasons to detect cycles. The technical one is preventing
> 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.
>
> > The cycle detection check where updated and leaved in place to make
> > sure the code will never enter into an infinite recursion loop.
>
> -ENOPARSE
>
> Suggest to retry in active voice :)
>
> > Signed-off-by: Benoit Canet <ben...@irqsave.net>
> > ---
> > 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
>
> Is this based on Luiz's queue-qmp?
>
> > 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 = fp.name
> > self.input_dir = os.path.dirname(input_fname)
> > self.input_file = input_relname
> > - self.include_hist = include_hist + [(input_relname, input_fname)]
> > + include_hist.append((input_relname, input_fname))
> > self.parent_info = parent_info
> > self.src = fp.read()
> > if self.src == '' or self.src[-1] != '\n':
>
> 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?
>
> > @@ -102,17 +102,16 @@ class QAPISchema:
> > 'Expected a file name (string),
> > got: %s'
> > % include)
> > include_path = os.path.join(self.input_dir, include)
> > - if any(include_path == elem[1]
> > - for elem in self.include_hist):
> > - raise QAPIExprError(expr_info, "Inclusion loop for %s"
> > - % include)
> > + # make include idempotent by skipping further includes
> > + if include_path in [x[1] for x in include_hist]:
> > + continue
>
> Loses cycle detection.
It simply also skip cycle includes. If cycle are skipped then cannot do no
harm.
>
> Is permitting cycles necessary to solve your problem? Or asking more
> directly: do you actually need cyclic includes?
>
> > try:
> > fobj = open(include_path, 'r')
> > except IOError as e:
> > raise QAPIExprError(expr_info,
> > '%s: %s' % (e.strerror, include))
> > - exprs_include = QAPISchema(fobj, include,
> > - self.include_hist, expr_info)
> > + exprs_include = QAPISchema(fobj, include, include_hist,
> > + expr_info)
> > self.exprs.extend(exprs_include.exprs)
> > else:
> > expr_elem = {'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 := $(addprefix tests/qapi-schema/, \
> > 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-err.json
> > \
> > - include-nested-err.json include-self-cycle.json include-cycle.json)
> > + include-nested-err.json include-self-cycle.json include-cycle.json
> > \
> > + include-idempotent.json)
> >
> > GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h
> > tests/test-qmp-commands.h
> >
> > 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 @@
> > +[]
> > +[]
> > +[]
>
> 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.:
>
> > diff --git a/tests/qapi-schema/include-idempotent.err
> > b/tests/qapi-schema/include-idempotent.err
> > new file mode 100644
> > index 0000000..e69de29
> > diff --git a/tests/qapi-schema/include-idempotent.exit
> > b/tests/qapi-schema/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-schema/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-schema/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-schema/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
> > include-self-cycle.json
> > diff --git a/tests/qapi-schema/include-self-cycle.exit
> > b/tests/qapi-schema/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-schema/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 @@
> > +[]
> > +[]
> > +[]
>
> This test shows the loss of cycle detection, too.
>
> > diff --git a/tests/qapi-schema/sub-include-idempotent.json
> > b/tests/qapi-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' }
>
> For what it's worth, your include-idempotent test case has a cycle.
>
