From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=37136 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PPz3c-00060B-Cr for qemu-devel@nongnu.org; Tue, 07 Dec 2010 10:02:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PPz3b-0002J9-4Y for qemu-devel@nongnu.org; Tue, 07 Dec 2010 10:02:20 -0500 Received: from mx1.redhat.com ([209.132.183.28]:44064) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PPz3a-0002Io-Q3 for qemu-devel@nongnu.org; Tue, 07 Dec 2010 10:02:19 -0500 Message-ID: <4CFE4C6B.1010600@redhat.com> Date: Tue, 07 Dec 2010 16:02:03 +0100 From: Jes Sorensen MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions References: <1291399402-20366-1-git-send-email-mdroth@linux.vnet.ibm.com> <1291399402-20366-2-git-send-email-mdroth@linux.vnet.ibm.com> <4CFE3738.4010506@redhat.com> <4CFE4928.5000409@linux.vnet.ibm.com> In-Reply-To: <4CFE4928.5000409@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: agl@linux.vnet.ibm.com, stefanha@linux.vnet.ibm.com, abeekhof@redhat.com, qemu-devel@nongnu.org, aliguori@linux.vnet.ibm.com, ryanh@us.ibm.com On 12/07/10 15:48, Michael Roth wrote: > On 12/07/2010 07:31 AM, Jes Sorensen wrote: >> On 12/03/10 19:03, Michael Roth wrote: >>> This allows us to implement an i/o loop outside of vl.c that can >>> interact with objects that use qemu_set_fd_handler() >>> >>> Signed-off-by: Michael Roth >> >> This commit message really tells us nothing. Please be more specific >> about what is in the commit. >> > > Currently, in qemu, the virtagent client/server functionality is driven > by vl.c:main_loop_wait(), which implements a select() loop that kicks > off handlers registered for various FDs/events via qemu_set_fd_handler(). > > To share the code with the agent, qemu-va.c, I re-implemented this i/o > loop to do same thing, along with vl.c:qemu_set_fd_handler*() and > friends. It was big nasty copy/paste job and I think most of the > reviewers agreed that the i/o loop code should be shared. > > This commit moves the shared code into back-end utility functions that > get called by vl.c:qemu_set_fd_handler()/qemu_process_fd_handlers() and > friends for qemu, and by > qemu-tools.c:qemu_set_fd_handler()/qemu_process_fd_handlers() for tools. > > So now to interact with code that uses qemu_set_fd_handler() you can > built a select() loop around these utility functions. Please put some of this in the commit message. >> I am not happy with this addition of numbers to these functions, it >> doesn't tell us why we have a 3 and how it differs from 2. If 3 is >> really the backend for implementing 2, maybe it would be better to name >> it __qemu_set_fd_handler2() and then have macros/wrappers calling into >> it. > > That was the initial plan, but qemu_set_fd_handler2() is a back-end of > sorts for qemu_set_fd_handler(), so I was just trying to be consistent > with the naming. Personally I don't have any objections one way or the > other. Anything to avoid qemu_set_fd_handler17() at some point. I think using __qemu_set_fd_handler() encourages people to modify that code rather than copy it. Cheers, Jes