Re: [Qemu-devel] [RFC 05/15] qjson: add "opaque" field to JSONMessageParser

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2762 bytes --]

On 09/14/2017 02:50 AM, Peter Xu wrote:
> It'll be passed to emit() as well when it happens.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qapi/qmp/json-streamer.h | 8 ++++++--
>  monitor.c                        | 7 ++++---
>  qga/main.c                       | 5 +++--
>  qobject/json-streamer.c          | 7 +++++--
>  qobject/qjson.c                  | 5 +++--
>  tests/libqtest.c                 | 5 +++--
>  6 files changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/include/qapi/qmp/json-streamer.h b/include/qapi/qmp/json-streamer.h
> index 00d8a23..b83270c 100644
> --- a/include/qapi/qmp/json-streamer.h
> +++ b/include/qapi/qmp/json-streamer.h
> @@ -25,16 +25,20 @@ typedef struct JSONToken {
>  
>  typedef struct JSONMessageParser
>  {
> -    void (*emit)(struct JSONMessageParser *parser, GQueue *tokens);
> +    void (*emit)(struct JSONMessageParser *parser, GQueue *tokens, void *opaque);
>      JSONLexer lexer;
>      int brace_count;
>      int bracket_count;
>      GQueue *tokens;
>      uint64_t token_size;
> +    /* To be passed in when emit(). */

Reads awkwardly, better might be: /* Passed to emit() */

I might group void *opaque right next to emit, rather than separated by
the rest of the struct, to make it obvious they are related, at which
point the comment isn't necessary.

> +    void *opaque;
>  } JSONMessageParser;
>  
>  void json_message_parser_init(JSONMessageParser *parser,
> -                              void (*func)(JSONMessageParser *, GQueue *));
> +                              void (*func)(JSONMessageParser *, GQueue *,
> +                                           void *opaque),

Inconsistent to name only one of the three parameters in the inner
function pointer type for the outer parameter 'func'.  I wonder if a
typedef would make things more legible.

> +                              void *opaque);
>  

> +++ b/qobject/json-streamer.c
> @@ -102,18 +102,21 @@ out_emit:
>       */
>      tokens = parser->tokens;
>      parser->tokens = g_queue_new();
> -    parser->emit(parser, tokens);
> +    parser->emit(parser, tokens, parser->opaque);
>      parser->token_size = 0;
>  }
>  
>  void json_message_parser_init(JSONMessageParser *parser,
> -                              void (*func)(JSONMessageParser *, GQueue *))
> +                              void (*func)(JSONMessageParser *,
> +                                           GQueue *, void *opaque),

Again, inconsistent that you named only 1 of the three inner parameters.

Overall, the idea looks reasonable.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]