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

[Lluís Vilanova]

Markus Armbruster writes:

> Benoît Canet <address@hidden> 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