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

[Markus Armbruster]

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.

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.

> 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.