Hi
On Tue, Aug 2, 2022 at 3:04 PM Markus Armbruster <arm...@redhat.com> wrote:
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
> >
> > 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 <marcandre.lur...@redhat.com>
> > ---
> > 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
