[Qemu-devel] [PATCH v2 0/3] chardev: convert leftover glib APIs to use dedicate gcontext

[Qemu-devel] [PATCH v2 0/3] chardev: convert leftover glib APIs to use dedicate gcontext

[Peter Xu]

v2:
- add r-bs
- fix patch 3 on some s->ms conversion [Marc-André]

There were existing work that tried to allow chardev to be run in a
dedicated gcontext rather than the default main context/thread.
Basically that work passed in the correct gcontext during
g_source_attach().  However, I found something missing along the way,
that some legacy glib APIs are used by chardev code which take the
main context as default:

   g_timeout_add_seconds
   g_timeout_add
   g_idle_add

To fully allow the chardevs to be run in dedicated gcontext, we need
to convert all these legacy APIs into g_source_attach() calls as well,
with the correct gcontext passed in.

This series tries to clean the rest of things up.

I picked up patch 1 from monitor-oob series into this series (which is
a missing of chardev frontend call fix for g_source_attach()), so that
this series can be a complete fix.

Please review.  Thanks,

Peter Xu (3):
  chardev: use backend chr context when watch for fe
  chardev: let g_idle_add() be with chardev gcontext
  chardev: introduce qemu_chr_timeout_add() and use

 chardev/char-fe.c      |  2 +-
 chardev/char-pty.c     | 16 ++++++++--------
 chardev/char-socket.c  |  4 ++--
 chardev/char.c         | 20 ++++++++++++++++++++
 hw/char/terminal3270.c |  7 ++++---
 include/chardev/char.h |  2 ++
 6 files changed, 37 insertions(+), 14 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v2 1/3] chardev: use backend chr context when watch for fe

[Peter Xu]

In commit 6bbb6c0644 ("chardev: use per-dev context for
io_add_watch_poll", 2017-09-22) all the chardev watches are converted to
use per-chardev gcontext to support chardev to be run outside default
main thread.  However that's still missing one call from the frontend
code.  Touch that up.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-fe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index ee6d596100..c611b3fa3e 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -356,7 +356,7 @@ guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
     }
 
     g_source_set_callback(src, (GSourceFunc)func, user_data, NULL);
-    tag = g_source_attach(src, NULL);
+    tag = g_source_attach(src, s->gcontext);
     g_source_unref(src);
 
     return tag;
-- 
2.14.3

[Qemu-devel] [PATCH v2 2/3] chardev: let g_idle_add() be with chardev gcontext

[Peter Xu]

The idle task will be attached to main gcontext even if the chardev
backend is running in another gcontext.  Fix the only caller by
extending the g_idle_add() logic into the more powerful
g_source_attach().  It's basically g_idle_add_full() implementation, but
with the chardev's gcontext passed in.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-pty.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index 761ae6dec1..dd17b1b823 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -210,9 +210,14 @@ static void pty_chr_state(Chardev *chr, int connected)
             s->timer_tag = 0;
         }
         if (!s->connected) {
+            GSource *source = g_idle_source_new();
+
             g_assert(s->open_tag == 0);
             s->connected = 1;
-            s->open_tag = g_idle_add(qemu_chr_be_generic_open_func, chr);
+            g_source_set_callback(source, qemu_chr_be_generic_open_func,
+                                  chr, NULL);
+            s->open_tag = g_source_attach(source, chr->gcontext);
+            g_source_unref(source);
         }
         if (!chr->gsource) {
             chr->gsource = io_add_watch_poll(chr, s->ioc,
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 2/3] chardev: let g_idle_add() be with chardev gcontext

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 636 bytes --]

On Wed, Jan 03, 2018 at 10:14:55AM +0800, Peter Xu wrote:
> The idle task will be attached to main gcontext even if the chardev
> backend is running in another gcontext.  Fix the only caller by
> extending the g_idle_add() logic into the more powerful
> g_source_attach().  It's basically g_idle_add_full() implementation, but
> with the chardev's gcontext passed in.
> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-pty.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH v2 3/3] chardev: introduce qemu_chr_timeout_add() and use

[Peter Xu]

It's a replacement of g_timeout_add[_seconds]() for chardevs.  Chardevs
now can have dedicated gcontext, we should always bind chardev tasks
onto those gcontext rather than the default main context.  Since there
are quite a few of g_timeout_add[_seconds]() callers, a new function
qemu_chr_timeout_add() is introduced.

