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

[Peter Xu <peterx@redhat.com>]

On Tue, Sep 19, 2017 at 03:55:41PM -0500, Eric Blake wrote:
> 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.

Will remove that comment and move it upwards to emit().

> 
> > +    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.

Yep, will do.

> 
> > +                              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.

Fixing up.

> 
> Overall, the idea looks reasonable.

Thanks!

-- 
Peter Xu