qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 0/4] another round of qemu-char fixes
@ 2013-04-19 15:32 Paolo Bonzini
  2013-04-19 15:32 ` [Qemu-devel] [PATCH v2 1/4] qemu-char: use consistent idiom for removing sources Paolo Bonzini
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: Paolo Bonzini @ 2013-04-19 15:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: amit.shah, aliguori, kraxel

These three patches fix the bugs that Gerd reported.

The first patches try to make the code consistent (which should avoid
that half of the drivers bark with CRITICAL messages from glib) and
also fix detection of pty connections so that they do not go berserk
with polling.  This is likely the same failure reported by Michael Hines,
but I couldn't reproduce it with "-serial pty", only with the monitor.

The last patch works around a glib bug that happens on RHEL6 and Wheezy,
where destroying a source from a finalize callback can deadlock the
GMainLoop.  This bug locks up QEMU when a socket chardev's client
disconnects.

I tested the pty fix on F18 and Gerd tested it on RHEL6.  I tested
the socket fix on RHEL6.

I believe these patches let us revert commit 893986f (main-loop: drop
the BQL if the I/O appears to be spinning, 2013-04-05).

Paolo


Paolo Bonzini (4):
  qemu-char: use consistent idiom for removing sources
  qemu-char: simplify pty polling
  qemu-char: correct return value from chr_read functions
  qemu-char: do not operate on sources from finalize callbacks

 qemu-char.c |  134 ++++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 88 insertions(+), 46 deletions(-)

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH v2 1/4] qemu-char: use consistent idiom for removing sources
  2013-04-19 15:32 [Qemu-devel] [PATCH v2 0/4] another round of qemu-char fixes Paolo Bonzini
@ 2013-04-19 15:32 ` Paolo Bonzini
  2013-04-19 15:32 ` [Qemu-devel] [PATCH v2 2/4] qemu-char: simplify pty polling Paolo Bonzini
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2013-04-19 15:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: amit.shah, aliguori, kraxel

Always check that the source is active, and zero the tag afterwards.

The occurrence in pty_chr_state will trigger with the next patch, the
others are just theoretical.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-char.c |   32 +++++++++++++++++++++++---------
 1 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 728ed9b..552a498 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -644,9 +644,11 @@ static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback,
 static void io_watch_poll_finalize(GSource *source)
 {
     IOWatchPoll *iwp = io_watch_poll_from_source(source);
-    g_source_destroy(iwp->src);
-    g_source_unref(iwp->src);
-    iwp->src = NULL;
+    if (iwp->src) {
+        g_source_destroy(iwp->src);
+        g_source_unref(iwp->src);
+        iwp->src = NULL;
+    }
 }
 
 static GSourceFuncs io_watch_poll_funcs = {
@@ -816,6 +818,7 @@ static void fd_chr_update_read_handler(CharDriverState *chr)
 
     if (s->fd_in_tag) {
         g_source_remove(s->fd_in_tag);
+        s->fd_in_tag = 0;
     }
 
     if (s->fd_in) {
@@ -1148,8 +1151,10 @@ static void pty_chr_state(CharDriverState *chr, int connected)
     PtyCharDriver *s = chr->opaque;
 
     if (!connected) {
-        g_source_remove(s->fd_tag);
-        s->fd_tag = 0;
+        if (s->fd_tag) {
+            g_source_remove(s->fd_tag);
+            s->fd_tag = 0;
+        }
         s->connected = 0;
         s->polling = 0;
         /* (re-)connect poll interval for idle guests: once per second.
@@ -1171,12 +1176,14 @@ static void pty_chr_close(struct CharDriverState *chr)
 
     if (s->fd_tag) {
         g_source_remove(s->fd_tag);
+        s->fd_tag = 0;
     }
     fd = g_io_channel_unix_get_fd(s->fd);
     g_io_channel_unref(s->fd);
     close(fd);
     if (s->timer_tag) {
         g_source_remove(s->timer_tag);
+        s->timer_tag = 0;
     }
     g_free(s);
     qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
@@ -2277,6 +2284,7 @@ static void udp_chr_close(CharDriverState *chr)
     NetCharDriver *s = chr->opaque;
     if (s->tag) {
         g_source_remove(s->tag);
+        s->tag = 0;
     }
     if (s->chan) {
         g_io_channel_unref(s->chan);
@@ -2508,8 +2516,10 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
         if (s->listen_chan) {
             s->listen_tag = g_io_add_watch(s->listen_chan, G_IO_IN, tcp_chr_accept, chr);
         }
-        g_source_remove(s->tag);
-        s->tag = 0;
+        if (s->tag) {
+            g_source_remove(s->tag);
+            s->tag = 0;
+        }
         g_io_channel_unref(s->chan);
         s->chan = NULL;
         closesocket(s->fd);
@@ -2570,8 +2580,10 @@ static int tcp_chr_add_client(CharDriverState *chr, int fd)
         socket_set_nodelay(fd);
     s->fd = fd;
     s->chan = io_channel_from_socket(fd);
-    g_source_remove(s->listen_tag);
-    s->listen_tag = 0;
+    if (s->listen_tag) {
+        g_source_remove(s->listen_tag);
+        s->listen_tag = 0;
+    }
     tcp_chr_connect(chr);
 
     return 0;
@@ -2621,6 +2633,7 @@ static void tcp_chr_close(CharDriverState *chr)
     if (s->fd >= 0) {
         if (s->tag) {
             g_source_remove(s->tag);
+            s->tag = 0;
         }
         if (s->chan) {
             g_io_channel_unref(s->chan);
@@ -2630,6 +2643,7 @@ static void tcp_chr_close(CharDriverState *chr)
     if (s->listen_fd >= 0) {
         if (s->listen_tag) {
             g_source_remove(s->listen_tag);
+            s->listen_tag = 0;
         }
         if (s->listen_chan) {
             g_io_channel_unref(s->listen_chan);
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH v2 2/4] qemu-char: simplify pty polling
  2013-04-19 15:32 [Qemu-devel] [PATCH v2 0/4] another round of qemu-char fixes Paolo Bonzini
  2013-04-19 15:32 ` [Qemu-devel] [PATCH v2 1/4] qemu-char: use consistent idiom for removing sources Paolo Bonzini