One thing to mention is that, terminal3270 is still always running on
main gcontext.  However let's convert that as well since it's still part
of chardev codes and in case one day we'll miss that when we move it out
of main gcontext too.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-pty.c     |  9 ++-------
 chardev/char-socket.c  |  4 ++--
 chardev/char.c         | 20 ++++++++++++++++++++
 hw/char/terminal3270.c |  7 ++++---
 include/chardev/char.h |  2 ++
 5 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index dd17b1b823..cbd8ac5eb7 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -78,13 +78,8 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms)
         s->timer_tag = 0;
     }
 
-    if (ms == 1000) {
-        name = g_strdup_printf("pty-timer-secs-%s", chr->label);
-        s->timer_tag = g_timeout_add_seconds(1, pty_chr_timer, chr);
-    } else {
-        name = g_strdup_printf("pty-timer-ms-%s", chr->label);
-        s->timer_tag = g_timeout_add(ms, pty_chr_timer, chr);
-    }
+    name = g_strdup_printf("pty-timer-ms-%s", chr->label);
+    s->timer_tag = qemu_chr_timeout_add(chr, ms, pty_chr_timer, chr);
     g_source_set_name_by_id(s->timer_tag, name);
     g_free(name);
 }
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 630a7f2995..5cca32f963 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -73,8 +73,8 @@ static void qemu_chr_socket_restart_timer(Chardev *chr)
     char *name;
 
     assert(s->connected == 0);
-    s->reconnect_timer = g_timeout_add_seconds(s->reconnect_time,
-                                               socket_reconnect_timeout, chr);
+    s->reconnect_timer = qemu_chr_timeout_add(chr, s->reconnect_time * 1000,
+                                              socket_reconnect_timeout, chr);
     name = g_strdup_printf("chardev-socket-reconnect-%s", chr->label);
     g_source_set_name_by_id(s->reconnect_timer, name);
     g_free(name);
diff --git a/chardev/char.c b/chardev/char.c
index 8c3765ee99..a1de662fec 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -1084,6 +1084,26 @@ void qmp_chardev_send_break(const char *id, Error **errp)
     qemu_chr_be_event(chr, CHR_EVENT_BREAK);
 }
 
+/*
+ * Add a timeout callback for the chardev (in milliseconds). Please
+ * use this to add timeout hook for chardev instead of g_timeout_add()
+ * and g_timeout_add_seconds(), to make sure the gcontext that the
+ * task bound to is correct.
+ */
+guint qemu_chr_timeout_add(Chardev *chr, guint ms, GSourceFunc func,
+                           void *private)
+{
+    GSource *source = g_timeout_source_new(ms);
+    guint id;
+
+    assert(func);
+    g_source_set_callback(source, func, private, NULL);
+    id = g_source_attach(source, chr->gcontext);
+    g_source_unref(source);
+
+    return id;
+}
+
 void qemu_chr_cleanup(void)
 {
     object_unparent(get_chardevs_root());
diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
index a109ce5987..250137b78b 100644
--- a/hw/char/terminal3270.c
+++ b/hw/char/terminal3270.c
@@ -94,8 +94,8 @@ static void terminal_read(void *opaque, const uint8_t *buf, int size)
         g_source_remove(t->timer_tag);
         t->timer_tag = 0;
     }
