[Qemu-devel] [PATCH v2] char: Fix removing wrong GSource that be found by fd_in_tag

zhanghailiang posted 1 patch 6 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1492428140-1968-1-git-send-email-zhang.zhanghailiang@huawei.com
Test checkpatch passed
Test docker passed
Test s390x passed
There is a newer version of this series
chardev/char-fd.c     |  8 ++++----
chardev/char-io.c     | 23 ++++++++---------------
chardev/char-io.h     |  4 ++--
chardev/char-pty.c    |  6 +++---
chardev/char-socket.c |  8 ++++----
chardev/char-udp.c    |  8 ++++----
chardev/char.c        |  2 +-
include/sysemu/char.h |  2 +-
8 files changed, 27 insertions(+), 34 deletions(-)
[Qemu-devel] [PATCH v2] char: Fix removing wrong GSource that be found by fd_in_tag
Posted by zhanghailiang 6 years, 11 months ago
We use fd_in_tag to find a GSource, fd_in_tag is return value of
g_source_attach(GSource *source, GMainContext *context), the return
value is unique only in the same context, so we may get the same
values with different 'context' parameters.

It is no problem to find the right fd_in_tag by using
 g_main_context_find_source_by_id(GMainContext *context, guint source_id)
while there is only one default main context.

But colo-compare tries to create/use its own context, and if we pass wrong
'context' parameter with right fd_in_tag, we will find a wrong GSource
to handle. We tried to fix the related codes in commit b43dec, but it didn't
fix the bug completely, because we still have some codes didn't pass *right*
context parameter for remove_fd_in_watch().

Let's fix it by record the GSource directly instead of fd_in_tag.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
v2:
- Fix minor comments from Marc-André Lureau
---
 chardev/char-fd.c     |  8 ++++----
 chardev/char-io.c     | 23 ++++++++---------------
 chardev/char-io.h     |  4 ++--
 chardev/char-pty.c    |  6 +++---
 chardev/char-socket.c |  8 ++++----
 chardev/char-udp.c    |  8 ++++----
 chardev/char.c        |  2 +-
 include/sysemu/char.h |  2 +-
 8 files changed, 27 insertions(+), 34 deletions(-)

diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index 548dd4c..7f0169d 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -58,7 +58,7 @@ static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
     ret = qio_channel_read(
         chan, (gchar *)buf, len, NULL);
     if (ret == 0) {
-        remove_fd_in_watch(chr, NULL);
+        remove_fd_in_watch(chr);
         qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
         return FALSE;
     }
