[Qemu-devel] Missing mon in monitor_cur_is_qmp() and qerror_report()

[Qemu-devel] Missing mon in monitor_cur_is_qmp() and qerror_report()

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 613 bytes --]

Luiz,

I missed this when the API was first proposed:

cur_mon is scheduled for removal (one day...). It's just an intermediate
step to convert all users to explicit 'mon' passing. Thus, new APIs
should not rely it.

I just realized that monitor_cur_is_qmp() does so. It should be
refactored to monitor_is_qmp(Monitor *mon). And qerror should be enhance
by a 'mon' argument as well. Callers that aren't passed a 'mon'
themselves should either be fixed at this chance or could fall back to
cur_mon for the time being.

So far for the theory - do you see any pitfalls in the existing usage?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] Missing mon in monitor_cur_is_qmp() and qerror_report()

[Markus Armbruster]

Jan Kiszka <jan.kiszka@web.de> writes:

> Luiz,
>
> I missed this when the API was first proposed:
>
> cur_mon is scheduled for removal (one day...). It's just an intermediate
> step to convert all users to explicit 'mon' passing. Thus, new APIs
> should not rely it.
>
> I just realized that monitor_cur_is_qmp() does so. It should be
> refactored to monitor_is_qmp(Monitor *mon). And qerror should be enhance
> by a 'mon' argument as well. Callers that aren't passed a 'mon'
> themselves should either be fixed at this chance or could fall back to
> cur_mon for the time being.
>
> So far for the theory - do you see any pitfalls in the existing usage?

I put in the new uses of cur_mon, Luiz "only" ACKed them.

At any point in the program execution, we have one current monitor, or
none.  Passing around the current monitor within monitor code is
workable, if somewhat tedious.  But we need it not just in monitor code,
we need it anywhere where we report errors.  In other words, pretty much
everywhere.  Including places that do not and should not know about the
monitor.  Handing a monitor parameter down pretty much every call chain
is beyond tedious, it's impractical.

The code reporting an error generally does not and should not know
anything about *how* the error gets communicated to the user.
Insulating it from that detail is proper separation of concerns, and
global variable cur_mon is my tool to get it.  Good software
engineering.  Like many powerful tools, global variables should be used
sparingly and with care.  I feel this use is well justified.

Instead of eliminating cur_mon, I'd like it to be hidden within
monitor.c.  There are a few uses left outside it.

Re: [Qemu-devel] Missing mon in monitor_cur_is_qmp() and qerror_report()

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2318 bytes --]

Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@web.de> writes:
> 
>> Luiz,
>>
>> I missed this when the API was first proposed:
>>
>> cur_mon is scheduled for removal (one day...). It's just an intermediate
>> step to convert all users to explicit 'mon' passing. Thus, new APIs
>> should not rely it.
>>
>> I just realized that monitor_cur_is_qmp() does so. It should be
>> refactored to monitor_is_qmp(Monitor *mon). And qerror should be enhance
>> by a 'mon' argument as well. Callers that aren't passed a 'mon'
>> themselves should either be fixed at this chance or could fall back to
>> cur_mon for the time being.
>>
>> So far for the theory - do you see any pitfalls in the existing usage?
> 
> I put in the new uses of cur_mon, Luiz "only" ACKed them.
> 
> At any point in the program execution, we have one current monitor, or
> none.  Passing around the current monitor within monitor code is
> workable, if somewhat tedious.  But we need it not just in monitor code,
> we need it anywhere where we report errors.  In other words, pretty much
> everywhere.  Including places that do not and should not know about the
> monitor.  Handing a monitor parameter down pretty much every call chain
> is beyond tedious, it's impractical.

It's a process, but I don't think it's impractical per se.

> 
> The code reporting an error generally does not and should not know
> anything about *how* the error gets communicated to the user.
> Insulating it from that detail is proper separation of concerns, and
> global variable cur_mon is my tool to get it.  Good software
> engineering.  Like many powerful tools, global variables should be used
> sparingly and with care.  I feel this use is well justified.
> 
> Instead of eliminating cur_mon, I'd like it to be hidden within
> monitor.c.  There are a few uses left outside it.

