From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:42973) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R1N7R-0007Pp-P2 for qemu-devel@nongnu.org; Wed, 07 Sep 2011 14:45:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R1N7Q-0000va-Hy for qemu-devel@nongnu.org; Wed, 07 Sep 2011 14:45:05 -0400 Received: from e34.co.us.ibm.com ([32.97.110.152]:57224) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R1N7Q-0000uN-C4 for qemu-devel@nongnu.org; Wed, 07 Sep 2011 14:45:04 -0400 Received: from d03relay05.boulder.ibm.com (d03relay05.boulder.ibm.com [9.17.195.107]) by e34.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p87IitJc016640 for ; Wed, 7 Sep 2011 12:44:55 -0600 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay05.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p87IigGG121082 for ; Wed, 7 Sep 2011 12:44:43 -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 p87CiEO6021740 for ; Wed, 7 Sep 2011 06:44:15 -0600 Message-ID: <4E67BB97.8070306@us.ibm.com> Date: Wed, 07 Sep 2011 13:44:39 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1315398653-29945-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com> In-Reply-To: <1315398653-29945-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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: "Aneesh Kumar K.V" Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi , Avi Kivity 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? 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; Regards, Anthony Liguori > return 0; > } >