From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=51984 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PE0X5-0003SN-DU for qemu-devel@nongnu.org; Thu, 04 Nov 2010 10:11:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PE0K0-0002CP-BG for qemu-devel@nongnu.org; Thu, 04 Nov 2010 09:57:45 -0400 Received: from e8.ny.us.ibm.com ([32.97.182.138]:48408) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PE0K0-0002BV-8h for qemu-devel@nongnu.org; Thu, 04 Nov 2010 09:57:44 -0400 Received: from d01relay05.pok.ibm.com (d01relay05.pok.ibm.com [9.56.227.237]) by e8.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id oA4Deevs025868 for ; Thu, 4 Nov 2010 09:40:40 -0400 Received: from d01av01.pok.ibm.com (d01av01.pok.ibm.com [9.56.224.215]) by d01relay05.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id oA4DvI04155394 for ; Thu, 4 Nov 2010 09:57:18 -0400 Received: from d01av01.pok.ibm.com (loopback [127.0.0.1]) by d01av01.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id oA4DvGqU009731 for ; Thu, 4 Nov 2010 09:57:18 -0400 Message-ID: <4CD2BBBA.3070002@linux.vnet.ibm.com> Date: Thu, 04 Nov 2010 08:57:14 -0500 From: Michael Roth MIME-Version: 1.0 References: <1288798090-7127-1-git-send-email-mdroth@linux.vnet.ibm.com> <1288798090-7127-3-git-send-email-mdroth@linux.vnet.ibm.com> <1288824456.2846.15.camel@aglitke> In-Reply-To: <1288824456.2846.15.camel@aglitke> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [RFC][RESEND][PATCH v1 02/15] virtproxy: qemu-vp, standalone daemon skeleton List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Adam Litke Cc: abeekhof@redhat.com, agl@linux.vnet.ibm.com, qemu-devel@nongnu.org, aliguori@linux.vnet.ibm.com On 11/03/2010 05:47 PM, Adam Litke wrote: > On Wed, 2010-11-03 at 10:27 -0500, Michael Roth wrote: >> +/* mirror qemu I/O-related code for standalone daemon */ >> +typedef struct IOHandlerRecord { >> + int fd; >> + IOCanReadHandler *fd_read_poll; >> + IOHandler *fd_read; >> + IOHandler *fd_write; >> + int deleted; >> + void *opaque; >> + /* temporary data */ >> + struct pollfd *ufd; >> + QLIST_ENTRY(IOHandlerRecord) next; >> +} IOHandlerRecord; > > As you say, this is exactly the same structure as defined in vl.c. If > you copy and paste code for a new feature you should be looking for a > way to share it instead. Why not create a separate patch that defines > this structure in vl.h which can then be included by vl.c and virtproxy > code. > >> +static QLIST_HEAD(, IOHandlerRecord) io_handlers = >> + QLIST_HEAD_INITIALIZER(io_handlers); >> + >> +int vp_set_fd_handler2(int fd, >> + IOCanReadHandler *fd_read_poll, >> + IOHandler *fd_read, >> + IOHandler *fd_write, >> + void *opaque) >> +{ >> + IOHandlerRecord *ioh; >> + >> + if (!fd_read&& !fd_write) { >> + QLIST_FOREACH(ioh,&io_handlers, next) { >> + if (ioh->fd == fd) { >> + ioh->deleted = 1; >> + break; >> + } >> + } >> + } else { >> + QLIST_FOREACH(ioh,&io_handlers, next) { >> + if (ioh->fd == fd) >> + goto found; >> + } >> + ioh = qemu_mallocz(sizeof(IOHandlerRecord)); >> + QLIST_INSERT_HEAD(&io_handlers, ioh, next); >> + found: >> + ioh->fd = fd; >> + ioh->fd_read_poll = fd_read_poll; >> + ioh->fd_read = fd_read; >> + ioh->fd_write = fd_write; >> + ioh->opaque = opaque; >> + ioh->deleted = 0; >> + } >> + return 0; >> +} > > Copied from vl.c -- export it instead. > >> +int vp_set_fd_handler(int fd, >> + IOHandler *fd_read, >> + IOHandler *fd_write, >> + void *opaque) >> +{ >> + return vp_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque); >> +} >> + >> +int vp_send_all(int fd, const void *buf, int len1) >> +{ >> + int ret, len; >> + >> + len = len1; >> + while (len> 0) { >> + ret = write(fd, buf, len); >> + if (ret< 0) { >> + if (errno != EINTR&& errno != EAGAIN) { >> + warn("write() failed"); >> + return -1; >> + } >> + } else if (ret == 0) { >> + break; >> + } else { >> + buf += ret; >> + len -= ret; >> + } >> + } >> + return len1 - len; >> +} > > Copied from qemu-char.c -- Share please. > >> +static void main_loop_wait(int nonblocking) >> +{ >> + IOHandlerRecord *ioh; >> + fd_set rfds, wfds, xfds; >> + int ret, nfds; >> + struct timeval tv; >> + int timeout = 1000; >> + >> + if (nonblocking) { >> + timeout = 0; >> + } >> + >> + /* poll any events */ >> + nfds = -1; >> + FD_ZERO(&rfds); >> + FD_ZERO(&wfds); >> + FD_ZERO(&xfds); >> + QLIST_FOREACH(ioh,&io_handlers, next) { >> + if (ioh->deleted) >> + continue; >> + if (ioh->fd_read&& >> + (!ioh->fd_read_poll || >> + ioh->fd_read_poll(ioh->opaque) != 0)) { >> + FD_SET(ioh->fd,&rfds); >> + if (ioh->fd> nfds) >> + nfds = ioh->fd; >> + } >> + if (ioh->fd_write) { >> + FD_SET(ioh->fd,&wfds); >> + if (ioh->fd> nfds) >> + nfds = ioh->fd; >> + } >> + } >> + >> + tv.tv_sec = timeout / 1000; >> + tv.tv_usec = (timeout % 1000) * 1000; >> + >> + ret = select(nfds + 1,&rfds,&wfds,&xfds,&tv); >> + >> + if (ret> 0) { >> + IOHandlerRecord *pioh; >> + >> + QLIST_FOREACH_SAFE(ioh,&io_handlers, next, pioh) { >> + if (ioh->deleted) { >> + QLIST_REMOVE(ioh, next); >> + qemu_free(ioh); >> + continue; >> + } >> + if (ioh->fd_read&& FD_ISSET(ioh->fd,&rfds)) { >> + ioh->fd_read(ioh->opaque); >> + } >> + if (ioh->fd_write&& FD_ISSET(ioh->fd,&wfds)) { >> + ioh->fd_write(ioh->opaque); >> + } >> + } >> + } >> + > > This resembles vl.c's main_loop_wait() but I can see why you might want > your own. There is opportunity for sharing the select logic and ioh > callbacks but I think that could be addressed later. > Yup these are all basically copy/pastes from vl.c. It feels a bit dirty, but I modeled things after the other qemu tools like qemu-nbd/qemu-io, which don't link against vl.c (and adding a target for tools to do so looks like it'd be a bit hairy since vl.c touches basically everything). It might still make sense to share things like structs...but the ones I'm stealing here are specific to reproducing the main_loop_wait logic. So I guess the real question is whether main_loop_wait and friends make sense to expose as a utility function of some sort, and virtproxy seems to be the only use case so far.