Hi On Tue, Aug 2, 2022 at 3:04 PM Markus Armbruster wrote: > marcandre.lureau@redhat.com writes: > > > From: Marc-André Lureau > > > > Make QMP-dispatch code free from QEMU-specific OOB dispatch/async > > coroutine handling. This will allow to move the base code to > > qemu-common, and clear other users from potential mis-ususe (QGA doesn't > misuse :) > have OOB or coroutine). > > I trust the utilty of such a move will become clear later in this > series. > > > > > To do that, introduce an optional callback QmpDispatchRun called when a > > QMP command should be run, to allow QEMU to override the default > > behaviour. > > > > Signed-off-by: Marc-André Lureau > > --- > > include/qapi/qmp/dispatch.h | 7 ++-- > > monitor/qmp.c | 68 ++++++++++++++++++++++++++++++++++++- > > qapi/qmp-dispatch.c | 64 +++------------------------------- > > qga/main.c | 2 +- > > tests/unit/test-qmp-cmds.c | 6 ++-- > > 5 files changed, 81 insertions(+), 66 deletions(-) > > > > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h > > index 1e4240fd0dbc..b659da613f2e 100644 > > --- a/include/qapi/qmp/dispatch.h > > +++ b/include/qapi/qmp/dispatch.h > > @@ -14,7 +14,6 @@ > > #ifndef QAPI_QMP_DISPATCH_H > > #define QAPI_QMP_DISPATCH_H > > > > -#include "monitor/monitor.h" > > #include "qemu/queue.h" > > > > typedef void (QmpCommandFunc)(QDict *, QObject **, Error **); > > @@ -41,6 +40,10 @@ typedef struct QmpCommand > > > > typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList; > > > > +typedef void (QmpDispatchRun)(bool oob, const QmpCommand *cmd, > > + QDict *args, QObject **ret, Error **errp, > > + void *run_data); > > + > > void qmp_register_command(QmpCommandList *cmds, const char *name, > > QmpCommandFunc *fn, QmpCommandOptions options, > > unsigned special_features); > > @@ -56,7 +59,7 @@ const char *qmp_command_name(const QmpCommand *cmd); > > bool qmp_has_success_response(const QmpCommand *cmd); > > QDict *qmp_error_response(Error *err); > > QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request, > > - bool allow_oob, Monitor *cur_mon); > > + bool allow_oob, QmpDispatchRun run_cb, void > *run_data); > > bool qmp_is_oob(const QDict *dict); > > > > typedef void (*qmp_cmd_callback_fn)(const QmpCommand *cmd, void > *opaque); > > diff --git a/monitor/qmp.c b/monitor/qmp.c > > index 092c527b6fc9..f8dec97c96bb 100644 > > --- a/monitor/qmp.c > > +++ b/monitor/qmp.c > > @@ -132,6 +132,72 @@ static void monitor_qmp_respond(MonitorQMP *mon, > QDict *rsp) > > } > > } > > > > +typedef struct QmpDispatchBH { > > + const QmpCommand *cmd; > > + Monitor *cur_mon; > > + QDict *args; > > + QObject **ret; > > + Error **errp; > > + Coroutine *co; > > +} QmpDispatchBH; > > + > > +static void do_qmp_dispatch_bh(void *opaque) > > +{ > > + QmpDispatchBH *data = opaque; > > + > > + assert(monitor_cur() == NULL); > > + monitor_set_cur(qemu_coroutine_self(), data->cur_mon); > > + data->cmd->fn(data->args, data->ret, data->errp); > > + monitor_set_cur(qemu_coroutine_self(), NULL); > > + aio_co_wake(data->co); > > +} > > + > > +/* > > + * Runs outside of coroutine context for OOB commands, but in coroutine > > + * context for everything else. > > + */ > > +static void qmp_dispatch_run(bool oob, const QmpCommand *cmd, > > + QDict *args, QObject **ret, Error **errp, > > + void *run_data) > > +{ > > + Monitor *cur_mon = run_data; > > + > > + assert(!(oob && qemu_in_coroutine())); > > + assert(monitor_cur() == NULL); > > + > > + if (!!(cmd->options & QCO_COROUTINE) == qemu_in_coroutine()) { > > + monitor_set_cur(qemu_coroutine_self(), cur_mon); > > + cmd->fn(args, ret, errp); > > + monitor_set_cur(qemu_coroutine_self(), NULL); > > + } else { > > + /* > > + * Actual context doesn't match the one the command needs. > > + * > > + * Case 1: we are in coroutine context, but command does not > > + * have QCO_COROUTINE. We need to drop out of coroutine > > + * context for executing it. > > + * > > + * Case 2: we are outside coroutine context, but command has > > + * QCO_COROUTINE. Can't actually happen, because we get here > > + * outside coroutine context only when executing a command > > + * out of band, and OOB commands never have QCO_COROUTINE. > > + */ > > + assert(!oob && qemu_in_coroutine() && !(cmd->options & > QCO_COROUTINE)); > > + > > + QmpDispatchBH data = { > > + .cur_mon = cur_mon, > > + .cmd = cmd, > > + .args = args, > > + .ret = ret, > > + .errp = errp, > > + .co = qemu_coroutine_self(), > > + }; > > + aio_bh_schedule_oneshot(qemu_get_aio_context(), > do_qmp_dispatch_bh, > > + &data); > > + qemu_coroutine_yield(); > > + } > > +} > > + > > /* > > * Runs outside of coroutine context for OOB commands, but in > > * coroutine context for everything else. > > @@ -142,7 +208,7 @@ static void monitor_qmp_dispatch(MonitorQMP *mon, > QObject *req) > > QDict *error; > > > > rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon), > > - &mon->common); > > + qmp_dispatch_run, &mon->common); > > > > if (mon->commands == &qmp_cap_negotiation_commands) { > > error = qdict_get_qdict(rsp, "error"); > > diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c > > index 0990873ec8ec..342b13d7ebbd 100644 > > --- a/qapi/qmp-dispatch.c > > +++ b/qapi/qmp-dispatch.c > > @@ -13,7 +13,6 @@ > > > > #include "qemu/osdep.h" > > > > -#include "block/aio.h" > > #include "qapi/compat-policy.h" > > #include "qapi/error.h" > > #include "qapi/qmp/dispatch.h" > > @@ -22,8 +21,6 @@ > > #include "qapi/qobject-input-visitor.h" > > #include "qapi/qobject-output-visitor.h" > > #include "qapi/qmp/qbool.h" > > -#include "qemu/coroutine.h" > > -#include "qemu/main-loop.h" > > > > Visitor *qobject_input_visitor_new_qmp(QObject *obj) > > { > > @@ -110,32 +107,8 @@ bool qmp_is_oob(const QDict *dict) > > && !qdict_haskey(dict, "execute"); > > } > > > > -typedef struct QmpDispatchBH { > > - const QmpCommand *cmd; > > - Monitor *cur_mon; > > - QDict *args; > > - QObject **ret; > > - Error **errp; > > - Coroutine *co; > > -} QmpDispatchBH; > > - > > -static void do_qmp_dispatch_bh(void *opaque) > > -{ > > - QmpDispatchBH *data = opaque; > > - > > - assert(monitor_cur() == NULL); > > - monitor_set_cur(qemu_coroutine_self(), data->cur_mon); > > - data->cmd->fn(data->args, data->ret, data->errp); > > - monitor_set_cur(qemu_coroutine_self(), NULL); > > - aio_co_wake(data->co); > > -} > > - > > -/* > > - * Runs outside of coroutine context for OOB commands, but in coroutine > > - * context for everything else. > > - */ > > QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request, > > - bool allow_oob, Monitor *cur_mon) > > + bool allow_oob, QmpDispatchRun run_cb, void > *run_data) > > { > > Error *err = NULL; > > bool oob; > > @@ -203,39 +176,12 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, > QObject *request, > > qobject_ref(args); > > } > > > > - assert(!(oob && qemu_in_coroutine())); > > - assert(monitor_cur() == NULL); > > - if (!!(cmd->options & QCO_COROUTINE) == qemu_in_coroutine()) { > > - monitor_set_cur(qemu_coroutine_self(), cur_mon); > > - cmd->fn(args, &ret, &err); > > - monitor_set_cur(qemu_coroutine_self(), NULL); > > + if (run_cb) { > > + run_cb(oob, cmd, args, &ret, &err, run_data); > > } else { > > - /* > > - * Actual context doesn't match the one the command needs. > > - * > > - * Case 1: we are in coroutine context, but command does not > > - * have QCO_COROUTINE. We need to drop out of coroutine > > - * context for executing it. > > - * > > - * Case 2: we are outside coroutine context, but command has > > - * QCO_COROUTINE. Can't actually happen, because we get here > > - * outside coroutine context only when executing a command > > - * out of band, and OOB commands never have QCO_COROUTINE. > > - */ > > - assert(!oob && qemu_in_coroutine() && !(cmd->options & > QCO_COROUTINE)); > > - > > - QmpDispatchBH data = { > > - .cur_mon = cur_mon, > > - .cmd = cmd, > > - .args = args, > > - .ret = &ret, > > - .errp = &err, > > - .co = qemu_coroutine_self(), > > - }; > > - aio_bh_schedule_oneshot(qemu_get_aio_context(), > do_qmp_dispatch_bh, > > - &data); > > - qemu_coroutine_yield(); > > + cmd->fn(args, &ret, &err); > > } > > + > > qobject_unref(args); > > if (err) { > > /* or assert(!ret) after reviewing all handlers: */ > > A callback works, but note that each program's function is fixed (the > simple and common function is inlined, but that's just for convenience). > > We could use the linker instead. We already do for > qmp_command_available(), and the patch doesn't change that. > Tbh, using the linker override trick makes me a bit uncomfortable when trying to make a "common" qemu library. The "trick" is not well documented (I couldn't find a good reference for the expected behaviour, and my experience with it isn't great when I struggled with linking issues earlier). It also makes the library usage a bit hidden. And it limits the full potential of the library to static linking. Callbacks are not always meant to be dynamically changeable. > Perhaps a layering argument could be made for callbacks. Before the > series, monitor/qmp.c's monitor_qmp_dispatch() calls > qapi/qmp-dispatch.c's qmp_dispatch(), which calls a few functions from > monitor/. However, consistency seems desirable. > > What do you think? > No strong opinion, as long as the qemu-common project is internal to qemu projects. If we imagine the code can be made into a shared library, it will need callbacks. -- Marc-André Lureau