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

[Adam Litke]

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 :)

> > +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.

> >  /* file descriptors passed via SCM_RIGHTS */
> >  typedef struct mon_fd_t mon_fd_t;
> >  struct mon_fd_t {
> > @@ -255,6 +269,11 @@ static inline int monitor_handler_ported(const 
> > mon_cmd_t *cmd)
> >      return cmd->user_print != NULL;
> >  }
> >  
> > +static inline bool monitor_handler_is_async(const mon_cmd_t *cmd)
> > +{
> > +    return cmd->async != 0;
> > +}
> > +
> >  static inline int monitor_has_error(const Monitor *mon)
> >  {
> >      return mon->error != NULL;
> > @@ -453,6 +472,23 @@ static void do_commit(Monitor *mon, const QDict *qdict)
> >      }
> >  }
> >  
> > +static void user_monitor_complete(void *opaque, QObject *ret_data)
> > +{
> > +    UserQMPCompletionData *data = (UserQMPCompletionData *)opaque; 
> > +
> > +    if (ret_data) {
> > +        data->user_print(data->mon, ret_data);
> > +    }
> > +    monitor_resume(data->mon);
> > +    qemu_free(data);
> > +}
> 
>  MonitorCompletionData?

Sure.  I like this name too.

> > +
> > +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?

>  Also, you can pass 'opaque' directly to monitor_protocol_emitter().

Ok.

> 
> > +
> >  static void do_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
> >  {
> >      const mon_cmd_t *cmd;
> > @@ -476,7 +512,15 @@ static void do_info(Monitor *mon, const QDict *qdict, 
> > QObject **ret_data)
> >          goto help;
> >      }
> >  
> > -    if (monitor_handler_ported(cmd)) {
> > +    if (monitor_handler_is_async(cmd)) {
> > +        do_async_info_handler(mon, cmd);
> > +        /*
> > +         * Indicate that this command is asynchronous and will not return 
> > any
> > +         * date (not even empty).  Instead, the data will be returned via a
> > +         * completion callback.
> 
>  s/date/data

Bah, my #1 typo.

> 
> > +         */
> > +        *ret_data = qobject_from_jsonf("{ 'async': 'return' }");
> 
>  This is obviously only used internally, right? Sure it's _impossible_
> to conflict with handlers' returned keys?
> 
>  Anyway, I think it's a good idea to have a standard for internal
> keys. Maybe something like: "__mon_".

Good idea.  I'll make this change.

> > +    } else if (monitor_handler_ported(cmd)) {
> >          cmd->mhandler.info_new(mon, ret_data);
> >  
> >          if (!monitor_ctrl_mode(mon)) {
> > @@ -3612,6 +3656,11 @@ static void monitor_print_error(Monitor *mon)
> >      mon->error = NULL;
> >  }
> >  
> > +static int is_async_return(const QObject *data)
> > +{
> > +    return data && qdict_haskey(qobject_to_qdict(data), "async");
> > +}
> > +
> >  static void monitor_call_handler(Monitor *mon, const mon_cmd_t *cmd,
> >                                   const QDict *params)
> >  {
> > @@ -3619,7 +3668,15 @@ static void monitor_call_handler(Monitor *mon, const 
> > mon_cmd_t *cmd,
> >  
> >      cmd->mhandler.cmd_new(mon, params, &data);
> >  
> > -    if (monitor_ctrl_mode(mon)) {
> > +    if (is_async_return(data)) {
> > +        /*
> > +         * Asynchronous commands have no initial return data but they can
> > +         * generate errors.  Data is returned via the async completion 
> > handler.
> > +         */
> > +        if (monitor_ctrl_mode(mon) && monitor_has_error(mon)) {
> > +            monitor_protocol_emitter(mon, NULL);
> > +        }
> > +    } else if (monitor_ctrl_mode(mon)) {
> >          /* Monitor Protocol */
> >          monitor_protocol_emitter(mon, data);
> >      } else {
> > @@ -3631,6 +3688,46 @@ static void monitor_call_handler(Monitor *mon, const 
> > mon_cmd_t *cmd,
> >      qobject_decref(data);
> >  }
> >  
> > +static void do_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
> > +                                 const QDict *params)
> > +{
> > +    if (monitor_ctrl_mode(mon)) {
> > +        cmd->mhandler.cmd_async(mon, params, qmp_monitor_complete, mon);
> > +    } else {
> > +        int ret;
> > +
> > +        UserQMPCompletionData *cb_data = qemu_malloc(sizeof(*cb_data));
> > +        cb_data->mon = mon;
> > +        cb_data->user_print = cmd->user_print;
> > +        monitor_suspend(mon);
> > +        ret = cmd->mhandler.cmd_async(mon, params,
> > +                                      user_monitor_complete, cb_data);
> > +        if (ret < 0) {
> > +            monitor_resume(mon);
> > +            qemu_free(cb_data);
> > +        }
> > +    }
> > +}
> 
>  I'm trying to decouple QMP and user Monitor functions so that in the
> near future we can have four modules: monitor.c (common code),
> monitor_qmp.c, monitor_user.c and monitor_handlers.c.
> 
>  So, maybe it's better to split this.

No problem.


-- 
Thanks,
Adam