| Message ID | 1398764656-27534-2-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 29.04.2014 um 11:44 hat Fam Zheng geschrieben: > This allows giving decimal constants in the schema as expr. > > Signed-off-by: Fam Zheng <famz@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
On 04/29/2014 03:44 AM, Fam Zheng wrote: > This allows giving decimal constants in the schema as expr. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > scripts/qapi.py | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index b474c39..6022de5 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -108,6 +108,14 @@ class QAPISchema: > return > else: > string += ch > + elif self.tok in "123456789": > + self.val = int(self.tok) > + while True: > + ch = self.src[self.cursor] > + if ch not in "1234567890": > + return > + self.val = self.val * 10 + int(ch) > + self.cursor += 1 What does this do on integer overflow? Should you validate that the result fits in [u]int64_t? Should you allow for a leading '-' sign for a default of a negative number? You have forbidden '0' as a valid number (although you correctly forbid '00' and any non-zero number with a leading 0, which matches JSON restrictions). > - if not self.tok in [ '{', '[', "'" ]: > - raise QAPISchemaError(self, 'Expected "{", "[", "]" or string') > + if not self.tok in "{['123456789": > + raise QAPISchemaError(self, 'Expected "{", "[", "]", string or number') Again, this forbids the use of '0' as a default.
Eric Blake <eblake@redhat.com> writes: > On 04/29/2014 03:44 AM, Fam Zheng wrote: >> This allows giving decimal constants in the schema as expr. >> >> Signed-off-by: Fam Zheng <famz@redhat.com> >> --- >> scripts/qapi.py | 15 +++++++++++++-- >> 1 file changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/scripts/qapi.py b/scripts/qapi.py >> index b474c39..6022de5 100644 >> --- a/scripts/qapi.py >> +++ b/scripts/qapi.py >> @@ -108,6 +108,14 @@ class QAPISchema: >> return >> else: >> string += ch >> + elif self.tok in "123456789": >> + self.val = int(self.tok) >> + while True: >> + ch = self.src[self.cursor] >> + if ch not in "1234567890": >> + return >> + self.val = self.val * 10 + int(ch) >> + self.cursor += 1 > > What does this do on integer overflow? Should you validate that the > result fits in [u]int64_t? Should you allow for a leading '-' sign for > a default of a negative number? You have forbidden '0' as a valid > number (although you correctly forbid '00' and any non-zero number with > a leading 0, which matches JSON restrictions). If every valid JSON number is also a valid Python number, then you can accumulate the token, then convert it with built-in function int(). Just like our C JSON lexer uses strtoll(), in parse_literal(). >> - if not self.tok in [ '{', '[', "'" ]: >> - raise QAPISchemaError(self, 'Expected "{", "[", "]" or string') >> + if not self.tok in "{['123456789": >> + raise QAPISchemaError(self, 'Expected "{", "[", "]", string or >> number') > > Again, this forbids the use of '0' as a default. The spec for our lexical analysis comes from RFC 4627. We take some liberties with it, but we should do so only deliberately, and with a sensible reason. We should also keep the Python version (qapi.py) consistent with the C version (qobject/json*.[ch]). In particular see how parse_literal() deals with integer overflow. Grammar for numbers, straight from RFC 4627: 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 Since we distinguish between integral and fractional numbers everywhere in our usage of JSON, it makes sense to do so here as well. This means we want to accept this sub-grammar: inum = [ minus ] int minus = %x2D ; - digit1-9 = %x31-39 ; 1-9 zero = %x30 ; 0 int = zero / ( digit1-9 *DIGIT ) json-lexer.c's state machine implements this faithfully (as far as I can tell). Fractional numbers could be left for later here, since your PATCH 2/2, the first and so far only user of numbers, arbitrarily restricts them to integer. Just implementing them might be easier than documenting the restriction, though...
diff --git a/scripts/qapi.py b/scripts/qapi.py index b474c39..6022de5 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -108,6 +108,14 @@ class QAPISchema: return else: string += ch + elif self.tok in "123456789": + self.val = int(self.tok) + while True: + ch = self.src[self.cursor] + if ch not in "1234567890": + return + self.val = self.val * 10 + int(ch) + self.cursor += 1 elif self.tok == '\n': if self.cursor == len(self.src): self.tok = None @@ -147,8 +155,8 @@ class QAPISchema: if self.tok == ']': self.accept() return expr - if not self.tok in [ '{', '[', "'" ]: - raise QAPISchemaError(self, 'Expected "{", "[", "]" or string') + if not self.tok in "{['123456789": + raise QAPISchemaError(self, 'Expected "{", "[", "]", string or number') while True: expr.append(self.get_expr(True)) if self.tok == ']': @@ -170,6 +178,9 @@ class QAPISchema: elif self.tok == "'": expr = self.val self.accept() + elif self.tok in "123456789": + expr = self.val + self.accept() else: raise QAPISchemaError(self, 'Expected "{", "[" or string') return expr
This allows giving decimal constants in the schema as expr. Signed-off-by: Fam Zheng <famz@redhat.com> --- scripts/qapi.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)