[Qemu-devel] [PATCH v2 0/4] chardev: support non-default gcontext

[Qemu-devel] [PATCH v2 0/4] chardev: support non-default gcontext

[Peter Xu]

The old chardev may not fully support non-default GMainContext.  One
direct clue is that when we call io_add_watch_poll() sometimes we are
still passing in the NULL context pointer.

IIUC we are fine during setup since the context will be passed
correctly during setup via chr_update_read_handler().  However it
won't survive if chardev reconnected due to some reason.

This series tries to solve above problem by caching the gcontext
pointer in Chardev itself.

This will be required for the monitor OOB (out-of-band) support, since
in that series monitor backends may be run in non-default contexts.

Please review.  Thanks.

Peter Xu (4):
  chardev: new qemu_chr_be_update_read_handlers()
  chardev: add Chardev.gcontext field
  chardev: use per-dev context for io_add_watch_poll
  chardev: remove context in chr_update_read_handler

 chardev/char-fd.c      |  5 ++---
 chardev/char-fe.c      |  7 ++-----
 chardev/char-pty.c     |  5 ++---
 chardev/char-socket.c  |  7 +++----
 chardev/char-udp.c     |  5 ++---
 chardev/char.c         | 11 +++++++++++
 include/chardev/char.h | 13 ++++++++++++-
 7 files changed, 34 insertions(+), 19 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/4] chardev: new qemu_chr_be_update_read_handlers()

[Peter Xu]

Add a wrapper for the chr_update_read_handler().

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-fe.c      |  7 ++-----
 chardev/char.c         | 10 ++++++++++
 include/chardev/char.h | 10 ++++++++++
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index f3af6ae..ee6d596 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -253,7 +253,6 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
                               bool set_open)
 {
     Chardev *s;
-    ChardevClass *cc;
     int fe_open;
 
     s = b->chr;
@@ -261,7 +260,6 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
         return;
     }
 
-    cc = CHARDEV_GET_CLASS(s);
     if (!opaque && !fd_can_read && !fd_read && !fd_event) {
         fe_open = 0;
         remove_fd_in_watch(s);
@@ -273,9 +271,8 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
     b->chr_event = fd_event;
     b->chr_be_change = be_change;
     b->opaque = opaque;
-    if (cc->chr_update_read_handler) {
-        cc->chr_update_read_handler(s, context);
-    }
+
+    qemu_chr_be_update_read_handlers(s, context);
 
     if (set_open) {
         qemu_chr_fe_set_open(b, fe_open);
diff --git a/chardev/char.c b/chardev/char.c
index b6fd5eb..e090dd5 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -180,6 +180,16 @@ void qemu_chr_be_write(Chardev *s, uint8_t *buf, int len)
     }
 }
 
