From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:35765) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rvv2u-0005BV-NE for qemu-devel@nongnu.org; Fri, 10 Feb 2012 13:18:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rvv2t-0000L0-PR for qemu-devel@nongnu.org; Fri, 10 Feb 2012 13:18:08 -0500 Received: from mail-pz0-f45.google.com ([209.85.210.45]:37957) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rvv2t-0000Kk-Kj for qemu-devel@nongnu.org; Fri, 10 Feb 2012 13:18:07 -0500 Received: by dadp14 with SMTP id p14so2908664dad.4 for ; Fri, 10 Feb 2012 10:18:05 -0800 (PST) Message-ID: <4F355F59.6020905@codemonkey.ws> Date: Fri, 10 Feb 2012 12:18:01 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <4F351B73.9020001@codemonkey.ws> <20120210170903.GA475@amit.redhat.com> <4F35524C.5070303@codemonkey.ws> <4F355D24.9030603@redhat.com> In-Reply-To: <4F355D24.9030603@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/1] char: Add a QemuChrHandlers struct to initialise chardev handlers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Amit Shah , qemu list On 02/10/2012 12:08 PM, Kevin Wolf wrote: > Am 10.02.2012 18:22, schrieb Anthony Liguori: >> On 02/10/2012 11:09 AM, Amit Shah wrote: >>> On (Fri) 10 Feb 2012 [07:28:19], Anthony Liguori wrote: >>>> On 02/10/2012 07:19 AM, Amit Shah wrote: >>>>> Instead of passing each handler in the qemu_add_handlers() function, >>>>> create a struct of handlers that can be passed to the function instead. >>>>> >>>>> Signed-off-by: Amit Shah >>>> >>>> Why? >>> >>> It's a good cleanup. >>> >>>> It's not a win in terms of code size. If you plan on introducing >>>> additional handlers, perhaps you should include this in that series >>>> where it's more appropriately justified. >>>> >>>> As a change on it's own, it doesn't seem to make a lot of sense. >>> >>> Makes the code much easier to look at. Can't really compare on code >>> size, since there's zero change in the resulting binary, but the code >>> just becomes more readable and manageable. >> >> It's not more readable IMHO. You've taken function call arguments from the >> place they naturally belong (in the function call) and placed them somewhere else. >> >> More importantly, this isn't a pattern we use in QEMU anywhere. > > The obvious next step is to replace > CharDriverState.chr_can_read/read/event with a CharDriverState.ops that > refers to a CharDriverHandler struct, and suddenly you have a pattern > that is used a lot in QEMU (and that indeed increases readability in my > opinion). I actually think that you need to change chr_can_read/read/event to take CharDriverHandler as the first argument instead of CharDriverState, then drop the handle_opaque and refactor callers appropriately. But then at this stage, we should drop NULL handlers as a disconnect mechanism and switch to a qemu_chr_fe_connect(), qemu_chr_fe_disconnect(). And that indicates that we should call it CharFrontEnd instead of CharDriverHandler. So in such a series, I don't think this patch would even survive. Regards, Anthony Liguori > > Kevin >