[Qemu-devel] [PATCH 0/6]: QMP: Fix issues in parser/lexer

[Qemu-devel] [PATCH 0/6]: QMP: Fix issues in parser/lexer

[Luiz Capitulino]

 Hi Anthony,

 While investigating a QMP bug reported by a user, I've found a few issues
in our parser/lexer.

 The patches in this series fix the problems I was able to solve, but we
still have the following issues:

1. Our 'private extension' is open to the public

   Eg. The following input issued by a client is valid:

   { 'execute': 'query-pci' }

   I don't think it's a good idea to have clients relying on this kind of
   JSON extension.

   To fix this we could add a 'extension' flag to JSONLexer and set it to
   nonzero in internal functions (eg. qobject_from_jsonf()), of course that
   the lexer code should handle this too.

2. QMP doesn't check the return of json_message_parser_feed()

   Which means we don't handle JSON syntax errors. While the fix might seem
   trivial (ie. just return an error!), I'm not sure what's the best way
   to handle this, because the streamer seems to return multiple errors for
   the same input string.

   For example, this input:

   { "execute": yy_uu }

   Seems to return an error for each bad character (yy_uu), shouldn't it
   return only once and stop processing the whole string?

3. The lexer enter in ERROR state when processing is done

   Not sure whether this is an issue, but I found it while reviewing the code
   and maybe this is related with item 2 above.

   When json_lexer_feed_char() is finished scanning a string, (ie. ch='\0')
   the JSON_SKIP clause will set lexer->state to ERROR as there's no entry
   for '\0' in the IN_START array.

   Shouldn't we have a LEXER_DONE or something like it instead?

4. Lexer expects a 'terminal' char to process a token

   Which means clients must send a sort of end of line char, so that we
   process their input.

   Maybe I'm missing something here, but I thought that the whole point of
   writing our own parser was to avoid this.

[Qemu-devel] [PATCH 1/6] json-lexer: Initialize 'x' and 'y'

[Luiz Capitulino]

The 'lexer' variable is passed by the caller, it can contain anything
(eg. garbage).

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 json-lexer.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/json-lexer.c b/json-lexer.c
index 9d64920..0b145d1 100644
--- a/json-lexer.c
+++ b/json-lexer.c
@@ -275,6 +275,7 @@ void json_lexer_init(JSONLexer *lexer, JSONLexerEmitter func)
     lexer->emit = func;
     lexer->state = IN_START;
     lexer->token = qstring_new();
+    lexer->x = lexer->y = 0;
 }
 
 static int json_lexer_feed_char(JSONLexer *lexer, char ch)
-- 
1.7.1.86.g0e460

[Qemu-devel] [PATCH 2/6] json-lexer: Handle missing escapes

[Luiz Capitulino]

