Re: [Qemu-devel] [PATCH v4 01/14] qapi: Parse numeric values

[Markus Armbruster <armbru@redhat.com>]

Max Reitz <mreitz@redhat.com> writes:

> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qapi-schema/bad-type-int.json      |  1 -
>  tests/qapi-schema/enum-int-member.json   |  1 -
>  scripts/qapi/common.py                   | 25 ++++++++++++++++++++----
>  scripts/qapi/introspect.py               |  2 ++
>  tests/qapi-schema/bad-type-int.err       |  2 +-
>  tests/qapi-schema/enum-int-member.err    |  2 +-
>  tests/qapi-schema/leading-comma-list.err |  2 +-
>  7 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/tests/qapi-schema/bad-type-int.json b/tests/qapi-schema/bad-type-int.json
> index 56fc6f8126..81355eb196 100644
> --- a/tests/qapi-schema/bad-type-int.json
> +++ b/tests/qapi-schema/bad-type-int.json
> @@ -1,3 +1,2 @@
>  # we reject an expression with a metatype that is not a string
> -# FIXME: once the parser understands integer inputs, improve the error message
>  { 'struct': 1, 'data': { } }
> diff --git a/tests/qapi-schema/enum-int-member.json b/tests/qapi-schema/enum-int-member.json
> index 6c9c32e149..6958440c6d 100644
> --- a/tests/qapi-schema/enum-int-member.json
> +++ b/tests/qapi-schema/enum-int-member.json
> @@ -1,3 +1,2 @@
>  # we reject any enum member that is not a string
> -# FIXME: once the parser understands integer inputs, improve the error message
>  { 'enum': 'MyEnum', 'data': [ 1 ] }
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index d61bfdc526..3396ea4a09 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -498,6 +498,8 @@ class QAPISchemaParser(object):
>              raise QAPISemError(info, "Unknown pragma '%s'" % name)
>  
>      def accept(self, skip_comment=True):
> +        num_match = re.compile(r'([-+]?inf|nan|[-+0-9.][0-9a-f.ex]*)')
> +
>          while True:
>              self.tok = self.src[self.cursor]
>              self.pos = self.cursor

This is yet another extension over plain JSON.  RFC 8259:

      number = [ minus ] int [ frac ] [ exp ]
      decimal-point = %x2E       ; .
      digit1-9 = %x31-39         ; 1-9
      e = %x65 / %x45            ; e E
      exp = e [ minus / plus ] 1*DIGIT
      frac = decimal-point 1*DIGIT
      int = zero / ( digit1-9 *DIGIT )
      minus = %x2D               ; -
      plus = %x2B                ; +
      zero = %x30                ; 0

Extensions are acceptable when we have an actual use for it, and we
document them properly.

Isn't the parenthesis in your regular expression redundant?

What use do you have in mind for 'inf' and 'nan'?

Why accept leading '+' as in '+123'?

Why accept empty integral part as in '.123'?

Why accept '.xe.'?  Kidding you, that must be a bug in your regexp.
Please decide what number syntax you'd like to accept, then specify it
in docs/devel/qapi-code-gen.txt, so we can first discuss the
specification, and then check the regexp implements it.

