On Mon, 25 Jan 2010 08:00:45 -0600
Adam Litke <a...@us.ibm.com> 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.
