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 <pet...@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
