[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC 05/15] qjson: add "opaque" field to JSONMessagePar
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [RFC 05/15] qjson: add "opaque" field to JSONMessageParser |
Date: |
Wed, 20 Sep 2017 13:45:53 +0800 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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 <address@hidden>
> > ---
> > 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
- [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll, (continued)
- [Qemu-devel] [RFC 01/15] char-io: fix possible race on IOWatchPoll, Peter Xu, 2017/09/14
- [Qemu-devel] [RFC 02/15] qobject: allow NULL for qstring_get_str(), Peter Xu, 2017/09/14
- [Qemu-devel] [RFC 03/15] qobject: introduce qobject_to_str(), Peter Xu, 2017/09/14
- [Qemu-devel] [RFC 04/15] monitor: move skip_flush into monitor_data_init, Peter Xu, 2017/09/14
- [Qemu-devel] [RFC 05/15] qjson: add "opaque" field to JSONMessageParser, Peter Xu, 2017/09/14
- [Qemu-devel] [RFC 06/15] monitor: move the cur_mon hack deeper for QMP, Peter Xu, 2017/09/14
- [Qemu-devel] [RFC 07/15] monitor: unify global init, Peter Xu, 2017/09/14
- [Qemu-devel] [RFC 08/15] monitor: create IO thread, Peter Xu, 2017/09/14
- [Qemu-devel] [RFC 09/15] monitor: allow to use IO thread for parsing, Peter Xu, 2017/09/14
- [Qemu-devel] [RFC 10/15] monitor: introduce monitor_qmp_respond(), Peter Xu, 2017/09/14