@ 2013-04-19 15:32 ` Paolo Bonzini
  2013-04-19 15:32 ` [Qemu-devel] [PATCH v2 3/4] qemu-char: correct return value from chr_read functions Paolo Bonzini
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2013-04-19 15:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: amit.shah, aliguori, kraxel

There is no need to use a timer and pty_chr_read to detect a connected
pty.  It is simpler to just call g_poll periodically and check for POLLHUP.
It is done once per second, and only if the pty is disconnected, so it
is cheap enough.

Tested with "-monitor pty" and "-serial mon:pty", both of which work
correctly and do not freeze QEMU.  (How to test ptys?  "socat -,raw,echo=0
/dev/pts/4,raw").

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-char.c |   41 +++++++++++++++++------------------------
 1 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 552a498..d14888d 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1028,7 +1028,6 @@ typedef struct {
     GIOChannel *fd;
     guint fd_tag;
     int connected;
-    int polling;
     int read_bytes;
     guint timer_tag;
 } PtyCharDriver;
@@ -1044,12 +1043,6 @@ static gboolean pty_chr_timer(gpointer opaque)
     if (s->connected) {
         goto out;
     }
-    if (s->polling) {
-        /* If we arrive here without polling being cleared due
-         * read returning -EIO, then we are (re-)connected */
-        pty_chr_state(chr, 1);
-        goto out;
-    }
 
     /* Next poll ... */
     pty_chr_update_read_handler(chr);
@@ -1128,22 +1121,17 @@ static gboolean pty_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
 static void pty_chr_update_read_handler(CharDriverState *chr)
 {
     PtyCharDriver *s = chr->opaque;
+    GPollFD pfd;
 
-    if (s->fd_tag) {
-        g_source_remove(s->fd_tag);
+    pfd.fd = g_io_channel_unix_get_fd(s->fd);
+    pfd.events = G_IO_OUT;
+    pfd.revents = 0;
+    g_poll(&pfd, 1, 0);
+    if (pfd.revents & G_IO_HUP) {
+        pty_chr_state(chr, 0);
+    } else {
+        pty_chr_state(chr, 1);
     }
-
-    s->fd_tag = io_add_watch_poll(s->fd, pty_chr_read_poll, pty_chr_read, chr);
-    s->polling = 1;
-    /*
-     * Short timeout here: just need wait long enougth that qemu makes
-     * it through the poll loop once.  When reconnected we want a
-     * short timeout so we notice it almost instantly.  Otherwise
-     * read() gives us -EIO instantly, making pty_chr_state() reset the
-     * timeout to the normal (much longer) poll interval before the
-     * timer triggers.
-     */
-    pty_chr_rearm_timer(chr, 10);
 }
 
 static void pty_chr_state(CharDriverState *chr, int connected)
@@ -1156,15 +1144,20 @@ static void pty_chr_state(CharDriverState *chr, int connected)
             s->fd_tag = 0;
         }
         s->connected = 0;
-        s->polling = 0;
         /* (re-)connect poll interval for idle guests: once per second.
          * We check more frequently in case the guests sends data to
          * the virtual device linked to our pty. */
         pty_chr_rearm_timer(chr, 1000);
     } else {
-        if (!s->connected)
+        if (s->timer_tag) {
+            g_source_remove(s->timer_tag);
+            s->timer_tag = 0;
+        }
+        if (!s->connected) {
             qemu_chr_be_generic_open(chr);
-        s->connected = 1;
+            s->connected = 1;
+            s->fd_tag = io_add_watch_poll(s->fd, pty_chr_read_poll, pty_chr_read, chr);
+        }
     }
 }
 
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH v2 3/4] qemu-char: correct return value from chr_read functions
  2013-04-19 15:32 [Qemu-devel] [PATCH v2 0/4] another round of qemu-char fixes Paolo Bonzini
  2013-04-19 15:32 ` [Qemu-devel] [PATCH v2 1/4] qemu-char: use consistent idiom for removing sources Paolo Bonzini
  2013-04-19 15:32 ` [Qemu-devel] [PATCH v2 2/4] qemu-char: simplify pty polling Paolo Bonzini
