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

[Jan Kiszka <jan.kiszka@web.de>]

[-- 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 --]