qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] chardev: fix mess in OPENED/CLOSED events when muxed
@ 2018-11-01 14:55 Artem Pisarenko
  2018-11-03  4:06 ` no-reply
  2018-11-04 14:00 ` Marc-André Lureau
  0 siblings, 2 replies; 5+ messages in thread
From: Artem Pisarenko @ 2018-11-01 14:55 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, Marc-André Lureau, Artem Pisarenko

When chardev is multiplexed (mux=on) there are a lot of cases, when
CHR_EVENT_OPENED/CHR_EVENT_CLOSED events pairing (expected from
frontend side) is broken. There are either generation of multiple
repeated or extra CHR_EVENT_OPENED events, or CHR_EVENT_CLOSED just
isn't generated at all (when it does with mux=off).
Fix that.

Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
---

Notes:
    This issue actually more complex. Idea of generating events from inside function called '*_set_handlers' isn't good, at least its implicit nature, and especially a fact, that function decides about open state (see 'fe_open' variable), but generates event only in one direction. Combined with 'mux_chr_set_handlers()' hack this makes things even worse.
    Better solution is to change fe interface and rewrite all frontends code (a lot of stuff in hw/char/* and somewhere else).
    Although this patch doesn't fix any issue/bug (known to me), it prevents them in future. Also it optimizes emulation performance by avoiding extra activity.
    I did several trivial tests on x86_64 target and seems like nothing broken.

 chardev/char-fe.c          |  9 ++++++---
 chardev/char-mux.c         | 13 ++++++++-----
 include/chardev/char-mux.h |  2 +-
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index a8931f7..31cf7f0 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -257,6 +257,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
 {
     Chardev *s;
     int fe_open;
+    static __thread bool mux_reentered;
 
     s = b->chr;
     if (!s) {
@@ -284,14 +285,16 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
     if (fe_open) {
         qemu_chr_fe_take_focus(b);
         /* We're connecting to an already opened device, so let's make sure we
-           also get the open event */
-        if (s->be_open) {
+           also get the open event (hack: except when chardev is muxed) */
+        if (s->be_open && !mux_reentered) {
             qemu_chr_be_event(s, CHR_EVENT_OPENED);
         }
     }
 
     if (CHARDEV_IS_MUX(s)) {
-        mux_chr_set_handlers(s, context);
+        mux_reentered = true;
+        mux_chr_set_handlers(s, fe_open, context);
+        mux_reentered = false;
     }
 }
 
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 6055e76..9244802 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -272,21 +272,24 @@ static void char_mux_finalize(Object *obj)
     for (i = 0; i < d->mux_cnt; i++) {
         CharBackend *be = d->backends[i];
         if (be) {
+            if (be->chr && be->chr->be_open) {
+                qemu_chr_be_event(be->chr, CHR_EVENT_CLOSED);
+            }
             be->chr = NULL;
         }
     }
     qemu_chr_fe_deinit(&d->chr, false);
 }
 
-void mux_chr_set_handlers(Chardev *chr, GMainContext *context)
+void mux_chr_set_handlers(Chardev *chr, bool is_open, GMainContext *context)
 {
     MuxChardev *d = MUX_CHARDEV(chr);
 
     /* Fix up the real driver with mux routines */
     qemu_chr_fe_set_handlers(&d->chr,
-                             mux_chr_can_read,
-                             mux_chr_read,
-                             mux_chr_event,
+                             is_open ? mux_chr_can_read : NULL,
+                             is_open ? mux_chr_read : NULL,
+                             is_open ? mux_chr_event : NULL,
                              NULL,
                              chr,
                              context, true);
@@ -367,7 +370,7 @@ static int open_muxes(Chardev *chr)
      * mark mux as OPENED so any new FEs will immediately receive
      * OPENED event
      */
-    qemu_chr_be_event(chr, CHR_EVENT_OPENED);
+    chr->be_open = 1;
 
     return 0;
 }
diff --git a/include/chardev/char-mux.h b/include/chardev/char-mux.h
index 1e13187..4b4df6e 100644
--- a/include/chardev/char-mux.h
+++ b/include/chardev/char-mux.h
@@ -55,7 +55,7 @@ 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_chr_set_handlers(Chardev *chr, bool is_open, GMainContext *context);
 void mux_set_focus(Chardev *chr, int focus);
 void mux_chr_send_all_event(Chardev *chr, int event);
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-11-05  7:29 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-01 14:55 [Qemu-devel] [PATCH] chardev: fix mess in OPENED/CLOSED events when muxed Artem Pisarenko
2018-11-03  4:06 ` no-reply
2018-11-04 14:00 ` Marc-André Lureau
2018-11-05  7:22   ` Artem Pisarenko
2018-11-05  7:29     ` Marc-André Lureau

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).