From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NZQSy-0006Gb-3L for qemu-devel@nongnu.org; Mon, 25 Jan 2010 10:03:00 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NZQSt-0006Cp-4G for qemu-devel@nongnu.org; Mon, 25 Jan 2010 10:02:59 -0500 Received: from [199.232.76.173] (port=46711 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NZQSs-0006Ch-TB for qemu-devel@nongnu.org; Mon, 25 Jan 2010 10:02:54 -0500 Received: from mx1.redhat.com ([209.132.183.28]:7457) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NZQSs-0008HK-5l for qemu-devel@nongnu.org; Mon, 25 Jan 2010 10:02:54 -0500 Date: Mon, 25 Jan 2010 13:02:41 -0200 From: Luiz Capitulino Message-ID: <20100125130241.1f1d603e@doriath> In-Reply-To: <1264428045.24893.22.camel@aglitke> References: <1264187031.2861.13.camel@aglitke> <20100125110803.3841605a@doriath> <1264428045.24893.22.camel@aglitke> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [RFC] New API for asynchronous monitor commands List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Adam Litke Cc: qemu-devel@nongnu.org On Mon, 25 Jan 2010 08:00:45 -0600 Adam Litke wrote: > On Mon, 2010-01-25 at 11:08 -0200, Luiz Capitulino wrote: > > > @@ -85,11 +91,19 @@ typedef struct mon_cmd_t { > > > union { > > > void (*info)(Monitor *mon); > > > void (*info_new)(Monitor *mon, QObject **ret_data); > > > + int (*info_async)(Monitor *mon, QMPCompletion *cb, void *opaque); > > > void (*cmd)(Monitor *mon, const QDict *qdict); > > > void (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data); > > > + int (*cmd_async)(Monitor *mon, const QDict *params, > > > + QMPCompletion *cb, void *opaque); > > > } mhandler; > > > + int async; > > > } mon_cmd_t; > > > > Is 'async' really needed, can't use 'info_async' or 'cmd_async'? > > Yes. Otherwise the code cannot tell the difference between a monitor > command that uses cmd_new and a one using the cmd_async. They both pass > the monitor_handler_ported() test. Unless there is some underhanded way > of determining which union type is in use for mhandler, we are stuck > with the extra variable -- that is, unless we port all cmd_new cmds to > the new async API :) The async member can stay then :) > > > +static void do_async_info_handler(Monitor *mon, const mon_cmd_t *cmd); > > > +static void do_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd, > > > + const QDict *params); > > > + > > > > Isn't it possible to avoid this forward declarations? > > Sure, but I found the code more readable when I could define the > handlers near monitor_call_handler(). However, I dislike forward > declarations as much as the next guy. I'll make it go away. The real solution is to split this code in more files. > > > +static void qmp_monitor_complete(void *opaque, QObject *ret_data) > > > +{ > > > + Monitor *mon = (Monitor *)opaque; > > > + monitor_protocol_emitter(mon, ret_data); > > > +} > > > > You should free ret_data as well with: > > > > qobject_decref(ret_data); > > Hmm. The way I saw this working was like so: > > do_async_cmd_handler() > cmd->mhandler.cmd_async() > dispatch_async_cmd() > ... > command_completion_event() > QObject *ret_data = qobject_from_jsonf("'foo': 'bar'"); > QMPCompletion(opaque, ret_data); > qobject_decref(ret_data); > > In other words, the qobject ret_data is created by the caller of the > QMPCompletion callback. Therefore, it seemed natural to let that > routine clean up the qobject rather than letting the callback "consume" > it. I realize that this patch makes it impossible to infer the above > explanation since an example async command implementation was not > provided. Since you designed the qobject interfaces, you have the best > idea on how it should work. Does the above make sense? Yes, it does. No need to change.