If we start to allow cur_mon for error reporting, there is no reason not
to convert monitor_printf back to where it came from. Back then we
agreed on the current path. If we now decide to roll back, then let's
make it consistently. But we already refactored quite a lot of code for
explicit monitor passing...

Jan

PS: A patch for establishing monitor_is_qmp is in my queue. Holding it
back for now until we agreed how to proceed.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] Missing mon in monitor_cur_is_qmp() and qerror_report()

[Markus Armbruster]

Jan Kiszka <jan.kiszka@web.de> writes:

> Markus Armbruster wrote:
>> Jan Kiszka <jan.kiszka@web.de> writes:
>> 
>>> Luiz,
>>>
>>> I missed this when the API was first proposed:
>>>
>>> cur_mon is scheduled for removal (one day...). It's just an intermediate
>>> step to convert all users to explicit 'mon' passing. Thus, new APIs
>>> should not rely it.
>>>
>>> I just realized that monitor_cur_is_qmp() does so. It should be
>>> refactored to monitor_is_qmp(Monitor *mon). And qerror should be enhance
>>> by a 'mon' argument as well. Callers that aren't passed a 'mon'
>>> themselves should either be fixed at this chance or could fall back to
>>> cur_mon for the time being.
>>>
>>> So far for the theory - do you see any pitfalls in the existing usage?
>> 
>> I put in the new uses of cur_mon, Luiz "only" ACKed them.
>> 
>> At any point in the program execution, we have one current monitor, or
>> none.  Passing around the current monitor within monitor code is
>> workable, if somewhat tedious.  But we need it not just in monitor code,
>> we need it anywhere where we report errors.  In other words, pretty much
>> everywhere.  Including places that do not and should not know about the
>> monitor.  Handing a monitor parameter down pretty much every call chain
>> is beyond tedious, it's impractical.
>
> It's a process, but I don't think it's impractical per se.
>
>> 
>> The code reporting an error generally does not and should not know
>> anything about *how* the error gets communicated to the user.
>> Insulating it from that detail is proper separation of concerns, and
>> global variable cur_mon is my tool to get it.  Good software
>> engineering.  Like many powerful tools, global variables should be used
>> sparingly and with care.  I feel this use is well justified.
>> 
>> Instead of eliminating cur_mon, I'd like it to be hidden within
>> monitor.c.  There are a few uses left outside it.
>
> If we start to allow cur_mon for error reporting, there is no reason not
> to convert monitor_printf back to where it came from. Back then we
> agreed on the current path. If we now decide to roll back, then let's
> make it consistently.

Makes sense.

>                       But we already refactored quite a lot of code for
> explicit monitor passing...
>
> Jan
>
> PS: A patch for establishing monitor_is_qmp is in my queue. Holding it
> back for now until we agreed how to proceed.

monitor_is_qmp() is used only in a few places.  The real troublemakers
are error_report() & friends, and qerror_report().  These are all over
the place, with more to come.

Re: [Qemu-devel] Missing mon in monitor_cur_is_qmp() and qerror_report()

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3079 bytes --]

Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@web.de> writes:
> 
>> Markus Armbruster wrote:
>>> Jan Kiszka <jan.kiszka@web.de> writes:
>>>
>>>> Luiz,
>>>>
>>>> I missed this when the API was first proposed:
>>>>
>>>> cur_mon is scheduled for removal (one day...). It's just an intermediate
>>>> step to convert all users to explicit 'mon' passing. Thus, new APIs
>>>> should not rely it.
>>>>
>>>> I just realized that monitor_cur_is_qmp() does so. It should be
>>>> refactored to monitor_is_qmp(Monitor *mon). And qerror should be enhance
>>>> by a 'mon' argument as well. Callers that aren't passed a 'mon'
>>>> themselves should either be fixed at this chance or could fall back to
>>>> cur_mon for the time being.
>>>>
>>>> So far for the theory - do you see any pitfalls in the existing usage?
>>> I put in the new uses of cur_mon, Luiz "only" ACKed them.
>>>
>>> At any point in the program execution, we have one current monitor, or
>>> none.  Passing around the current monitor within monitor code is
>>> workable, if somewhat tedious.  But we need it not just in monitor code,
>>> we need it anywhere where we report errors.  In other words, pretty much
>>> everywhere.  Including places that do not and should not know about the
>>> monitor.  Handing a monitor parameter down pretty much every call chain
>>> is beyond tedious, it's impractical.
>> It's a process, but I don't think it's impractical per se.
>>
>>> The code reporting an error generally does not and should not know
>>> anything about *how* the error gets communicated to the user.
>>> Insulating it from that detail is proper separation of concerns, and
>>> global variable cur_mon is my tool to get it.  Good software
>>> engineering.  Like many powerful tools, global variables should be used
>>> sparingly and with care.  I feel this use is well justified.
>>>
>>> Instead of eliminating cur_mon, I'd like it to be hidden within
>>> monitor.c.  There are a few uses left outside it.
>> If we start to allow cur_mon for error reporting, there is no reason not
>> to convert monitor_printf back to where it came from. Back then we
>> agreed on the current path. If we now decide to roll back, then let's
>> make it consistently.
> 
> Makes sense.
> 
>>                       But we already refactored quite a lot of code for
>> explicit monitor passing...
>>
>> Jan
>>
>> PS: A patch for establishing monitor_is_qmp is in my queue. Holding it
>> back for now until we agreed how to proceed.
> 
> monitor_is_qmp() is used only in a few places.  The real troublemakers
> are error_report() & friends, and qerror_report().  These are all over
> the place, with more to come.