@@ -89,9 +89,9 @@ static void fd_chr_update_read_handler(Chardev *chr,
 {
     FDChardev *s = FD_CHARDEV(chr);
 
-    remove_fd_in_watch(chr, NULL);
+    remove_fd_in_watch(chr);
     if (s->ioc_in) {
-        chr->fd_in_tag = io_add_watch_poll(chr, s->ioc_in,
+        chr->chr_gsource = io_add_watch_poll(chr, s->ioc_in,
                                            fd_chr_read_poll,
                                            fd_chr_read, chr,
                                            context);
@@ -103,7 +103,7 @@ static void char_fd_finalize(Object *obj)
     Chardev *chr = CHARDEV(obj);
     FDChardev *s = FD_CHARDEV(obj);
 
-    remove_fd_in_watch(chr, NULL);
+    remove_fd_in_watch(chr);
     if (s->ioc_in) {
         object_unref(OBJECT(s->ioc_in));
     }
diff --git a/chardev/char-io.c b/chardev/char-io.c
index b4bb094..d781ad6 100644
--- a/chardev/char-io.c
+++ b/chardev/char-io.c
@@ -98,7 +98,7 @@ static GSourceFuncs io_watch_poll_funcs = {
     .finalize = io_watch_poll_finalize,
 };
 
-guint io_add_watch_poll(Chardev *chr,
+GSource *io_add_watch_poll(Chardev *chr,
                         QIOChannel *ioc,
                         IOCanReadHandler *fd_can_read,
                         QIOChannelFunc fd_read,
@@ -106,7 +106,6 @@ guint io_add_watch_poll(Chardev *chr,
                         GMainContext *context)
 {
     IOWatchPoll *iwp;
-    int tag;
     char *name;
 
     iwp = (IOWatchPoll *) g_source_new(&io_watch_poll_funcs,
@@ -122,21 +121,15 @@ guint io_add_watch_poll(Chardev *chr,
     g_source_set_name((GSource *)iwp, name);
     g_free(name);
 
-    tag = g_source_attach(&iwp->parent, context);
+    g_source_attach(&iwp->parent, context);
     g_source_unref(&iwp->parent);
-    return tag;
+    return (GSource *)iwp;
 }
 
-static void io_remove_watch_poll(guint tag, GMainContext *context)
+static void io_remove_watch_poll(GSource *source)
 {
-    GSource *source;
     IOWatchPoll *iwp;
 
-    g_return_if_fail(tag > 0);
-
-    source = g_main_context_find_source_by_id(context, tag);
-    g_return_if_fail(source != NULL);
-
     iwp = io_watch_poll_from_source(source);
     if (iwp->src) {
         g_source_destroy(iwp->src);
@@ -146,11 +139,11 @@ static void io_remove_watch_poll(guint tag, GMainContext *context)
     g_source_destroy(&iwp->parent);
 }
 
-void remove_fd_in_watch(Chardev *chr, GMainContext *context)
+void remove_fd_in_watch(Chardev *chr)
 {
-    if (chr->fd_in_tag) {
-        io_remove_watch_poll(chr->fd_in_tag, context);
-        chr->fd_in_tag = 0;
+    if (chr->chr_gsource) {
+        io_remove_watch_poll(chr->chr_gsource);
+        chr->chr_gsource = NULL;
     }
 }
 
diff --git a/chardev/char-io.h b/chardev/char-io.h
index 842be56..55973a7 100644
--- a/chardev/char-io.h
+++ b/chardev/char-io.h
@@ -29,14 +29,14 @@
 #include "sysemu/char.h"
 
 /* Can only be used for read */
-guint io_add_watch_poll(Chardev *chr,
+GSource *io_add_watch_poll(Chardev *chr,
                         QIOChannel *ioc,
                         IOCanReadHandler *fd_can_read,
                         QIOChannelFunc fd_read,
                         gpointer user_data,
                         GMainContext *context);
 
-void remove_fd_in_watch(Chardev *chr, GMainContext *context);
+void remove_fd_in_watch(Chardev *chr);
 
 int io_channel_send(QIOChannel *ioc, const void *buf, size_t len);
 
diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index a6337be..561926f 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -199,7 +199,7 @@ static void pty_chr_state(Chardev *chr, int connected)
             g_source_remove(s->open_tag);
             s->open_tag = 0;
         }
-        remove_fd_in_watch(chr, NULL);
+        remove_fd_in_watch(chr);
         s->connected = 0;
         /* (re-)connect poll interval for idle guests: once per second.
          * We check more frequently in case the guests sends data to
@@ -215,8 +215,8 @@ static void pty_chr_state(Chardev *chr, int connected)
             s->connected = 1;
             s->open_tag = g_idle_add(qemu_chr_be_generic_open_func, chr);
         }
-        if (!chr->fd_in_tag) {
-            chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
+        if (!chr->chr_gsource) {
+            chr->chr_gsource = io_add_watch_poll(chr, s->ioc,
                                                pty_chr_read_poll,
                                                pty_chr_read,
                                                chr, NULL);
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 36ab0d6..376a70b 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -327,7 +327,7 @@ static void tcp_chr_free_connection(Chardev *chr)
     }
 
     tcp_set_msgfds(chr, NULL, 0);
-    remove_fd_in_watch(chr, NULL);
+    remove_fd_in_watch(chr);
     object_unref(OBJECT(s->sioc));
     s->sioc = NULL;
     object_unref(OBJECT(s->ioc));
@@ -484,7 +484,7 @@ static void tcp_chr_connect(void *opaque)
 
     s->connected = 1;
     if (s->ioc) {
-        chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
+        chr->chr_gsource = io_add_watch_poll(chr, s->ioc,
                                            tcp_chr_read_poll,
                                            tcp_chr_read,
                                            chr, NULL);
@@ -501,9 +501,9 @@ static void tcp_chr_update_read_handler(Chardev *chr,
         return;
     }
 
-    remove_fd_in_watch(chr, NULL);
+    remove_fd_in_watch(chr);
     if (s->ioc) {
-        chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
+        chr->chr_gsource = io_add_watch_poll(chr, s->ioc,
                                            tcp_chr_read_poll,
                                            tcp_chr_read, chr,
                                            context);
diff --git a/chardev/char-udp.c b/chardev/char-udp.c
index 804bd22..9d050bc 100644
--- a/chardev/char-udp.c
+++ b/chardev/char-udp.c
@@ -81,7 +81,7 @@ static gboolean udp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
     ret = qio_channel_read(
         s->ioc, (char *)s->buf, sizeof(s->buf), NULL);
     if (ret <= 0) {
-        remove_fd_in_watch(chr, NULL);
+        remove_fd_in_watch(chr);
         return FALSE;
     }
     s->bufcnt = ret;
@@ -101,9 +101,9 @@ static void udp_chr_update_read_handler(Chardev *chr,
 {
     UdpChardev *s = UDP_CHARDEV(chr);
 
-    remove_fd_in_watch(chr, NULL);
+    remove_fd_in_watch(chr);
     if (s->ioc) {
-        chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
+        chr->chr_gsource = io_add_watch_poll(chr, s->ioc,
                                            udp_chr_read_poll,
                                            udp_chr_read, chr,
                                            context);
@@ -115,7 +115,7 @@ static void char_udp_finalize(Object *obj)
     Chardev *chr = CHARDEV(obj);
     UdpChardev *s = UDP_CHARDEV(obj);
 
-    remove_fd_in_watch(chr, NULL);
+    remove_fd_in_watch(chr);
     if (s->ioc) {
         object_unref(OBJECT(s->ioc));
     }
diff --git a/chardev/char.c b/chardev/char.c
index 3df1163..54cd5f4 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -560,7 +560,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
     cc = CHARDEV_GET_CLASS(s);
     if (!opaque && !fd_can_read && !fd_read && !fd_event) {
         fe_open = 0;
-        remove_fd_in_watch(s, context);
+        remove_fd_in_watch(s);
     } else {
         fe_open = 1;
     }
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 450881d..e157f96 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -93,7 +93,7 @@ struct Chardev {
     char *filename;
     int logfd;
     int be_open;
-    guint fd_in_tag;
+    GSource *chr_gsource;
     DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
     QTAILQ_ENTRY(Chardev) next;
 };
-- 
1.8.3.1



Re: [Qemu-devel] [PATCH v2] char: Fix removing wrong GSource that be found by fd_in_tag
Posted by Marc-André Lureau 6 years, 11 months ago
Hi

----- Original Message -----
> We use fd_in_tag to find a GSource, fd_in_tag is return value of
> g_source_attach(GSource *source, GMainContext *context), the return
> value is unique only in the same context, so we may get the same
> values with different 'context' parameters.
> 
> It is no problem to find the right fd_in_tag by using
>  g_main_context_find_source_by_id(GMainContext *context, guint source_id)
> while there is only one default main context.
> 
> But colo-compare tries to create/use its own context, and if we pass wrong
> 'context' parameter with right fd_in_tag, we will find a wrong GSource
> to handle. We tried to fix the related codes in commit b43dec, but it didn't
> fix the bug completely, because we still have some codes didn't pass *right*
> context parameter for remove_fd_in_watch().
> 
> Let's fix it by record the GSource directly instead of fd_in_tag.

You could simply name it "gsource" or "source" instead of "chr_gsource" to avoid redundancy in chr->chr_gsource.

> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>

looks good to me:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
> v2:
> - Fix minor comments from Marc-André Lureau
> ---
>  chardev/char-fd.c     |  8 ++++----
>  chardev/char-io.c     | 23 ++++++++---------------
>  chardev/char-io.h     |  4 ++--
>  chardev/char-pty.c    |  6 +++---
>  chardev/char-socket.c |  8 ++++----
>  chardev/char-udp.c    |  8 ++++----
>  chardev/char.c        |  2 +-
>  include/sysemu/char.h |  2 +-
>  8 files changed, 27 insertions(+), 34 deletions(-)
> 
> diff --git a/chardev/char-fd.c b/chardev/char-fd.c
> index 548dd4c..7f0169d 100644
> --- a/chardev/char-fd.c
> +++ b/chardev/char-fd.c
> @@ -58,7 +58,7 @@ static gboolean fd_chr_read(QIOChannel *chan, GIOCondition
> cond, void *opaque)
>      ret = qio_channel_read(
>          chan, (gchar *)buf, len, NULL);
>      if (ret == 0) {
> -        remove_fd_in_watch(chr, NULL);
> +        remove_fd_in_watch(chr);
>          qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>          return FALSE;
>      }
> @@ -89,9 +89,9 @@ static void fd_chr_update_read_handler(Chardev *chr,
>  {
>      FDChardev *s = FD_CHARDEV(chr);
>  
> -    remove_fd_in_watch(chr, NULL);
> +    remove_fd_in_watch(chr);
>      if (s->ioc_in) {
> -        chr->fd_in_tag = io_add_watch_poll(chr, s->ioc_in,
> +        chr->chr_gsource = io_add_watch_poll(chr, s->ioc_in,
>                                             fd_chr_read_poll,
>                                             fd_chr_read, chr,
>                                             context);
> @@ -103,7 +103,7 @@ static void char_fd_finalize(Object *obj)
>      Chardev *chr = CHARDEV(obj);
>      FDChardev *s = FD_CHARDEV(obj);
>  
> -    remove_fd_in_watch(chr, NULL);
> +    remove_fd_in_watch(chr);
>      if (s->ioc_in) {
>          object_unref(OBJECT(s->ioc_in));
>      }
> diff --git a/chardev/char-io.c b/chardev/char-io.c
> index b4bb094..d781ad6 100644
> --- a/chardev/char-io.c
> +++ b/chardev/char-io.c
> @@ -98,7 +98,7 @@ static GSourceFuncs io_watch_poll_funcs = {
>      .finalize = io_watch_poll_finalize,
>  };
>  
> -guint io_add_watch_poll(Chardev *chr,
> +GSource *io_add_watch_poll(Chardev *chr,
>                          QIOChannel *ioc,
>                          IOCanReadHandler *fd_can_read,
>                          QIOChannelFunc fd_read,
> @@ -106,7 +106,6 @@ guint io_add_watch_poll(Chardev *chr,
>                          GMainContext *context)
>  {
>      IOWatchPoll *iwp;
> -    int tag;
>      char *name;
>  
>      iwp = (IOWatchPoll *) g_source_new(&io_watch_poll_funcs,
> @@ -122,21 +121,15 @@ guint io_add_watch_poll(Chardev *chr,
>      g_source_set_name((GSource *)iwp, name);
>      g_free(name);
>  
> -    tag = g_source_attach(&iwp->parent, context);
> +    g_source_attach(&iwp->parent, context);
>      g_source_unref(&iwp->parent);
> -    return tag;
> +    return (GSource *)iwp;
>  }
>  
> -static void io_remove_watch_poll(guint tag, GMainContext *context)
> +static void io_remove_watch_poll(GSource *source)
>  {
> -    GSource *source;
>      IOWatchPoll *iwp;
>  
> -    g_return_if_fail(tag > 0);
> -
> -    source = g_main_context_find_source_by_id(context, tag);
> -    g_return_if_fail(source != NULL);
> -
>      iwp = io_watch_poll_from_source(source);
>      if (iwp->src) {
>          g_source_destroy(iwp->src);
> @@ -146,11 +139,11 @@ static void io_remove_watch_poll(guint tag,
> GMainContext *context)
>      g_source_destroy(&iwp->parent);
>  }
>  
> -void remove_fd_in_watch(Chardev *chr, GMainContext *context)
> +void remove_fd_in_watch(Chardev *chr)
>  {
> -    if (chr->fd_in_tag) {
> -        io_remove_watch_poll(chr->fd_in_tag, context);
> -        chr->fd_in_tag = 0;
> +    if (chr->chr_gsource) {
> +        io_remove_watch_poll(chr->chr_gsource);
> +        chr->chr_gsource = NULL;
>      }
>  }
>  
> diff --git a/chardev/char-io.h b/chardev/char-io.h
> index 842be56..55973a7 100644
> --- a/chardev/char-io.h
> +++ b/chardev/char-io.h
> @@ -29,14 +29,14 @@
>  #include "sysemu/char.h"
>  
>  /* Can only be used for read */
> -guint io_add_watch_poll(Chardev *chr,
> +GSource *io_add_watch_poll(Chardev *chr,
>                          QIOChannel *ioc,
>                          IOCanReadHandler *fd_can_read,
>                          QIOChannelFunc fd_read,
>                          gpointer user_data,
>                          GMainContext *context);
>  
> -void remove_fd_in_watch(Chardev *chr, GMainContext *context);
> +void remove_fd_in_watch(Chardev *chr);
>  
>  int io_channel_send(QIOChannel *ioc, const void *buf, size_t len);
>  
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index a6337be..561926f 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -199,7 +199,7 @@ static void pty_chr_state(Chardev *chr, int connected)
>              g_source_remove(s->open_tag);
>              s->open_tag = 0;
>          }
> -        remove_fd_in_watch(chr, NULL);
> +        remove_fd_in_watch(chr);
>          s->connected = 0;
>          /* (re-)connect poll interval for idle guests: once per second.
>           * We check more frequently in case the guests sends data to
> @@ -215,8 +215,8 @@ static void pty_chr_state(Chardev *chr, int connected)
>              s->connected = 1;
>              s->open_tag = g_idle_add(qemu_chr_be_generic_open_func, chr);
>          }
> -        if (!chr->fd_in_tag) {
> -            chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
> +        if (!chr->chr_gsource) {
> +            chr->chr_gsource = io_add_watch_poll(chr, s->ioc,
>                                                 pty_chr_read_poll,
>                                                 pty_chr_read,
>                                                 chr, NULL);
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 36ab0d6..376a70b 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -327,7 +327,7 @@ static void tcp_chr_free_connection(Chardev *chr)
>      }
>  
>      tcp_set_msgfds(chr, NULL, 0);
> -    remove_fd_in_watch(chr, NULL);
> +    remove_fd_in_watch(chr);
>      object_unref(OBJECT(s->sioc));
>      s->sioc = NULL;
>      object_unref(OBJECT(s->ioc));
> @@ -484,7 +484,7 @@ static void tcp_chr_connect(void *opaque)
>  
>      s->connected = 1;
>      if (s->ioc) {
> -        chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
> +        chr->chr_gsource = io_add_watch_poll(chr, s->ioc,
>                                             tcp_chr_read_poll,
>                                             tcp_chr_read,
>                                             chr, NULL);
> @@ -501,9 +501,9 @@ static void tcp_chr_update_read_handler(Chardev *chr,
>          return;
>      }
>  
> -    remove_fd_in_watch(chr, NULL);
> +    remove_fd_in_watch(chr);
>      if (s->ioc) {
> -        chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
> +        chr->chr_gsource = io_add_watch_poll(chr, s->ioc,
>                                             tcp_chr_read_poll,
>                                             tcp_chr_read, chr,
>                                             context);
> diff --git a/chardev/char-udp.c b/chardev/char-udp.c
> index 804bd22..9d050bc 100644
> --- a/chardev/char-udp.c
> +++ b/chardev/char-udp.c
> @@ -81,7 +81,7 @@ static gboolean udp_chr_read(QIOChannel *chan, GIOCondition
> cond, void *opaque)
>      ret = qio_channel_read(
>          s->ioc, (char *)s->buf, sizeof(s->buf), NULL);
>      if (ret <= 0) {
> -        remove_fd_in_watch(chr, NULL);
> +        remove_fd_in_watch(chr);
>          return FALSE;
>      }
>      s->bufcnt = ret;
> @@ -101,9 +101,9 @@ static void udp_chr_update_read_handler(Chardev *chr,
>  {
>      UdpChardev *s = UDP_CHARDEV(chr);
>  
> -    remove_fd_in_watch(chr, NULL);
> +    remove_fd_in_watch(chr);
>      if (s->ioc) {
> -        chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
> +        chr->chr_gsource = io_add_watch_poll(chr, s->ioc,
>                                             udp_chr_read_poll,
>                                             udp_chr_read, chr,
>                                             context);
> @@ -115,7 +115,7 @@ static void char_udp_finalize(Object *obj)
>      Chardev *chr = CHARDEV(obj);
>      UdpChardev *s = UDP_CHARDEV(obj);
>  
> -    remove_fd_in_watch(chr, NULL);
> +    remove_fd_in_watch(chr);
>      if (s->ioc) {
>          object_unref(OBJECT(s->ioc));
>      }
> diff --git a/chardev/char.c b/chardev/char.c
> index 3df1163..54cd5f4 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -560,7 +560,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
>      cc = CHARDEV_GET_CLASS(s);
>      if (!opaque && !fd_can_read && !fd_read && !fd_event) {
>          fe_open = 0;
> -        remove_fd_in_watch(s, context);
> +        remove_fd_in_watch(s);
>      } else {
>          fe_open = 1;
>      }
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 450881d..e157f96 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -93,7 +93,7 @@ struct Chardev {
>      char *filename;
>      int logfd;
>      int be_open;
> -    guint fd_in_tag;
> +    GSource *chr_gsource;
>      DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
>      QTAILQ_ENTRY(Chardev) next;
>  };
> --
> 1.8.3.1
> 
> 
> 

Re: [Qemu-devel] [PATCH v2] char: Fix removing wrong GSource that be found by fd_in_tag
Posted by Hailiang Zhang 6 years, 11 months ago
On 2017/4/18 16:11, Marc-André Lureau wrote:
> Hi
>
> ----- Original Message -----
>> We use fd_in_tag to find a GSource, fd_in_tag is return value of
>> g_source_attach(GSource *source, GMainContext *context), the return
>> value is unique only in the same context, so we may get the same
>> values with different 'context' parameters.
>>
>> It is no problem to find the right fd_in_tag by using
>>   g_main_context_find_source_by_id(GMainContext *context, guint source_id)
>> while there is only one default main context.
>>
>> But colo-compare tries to create/use its own context, and if we pass wrong
>> 'context' parameter with right fd_in_tag, we will find a wrong GSource
>> to handle. We tried to fix the related codes in commit b43dec, but it didn't
>> fix the bug completely, because we still have some codes didn't pass *right*
>> context parameter for remove_fd_in_watch().
>>
>> Let's fix it by record the GSource directly instead of fd_in_tag.
> You could simply name it "gsource" or "source" instead of "chr_gsource" to avoid redundancy in chr->chr_gsource.

OK, besides, i forgot to update the hash, I'll  fix both of them in V3, thanks.

>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> looks good to me:
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>
>> ---
>> v2:
>> - Fix minor comments from Marc-André Lureau
>> ---
>>   chardev/char-fd.c     |  8 ++++----
>>   chardev/char-io.c     | 23 ++++++++---------------
>>   chardev/char-io.h     |  4 ++--
>>   chardev/char-pty.c    |  6 +++---
>>   chardev/char-socket.c |  8 ++++----
>>   chardev/char-udp.c    |  8 ++++----
>>   chardev/char.c        |  2 +-
>>   include/sysemu/char.h |  2 +-
>>   8 files changed, 27 insertions(+), 34 deletions(-)
>>
>> diff --git a/chardev/char-fd.c b/chardev/char-fd.c
>> index 548dd4c..7f0169d 100644
>> --- a/chardev/char-fd.c
>> +++ b/chardev/char-fd.c
>> @@ -58,7 +58,7 @@ static gboolean fd_chr_read(QIOChannel *chan, GIOCondition
>> cond, void *opaque)
>>       ret = qio_channel_read(
>>           chan, (gchar *)buf, len, NULL);
>>       if (ret == 0) {
>> -        remove_fd_in_watch(chr, NULL);
>> +        remove_fd_in_watch(chr);
>>           qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>>           return FALSE;
>>       }
>> @@ -89,9 +89,9 @@ static void fd_chr_update_read_handler(Chardev *chr,
>>   {
>>       FDChardev *s = FD_CHARDEV(chr);
>>   
>> -    remove_fd_in_watch(chr, NULL);
>> +    remove_fd_in_watch(chr);
>>       if (s->ioc_in) {
>> -        chr->fd_in_tag = io_add_watch_poll(chr, s->ioc_in,
>> +        chr->chr_gsource = io_add_watch_poll(chr, s->ioc_in,
>>                                              fd_chr_read_poll,
>>                                              fd_chr_read, chr,
>>                                              context);
>> @@ -103,7 +103,7 @@ static void char_fd_finalize(Object *obj)
>>       Chardev *chr = CHARDEV(obj);
>>       FDChardev *s = FD_CHARDEV(obj);
>>   
>> -    remove_fd_in_watch(chr, NULL);
>> +    remove_fd_in_watch(chr);
>>       if (s->ioc_in) {
>>           object_unref(OBJECT(s->ioc_in));
>>       }
>> diff --git a/chardev/char-io.c b/chardev/char-io.c
>> index b4bb094..d781ad6 100644
>> --- a/chardev/char-io.c
>> +++ b/chardev/char-io.c
>> @@ -98,7 +98,7 @@ static GSourceFuncs io_watch_poll_funcs = {
>>       .finalize = io_watch_poll_finalize,
>>   };
>>   
>> -guint io_add_watch_poll(Chardev *chr,
>> +GSource *io_add_watch_poll(Chardev *chr,
>>                           QIOChannel *ioc,
>>                           IOCanReadHandler *fd_can_read,
>>                           QIOChannelFunc fd_read,
>> @@ -106,7 +106,6 @@ guint io_add_watch_poll(Chardev *chr,
>>                           GMainContext *context)
>>   {
>>       IOWatchPoll *iwp;
>> -    int tag;
>>       char *name;
>>   
>>       iwp = (IOWatchPoll *) g_source_new(&io_watch_poll_funcs,
>> @@ -122,21 +121,15 @@ guint io_add_watch_poll(Chardev *chr,
>>       g_source_set_name((GSource *)iwp, name);
>>       g_free(name);
>>   
>> -    tag = g_source_attach(&iwp->parent, context);
>> +    g_source_attach(&iwp->parent, context);
>>       g_source_unref(&iwp->parent);
>> -    return tag;
>> +    return (GSource *)iwp;
>>   }
>>   
>> -static void io_remove_watch_poll(guint tag, GMainContext *context)
>> +static void io_remove_watch_poll(GSource *source)
>>   {
>> -    GSource *source;
>>       IOWatchPoll *iwp;
>>   
>> -    g_return_if_fail(tag > 0);
>> -
>> -    source = g_main_context_find_source_by_id(context, tag);
>> -    g_return_if_fail(source != NULL);
>> -
>>       iwp = io_watch_poll_from_source(source);
>>       if (iwp->src) {
>>           g_source_destroy(iwp->src);
>> @@ -146,11 +139,11 @@ static void io_remove_watch_poll(guint tag,
>> GMainContext *context)
>>       g_source_destroy(&iwp->parent);
>>   }
>>   
>> -void remove_fd_in_watch(Chardev *chr, GMainContext *context)
>> +void remove_fd_in_watch(Chardev *chr)
>>   {
>> -    if (chr->fd_in_tag) {
>> -        io_remove_watch_poll(chr->fd_in_tag, context);
>> -        chr->fd_in_tag = 0;
>> +    if (chr->chr_gsource) {
>> +        io_remove_watch_poll(chr->chr_gsource);
>> +        chr->chr_gsource = NULL;
>>       }
>>   }
>>   
>> diff --git a/chardev/char-io.h b/chardev/char-io.h
>> index 842be56..55973a7 100644
>> --- a/chardev/char-io.h
>> +++ b/chardev/char-io.h
>> @@ -29,14 +29,14 @@
>>   #include "sysemu/char.h"
>>   
>>   /* Can only be used for read */
>> -guint io_add_watch_poll(Chardev *chr,
>> +GSource *io_add_watch_poll(Chardev *chr,
>>                           QIOChannel *ioc,
>>                           IOCanReadHandler *fd_can_read,
>>                           QIOChannelFunc fd_read,
>>                           gpointer user_data,
>>                           GMainContext *context);
>>   
>> -void remove_fd_in_watch(Chardev *chr, GMainContext *context);
>> +void remove_fd_in_watch(Chardev *chr);
>>   
>>   int io_channel_send(QIOChannel *ioc, const void *buf, size_t len);
>>   
>> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
>> index a6337be..561926f 100644
>> --- a/chardev/char-pty.c
>> +++ b/chardev/char-pty.c
>> @@ -199,7 +199,7 @@ static void pty_chr_state(Chardev *chr, int connected)
>>               g_source_remove(s->open_tag);
>>               s->open_tag = 0;
>>           }
>> -        remove_fd_in_watch(chr, NULL);
>> +        remove_fd_in_watch(chr);
>>           s->connected = 0;
>>           /* (re-)connect poll interval for idle guests: once per second.
>>            * We check more frequently in case the guests sends data to
>> @@ -215,8 +215,8 @@ static void pty_chr_state(Chardev *chr, int connected)
>>               s->connected = 1;
>>               s->open_tag = g_idle_add(qemu_chr_be_generic_open_func, chr);
>>           }
>> -        if (!chr->fd_in_tag) {
>> -            chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
>> +        if (!chr->chr_gsource) {
>> +            chr->chr_gsource = io_add_watch_poll(chr, s->ioc,
>>                                                  pty_chr_read_poll,
>>                                                  pty_chr_read,
>>                                                  chr, NULL);
>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> index 36ab0d6..376a70b 100644
>> --- a/chardev/char-socket.c
>> +++ b/chardev/char-socket.c
>> @@ -327,7 +327,7 @@ static void tcp_chr_free_connection(Chardev *chr)
>>       }
>>   
>>       tcp_set_msgfds(chr, NULL, 0);
>> -    remove_fd_in_watch(chr, NULL);
>> +    remove_fd_in_watch(chr);
>>       object_unref(OBJECT(s->sioc));
>>       s->sioc = NULL;
>>       object_unref(OBJECT(s->ioc));
>> @@ -484,7 +484,7 @@ static void tcp_chr_connect(void *opaque)
>>   
>>       s->connected = 1;
>>       if (s->ioc) {
>> -        chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
>> +        chr->chr_gsource = io_add_watch_poll(chr, s->ioc,
>>                                              tcp_chr_read_poll,
>>                                              tcp_chr_read,
>>                                              chr, NULL);
>> @@ -501,9 +501,9 @@ static void tcp_chr_update_read_handler(Chardev *chr,
>>           return;
>>       }
>>   
>> -    remove_fd_in_watch(chr, NULL);
>> +    remove_fd_in_watch(chr);
>>       if (s->ioc) {
>> -        chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
>> +        chr->chr_gsource = io_add_watch_poll(chr, s->ioc,
>>                                              tcp_chr_read_poll,
>>                                              tcp_chr_read, chr,
>>                                              context);
>> diff --git a/chardev/char-udp.c b/chardev/char-udp.c
>> index 804bd22..9d050bc 100644
>> --- a/chardev/char-udp.c
>> +++ b/chardev/char-udp.c
>> @@ -81,7 +81,7 @@ static gboolean udp_chr_read(QIOChannel *chan, GIOCondition
>> cond, void *opaque)
>>       ret = qio_channel_read(
>>           s->ioc, (char *)s->buf, sizeof(s->buf), NULL);
>>       if (ret <= 0) {
>> -        remove_fd_in_watch(chr, NULL);
>> +        remove_fd_in_watch(chr);
>>           return FALSE;
>>       }
>>       s->bufcnt = ret;
>> @@ -101,9 +101,9 @@ static void udp_chr_update_read_handler(Chardev *chr,
>>   {
>>       UdpChardev *s = UDP_CHARDEV(chr);
>>   
>> -    remove_fd_in_watch(chr, NULL);
>> +    remove_fd_in_watch(chr);
>>       if (s->ioc) {
>> -        chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
>> +        chr->chr_gsource = io_add_watch_poll(chr, s->ioc,
>>                                              udp_chr_read_poll,
>>                                              udp_chr_read, chr,
>>                                              context);
>> @@ -115,7 +115,7 @@ static void char_udp_finalize(Object *obj)
>>       Chardev *chr = CHARDEV(obj);
>>       UdpChardev *s = UDP_CHARDEV(obj);
>>   
>> -    remove_fd_in_watch(chr, NULL);
>> +    remove_fd_in_watch(chr);
>>       if (s->ioc) {
>>           object_unref(OBJECT(s->ioc));
>>       }
>> diff --git a/chardev/char.c b/chardev/char.c
>> index 3df1163..54cd5f4 100644
>> --- a/chardev/char.c
>> +++ b/chardev/char.c
>> @@ -560,7 +560,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
>>       cc = CHARDEV_GET_CLASS(s);
>>       if (!opaque && !fd_can_read && !fd_read && !fd_event) {
>>           fe_open = 0;
>> -        remove_fd_in_watch(s, context);
>> +        remove_fd_in_watch(s);
>>       } else {
>>           fe_open = 1;
>>       }
>> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
>> index 450881d..e157f96 100644
>> --- a/include/sysemu/char.h
>> +++ b/include/sysemu/char.h
>> @@ -93,7 +93,7 @@ struct Chardev {
>>       char *filename;
>>       int logfd;
>>       int be_open;
>> -    guint fd_in_tag;
>> +    GSource *chr_gsource;
>>       DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
>>       QTAILQ_ENTRY(Chardev) next;
>>   };
>> --
>> 1.8.3.1
>>
>>
>>
> .
>