@ 2013-04-19 15:32 ` Paolo Bonzini
  2013-04-19 15:32 ` [Qemu-devel] [PATCH v2 4/4] qemu-char: do not operate on sources from finalize callbacks Paolo Bonzini
  2013-04-22 18:34 ` [Qemu-devel] [PATCH v2 0/4] another round of qemu-char fixes Anthony Liguori
  4 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2013-04-19 15:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: amit.shah, aliguori, kraxel

Even if a CharDriverState's source is blocked by the front-end,
it must not be dropped. The IOWatchPoll that wraps it will take
care of adding and removing it to the main loop.  Only remove
the source when the channel is closed; and in that case, make sure
that the wrapping IOWatchPoll is removed too.

These should just be theoretical bugs.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-char.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index d14888d..6e897da 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -781,12 +781,16 @@ static gboolean fd_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
         len = s->max_size;
     }
     if (len == 0) {
-        return FALSE;
+        return TRUE;
     }
 
     status = g_io_channel_read_chars(chan, (gchar *)buf,
                                      len, &bytes_read, NULL);
     if (status == G_IO_STATUS_EOF) {
+        if (s->fd_in_tag) {
+            g_source_remove(s->fd_in_tag);
+            s->fd_in_tag = 0;
+        }
         qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
         return FALSE;
     }
@@ -1105,8 +1109,9 @@ static gboolean pty_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
     len = sizeof(buf);
     if (len > s->read_bytes)
         len = s->read_bytes;
-    if (len == 0)
-        return FALSE;
+    if (len == 0) {
+        return TRUE;
+    }
     status = g_io_channel_read_chars(s->fd, (gchar *)buf, len, &size, NULL);
     if (status != G_IO_STATUS_NORMAL) {
         pty_chr_state(chr, 0);
@@ -2238,13 +2243,18 @@ static gboolean udp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
     gsize bytes_read = 0;
     GIOStatus status;
 
-    if (s->max_size == 0)
-        return FALSE;
+    if (s->max_size == 0) {
+        return TRUE;
+    }
     status = g_io_channel_read_chars(s->chan, (gchar *)s->buf, sizeof(s->buf),
                                      &bytes_read, NULL);
     s->bufcnt = bytes_read;
     s->bufptr = s->bufcnt;
     if (status != G_IO_STATUS_NORMAL) {
+        if (s->tag) {
+            g_source_remove(s->tag);
+            s->tag = 0;
+        }
         return FALSE;
     }
 
@@ -2497,7 +2507,7 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
     int len, size;
 
     if (!s->connected || s->max_size <= 0) {
-        return FALSE;
+        return TRUE;
     }
     len = sizeof(buf);
     if (len > s->max_size)
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [Qemu-devel] [PATCH v2 4/4] qemu-char: do not operate on sources from finalize callbacks
  2013-04-19 15:32 [Qemu-devel] [PATCH v2 0/4] another round of qemu-char fixes Paolo Bonzini
                   ` (2 preceding siblings ...)
  2013-04-19 15:32 ` [Qemu-devel] [PATCH v2 3/4] qemu-char: correct return value from chr_read functions Paolo Bonzini
@ 2013-04-19 15:32 ` Paolo Bonzini
  2013-04-19 17:12   ` Sander Eikelenboom
  2013-04-22 18:34 ` [Qemu-devel] [PATCH v2 0/4] another round of qemu-char fixes Anthony Liguori
  4 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2013-04-19 15:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: amit.shah, aliguori, kraxel

