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

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

From: Hailiang Zhang
Subject: Re: [Qemu-devel] [PATCH] char: Fix removing wrong GSource that be found by fd_in_tag
Date: Fri, 14 Apr 2017 18:43:26 +0800
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 
Hi,

On 2017/4/14 18:10, 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 tied to fix the related codes in commit b43dec, but it didn't

tied->tried

Please use a bit longer commit sha1, or full sha1, it will likely conflict 
otherwise in the future.


OK, I'll replace it with the full sha1.

fix the bug completely, because we still have some codes didn't pass *right*
context parameter for remove_fd_in_watch().

Mixing contexts sounds like a colo-compare bug, did you fix it there too?


Yes, we don't have to change any other codes in colo-compare,
We just call qemu_chr_fe_set_handlers() in colo-compare to detach the old 
GSource
from main default context, and attach the new GSource to the new context.


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

Make sense to me, and even simplify a bit the code. We should be more careful 
with pointers though (the reason why tag existed in the first place I imagine).


Agreed.


Signed-off-by: zhanghailiang <address@hidden>
---
  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..6deb193 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 = 0;

It's a pointer, let's use NULL.


OK. Will fix in next version.

Thanks,
Hailiang

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