[Qemu-devel] [PATCH v2 0/6] chardev: various cleanups and improvements (resend)

[Qemu-devel] [PATCH v2 0/6] chardev: various cleanups and improvements (resend)

[Marc-André Lureau]

Hi,

Some chardev-related patches I have accumulated.
Please review, thanks!

v2:
- rebased
- update commit messages

Marc-André Lureau (6):
  char: update the mux handlers in class callback
  char-fe: set_handlers() needs an associated chardev
  terminal3270: do not use backend timer sources
  chardev: add a note about frontend sources and context switch
  char-pty: remove the check for connection on write
  char-pty: remove write_lock usage

 include/chardev/char-fe.h  |  5 ++--
 include/chardev/char-mux.h |  1 -
 chardev/char-fe.c          | 11 +-------
 chardev/char-mux.c         |  5 ++--
 chardev/char-pty.c         | 56 +++-----------------------------------
 hw/char/terminal3270.c     | 15 ++++------
 6 files changed, 17 insertions(+), 76 deletions(-)

-- 
2.20.1.519.g8feddda32c

[Qemu-devel] [PATCH v2 1/6] char: update the mux handlers in class callback

[Marc-André Lureau]

Instead of handling mux chardev in a special way in
qemu_chr_fe_set_handlers(), we may use the chr_update_read_handler
class callback instead.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/chardev/char-mux.h | 1 -
 chardev/char-fe.c          | 4 ----
 chardev/char-mux.c         | 5 +++--
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/include/chardev/char-mux.h b/include/chardev/char-mux.h
index 1e13187767..572cefd517 100644
--- a/include/chardev/char-mux.h
+++ b/include/chardev/char-mux.h
@@ -55,7 +55,6 @@ typedef struct MuxChardev {
 #define CHARDEV_IS_MUX(chr)                             \
     object_dynamic_cast(OBJECT(chr), TYPE_CHARDEV_MUX)
 
-void mux_chr_set_handlers(Chardev *chr, GMainContext *context);
 void mux_set_focus(Chardev *chr, int focus);
 void mux_chr_send_all_event(Chardev *chr, int event);
 
diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index a8931f7afd..f69ca18647 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -289,10 +289,6 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
             qemu_chr_be_event(s, CHR_EVENT_OPENED);
         }
     }
-
-    if (CHARDEV_IS_MUX(s)) {
-        mux_chr_set_handlers(s, context);
-    }
 }
 
 void qemu_chr_fe_take_focus(CharBackend *b)
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 6055e76293..9406eaf08d 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -278,7 +278,7 @@ static void char_mux_finalize(Object *obj)
     qemu_chr_fe_deinit(&d->chr, false);
 }
 
-void mux_chr_set_handlers(Chardev *chr, GMainContext *context)
+static void mux_chr_update_read_handlers(Chardev *chr)
 {
     MuxChardev *d = MUX_CHARDEV(chr);
 
@@ -289,7 +289,7 @@ void mux_chr_set_handlers(Chardev *chr, GMainContext *context)
                              mux_chr_event,
                              NULL,
                              chr,
-                             context, true);
+                             chr->gcontext, true);
 }
 
 void mux_set_focus(Chardev *chr, int focus)
@@ -383,6 +383,7 @@ static void char_mux_class_init(ObjectClass *oc, void *data)
     cc->chr_add_watch = mux_chr_add_watch;
     cc->chr_be_event = mux_chr_be_event;
     cc->chr_machine_done = open_muxes;