Due to a glib bug, the finalize callback is called with the GMainContext
lock held.  Thus, any operation on the context from the callback will
cause recursive locking and a deadlock.  This happens, for example,
when a client disconnects from a socket chardev.

The fix for this is somewhat ugly, because we need to forego polymorphism
and implement our own function to destroy IOWatchPoll sources.  The
right thing to do here would be child sources, but we support older
glib versions that do not have them.  Not coincidentially, glib developers
found and fixed the deadlock as part of implementing child sources.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-char.c |   55 ++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 6e897da..f29f9b1 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -643,12 +643,18 @@ static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback,
 
 static void io_watch_poll_finalize(GSource *source)
 {
+    /* Due to a glib bug, removing the last reference to a source
+     * inside a finalize callback causes recursive locking (and a
+     * deadlock).  This is not a problem inside other callbacks,
+     * including dispatch callbacks, so we call io_remove_watch_poll
+     * to remove this source.  At this point, iwp->src must
+     * be NULL, or we would leak it.
+     *
+     * This would be solved much more elegantly by child sources,
+     * but we support older glib versions that do not have them.
+     */
     IOWatchPoll *iwp = io_watch_poll_from_source(source);
-    if (iwp->src) {
-        g_source_destroy(iwp->src);
-        g_source_unref(iwp->src);
-        iwp->src = NULL;
-    }
+    assert(iwp->src == NULL);
 }
 
 static GSourceFuncs io_watch_poll_funcs = {
@@ -679,6 +685,25 @@ static guint io_add_watch_poll(GIOChannel *channel,
     return tag;
 }
 
+static void io_remove_watch_poll(guint tag)
+{
+    GSource *source;
+    IOWatchPoll *iwp;
+
+    g_return_if_fail (tag > 0);
+
+    source = g_main_context_find_source_by_id(NULL, tag);
+    g_return_if_fail (source != NULL);
+
+    iwp = io_watch_poll_from_source(source);
+    if (iwp->src) {
+        g_source_destroy(iwp->src);
+        g_source_unref(iwp->src);
+        iwp->src = NULL;
+    }
+    g_source_destroy(&iwp->parent);
+}
+
 #ifndef _WIN32
 static GIOChannel *io_channel_from_fd(int fd)
 {
@@ -788,7 +813,7 @@ static gboolean fd_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
                                      len, &bytes_read, NULL);
     if (status == G_IO_STATUS_EOF) {
         if (s->fd_in_tag) {
-            g_source_remove(s->fd_in_tag);
+            io_remove_watch_poll(s->fd_in_tag);
             s->fd_in_tag = 0;
         }
         qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
@@ -821,7 +846,7 @@ static void fd_chr_update_read_handler(CharDriverState *chr)
     FDCharDriver *s = chr->opaque;
 
     if (s->fd_in_tag) {
-        g_source_remove(s->fd_in_tag);
+        io_remove_watch_poll(s->fd_in_tag);
         s->fd_in_tag = 0;
     }
 
@@ -835,7 +860,7 @@ static void fd_chr_close(struct CharDriverState *chr)
     FDCharDriver *s = chr->opaque;
 
     if (s->fd_in_tag) {
-        g_source_remove(s->fd_in_tag);
+        io_remove_watch_poll(s->fd_in_tag);
         s->fd_in_tag = 0;
     }
 
@@ -1145,7 +1170,7 @@ static void pty_chr_state(CharDriverState *chr, int connected)
 
     if (!connected) {
         if (s->fd_tag) {
-            g_source_remove(s->fd_tag);
+            io_remove_watch_poll(s->fd_tag);
             s->fd_tag = 0;
         }
         s->connected = 0;
@@ -1173,7 +1198,7 @@ static void pty_chr_close(struct CharDriverState *chr)
     int fd;
 
     if (s->fd_tag) {
-        g_source_remove(s->fd_tag);
+        io_remove_watch_poll(s->fd_tag);
         s->fd_tag = 0;
     }
     fd = g_io_channel_unix_get_fd(s->fd);
@@ -2252,7 +2277,7 @@ static gboolean udp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
     s->bufptr = s->bufcnt;
     if (status != G_IO_STATUS_NORMAL) {
         if (s->tag) {
-            g_source_remove(s->tag);
+            io_remove_watch_poll(s->tag);
             s->tag = 0;
         }
         return FALSE;
@@ -2273,7 +2298,7 @@ static void udp_chr_update_read_handler(CharDriverState *chr)
     NetCharDriver *s = chr->opaque;
 
     if (s->tag) {
-        g_source_remove(s->tag);
+        io_remove_watch_poll(s->tag);
         s->tag = 0;
     }
 
@@ -2286,7 +2311,7 @@ static void udp_chr_close(CharDriverState *chr)
 {
     NetCharDriver *s = chr->opaque;
     if (s->tag) {
-        g_source_remove(s->tag);
+        io_remove_watch_poll(s->tag);
         s->tag = 0;
     }
     if (s->chan) {
@@ -2520,7 +2545,7 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
             s->listen_tag = g_io_add_watch(s->listen_chan, G_IO_IN, tcp_chr_accept, chr);
         }
         if (s->tag) {
-            g_source_remove(s->tag);
+            io_remove_watch_poll(s->tag);
             s->tag = 0;
         }
         g_io_channel_unref(s->chan);
@@ -2635,7 +2660,7 @@ static void tcp_chr_close(CharDriverState *chr)
     TCPCharDriver *s = chr->opaque;
     if (s->fd >= 0) {
         if (s->tag) {
-            g_source_remove(s->tag);
+            io_remove_watch_poll(s->tag);
             s->tag = 0;
         }
         if (s->chan) {
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v2 4/4] qemu-char: do not operate on sources from finalize callbacks
  2013-04-19 15:32 ` [Qemu-devel] [PATCH v2 4/4] qemu-char: do not operate on sources from finalize callbacks Paolo Bonzini
@ 2013-04-19 17:12   ` Sander Eikelenboom
  0 siblings, 0 replies; 7+ messages in thread
From: Sander Eikelenboom @ 2013-04-19 17:12 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: amit.shah, aliguori, qemu-devel, kraxel


Friday, April 19, 2013, 5:32:09 PM, you wrote:

> Due to a glib bug, the finalize callback is called with the GMainContext
> lock held.  Thus, any operation on the context from the callback will
> cause recursive locking and a deadlock.  This happens, for example,
> when a client disconnects from a socket chardev.

> The fix for this is somewhat ugly, because we need to forego polymorphism
> and implement our own function to destroy IOWatchPoll sources.  The
> right thing to do here would be child sources, but we support older
> glib versions that do not have them.  Not coincidentially, glib developers
> found and fixed the deadlock as part of implementing child sources.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Tested-by: Sander Eikelenboom <linux@eikelenboom.it>

> ---
>  qemu-char.c |   55 ++++++++++++++++++++++++++++++++++++++++---------------
>  1 files changed, 40 insertions(+), 15 deletions(-)

> diff --git a/qemu-char.c b/qemu-char.c
> index 6e897da..f29f9b1 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -643,12 +643,18 @@ static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback,
>  
>  static void io_watch_poll_finalize(GSource *source)
>  {
> +    /* Due to a glib bug, removing the last reference to a source
> +     * inside a finalize callback causes recursive locking (and a
> +     * deadlock).  This is not a problem inside other callbacks,
> +     * including dispatch callbacks, so we call io_remove_watch_poll
> +     * to remove this source.  At this point, iwp->src must
> +     * be NULL, or we would leak it.
> +     *
> +     * This would be solved much more elegantly by child sources,
> +     * but we support older glib versions that do not have them.
> +     */
>      IOWatchPoll *iwp = io_watch_poll_from_source(source);
-    if (iwp->>src) {
> -        g_source_destroy(iwp->src);
> -        g_source_unref(iwp->src);
-        iwp->>src = NULL;
> -    }
+    assert(iwp->>src == NULL);
>  }
>  
>  static GSourceFuncs io_watch_poll_funcs = {
> @@ -679,6 +685,25 @@ static guint io_add_watch_poll(GIOChannel *channel,
>      return tag;
>  }
>  
> +static void io_remove_watch_poll(guint tag)
> +{
> +    GSource *source;
> +    IOWatchPoll *iwp;
> +
> +    g_return_if_fail (tag > 0);
> +
> +    source = g_main_context_find_source_by_id(NULL, tag);
> +    g_return_if_fail (source != NULL);
> +
> +    iwp = io_watch_poll_from_source(source);
+    if (iwp->>src) {
> +        g_source_destroy(iwp->src);
> +        g_source_unref(iwp->src);
+        iwp->>src = NULL;
> +    }
> +    g_source_destroy(&iwp->parent);
> +}
> +
>  #ifndef _WIN32
>  static GIOChannel *io_channel_from_fd(int fd)
>  {
> @@ -788,7 +813,7 @@ static gboolean fd_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
>                                       len, &bytes_read, NULL);
>      if (status == G_IO_STATUS_EOF) {
>          if (s->fd_in_tag) {
> -            g_source_remove(s->fd_in_tag);
> +            io_remove_watch_poll(s->fd_in_tag);
>              s->fd_in_tag = 0;
>          }
>          qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> @@ -821,7 +846,7 @@ static void fd_chr_update_read_handler(CharDriverState *chr)
>      FDCharDriver *s = chr->opaque;
>  
>      if (s->fd_in_tag) {
> -        g_source_remove(s->fd_in_tag);
> +        io_remove_watch_poll(s->fd_in_tag);
>          s->fd_in_tag = 0;
>      }
>  
> @@ -835,7 +860,7 @@ static void fd_chr_close(struct CharDriverState *chr)
>      FDCharDriver *s = chr->opaque;
>  
>      if (s->fd_in_tag) {
> -        g_source_remove(s->fd_in_tag);
> +        io_remove_watch_poll(s->fd_in_tag);
>          s->fd_in_tag = 0;
>      }
>  
> @@ -1145,7 +1170,7 @@ static void pty_chr_state(CharDriverState *chr, int connected)
>  
>      if (!connected) {
>          if (s->fd_tag) {
> -            g_source_remove(s->fd_tag);
> +            io_remove_watch_poll(s->fd_tag);
>              s->fd_tag = 0;
>          }
>          s->connected = 0;
> @@ -1173,7 +1198,7 @@ static void pty_chr_close(struct CharDriverState *chr)
>      int fd;
>  
>      if (s->fd_tag) {
> -        g_source_remove(s->fd_tag);
> +        io_remove_watch_poll(s->fd_tag);
>          s->fd_tag = 0;
>      }
>      fd = g_io_channel_unix_get_fd(s->fd);
> @@ -2252,7 +2277,7 @@ static gboolean udp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
>      s->bufptr = s->bufcnt;
>      if (status != G_IO_STATUS_NORMAL) {
>          if (s->tag) {
> -            g_source_remove(s->tag);
> +            io_remove_watch_poll(s->tag);
>              s->tag = 0;
>          }
>          return FALSE;
> @@ -2273,7 +2298,7 @@ static void udp_chr_update_read_handler(CharDriverState *chr)
>      NetCharDriver *s = chr->opaque;
>  
>      if (s->tag) {
> -        g_source_remove(s->tag);
> +        io_remove_watch_poll(s->tag);
>          s->tag = 0;
>      }
>  
> @@ -2286,7 +2311,7 @@ static void udp_chr_close(CharDriverState *chr)
>  {
>      NetCharDriver *s = chr->opaque;
>      if (s->tag) {
> -        g_source_remove(s->tag);
> +        io_remove_watch_poll(s->tag);
>          s->tag = 0;
>      }
>      if (s->chan) {
> @@ -2520,7 +2545,7 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
>              s->listen_tag = g_io_add_watch(s->listen_chan, G_IO_IN, tcp_chr_accept, chr);
>          }
>          if (s->tag) {
> -            g_source_remove(s->tag);
> +            io_remove_watch_poll(s->tag);
>              s->tag = 0;
>          }
>          g_io_channel_unref(s->chan);
> @@ -2635,7 +2660,7 @@ static void tcp_chr_close(CharDriverState *chr)
>      TCPCharDriver *s = chr->opaque;
>      if (s->fd >= 0) {
>          if (s->tag) {
> -            g_source_remove(s->tag);
> +            io_remove_watch_poll(s->tag);
>              s->tag = 0;
>          }
>          if (s->chan) {

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH v2 0/4] another round of qemu-char fixes
  2013-04-19 15:32 [Qemu-devel] [PATCH v2 0/4] another round of qemu-char fixes Paolo Bonzini
                   ` (3 preceding siblings ...)
  2013-04-19 15:32 ` [Qemu-devel] [PATCH v2 4/4] qemu-char: do not operate on sources from finalize callbacks Paolo Bonzini
@ 2013-04-22 18:34 ` Anthony Liguori
  4 siblings, 0 replies; 7+ messages in thread
From: Anthony Liguori @ 2013-04-22 18:34 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel; +Cc: amit.shah, aliguori, kraxel

Applied.  Thanks.

Regards,

Anthony Liguori

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2013-04-22 18:35 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-19 15:32 [Qemu-devel] [PATCH v2 0/4] another round of qemu-char fixes Paolo Bonzini
2013-04-19 15:32 ` [Qemu-devel] [PATCH v2 1/4] qemu-char: use consistent idiom for removing sources Paolo Bonzini
2013-04-19 15:32 ` [Qemu-devel] [PATCH v2 2/4] qemu-char: simplify pty polling Paolo Bonzini
2013-04-19 15:32 ` [Qemu-devel] [PATCH v2 3/4] qemu-char: correct return value from chr_read functions Paolo Bonzini
2013-04-19 15:32 ` [Qemu-devel] [PATCH v2 4/4] qemu-char: do not operate on sources from finalize callbacks Paolo Bonzini
2013-04-19 17:12   ` Sander Eikelenboom
2013-04-22 18:34 ` [Qemu-devel] [PATCH v2 0/4] another round of qemu-char fixes Anthony Liguori

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).