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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Fri, 22 Jan 2010 13:03:51 -0600
Adam Litke <agl@us.ibm.com> wrote:

> Qemu has a number of commands that can operate asynchronously (savevm, migrate,
> etc) and it will be getting more.  For these commands, the user monitor needs
> to be suspended, but QMP monitors could continue to to accept other commands.
> This patch introduces a new command API that isolates the details of handling
> different monitor types from the actual command execution.
> 
> A monitor command can use this API by implementing the mhandler.cmd_async
> handler (or info_async if appropriate).  This function is responsible for
> submitting the command and does not return any data although it may raise
> errors.  When the command completes, the QMPCompletion callback should be
> invoked with its opaque data and the command result.
> 
> The process for submitting and completing an asynchronous command is different
> for QMP and user monitors.  A user monitor must be suspended at submit time and
> resumed at completion time.  The user_print() function must be passed to the
> QMPCompletion callback so the result can be displayed properly.  QMP monitors
> are simpler.  No submit time setup is required.  When the command completes,
> monitor_protocol_emitter() writes the result in JSON format.

 The QMPCompletion callback is called when the asynchronous event happens,
by the function handling it, right?

> This API can also be used to implement synchronous commands.  In this case, the
> cmd_async handler should immediately call the QMPCompletion callback.  It is my
> hope that this new interface will work for all commands, leading to a
> drastically simplified monitor.c once all commands are ported.

 I like the patch, but I don't think it's a good idea to use this in
synchronous commands as they will have to call QMPCompletion (not to
mention unneeded suspend/resume calls).

 Also, better to call it MonitorCompletion as this is not only about
QMP.

 More comments follow.

> Thanks to Anthony for helping me out with the initial design.
> 
> Signed-off-by: Adam Litke <agl@us.ibm.com>
> To: Anthony Liguori <anthony@codemonkey.ws>
> cc: Luiz Capitulino <lcapitulino@redhat.com>
> Cc: qemu-devel@nongnu.org
> 
> diff --git a/monitor.c b/monitor.c
> index cadf422..c0d0fa9 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -76,6 +76,12 @@
>   *
>   */
>  
> +typedef struct UserQMPCompletionData UserQMPCompletionData;
> +struct UserQMPCompletionData {
> +    Monitor *mon;
> +    void (*user_print)(Monitor *mon, const QObject *data);
> +};
> +
>  typedef struct mon_cmd_t {
>      const char *name;
>      const char *args_type;
> @@ -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'?

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

>  /* 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?

> +
> +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);

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

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

> +         */
> +        *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_".

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

> +
> +static void do_async_info_handler(Monitor *mon, const mon_cmd_t *cmd)
> +{
> +    if (monitor_ctrl_mode(mon)) {
> +        cmd->mhandler.info_async(mon, 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.info_async(mon, user_monitor_complete, cb_data);
> +        if (ret < 0) {
> +            monitor_resume(mon);
> +            qemu_free(cb_data);
> +        }
> +    }
> +}
> +
>  static void handle_user_command(Monitor *mon, const char *cmdline)
>  {
>      QDict *qdict;
> @@ -3644,7 +3741,9 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
>  
>      qemu_errors_to_mon(mon);
>  
> -    if (monitor_handler_ported(cmd)) {
> +    if (monitor_handler_is_async(cmd)) {
> +        do_async_cmd_handler(mon, cmd, qdict);
> +    } else if (monitor_handler_ported(cmd)) {
>          monitor_call_handler(mon, cmd, qdict);
>      } else {
>          cmd->mhandler.cmd(mon, qdict);
> @@ -4106,7 +4205,11 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>          goto err_out;
>      }
>  
> -    monitor_call_handler(mon, cmd, args);
> +    if (monitor_handler_is_async(cmd)) {
> +        do_async_cmd_handler(mon, cmd, args);
> +    } else {
> +        monitor_call_handler(mon, cmd, args);
> +    }
>      goto out;
>  
>  err_input:
> diff --git a/monitor.h b/monitor.h
> index 2da30e8..366b423 100644
> --- a/monitor.h
> +++ b/monitor.h
> @@ -44,4 +44,6 @@ void monitor_printf(Monitor *mon, const char *fmt, ...)
>  void monitor_print_filename(Monitor *mon, const char *filename);
>  void monitor_flush(Monitor *mon);
>  
> +typedef void (QMPCompletion)(void *opaque, QObject *ret_data);
> +
>  #endif /* !MONITOR_H */
> 
>