Am 28.09.2020 um 13:42 hat Markus Armbruster geschrieben: > Kevin Wolf <kw...@redhat.com> writes: > > > Am 14.09.2020 um 17:10 hat Markus Armbruster geschrieben: > >> Kevin Wolf <kw...@redhat.com> writes: > >> > >> > The correct way to set the current monitor for a coroutine handler will > >> > be different than for a blocking handler, so monitor_set_cur() needs to > >> > be called in qmp_dispatch(). > >> > > >> > Signed-off-by: Kevin Wolf <kw...@redhat.com> > >> > --- > >> > include/qapi/qmp/dispatch.h | 3 ++- > >> > monitor/qmp.c | 8 +------- > >> > qapi/qmp-dispatch.c | 8 +++++++- > >> > qga/main.c | 2 +- > >> > stubs/monitor-core.c | 5 +++++ > >> > tests/test-qmp-cmds.c | 6 +++--- > >> > 6 files changed, 19 insertions(+), 13 deletions(-) > >> > > >> > diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h > >> > index 5a9cf82472..0c2f467028 100644 > >> > --- a/include/qapi/qmp/dispatch.h > >> > +++ b/include/qapi/qmp/dispatch.h > >> > @@ -14,6 +14,7 @@ > >> > #ifndef QAPI_QMP_DISPATCH_H > >> > #define QAPI_QMP_DISPATCH_H > >> > > >> > +#include "monitor/monitor.h" > >> > #include "qemu/queue.h" > >> > > >> > typedef void (QmpCommandFunc)(QDict *, QObject **, Error **); > >> > @@ -49,7 +50,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); > >> > + bool allow_oob, Monitor *cur_mon); > >> > 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 8469970c69..922fdb5541 100644 > >> > --- a/monitor/qmp.c > >> > +++ b/monitor/qmp.c > >> > @@ -135,16 +135,10 @@ static void monitor_qmp_respond(MonitorQMP *mon, > >> > QDict *rsp) > >> > > >> > static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req) > >> > { > >> > - Monitor *old_mon; > >> > QDict *rsp; > >> > QDict *error; > >> > > >> > - old_mon = monitor_set_cur(&mon->common); > >> > - assert(old_mon == NULL); > >> > - > >> > - rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon)); > >> > - > >> > - monitor_set_cur(NULL); > >> > + rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon), > >> > &mon->common); > >> > >> Long line. Happy to wrap it in my tree. A few more in PATCH 08-11. > > > > It's 79 characters. Should be fine even with your local deviation from > > the coding style to require less than that for comments? > > Let me rephrase my remark. > > For me, > > rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon), > &mon->common); > > is significantly easier to read than > > rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon), > &mon->common);
I guess this is highly subjective. I find wrapped lines harder to read. For answering subjective questions like this, we generally use the coding style document. Anyway, I guess following an idiosyncratic coding style that is different from every other subsystem in QEMU is possible (if inconvenient) if I know what it is. My problem is more that I don't know what the exact rules are. Can they only be figured out experimentally by submitting patches and seeing whether you like them or not? > Would you mind me wrapping this line in my tree? I have no say in this subsystem and I take it that you want all code to look as if you had written it yourself, so do as you wish. But I understand that I'll have to respin anyway, so if you could explain what you're after, I might be able to apply the rules for the next version of the series. Kevin