-    t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
-
+    t->timer_tag = qemu_chr_timeout_add(t->chr.chr, 600 * 1000,
+                                        send_timing_mark_cb, t);
     memcpy(&t->inv[t->in_len], buf, size);
     t->in_len += size;
     if (t->in_len < 2) {
@@ -157,7 +157,8 @@ static void chr_event(void *opaque, int event)
          * char-socket.c. Once qemu receives the terminal-type of the
          * client, mark handshake done and trigger everything rolling again.
          */
-        t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
+        t->timer_tag = qemu_chr_timeout_add(t->chr.chr, 600 * 1000,
+                                            send_timing_mark_cb, t);
         break;
     case CHR_EVENT_CLOSED:
         sch->curr_status.scsw.dstat = SCSW_DSTAT_DEVICE_END;
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 778d610295..4970e46457 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -258,5 +258,7 @@ extern int term_escape_char;
 
 /* console.c */
 void qemu_chr_parse_vc(QemuOpts *opts, ChardevBackend *backend, Error **errp);
+guint qemu_chr_timeout_add(Chardev *chr, guint ms, GSourceFunc func,
+                           void *private);
 
 #endif
-- 
2.14.3

[Qemu-devel] [PATCH v2.1 3/3] chardev: introduce qemu_chr_timeout_add() and use

[Peter Xu]

It's a replacement of g_timeout_add[_seconds]() for chardevs.  Chardevs
now can have dedicated gcontext, we should always bind chardev tasks
onto those gcontext rather than the default main context.  Since there
are quite a few of g_timeout_add[_seconds]() callers, a new function
qemu_chr_timeout_add() is introduced.

One thing to mention is that, terminal3270 is still always running on
main gcontext.  However let's convert that as well since it's still part
of chardev codes and in case one day we'll miss that when we move it out
of main gcontext too.

Signed-off-by: Peter Xu <peterx@redhat.com>
---

v2 -> v2.1: Sorry I forgot to do the move in char.h.  Did it in this
minor version.

 chardev/char-pty.c     |  9 ++-------
 chardev/char-socket.c  |  4 ++--
 chardev/char.c         | 20 ++++++++++++++++++++
 hw/char/terminal3270.c |  7 ++++---
 include/chardev/char.h |  3 +++
 5 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index dd17b1b823..cbd8ac5eb7 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -78,13 +78,8 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms)
         s->timer_tag = 0;
     }
 
-    if (ms == 1000) {
-        name = g_strdup_printf("pty-timer-secs-%s", chr->label);
-        s->timer_tag = g_timeout_add_seconds(1, pty_chr_timer, chr);
-    } else {
-        name = g_strdup_printf("pty-timer-ms-%s", chr->label);
-        s->timer_tag = g_timeout_add(ms, pty_chr_timer, chr);
-    }
+    name = g_strdup_printf("pty-timer-ms-%s", chr->label);
+    s->timer_tag = qemu_chr_timeout_add(chr, ms, pty_chr_timer, chr);
     g_source_set_name_by_id(s->timer_tag, name);
     g_free(name);
 }
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 630a7f2995..5cca32f963 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -73,8 +73,8 @@ static void qemu_chr_socket_restart_timer(Chardev *chr)
     char *name;
 
     assert(s->connected == 0);
-    s->reconnect_timer = g_timeout_add_seconds(s->reconnect_time,
-                                               socket_reconnect_timeout, chr);
+    s->reconnect_timer = qemu_chr_timeout_add(chr, s->reconnect_time * 1000,
+                                              socket_reconnect_timeout, chr);
     name = g_strdup_printf("chardev-socket-reconnect-%s", chr->label);
     g_source_set_name_by_id(s->reconnect_timer, name);
     g_free(name);
diff --git a/chardev/char.c b/chardev/char.c
index 8c3765ee99..a1de662fec 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -1084,6 +1084,26 @@ void qmp_chardev_send_break(const char *id, Error **errp)
     qemu_chr_be_event(chr, CHR_EVENT_BREAK);
 }
 
+/*
+ * Add a timeout callback for the chardev (in milliseconds). Please
+ * use this to add timeout hook for chardev instead of g_timeout_add()
+ * and g_timeout_add_seconds(), to make sure the gcontext that the
+ * task bound to is correct.
+ */
+guint qemu_chr_timeout_add(Chardev *chr, guint ms, GSourceFunc func,
+                           void *private)
+{
+    GSource *source = g_timeout_source_new(ms);
+    guint id;
+
+    assert(func);
+    g_source_set_callback(source, func, private, NULL);
+    id = g_source_attach(source, chr->gcontext);
+    g_source_unref(source);
+
+    return id;
+}
+
 void qemu_chr_cleanup(void)
 {
     object_unparent(get_chardevs_root());
diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
index a109ce5987..250137b78b 100644
--- a/hw/char/terminal3270.c
+++ b/hw/char/terminal3270.c
@@ -94,8 +94,8 @@ static void terminal_read(void *opaque, const uint8_t *buf, int size)
         g_source_remove(t->timer_tag);
         t->timer_tag = 0;
     }