+    cc->chr_update_read_handler = mux_chr_update_read_handlers;
 }
 
 static const TypeInfo char_mux_type_info = {
-- 
2.20.1.519.g8feddda32c

[Qemu-devel] [PATCH v2 2/6] char-fe: set_handlers() needs an associated chardev

[Marc-André Lureau]

It is futile to call qemu_chr_fe_set_handlers() without an associated
chardev, because the function is doing nothing in that case, not even
reporting an error, it would likely be a programming error. Let's not
handle that hypothetical case.

(fwiw, I introduced the check in commit
94a40fc56036b5058b0b194d9e372a22e65ce7be, that was a mistake imho)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/chardev/char-fe.h | 2 --
 chardev/char-fe.c         | 7 +------
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index 46c997d352..54f0d61d32 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -82,8 +82,6 @@ bool qemu_chr_fe_backend_open(CharBackend *be);
  *
  * Set the front end char handlers. The front end takes the focus if
  * any of the handler is non-NULL.
- *
- * Without associated Chardev, nothing is changed.
  */
 void qemu_chr_fe_set_handlers(CharBackend *b,
                               IOCanReadHandler *fd_can_read,
diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index f69ca18647..4ec9823d34 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -255,14 +255,9 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
                               GMainContext *context,
                               bool set_open)
 {
-    Chardev *s;
+    Chardev *s = b->chr;
     int fe_open;
 
-    s = b->chr;
-    if (!s) {
-        return;
-    }
-
     if (!opaque && !fd_can_read && !fd_read && !fd_event) {
         fe_open = 0;
         remove_fd_in_watch(s);
-- 
2.20.1.519.g8feddda32c

[Qemu-devel] [PATCH v2 3/6] terminal3270: do not use backend timer sources

[Marc-André Lureau]

terminal3270 uses the front-end side of the chardev. It shouldn't
create sources from backend side context (with backend
functions).

send_timing_mark_cb calls qemu_chr_fe_write_all() which should be
thread safe.

This partially reverts changes from commit
2c716ba1506769c9be2caa02f0f6d6e7c00f4304.

CC: Peter Xu <peterx@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/char/terminal3270.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
index e9c45e55b1..35b079d5c4 100644
--- a/hw/char/terminal3270.c
+++ b/hw/char/terminal3270.c
@@ -31,7 +31,7 @@ typedef struct Terminal3270 {
     uint8_t outv[OUTPUT_BUFFER_SIZE];
     int in_len;
     bool handshake_done;
-    GSource *timer_src;
+    guint timer_tag;
 } Terminal3270;
 
 #define TYPE_TERMINAL_3270 "x-terminal3270"
@@ -47,10 +47,9 @@ static int terminal_can_read(void *opaque)
 
 static void terminal_timer_cancel(Terminal3270 *t)
 {
-    if (t->timer_src) {
-        g_source_destroy(t->timer_src);
-        g_source_unref(t->timer_src);
-        t->timer_src = NULL;
+    if (t->timer_tag) {
+        g_source_remove(t->timer_tag);
+        t->timer_tag = 0;
     }
 }
 
@@ -100,8 +99,7 @@ static void terminal_read(void *opaque, const uint8_t *buf, int size)
     assert(size <= (INPUT_BUFFER_SIZE - t->in_len));
 
     terminal_timer_cancel(t);
-    t->timer_src = qemu_chr_timeout_add_ms(t->chr.chr, 600 * 1000,
-                                           send_timing_mark_cb, t);
+    t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
     memcpy(&t->inv[t->in_len], buf, size);
     t->in_len += size;
     if (t->in_len < 2) {
@@ -160,8 +158,7 @@ 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_src = qemu_chr_timeout_add_ms(t->chr.chr, 600 * 1000,
-                                               send_timing_mark_cb, t);
+        t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
         break;
     case CHR_EVENT_CLOSED:
         sch->curr_status.scsw.dstat = SCSW_DSTAT_DEVICE_END;
-- 
2.20.1.519.g8feddda32c

[Qemu-devel] [PATCH v2 4/6] chardev: add a note about frontend sources and context switch

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/chardev/char-fe.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index 54f0d61d32..2211913734 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -166,6 +166,9 @@ void qemu_chr_fe_printf(CharBackend *be, const char *fmt, ...)
  * is active; return the #GSource's tag.  If it is disconnected,
  * or without associated Chardev, return 0.
  *
+ * Note that you are responsible to update the front-end sources if
+ * you are switching the main context with qemu_chr_fe_set_handlers().
+ *
  * Returns: the source tag
  */
 guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
-- 
2.20.1.519.g8feddda32c

[Qemu-devel] [PATCH v2 5/6] char-pty: remove the check for connection on write

[Marc-André Lureau]

This doesn't help much compared to the 1 second poll PTY
timer. I can't think of a use case where this would help.

However, we can simplify the code around chr_write(): the write lock
is no longer needed for other char-pty callbacks (see following
patch).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-pty.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index f681d637c1..f8772c9e15 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -130,11 +130,7 @@ static int char_pty_chr_write(Chardev *chr, const uint8_t *buf, int len)
     PtyChardev *s = PTY_CHARDEV(chr);
 
     if (!s->connected) {
-        /* guest sends data, check for (re-)connect */
-        pty_chr_update_read_handler_locked(chr);
-        if (!s->connected) {
-            return len;
-        }
+        return len;
     }
     return io_channel_send(s->ioc, buf, len);
 }
-- 
2.20.1.519.g8feddda32c

[Qemu-devel] [PATCH v2 6/6] char-pty: remove write_lock usage

[Marc-André Lureau]

The lock usage was described with its introduction in commit
9005b2a7589540a3733b3abdcfbccfe7746cd1a1. It was necessary because PTY
write() shares more state than GIOChannel with other
operations.

This made char-pty a bit different from other chardev, that only lock
around the write operation.  This was apparent in commit
7b3621f47a990c5099c6385728347f69a8d0e55c, which introduced an idle
source to avoid the lock.

By removing the PTY chardev state sharing on write() with previous
patch, we can remove the lock and the idle source.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-pty.c | 50 +++-------------------------------------------
 1 file changed, 3 insertions(+), 47 deletions(-)

diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index f8772c9e15..b034332edd 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -36,15 +36,12 @@ typedef struct {
     QIOChannel *ioc;
     int read_bytes;
 
-    /* Protected by the Chardev chr_write_lock.  */
     int connected;
     GSource *timer_src;
-    GSource *open_source;
 } PtyChardev;
 
 #define PTY_CHARDEV(obj) OBJECT_CHECK(PtyChardev, (obj), TYPE_CHARDEV_PTY)
 
-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)
@@ -56,32 +53,19 @@ static void pty_chr_timer_cancel(PtyChardev *s)
     }
 }
 
-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);
     pty_chr_timer_cancel(s);
