[{"id":1771387,"web_url":"http://patchwork.ozlabs.org/comment/1771387/","msgid":"<94721733-af41-690f-d17f-dbb3f7c593f5@redhat.com>","list_archive_url":null,"date":"2017-09-19T20:55:41","subject":"Re: [Qemu-devel] [RFC 05/15] qjson: add \"opaque\" field to\n\tJSONMessageParser","submitter":{"id":6591,"url":"http://patchwork.ozlabs.org/api/people/6591/","name":"Eric Blake","email":"eblake@redhat.com"},"content":"On 09/14/2017 02:50 AM, Peter Xu wrote:\n> It'll be passed to emit() as well when it happens.\n> \n> Signed-off-by: Peter Xu \n> ---\n> include/qapi/qmp/json-streamer.h | 8 ++++++--\n> monitor.c | 7 ++++---\n> qga/main.c | 5 +++--\n> qobject/json-streamer.c | 7 +++++--\n> qobject/qjson.c | 5 +++--\n> tests/libqtest.c | 5 +++--\n> 6 files changed, 24 insertions(+), 13 deletions(-)\n> \n> diff --git a/include/qapi/qmp/json-streamer.h b/include/qapi/qmp/json-streamer.h\n> index 00d8a23..b83270c 100644\n> --- a/include/qapi/qmp/json-streamer.h\n> +++ b/include/qapi/qmp/json-streamer.h\n> @@ -25,16 +25,20 @@ typedef struct JSONToken {\n> \n> typedef struct JSONMessageParser\n> {\n> - void (*emit)(struct JSONMessageParser *parser, GQueue *tokens);\n> + void (*emit)(struct JSONMessageParser *parser, GQueue *tokens, void *opaque);\n> JSONLexer lexer;\n> int brace_count;\n> int bracket_count;\n> GQueue *tokens;\n> uint64_t token_size;\n> + /* To be passed in when emit(). */\n\nReads awkwardly, better might be: /* Passed to emit() */\n\nI might group void *opaque right next to emit, rather than separated by\nthe rest of the struct, to make it obvious they are related, at which\npoint the comment isn't necessary.\n\n> + void *opaque;\n> } JSONMessageParser;\n> \n> void json_message_parser_init(JSONMessageParser *parser,\n> - void (*func)(JSONMessageParser *, GQueue *));\n> + void (*func)(JSONMessageParser *, GQueue *,\n> + void *opaque),\n\nInconsistent to name only one of the three parameters in the inner\nfunction pointer type for the outer parameter 'func'. I wonder if a\ntypedef would make things more legible.\n\n> + void *opaque);\n> \n\n> +++ b/qobject/json-streamer.c\n> @@ -102,18 +102,21 @@ out_emit:\n> */\n> tokens = parser->tokens;\n> parser->tokens = g_queue_new();\n> - parser->emit(parser, tokens);\n> + parser->emit(parser, tokens, parser->opaque);\n> parser->token_size = 0;\n> }\n> \n> void json_message_parser_init(JSONMessageParser *parser,\n> - void (*func)(JSONMessageParser *, GQueue *))\n> + void (*func)(JSONMessageParser *,\n> + GQueue *, void *opaque),\n\nAgain, inconsistent that you named only 1 of the three inner parameters.\n\nOverall, the idea looks reasonable.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx03.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx03.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=eblake@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xxZtG6F12z9sNr\n\tfor ;\n\tWed, 20 Sep 2017 06:56:37 +1000 (AEST)","from localhost ([::1]:45407 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duPZL-0008IU-1k\n\tfor incoming@patchwork.ozlabs.org; Tue, 19 Sep 2017 16:56:35 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:37780)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duPYd-000861-0f\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 16:55:52 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duPYa-0007I2-Cv\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 16:55:51 -0400","from mx1.redhat.com ([209.132.183.28]:35598)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1duPYZ-0007H2-TP\n\tfor qemu-devel@nongnu.org; Tue, 19 Sep 2017 16:55:48 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 7B1E17E429;\n\tTue, 19 Sep 2017 20:55:46 +0000 (UTC)","from [10.10.124.97] (ovpn-124-97.rdu2.redhat.com [10.10.124.97])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 8769A5D97C;\n\tTue, 19 Sep 2017 20:55:42 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 7B1E17E429","To":"Peter Xu , qemu-devel@nongnu.org","References":"<1505375436-28439-1-git-send-email-peterx@redhat.com>\n\t<1505375436-28439-6-git-send-email-peterx@redhat.com>","From":"Eric Blake ","Openpgp":"url=http://people.redhat.com/eblake/eblake.gpg","Organization":"Red Hat, Inc.","Message-ID":"<94721733-af41-690f-d17f-dbb3f7c593f5@redhat.com>","Date":"Tue, 19 Sep 2017 15:55:41 -0500","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<1505375436-28439-6-git-send-email-peterx@redhat.com>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"HGATPHq0biWJaRGaO1ouOuKMJ3uE7d4hL\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.27]);\n\tTue, 19 Sep 2017 20:55:46 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","X-Content-Filtered-By":"Mailman/MimeDel 2.1.21","Subject":"Re: [Qemu-devel] [RFC 05/15] qjson: add \"opaque\" field to\n\tJSONMessageParser","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Laurent Vivier , Fam Zheng , Juan\n\tQuintela , Markus Armbruster ,\n\tmdroth@linux.vnet.ibm.com, \tStefan Hajnoczi ,\n\t=?utf-8?q?Marc-Andr=C3=A9_Lure?= =?utf-8?q?au?=\n\t, \tPaolo Bonzini ,\n\t\"Dr . David Alan Gilbert\" ","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1771580,"web_url":"http://patchwork.ozlabs.org/comment/1771580/","msgid":"<20170920054553.GU3617@pxdev.xzpeter.org>","list_archive_url":null,"date":"2017-09-20T05:45:53","subject":"Re: [Qemu-devel] [RFC 05/15] qjson: add \"opaque\" field to\n\tJSONMessageParser","submitter":{"id":67717,"url":"http://patchwork.ozlabs.org/api/people/67717/","name":"Peter Xu","email":"peterx@redhat.com"},"content":"On Tue, Sep 19, 2017 at 03:55:41PM -0500, Eric Blake wrote:\n> On 09/14/2017 02:50 AM, Peter Xu wrote:\n> > It'll be passed to emit() as well when it happens.\n> > \n> > Signed-off-by: Peter Xu \n> > ---\n> > include/qapi/qmp/json-streamer.h | 8 ++++++--\n> > monitor.c | 7 ++++---\n> > qga/main.c | 5 +++--\n> > qobject/json-streamer.c | 7 +++++--\n> > qobject/qjson.c | 5 +++--\n> > tests/libqtest.c | 5 +++--\n> > 6 files changed, 24 insertions(+), 13 deletions(-)\n> > \n> > diff --git a/include/qapi/qmp/json-streamer.h b/include/qapi/qmp/json-streamer.h\n> > index 00d8a23..b83270c 100644\n> > --- a/include/qapi/qmp/json-streamer.h\n> > +++ b/include/qapi/qmp/json-streamer.h\n> > @@ -25,16 +25,20 @@ typedef struct JSONToken {\n> > \n> > typedef struct JSONMessageParser\n> > {\n> > - void (*emit)(struct JSONMessageParser *parser, GQueue *tokens);\n> > + void (*emit)(struct JSONMessageParser *parser, GQueue *tokens, void *opaque);\n> > JSONLexer lexer;\n> > int brace_count;\n> > int bracket_count;\n> > GQueue *tokens;\n> > uint64_t token_size;\n> > + /* To be passed in when emit(). */\n> \n> Reads awkwardly, better might be: /* Passed to emit() */\n> \n> I might group void *opaque right next to emit, rather than separated by\n> the rest of the struct, to make it obvious they are related, at which\n> point the comment isn't necessary.\n\nWill remove that comment and move it upwards to emit().\n\n> \n> > + void *opaque;\n> > } JSONMessageParser;\n> > \n> > void json_message_parser_init(JSONMessageParser *parser,\n> > - void (*func)(JSONMessageParser *, GQueue *));\n> > + void (*func)(JSONMessageParser *, GQueue *,\n> > + void *opaque),\n> \n> Inconsistent to name only one of the three parameters in the inner\n> function pointer type for the outer parameter 'func'. I wonder if a\n> typedef would make things more legible.\n\nYep, will do.\n\n> \n> > + void *opaque);\n> > \n> \n> > +++ b/qobject/json-streamer.c\n> > @@ -102,18 +102,21 @@ out_emit:\n> > */\n> > tokens = parser->tokens;\n> > parser->tokens = g_queue_new();\n> > - parser->emit(parser, tokens);\n> > + parser->emit(parser, tokens, parser->opaque);\n> > parser->token_size = 0;\n> > }\n> > \n> > void json_message_parser_init(JSONMessageParser *parser,\n> > - void (*func)(JSONMessageParser *, GQueue *))\n> > + void (*func)(JSONMessageParser *,\n> > + GQueue *, void *opaque),\n> \n> Again, inconsistent that you named only 1 of the three inner parameters.\n\nFixing up.\n\n> \n> Overall, the idea looks reasonable.\n\nThanks!","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx08.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx08.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=peterx@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xxpdk0VR7z9sBW\n\tfor ;\n\tWed, 20 Sep 2017 15:46:33 +1000 (AEST)","from localhost ([::1]:46819 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1duXq9-0002tS-Nx\n\tfor incoming@patchwork.ozlabs.org; Wed, 20 Sep 2017 01:46:29 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:40432)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1duXpp-0002tE-0Q\n\tfor qemu-devel@nongnu.org; Wed, 20 Sep 2017 01:46:10 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1duXpl-0002H4-Sf\n\tfor qemu-devel@nongnu.org; Wed, 20 Sep 2017 01:46:09 -0400","from mx1.redhat.com ([209.132.183.28]:36544)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1duXpl-000285-Fx\n\tfor qemu-devel@nongnu.org; Wed, 20 Sep 2017 01:46:05 -0400","from smtp.corp.redhat.com\n\t(int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 5CBC1C0587FE;\n\tWed, 20 Sep 2017 05:46:04 +0000 (UTC)","from pxdev.xzpeter.org (dhcp-15-224.nay.redhat.com [10.66.15.224])\n\tby smtp.corp.redhat.com (Postfix) with ESMTPS id 03920600CA;\n\tWed, 20 Sep 2017 05:45:55 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 5CBC1C0587FE","Date":"Wed, 20 Sep 2017 13:45:53 +0800","From":"Peter Xu ","To":"Eric Blake ","Message-ID":"<20170920054553.GU3617@pxdev.xzpeter.org>","References":"<1505375436-28439-1-git-send-email-peterx@redhat.com>\n\t<1505375436-28439-6-git-send-email-peterx@redhat.com>\n\t<94721733-af41-690f-d17f-dbb3f7c593f5@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<94721733-af41-690f-d17f-dbb3f7c593f5@redhat.com>","User-Agent":"Mutt/1.5.24 (2015-08-30)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.11","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.32]);\n\tWed, 20 Sep 2017 05:46:04 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [RFC 05/15] qjson: add \"opaque\" field to\n\tJSONMessageParser","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Laurent Vivier , Fam Zheng ,\n\tmdroth@linux.vnet.ibm.com, Juan Quintela ,\n\tMarkus Armbruster , qemu-devel@nongnu.org, Stefan\n\tHajnoczi , =?utf-8?q?Marc-Andr=C3=A9?=\n\tLureau ,\n\tPaolo Bonzini , \"Dr . David Alan Gilbert\"\n\t","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]