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.
