[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch() |
Date: |
Wed, 30 Sep 2020 13:29:03 +0200 |
Am 30.09.2020 um 11:26 hat Markus Armbruster geschrieben:
> 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.
I'll happily apply PEP 8 to Python code, but this is C. I don't think
PEP 8 applies very well to C code. (In fact, PEP 7 exists as a C style
guide, but we're not writing C code for the Python project here...)
> > 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.
The project style guide for C is defined in CODING_STYLE.rst. Missing
consistency with it is what I'm complaining about.
I also agree that consistency within one module or function is most
important, which is why I allow you to reformat my code. But I don't
think it means that local coding style rules shouldn't be documented,
especially if you can't just look at the code and see immediately how
it's supposed to be.
> >> 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.
Ok, that's finally clear limits at least.
Any other rules from PEP 8 that you want to see applied to C code?
Would you mind documenting this somewhere?
> 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.
As I said, I don't mind the exact number much. I do mind predictability,
though. (And ideally also consistency across the project because
otherwise I need to change my editor settings for individual files.)
So if you don't like 79 columns, give me any other number. But
please, do give me something specific I can work with. "illegibly wide"
is not something I can work with because it's highly subjective.
Kevin
- Re: [PATCH v7 07/13] monitor: Make current monitor a per-coroutine property, (continued)
[PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch(), Kevin Wolf, 2020/09/09
- Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch(), Markus Armbruster, 2020/09/14
- Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch(), Kevin Wolf, 2020/09/25
- Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch(), Markus Armbruster, 2020/09/28
- Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch(), Kevin Wolf, 2020/09/28
- Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch(), Markus Armbruster, 2020/09/30
- Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch(),
Kevin Wolf <=
- Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch(), Markus Armbruster, 2020/09/30
- Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch(), Kevin Wolf, 2020/09/30
- Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch(), Dr. David Alan Gilbert, 2020/09/30
[PATCH v7 08/13] qapi: Add a 'coroutine' flag for commands, Kevin Wolf, 2020/09/09
[PATCH v7 09/13] qmp: Move dispatcher to a coroutine, Kevin Wolf, 2020/09/09