[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: |
Jan Kiszka |
Subject: |
Re: [Qemu-devel] Missing mon in monitor_cur_is_qmp() and qerror_report() |
Date: |
Mon, 10 May 2010 15:26:44 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
Markus Armbruster wrote:
> 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.
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
signature.asc
Description: OpenPGP digital signature