Right, therefore we need a quick decision avoid introducing more
[q]error_report users without mon if cur_mon shall not stay.

Just noticed: As long as we rely on cur_mon, user_monitor_complete and
qmp_monitor_complete need to establish this context just link the
command callers. Without this error messages and the qmp test use a
wrong monitor.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] Missing mon in monitor_cur_is_qmp() and qerror_report()

[Luiz Capitulino]

On Mon, 10 May 2010 14:23:05 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:

> Markus Armbruster wrote:
> > Jan Kiszka <jan.kiszka@web.de> writes:
> > 
> >> Markus Armbruster wrote:
> >>> Jan Kiszka <jan.kiszka@web.de> writes:
> >>>
> >>>> Luiz,
> >>>>
> >>>> I missed this when the API was first proposed:
> >>>>
> >>>> cur_mon is scheduled for removal (one day...). It's just an intermediate
> >>>> step to convert all users to explicit 'mon' passing. Thus, new APIs
> >>>> should not rely it.
> >>>>
> >>>> I just realized that monitor_cur_is_qmp() does so. It should be
> >>>> refactored to monitor_is_qmp(Monitor *mon). And qerror should be enhance
> >>>> by a 'mon' argument as well. Callers that aren't passed a 'mon'
> >>>> themselves should either be fixed at this chance or could fall back to
> >>>> cur_mon for the time being.
> >>>>
> >>>> So far for the theory - do you see any pitfalls in the existing usage?
> >>> I put in the new uses of cur_mon, Luiz "only" ACKed them.
> >>>
> >>> At any point in the program execution, we have one current monitor, or
> >>> none.  Passing around the current monitor within monitor code is
> >>> workable, if somewhat tedious.  But we need it not just in monitor code,
> >>> we need it anywhere where we report errors.  In other words, pretty much
> >>> everywhere.  Including places that do not and should not know about the
> >>> monitor.  Handing a monitor parameter down pretty much every call chain
> >>> is beyond tedious, it's impractical.
> >> It's a process, but I don't think it's impractical per se.
> >>
> >>> The code reporting an error generally does not and should not know
> >>> anything about *how* the error gets communicated to the user.
> >>> Insulating it from that detail is proper separation of concerns, and
> >>> global variable cur_mon is my tool to get it.  Good software
> >>> engineering.  Like many powerful tools, global variables should be used
> >>> sparingly and with care.  I feel this use is well justified.
> >>>
> >>> Instead of eliminating cur_mon, I'd like it to be hidden within
> >>> monitor.c.  There are a few uses left outside it.
> >> If we start to allow cur_mon for error reporting, there is no reason not
> >> to convert monitor_printf back to where it came from. Back then we
> >> agreed on the current path. If we now decide to roll back, then let's
> >> make it consistently.
> > 
> > Makes sense.
> > 
> >>                       But we already refactored quite a lot of code for
> >> explicit monitor passing...
> >>
> >> Jan
> >>
> >> PS: A patch for establishing monitor_is_qmp is in my queue. Holding it
> >> back for now until we agreed how to proceed.
> > 
> > monitor_is_qmp() is used only in a few places.  The real troublemakers
> > are error_report() & friends, and qerror_report().  These are all over
> > the place, with more to come.
> 
> Right, therefore we need a quick decision avoid introducing more
> [q]error_report users without mon if cur_mon shall not stay.

 I knew there were problems with cur_mon, but I agree with Markus that
