Re: [Qemu-devel] [PATCH v6 3/4] qapi: Add a primitive to include other files from a QAPI schema file

[Lluís Vilanova]

Eric Blake writes:

> On 03/31/2014 01:16 PM, Lluís Vilanova wrote:
[...]
>> +                if not os.path.isabs(include_path):
>> +                    include_path = os.path.join(self.input_dir, 
>> include_path)
>> +                if not os.path.isfile(include_path):
>> +                        raise QAPIExprError(
>> +                            expr_info,
>> +                            'Non-existing included file "%s"' %
>> +                            include_path)

> Is this error necessary, or would you get a sane error message by just
> trying to open the file?

Not catching it would throw a Python exception, with no context of how the user
got there.


[...]
>> +                if include_path in self.included:
>> +                    raise QAPIExprError(expr_info, "Infinite inclusion 
>> loop: %s"
>> +                                        % " -> ".join(self.included +
>> +                                                      [include_path]))

> Good, include-self-cycle.err covers a one-file loop, and
> include-cycle.err covers a 3-file loop.

> Not so good: your error message is an extremely long line:

> +tests/qapi-schema/include-cycle-c.json:1: Infinite inclusion loop:
> tests/qapi-schema/include-cycle.json ->
> tests/qapi-schema/include-cycle-b.json ->
> tests/qapi-schema/include-cycle-c.json ->
> tests/qapi-schema/include-cycle.json

> The formatting in Benoît's series was a little nicer aesthetically:

> +Inclusion loop detected with file: multi_file_loop_include.json
> +Path to the broken include is:
> +     multi_file_loop_include.json
> +     multi_loop.json

> Furthermore, it had the benefit of using the spelling provided by the
> user, rather than the absolute path to the files.  You want to track
> canonical paths for detecting the loop, but do NOT want to report
> absolute paths back to the user - instead, it's nicer to report back the
> names as they spelled it.

I think it's better reporting absolute paths, otherwise the user has to mentally
keep track of the relative paths to get to the file.


Thanks,
  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