-    t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
-
+    t->timer_tag = qemu_chr_timeout_add(t->chr.chr, 600 * 1000,
+                                        send_timing_mark_cb, t);
     memcpy(&t->inv[t->in_len], buf, size);
     t->in_len += size;
     if (t->in_len < 2) {
@@ -157,7 +157,8 @@ static void chr_event(void *opaque, int event)
          * char-socket.c. Once qemu receives the terminal-type of the
          * client, mark handshake done and trigger everything rolling again.
          */
-        t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
+        t->timer_tag = qemu_chr_timeout_add(t->chr.chr, 600 * 1000,
+                                            send_timing_mark_cb, t);
         break;
     case CHR_EVENT_CLOSED:
         sch->curr_status.scsw.dstat = SCSW_DSTAT_DEVICE_END;
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 778d610295..7f71f0def0 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -256,6 +256,9 @@ Chardev *qemu_chardev_new(const char *id, const char *typename,
 
 extern int term_escape_char;
 
+guint qemu_chr_timeout_add(Chardev *chr, guint ms, GSourceFunc func,
+                           void *private);
+
 /* console.c */
 void qemu_chr_parse_vc(QemuOpts *opts, ChardevBackend *backend, Error **errp);
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2.1 3/3] chardev: introduce qemu_chr_timeout_add() and use

[Marc-André Lureau]

----- Original Message -----
> It's a replacement of g_timeout_add[_seconds]() for chardevs.  Chardevs
> now can have dedicated gcontext, we should always bind chardev tasks
> onto those gcontext rather than the default main context.  Since there
> are quite a few of g_timeout_add[_seconds]() callers, a new function
> qemu_chr_timeout_add() is introduced.
> 
> One thing to mention is that, terminal3270 is still always running on
> main gcontext.  However let's convert that as well since it's still part
> of chardev codes and in case one day we'll miss that when we move it out
> of main gcontext too.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

> ---
> 
> v2 -> v2.1: Sorry I forgot to do the move in char.h.  Did it in this
> minor version.
> 
>  chardev/char-pty.c     |  9 ++-------
>  chardev/char-socket.c  |  4 ++--
>  chardev/char.c         | 20 ++++++++++++++++++++
>  hw/char/terminal3270.c |  7 ++++---
>  include/chardev/char.h |  3 +++
>  5 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index dd17b1b823..cbd8ac5eb7 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -78,13 +78,8 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms)
>          s->timer_tag = 0;
>      }
>  
> -    if (ms == 1000) {
> -        name = g_strdup_printf("pty-timer-secs-%s", chr->label);
> -        s->timer_tag = g_timeout_add_seconds(1, pty_chr_timer, chr);
> -    } else {
> -        name = g_strdup_printf("pty-timer-ms-%s", chr->label);
> -        s->timer_tag = g_timeout_add(ms, pty_chr_timer, chr);
> -    }
> +    name = g_strdup_printf("pty-timer-ms-%s", chr->label);
> +    s->timer_tag = qemu_chr_timeout_add(chr, ms, pty_chr_timer, chr);
>      g_source_set_name_by_id(s->timer_tag, name);
>      g_free(name);
>  }
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 630a7f2995..5cca32f963 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -73,8 +73,8 @@ static void qemu_chr_socket_restart_timer(Chardev *chr)
>      char *name;
>  
>      assert(s->connected == 0);
> -    s->reconnect_timer = g_timeout_add_seconds(s->reconnect_time,
> -                                               socket_reconnect_timeout,
> chr);
> +    s->reconnect_timer = qemu_chr_timeout_add(chr, s->reconnect_time * 1000,
> +                                              socket_reconnect_timeout,
> chr);
>      name = g_strdup_printf("chardev-socket-reconnect-%s", chr->label);
>      g_source_set_name_by_id(s->reconnect_timer, name);
>      g_free(name);
> diff --git a/chardev/char.c b/chardev/char.c
> index 8c3765ee99..a1de662fec 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -1084,6 +1084,26 @@ void qmp_chardev_send_break(const char *id, Error
> **errp)
>      qemu_chr_be_event(chr, CHR_EVENT_BREAK);
>  }
>  
> +/*
> + * Add a timeout callback for the chardev (in milliseconds). Please
> + * use this to add timeout hook for chardev instead of g_timeout_add()
> + * and g_timeout_add_seconds(), to make sure the gcontext that the
> + * task bound to is correct.
> + */
> +guint qemu_chr_timeout_add(Chardev *chr, guint ms, GSourceFunc func,
> +                           void *private)
> +{
> +    GSource *source = g_timeout_source_new(ms);
> +    guint id;
> +
> +    assert(func);
> +    g_source_set_callback(source, func, private, NULL);
> +    id = g_source_attach(source, chr->gcontext);
> +    g_source_unref(source);
> +
> +    return id;
> +}
> +
>  void qemu_chr_cleanup(void)
>  {
>      object_unparent(get_chardevs_root());
> diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
> index a109ce5987..250137b78b 100644
> --- a/hw/char/terminal3270.c
> +++ b/hw/char/terminal3270.c
> @@ -94,8 +94,8 @@ static void terminal_read(void *opaque, const uint8_t *buf,
> int size)
>          g_source_remove(t->timer_tag);
>          t->timer_tag = 0;
>      }
> -    t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
> -
> +    t->timer_tag = qemu_chr_timeout_add(t->chr.chr, 600 * 1000,
> +                                        send_timing_mark_cb, t);
>      memcpy(&t->inv[t->in_len], buf, size);
>      t->in_len += size;
>      if (t->in_len < 2) {
> @@ -157,7 +157,8 @@ static void chr_event(void *opaque, int event)
>           * char-socket.c. Once qemu receives the terminal-type of the
>           * client, mark handshake done and trigger everything rolling again.
>           */
> -        t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
> +        t->timer_tag = qemu_chr_timeout_add(t->chr.chr, 600 * 1000,
> +                                            send_timing_mark_cb, t);
>          break;
>      case CHR_EVENT_CLOSED:
>          sch->curr_status.scsw.dstat = SCSW_DSTAT_DEVICE_END;
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 778d610295..7f71f0def0 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -256,6 +256,9 @@ Chardev *qemu_chardev_new(const char *id, const char
> *typename,
>  
>  extern int term_escape_char;
>  
> +guint qemu_chr_timeout_add(Chardev *chr, guint ms, GSourceFunc func,
> +                           void *private);
> +
>  /* console.c */
>  void qemu_chr_parse_vc(QemuOpts *opts, ChardevBackend *backend, Error
>  **errp);
>  
> --
> 2.14.3
> 
> 

Re: [Qemu-devel] [PATCH v2.1 3/3] chardev: introduce qemu_chr_timeout_add() and use

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3737 bytes --]

On Wed, Jan 03, 2018 at 10:24:18AM +0800, Peter Xu wrote:
> It's a replacement of g_timeout_add[_seconds]() for chardevs.  Chardevs
> now can have dedicated gcontext, we should always bind chardev tasks
> onto those gcontext rather than the default main context.  Since there
> are quite a few of g_timeout_add[_seconds]() callers, a new function
> qemu_chr_timeout_add() is introduced.
> 
> One thing to mention is that, terminal3270 is still always running on
> main gcontext.  However let's convert that as well since it's still part
> of chardev codes and in case one day we'll miss that when we move it out
> of main gcontext too.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> 
> v2 -> v2.1: Sorry I forgot to do the move in char.h.  Did it in this
> minor version.
> 
>  chardev/char-pty.c     |  9 ++-------
>  chardev/char-socket.c  |  4 ++--
>  chardev/char.c         | 20 ++++++++++++++++++++
>  hw/char/terminal3270.c |  7 ++++---
>  include/chardev/char.h |  3 +++
>  5 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index dd17b1b823..cbd8ac5eb7 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -78,13 +78,8 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms)
>          s->timer_tag = 0;
>      }
>  
> -    if (ms == 1000) {
> -        name = g_strdup_printf("pty-timer-secs-%s", chr->label);
> -        s->timer_tag = g_timeout_add_seconds(1, pty_chr_timer, chr);
> -    } else {
> -        name = g_strdup_printf("pty-timer-ms-%s", chr->label);
> -        s->timer_tag = g_timeout_add(ms, pty_chr_timer, chr);
> -    }
> +    name = g_strdup_printf("pty-timer-ms-%s", chr->label);
> +    s->timer_tag = qemu_chr_timeout_add(chr, ms, pty_chr_timer, chr);

