From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:59028) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R1NmA-0003Oh-Ty for qemu-devel@nongnu.org; Wed, 07 Sep 2011 15:27:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R1Nm9-0001rs-OC for qemu-devel@nongnu.org; Wed, 07 Sep 2011 15:27:10 -0400 Received: from e36.co.us.ibm.com ([32.97.110.154]:41487) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R1Nm9-0001kb-9q for qemu-devel@nongnu.org; Wed, 07 Sep 2011 15:27:09 -0400 Received: from d03relay02.boulder.ibm.com (d03relay02.boulder.ibm.com [9.17.195.227]) by e36.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p87JK5qg011254 for ; Wed, 7 Sep 2011 13:20:05 -0600 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay02.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p87JQeBU024376 for ; Wed, 7 Sep 2011 13:26:41 -0600 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p87DQDVG016181 for ; Wed, 7 Sep 2011 07:26:13 -0600 From: "Aneesh Kumar K.V" In-Reply-To: <4E67BB97.8070306@us.ibm.com> References: <1315398653-29945-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <4E67BB97.8070306@us.ibm.com> Date: Thu, 08 Sep 2011 00:56:27 +0530 Message-ID: <87zkigb1cs.fsf@skywalker.in.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH -V2] iohandler: update qemu_fd_set_handler to work with null call back arg List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi , Avi Kivity On Wed, 07 Sep 2011 13:44:39 -0500, Anthony Liguori wrote: > On 09/07/2011 07:30 AM, Aneesh Kumar K.V wrote: > > Many users of qemu_fd_set_handler including VirtFS use NULL call back arg. > > Update fd_trampoline and qemu_fd_set_handler to work with NULL call back arg > > > > Signed-off-by: Aneesh Kumar K.V > > --- > > Changes from V1: > > Add support for dropping event source > > > > iohandler.c | 31 +++++++++++++------------------ > > 1 files changed, 13 insertions(+), 18 deletions(-) > > > > diff --git a/iohandler.c b/iohandler.c > > index 5ef66fb..783f3ac 100644 > > --- a/iohandler.c > > +++ b/iohandler.c > > @@ -93,9 +93,8 @@ static gboolean fd_trampoline(GIOChannel *chan, GIOCondition cond, gpointer opaq > > { > > IOTrampoline *tramp = opaque; > > > > - if (tramp->opaque == NULL) { > > + if (!tramp->fd_read&& !tramp->fd_write) > > return FALSE; > > - } > > Heh, this is pretty anti-CODING_STYLE :-) > > At any rate, I think this whole segment has to go. It's easier to not > deal with removing sources in two places. > > > > > if ((cond& G_IO_IN)&& tramp->fd_read) { > > tramp->fd_read(tramp->opaque); > > @@ -113,6 +112,7 @@ int qemu_set_fd_handler(int fd, > > IOHandler *fd_write, > > void *opaque) > > { > > + GIOCondition cond = 0; > > static IOTrampoline fd_trampolines[FD_SETSIZE]; > > IOTrampoline *tramp =&fd_trampolines[fd]; > > > > @@ -121,25 +121,20 @@ int qemu_set_fd_handler(int fd, > > g_source_remove(tramp->tag); > > } > > > > - if (opaque) { > > - GIOCondition cond = 0; > > - > > - tramp->fd_read = fd_read; > > - tramp->fd_write = fd_write; > > - tramp->opaque = opaque; > > - > > - if (fd_read) { > > - cond |= G_IO_IN | G_IO_ERR; > > - } > > - > > - if (fd_write) { > > - cond |= G_IO_OUT | G_IO_ERR; > > - } > > + if (fd_read) { > > + cond |= G_IO_IN | G_IO_ERR; > > + } > > > > - tramp->chan = g_io_channel_unix_new(fd); > > - tramp->tag = g_io_add_watch(tramp->chan, cond, fd_trampoline, tramp); > > + if (fd_write) { > > + cond |= G_IO_OUT | G_IO_ERR; > > } > > > > + tramp->fd_read = fd_read; > > + tramp->fd_write = fd_write; > > + tramp->opaque = opaque; > > + tramp->chan = g_io_channel_unix_new(fd); > > + tramp->tag = g_io_add_watch(tramp->chan, cond, fd_trampoline, tramp); > > + > > I think this is a bit more complicated than is really needed. Here's > what I came up with which also fixes another bug where the io channel > could be freed twice. I stumbled across this via a very strange failure > scenario. Avi, it might be worth trying this patch to see if it fixes > your problem too. > > One thing that I found challenging debugging this, coroutines make > valgrind very unhappy. Is it possible that we could have a command line > switch to fall back to the thread based coroutines so to make things > more valgrind friendly? We definitely can look at a configure option that build with coroutine-gthread > > diff --git a/iohandler.c b/iohandler.c > index 5ef66fb..4cc1c5a 100644 > --- a/iohandler.c > +++ b/iohandler.c > @@ -93,10 +93,6 @@ static gboolean fd_trampoline(GIOChannel *chan, > GIOCondition cond, gpointer opaq > { > IOTrampoline *tramp = opaque; > > - if (tramp->opaque == NULL) { > - return FALSE; > - } > - > if ((cond & G_IO_IN) && tramp->fd_read) { > tramp->fd_read(tramp->opaque); > } > @@ -119,9 +115,10 @@ int qemu_set_fd_handler(int fd, > if (tramp->tag != 0) { > g_io_channel_unref(tramp->chan); > g_source_remove(tramp->tag); > + tramp->tag = 0; > } > > - if (opaque) { > + if (fd_read || fd_write || opaque) { > GIOCondition cond = 0; > > tramp->fd_read = fd_read; > I tested above change with virtfs and it works. -aneesh