Re: [Qemu-devel] [PATCH] qapi: Make the include directive idempotent.

[Benoît Canet]

The Thursday 08 May 2014 à 20:30:33 (+0200), Markus Armbruster wrote :
> Humor me: no period at end of subject.
> 
> Benoît Canet <address@hidden> 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 <address@hidden>
> > ---
> >  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.
>