functions like error reporting ones should not know where the output is
going to.

 But I feel I didn't spend much time trying to understand the problems
with cur_mon, how unreliable is it?

Re: [Qemu-devel] Missing mon in monitor_cur_is_qmp() and qerror_report()

[Markus Armbruster]

Jan Kiszka <jan.kiszka@web.de> writes:

> Markus Armbruster wrote:
>> Jan Kiszka <jan.kiszka@web.de> writes:
>> 
>>> Markus Armbruster wrote:
>>>> Jan Kiszka <jan.kiszka@web.de> writes:
>>>>
>>>>> Luiz,
>>>>>
>>>>> I missed this when the API was first proposed:
>>>>>
>>>>> cur_mon is scheduled for removal (one day...). It's just an intermediate
>>>>> step to convert all users to explicit 'mon' passing. Thus, new APIs
>>>>> should not rely it.
>>>>>
>>>>> I just realized that monitor_cur_is_qmp() does so. It should be
>>>>> refactored to monitor_is_qmp(Monitor *mon). And qerror should be enhance
>>>>> by a 'mon' argument as well. Callers that aren't passed a 'mon'
>>>>> themselves should either be fixed at this chance or could fall back to
>>>>> cur_mon for the time being.
>>>>>
>>>>> So far for the theory - do you see any pitfalls in the existing usage?
>>>> I put in the new uses of cur_mon, Luiz "only" ACKed them.
>>>>
>>>> At any point in the program execution, we have one current monitor, or
>>>> none.  Passing around the current monitor within monitor code is
>>>> workable, if somewhat tedious.  But we need it not just in monitor code,
>>>> we need it anywhere where we report errors.  In other words, pretty much
>>>> everywhere.  Including places that do not and should not know about the
>>>> monitor.  Handing a monitor parameter down pretty much every call chain
>>>> is beyond tedious, it's impractical.
>>> It's a process, but I don't think it's impractical per se.
>>>
>>>> The code reporting an error generally does not and should not know
>>>> anything about *how* the error gets communicated to the user.
>>>> Insulating it from that detail is proper separation of concerns, and
>>>> global variable cur_mon is my tool to get it.  Good software
>>>> engineering.  Like many powerful tools, global variables should be used
>>>> sparingly and with care.  I feel this use is well justified.
>>>>
>>>> Instead of eliminating cur_mon, I'd like it to be hidden within
>>>> monitor.c.  There are a few uses left outside it.
>>> If we start to allow cur_mon for error reporting, there is no reason not
>>> to convert monitor_printf back to where it came from. Back then we
>>> agreed on the current path. If we now decide to roll back, then let's
>>> make it consistently.
>> 
>> Makes sense.
>> 
>>>                       But we already refactored quite a lot of code for
>>> explicit monitor passing...
>>>
>>> Jan
>>>
>>> PS: A patch for establishing monitor_is_qmp is in my queue. Holding it
>>> back for now until we agreed how to proceed.
>> 
>> monitor_is_qmp() is used only in a few places.  The real troublemakers
>> are error_report() & friends, and qerror_report().  These are all over
>> the place, with more to come.
>
> Right, therefore we need a quick decision avoid introducing more
> [q]error_report users without mon if cur_mon shall not stay.

We still report errors to stderr in many places that are reachable from
the monitor, and fixing that will add error_report() calls.

qerror_report() replaces error_report() as needed to provide
sufficiently specific errors for QMP.  Not relevant to the issue at
hand, because both error_report() and qerror_report() use cur_mon the
same way.

> Just noticed: As long as we rely on cur_mon, user_monitor_complete and
> qmp_monitor_complete need to establish this context just link the
> command callers. Without this error messages and the qmp test use a
> wrong monitor.

Can they emit errors?  If yes, we have a bug.  Can you give an example?
If no, we may want to set up cur_mon anyway, just for robustness.