The label is user-visible.  Why did you remove the seconds label format?

Please either include justification in the commit description or avoid
spurious changes like this so reviewers don't need to worry about code
changes that are not essential.

>      g_source_set_name_by_id(s->timer_tag, name);
>      g_free(name);
>  }
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 630a7f2995..5cca32f963 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -73,8 +73,8 @@ static void qemu_chr_socket_restart_timer(Chardev *chr)
>      char *name;
>  
>      assert(s->connected == 0);
> -    s->reconnect_timer = g_timeout_add_seconds(s->reconnect_time,
> -                                               socket_reconnect_timeout, chr);

Here it was clear that reconnect_time is in seconds...

> +    s->reconnect_timer = qemu_chr_timeout_add(chr, s->reconnect_time * 1000,
> +                                              socket_reconnect_timeout, chr);

...now I can't tell what the unit is.

Please rename qemu_chr_timeout_add() to include the units:

  s->reconnect_timer = qemu_chr_timeout_add_ms(chr, s->reconnect_time * 1000,

>      name = g_strdup_printf("chardev-socket-reconnect-%s", chr->label);
>      g_source_set_name_by_id(s->reconnect_timer, name);
>      g_free(name);
> diff --git a/chardev/char.c b/chardev/char.c
> index 8c3765ee99..a1de662fec 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -1084,6 +1084,26 @@ void qmp_chardev_send_break(const char *id, Error **errp)
>      qemu_chr_be_event(chr, CHR_EVENT_BREAK);
>  }
>  
> +/*
> + * Add a timeout callback for the chardev (in milliseconds). Please
> + * use this to add timeout hook for chardev instead of g_timeout_add()
> + * and g_timeout_add_seconds(), to make sure the gcontext that the
> + * task bound to is correct.
> + */

