| Message ID | 20200909151149.490589-7-kwolf@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | monitor: Optionally run handlers in coroutines | expand |
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. > > 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 79347e0864..2fdbc0fba4 100644 > --- a/qapi/qmp-dispatch.c > +++ b/qapi/qmp-dispatch.c > @@ -89,7 +89,7 @@ bool qmp_is_oob(const QDict *dict) > } > > QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request, > - bool allow_oob) > + bool allow_oob, Monitor *cur_mon) > { > Error *err = NULL; > bool oob; > @@ -152,7 +152,13 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request, > args = qdict_get_qdict(dict, "arguments"); > qobject_ref(args); > } > + > + assert(monitor_cur() == NULL); > + monitor_set_cur(cur_mon); > + > cmd->fn(args, &ret, &err); > + > + monitor_set_cur(NULL); > qobject_unref(args); > if (err) { > /* or assert(!ret) after reviewing all handlers: */ > diff --git a/qga/main.c b/qga/main.c > index 3febf3b0fd..241779a1d6 100644 > --- a/qga/main.c > +++ b/qga/main.c > @@ -578,7 +578,7 @@ static void process_event(void *opaque, QObject *obj, Error *err) > } > > g_debug("processing command"); > - rsp = qmp_dispatch(&ga_commands, obj, false); > + rsp = qmp_dispatch(&ga_commands, obj, false, NULL); > > end: > ret = send_response(s, rsp); I dislike how this entangles qemu-ga even more with the monitor. But I want to get this wrapped more than I want it to be spotless. Perhaps I can clean up some on top. [...]
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? Kevin
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); Would you mind me wrapping this line in my tree? A few more in PATCH 08-11.
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
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.
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
Kevin Wolf <kwolf@redhat.com> writes: > 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: [...] >> >> >> > 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...) I got confused (too much Python code review), my apologies. >> > 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? PEP 8 does not apply to C. > 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. Taste is subjective. We can always make CODING_STYLE.rst more detailed. I view that as a last resort when we waste too much time arguing. Back to line length. CODING_STYLE.rst sets a *limit*. Going over the limit violates CODING_STYLE.rst. There are (rare) cases where that is justified. CODING_STYLE.rst neither demands nor prohibits breaking lines before the limit is reached. Until CODING_STYLE.rst prohibits breaking lines unless they exceed the limit, I will continue to ask for breaking lines when that makes the code easier to read and more consistent with the code around it, for code I maintain, and admittedly in my opinion. These requests appear to irk you a great deal. I don't understand, but I'm sorry about it all the same. By arguing about it repeatedly, you've irked some back. Brought it on myself, I guess. However, if that's what it takes to keep the code I maintain legible and consistent, I'll pay the price.
Am 30.09.2020 um 15:14 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > 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: > [...] > >> >> >> > 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...) > > I got confused (too much Python code review), my apologies. > > >> > 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? > > PEP 8 does not apply to C. > > > 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. > > Taste is subjective. > > We can always make CODING_STYLE.rst more detailed. I view that as a > last resort when we waste too much time arguing. > > Back to line length. > > CODING_STYLE.rst sets a *limit*. > > Going over the limit violates CODING_STYLE.rst. There are (rare) cases > where that is justified. > > CODING_STYLE.rst neither demands nor prohibits breaking lines before the > limit is reached. > > Until CODING_STYLE.rst prohibits breaking lines unless they exceed the > limit, I will continue to ask for breaking lines when that makes the > code easier to read and more consistent with the code around it, for > code I maintain, and admittedly in my opinion. > > These requests appear to irk you a great deal. I don't understand, but > I'm sorry about it all the same. By arguing about it repeatedly, you've > irked some back. Brought it on myself, I guess. However, if that's > what it takes to keep the code I maintain legible and consistent, I'll > pay the price. I conclude that I'll never be able to submit code that passes your review in the first attempt because I don't know the specific criteria (and you don't seem to know them either before you see the patch). Fine, I'll live with it. It's just one of the things that makes working in your subsystems more frustrating than in others. Kevin
* Kevin Wolf (kwolf@redhat.com) wrote: > Am 30.09.2020 um 15:14 hat Markus Armbruster geschrieben: > > Kevin Wolf <kwolf@redhat.com> writes: > > > > > 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: > > [...] > > >> >> >> > 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...) > > > > I got confused (too much Python code review), my apologies. > > > > >> > 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? > > > > PEP 8 does not apply to C. > > > > > 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. > > > > Taste is subjective. > > > > We can always make CODING_STYLE.rst more detailed. I view that as a > > last resort when we waste too much time arguing. > > > > Back to line length. > > > > CODING_STYLE.rst sets a *limit*. > > > > Going over the limit violates CODING_STYLE.rst. There are (rare) cases > > where that is justified. > > > > CODING_STYLE.rst neither demands nor prohibits breaking lines before the > > limit is reached. > > > > Until CODING_STYLE.rst prohibits breaking lines unless they exceed the > > limit, I will continue to ask for breaking lines when that makes the > > code easier to read and more consistent with the code around it, for > > code I maintain, and admittedly in my opinion. > > > > These requests appear to irk you a great deal. I don't understand, but > > I'm sorry about it all the same. By arguing about it repeatedly, you've > > irked some back. Brought it on myself, I guess. However, if that's > > what it takes to keep the code I maintain legible and consistent, I'll > > pay the price. > > I conclude that I'll never be able to submit code that passes your > review in the first attempt because I don't know the specific criteria > (and you don't seem to know them either before you see the patch). > > Fine, I'll live with it. It's just one of the things that makes working > in your subsystems more frustrating than in others. Hmm, IMHO the thing here is that there's two different things here: a) A CODING_STYLE limit - and personally I use every last character of that when appropriate b) For this particular case, Markus is saying he prefers the wrap there. I don't think I see (b) as incompatible as a preference, but lets be sensible; if it's something you want to change in merge that seems reasonable, if it's something that you ask to change in a respin that's kind of reasonable, just don't hold up a big patch series for an argument over something that's legal in the coding style and isn't particularly offensive! Dave > Kevin
Am 30.09.2020 um 19:20 hat Dr. David Alan Gilbert geschrieben: > * Kevin Wolf (kwolf@redhat.com) wrote: > > Am 30.09.2020 um 15:14 hat Markus Armbruster geschrieben: > > > Kevin Wolf <kwolf@redhat.com> writes: > > > > > > > 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: > > > [...] > > > >> >> >> > 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...) > > > > > > I got confused (too much Python code review), my apologies. > > > > > > >> > 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? > > > > > > PEP 8 does not apply to C. > > > > > > > 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. > > > > > > Taste is subjective. > > > > > > We can always make CODING_STYLE.rst more detailed. I view that as a > > > last resort when we waste too much time arguing. > > > > > > Back to line length. > > > > > > CODING_STYLE.rst sets a *limit*. > > > > > > Going over the limit violates CODING_STYLE.rst. There are (rare) cases > > > where that is justified. > > > > > > CODING_STYLE.rst neither demands nor prohibits breaking lines before the > > > limit is reached. > > > > > > Until CODING_STYLE.rst prohibits breaking lines unless they exceed the > > > limit, I will continue to ask for breaking lines when that makes the > > > code easier to read and more consistent with the code around it, for > > > code I maintain, and admittedly in my opinion. > > > > > > These requests appear to irk you a great deal. I don't understand, but > > > I'm sorry about it all the same. By arguing about it repeatedly, you've > > > irked some back. Brought it on myself, I guess. However, if that's > > > what it takes to keep the code I maintain legible and consistent, I'll > > > pay the price. > > > > I conclude that I'll never be able to submit code that passes your > > review in the first attempt because I don't know the specific criteria > > (and you don't seem to know them either before you see the patch). > > > > Fine, I'll live with it. It's just one of the things that makes working > > in your subsystems more frustrating than in others. > > Hmm, > IMHO the thing here is that there's two different things here: > > a) A CODING_STYLE limit - and personally I use every last character > of that when appropriate > b) For this particular case, Markus is saying he prefers the wrap > there. > > I don't think I see (b) as incompatible as a preference, but lets be > sensible; if it's something you want to change in merge that seems > reasonable, if it's something that you ask to change in a respin that's > kind of reasonable, just don't hold up a big patch series for an > argument over something that's legal in the coding style and isn't > particularly offensive! I'll just change this one in the next version. Changing a single well-known instance not a big problem. It's just unfortunate that there are "A few more in PATCH 08-11" and I don't know how to identify them. So Markus will have to comment again in the next version (as he did in other places in previous versions of this series) and potentially modify my patches while applying to match his taste. The latter is something I try hard to avoid as a maintainer, but I admit this means compromising instead of perfectionism. And the former is just a bit tiring when with every version of a series you get additional comments about style preferences. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 30.09.2020 um 19:20 hat Dr. David Alan Gilbert geschrieben: >> * Kevin Wolf (kwolf@redhat.com) wrote: >> > Am 30.09.2020 um 15:14 hat Markus Armbruster geschrieben: >> > > Kevin Wolf <kwolf@redhat.com> writes: >> > > >> > > > 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: >> > > [...] >> > > >> >> >> > 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...) >> > > >> > > I got confused (too much Python code review), my apologies. >> > > >> > > >> > 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? >> > > >> > > PEP 8 does not apply to C. >> > > >> > > > 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. >> > > >> > > Taste is subjective. >> > > >> > > We can always make CODING_STYLE.rst more detailed. I view that as a >> > > last resort when we waste too much time arguing. >> > > >> > > Back to line length. >> > > >> > > CODING_STYLE.rst sets a *limit*. >> > > >> > > Going over the limit violates CODING_STYLE.rst. There are (rare) cases >> > > where that is justified. >> > > >> > > CODING_STYLE.rst neither demands nor prohibits breaking lines before the >> > > limit is reached. >> > > >> > > Until CODING_STYLE.rst prohibits breaking lines unless they exceed the >> > > limit, I will continue to ask for breaking lines when that makes the >> > > code easier to read and more consistent with the code around it, for >> > > code I maintain, and admittedly in my opinion. >> > > >> > > These requests appear to irk you a great deal. I don't understand, but >> > > I'm sorry about it all the same. By arguing about it repeatedly, you've >> > > irked some back. Brought it on myself, I guess. However, if that's >> > > what it takes to keep the code I maintain legible and consistent, I'll >> > > pay the price. >> > >> > I conclude that I'll never be able to submit code that passes your >> > review in the first attempt because I don't know the specific criteria >> > (and you don't seem to know them either before you see the patch). >> > >> > Fine, I'll live with it. It's just one of the things that makes working >> > in your subsystems more frustrating than in others. >> >> Hmm, >> IMHO the thing here is that there's two different things here: >> >> a) A CODING_STYLE limit - and personally I use every last character >> of that when appropriate >> b) For this particular case, Markus is saying he prefers the wrap >> there. >> >> I don't think I see (b) as incompatible as a preference, but lets be >> sensible; if it's something you want to change in merge that seems >> reasonable, if it's something that you ask to change in a respin that's >> kind of reasonable, just don't hold up a big patch series for an >> argument over something that's legal in the coding style and isn't >> particularly offensive! I don't think I ever asked for a respin just to adjust style. I always offer to adjust style myself in my tree. If a respin is needed for some other reason, also making the style adjustments I requested is courteous and appreciated. I don't think I ever rejected a patch just due to differences over style. If a patch submitter refused to make the style adjustments I want, and refused to permit me to make them, I'd commit as is, then maye adjust on top. This is hypothetical. > I'll just change this one in the next version. Changing a single > well-known instance not a big problem. It's just unfortunate that there > are "A few more in PATCH 08-11" and I don't know how to identify them. When I do that, and you'd rather have a complete list, just ask. Out of time for today, but I can get it for you first thing tomorrow. [...]
Markus Armbruster <armbru@redhat.com> writes: > Kevin Wolf <kwolf@redhat.com> writes: [...] >> I'll just change this one in the next version. Changing a single >> well-known instance not a big problem. It's just unfortunate that there >> are "A few more in PATCH 08-11" and I don't know how to identify them. > > When I do that, and you'd rather have a complete list, just ask. Out of > time for today, but I can get it for you first thing tomorrow. > > [...] Done, with the detail level cranked up to "lots" ;)
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); 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 79347e0864..2fdbc0fba4 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -89,7 +89,7 @@ bool qmp_is_oob(const QDict *dict) } QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request, - bool allow_oob) + bool allow_oob, Monitor *cur_mon) { Error *err = NULL; bool oob; @@ -152,7 +152,13 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject *request, args = qdict_get_qdict(dict, "arguments"); qobject_ref(args); } + + assert(monitor_cur() == NULL); + monitor_set_cur(cur_mon); + cmd->fn(args, &ret, &err); + + monitor_set_cur(NULL); qobject_unref(args); if (err) { /* or assert(!ret) after reviewing all handlers: */ diff --git a/qga/main.c b/qga/main.c index 3febf3b0fd..241779a1d6 100644 --- a/qga/main.c +++ b/qga/main.c @@ -578,7 +578,7 @@ static void process_event(void *opaque, QObject *obj, Error *err) } g_debug("processing command"); - rsp = qmp_dispatch(&ga_commands, obj, false); + rsp = qmp_dispatch(&ga_commands, obj, false, NULL); end: ret = send_response(s, rsp); diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c index 0cd2d864b2..dc1748bf13 100644 --- a/stubs/monitor-core.c +++ b/stubs/monitor-core.c @@ -8,6 +8,11 @@ Monitor *monitor_cur(void) return NULL; } +Monitor *monitor_set_cur(Monitor *mon) +{ + return NULL; +} + void monitor_init_qmp(Chardev *chr, bool pretty, Error **errp) { } diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c index d12ff47e26..5f1b245e19 100644 --- a/tests/test-qmp-cmds.c +++ b/tests/test-qmp-cmds.c @@ -152,7 +152,7 @@ static QObject *do_qmp_dispatch(bool allow_oob, const char *template, ...) req = qdict_from_vjsonf_nofail(template, ap); va_end(ap); - resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob); + resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob, NULL); g_assert(resp); ret = qdict_get(resp, "return"); g_assert(ret); @@ -175,7 +175,7 @@ static void do_qmp_dispatch_error(bool allow_oob, ErrorClass cls, req = qdict_from_vjsonf_nofail(template, ap); va_end(ap); - resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob); + resp = qmp_dispatch(&qmp_commands, QOBJECT(req), allow_oob, NULL); g_assert(resp); error = qdict_get_qdict(resp, "error"); g_assert(error); @@ -231,7 +231,7 @@ static void test_dispatch_cmd_success_response(void) QDict *resp; qdict_put_str(req, "execute", "cmd-success-response"); - resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false); + resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false, NULL); g_assert_null(resp); qobject_unref(req); }
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(-)