Re: [Qemu-devel] [PATCH v6 07/17] qapi: use a QAPIParseError in parser

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/qapi.py                          | 13 +++++++------
>  tests/qapi-schema/include-cycle.err      |  2 +-
>  tests/qapi-schema/include-format-err.err |  2 +-
>  tests/qapi-schema/include-no-file.err    |  2 +-
>  tests/qapi-schema/include-non-file.err   |  2 +-
>  tests/qapi-schema/include-self-cycle.err |  2 +-
>  6 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 5885c9e4ad..5423b64b23 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -148,19 +148,19 @@ class QAPISchemaParser(object):
>              expr = self.get_expr(False)
>              if isinstance(expr, dict) and "include" in expr:
>                  if len(expr) != 1:
> -                    raise QAPISemError(info, "Invalid 'include' directive")
> +                    raise QAPIParseError(self, "Invalid 'include' directive")
>                  include = expr["include"]
>                  if not isinstance(include, str):
> -                    raise QAPISemError(info,
> -                                       "Value of 'include' must be a string")
> +                    raise QAPIParseError(self,
> +                                         "Value of 'include' must be a string")
>                  incl_abs_fname = os.path.join(os.path.dirname(abs_fname),
>                                                include)
>                  # catch inclusion cycle
>                  inf = info
>                  while inf:
>                      if incl_abs_fname == os.path.abspath(inf['file']):
> -                        raise QAPISemError(info, "Inclusion loop for %s"
> -                                           % include)
> +                        raise QAPIParseError(self, "Inclusion loop for %s"
> +                                             % include)
>                      inf = inf['parent']
>                  # skip multiple include of the same file
>                  if incl_abs_fname in previously_included:
> @@ -168,7 +168,8 @@ class QAPISchemaParser(object):
>                  try:
>                      fobj = open(incl_abs_fname, 'r')
>                  except IOError as e:
> -                    raise QAPISemError(info, '%s: %s' % (e.strerror, include))
> +                    raise QAPIParseError(self, '%s: %s' %
> +                                         (e.strerror, include))
>                  exprs_include = QAPISchemaParser(fobj, previously_included,
>                                                   info)
>                  self.exprs.extend(exprs_include.exprs)
> diff --git a/tests/qapi-schema/include-cycle.err b/tests/qapi-schema/include-cycle.err
> index bdcd07dce2..f96edd2eff 100644
> --- a/tests/qapi-schema/include-cycle.err
> +++ b/tests/qapi-schema/include-cycle.err
> @@ -1,3 +1,3 @@
>  In file included from tests/qapi-schema/include-cycle.json:1:
>  In file included from tests/qapi-schema/include-cycle-b.json:1:
> -tests/qapi-schema/include-cycle-c.json:1: Inclusion loop for include-cycle.json
> +tests/qapi-schema/include-cycle-c.json:1:36: Inclusion loop for include-cycle.json

Location :1:36 points to end of include expression.

> diff --git a/tests/qapi-schema/include-format-err.err b/tests/qapi-schema/include-format-err.err
> index 721ff4eccc..ac79ee4769 100644
> --- a/tests/qapi-schema/include-format-err.err
> +++ b/tests/qapi-schema/include-format-err.err
> @@ -1 +1 @@
> -tests/qapi-schema/include-format-err.json:1: Invalid 'include' directive
> +tests/qapi-schema/include-format-err.json:2:17: Invalid 'include' directive
> diff --git a/tests/qapi-schema/include-no-file.err b/tests/qapi-schema/include-no-file.err

Likewise.

> index d5b9b22d85..de321ac477 100644
> --- a/tests/qapi-schema/include-no-file.err
> +++ b/tests/qapi-schema/include-no-file.err
> @@ -1 +1 @@
> -tests/qapi-schema/include-no-file.json:1: No such file or directory: include-no-file-sub.json
> +tests/qapi-schema/include-no-file.json:1:42: No such file or directory: include-no-file-sub.json
> diff --git a/tests/qapi-schema/include-non-file.err b/tests/qapi-schema/include-non-file.err
> index faae1eacf1..345f6803eb 100644
> --- a/tests/qapi-schema/include-non-file.err
> +++ b/tests/qapi-schema/include-non-file.err
> @@ -1 +1 @@
> -tests/qapi-schema/include-non-file.json:1: Value of 'include' must be a string
> +tests/qapi-schema/include-non-file.json:1:18: Value of 'include' must be a string

Likewise.

> diff --git a/tests/qapi-schema/include-self-cycle.err b/tests/qapi-schema/include-self-cycle.err
> index 981742ae5f..abfc3ed6f3 100644
> --- a/tests/qapi-schema/include-self-cycle.err
> +++ b/tests/qapi-schema/include-self-cycle.err
> @@ -1 +1 @@
> -tests/qapi-schema/include-self-cycle.json:1: Inclusion loop for include-self-cycle.json
> +tests/qapi-schema/include-self-cycle.json:1:41: Inclusion loop for include-self-cycle.json

Likewise.

The reason for this location is that the error isn't actually a parse
error.

Parse errors happen when the parser runs into a unexpected token, and
then the location stored in the parser points exactly there.  Good.

The errors above happen while checking an expression the parser
accepted, in other words, they're semantic errors.  Because the parser
already accepted the expression, the location stored in it points beyond
the expression.  Using it is not so good.

I proposed to use QAPIParseError here.  Sorry for misleading you.

Thankfully, you made it its own patch, which should be easy to drop.