What is the return value?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2.1 3/3] chardev: introduce qemu_chr_timeout_add() and use

[Peter Xu]

On Wed, Jan 03, 2018 at 05:41:53PM +0000, Stefan Hajnoczi wrote:
> On Wed, Jan 03, 2018 at 10:24:18AM +0800, Peter Xu wrote:
> > It's a replacement of g_timeout_add[_seconds]() for chardevs.  Chardevs
> > now can have dedicated gcontext, we should always bind chardev tasks
> > onto those gcontext rather than the default main context.  Since there
> > are quite a few of g_timeout_add[_seconds]() callers, a new function
> > qemu_chr_timeout_add() is introduced.
> > 
> > One thing to mention is that, terminal3270 is still always running on
> > main gcontext.  However let's convert that as well since it's still part
> > of chardev codes and in case one day we'll miss that when we move it out
> > of main gcontext too.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > 
> > v2 -> v2.1: Sorry I forgot to do the move in char.h.  Did it in this
> > minor version.
> > 
> >  chardev/char-pty.c     |  9 ++-------
> >  chardev/char-socket.c  |  4 ++--
> >  chardev/char.c         | 20 ++++++++++++++++++++
> >  hw/char/terminal3270.c |  7 ++++---
> >  include/chardev/char.h |  3 +++
> >  5 files changed, 31 insertions(+), 12 deletions(-)
> > 
> > diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> > index dd17b1b823..cbd8ac5eb7 100644
> > --- a/chardev/char-pty.c
> > +++ b/chardev/char-pty.c
> > @@ -78,13 +78,8 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms)
> >          s->timer_tag = 0;
> >      }
> >  
> > -    if (ms == 1000) {
> > -        name = g_strdup_printf("pty-timer-secs-%s", chr->label);
> > -        s->timer_tag = g_timeout_add_seconds(1, pty_chr_timer, chr);
> > -    } else {
> > -        name = g_strdup_printf("pty-timer-ms-%s", chr->label);
> > -        s->timer_tag = g_timeout_add(ms, pty_chr_timer, chr);
> > -    }
> > +    name = g_strdup_printf("pty-timer-ms-%s", chr->label);
> > +    s->timer_tag = qemu_chr_timeout_add(chr, ms, pty_chr_timer, chr);
> 
> The label is user-visible.  Why did you remove the seconds label format?

It's used for g_source_set_name_by_id() below, and that's not
user-visible AFAICT?

I removed it because I thought it was not user visible and actually I
didn't see a point on doing that.  Please let me know if I made a
mistake.

> 
> Please either include justification in the commit description or avoid
> spurious changes like this so reviewers don't need to worry about code
> changes that are not essential.

Yes. I can add this into commit message after confirmed with you on above.

