[Qemu-devel] Re: [RFC] New API for asynchronous monitor commands

[Luiz Capitulino]

On Mon, 25 Jan 2010 08:00:45 -0600
Adam Litke <address@hidden> 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.