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

[Markus Armbruster]

Jan Kiszka <address@hidden> writes:

> Markus Armbruster wrote:
>> Jan Kiszka <address@hidden> writes:
>> 
>>> Markus Armbruster wrote:
>>>> Jan Kiszka <address@hidden> 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.