Markus Armbruster wrote:
> Jan Kiszka <jan.kis...@web.de> writes:
> 
>> Markus Armbruster wrote:
>>> Jan Kiszka <jan.kis...@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: signature.asc
Description: OpenPGP digital signature

Reply via email to