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

[Eric Blake]

On 03/31/2014 01:16 PM, Lluís Vilanova wrote:
> Signed-off-by: Lluís Vilanova <address@hidden>
> ---
>  docs/qapi-code-gen.txt |   11 +++++++++++
>  scripts/qapi.py        |   39 ++++++++++++++++++++++++++++++++++-----
>  2 files changed, 45 insertions(+), 5 deletions(-)

I would consider squashing 3 and 4 together, but not the end of the
world to keep them separate.

> @@ -75,9 +80,33 @@ class QAPISchema:
>  
>          while self.tok != None:
>              expr_info = {'fp': fp, 'line': self.line}
> -            expr_elem = {'expr': self.get_expr(False),
> -                         'info': expr_info}
> -            self.exprs.append(expr_elem)
> +            expr = self.get_expr(False)
> +            if isinstance(expr, dict) and "include" in expr:
> +                if len(expr) != 1:
> +                    raise QAPIExprError(expr_info, "Invalid 'include' 
> directive")

Not covered in patch 4.  Ideally, ALL new error messages also come with
a test that exposes the message.

> +                include_path = expr["include"]
> +                if not isinstance(include_path, str):
> +                    raise QAPIExprError(expr_info,
> +                                        'Expected a file path (string), got: 
> %s'
> +                                        % include_path)

s/path/name/ ("path" implies a series of directories to some people)

Good, this one is covered by include-non-file.err

> +                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?

s/included/include/

Good, covered by include-no-file.err

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature