[Qemu-devel] [PATCH 2/4] chardev: make qemu_chr_fe_set_handlers() context switching safer

[Marc-André Lureau]

qemu_chr_fe_set_handlers() may switch the context of various
sources. In order to prevent dispatch races from different threads,
let's acquire or freeze the context, do all the source switches, and
then release/resume the contexts. This should help to make context
switching safer.

Signed-off-by: Marc-André Lureau <address@hidden>
---
 include/chardev/char-fe.h |  23 +++++++++
 chardev/char-fe.c         | 103 +++++++++++++++++++++++++++++++++-----
 chardev/char-mux.c        |  14 +++---
 3 files changed, 121 insertions(+), 19 deletions(-)

diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index aa1b864ccd..4051435a1c 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -84,6 +84,14 @@ 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.
  *
+ * A chardev may have multiple main loop sources. In order to prevent
+ * races when switching contexts, the function will temporarily block
+ * the contexts before the source switch to prevent them from
+ * dispatching in different threads concurrently.
+ *
+ * The current and the new @context must be acquirable or
+ * running & dispatched in a loop (the function will hang otherwise).
+ *
  * Without associated Chardev, nothing is changed.
  */
 void qemu_chr_fe_set_handlers_full(CharBackend *b,
@@ -110,6 +118,21 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
                               GMainContext *context,
                               bool set_open);
 
+/**
+ * qemu_chr_fe_set_handlers_internal:
+ *
+ * Same as qemu_chr_fe_set_handlers(), without context freezing.
+ */
+void qemu_chr_fe_set_handlers_internal(CharBackend *b,
+                                       IOCanReadHandler *fd_can_read,
+                                       IOReadHandler *fd_read,
+                                       IOEventHandler *fd_event,
+                                       BackendChangeHandler *be_change,
+                                       void *opaque,
+                                       GMainContext *context,
+                                       bool set_open,
+                                       bool sync_state);
+
 /**
  * qemu_chr_fe_take_focus:
  *
diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index f3530a90e6..90cd7db007 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -246,15 +246,67 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del)
     }
 }
 
-void qemu_chr_fe_set_handlers_full(CharBackend *b,
-                                   IOCanReadHandler *fd_can_read,
-                                   IOReadHandler *fd_read,
-                                   IOEventHandler *fd_event,
-                                   BackendChangeHandler *be_change,
-                                   void *opaque,
-                                   GMainContext *context,
-                                   bool set_open,
-                                   bool sync_state)
+struct MainContextWait {
+    QemuCond cond;
+    QemuMutex lock;
+};
+
+static gboolean
+main_context_wait_cb(gpointer user_data)
+{
+    struct MainContextWait *w = user_data;
+
+    qemu_mutex_lock(&w->lock);
+    qemu_cond_signal(&w->cond);
+    /* wait until switching is over */
+    qemu_cond_wait(&w->cond, &w->lock);
+    qemu_mutex_unlock(&w->lock);
+
+    qemu_mutex_destroy(&w->lock);
+    qemu_cond_destroy(&w->cond);
+    g_free(w);
+
+    return G_SOURCE_REMOVE;
+}
+
+static void
+main_context_wait(struct MainContextWait **wait, GMainContext *ctxt)
+{
+    struct MainContextWait *w = NULL;
+
+    if (!g_main_context_acquire(ctxt)) {
+        w = g_new0(struct MainContextWait, 1);
+        qemu_mutex_init(&w->lock);
+        qemu_cond_init(&w->cond);
+        qemu_mutex_lock(&w->lock);
+        g_main_context_invoke(ctxt, main_context_wait_cb, w);
+        /* wait for the context to freeze */
+        qemu_cond_wait(&w->cond, &w->lock);
+        qemu_mutex_unlock(&w->lock);
+    }
+
+    *wait = w;
+}
+
+static void
+main_context_resume(struct MainContextWait *wait, GMainContext *ctxt)
+{
+    if (wait) {
+        qemu_cond_signal(&wait->cond);
+    } else {
+        g_main_context_release(ctxt);
+    }
+}
+
+void qemu_chr_fe_set_handlers_internal(CharBackend *b,
+                                       IOCanReadHandler *fd_can_read,
+                                       IOReadHandler *fd_read,
+                                       IOEventHandler *fd_event,
+                                       BackendChangeHandler *be_change,
+                                       void *opaque,
+                                       GMainContext *new_context,
+                                       bool set_open,
+                                       bool sync_state)
 {
     Chardev *s;
     int fe_open;
@@ -276,7 +328,7 @@ void qemu_chr_fe_set_handlers_full(CharBackend *b,
     b->chr_be_change = be_change;
     b->opaque = opaque;
 
-    qemu_chr_be_update_read_handlers(s, context);
+    qemu_chr_be_update_read_handlers(s, new_context);
 
     if (set_open) {
         qemu_chr_fe_set_open(b, fe_open);
@@ -292,6 +344,34 @@ void qemu_chr_fe_set_handlers_full(CharBackend *b,
     }
 }
 
+void qemu_chr_fe_set_handlers_full(CharBackend *b,
+                                   IOCanReadHandler *fd_can_read,
+                                   IOReadHandler *fd_read,
+                                   IOEventHandler *fd_event,
+                                   BackendChangeHandler *be_change,
+                                   void *opaque,
+                                   GMainContext *new_context,
+                                   bool set_open,
+                                   bool sync_state)
+{
+    GMainContext *old_context = b->chr->gcontext;
+    struct MainContextWait *old_ctxt_wait, *new_ctxt_wait;
+
+    main_context_wait(&old_ctxt_wait, old_context);
+    if (old_context != new_context) {
+        main_context_wait(&new_ctxt_wait, new_context);
+    }
+
+    qemu_chr_fe_set_handlers_internal(b, fd_can_read, fd_read, fd_event,
+                                      be_change, opaque, new_context,
+                                      set_open, sync_state);
+
+    main_context_resume(old_ctxt_wait, old_context);
+    if (old_context != new_context) {
+        main_context_resume(new_ctxt_wait, new_context);
+    }
+}
+
 void qemu_chr_fe_set_handlers(CharBackend *b,
                               IOCanReadHandler *fd_can_read,
                               IOReadHandler *fd_read,
@@ -302,8 +382,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
                               bool set_open)
 {
     qemu_chr_fe_set_handlers_full(b, fd_can_read, fd_read, fd_event, be_change,
-                                  opaque, context, set_open,
-                                  true);
+                                  opaque, context, set_open, true);
 }
 
 void qemu_chr_fe_take_focus(CharBackend *b)
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 23aa82125d..9830cc4b37 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -283,13 +283,13 @@ static void mux_chr_update_read_handlers(Chardev *chr)
     MuxChardev *d = MUX_CHARDEV(chr);
 
     /* Fix up the real driver with mux routines */
-    qemu_chr_fe_set_handlers_full(&d->chr,
-                                  mux_chr_can_read,
-                                  mux_chr_read,
-                                  mux_chr_event,
-                                  NULL,
-                                  chr,
-                                  chr->gcontext, true, false);
+    qemu_chr_fe_set_handlers_internal(&d->chr,
+                                      mux_chr_can_read,
+                                      mux_chr_read,
+                                      mux_chr_event,
+                                      NULL,
+                                      chr,
+                                      chr->gcontext, true, false);
 }
 
 void mux_set_focus(Chardev *chr, int focus)
-- 
2.21.0.rc1