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

[Markus Armbruster]

Max Reitz <address@hidden> writes:

> On 14.11.19 10:15, Markus Armbruster wrote:
>> Max Reitz <address@hidden> writes:
>> 
>>> Signed-off-by: Max Reitz <address@hidden>
>>> ---
>>>  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.

I'm totally cool with deviating from JSON, all I really care about is
proper schema language documentation.

If we stick to JSON form number syntax, this is easy: replace "Numbers
and null are not supported" by "null is not supported", done.
Implementation should also be easy enough: convert RFC 8259's EBNF to a
regexp, feed the matched string to Python's number parser, done.

Fancier syntax we'd need to document ourselves.  I'm willing to deal
with that if we have a sufficiently compelling use for them.

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

Ah, now I see.

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

Let's start simple & stupid.  Extensions can be added on top.
Hexadecimal integers may well be compelling enough to justify 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.

The test name 'bad-type-int' is now bad, because the int in it is no
longer bad (pardon my lame puns).

> Thanks for reviewing!

Better late than never, I guess...  You're welcome!

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