The JSON escape sequence "\/" and "\\" are valid and should be
handled.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 json-lexer.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/json-lexer.c b/json-lexer.c
index 0b145d1..5cc7e6c 100644
--- a/json-lexer.c
+++ b/json-lexer.c
@@ -97,6 +97,8 @@ static const uint8_t json_lexer[][256] =  {
         ['n'] =  IN_DQ_STRING,
         ['r'] =  IN_DQ_STRING,
         ['t'] =  IN_DQ_STRING,
+        ['/'] = IN_DQ_STRING,
+        ['\\'] = IN_DQ_STRING,
         ['\''] = IN_DQ_STRING,
         ['\"'] = IN_DQ_STRING,
         ['u'] = IN_DQ_UCODE0,
@@ -134,6 +136,8 @@ static const uint8_t json_lexer[][256] =  {
         ['n'] =  IN_SQ_STRING,
         ['r'] =  IN_SQ_STRING,
         ['t'] =  IN_SQ_STRING,
+        ['/'] = IN_DQ_STRING,
+        ['\\'] = IN_DQ_STRING,
         ['\''] = IN_SQ_STRING,
         ['\"'] = IN_SQ_STRING,
         ['u'] = IN_SQ_UCODE0,
-- 
1.7.1.86.g0e460

Re: [Qemu-devel] [PATCH 2/6] json-lexer: Handle missing escapes

[Anthony Liguori]

On 05/19/2010 04:15 PM, Luiz Capitulino wrote:
> The JSON escape sequence "\/" and "\\" are valid and should be
> handled.
>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>    

Good catch.

> ---
>   json-lexer.c |    4 ++++
>   1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/json-lexer.c b/json-lexer.c
> index 0b145d1..5cc7e6c 100644
> --- a/json-lexer.c
> +++ b/json-lexer.c
> @@ -97,6 +97,8 @@ static const uint8_t json_lexer[][256] =  {
>           ['n'] =  IN_DQ_STRING,
>           ['r'] =  IN_DQ_STRING,
>           ['t'] =  IN_DQ_STRING,
> +        ['/'] = IN_DQ_STRING,
> +        ['\\'] = IN_DQ_STRING,
>           ['\''] = IN_DQ_STRING,
>           ['\"'] = IN_DQ_STRING,
>           ['u'] = IN_DQ_UCODE0,
> @@ -134,6 +136,8 @@ static const uint8_t json_lexer[][256] =  {
>           ['n'] =  IN_SQ_STRING,
>           ['r'] =  IN_SQ_STRING,
>           ['t'] =  IN_SQ_STRING,
> +        ['/'] = IN_DQ_STRING,
> +        ['\\'] = IN_DQ_STRING,
>           ['\''] = IN_SQ_STRING,
>           ['\"'] = IN_SQ_STRING,
>           ['u'] = IN_SQ_UCODE0,
>    

Re: [Qemu-devel] [PATCH 2/6] json-lexer: Handle missing escapes

[Luiz Capitulino]

On Wed, 19 May 2010 16:44:47 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 05/19/2010 04:15 PM, Luiz Capitulino wrote:
> > The JSON escape sequence "\/" and "\\" are valid and should be
> > handled.
> >
> > Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
> >    
> 
> Good catch.

 I think there's another issue in the handling of strings.

 The spec says that valid unescaped chars are in the following range:

    unescaped = %x20-21 / %x23-5B / %x5D-10FFFF

 But we do:

    [IN_DQ_STRING] = {
        [1 ... 0xFF] = IN_DQ_STRING,
        ['\\'] = IN_DQ_STRING_ESCAPE,
        ['"'] = IN_DONE_STRING,
    },

 Shouldn't we cover 0x20 .. 0xFF instead?

[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes

[Paolo Bonzini]

On 05/20/2010 03:44 PM, Luiz Capitulino wrote:
>   I think there's another issue in the handling of strings.
>
>   The spec says that valid unescaped chars are in the following range:
>
>      unescaped = %x20-21 / %x23-5B / %x5D-10FFFF
>
>   But we do:
>
>      [IN_DQ_STRING] = {
>          [1 ... 0xFF] = IN_DQ_STRING,
>          ['\\'] = IN_DQ_STRING_ESCAPE,
>          ['"'] = IN_DONE_STRING,
>      },
>
>   Shouldn't we cover 0x20 .. 0xFF instead?

If it's the lexer, isn't just it being liberal in what it accepts?

paolo

[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes

[Luiz Capitulino]

On Thu, 20 May 2010 17:16:01 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 05/20/2010 03:44 PM, Luiz Capitulino wrote:
> >   I think there's another issue in the handling of strings.
> >
> >   The spec says that valid unescaped chars are in the following range:
> >
> >      unescaped = %x20-21 / %x23-5B / %x5D-10FFFF
> >
> >   But we do:
> >
> >      [IN_DQ_STRING] = {
> >          [1 ... 0xFF] = IN_DQ_STRING,
> >          ['\\'] = IN_DQ_STRING_ESCAPE,
> >          ['"'] = IN_DONE_STRING,
> >      },
> >
> >   Shouldn't we cover 0x20 .. 0xFF instead?
> 
> If it's the lexer, isn't just it being liberal in what it accepts?

 Yes, it's the lexer, but you meant that the fix should be in
somewhere else?

[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes

[Paolo Bonzini]

On 05/20/2010 05:25 PM, Luiz Capitulino wrote:
> On Thu, 20 May 2010 17:16:01 +0200
> Paolo Bonzini<pbonzini@redhat.com>  wrote:
>
>> On 05/20/2010 03:44 PM, Luiz Capitulino wrote:
>>>    I think there's another issue in the handling of strings.
>>>
>>>    The spec says that valid unescaped chars are in the following range:
>>>
>>>       unescaped = %x20-21 / %x23-5B / %x5D-10FFFF
>>>
>>>    But we do:
>>>
>>>       [IN_DQ_STRING] = {
>>>           [1 ... 0xFF] = IN_DQ_STRING,
>>>           ['\\'] = IN_DQ_STRING_ESCAPE,
>>>           ['"'] = IN_DONE_STRING,
>>>       },
>>>
>>>    Shouldn't we cover 0x20 .. 0xFF instead?
>>
>> If it's the lexer, isn't just it being liberal in what it accepts?
>
>   Yes, it's the lexer, but you meant that the fix should be in
> somewhere else?

I meant that we're just accepting some invalid JSON and that's not a big 
deal.

Paolo

[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes

[Luiz Capitulino]

On Thu, 20 May 2010 17:26:03 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 05/20/2010 05:25 PM, Luiz Capitulino wrote:
> > On Thu, 20 May 2010 17:16:01 +0200
> > Paolo Bonzini<pbonzini@redhat.com>  wrote:
> >
> >> On 05/20/2010 03:44 PM, Luiz Capitulino wrote:
> >>>    I think there's another issue in the handling of strings.
> >>>
> >>>    The spec says that valid unescaped chars are in the following range:
> >>>
> >>>       unescaped = %x20-21 / %x23-5B / %x5D-10FFFF
> >>>
> >>>    But we do:
> >>>
> >>>       [IN_DQ_STRING] = {
> >>>           [1 ... 0xFF] = IN_DQ_STRING,
> >>>           ['\\'] = IN_DQ_STRING_ESCAPE,
> >>>           ['"'] = IN_DONE_STRING,
> >>>       },
> >>>
> >>>    Shouldn't we cover 0x20 .. 0xFF instead?
> >>
> >> If it's the lexer, isn't just it being liberal in what it accepts?
> >
> >   Yes, it's the lexer, but you meant that the fix should be in
> > somewhere else?
> 
> I meant that we're just accepting some invalid JSON and that's not a big 
> deal.

 It can become a big deal if clients rely on it and for some reason we
decide we should drop it. Ie. after QMP is declared stable such changes
won't be allowed.

 Yes, I know, the chances of someone relying on this kind of thing is
probably almost zero. At the same time I think we should be very
conservative if there's no good reason to do otherwise.

[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes

[Anthony Liguori]

On 05/20/2010 10:35 AM, Luiz Capitulino wrote:
>> I meant that we're just accepting some invalid JSON and that's not a big
>> deal.
>>      
>   It can become a big deal if clients rely on it and for some reason we
> decide we should drop it. Ie. after QMP is declared stable such changes
> won't be allowed.
>    

Clients should only rely on standard JSON.  Anything else is a bug in 
the client.

Regards,

Anthony Liguori

>   Yes, I know, the chances of someone relying on this kind of thing is
> probably almost zero. At the same time I think we should be very
> conservative if there's no good reason to do otherwise.
>    

[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes

[Luiz Capitulino]

On Thu, 20 May 2010 10:54:42 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 05/20/2010 10:35 AM, Luiz Capitulino wrote:
> >> I meant that we're just accepting some invalid JSON and that's not a big
> >> deal.
> >>      
> >   It can become a big deal if clients rely on it and for some reason we
> > decide we should drop it. Ie. after QMP is declared stable such changes
> > won't be allowed.
> >    
> 
> Clients should only rely on standard JSON.  Anything else is a bug in 
> the client.

 I feel this is like a trap, why exposing it if don't want clients
to use them?

[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes

[Anthony Liguori]

On 05/20/2010 10:16 AM, Paolo Bonzini wrote:
> On 05/20/2010 03:44 PM, Luiz Capitulino wrote:
>>   I think there's another issue in the handling of strings.
>>
>>   The spec says that valid unescaped chars are in the following range:
>>
>>      unescaped = %x20-21 / %x23-5B / %x5D-10FFFF

That's a spec bug IMHO.  Tab is %x09.  Surely you can include tabs in 
strings.  Any parser that didn't accept that would be broken.

>>
>>   But we do:
>>
>>      [IN_DQ_STRING] = {
>>          [1 ... 0xFF] = IN_DQ_STRING,
>>          ['\\'] = IN_DQ_STRING_ESCAPE,
>>          ['"'] = IN_DONE_STRING,
>>      },
>>
>>   Shouldn't we cover 0x20 .. 0xFF instead?
>
> If it's the lexer, isn't just it being liberal in what it accepts?

I believe the parser correctly rejects invalid UTF-8 sequences.

Regards,

Anthony Liguori

> paolo

[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes

[Luiz Capitulino]

On Thu, 20 May 2010 10:50:41 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 05/20/2010 10:16 AM, Paolo Bonzini wrote:
> > On 05/20/2010 03:44 PM, Luiz Capitulino wrote:
> >>   I think there's another issue in the handling of strings.
> >>
> >>   The spec says that valid unescaped chars are in the following range:
> >>
> >>      unescaped = %x20-21 / %x23-5B / %x5D-10FFFF
> 
> That's a spec bug IMHO.  Tab is %x09.  Surely you can include tabs in 
> strings.  Any parser that didn't accept that would be broken.

 Honestly, I had the impression this should be encoded as: %x5C %x74, but
if you're right, wouldn't this be true for other sequences as well?

> >>
> >>   But we do:
> >>
> >>      [IN_DQ_STRING] = {
> >>          [1 ... 0xFF] = IN_DQ_STRING,
> >>          ['\\'] = IN_DQ_STRING_ESCAPE,
> >>          ['"'] = IN_DONE_STRING,
> >>      },
> >>
> >>   Shouldn't we cover 0x20 .. 0xFF instead?
> >
> > If it's the lexer, isn't just it being liberal in what it accepts?
> 
> I believe the parser correctly rejects invalid UTF-8 sequences.

 Will check.

[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes

[Anthony Liguori]

On 05/20/2010 11:27 AM, Luiz Capitulino wrote:
> On Thu, 20 May 2010 10:50:41 -0500
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>
>    
>> On 05/20/2010 10:16 AM, Paolo Bonzini wrote:
>>      
>>> On 05/20/2010 03:44 PM, Luiz Capitulino wrote:
>>>        
>>>>    I think there's another issue in the handling of strings.
>>>>
>>>>    The spec says that valid unescaped chars are in the following range:
>>>>
>>>>       unescaped = %x20-21 / %x23-5B / %x5D-10FFFF
>>>>          
>> That's a spec bug IMHO.  Tab is %x09.  Surely you can include tabs in
>> strings.  Any parser that didn't accept that would be broken.
>>      
>   Honestly, I had the impression this should be encoded as: %x5C %x74, but
> if you're right, wouldn't this be true for other sequences as well?
>    

I don't think most reasonable clients are going to quote tabs as '\t'.

Regards,

Anthony Liguori

>>>>    But we do:
>>>>
>>>>       [IN_DQ_STRING] = {
>>>>           [1 ... 0xFF] = IN_DQ_STRING,
>>>>           ['\\'] = IN_DQ_STRING_ESCAPE,
>>>>           ['"'] = IN_DONE_STRING,
>>>>       },
>>>>
>>>>    Shouldn't we cover 0x20 .. 0xFF instead?
>>>>          
>>> If it's the lexer, isn't just it being liberal in what it accepts?
>>>        
>> I believe the parser correctly rejects invalid UTF-8 sequences.
>>      
>   Will check.
>    

[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes

[Luiz Capitulino]

On Thu, 20 May 2010 11:55:00 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 05/20/2010 11:27 AM, Luiz Capitulino wrote:
> > On Thu, 20 May 2010 10:50:41 -0500
> > Anthony Liguori<anthony@codemonkey.ws>  wrote:
> >
> >    
> >> On 05/20/2010 10:16 AM, Paolo Bonzini wrote:
> >>      
> >>> On 05/20/2010 03:44 PM, Luiz Capitulino wrote:
> >>>        
> >>>>    I think there's another issue in the handling of strings.
> >>>>
> >>>>    The spec says that valid unescaped chars are in the following range:
> >>>>
> >>>>       unescaped = %x20-21 / %x23-5B / %x5D-10FFFF
> >>>>          
> >> That's a spec bug IMHO.  Tab is %x09.  Surely you can include tabs in
> >> strings.  Any parser that didn't accept that would be broken.
> >>      
> >   Honestly, I had the impression this should be encoded as: %x5C %x74, but
> > if you're right, wouldn't this be true for other sequences as well?
> >    
> 
> I don't think most reasonable clients are going to quote tabs as '\t'.

 That would be a bug, wouldn't it?

 Python example:

>>> json.dumps('\t')
'"\\t"'
>>>

 YAJL example (inlined below):

/tmp/ ./teste
0x22 0x5c 0x74 0x22
/tmp/ 

 I think we should strictly conform to the spec, quirks should only be
added when really needed.

#include <stdio.h>
#include <yajl/yajl_gen.h>

int main(void)
{
    yajl_gen g;
    unsigned int i, len = 0;
    const unsigned char *str = NULL;
    yajl_gen_config conf = { 0, " " };

    g = yajl_gen_alloc(&conf, NULL);

    if (yajl_gen_string(g, (unsigned char *) "\t", 1) != yajl_gen_status_ok)
        return 1;
    if (yajl_gen_get_buf(g, &str, &len) != yajl_gen_status_ok)
        return 1;

    for (i = 0; i < len; i++)
        printf("0x%x ", str[i]);
    printf("\n");

    return 0;
}

[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes

[Anthony Liguori]

On 05/20/2010 01:47 PM, Luiz Capitulino wrote:
> On Thu, 20 May 2010 11:55:00 -0500
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>
>    
>> On 05/20/2010 11:27 AM, Luiz Capitulino wrote:
>>      
>>> On Thu, 20 May 2010 10:50:41 -0500
>>> Anthony Liguori<anthony@codemonkey.ws>   wrote:
>>>
>>>
>>>        
>>>> On 05/20/2010 10:16 AM, Paolo Bonzini wrote:
>>>>
>>>>          
>>>>> On 05/20/2010 03:44 PM, Luiz Capitulino wrote:
>>>>>
>>>>>            
>>>>>>     I think there's another issue in the handling of strings.
>>>>>>
>>>>>>     The spec says that valid unescaped chars are in the following range:
>>>>>>
>>>>>>        unescaped = %x20-21 / %x23-5B / %x5D-10FFFF
>>>>>>
>>>>>>              
>>>> That's a spec bug IMHO.  Tab is %x09.  Surely you can include tabs in
>>>> strings.  Any parser that didn't accept that would be broken.
>>>>
>>>>          
>>>    Honestly, I had the impression this should be encoded as: %x5C %x74, but
>>> if you're right, wouldn't this be true for other sequences as well?
>>>
>>>        
>> I don't think most reasonable clients are going to quote tabs as '\t'.
>>      
>   That would be a bug, wouldn't it?
>    

Tabs are valid in JavaScript strings and I don't think it's reasonable 
to expect that a valid JavaScript string is not a valid JSON string.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes

[Luiz Capitulino]

On Thu, 20 May 2010 13:52:08 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 05/20/2010 01:47 PM, Luiz Capitulino wrote:
> > On Thu, 20 May 2010 11:55:00 -0500
> > Anthony Liguori<anthony@codemonkey.ws>  wrote:
> >
> >    
> >> On 05/20/2010 11:27 AM, Luiz Capitulino wrote:
> >>      
> >>> On Thu, 20 May 2010 10:50:41 -0500
> >>> Anthony Liguori<anthony@codemonkey.ws>   wrote:
> >>>
> >>>
> >>>        
> >>>> On 05/20/2010 10:16 AM, Paolo Bonzini wrote:
> >>>>
> >>>>          
> >>>>> On 05/20/2010 03:44 PM, Luiz Capitulino wrote:
> >>>>>
> >>>>>            
> >>>>>>     I think there's another issue in the handling of strings.
> >>>>>>
> >>>>>>     The spec says that valid unescaped chars are in the following range:
> >>>>>>
> >>>>>>        unescaped = %x20-21 / %x23-5B / %x5D-10FFFF
> >>>>>>
> >>>>>>              
> >>>> That's a spec bug IMHO.  Tab is %x09.  Surely you can include tabs in
> >>>> strings.  Any parser that didn't accept that would be broken.
> >>>>
> >>>>          
> >>>    Honestly, I had the impression this should be encoded as: %x5C %x74, but
> >>> if you're right, wouldn't this be true for other sequences as well?
> >>>
> >>>        
> >> I don't think most reasonable clients are going to quote tabs as '\t'.
> >>      
> >   That would be a bug, wouldn't it?
> >    
> 
> Tabs are valid in JavaScript strings and I don't think it's reasonable 
> to expect that a valid JavaScript string is not a valid JSON string.

 IMO, we should do what the spec says and what bug free clients expect,
what we consider reasonable or unreasonable is a different matter.

 I would be with you if the spec was proved wrong, specially if reference
implementations out there didn't follow it either, but everything I found
so far shows this is not the case.

 Another example:

   http://www.json.org/json2.js

 Search for 'character substitutions'.

Re: [Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes

[Anthony Liguori]

On 05/20/2010 02:22 PM, Luiz Capitulino wrote:
> On Thu, 20 May 2010 13:52:08 -0500
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>
>    
>> On 05/20/2010 01:47 PM, Luiz Capitulino wrote:
>>      
>>> On Thu, 20 May 2010 11:55:00 -0500
>>> Anthony Liguori<anthony@codemonkey.ws>   wrote:
>>>
>>>
>>>        
>>>> On 05/20/2010 11:27 AM, Luiz Capitulino wrote:
>>>>
>>>>          
>>>>> On Thu, 20 May 2010 10:50:41 -0500
>>>>> Anthony Liguori<anthony@codemonkey.ws>    wrote:
>>>>>
>>>>>
>>>>>
>>>>>            
>>>>>> On 05/20/2010 10:16 AM, Paolo Bonzini wrote:
>>>>>>
>>>>>>
>>>>>>              
>>>>>>> On 05/20/2010 03:44 PM, Luiz Capitulino wrote:
>>>>>>>
>>>>>>>
>>>>>>>                
>>>>>>>>      I think there's another issue in the handling of strings.
>>>>>>>>
>>>>>>>>      The spec says that valid unescaped chars are in the following range:
>>>>>>>>
>>>>>>>>         unescaped = %x20-21 / %x23-5B / %x5D-10FFFF
>>>>>>>>
>>>>>>>>
>>>>>>>>                  
>>>>>> That's a spec bug IMHO.  Tab is %x09.  Surely you can include tabs in
>>>>>> strings.  Any parser that didn't accept that would be broken.
>>>>>>
>>>>>>
>>>>>>              
>>>>>     Honestly, I had the impression this should be encoded as: %x5C %x74, but
>>>>> if you're right, wouldn't this be true for other sequences as well?
>>>>>
>>>>>
>>>>>            
>>>> I don't think most reasonable clients are going to quote tabs as '\t'.
>>>>
>>>>          
>>>    That would be a bug, wouldn't it?
>>>
>>>        
>> Tabs are valid in JavaScript strings and I don't think it's reasonable
>> to expect that a valid JavaScript string is not a valid JSON string.
>>      
>   IMO, we should do what the spec says and what bug free clients expect,
> what we consider reasonable or unreasonable is a different matter.
>    

How we encode strings is one thing, what we accept is something else.

Why shouldn't we be liberal in what we accept?  It doesn't violate the 
spec to accept more than it requires so why shouldn't we?

Regards,

Anthony Liguori

>   I would be with you if the spec was proved wrong, specially if reference
> implementations out there didn't follow it either, but everything I found
> so far shows this is not the case.
>
>   Another example:
>
>     http://www.json.org/json2.js
>
>   Search for 'character substitutions'.
>
>
>    

Re: [Qemu-devel] Re: [PATCH 2/6] json-lexer: Handle missing escapes

[Luiz Capitulino]

On Mon, 24 May 2010 14:29:58 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 05/20/2010 02:22 PM, Luiz Capitulino wrote:
> > On Thu, 20 May 2010 13:52:08 -0500
> > Anthony Liguori<anthony@codemonkey.ws>  wrote:
> >
> >    
> >> On 05/20/2010 01:47 PM, Luiz Capitulino wrote:
> >>      
> >>> On Thu, 20 May 2010 11:55:00 -0500
> >>> Anthony Liguori<anthony@codemonkey.ws>   wrote:
> >>>
> >>>
> >>>        
> >>>> On 05/20/2010 11:27 AM, Luiz Capitulino wrote:
> >>>>
> >>>>          
> >>>>> On Thu, 20 May 2010 10:50:41 -0500
> >>>>> Anthony Liguori<anthony@codemonkey.ws>    wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>>            
> >>>>>> On 05/20/2010 10:16 AM, Paolo Bonzini wrote:
> >>>>>>
> >>>>>>
> >>>>>>              
> >>>>>>> On 05/20/2010 03:44 PM, Luiz Capitulino wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>                
> >>>>>>>>      I think there's another issue in the handling of strings.
> >>>>>>>>
> >>>>>>>>      The spec says that valid unescaped chars are in the following range:
> >>>>>>>>
> >>>>>>>>         unescaped = %x20-21 / %x23-5B / %x5D-10FFFF
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>                  
> >>>>>> That's a spec bug IMHO.  Tab is %x09.  Surely you can include tabs in
> >>>>>> strings.  Any parser that didn't accept that would be broken.
> >>>>>>
> >>>>>>
> >>>>>>              
> >>>>>     Honestly, I had the impression this should be encoded as: %x5C %x74, but
> >>>>> if you're right, wouldn't this be true for other sequences as well?
> >>>>>
> >>>>>
> >>>>>            
> >>>> I don't think most reasonable clients are going to quote tabs as '\t'.
> >>>>
> >>>>          
> >>>    That would be a bug, wouldn't it?
> >>>
> >>>        
> >> Tabs are valid in JavaScript strings and I don't think it's reasonable
> >> to expect that a valid JavaScript string is not a valid JSON string.
> >>      
> >   IMO, we should do what the spec says and what bug free clients expect,
> > what we consider reasonable or unreasonable is a different matter.
> >    
> 
> How we encode strings is one thing, what we accept is something else.

 True.

> Why shouldn't we be liberal in what we accept?  It doesn't violate the 
> spec to accept more than it requires so why shouldn't we?

 For the reasons outlined by Avi, not sure how this serious this is though.

[Qemu-devel] [PATCH 3/6] qjson: Handle "\f"

[Luiz Capitulino]

It's valid JSON and should be handled.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 json-parser.c |    4 ++++
 qjson.c       |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/json-parser.c b/json-parser.c
index b55d763..83212bc 100644
--- a/json-parser.c
+++ b/json-parser.c
@@ -206,6 +206,10 @@ static QString *qstring_from_escaped_str(JSONParserContext *ctxt, QObject *token
                 qstring_append(str, "\b");
                 ptr++;
                 break;
+            case 'f':
+                qstring_append(str, "\f");
+                ptr++;
+                break;
             case 'n':
                 qstring_append(str, "\n");
                 ptr++;
diff --git a/qjson.c b/qjson.c
index 483c667..e4ee433 100644
--- a/qjson.c
+++ b/qjson.c
@@ -158,6 +158,9 @@ static void to_json(const QObject *obj, QString *str)
                 case '\b':
                     qstring_append(str, "\\b");
                     break;
+                case '\f':
+                    qstring_append(str, "\\f");
+                    break;
                 case '\n':
                     qstring_append(str, "\\n");
                     break;
-- 
1.7.1.86.g0e460

[Qemu-devel] [PATCH 4/6] check-qjson: Add more escape tests

[Luiz Capitulino]

While there make the fail_unless() calls print error messages.

IMPORTANT: The test for "\/" is failing, don't know why.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 check-qjson.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/check-qjson.c b/check-qjson.c
index 109e777..d365799 100644
--- a/check-qjson.c
+++ b/check-qjson.c
@@ -29,6 +29,13 @@ START_TEST(escaped_string)
         const char *decoded;
         int skip;
     } test_cases[] = {
+        { "\"\\b\"", "\b" },
+        { "\"\\f\"", "\f" },
+        { "\"\\n\"", "\n" },
+        { "\"\\r\"", "\r" },
+        { "\"\\t\"", "\t" },
+        { "\"\\/\"", "\\/" },
+        { "\"\\\\\"", "\\" },
         { "\"\\\"\"", "\"" },
         { "\"hello world \\\"embedded string\\\"\"",
           "hello world \"embedded string\"" },
@@ -49,11 +56,14 @@ START_TEST(escaped_string)
         fail_unless(qobject_type(obj) == QTYPE_QSTRING);
         
         str = qobject_to_qstring(obj);
-        fail_unless(strcmp(qstring_get_str(str), test_cases[i].decoded) == 0);
+        fail_unless(strcmp(qstring_get_str(str), test_cases[i].decoded) == 0,
+                    "%s != %s\n", qstring_get_str(str), test_cases[i].decoded);
 
         if (test_cases[i].skip == 0) {
             str = qobject_to_json(obj);
-            fail_unless(strcmp(qstring_get_str(str), test_cases[i].encoded) == 0);
+            fail_unless(strcmp(qstring_get_str(str),test_cases[i].encoded) == 0,
+                        "%s != %s\n", qstring_get_str(str),
+                                      test_cases[i].encoded);
 
             qobject_decref(obj);
         }
-- 
1.7.1.86.g0e460

[Qemu-devel] [PATCH 5/6] json-lexer: Drop 'buf'

[Luiz Capitulino]

QString supports adding a single char, 'buf' is unneeded.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 json-lexer.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/json-lexer.c b/json-lexer.c
index 5cc7e6c..1d9b81f 100644
--- a/json-lexer.c
+++ b/json-lexer.c
@@ -284,8 +284,6 @@ void json_lexer_init(JSONLexer *lexer, JSONLexerEmitter func)
 
 static int json_lexer_feed_char(JSONLexer *lexer, char ch)
 {
-    char buf[2];
-
     lexer->x++;
     if (ch == '\n') {
         lexer->x = 0;
@@ -313,10 +311,7 @@ static int json_lexer_feed_char(JSONLexer *lexer, char ch)
         break;
     }
 
-    buf[0] = ch;
-    buf[1] = 0;
-
-    qstring_append(lexer->token, buf);
+    qstring_append_chr(lexer->token, ch);
 
     return 0;
 }
-- 
1.7.1.86.g0e460

[Qemu-devel] [PATCH 6/6] json-streamer: Don't use qdict_put_obj()

[Luiz Capitulino]

It's not needed, use qobject_put() instead and get a cleaner code.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 json-streamer.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/json-streamer.c b/json-streamer.c
index 610ffea..f7e7a68 100644
--- a/json-streamer.c
+++ b/json-streamer.c
@@ -43,11 +43,11 @@ static void json_message_process_token(JSONLexer *lexer, QString *token, JSONTok
     }
 
     dict = qdict_new();
-    qdict_put_obj(dict, "type", QOBJECT(qint_from_int(type)));
+    qdict_put(dict, "type", qint_from_int(type));
     QINCREF(token);
-    qdict_put_obj(dict, "token", QOBJECT(token));
-    qdict_put_obj(dict, "x", QOBJECT(qint_from_int(x)));
-    qdict_put_obj(dict, "y", QOBJECT(qint_from_int(y)));
+    qdict_put(dict, "token", token);
+    qdict_put(dict, "x", qint_from_int(x));
+    qdict_put(dict, "y", qint_from_int(y));
 
     qlist_append(parser->tokens, dict);
 
-- 
1.7.1.86.g0e460

Re: [Qemu-devel] [PATCH 0/6]: QMP: Fix issues in parser/lexer

[Anthony Liguori]

On 05/19/2010 04:15 PM, Luiz Capitulino wrote:
>   Hi Anthony,
>
>   While investigating a QMP bug reported by a user, I've found a few issues
> in our parser/lexer.
>
>   The patches in this series fix the problems I was able to solve, but we
> still have the following issues:
>
> 1. Our 'private extension' is open to the public
>
>     Eg. The following input issued by a client is valid:
>
>     { 'execute': 'query-pci' }
>
>     I don't think it's a good idea to have clients relying on this kind of
>     JSON extension.
>
>     To fix this we could add a 'extension' flag to JSONLexer and set it to
>     nonzero in internal functions (eg. qobject_from_jsonf()), of course that
>     the lexer code should handle this too.
>    

The JSON specification explicitly says:

"A JSON parser transforms a JSON text into another representation. A 
JSON parser MUST accept all texts that conform to the JSON grammar.  A 
JSON parser MAY accept non-JSON forms or extensions."

IOW, we're under no obligation to reject extensions and I can't think of 
a reason why we should.

> 2. QMP doesn't check the return of json_message_parser_feed()
>
>     Which means we don't handle JSON syntax errors. While the fix might seem
>     trivial (ie. just return an error!), I'm not sure what's the best way
>     to handle this, because the streamer seems to return multiple errors for
>     the same input string.
>
>     For example, this input:
>
>     { "execute": yy_uu }
>
>     Seems to return an error for each bad character (yy_uu), shouldn't it
>     return only once and stop processing the whole string?
>    

It probably should kill the connection.

> 3. The lexer enter in ERROR state when processing is done
>
>     Not sure whether this is an issue, but I found it while reviewing the code
>     and maybe this is related with item 2 above.
>
>     When json_lexer_feed_char() is finished scanning a string, (ie. ch='\0')
>     the JSON_SKIP clause will set lexer->state to ERROR as there's no entry
>     for '\0' in the IN_START array.
>
>     Shouldn't we have a LEXER_DONE or something like it instead?
>    

No, you must have malformed input if an error occurs.

[IN_WHITESPACE] -> TERMINAL(JSON_SKIP)

JSON_SKIP is a terminal so once you're in that state, you go back to 
IN_START.

> 4. Lexer expects a 'terminal' char to process a token
>
>     Which means clients must send a sort of end of line char, so that we
>     process their input.
>
>     Maybe I'm missing something here, but I thought that the whole point of
>     writing our own parser was to avoid this.
>    

If the lexer gets:

"abc"

It has no way of knowing if that's a token or if we're going to get:

"abcd"

As a token.  You can fix this in two ways.  You can either flush() the 
lexer to significant end of input or you can wait until there's some 
other valid symbol to cause the previous symbol to be emitted.

IOW, a client either needs to: 1) send the request and follow it with a 
newline or some form of whitespace or 2) close the connection to flush 
the request

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 0/6]: QMP: Fix issues in parser/lexer

[Luiz Capitulino]

On Wed, 19 May 2010 16:43:08 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 05/19/2010 04:15 PM, Luiz Capitulino wrote:
> >   Hi Anthony,
> >
> >   While investigating a QMP bug reported by a user, I've found a few issues
> > in our parser/lexer.
> >
> >   The patches in this series fix the problems I was able to solve, but we
> > still have the following issues:
> >
> > 1. Our 'private extension' is open to the public
> >
> >     Eg. The following input issued by a client is valid:
> >
> >     { 'execute': 'query-pci' }
> >
> >     I don't think it's a good idea to have clients relying on this kind of
> >     JSON extension.
> >
> >     To fix this we could add a 'extension' flag to JSONLexer and set it to
> >     nonzero in internal functions (eg. qobject_from_jsonf()), of course that
> >     the lexer code should handle this too.
> >    
> 
> The JSON specification explicitly says:
> 
> "A JSON parser transforms a JSON text into another representation. A 
> JSON parser MUST accept all texts that conform to the JSON grammar.  A 
> JSON parser MAY accept non-JSON forms or extensions."
> 
> IOW, we're under no obligation to reject extensions and I can't think of 
> a reason why we should.

 I know we're legal, but what's the point to offer this extension to clients?

 The main motivation behind this was to write JSON in C strings w/o the
need of repetitive escapes. This is internal to QEMU, but it's also
available to clients for no reason.

 And you know, after 0.13 we won't be able to remove it.

> > 2. QMP doesn't check the return of json_message_parser_feed()
> >
> >     Which means we don't handle JSON syntax errors. While the fix might seem
> >     trivial (ie. just return an error!), I'm not sure what's the best way
> >     to handle this, because the streamer seems to return multiple errors for
> >     the same input string.
> >
> >     For example, this input:
> >
> >     { "execute": yy_uu }
> >
> >     Seems to return an error for each bad character (yy_uu), shouldn't it
> >     return only once and stop processing the whole string?
> >    
> 
> It probably should kill the connection.

 Ok.

> > 3. The lexer enter in ERROR state when processing is done
> >
> >     Not sure whether this is an issue, but I found it while reviewing the code
> >     and maybe this is related with item 2 above.
> >
> >     When json_lexer_feed_char() is finished scanning a string, (ie. ch='\0')
> >     the JSON_SKIP clause will set lexer->state to ERROR as there's no entry
> >     for '\0' in the IN_START array.
> >
> >     Shouldn't we have a LEXER_DONE or something like it instead?
> >    
> 
> No, you must have malformed input if an error occurs.

 Yes, json_message_parser_feed() returns OK.

> [IN_WHITESPACE] -> TERMINAL(JSON_SKIP)
> 
> JSON_SKIP is a terminal so once you're in that state, you go back to 
> IN_START.

 Yes, but what I'm trying to say is that when ch='\0' and you do:

     lexer->state = json_lexer[IN_START][(uint8_t)ch];

 Then 'lexer->state' becomes 0, which is what the code recognizes as ERROR.

 Again, not sure if this is an issue. Just caught my attention.

> > 4. Lexer expects a 'terminal' char to process a token
> >
> >     Which means clients must send a sort of end of line char, so that we
> >     process their input.
> >
> >     Maybe I'm missing something here, but I thought that the whole point of
> >     writing our own parser was to avoid this.
> >    
> 
> If the lexer gets:
> 
> "abc"
> 
> It has no way of knowing if that's a token or if we're going to get:
> 
> "abcd"
> 
> As a token.  You can fix this in two ways.  You can either flush() the 
> lexer to significant end of input or you can wait until there's some 
> other valid symbol to cause the previous symbol to be emitted.
> 
> IOW, a client either needs to: 1) send the request and follow it with a 
> newline or some form of whitespace or 2) close the connection to flush 
> the request

 Ok.

Re: [Qemu-devel] [PATCH 0/6]: QMP: Fix issues in parser/lexer

[Luiz Capitulino]

On Thu, 20 May 2010 10:35:52 -0300
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> > > 2. QMP doesn't check the return of json_message_parser_feed()
> > >
> > >     Which means we don't handle JSON syntax errors. While the fix might seem
> > >     trivial (ie. just return an error!), I'm not sure what's the best way
> > >     to handle this, because the streamer seems to return multiple errors for
> > >     the same input string.
> > >
> > >     For example, this input:
> > >
> > >     { "execute": yy_uu }
> > >
> > >     Seems to return an error for each bad character (yy_uu), shouldn't it
> > >     return only once and stop processing the whole string?
> > >    
> > 
> > It probably should kill the connection.
> 
>  Ok.

 I was working on this and it's not that simple.

 As we talked by IRC, there isn't an easy way to just drop a qemu-char connection
as a char device doesn't have a notion of connect/disconnect (as you explained).

 So, another way of doing this would be to close the chardev and open it again,
however this is also complicated for two reasons:

1. qemu_chr_close() completely destroys the chardev and creating it again requires
   access to the original QemuOpts, created at boot time

2. Some char devices' open() method have specific boot-time only options
   (eg. socket & wait)

 It's possible to hack, we could make a copy of the QemOpts, remove boot-time
options and store it in CharDriverState. But this is ugly and fragile.

 Isn't it easier to handle this in the lexer itself? For example, couldn't it
just mark the token as bad and keep scanning?

 We handle parser's errors just fine.

[Qemu-devel] Re: [PATCH 0/6]: QMP: Fix issues in parser/lexer

[Paolo Bonzini]

On 05/19/2010 11:43 PM, Anthony Liguori wrote:
>
>> 4. Lexer expects a 'terminal' char to process a token
>>
>>     Which means clients must send a sort of end of line char, so that we
>>     process their input.
>>
>>     Maybe I'm missing something here, but I thought that the whole
>>     point of writing our own parser was to avoid this.
>
> If the lexer gets:
>
> "abc"
>
> It has no way of knowing if that's a token or if we're going to get:
>
> "abcd"

Only } and ] are valid characters at the end of a JSON object, and 
neither requires lookahead.

Paolo

[Qemu-devel] Re: [PATCH 0/6]: QMP: Fix issues in parser/lexer

[Luiz Capitulino]

On Thu, 20 May 2010 17:18:23 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 05/19/2010 11:43 PM, Anthony Liguori wrote:
> >
> >> 4. Lexer expects a 'terminal' char to process a token
> >>
> >>     Which means clients must send a sort of end of line char, so that we
> >>     process their input.
> >>
> >>     Maybe I'm missing something here, but I thought that the whole
> >>     point of writing our own parser was to avoid this.
> >
> > If the lexer gets:
> >
> > "abc"
> >
> > It has no way of knowing if that's a token or if we're going to get:
> >
> > "abcd"
> 
> Only } and ] are valid characters at the end of a JSON object, and 
> neither requires lookahead.

 Good point.

[Qemu-devel] Re: [PATCH 0/6]: QMP: Fix issues in parser/lexer

[Anthony Liguori]

On 05/20/2010 10:18 AM, Paolo Bonzini wrote:
> On 05/19/2010 11:43 PM, Anthony Liguori wrote:
>>
>>> 4. Lexer expects a 'terminal' char to process a token
>>>
>>>     Which means clients must send a sort of end of line char, so 
>>> that we
>>>     process their input.
>>>
>>>     Maybe I'm missing something here, but I thought that the whole
>>>     point of writing our own parser was to avoid this.
>>
>> If the lexer gets:
>>
>> "abc"
>>
>> It has no way of knowing if that's a token or if we're going to get:
>>
>> "abcd"
>
> Only } and ] are valid characters at the end of a JSON object, and 
> neither requires lookahead.

Having look ahead operate differently for different states really 
complicates the lexer.  I don't see this as a big problem in practice.

Regards,

Anthony Liguori

> Paolo

[Qemu-devel] Re: [PATCH 0/6]: QMP: Fix issues in parser/lexer

[Luiz Capitulino]

On Thu, 20 May 2010 10:52:58 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> On 05/20/2010 10:18 AM, Paolo Bonzini wrote:
> > On 05/19/2010 11:43 PM, Anthony Liguori wrote:
> >>
> >>> 4. Lexer expects a 'terminal' char to process a token
> >>>
> >>>     Which means clients must send a sort of end of line char, so 
> >>> that we
> >>>     process their input.
> >>>
> >>>     Maybe I'm missing something here, but I thought that the whole
> >>>     point of writing our own parser was to avoid this.
> >>
> >> If the lexer gets:
> >>
> >> "abc"
> >>
> >> It has no way of knowing if that's a token or if we're going to get:
> >>
> >> "abcd"
> >
> > Only } and ] are valid characters at the end of a JSON object, and 
> > neither requires lookahead.
> 
> Having look ahead operate differently for different states really 
> complicates the lexer.  I don't see this as a big problem in practice.

 Would be a nice feature, but it's fine for me too and we'll have to
note that in the QMP's spec.

[Qemu-devel] [PATCH] do not require lookahead in json-lexer.c if not necessary

[Paolo Bonzini]

> Having look ahead operate differently for different states
> really complicates the lexer.  I don't see this as a big
> problem in practice. 

I also don't see this as a big problem in practice, except maybe
for a quit command.  However, WRT the complication of the lexer,
it is really not so bad.  The trick is to split

   lexer->state = json_lexer[IN_START][(uint8_t)ch];

in two phases, namely transitioning to IN_START and (only if
lookahead was used) doing another iteration of the loop.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        The patch is on top of Luiz's changes.

 json-lexer.c |   85 +++++++++++++++++++++++++++++----------------------------
 1 files changed, 43 insertions(+), 42 deletions(-)

diff --git a/json-lexer.c b/json-lexer.c
index d1d8033..b9250c1 100644
--- a/json-lexer.c
+++ b/json-lexer.c
@@ -29,7 +29,6 @@
 
 enum json_lexer_state {
     ERROR = 0,
-    IN_DONE_STRING,
     IN_DQ_UCODE3,
     IN_DQ_UCODE2,
     IN_DQ_UCODE1,
@@ -59,17 +58,18 @@ enum json_lexer_state {
     IN_ESCAPE_I64,
     IN_ESCAPE_DONE,
     IN_WHITESPACE,
-    IN_OPERATOR_DONE,
     IN_START,
 };
 
 #define TERMINAL(state) [0 ... 0x7F] = (state)
 
-static const uint8_t json_lexer[][256] =  {
-    [IN_DONE_STRING] = {
-        TERMINAL(JSON_STRING),
-    },
+/* Return whether TERMINAL is a terminal state and the transition to it
+   from OLD_STATE required lookahead.  This can happens whenever the table
+   below uses the TERMINAL macro.  */
+#define TERMINAL_NEEDED_LOOKAHEAD(old_state, terminal) \
+            (json_lexer[(old_state)][0] == (terminal))
 
+static const uint8_t json_lexer[][256] =  {
     /* double quote string */
     [IN_DQ_UCODE3] = {
         ['0' ... '9'] = IN_DQ_STRING,
@@ -104,7 +104,7 @@ static const uint8_t json_lexer[][256] =  {
     [IN_DQ_STRING] = {
         [1 ... 0xFF] = IN_DQ_STRING,
         ['\\'] = IN_DQ_STRING_ESCAPE,
-        ['"'] = IN_DONE_STRING,
+        ['"'] = JSON_STRING,
     },
 
     /* single quote string */
@@ -141,7 +141,7 @@ static const uint8_t json_lexer[][256] =  {
     [IN_SQ_STRING] = {
         [1 ... 0xFF] = IN_SQ_STRING,
         ['\\'] = IN_SQ_STRING_ESCAPE,
-        ['\''] = IN_DONE_STRING,
+        ['\''] = JSON_STRING,
     },
 
     /* Zero */
@@ -207,11 +207,6 @@ static const uint8_t json_lexer[][256] =  {
         ['\n'] = IN_WHITESPACE,
     },        
 
-    /* operator */
-    [IN_OPERATOR_DONE] = {
-        TERMINAL(JSON_OPERATOR),
-    },
-
     /* escape */
     [IN_ESCAPE_DONE] = {
         TERMINAL(JSON_ESCAPE),
@@ -255,12 +250,12 @@ static const uint8_t json_lexer[][256] =  {
         ['0'] = IN_ZERO,
         ['1' ... '9'] = IN_NONZERO_NUMBER,
         ['-'] = IN_NEG_NONZERO_NUMBER,
-        ['{'] = IN_OPERATOR_DONE,
-        ['}'] = IN_OPERATOR_DONE,
-        ['['] = IN_OPERATOR_DONE,
-        [']'] = IN_OPERATOR_DONE,
-        [','] = IN_OPERATOR_DONE,
-        [':'] = IN_OPERATOR_DONE,
+        ['{'] = JSON_OPERATOR,
+        ['}'] = JSON_OPERATOR,
+        ['['] = JSON_OPERATOR,
+        [']'] = JSON_OPERATOR,
+        [','] = JSON_OPERATOR,
+        [':'] = JSON_OPERATOR,
         ['a' ... 'z'] = IN_KEYWORD,
         ['%'] = IN_ESCAPE,
         [' '] = IN_WHITESPACE,
@@ -279,35 +274,41 @@ void json_lexer_init(JSONLexer *lexer, JSONLexerEmitter func)
 
 static int json_lexer_feed_char(JSONLexer *lexer, char ch)
 {
+    int char_consumed, new_state;
+
     lexer->x++;
     if (ch == '\n') {
         lexer->x = 0;
         lexer->y++;
     }
 
-    lexer->state = json_lexer[lexer->state][(uint8_t)ch];
-
-    switch (lexer->state) {
-    case JSON_OPERATOR:
-    case JSON_ESCAPE:
-    case JSON_INTEGER:
-    case JSON_FLOAT:
-    case JSON_KEYWORD:
-    case JSON_STRING:
-        lexer->emit(lexer, lexer->token, lexer->state, lexer->x, lexer->y);
-    case JSON_SKIP:
-        lexer->state = json_lexer[IN_START][(uint8_t)ch];
-        QDECREF(lexer->token);
-        lexer->token = qstring_new();
-        break;
-    case ERROR:
-        return -EINVAL;
-    default:
-        break;
-    }
-
-    qstring_append_chr(lexer->token, ch);
+    do {
+        new_state = json_lexer[lexer->state][(uint8_t)ch];
+        char_consumed = !TERMINAL_NEEDED_LOOKAHEAD(lexer->state, new_state);
+        if (char_consumed) {
+            qstring_append_chr(lexer->token, ch);
+        }
 
+        switch (new_state) {
+        case JSON_OPERATOR:
+        case JSON_ESCAPE:
+        case JSON_INTEGER:
+        case JSON_FLOAT:
+        case JSON_KEYWORD:
+        case JSON_STRING:
+            lexer->emit(lexer, lexer->token, new_state, lexer->x, lexer->y);
+        case JSON_SKIP:
+            QDECREF(lexer->token);
+            lexer->token = qstring_new();
+            new_state = IN_START;
+            break;
+        case ERROR:
+            return -EINVAL;
+        default:
+            break;
+        }
+        lexer->state = new_state;
+    } while (!char_consumed);
     return 0;
 }
 
@@ -329,7 +330,7 @@ int json_lexer_feed(JSONLexer *lexer, const char *buffer, size_t size)
 
 int json_lexer_flush(JSONLexer *lexer)
 {
-    return json_lexer_feed_char(lexer, 0);
+    return lexer->state == IN_START ? 0 : json_lexer_feed_char(lexer, 0);
 }
 
 void json_lexer_destroy(JSONLexer *lexer)
-- 
1.6.6.1

[Qemu-devel] [PATCH] do not require lookahead for escapes too

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 json-lexer.c |   21 ++++++++-------------
 roms/seabios |    2 +-
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/json-lexer.c b/json-lexer.c
index b9250c1..bc9dfae 100644
--- a/json-lexer.c
+++ b/json-lexer.c
@@ -56,7 +56,6 @@ enum json_lexer_state {
     IN_ESCAPE_I,
     IN_ESCAPE_I6,
     IN_ESCAPE_I64,
-    IN_ESCAPE_DONE,
     IN_WHITESPACE,
     IN_START,
 };
@@ -208,21 +207,17 @@ static const uint8_t json_lexer[][256] =  {
     },        
 
     /* escape */
-    [IN_ESCAPE_DONE] = {
-        TERMINAL(JSON_ESCAPE),
-    },
-
     [IN_ESCAPE_LL] = {
-        ['d'] = IN_ESCAPE_DONE,
+        ['d'] = JSON_ESCAPE,
     },
 
     [IN_ESCAPE_L] = {
-        ['d'] = IN_ESCAPE_DONE,
+        ['d'] = JSON_ESCAPE,
         ['l'] = IN_ESCAPE_LL,
     },
 
     [IN_ESCAPE_I64] = {
-        ['d'] = IN_ESCAPE_DONE,
+        ['d'] = JSON_ESCAPE,
     },
 
     [IN_ESCAPE_I6] = {
@@ -234,11 +229,11 @@ static const uint8_t json_lexer[][256] =  {
     },
 
     [IN_ESCAPE] = {
-        ['d'] = IN_ESCAPE_DONE,
-        ['i'] = IN_ESCAPE_DONE,
-        ['p'] = IN_ESCAPE_DONE,
-        ['s'] = IN_ESCAPE_DONE,
-        ['f'] = IN_ESCAPE_DONE,
+        ['d'] = JSON_ESCAPE,
+        ['i'] = JSON_ESCAPE,
+        ['p'] = JSON_ESCAPE,
+        ['s'] = JSON_ESCAPE,
+        ['f'] = JSON_ESCAPE,
         ['l'] = IN_ESCAPE_L,
         ['I'] = IN_ESCAPE_I,
     },
diff --git a/roms/seabios b/roms/seabios
index 7d09d0e..8f469b9 160000
--- a/roms/seabios
+++ b/roms/seabios
@@ -1 +1 @@
-Subproject commit 7d09d0e3ba11310e973d4302c7fcc3fc2184e04c
+Subproject commit 8f469b9676127ba6bb52609d89ec774e61db0ee1
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH] do not require lookahead for escapes too

[Paolo Bonzini]

On 05/21/2010 12:10 PM, Paolo Bonzini wrote:
> diff --git a/roms/seabios b/roms/seabios
> index 7d09d0e..8f469b9 160000
> --- a/roms/seabios
> +++ b/roms/seabios
> @@ -1 +1 @@
> -Subproject commit 7d09d0e3ba11310e973d4302c7fcc3fc2184e04c
> +Subproject commit 8f469b9676127ba6bb52609d89ec774e61db0ee1

This was obviously not intended.

I'll send a new version tomorrow.

Paolo

Re: [Qemu-devel] [PATCH 0/6]: QMP: Fix issues in parser/lexer

[Avi Kivity]

On 05/20/2010 12:43 AM, Anthony Liguori wrote:
>
> The JSON specification explicitly says:
>
> "A JSON parser transforms a JSON text into another representation. A 
> JSON parser MUST accept all texts that conform to the JSON grammar.  A 
> JSON parser MAY accept non-JSON forms or extensions."
>
> IOW, we're under no obligation to reject extensions and I can't think 
> of a reason why we should.

At the very least, we should document them.  If the extension doesn't 
add any value but is merely a side effect of the implementation, we 
should remove it.

Examples where this could hurt us:

- we move to a json parsing library, the extension disappears, client breaks
- someone writes a qemu simulator to test managment tool scalability 
(run zillions of fake guests on one machine), client breaks
- someone writes a debug tool that interposes between client and qemu, 
client breaks
- the json specification adds a new form that conflicts with one of our 
extensions [1], we can't use the new form

Being strict in what we accept will reduce our support burden later on.

[1] allowing infinite extensibility like this is irresponsible

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.