* [Qemu-devel] [PATCH] qapi: Make the include directive idempotent.
@ 2014-05-08 15:18 Benoît Canet
2014-05-08 18:30 ` Markus Armbruster
0 siblings, 1 reply; 6+ messages in thread
From: Benoît Canet @ 2014-05-08 15:18 UTC (permalink / raw)
To: qemu-devel; +Cc: Benoît Canet, Benoit Canet, armbru, lcapitulino, vilanova
The purpose of this change is to help create a json file containing
common definitions.
The history used to detect multiple inclusions is not a stack anymore
but a regular list.
The cycle detection check where updated and leaved in place to make
sure the code will never enter into an infinite recursion loop.
Signed-off-by: Benoit Canet <benoit@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
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':
@@ -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
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 @@
+[]
+[]
+[]
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 @@
+[]
+[]
+[]
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' }
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qapi: Make the include directive idempotent.
2014-05-08 15:18 [Qemu-devel] [PATCH] qapi: Make the include directive idempotent Benoît Canet
@ 2014-05-08 18:30 ` Markus Armbruster
2014-05-08 20:04 ` Benoît Canet
0 siblings, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2014-05-08 18:30 UTC (permalink / raw)
To: Benoît Canet; +Cc: vilanova, lcapitulino, qemu-devel, Benoit Canet
Humor me: no period at end of subject.
Benoît Canet <benoit.canet@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 <benoit@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.
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qapi: Make the include directive idempotent.
2014-05-08 18:30 ` Markus Armbruster
@ 2014-05-08 20:04 ` Benoît Canet
2014-05-09 5:36 ` Markus Armbruster
0 siblings, 1 reply; 6+ messages in thread
From: Benoît Canet @ 2014-05-08 20:04 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Benoît Canet, qemu-devel, vilanova, lcapitulino
The Thursday 08 May 2014 à 20:30:33 (+0200), Markus Armbruster wrote :
> Humor me: no period at end of subject.
>
> Benoît Canet <benoit.canet@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 <benoit@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.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qapi: Make the include directive idempotent.
2014-05-08 20:04 ` Benoît Canet
@ 2014-05-09 5:36 ` Markus Armbruster
2014-05-09 14:55 ` Lluís Vilanova
2014-05-09 14:55 ` Benoît Canet
0 siblings, 2 replies; 6+ messages in thread
From: Markus Armbruster @ 2014-05-09 5:36 UTC (permalink / raw)
To: Benoît Canet; +Cc: lcapitulino, qemu-devel, vilanova
Benoît Canet <benoit.canet@irqsave.net> writes:
> The Thursday 08 May 2014 à 20:30:33 (+0200), Markus Armbruster wrote :
[...]
>> 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.
[...]
>> > @@ -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.
Your argument is based exclusively on the technical reason to detect
cycles: cycles need to be caught because they cause infinite recursion.
Since there is no infinite recursion with idempotent include, cycles are
just fine.
I'm arguing from a more abstract point of view: cycles should be
rejected because they're nonsensical. The fact that they can cause
infinite recursion is an implementation detail. Even without infinite
recursion, they're just as nonsensical as ever.
[...]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qapi: Make the include directive idempotent.
2014-05-09 5:36 ` Markus Armbruster
@ 2014-05-09 14:55 ` Lluís Vilanova
2014-05-09 14:55 ` Benoît Canet
1 sibling, 0 replies; 6+ messages in thread
From: Lluís Vilanova @ 2014-05-09 14:55 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Benoît Canet, qemu-devel, lcapitulino
Markus Armbruster writes:
> Benoît Canet <benoit.canet@irqsave.net> writes:
>> The Thursday 08 May 2014 à 20:30:33 (+0200), Markus Armbruster wrote :
> [...]
>>> 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.
> [...]
>>> > @@ -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.
> Your argument is based exclusively on the technical reason to detect
> cycles: cycles need to be caught because they cause infinite recursion.
> Since there is no infinite recursion with idempotent include, cycles are
> just fine.
> I'm arguing from a more abstract point of view: cycles should be
> rejected because they're nonsensical. The fact that they can cause
> infinite recursion is an implementation detail. Even without infinite
> recursion, they're just as nonsensical as ever.
> [...]
I agree, a cycle is not the same as a double definition due to a file being
included multiple times:
1) main.json: include bar.json
bar.json : include baz.json
baz.json : include bar.json
2) main.json: include bar.json
include baz.json
bar.json : include common.json
...
baz.json : include common.json
...
common.json: ....
The first one is obviously wrong (there's a cycle), while the second one needs
the idempotency feature to avoid doubly defining the contents in common.json.
Another question is whether keeping cycle detection as an error is worth the
coding effort. It should not be very complex if you keep a global (as in this
patch) and a local (as I did) inclusion history.
Lluis
--
"And it's much the same thing with knowledge, for whenever you learn
something new, the whole world becomes that much richer."
-- The Princess of Pure Reason, as told by Norton Juster in The Phantom
Tollbooth
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] qapi: Make the include directive idempotent.
2014-05-09 5:36 ` Markus Armbruster
2014-05-09 14:55 ` Lluís Vilanova
@ 2014-05-09 14:55 ` Benoît Canet
1 sibling, 0 replies; 6+ messages in thread
From: Benoît Canet @ 2014-05-09 14:55 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Benoît Canet, vilanova, qemu-devel, lcapitulino
The Friday 09 May 2014 à 07:36:31 (+0200), Markus Armbruster wrote :
> Benoît Canet <benoit.canet@irqsave.net> writes:
>
> > The Thursday 08 May 2014 à 20:30:33 (+0200), Markus Armbruster wrote :
> [...]
> >> 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.
> [...]
> >> > @@ -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.
>
> Your argument is based exclusively on the technical reason to detect
> cycles: cycles need to be caught because they cause infinite recursion.
> Since there is no infinite recursion with idempotent include, cycles are
> just fine.
>
> I'm arguing from a more abstract point of view: cycles should be
> rejected because they're nonsensical. The fact that they can cause
> infinite recursion is an implementation detail. Even without infinite
> recursion, they're just as nonsensical as ever.
>
> [...]
>
Ok. will redo it.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-05-09 14:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-08 15:18 [Qemu-devel] [PATCH] qapi: Make the include directive idempotent Benoît Canet
2014-05-08 18:30 ` Markus Armbruster
2014-05-08 20:04 ` Benoît Canet
2014-05-09 5:36 ` Markus Armbruster
2014-05-09 14:55 ` Lluís Vilanova
2014-05-09 14:55 ` Benoît Canet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).