Re: [PATCH v3 4/7] json-streamer: make brace/bracket count unsigned

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> It makes no sense to let brace_count and bracket_count go negative,
> also because it immediately ends error recovery and sets them both
> back to zero.  Instead set them to zero *before* choosing
> whether to process the token queue; this makes it possible to
> have the fields as unsigned.
>
> Note that JSON_END_OF_INPUT now forces the parentheses to appear
> balanced, so that the queue is emptied and an error is reported;
> hence, the "type != JSON_END_OF_INPUT" condition can be removed.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qobject/json-parser.h |  4 ++--
>  qobject/json-streamer.c       | 19 ++++++++++++++++---
>  2 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/include/qobject/json-parser.h b/include/qobject/json-parser.h
> index 4c3d89f751f..0cf6932ecdc 100644
> --- a/include/qobject/json-parser.h
> +++ b/include/qobject/json-parser.h
> @@ -31,8 +31,8 @@ typedef struct JSONMessageParser {
>      void *opaque;
>      JSONLexer lexer;
>      JSONParserContext parser;
> -    int brace_count;
> -    int bracket_count;
> +    unsigned int brace_count;
> +    unsigned int bracket_count;
>      GQueue tokens;
>      uint64_t token_size;
>  } JSONMessageParser;
> diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
> index f3dfdcaea12..b0bf2083ca6 100644
> --- a/qobject/json-streamer.c
> +++ b/qobject/json-streamer.c
> @@ -41,12 +41,18 @@ void json_message_process_token(JSONLexer *lexer, GString *input,
>          parser->brace_count++;
>          break;
>      case JSON_RCURLY:
> +        if (parser->brace_count <= 0) {

Why <= 0?  parser->brace_count is unsigned now...

> +            goto end_error_recovery;
> +        }
>          parser->brace_count--;
>          break;
>      case JSON_LSQUARE:
>          parser->bracket_count++;
>          break;
>      case JSON_RSQUARE:
> +        if (parser->bracket_count <= 0) {

Likewise.

> +            goto end_error_recovery;
> +        }
>          parser->bracket_count--;
>          break;
>      case JSON_ERROR:
> @@ -56,6 +62,15 @@ void json_message_process_token(JSONLexer *lexer, GString *input,
>          if (g_queue_is_empty(&parser->tokens)) {
>              return;
>          }
> +    end_error_recovery:
> +        /*
> +         * Cause error recovery to end immediately.
> +         * If not in error recovery, the parser will raise an error
> +         * (due to JSON_ERROR or unexpected JSON_R{CURLY,SQUARE})

What do you mean by "due to JSON_ERROR"?

We get here on unmatched } or ]: that's the gotos
you add above.

We also get here on end of non-empty input: that's when the if right
above doesn't return.

> +         * but error recovery won't be entered at all.
> +         */

This is the only comment on error recovery.  Would a comment giving a
brief overview on error recovery be useful?

> +        parser->brace_count = 0;
> +        parser->bracket_count = 0;
>          break;
>      default:
>          break;
> @@ -83,9 +98,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->brace_count >= 0 && parser->bracket_count >= 0
> -        && type != JSON_END_OF_INPUT) {
> +    if (parser->brace_count > 0 || parser->bracket_count > 0) {
>          return;
>      }

We get here when

* no lexical error, and

* no limits exceeded, and

* the curlies and squares are balanced, either in fact or because
  we passed through end_error_recovery.

Good.

       /* Process all tokens in the queue */
       while (!g_queue_is_empty(&parser->tokens)) {