From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1OBSmP-0008Ok-Rj for qemu-devel@nongnu.org; Mon, 10 May 2010 09:12:17 -0400 Received: from [140.186.70.92] (port=52037 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OBSmJ-0008Jn-Am for qemu-devel@nongnu.org; Mon, 10 May 2010 09:12:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OBSmA-0000hF-7z for qemu-devel@nongnu.org; Mon, 10 May 2010 09:12:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33268) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OBSmA-0000gN-09 for qemu-devel@nongnu.org; Mon, 10 May 2010 09:12:02 -0400 Date: Mon, 10 May 2010 10:11:52 -0300 From: Luiz Capitulino Subject: Re: [Qemu-devel] Missing mon in monitor_cur_is_qmp() and qerror_report() Message-ID: <20100510101152.59909e2f@redhat.com> In-Reply-To: <4BE7FAA9.4080202@web.de> References: <4BE48C48.5000100@web.de> <4BE7DC23.90100@web.de> <4BE7FAA9.4080202@web.de> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Markus Armbruster , qemu-devel On Mon, 10 May 2010 14:23:05 +0200 Jan Kiszka wrote: > Markus Armbruster wrote: > > Jan Kiszka writes: > > > >> Markus Armbruster wrote: > >>> Jan Kiszka 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. I knew there were problems with cur_mon, but I agree with Markus that functions like error reporting ones should not know where the output is going to. But I feel I didn't spend much time trying to understand the problems with cur_mon, how unreliable is it?