-    pty_chr_open_src_cancel(s);
     if (!s->connected) {
         /* Next poll ... */
-        pty_chr_update_read_handler_locked(chr);
+        qemu_chr_be_update_read_handlers(chr, chr->gcontext);
     }
-    qemu_mutex_unlock(&chr->chr_write_lock);
     return FALSE;
 }
 
-/* Called with chr_write_lock held.  */
 static void pty_chr_rearm_timer(Chardev *chr, int ms)
 {
     PtyChardev *s = PTY_CHARDEV(chr);
@@ -94,8 +78,7 @@ static void pty_chr_rearm_timer(Chardev *chr, int ms)
     g_free(name);
 }
 
-/* Called with chr_write_lock held.  */
-static void pty_chr_update_read_handler_locked(Chardev *chr)
+static void pty_chr_update_read_handler(Chardev *chr)
 {
     PtyChardev *s = PTY_CHARDEV(chr);
     GPollFD pfd;
@@ -117,14 +100,6 @@ static void pty_chr_update_read_handler_locked(Chardev *chr)
     }
 }
 
-static void pty_chr_update_read_handler(Chardev *chr)
-{
-    qemu_mutex_lock(&chr->chr_write_lock);
-    pty_chr_update_read_handler_locked(chr);
-    qemu_mutex_unlock(&chr->chr_write_lock);
-}
-
-/* Called with chr_write_lock held.  */
 static int char_pty_chr_write(Chardev *chr, const uint8_t *buf, int len)
 {
     PtyChardev *s = PTY_CHARDEV(chr);
@@ -179,23 +154,11 @@ static gboolean pty_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
     return TRUE;
 }
 
