On Wed, 07 Sep 2011 13:44:39 -0500, Anthony Liguori <aligu...@us.ibm.com> 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<aneesh.ku...@linux.vnet.ibm.com>
> > ---
> > 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
