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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9284 bytes --]

On 14.11.19 10:15, Markus Armbruster wrote:
> 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.

Well, it isn’t really an extension, because this isn’t a JSON parser but
just something that accepts anything that looks like a number and then
lets Python try a conversion on it.

> Isn't the parenthesis in your regular expression redundant?

You’re right, but on second thought, maybe I should surround it by \<
and \>.

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

I could imagine inf being a useful default value, actually.  nan,
probably not so much.

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

Well, kind of.

I wanted to accept anything that looks in any way like a number and then
let Python try to convert it.  That’s also the reason why the case comes
last.

For that reason, I decided to keep the regex as simple as possible,
because the attempted conversions would reject anything that isn’t (to
Python) a valid number later.

It was my impression that the QAPI schema isn’t really JSON anyway and
that our QAPI schema parser isn’t a JSON parser.  Under that assumption
it simply seemed useful to me to accept anything that could potentially
be a number to Python and convert it.

Now, honestly, I still don’t see the point of having a strict JSON
“parser” here, but if you insist.  Seems possible to do in a regex.

Though I do think it makes sense to support hex integers as an extension.

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

OK.

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

Because the match is so broad.

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

Will do.

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

I’m not adding a test here, it’s just the value has changed in
4d42815587063d.

Thanks for reviewing!

Max

>> 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"



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]