From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LWYUH-0007ut-0y for qemu-devel@nongnu.org; Mon, 09 Feb 2009 10:55:57 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LWYUF-0007tm-0H for qemu-devel@nongnu.org; Mon, 09 Feb 2009 10:55:56 -0500 Received: from [199.232.76.173] (port=37911 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LWYUE-0007th-QT for qemu-devel@nongnu.org; Mon, 09 Feb 2009 10:55:54 -0500 Received: from el-out-1112.google.com ([209.85.162.181]:12626) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LWYUE-0005ZF-EI for qemu-devel@nongnu.org; Mon, 09 Feb 2009 10:55:54 -0500 Received: by el-out-1112.google.com with SMTP id y26so965008ele.19 for ; Mon, 09 Feb 2009 07:55:54 -0800 (PST) Message-ID: <499051F0.50103@codemonkey.ws> Date: Mon, 09 Feb 2009 09:55:28 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [PATCH 14/17] monitor: Decouple terminals References: <20090207181627.13667.9979.stgit@mchn012c.ww002.siemens.net> <20090207181629.13667.20945.stgit@mchn012c.ww002.siemens.net> <499048C4.3030603@us.ibm.com> <49904F98.4030509@siemens.com> In-Reply-To: <49904F98.4030509@siemens.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Jan Kiszka wrote: > Anthony Liguori wrote: >> 1) Make monitor_printf() take a monitor state. The easiest thing to do >> would be to introduce this in your previous rename patch making >> everything use current_monitor. >> > > Lazy /me was hoping to get around this... > You are already changing every instance of term_printf(). How hard can it be to add another parameter that gets completely ignored for now :-) The reason I want to see this in this changeset is that you're touching every term_printf(). Adjusting it to the right interface means we don't have to churn again and we don't have the hurdle of touching every term_printf() to get started. >> 2) Introduce current_monitor and default_monitor global variables. They >> map to what you describe above and should be maintained as such. >> 3) Make all monitor callbacks take a monitor state >> 4) Convert monitor_printf()s called from monitor callbacks to use the >> passed monitor state >> 5) Eliminate all uses of current_monitor/default_monitor. >> >> I'd say, 1 and 2 are required for this patchset. I think 3 and 4 would >> be pretty easy to add to your patchset. I think 5 is probably tougher >> and could wait for another day. >> > > My feeling is (though I have not sound stats at hand) that a lot of > functions all over the place will have to be extended in order to pass > the target monitor around: From the command callbacks, through all the > subsystems, finally reaching the monitor API. Yes, my feeling is the same. That's why I suggest an incremental approach. You already have introduced a concept of an active and default terminal. You hide this behind the monitor_printf() function. I'm simply suggesting making it an explicit part of the interface. > Some use cases only > consist of the command callback itself, OK, Yes, these are the low hanging fruit for conversion. > but the others worry me a > bit. All doable, for sure, but I just want to make sure that we all > agree on the result before starting this "tough" endeavor. :) > It's been discussed a lot and there's a strong desire to make the monitor interface better for management tools. I don't expect you to address the whole problem in this one series but, as I said earlier, since you're already making changes everywhere, let's make the right changes at least :-) Regards, Anthony Liguori > Jan > >