Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()
Markus Armbruster writes: > Kevin Wolf 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" ;)
Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()
Kevin Wolf writes:
> Am 30.09.2020 um 19:20 hat Dr. David Alan Gilbert geschrieben:
>> * Kevin Wolf (kw...@redhat.com) wrote:
>> > Am 30.09.2020 um 15:14 hat Markus Armbruster geschrieben:
>> > > Kevin Wolf writes:
>> > >
>> > > > Am 30.09.2020 um 11:26 hat Markus Armbruster geschrieben:
>> > > >> Kevin Wolf writes:
>> > > >>
>> > > >> > Am 28.09.2020 um 13:42 hat Markus Armbruster geschrieben:
>> > > >> >> Kevin Wolf writes:
>> > > >> >>
>> > > >> >> > Am 14.09.2020 um 17:10 hat Markus Armbruster geschrieben:
>> > > >> >> >> Kevin Wolf 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(>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), >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),
>> > > >> >>>common);
>> > > >> >>
>> > > >> >> is significantly easier to read than
>> > > >> >>
>> > > >> >> rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(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
Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()
Am 30.09.2020 um 19:20 hat Dr. David Alan Gilbert geschrieben:
> * Kevin Wolf (kw...@redhat.com) wrote:
> > Am 30.09.2020 um 15:14 hat Markus Armbruster geschrieben:
> > > Kevin Wolf writes:
> > >
> > > > Am 30.09.2020 um 11:26 hat Markus Armbruster geschrieben:
> > > >> Kevin Wolf writes:
> > > >>
> > > >> > Am 28.09.2020 um 13:42 hat Markus Armbruster geschrieben:
> > > >> >> Kevin Wolf writes:
> > > >> >>
> > > >> >> > Am 14.09.2020 um 17:10 hat Markus Armbruster geschrieben:
> > > >> >> >> Kevin Wolf 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(>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), >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),
> > > >> >>>common);
> > > >> >>
> > > >> >> is significantly easier to read than
> > > >> >>
> > > >> >> rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(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
Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()
* Kevin Wolf (kw...@redhat.com) wrote:
> Am 30.09.2020 um 15:14 hat Markus Armbruster geschrieben:
> > Kevin Wolf writes:
> >
> > > Am 30.09.2020 um 11:26 hat Markus Armbruster geschrieben:
> > >> Kevin Wolf writes:
> > >>
> > >> > Am 28.09.2020 um 13:42 hat Markus Armbruster geschrieben:
> > >> >> Kevin Wolf writes:
> > >> >>
> > >> >> > Am 14.09.2020 um 17:10 hat Markus Armbruster geschrieben:
> > >> >> >> Kevin Wolf 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(>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), >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),
> > >> >>>common);
> > >> >>
> > >> >> is significantly easier to read than
> > >> >>
> > >> >> rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(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
> > >>
Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()
Am 30.09.2020 um 15:14 hat Markus Armbruster geschrieben:
> Kevin Wolf writes:
>
> > Am 30.09.2020 um 11:26 hat Markus Armbruster geschrieben:
> >> Kevin Wolf writes:
> >>
> >> > Am 28.09.2020 um 13:42 hat Markus Armbruster geschrieben:
> >> >> Kevin Wolf writes:
> >> >>
> >> >> > Am 14.09.2020 um 17:10 hat Markus Armbruster geschrieben:
> >> >> >> Kevin Wolf 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(>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),
> >> >> >> > >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),
> >> >>>common);
> >> >>
> >> >> is significantly easier to read than
> >> >>
> >> >> rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(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
Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()
Kevin Wolf writes:
> Am 30.09.2020 um 11:26 hat Markus Armbruster geschrieben:
>> Kevin Wolf writes:
>>
>> > Am 28.09.2020 um 13:42 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf writes:
>> >>
>> >> > Am 14.09.2020 um 17:10 hat Markus Armbruster geschrieben:
>> >> >> Kevin Wolf 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(>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),
>> >> >> > >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),
>> >>>common);
>> >>
>> >> is significantly easier to read than
>> >>
>> >> rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(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.
Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()
Am 30.09.2020 um 11:26 hat Markus Armbruster geschrieben:
> Kevin Wolf writes:
>
> > Am 28.09.2020 um 13:42 hat Markus Armbruster geschrieben:
> >> Kevin Wolf writes:
> >>
> >> > Am 14.09.2020 um 17:10 hat Markus Armbruster geschrieben:
> >> >> Kevin Wolf 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
> >> >> > ---
> >> >> > 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(>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),
> >> >> > >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),
> >>>common);
> >>
> >> is significantly easier to read than
> >>
> >> rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(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
Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()
Kevin Wolf writes:
> Am 28.09.2020 um 13:42 hat Markus Armbruster geschrieben:
>> Kevin Wolf writes:
>>
>> > Am 14.09.2020 um 17:10 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf 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
>> >> > ---
>> >> > 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(>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),
>> >> > >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),
>>>common);
>>
>> is significantly easier to read than
>>
>> rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(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
Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()
Am 28.09.2020 um 13:42 hat Markus Armbruster geschrieben:
> Kevin Wolf writes:
>
> > Am 14.09.2020 um 17:10 hat Markus Armbruster geschrieben:
> >> Kevin Wolf 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
> >> > ---
> >> > 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(>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),
> >> > >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),
>>common);
>
> is significantly easier to read than
>
> rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(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
Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()
Kevin Wolf writes:
> Am 14.09.2020 um 17:10 hat Markus Armbruster geschrieben:
>> Kevin Wolf 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
>> > ---
>> > 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(>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),
>> > >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),
>common);
is significantly easier to read than
rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon), >common);
Would you mind me wrapping this line in my tree?
A few more in PATCH 08-11.
Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()
Am 14.09.2020 um 17:10 hat Markus Armbruster geschrieben:
> Kevin Wolf 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
> > ---
> > 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(>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),
> > >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
Re: [PATCH v7 06/13] qmp: Call monitor_set_cur() only in qmp_dispatch()
Kevin Wolf 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
> ---
> 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(>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),
> >common);
Long line. Happy to wrap it in my tree. A few more in PATCH 08-11.
>
> if (mon->commands == _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, , );
> +
> +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(_commands, obj, false);
> +rsp = qmp_dispatch(_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.
[...]
