Re: [Qemu-devel] [PATCH -V2] iohandler: update qemu_fd_set_handler to work with null call back arg

[Anthony Liguori <aliguori@us.ibm.com>]

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.kumar@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?

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;
>   }
>