[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] Missing mon in monitor_cur_is_qmp() and qerror_report()
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] Missing mon in monitor_cur_is_qmp() and qerror_report() |
Date: |
Mon, 10 May 2010 13:14:01 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) |
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.