qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2] char: Fix removing wrong GSource that be fou

From: Hailiang Zhang
Subject: Re: [Qemu-devel] [PATCH v2] char: Fix removing wrong GSource that be found by fd_in_tag
Date: Wed, 19 Apr 2017 08:42:42 +0800
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 
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 <address@hidden>

looks good to me:
Reviewed-by: Marc-André Lureau <address@hidden>



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




.
reply via email to
[Prev in Thread] Current Thread [Next in Thread]