docs/devel/qapi-code-gen.txt update goes here:

    === Schema syntax ===

    Syntax is loosely based on JSON (http://www.ietf.org/rfc/rfc8259.txt).
    Differences:

    * Comments: start with a hash character (#) that is not part of a
      string, and extend to the end of the line.

    * Strings are enclosed in 'single quotes', not "double quotes".

    * Strings are restricted to printable ASCII, and escape sequences to
      just '\\'.

--> * Numbers and null are not supported.

Hrmm, commit 9d55380b5a "qapi: Remove null from schema language" left
two instances in error messages behind.  I'll fix them.

> @@ -584,7 +586,22 @@ class QAPISchemaParser(object):
>                      return
>                  self.line += 1
>                  self.line_pos = self.cursor
> -            elif not self.tok.isspace():
> +            elif self.tok.isspace():
> +                pass
> +            elif num_match.match(self.src[self.pos:]):
> +                match = num_match.match(self.src[self.pos:]).group(0)

Sadly, the walrus operator is Python 3.8.

> +                try:
> +                    self.val = int(match, 0)
> +                except ValueError:
> +                    try:
> +                        self.val = float(match)
> +                    except ValueError:
> +                        raise QAPIParseError(self,
> +                                '"%s" is not a valid integer or float' % match)
> +
> +                self.cursor += len(match) - 1
> +                return
> +            else:
>                  raise QAPIParseError(self, 'Stray "%s"' % self.tok)

Any particular reason for putting the number case last?

>  
>      def get_members(self):
> @@ -617,9 +634,9 @@ class QAPISchemaParser(object):
>          if self.tok == ']':
>              self.accept()
>              return expr
> -        if self.tok not in "{['tfn":
> +        if self.tok not in "{['tfn-+0123456789.i":

This is getting a bit ugly.  Let's not worry about it now.

>              raise QAPIParseError(self, 'Expected "{", "[", "]", string, '
> -                                 'boolean or "null"')
> +                                 'boolean, number or "null"')
>          while True:
>              expr.append(self.get_expr(True))
>              if self.tok == ']':
> @@ -638,7 +655,7 @@ class QAPISchemaParser(object):
>          elif self.tok == '[':
>              self.accept()
>              expr = self.get_values()
> -        elif self.tok in "'tfn":
> +        elif self.tok in "'tfn-+0123456789.i":
>              expr = self.val
>              self.accept()
>          else:
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index f62cf0a2e1..6a61dd831f 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -57,6 +57,8 @@ def to_qlit(obj, level=0, suppress_first_indent=False):
>          ret += indent(level) + '}))'
>      elif isinstance(obj, bool):
>          ret += 'QLIT_QBOOL(%s)' % ('true' if obj else 'false')
> +    elif isinstance(obj, int) and obj >= -(2 ** 63) and obj < 2 ** 63:
> +        ret += 'QLIT_QNUM(%i)' % obj

Please explain the range check.

>      else:
>          assert False                # not implemented
>      if level > 0:
> diff --git a/tests/qapi-schema/bad-type-int.err b/tests/qapi-schema/bad-type-int.err
> index da89895404..e22fb4f655 100644
> --- a/tests/qapi-schema/bad-type-int.err
> +++ b/tests/qapi-schema/bad-type-int.err
> @@ -1 +1 @@
> -tests/qapi-schema/bad-type-int.json:3:13: Stray "1"
> +tests/qapi-schema/bad-type-int.json:2: 'struct' key must have a string value

Test needs a rename, assuming it's not redundant now.

> diff --git a/tests/qapi-schema/enum-int-member.err b/tests/qapi-schema/enum-int-member.err
> index 071c5213d8..112175f79d 100644
> --- a/tests/qapi-schema/enum-int-member.err
> +++ b/tests/qapi-schema/enum-int-member.err
> @@ -1 +1 @@
> -tests/qapi-schema/enum-int-member.json:3:31: Stray "1"
> +tests/qapi-schema/enum-int-member.json:2: Member of enum 'MyEnum' requires a string name

This one's name is still good.

> diff --git a/tests/qapi-schema/leading-comma-list.err b/tests/qapi-schema/leading-comma-list.err
> index f5c870bb9c..fa9c80aa57 100644
> --- a/tests/qapi-schema/leading-comma-list.err
> +++ b/tests/qapi-schema/leading-comma-list.err
> @@ -1 +1 @@
> -tests/qapi-schema/leading-comma-list.json:2:13: Expected "{", "[", "]", string, boolean or "null"
> +tests/qapi-schema/leading-comma-list.json:2:13: Expected "{", "[", "]", string, boolean, number or "null"