diff mbox series

[RFC,05/15] qjson: add "opaque" field to JSONMessageParser

Message ID 1505375436-28439-6-git-send-email-peterx@redhat.com
State New
Headers show
Series QMP: out-of-band (OOB) execution support | expand

Commit Message

Peter Xu Sept. 14, 2017, 7:50 a.m. UTC 
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(-)

Comments

Eric Blake Sept. 19, 2017, 8:55 p.m. UTC | #1 
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.
Peter Xu Sept. 20, 2017, 5:45 a.m. UTC | #2 
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!
diff mbox series

Patch

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(). */
+    void *opaque;
 } JSONMessageParser;
 
 void json_message_parser_init(JSONMessageParser *parser,
-                              void (*func)(JSONMessageParser *, GQueue *));
+                              void (*func)(JSONMessageParser *, GQueue *,
+                                           void *opaque),
+                              void *opaque);
 
 int json_message_parser_feed(JSONMessageParser *parser,
                              const char *buffer, size_t size);
diff --git a/monitor.c b/monitor.c
index 8b32519..9096b64 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3817,7 +3817,8 @@  static int monitor_can_read(void *opaque)
     return (mon->suspend_cnt == 0) ? 1 : 0;
 }
 
-static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
+static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens,
+                               void *opaque)
 {
     QObject *req, *rsp = NULL, *id = NULL;
     QDict *qdict = NULL;
@@ -3964,7 +3965,7 @@  static void monitor_qmp_event(void *opaque, int event)
         break;
     case CHR_EVENT_CLOSED:
         json_message_parser_destroy(&mon->qmp.parser);
-        json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
+        json_message_parser_init(&mon->qmp.parser, handle_qmp_command, NULL);
         mon_refcount--;
         monitor_fdsets_cleanup();
         break;
@@ -4114,7 +4115,7 @@  void monitor_init(Chardev *chr, int flags)
         qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
                                  monitor_qmp_event, NULL, mon, NULL, true);
         qemu_chr_fe_set_echo(&mon->chr, true);
-        json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
+        json_message_parser_init(&mon->qmp.parser, handle_qmp_command, NULL);
     } else {
         qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_read,
                                  monitor_event, NULL, mon, NULL, true);
diff --git a/qga/main.c b/qga/main.c
index 62a6275..3b5ebbc 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -593,7 +593,8 @@  static void process_command(GAState *s, QDict *req)
 }
 
 /* handle requests/control events coming in over the channel */
-static void process_event(JSONMessageParser *parser, GQueue *tokens)
+static void process_event(JSONMessageParser *parser, GQueue *tokens,
+                          void *opaque)
 {
     GAState *s = container_of(parser, GAState, parser);
     QDict *qdict;
@@ -1320,7 +1321,7 @@  static int run_agent(GAState *s, GAConfig *config, int socket_activation)
     s->command_state = ga_command_state_new();
     ga_command_state_init(s, s->command_state);
     ga_command_state_init_all(s->command_state);
-    json_message_parser_init(&s->parser, process_event);
+    json_message_parser_init(&s->parser, process_event, NULL);
 
 #ifndef _WIN32
     if (!register_signal_handlers()) {
diff --git a/qobject/json-streamer.c b/qobject/json-streamer.c
index c51c202..12865d5 100644
--- a/qobject/json-streamer.c
+++ 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),
+                              void *opaque)
 {
     parser->emit = func;
     parser->brace_count = 0;
     parser->bracket_count = 0;
     parser->tokens = g_queue_new();
     parser->token_size = 0;
+    parser->opaque = opaque;
 
     json_lexer_init(&parser->lexer, json_message_process_token);
 }
diff --git a/qobject/qjson.c b/qobject/qjson.c
index 2e09308..f9766fe 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -28,7 +28,8 @@  typedef struct JSONParsingState
     Error *err;
 } JSONParsingState;
 
-static void parse_json(JSONMessageParser *parser, GQueue *tokens)
+static void parse_json(JSONMessageParser *parser, GQueue *tokens,
+                       void *opaque)
 {
     JSONParsingState *s = container_of(parser, JSONParsingState, parser);
 
@@ -41,7 +42,7 @@  QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
 
     state.ap = ap;
 
-    json_message_parser_init(&state.parser, parse_json);
+    json_message_parser_init(&state.parser, parse_json, NULL);
     json_message_parser_feed(&state.parser, string, strlen(string));
     json_message_parser_flush(&state.parser);
     json_message_parser_destroy(&state.parser);
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 0c12b38..fbd8e12 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -387,7 +387,8 @@  typedef struct {
     QDict *response;
 } QMPResponseParser;
 
-static void qmp_response(JSONMessageParser *parser, GQueue *tokens)
+static void qmp_response(JSONMessageParser *parser, GQueue *tokens,
+                         void *opaque)
 {
     QMPResponseParser *qmp = container_of(parser, QMPResponseParser, parser);
     QObject *obj;
@@ -409,7 +410,7 @@  QDict *qmp_fd_receive(int fd)
     bool log = getenv("QTEST_LOG") != NULL;
 
     qmp.response = NULL;
-    json_message_parser_init(&qmp.parser, qmp_response);
+    json_message_parser_init(&qmp.parser, qmp_response, NULL);
     while (!qmp.response) {
         ssize_t len;
         char c;