On Fri, 22 Jan 2010 13:03:51 -0600
Adam Litke <a...@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 <a...@us.ibm.com>
> To: Anthony Liguori <anth...@codemonkey.ws>
> cc: Luiz Capitulino <lcapitul...@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 */
>
>
