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