Re: [Qemu-devel] Missing mon in monitor_cur_is_qmp() and qerror_report()

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4233 bytes --]

Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@web.de> writes:
> 
>> Markus Armbruster wrote:
>>> Jan Kiszka <jan.kiszka@web.de> writes:
>>>
>>>> Markus Armbruster wrote:
>>>>> Jan Kiszka <jan.kiszka@web.de> writes:
>>>>>
>>>>>> Luiz,
>>>>>>
>>>>>> I missed this when the API was first proposed:
>>>>>>
>>>>>> cur_mon is scheduled for removal (one day...). It's just an intermediate
>>>>>> step to convert all users to explicit 'mon' passing. Thus, new APIs
>>>>>> should not rely it.
>>>>>>
>>>>>> I just realized that monitor_cur_is_qmp() does so. It should be
>>>>>> refactored to monitor_is_qmp(Monitor *mon). And qerror should be enhance
>>>>>> by a 'mon' argument as well. Callers that aren't passed a 'mon'
>>>>>> themselves should either be fixed at this chance or could fall back to
>>>>>> cur_mon for the time being.
>>>>>>
>>>>>> So far for the theory - do you see any pitfalls in the existing usage?
>>>>> I put in the new uses of cur_mon, Luiz "only" ACKed them.
>>>>>
>>>>> At any point in the program execution, we have one current monitor, or
>>>>> none.  Passing around the current monitor within monitor code is
>>>>> workable, if somewhat tedious.  But we need it not just in monitor code,
>>>>> we need it anywhere where we report errors.  In other words, pretty much
>>>>> everywhere.  Including places that do not and should not know about the
>>>>> monitor.  Handing a monitor parameter down pretty much every call chain
>>>>> is beyond tedious, it's impractical.
>>>> It's a process, but I don't think it's impractical per se.
>>>>
>>>>> The code reporting an error generally does not and should not know
>>>>> anything about *how* the error gets communicated to the user.
>>>>> Insulating it from that detail is proper separation of concerns, and
>>>>> global variable cur_mon is my tool to get it.  Good software
>>>>> engineering.  Like many powerful tools, global variables should be used
>>>>> sparingly and with care.  I feel this use is well justified.
>>>>>
>>>>> Instead of eliminating cur_mon, I'd like it to be hidden within
>>>>> monitor.c.  There are a few uses left outside it.
>>>> If we start to allow cur_mon for error reporting, there is no reason not
>>>> to convert monitor_printf back to where it came from. Back then we
>>>> agreed on the current path. If we now decide to roll back, then let's
>>>> make it consistently.
>>> Makes sense.
>>>
>>>>                       But we already refactored quite a lot of code for
>>>> explicit monitor passing...
>>>>
>>>> Jan
>>>>
>>>> PS: A patch for establishing monitor_is_qmp is in my queue. Holding it
>>>> back for now until we agreed how to proceed.
>>> monitor_is_qmp() is used only in a few places.  The real troublemakers
>>> are error_report() & friends, and qerror_report().  These are all over
>>> the place, with more to come.
>> Right, therefore we need a quick decision avoid introducing more
>> [q]error_report users without mon if cur_mon shall not stay.
> 
> We still report errors to stderr in many places that are reachable from
> the monitor, and fixing that will add error_report() calls.

Right as this already breaks when invoked over a monitor.

> 
> qerror_report() replaces error_report() as needed to provide
> sufficiently specific errors for QMP.  Not relevant to the issue at
> hand, because both error_report() and qerror_report() use cur_mon the
> same way.
> 
>> Just noticed: As long as we rely on cur_mon, user_monitor_complete and
>> qmp_monitor_complete need to establish this context just link the
>> command callers. Without this error messages and the qmp test use a
>> wrong monitor.
> 
> Can they emit errors?  If yes, we have a bug.  Can you give an example?
> If no, we may want to set up cur_mon anyway, just for robustness.

Error during migration is one example, there are surely more (at least
in theory - and definitely if we drop mon from monitor_printf again).

And if more of such asynchronous use cases show up in the future, they
all need to be handled with an identical care - similar "tricky" like
getting cpu_single_env right in a multi-threaded environment. I'm not a
big fan of this.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]