| Message ID | 1264187031.2861.13.camel@aglitke |
|---|---|
| State | New |
| Headers | show |
On 01/22/2010 01:03 PM, Adam Litke 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. > > 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. > > 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 > I like this a lot and I'd like to see us remove cmd_new in place of cmd_async. The conversion is pretty easy since we just have to add a cb(ret_data) to the end of synchronous functions. Luiz/Markus/Avi, what do ya'll think? Regards, Anthony Liguori
On 01/23/2010 01:17 AM, Anthony Liguori wrote: > > I like this a lot and I'd like to see us remove cmd_new in place of > cmd_async. The conversion is pretty easy since we just have to add a > cb(ret_data) to the end of synchronous functions. > > Luiz/Markus/Avi, what do ya'll think? > Looks reasonable to me, minor nit about the implementation will be posted as a reply.
On 01/22/2010 09:03 PM, Adam Litke wrote: > > +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); > + } > + } > +} > Instead of sending opaques everywhere (and having them correspond to different types in different cases), I would prefer it if the handle always accepted an AsyncCommandCompletion *. That makes it easier to follow the code, since there are no opaques you have to guess the true type of. Somewhat related, we could have mon->suspend() and mon->resume() callbacks to avoid the check for monitor_ctrl_mode().
On 01/24/2010 04:59 AM, Avi Kivity wrote: > On 01/22/2010 09:03 PM, Adam Litke wrote: >> >> +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); >> + } >> + } >> +} > > Instead of sending opaques everywhere (and having them correspond to > different types in different cases), I would prefer it if the handle > always accepted an AsyncCommandCompletion *. That makes it easier to > follow the code, since there are no opaques you have to guess the true > type of. I agree with you in principle but the model of passing (function pointer, opaque) is pervasive within QEMU. I'd prefer consistency here and if we want to switch to something more like a function object, we do it globally. Regards, Anthony Liguori > > Somewhat related, we could have mon->suspend() and mon->resume() > callbacks to avoid the check for monitor_ctrl_mode(). >
On 01/24/2010 04:01 PM, Anthony Liguori wrote: >> Instead of sending opaques everywhere (and having them correspond to >> different types in different cases), I would prefer it if the handle >> always accepted an AsyncCommandCompletion *. That makes it easier to >> follow the code, since there are no opaques you have to guess the >> true type of. > > > I agree with you in principle but the model of passing (function > pointer, opaque) is pervasive within QEMU. I'd prefer consistency > here and if we want to switch to something more like a function > object, we do it globally. At least some recent code has moved in this direction (together with container_of()), but I don't think there's a reason to press this issue now.
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 */ > >
On Fri, 22 Jan 2010 17:17:27 -0600 Anthony Liguori <anthony@codemonkey.ws> wrote: > On 01/22/2010 01:03 PM, Adam Litke 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. > > > > 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. > > > > 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 > > > > I like this a lot and I'd like to see us remove cmd_new in place of > cmd_async. The conversion is pretty easy since we just have to add a > cb(ret_data) to the end of synchronous functions. > > Luiz/Markus/Avi, what do ya'll think? I like it too, but I don't like the idea of using it for synchronous commands, I explained the reasons in the patch itself.
On Sun, 24 Jan 2010 08:01:28 -0600 Anthony Liguori <anthony@codemonkey.ws> wrote: > On 01/24/2010 04:59 AM, Avi Kivity wrote: > > On 01/22/2010 09:03 PM, Adam Litke wrote: > >> > >> +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); > >> + } > >> + } > >> +} > > > > Instead of sending opaques everywhere (and having them correspond to > > different types in different cases), I would prefer it if the handle > > always accepted an AsyncCommandCompletion *. That makes it easier to > > follow the code, since there are no opaques you have to guess the true > > type of. > > I agree with you in principle but the model of passing (function > pointer, opaque) is pervasive within QEMU. I'd prefer consistency here > and if we want to switch to something more like a function object, we do > it globally. We could start doing it in the Monitor, could even serve as an example for other subsystems.
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.
On 01/25/2010 07:08 AM, Luiz Capitulino wrote: > On Fri, 22 Jan 2010 13:03:51 -0600 > Adam Litke<agl@us.ibm.com> wrote: > > 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). > I think the value of having a single mechanism is that it gives us a common code path that's exercised for every command. That helps avoid bugs in the long term. I'd like to see the monitor move from a static table of commands to dynamic command registration. IOW, we'd have something like: monitor_register_cmd(name, args_type, help, params, my_qmp_command, my_opaque); Given this API, it's pretty easy to write a wrapper that takes a simple synchronous callback without making such a concept core to the monitor infrastructure. Notice that there's no user_print here. I also would like to see us decouple QMP from the human monitor such that the human monitor was implemented purely in terms of QMP commands. The core monitor infrastructure should really only deal with QMP and the human monitor should be an independent client. Not all QMP concepts make sense for the human monitor and vice versa. The reason to start QMP as a mirror of the human monitor is to ensure management apps can have an easy transition. However, once we're there, we should not continue duplicating QMP commands in the human monitor. Regards, Anthony Liguori
On Mon, 25 Jan 2010 08:15:53 -0600 Anthony Liguori <anthony@codemonkey.ws> wrote: > On 01/25/2010 07:08 AM, Luiz Capitulino wrote: > > On Fri, 22 Jan 2010 13:03:51 -0600 > > Adam Litke<agl@us.ibm.com> wrote: > > > > 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). > > > > I think the value of having a single mechanism is that it gives us a > common code path that's exercised for every command. That helps avoid > bugs in the long term. I do agree with this, I just think that the calling of the completion callback should be done in common code, instead of duplicating its call among all synchronous clients. Also, we should not suspend/resume if there's no need for that. At least for now async handlers are the exception, making their handling the rule doesn't seem right to me. However, I believe we can check if the handler is synchronous and do all this job for it, can't we? > I'd like to see the monitor move from a static table of commands to > dynamic command registration. IOW, we'd have something like: > > monitor_register_cmd(name, args_type, help, params, my_qmp_command, > my_opaque); > > Given this API, it's pretty easy to write a wrapper that takes a simple > synchronous callback without making such a concept core to the monitor > infrastructure. Why do would we need this for the short-term? Before getting more complex, I'd like to see the Monitor code really refectored and this can't be done today w/o breaking mine (and probably Markus) queues, as we're still working on getting QMP ready for clients. > Notice that there's no user_print here. I also would like to see us > decouple QMP from the human monitor such that the human monitor was > implemented purely in terms of QMP commands. The core monitor > infrastructure should really only deal with QMP and the human monitor > should be an independent client. > > Not all QMP concepts make sense for the human monitor and vice versa. > The reason to start QMP as a mirror of the human monitor is to ensure > management apps can have an easy transition. However, once we're there, > we should not continue duplicating QMP commands in the human monitor. Sure thing! We are moving in this direction already and IMHO it would be desirable to see that happen first, so that we don't add more code today to be refectored later.
On Mon, 25 Jan 2010 08:00:45 -0600 Adam Litke <agl@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.
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. 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. 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; +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); + /* 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); +} + +static void qmp_monitor_complete(void *opaque, QObject *ret_data) +{ + Monitor *mon = (Monitor *)opaque; + monitor_protocol_emitter(mon, ret_data); +} + 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. + */ + *ret_data = qobject_from_jsonf("{ 'async': 'return' }"); + } 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); + } + } +} + +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 */