-static gboolean qemu_chr_be_generic_open_func(gpointer opaque)
-{
-    Chardev *chr = CHARDEV(opaque);
-    PtyChardev *s = PTY_CHARDEV(opaque);
-
-    s->open_source = NULL;
-    qemu_chr_be_event(chr, CHR_EVENT_OPENED);
-    return FALSE;
-}
-
-/* Called with chr_write_lock held.  */
 static void pty_chr_state(Chardev *chr, int connected)
 {
     PtyChardev *s = PTY_CHARDEV(chr);
 
     if (!connected) {
-        pty_chr_open_src_cancel(s);
         remove_fd_in_watch(chr);
         s->connected = 0;
         /* (re-)connect poll interval for idle guests: once per second.
@@ -205,13 +168,8 @@ static void pty_chr_state(Chardev *chr, int connected)
     } else {
         pty_chr_timer_cancel(s);
         if (!s->connected) {
-            g_assert(s->open_source == NULL);
-            s->open_source = g_idle_source_new();
             s->connected = 1;
-            g_source_set_callback(s->open_source,
-                                  qemu_chr_be_generic_open_func,
-                                  chr, NULL);
-            g_source_attach(s->open_source, chr->gcontext);
+            qemu_chr_be_event(chr, CHR_EVENT_OPENED);
         }
         if (!chr->gsource) {
             chr->gsource = io_add_watch_poll(chr, s->ioc,
@@ -227,11 +185,9 @@ static void char_pty_finalize(Object *obj)
     Chardev *chr = CHARDEV(obj);
     PtyChardev *s = PTY_CHARDEV(obj);
 
-    qemu_mutex_lock(&chr->chr_write_lock);
     pty_chr_state(chr, 0);
     object_unref(OBJECT(s->ioc));
     pty_chr_timer_cancel(s);
-    qemu_mutex_unlock(&chr->chr_write_lock);
     qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
 
-- 
2.20.1.519.g8feddda32c

Re: [Qemu-devel] [PATCH v2 3/6] terminal3270: do not use backend timer sources

[Cornelia Huck]

On Wed,  6 Feb 2019 18:43:25 +0100
Marc-André Lureau <marcandre.lureau@redhat.com> wrote:

> terminal3270 uses the front-end side of the chardev. It shouldn't
> create sources from backend side context (with backend
> functions).
> 
> send_timing_mark_cb calls qemu_chr_fe_write_all() which should be
> thread safe.
> 
> This partially reverts changes from commit
> 2c716ba1506769c9be2caa02f0f6d6e7c00f4304.
> 
> CC: Peter Xu <peterx@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/char/terminal3270.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)

I've verified that 3270 continues to work as before.

Acked-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH v2 3/6] terminal3270: do not use backend timer sources

[Peter Xu]

On Thu, Feb 07, 2019 at 06:14:15PM +0100, Cornelia Huck wrote:
> On Wed,  6 Feb 2019 18:43:25 +0100
> Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> 
> > terminal3270 uses the front-end side of the chardev. It shouldn't
> > create sources from backend side context (with backend
> > functions).
> > 
> > send_timing_mark_cb calls qemu_chr_fe_write_all() which should be
> > thread safe.
> > 
> > This partially reverts changes from commit
> > 2c716ba1506769c9be2caa02f0f6d6e7c00f4304.
> > 
> > CC: Peter Xu <peterx@redhat.com>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  hw/char/terminal3270.c | 15 ++++++---------
> >  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> I've verified that 3270 continues to work as before.
> 
> Acked-by: Cornelia Huck <cohuck@redhat.com>

A pure question: is it broken before this patch?

Asked since I don't understand this patch and what it tries to fix...
E.g., in terminal_init(), qemu_chr_fe_set_handlers() is always passing
NULL as chardev context so AFAICT this patch changes nothing from
functional-wise for now.  Meanwhile, if we pass in some non-NULL into
it in the future, IMHO this patch would break it instead of fixing
anything... anything I've missed?

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 3/6] terminal3270: do not use backend timer sources

[Marc-André Lureau]

Hi

On Mon, Feb 11, 2019 at 8:13 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Thu, Feb 07, 2019 at 06:14:15PM +0100, Cornelia Huck wrote:
> > On Wed,  6 Feb 2019 18:43:25 +0100
> > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> >
> > > terminal3270 uses the front-end side of the chardev. It shouldn't
> > > create sources from backend side context (with backend
> > > functions).
> > >
> > > send_timing_mark_cb calls qemu_chr_fe_write_all() which should be
> > > thread safe.
> > >
> > > This partially reverts changes from commit
> > > 2c716ba1506769c9be2caa02f0f6d6e7c00f4304.
> > >
> > > CC: Peter Xu <peterx@redhat.com>
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  hw/char/terminal3270.c | 15 ++++++---------
> > >  1 file changed, 6 insertions(+), 9 deletions(-)
> >
> > I've verified that 3270 continues to work as before.
> >
> > Acked-by: Cornelia Huck <cohuck@redhat.com>
>
> A pure question: is it broken before this patch?

No

>
> Asked since I don't understand this patch and what it tries to fix...

A front-end shouldn't use backend functions.

> E.g., in terminal_init(), qemu_chr_fe_set_handlers() is always passing
> NULL as chardev context so AFAICT this patch changes nothing from
> functional-wise for now.  Meanwhile, if we pass in some non-NULL into
> it in the future, IMHO this patch would break it instead of fixing
> anything... anything I've missed?
>

If the frontend switches the context and creates timers with this
context, it should do it without using the backend functions.

thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 3/6] terminal3270: do not use backend timer sources

[Peter Xu]

On Mon, Feb 11, 2019 at 10:57:01AM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Feb 11, 2019 at 8:13 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Thu, Feb 07, 2019 at 06:14:15PM +0100, Cornelia Huck wrote:
> > > On Wed,  6 Feb 2019 18:43:25 +0100
> > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> > >
> > > > terminal3270 uses the front-end side of the chardev. It shouldn't
> > > > create sources from backend side context (with backend
> > > > functions).
> > > >
> > > > send_timing_mark_cb calls qemu_chr_fe_write_all() which should be
> > > > thread safe.
> > > >
> > > > This partially reverts changes from commit
> > > > 2c716ba1506769c9be2caa02f0f6d6e7c00f4304.
> > > >
> > > > CC: Peter Xu <peterx@redhat.com>
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > ---
> > > >  hw/char/terminal3270.c | 15 ++++++---------
> > > >  1 file changed, 6 insertions(+), 9 deletions(-)
> > >
> > > I've verified that 3270 continues to work as before.
> > >
> > > Acked-by: Cornelia Huck <cohuck@redhat.com>
> >
> > A pure question: is it broken before this patch?
> 
> No
> 
> >
> > Asked since I don't understand this patch and what it tries to fix...
> 
> A front-end shouldn't use backend functions.
> 
> > E.g., in terminal_init(), qemu_chr_fe_set_handlers() is always passing
> > NULL as chardev context so AFAICT this patch changes nothing from
> > functional-wise for now.  Meanwhile, if we pass in some non-NULL into
> > it in the future, IMHO this patch would break it instead of fixing
> > anything... anything I've missed?
> >
> 
> If the frontend switches the context and creates timers with this
> context, it should do it without using the backend functions.

I see your point (assuming that concurrent writes are safe).  But IMHO
we should first discuss on how to fix the existing multi-threading
issues (since it's not really safe yet).  After all this patch offers
nothing real for us for now, and if one day we want to run the chardev
in a single thread again then this will just break again unnoticed.

Frankly speaking I don't think making the chardev to be completely
multi-threading is very attractive to me - chardevs are mostly slow,
otherwise imho better techniques should be used (e.g., virtio).  OOB
did that majorly for only isolating chardev IOs out of main thread so
that chardevs won't be blocked by other unpredictable behaviors (like
userfaults), but it still wants to keep the old code running with as
less change as possible.  For this we can discuss in the other
thread too for sure...

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 3/6] terminal3270: do not use backend timer sources

[Marc-André Lureau]

Hi

On Mon, Feb 11, 2019 at 11:27 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Mon, Feb 11, 2019 at 10:57:01AM +0100, Marc-André Lureau wrote:
> > Hi
> >
> > On Mon, Feb 11, 2019 at 8:13 AM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > On Thu, Feb 07, 2019 at 06:14:15PM +0100, Cornelia Huck wrote:
> > > > On Wed,  6 Feb 2019 18:43:25 +0100
> > > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> > > >
> > > > > terminal3270 uses the front-end side of the chardev. It shouldn't
> > > > > create sources from backend side context (with backend
> > > > > functions).
> > > > >
> > > > > send_timing_mark_cb calls qemu_chr_fe_write_all() which should be
> > > > > thread safe.
> > > > >
> > > > > This partially reverts changes from commit
> > > > > 2c716ba1506769c9be2caa02f0f6d6e7c00f4304.
> > > > >
> > > > > CC: Peter Xu <peterx@redhat.com>
> > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > ---
> > > > >  hw/char/terminal3270.c | 15 ++++++---------
> > > > >  1 file changed, 6 insertions(+), 9 deletions(-)
> > > >
> > > > I've verified that 3270 continues to work as before.
> > > >
> > > > Acked-by: Cornelia Huck <cohuck@redhat.com>
> > >
> > > A pure question: is it broken before this patch?
> >
> > No
> >
> > >
> > > Asked since I don't understand this patch and what it tries to fix...
> >
> > A front-end shouldn't use backend functions.
> >
> > > E.g., in terminal_init(), qemu_chr_fe_set_handlers() is always passing
> > > NULL as chardev context so AFAICT this patch changes nothing from
> > > functional-wise for now.  Meanwhile, if we pass in some non-NULL into
> > > it in the future, IMHO this patch would break it instead of fixing
> > > anything... anything I've missed?
> > >
> >
> > If the frontend switches the context and creates timers with this
> > context, it should do it without using the backend functions.
>
> I see your point (assuming that concurrent writes are safe).  But IMHO
> we should first discuss on how to fix the existing multi-threading
> issues (since it's not really safe yet).  After all this patch offers

Yes, various people are looking at this problem. We need to fix it.

> nothing real for us for now, and if one day we want to run the chardev
> in a single thread again then this will just break again unnoticed.
>

I assume you are not speaking about that patch in particular, when you
say it will "break again unnoticed".

This patch helps when you do whole-tree code review or change over a
particular aspect of the backend code. In my case, I wrote this patch
because I did a review of all the backend context users. And this one
is a "bad" user, I think you agree by now. So let's apply this change.
And let's focus on multi-thread bugs in respective threads.

thanks


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 0/6] chardev: various cleanups and improvements (resend)

[Paolo Bonzini]

On 06/02/19 18:43, Marc-André Lureau wrote:
> Hi,
> 
> Some chardev-related patches I have accumulated.
> Please review, thanks!
> 
> v2:
> - rebased
> - update commit messages
> 
> Marc-André Lureau (6):
>   char: update the mux handlers in class callback
>   char-fe: set_handlers() needs an associated chardev
>   terminal3270: do not use backend timer sources
>   chardev: add a note about frontend sources and context switch
>   char-pty: remove the check for connection on write
>   char-pty: remove write_lock usage
> 
>  include/chardev/char-fe.h  |  5 ++--
>  include/chardev/char-mux.h |  1 -
>  chardev/char-fe.c          | 11 +-------
>  chardev/char-mux.c         |  5 ++--
>  chardev/char-pty.c         | 56 +++-----------------------------------
>  hw/char/terminal3270.c     | 15 ++++------
>  6 files changed, 17 insertions(+), 76 deletions(-)
> 

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

Thanks!

Paolo