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

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> 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.

The applicable coding style document is PEP 8.

> 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?

PEP 8:

    A style guide is about consistency.  Consistency with this style
    guide is important.  Consistency within a project is more important.
    Consistency within one module or function is the most important.

In other words, you should make a reasonable effort to blend in.

>> 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.

I'm refusing the bait.

> 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.

First, PEP 8 again:

    Limit all lines to a maximum of 79 characters.

    For flowing long blocks of text with fewer structural restrictions
    (docstrings or comments), the line length should be limited to 72
    characters.

Second, an argument we two had on this list, during review of a prior
version of this patch series, talking about C:

    Legibility.  Humans tend to have trouble following long lines with
    their eyes (I sure do).  Typographic manuals suggest to limit
    columns to roughly 60 characters for exactly that reason[*].

    Code is special.  It's typically indented, and long identifiers push
    it further to the right, function arguments in particular.  We
    compromised at 80 columns.

    [...]

    [*] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style

The width of the line not counting indentation matters for legibility.

The line I flagged as long is 75 characters wide not counting
indentation.  That's needlessly hard to read for me.

PEP 8's line length limit is a *limit*, not a sacred right to push right
to the limit.

Since I get to read this code a lot, I've taken care to avoid illegibly
wide lines, and I've guided contributors to blend in.