qobject: json-streamer: Remove double test

[PATCH] qobject: json-streamer: Remove double test

Posted by Simran Singhal 5 years, 7 months ago

Remove the duplicate test "parser->bracket_count >= 0".

Signed-off-by: Simran Singhal <singhalsimran0@gmail.com>
---
 qobject/json-streamer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
index 47dd7ea576..ef48185283 100644
--- a/qobject/json-streamer.c
+++ b/qobject/json-streamer.c
@@ -85,7 +85,7 @@ void json_message_process_token(JSONLexer *lexer, GString *input,
     g_queue_push_tail(&parser->tokens, token);
 
     if ((parser->brace_count > 0 || parser->bracket_count > 0)
-        && parser->bracket_count >= 0 && parser->bracket_count >= 0) {
+        && parser->bracket_count >= 0) {
         return;
     }
 
-- 
2.17.1

Re: [PATCH] qobject: json-streamer: Remove double test

Posted by Eric Blake 5 years, 7 months ago

On 4/2/20 7:13 AM, Simran Singhal wrote:
> Remove the duplicate test "parser->bracket_count >= 0".
> 
> Signed-off-by: Simran Singhal <singhalsimran0@gmail.com>
> ---
>   qobject/json-streamer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
> index 47dd7ea576..ef48185283 100644
> --- a/qobject/json-streamer.c
> +++ b/qobject/json-streamer.c
> @@ -85,7 +85,7 @@ void json_message_process_token(JSONLexer *lexer, GString *input,
>       g_queue_push_tail(&parser->tokens, token);
>   

Adding some context:

>       if ((parser->brace_count > 0 || parser->bracket_count > 0)
> -        && parser->bracket_count >= 0 && parser->bracket_count >= 0) {
> +        && parser->bracket_count >= 0) {
>           return;
>       }
>   
>     json = json_parser_parse(parser->tokens, parser->ap, &err);
>     parser->tokens = NULL;
> 
> out_emit:

This code was rewritten in commit 8d3265b3.  Prior to that, it read:


     if (parser->brace_count < 0 ||
         parser->bracket_count < 0 ||
         (parser->brace_count == 0 &&
          parser->bracket_count == 0)) {
         json = json_parser_parse(parser->tokens, parser->ap, &err);
         parser->tokens = NULL;
         goto out_emit;
     }

     return;

out_emit:

Obviously, the goal of the rewrite was to convert:

if (cond) {
   do stuff
} else {
   return
}
more stuff

into the more legible

if (!cond) {
   return
}
do stuff
more stuff

Let's re-read my original review:

https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03017.html

> Applying deMorgan's rules:
> 
> !(brace < 0 || bracket < 0 || (brace == 0 && bracket == 0))
> !(brace < 0) && !(bracket < 0) && !(brace == 0 && bracket == 0)
> brace >= 0 && bracket >= 0 && (!(brace == 0) || !(bracket == 0))
> brace >= 0 && bracket >= 0 && (brace != 0 || bracket != 0)
> 
> But based on what we learned in the first two conjunctions, we can rewrite the third:
> 
> 
> brace >= 0 && bracket >= 0 && (brace > 0 || bracket > 0)
> 
> and then commute the logic:
> 
> (brace > 0 || bracket > 0) && brace >= 0 && bracket >= 0
> 

What I missed was the typo: we checked bracket >= 0 twice, instead of 
the intended brace >= 0 && bracket >= 0.  This needs a v2.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] qobject: json-streamer: Remove double test

Posted by Simran Singhal 5 years, 7 months ago

On Thu, Apr 2, 2020 at 6:42 PM Eric Blake <eblake@redhat.com> wrote:

> On 4/2/20 7:13 AM, Simran Singhal wrote:
> > Remove the duplicate test "parser->bracket_count >= 0".
> >
> > Signed-off-by: Simran Singhal <singhalsimran0@gmail.com>
> > ---
> >   qobject/json-streamer.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
> > index 47dd7ea576..ef48185283 100644
> > --- a/qobject/json-streamer.c
> > +++ b/qobject/json-streamer.c
> > @@ -85,7 +85,7 @@ void json_message_process_token(JSONLexer *lexer,
> GString *input,
> >       g_queue_push_tail(&parser->tokens, token);
> >
>
> Adding some context:
>
> >       if ((parser->brace_count > 0 || parser->bracket_count > 0)
> > -        && parser->bracket_count >= 0 && parser->bracket_count >= 0) {
> > +        && parser->bracket_count >= 0) {
> >           return;
> >       }
> >
> >     json = json_parser_parse(parser->tokens, parser->ap, &err);
> >     parser->tokens = NULL;
> >
> > out_emit:
>
> This code was rewritten in commit 8d3265b3.  Prior to that, it read:
>
>
>      if (parser->brace_count < 0 ||
>          parser->bracket_count < 0 ||
>          (parser->brace_count == 0 &&
>           parser->bracket_count == 0)) {
>          json = json_parser_parse(parser->tokens, parser->ap, &err);
>          parser->tokens = NULL;
>          goto out_emit;
>      }
>
>      return;
>
> out_emit:
>
> Obviously, the goal of the rewrite was to convert:
>
> if (cond) {
>    do stuff
> } else {
>    return
> }
> more stuff
>
> into the more legible
>
> if (!cond) {
>    return
> }
> do stuff
> more stuff
>
> Let's re-read my original review:
>
> https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03017.html
>
> > Applying deMorgan's rules:
> >
> > !(brace < 0 || bracket < 0 || (brace == 0 && bracket == 0))
> > !(brace < 0) && !(bracket < 0) && !(brace == 0 && bracket == 0)
> > brace >= 0 && bracket >= 0 && (!(brace == 0) || !(bracket == 0))
> > brace >= 0 && bracket >= 0 && (brace != 0 || bracket != 0)
> >
> > But based on what we learned in the first two conjunctions, we can
> rewrite the third:
> >
> >
> > brace >= 0 && bracket >= 0 && (brace > 0 || bracket > 0)
> >
> > and then commute the logic:
> >
> > (brace > 0 || bracket > 0) && brace >= 0 && bracket >= 0
> >
>
> What I missed was the typo: we checked bracket >= 0 twice, instead of
> the intended brace >= 0 && bracket >= 0.  This needs a v2.
>

Hello Eric,

Thanks for the explanation.
I'll send v2 with the required changes.

--
Simran


>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
>
>

Re: [PATCH] qobject: json-streamer: Remove double test

Posted by Eric Blake 5 years, 7 months ago

On 4/2/20 8:12 AM, Eric Blake wrote:
> On 4/2/20 7:13 AM, Simran Singhal wrote:
>> Remove the duplicate test "parser->bracket_count >= 0".
>>
>> Signed-off-by: Simran Singhal <singhalsimran0@gmail.com>
>> ---
>>   qobject/json-streamer.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>

>>
>> (brace > 0 || bracket > 0) && brace >= 0 && bracket >= 0
>>
> 
> What I missed was the typo: we checked bracket >= 0 twice, instead of 
> the intended brace >= 0 && bracket >= 0.  This needs a v2.

Effect of the bug:

Note that we can diagnose when we have unbalanced ] with no matching [ 
while inside {}:

$ qemu-kvm --nodefaults --nographic --qmp stdio
{"QMP": {"version": {"qemu": {"micro": 91, "minor": 2, "major": 4}, 
"package": "v5.0.0-rc1-1-gf6ce4a439a08"}, "capabilities": ["oob"]}}
{]
{"error": {"class": "GenericError", "desc": "JSON parse error, expecting 
value"}}

but that we fail to diagnose unbalanced } with no matching { while 
inside []:

[}


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org