[Qemu-devel] [PATCH] chardev: fix incorrect unref of source

[Qemu-devel] [PATCH] chardev: fix incorrect unref of source

[Peter Xu]

glib reported error when pty chardev used:

$ ./qemu-system-x86_64 -chardev pty,id=foo -device isa-serial,chardev=foo
qemu-system-x86_64: -chardev pty,id=foo: char device redirected to /dev/pts/2 (label foo)
(qemu-system-x86_64:27885): GLib-CRITICAL **: g_source_unref: assertion 'source != NULL' failed
(qemu-system-x86_64:27885): GLib-CRITICAL **: g_source_unref: assertion 'source != NULL' failed

This patch fixes that.

Fixes: 2c716ba150 ("chardev: introduce qemu_chr_timeout_add_ms()")
CC: Paolo Bonzini <pbonzini@redhat.com>
Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-pty.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index 89315e6807..68fd4e20c3 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -51,15 +51,32 @@ typedef struct {
 static void pty_chr_update_read_handler_locked(Chardev *chr);
 static void pty_chr_state(Chardev *chr, int connected);
 
+static void pty_chr_timer_cancel(PtyChardev *s)
+{
+    if (s->timer_src) {
+        g_source_destroy(s->timer_src);
+        g_source_unref(s->timer_src);
+        s->timer_src = NULL;
+    }
+}
+
+static void pty_chr_open_src_cancel(PtyChardev *s)
+{
+    if (s->open_source) {
+        g_source_destroy(s->open_source);
+        g_source_unref(s->open_source);
+        s->open_source = NULL;
+    }
+}
+
 static gboolean pty_chr_timer(gpointer opaque)
 {
     struct Chardev *chr = CHARDEV(opaque);
     PtyChardev *s = PTY_CHARDEV(opaque);
 
     qemu_mutex_lock(&chr->chr_write_lock);
-    s->timer_src = NULL;
-    g_source_unref(s->open_source);
-    s->open_source = NULL;
+    pty_chr_timer_cancel(s);
+    pty_chr_open_src_cancel(s);
     if (!s->connected) {
         /* Next poll ... */
         pty_chr_update_read_handler_locked(chr);
@@ -68,15 +85,6 @@ static gboolean pty_chr_timer(gpointer opaque)
     return FALSE;
 }
 
-static void pty_chr_timer_cancel(PtyChardev *s)
-{
-    if (s->timer_src) {
-        g_source_destroy(s->timer_src);
-        g_source_unref(s->timer_src);
-        s->timer_src = NULL;
-    }
-}
-
 /* Called with chr_write_lock held.  */
 static void pty_chr_rearm_timer(Chardev *chr, int ms)
 {
@@ -195,11 +203,7 @@ static void pty_chr_state(Chardev *chr, int connected)
     PtyChardev *s = PTY_CHARDEV(chr);
 
     if (!connected) {
-        if (s->open_source) {
-            g_source_destroy(s->open_source);
-            g_source_unref(s->open_source);
-            s->open_source = NULL;
-        }
+        pty_chr_open_src_cancel(s);
         remove_fd_in_watch(chr);
         s->connected = 0;
         /* (re-)connect poll interval for idle guests: once per second.
-- 
2.14.3

Re: [Qemu-devel] [PATCH] chardev: fix incorrect unref of source

[Paolo Bonzini]

On 18/01/2018 06:20, Peter Xu wrote:
> glib reported error when pty chardev used:
> 
> $ ./qemu-system-x86_64 -chardev pty,id=foo -device isa-serial,chardev=foo
> qemu-system-x86_64: -chardev pty,id=foo: char device redirected to /dev/pts/2 (label foo)
> (qemu-system-x86_64:27885): GLib-CRITICAL **: g_source_unref: assertion 'source != NULL' failed
> (qemu-system-x86_64:27885): GLib-CRITICAL **: g_source_unref: assertion 'source != NULL' failed
> 
> This patch fixes that.
> 
> Fixes: 2c716ba150 ("chardev: introduce qemu_chr_timeout_add_ms()")
> CC: Paolo Bonzini <pbonzini@redhat.com>
> Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-pty.c | 38 +++++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index 89315e6807..68fd4e20c3 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -51,15 +51,32 @@ typedef struct {
>  static void pty_chr_update_read_handler_locked(Chardev *chr);
>  static void pty_chr_state(Chardev *chr, int connected);
>  
> +static void pty_chr_timer_cancel(PtyChardev *s)
> +{
> +    if (s->timer_src) {
> +        g_source_destroy(s->timer_src);
> +        g_source_unref(s->timer_src);
> +        s->timer_src = NULL;
> +    }
> +}
> +
> +static void pty_chr_open_src_cancel(PtyChardev *s)
> +{
> +    if (s->open_source) {
> +        g_source_destroy(s->open_source);
> +        g_source_unref(s->open_source);
> +        s->open_source = NULL;
> +    }
> +}
> +
>  static gboolean pty_chr_timer(gpointer opaque)
>  {
>      struct Chardev *chr = CHARDEV(opaque);
>      PtyChardev *s = PTY_CHARDEV(opaque);
>  
>      qemu_mutex_lock(&chr->chr_write_lock);
> -    s->timer_src = NULL;
> -    g_source_unref(s->open_source);
> -    s->open_source = NULL;
> +    pty_chr_timer_cancel(s);
> +    pty_chr_open_src_cancel(s);
>      if (!s->connected) {
>          /* Next poll ... */
>          pty_chr_update_read_handler_locked(chr);
> @@ -68,15 +85,6 @@ static gboolean pty_chr_timer(gpointer opaque)
>      return FALSE;
>  }
>  
> -static void pty_chr_timer_cancel(PtyChardev *s)
> -{
> -    if (s->timer_src) {
> -        g_source_destroy(s->timer_src);
> -        g_source_unref(s->timer_src);
> -        s->timer_src = NULL;
> -    }
> -}
> -
>  /* Called with chr_write_lock held.  */
>  static void pty_chr_rearm_timer(Chardev *chr, int ms)
>  {
> @@ -195,11 +203,7 @@ static void pty_chr_state(Chardev *chr, int connected)
>      PtyChardev *s = PTY_CHARDEV(chr);
>  
>      if (!connected) {
> -        if (s->open_source) {
> -            g_source_destroy(s->open_source);
> -            g_source_unref(s->open_source);
> -            s->open_source = NULL;
> -        }
> +        pty_chr_open_src_cancel(s);
>          remove_fd_in_watch(chr);
>          s->connected = 0;
>          /* (re-)connect poll interval for idle guests: once per second.
> 

Queued, thanks.

Paolo