+void qemu_chr_be_update_read_handlers(Chardev *s,
+                                      GMainContext *context)
+{
+    ChardevClass *cc = CHARDEV_GET_CLASS(s);
+
+    if (cc->chr_update_read_handler) {
+        cc->chr_update_read_handler(s, context);
+    }
+}
+
 int qemu_chr_add_client(Chardev *s, int fd)
 {
     return CHARDEV_GET_CLASS(s)->chr_add_client ?
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 66dde46..2068ea4 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -169,6 +169,16 @@ void qemu_chr_be_write(Chardev *s, uint8_t *buf, int len);
 void qemu_chr_be_write_impl(Chardev *s, uint8_t *buf, int len);
 
 /**
+ * @qemu_chr_be_update_read_handlers:
+ *
+ * Invoked when frontend read handlers are setup
+ *
+ * @context the gcontext that will be used to attach the watch sources
+ */
+void qemu_chr_be_update_read_handlers(Chardev *s,
+                                      GMainContext *context);
+
+/**
  * @qemu_chr_be_event:
  *
  * Send an event from the back end to the front end.
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/4] chardev: add Chardev.gcontext field

[Peter Xu]

It caches the gcontext that is used to poll the chardev IO.  Before this
patch, we only passed it in via chr_update_read_handlers().  However
that may not be enough if the char backend is disconnected and
reconnected afterward.  There are chardev codes that still assumed the
context be NULL (which is the main context).  Will fix that up in
following up patches.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char.c         | 1 +
 include/chardev/char.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/chardev/char.c b/chardev/char.c
index e090dd5..89eabea 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -185,6 +185,7 @@ void qemu_chr_be_update_read_handlers(Chardev *s,
 {
     ChardevClass *cc = CHARDEV_GET_CLASS(s);
 
+    s->gcontext = context;
     if (cc->chr_update_read_handler) {
         cc->chr_update_read_handler(s, context);
     }
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 2068ea4..84fb773 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -55,6 +55,7 @@ struct Chardev {
     int logfd;
     int be_open;
     GSource *gsource;
+    GMainContext *gcontext;
     DECLARE_BITMAP(features, QEMU_CHAR_FEATURE_LAST);
 };
 
-- 
2.7.4

[Qemu-devel] [PATCH v2 3/4] chardev: use per-dev context for io_add_watch_poll

[Peter Xu]

It was only passed in by chr_update_read_handlers().  However when
reconnect, we'll lose that context information.  So if a chardev was
running on another context (rather than the default context, the NULL
pointer), it'll switch back to the default context if reconnection
happens.  But, it should really stick to the old context.

Convert all the callers of io_add_watch_poll() to use the internally
cached gcontext.  Then the context should be able to survive even after
reconnections.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-fd.c     | 2 +-
 chardev/char-pty.c    | 2 +-
 chardev/char-socket.c | 4 ++--
 chardev/char-udp.c    | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index 6a62a54..09fbb07 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -94,7 +94,7 @@ static void fd_chr_update_read_handler(Chardev *chr,
         chr->gsource = io_add_watch_poll(chr, s->ioc_in,
                                            fd_chr_read_poll,
                                            fd_chr_read, chr,
-                                           context);
+                                           chr->gcontext);
     }
 }
 
diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index e5d20a0..d239c04 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -219,7 +219,7 @@ static void pty_chr_state(Chardev *chr, int connected)
             chr->gsource = io_add_watch_poll(chr, s->ioc,
                                                pty_chr_read_poll,
                                                pty_chr_read,
-                                               chr, NULL);
+                                               chr, chr->gcontext);
         }
     }
 }
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 1ae730a..ee71cbe 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -516,7 +516,7 @@ static void tcp_chr_connect(void *opaque)
         chr->gsource = io_add_watch_poll(chr, s->ioc,
                                            tcp_chr_read_poll,
                                            tcp_chr_read,
-                                           chr, NULL);
+                                           chr, chr->gcontext);
     }
     qemu_chr_be_event(chr, CHR_EVENT_OPENED);
 }
@@ -535,7 +535,7 @@ static void tcp_chr_update_read_handler(Chardev *chr,
         chr->gsource = io_add_watch_poll(chr, s->ioc,
                                            tcp_chr_read_poll,
                                            tcp_chr_read, chr,
-                                           context);
+                                           chr->gcontext);
     }
 }
 
diff --git a/chardev/char-udp.c b/chardev/char-udp.c
index 4ee11d3..106dee1 100644
--- a/chardev/char-udp.c
+++ b/chardev/char-udp.c
@@ -110,7 +110,7 @@ static void udp_chr_update_read_handler(Chardev *chr,
         chr->gsource = io_add_watch_poll(chr, s->ioc,
                                            udp_chr_read_poll,
                                            udp_chr_read, chr,
-                                           context);
+                                           chr->gcontext);
     }
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH v2 4/4] chardev: remove context in chr_update_read_handler

[Peter Xu]

We had a per-chardev cache for context, then we don't need this
parameter to be passed in every time when chr_update_read_handler()
called.  As long as we are calling chr_update_read_handler() using
qemu_chr_be_update_read_handlers() we'll be fine.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-fd.c      | 3 +--
 chardev/char-pty.c     | 3 +--
 chardev/char-socket.c  | 3 +--
 chardev/char-udp.c     | 3 +--
 chardev/char.c         | 2 +-
 include/chardev/char.h | 2 +-
 6 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index 09fbb07..2c9b2ce 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -84,8 +84,7 @@ static GSource *fd_chr_add_watch(Chardev *chr, GIOCondition cond)
     return qio_channel_create_watch(s->ioc_out, cond);
 }
 
-static void fd_chr_update_read_handler(Chardev *chr,
-                                       GMainContext *context)
+static void fd_chr_update_read_handler(Chardev *chr)
 {
     FDChardev *s = FD_CHARDEV(chr);
 
diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index d239c04..761ae6d 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -112,8 +112,7 @@ static void pty_chr_update_read_handler_locked(Chardev *chr)
     }
 }
 
-static void pty_chr_update_read_handler(Chardev *chr,
-                                        GMainContext *context)
+static void pty_chr_update_read_handler(Chardev *chr)
 {
     qemu_mutex_lock(&chr->chr_write_lock);
     pty_chr_update_read_handler_locked(chr);
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index ee71cbe..e65148f 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -521,8 +521,7 @@ static void tcp_chr_connect(void *opaque)
     qemu_chr_be_event(chr, CHR_EVENT_OPENED);
 }
 
-static void tcp_chr_update_read_handler(Chardev *chr,
-                                        GMainContext *context)
+static void tcp_chr_update_read_handler(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
 
diff --git a/chardev/char-udp.c b/chardev/char-udp.c
index 106dee1..d46ff7a 100644
--- a/chardev/char-udp.c
+++ b/chardev/char-udp.c
@@ -100,8 +100,7 @@ static gboolean udp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
     return TRUE;
 }
 
-static void udp_chr_update_read_handler(Chardev *chr,
-                                        GMainContext *context)
+static void udp_chr_update_read_handler(Chardev *chr)
 {
     UdpChardev *s = UDP_CHARDEV(chr);
 
diff --git a/chardev/char.c b/chardev/char.c
index 89eabea..2ae4f46 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -187,7 +187,7 @@ void qemu_chr_be_update_read_handlers(Chardev *s,
 
     s->gcontext = context;
     if (cc->chr_update_read_handler) {
-        cc->chr_update_read_handler(s, context);
+        cc->chr_update_read_handler(s);
     }
 }
 
diff --git a/include/chardev/char.h b/include/chardev/char.h
index 84fb773..43aabcc 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -238,7 +238,7 @@ typedef struct ChardevClass {
     int (*chr_write)(Chardev *s, const uint8_t *buf, int len);
     int (*chr_sync_read)(Chardev *s, const uint8_t *buf, int len);
     GSource *(*chr_add_watch)(Chardev *s, GIOCondition cond);
-    void (*chr_update_read_handler)(Chardev *s, GMainContext *context);
+    void (*chr_update_read_handler)(Chardev *s);
     int (*chr_ioctl)(Chardev *s, int cmd, void *arg);
     int (*get_msgfds)(Chardev *s, int* fds, int num);
     int (*set_msgfds)(Chardev *s, int *fds, int num);
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 0/4] chardev: support non-default gcontext

[Marc-André Lureau]

On Thu, Sep 21, 2017 at 8:35 AM, Peter Xu <peterx@redhat.com> wrote:
> The old chardev may not fully support non-default GMainContext.  One
> direct clue is that when we call io_add_watch_poll() sometimes we are
> still passing in the NULL context pointer.
>
> IIUC we are fine during setup since the context will be passed
> correctly during setup via chr_update_read_handler().  However it
> won't survive if chardev reconnected due to some reason.
>
> This series tries to solve above problem by caching the gcontext
> pointer in Chardev itself.
>
> This will be required for the monitor OOB (out-of-band) support, since
> in that series monitor backends may be run in non-default contexts.
>
> Please review.  Thanks.
>
> Peter Xu (4):
>   chardev: new qemu_chr_be_update_read_handlers()
>   chardev: add Chardev.gcontext field
>   chardev: use per-dev context for io_add_watch_poll
>   chardev: remove context in chr_update_read_handler
>
>  chardev/char-fd.c      |  5 ++---
>  chardev/char-fe.c      |  7 ++-----
>  chardev/char-pty.c     |  5 ++---
>  chardev/char-socket.c  |  7 +++----
>  chardev/char-udp.c     |  5 ++---
>  chardev/char.c         | 11 +++++++++++
>  include/chardev/char.h | 13 ++++++++++++-
>  7 files changed, 34 insertions(+), 19 deletions(-)

It's only for the read handler, perhaps the gcontext field should be
named "read_context" or adding a comment to explain this is only used
for that.

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


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 3/4] chardev: use per-dev context for io_add_watch_poll

[Paolo Bonzini]

On 21/09/2017 08:35, Peter Xu wrote:
> It was only passed in by chr_update_read_handlers().  However when
> reconnect, we'll lose that context information.  So if a chardev was
> running on another context (rather than the default context, the NULL
> pointer), it'll switch back to the default context if reconnection
> happens.  But, it should really stick to the old context.
> 
> Convert all the callers of io_add_watch_poll() to use the internally
> cached gcontext.  Then the context should be able to survive even after
> reconnections.

Why keep the argument then?

Thanks,

Paolo

> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-fd.c     | 2 +-
>  chardev/char-pty.c    | 2 +-
>  chardev/char-socket.c | 4 ++--
>  chardev/char-udp.c    | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/chardev/char-fd.c b/chardev/char-fd.c
> index 6a62a54..09fbb07 100644
> --- a/chardev/char-fd.c
> +++ b/chardev/char-fd.c
> @@ -94,7 +94,7 @@ static void fd_chr_update_read_handler(Chardev *chr,
>          chr->gsource = io_add_watch_poll(chr, s->ioc_in,
>                                             fd_chr_read_poll,
>                                             fd_chr_read, chr,
> -                                           context);
> +                                           chr->gcontext);
>      }
>  }
>  
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index e5d20a0..d239c04 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -219,7 +219,7 @@ static void pty_chr_state(Chardev *chr, int connected)
>              chr->gsource = io_add_watch_poll(chr, s->ioc,
>                                                 pty_chr_read_poll,
>                                                 pty_chr_read,
> -                                               chr, NULL);
> +                                               chr, chr->gcontext);
>          }
>      }
>  }
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 1ae730a..ee71cbe 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -516,7 +516,7 @@ static void tcp_chr_connect(void *opaque)
>          chr->gsource = io_add_watch_poll(chr, s->ioc,
>                                             tcp_chr_read_poll,
>                                             tcp_chr_read,
> -                                           chr, NULL);
> +                                           chr, chr->gcontext);
>      }
>      qemu_chr_be_event(chr, CHR_EVENT_OPENED);
>  }
> @@ -535,7 +535,7 @@ static void tcp_chr_update_read_handler(Chardev *chr,
>          chr->gsource = io_add_watch_poll(chr, s->ioc,
>                                             tcp_chr_read_poll,
>                                             tcp_chr_read, chr,
> -                                           context);
> +                                           chr->gcontext);
>      }
>  }
>  
> diff --git a/chardev/char-udp.c b/chardev/char-udp.c
> index 4ee11d3..106dee1 100644
> --- a/chardev/char-udp.c
> +++ b/chardev/char-udp.c
> @@ -110,7 +110,7 @@ static void udp_chr_update_read_handler(Chardev *chr,
>          chr->gsource = io_add_watch_poll(chr, s->ioc,
>                                             udp_chr_read_poll,
>                                             udp_chr_read, chr,
> -                                           context);
> +                                           chr->gcontext);
>      }
>  }
>  
> 

Re: [Qemu-devel] [PATCH v2 3/4] chardev: use per-dev context for io_add_watch_poll

[Peter Xu]

On Thu, Sep 21, 2017 at 04:11:33PM +0200, Paolo Bonzini wrote:
> On 21/09/2017 08:35, Peter Xu wrote:
> > It was only passed in by chr_update_read_handlers().  However when
> > reconnect, we'll lose that context information.  So if a chardev was
> > running on another context (rather than the default context, the NULL
> > pointer), it'll switch back to the default context if reconnection
> > happens.  But, it should really stick to the old context.
> > 
> > Convert all the callers of io_add_watch_poll() to use the internally
> > cached gcontext.  Then the context should be able to survive even after
> > reconnections.
> 
> Why keep the argument then?

Yeah it's in next patch. I see that this series has been queued
already. Thanks!

-- 
Peter Xu