Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()

[Kevin Wolf]

Am 28.09.2020 um 13:42 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 14.09.2020 um 17:10 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@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 <kwolf@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