> 
> >      g_source_set_name_by_id(s->timer_tag, name);
> >      g_free(name);
> >  }
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 630a7f2995..5cca32f963 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -73,8 +73,8 @@ static void qemu_chr_socket_restart_timer(Chardev *chr)
> >      char *name;
> >  
> >      assert(s->connected == 0);
> > -    s->reconnect_timer = g_timeout_add_seconds(s->reconnect_time,
> > -                                               socket_reconnect_timeout, chr);
> 
> Here it was clear that reconnect_time is in seconds...
> 
> > +    s->reconnect_timer = qemu_chr_timeout_add(chr, s->reconnect_time * 1000,
> > +                                              socket_reconnect_timeout, chr);
> 
> ...now I can't tell what the unit is.
> 
> Please rename qemu_chr_timeout_add() to include the units:
> 
>   s->reconnect_timer = qemu_chr_timeout_add_ms(chr, s->reconnect_time * 1000,

Sure.

> 
> >      name = g_strdup_printf("chardev-socket-reconnect-%s", chr->label);
> >      g_source_set_name_by_id(s->reconnect_timer, name);
> >      g_free(name);
> > diff --git a/chardev/char.c b/chardev/char.c
> > index 8c3765ee99..a1de662fec 100644
> > --- a/chardev/char.c
> > +++ b/chardev/char.c
> > @@ -1084,6 +1084,26 @@ void qmp_chardev_send_break(const char *id, Error **errp)
> >      qemu_chr_be_event(chr, CHR_EVENT_BREAK);
> >  }
> >  
> > +/*
> > + * Add a timeout callback for the chardev (in milliseconds). Please
> > + * use this to add timeout hook for chardev instead of g_timeout_add()
> > + * and g_timeout_add_seconds(), to make sure the gcontext that the
> > + * task bound to is correct.
> > + */
> 
> What is the return value?

Basically I mean it's a wrapper of the other two functions so the
return value would be the same.  But sure I'll note that out.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2.1 3/3] chardev: introduce qemu_chr_timeout_add() and use

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2964 bytes --]

On Thu, Jan 04, 2018 at 10:31:58AM +0800, Peter Xu wrote:
> On Wed, Jan 03, 2018 at 05:41:53PM +0000, Stefan Hajnoczi wrote:
> > On Wed, Jan 03, 2018 at 10:24:18AM +0800, Peter Xu wrote:
> > > It's a replacement of g_timeout_add[_seconds]() for chardevs.  Chardevs
> > > now can have dedicated gcontext, we should always bind chardev tasks
> > > onto those gcontext rather than the default main context.  Since there
> > > are quite a few of g_timeout_add[_seconds]() callers, a new function
> > > qemu_chr_timeout_add() is introduced.
> > > 
> > > One thing to mention is that, terminal3270 is still always running on
> > > main gcontext.  However let's convert that as well since it's still part
> > > of chardev codes and in case one day we'll miss that when we move it out
> > > of main gcontext too.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > > 
> > > v2 -> v2.1: Sorry I forgot to do the move in char.h.  Did it in this
> > > minor version.
> > > 
> > >  chardev/char-pty.c     |  9 ++-------
> > >  chardev/char-socket.c  |  4 ++--
> > >  chardev/char.c         | 20 ++++++++++++++++++++
> > >  hw/char/terminal3270.c |  7 ++++---
> > >  include/chardev/char.h |  3 +++
> > >  5 files changed, 31 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> > > index dd17b1b823..cbd8ac5eb7 100644
> > > --- a/chardev/char-pty.c
> > > +++ b/chardev/char-pty.c
> > > @@ -78,13 +78,8 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms)
> > >          s->timer_tag = 0;
> > >      }
> > >  
> > > -    if (ms == 1000) {
> > > -        name = g_strdup_printf("pty-timer-secs-%s", chr->label);
> > > -        s->timer_tag = g_timeout_add_seconds(1, pty_chr_timer, chr);
> > > -    } else {
> > > -        name = g_strdup_printf("pty-timer-ms-%s", chr->label);
> > > -        s->timer_tag = g_timeout_add(ms, pty_chr_timer, chr);
> > > -    }
> > > +    name = g_strdup_printf("pty-timer-ms-%s", chr->label);
> > > +    s->timer_tag = qemu_chr_timeout_add(chr, ms, pty_chr_timer, chr);
> > 
> > The label is user-visible.  Why did you remove the seconds label format?
> 
> It's used for g_source_set_name_by_id() below, and that's not
> user-visible AFAICT?
> 
> I removed it because I thought it was not user visible and actually I
> didn't see a point on doing that.  Please let me know if I made a
> mistake.
> 
> > 
> > Please either include justification in the commit description or avoid
> > spurious changes like this so reviewers don't need to worry about code
> > changes that are not essential.
> 
> Yes. I can add this into commit message after confirmed with you on above.

You are right, the GSource name isn't visible (it's only used for
debugging).  I was thinking of chr->label.

It's still helpful to mention that the ms == 1000 special case is not
user-visible in the commit description.

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]