This patch implements multiplexing capability of several backend
devices, which opens up an opportunity to use a single frontend
device on the guest, which can be manipulated from several
backend devices.

The idea of the change is trivial: keep list of backend devices
(up to 4), init them on demand and forward data buffer back and
forth.

The following is QEMU command line example:

  -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
  -chardev vc,id=vc0 \
  -chardev mux,id=mux0,chardev=vc0,,sock0 \
  -device virtconsole,chardev=mux0 \
  -vnc 0.0.0.0:0

Which creates 2 backend devices: text virtual console (`vc0`)
and a socket (`sock0`) connected to the single virtio hvc
console with the multiplexer (`mux0`) help. `vc0` renders
text to an image, which can be shared over the VNC protocol.
`sock0` is a socket backend which provides biderectional
communication to the virtio hvc console.

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-devel@nongnu.org
---
 chardev/char-fe.c          |  14 +++--
 chardev/char-mux.c         | 120 +++++++++++++++++++++++++++++--------
 chardev/char.c             |   2 +-
 chardev/chardev-internal.h |   7 ++-
 4 files changed, 111 insertions(+), 32 deletions(-)

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index b214ba3802b1..d1f67338084d 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -197,16 +197,22 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
         if (CHARDEV_IS_MUX(s)) {
             MuxChardev *d = MUX_CHARDEV(s);
 
-            if (d->mux_cnt >= MAX_MUX) {
+            if (d->fe_cnt >= MAX_MUX) {
                 error_setg(errp,
                            "too many uses of multiplexed chardev '%s'"
                            " (maximum is " stringify(MAX_MUX) ")",
                            s->label);
                 return false;
             }
-
-            d->backends[d->mux_cnt] = b;
-            tag = d->mux_cnt++;
+            if (d->fe_cnt > 0 && d->be_cnt > 1) {
+                error_setg(errp,
+                           "multiplexed chardev '%s' is already used "
+                           "for backend multiplexing",
+                           s->label);
+                return false;
+            }
+            d->backends[d->fe_cnt] = b;
+            tag = d->fe_cnt++;
         } else if (s->be) {
             error_setg(errp, "chardev '%s' is already in use", s->label);
             return false;
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index ee2d47b20d9b..82f728b5caf8 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -26,6 +26,7 @@
 #include "qapi/error.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
+#include "qemu/cutils.h"
 #include "chardev/char.h"
 #include "sysemu/block-backend.h"
 #include "qapi/qapi-commands-control.h"
@@ -40,13 +41,39 @@
  */
 static bool muxes_opened = true;
 
+/* Write to all backends  */
+static int mux_chr_fe_write(MuxChardev *mux, const uint8_t *buf, int len)
+{
+        int r, ret = -1, i;
+
+        for (i = 0; i < mux->be_cnt; i++) {
+                r = qemu_chr_fe_write(&mux->chrs[i], buf, len);
+                ret = ret < 0 ? r : MAX(r, ret);
+        }
+
+        return ret;
+}
+
+/* Write to all backends  */
+static int mux_chr_fe_write_all(MuxChardev *mux, const uint8_t *buf, int len)
+{
+        int r, ret = -1, i;
+
+        for (i = 0; i < mux->be_cnt; i++) {
+                r = qemu_chr_fe_write_all(&mux->chrs[i], buf, len);
+                ret = ret < 0 ? r : MAX(r, ret);
+        }
+
+        return ret;
+}
+
 /* Called with chr_write_lock held.  */
 static int mux_chr_write(Chardev *chr, const uint8_t *buf, int len)
 {
     MuxChardev *d = MUX_CHARDEV(chr);
     int ret;
     if (!d->timestamps) {
-        ret = qemu_chr_fe_write(&d->chr, buf, len);
+        ret = mux_chr_fe_write(d, buf, len);
     } else {
         int i;
 
@@ -71,11 +98,11 @@ static int mux_chr_write(Chardev *chr, const uint8_t *buf, int len)
                          (int)(ti % 1000));
                 /* XXX this blocks entire thread. Rewrite to use
                  * qemu_chr_fe_write and background I/O callbacks */
-                qemu_chr_fe_write_all(&d->chr,
+                mux_chr_fe_write_all(d,
                                       (uint8_t *)buf1, strlen(buf1));
                 d->linestart = 0;
             }
-            ret += qemu_chr_fe_write(&d->chr, buf + i, 1);
+            ret += mux_chr_fe_write(d, buf + i, 1);
             if (buf[i] == '\n') {
                 d->linestart = 1;
             }
@@ -168,9 +195,9 @@ static int mux_proc_byte(Chardev *chr, MuxChardev *d, int ch)
             qemu_chr_be_event(chr, CHR_EVENT_BREAK);
             break;
         case 'c':
-            assert(d->mux_cnt > 0); /* handler registered with first fe */
+            assert(d->fe_cnt > 0); /* handler registered with first fe */
             /* Switch to the next registered device */
-            mux_set_focus(chr, (d->focus + 1) % d->mux_cnt);
+            mux_set_focus(chr, (d->focus + 1) % d->fe_cnt);
             break;
         case 't':
             d->timestamps = !d->timestamps;
@@ -248,8 +275,8 @@ void mux_chr_send_all_event(Chardev *chr, QEMUChrEvent event)
         return;
     }
 
-    /* Send the event to all registered listeners */
-    for (i = 0; i < d->mux_cnt; i++) {
+    /* Send the event to all registered frontend listeners */
+    for (i = 0; i < d->fe_cnt; i++) {
         mux_chr_send_event(d, i, event);
     }
 }
@@ -262,8 +289,16 @@ static void mux_chr_event(void *opaque, QEMUChrEvent event)
 static GSource *mux_chr_add_watch(Chardev *s, GIOCondition cond)
 {
     MuxChardev *d = MUX_CHARDEV(s);
-    Chardev *chr = qemu_chr_fe_get_driver(&d->chr);
-    ChardevClass *cc = CHARDEV_GET_CLASS(chr);
+    Chardev *chr;
+    ChardevClass *cc;
+
+    if (d->be_cnt > 1) {
+            /* TODO: multiple backends have to be combined on a single watch */
+            return NULL;
+    }
+
+    chr = qemu_chr_fe_get_driver(&d->chrs[0]);
+    cc = CHARDEV_GET_CLASS(chr);
 
     if (!cc->chr_add_watch) {
         return NULL;
@@ -277,27 +312,32 @@ static void char_mux_finalize(Object *obj)
     MuxChardev *d = MUX_CHARDEV(obj);
     int i;
 
-    for (i = 0; i < d->mux_cnt; i++) {
+    for (i = 0; i < d->fe_cnt; i++) {
         CharBackend *be = d->backends[i];
         if (be) {
             be->chr = NULL;
         }
     }
-    qemu_chr_fe_deinit(&d->chr, false);
+    for (i = 0; i < d->be_cnt; i++) {
+        qemu_chr_fe_deinit(&d->chrs[i], false);
+    }
 }
 
 static void mux_chr_update_read_handlers(Chardev *chr)
 {
     MuxChardev *d = MUX_CHARDEV(chr);
+    int i;
 
-    /* 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);
+    for (i = 0; i < d->be_cnt; i++) {
+        /* Fix up the real driver with mux routines */
+        qemu_chr_fe_set_handlers_full(&d->chrs[i],
+                                      mux_chr_can_read,
+                                      mux_chr_read,
+                                      mux_chr_event,
+                                      NULL,
+                                      chr,
+                                      chr->gcontext, true, false);
+    }
 }
 
 void mux_set_focus(Chardev *chr, int focus)
@@ -305,7 +345,7 @@ void mux_set_focus(Chardev *chr, int focus)
     MuxChardev *d = MUX_CHARDEV(chr);
 
     assert(focus >= 0);
-    assert(focus < d->mux_cnt);
+    assert(focus < d->fe_cnt);
 
     if (d->focus != -1) {
         mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_OUT);
@@ -324,19 +364,49 @@ static void qemu_chr_open_mux(Chardev *chr,
     ChardevMux *mux = backend->u.mux.data;
     Chardev *drv;
     MuxChardev *d = MUX_CHARDEV(chr);
-
-    drv = qemu_chr_find(mux->chardev);
-    if (drv == NULL) {
-        error_setg(errp, "mux: base chardev %s not found", mux->chardev);
+    const char *offset, *chardevs;
+    int length, i;
+
+    if (d->fe_cnt > 1) {
+        error_setg(errp,
+                   "multiplexed chardev '%s' is already used "
+                   "for frontend multiplexing",
+                   chr->label);
         return;
     }
 
+    chardevs = mux->chardev;
+    for (i = 0; i < MAX_MUX; i++) {
+        char *chardev;
+
+        offset = qemu_strchrnul(chardevs, ',');
+        length = offset - chardevs;
+        if (!length) {
+            break;
+        }
+        chardev = strndupa(chardevs, length);
+        chardevs += length + 1;
+        drv = qemu_chr_find(chardev);
+        if (drv == NULL) {
+            error_setg(errp, "mux: base chardev %s not found", chardev);
+            goto deinit_on_error;
+        }
+        qemu_chr_fe_init(&d->chrs[i], drv, errp);
+        d->be_cnt += 1;
+    }
     d->focus = -1;
     /* only default to opened state if we've realized the initial
      * set of muxes
      */
     *be_opened = muxes_opened;
-    qemu_chr_fe_init(&d->chr, drv, errp);
+
+    return;
+
+deinit_on_error:
+    for (i = 0; i < d->be_cnt; i++) {
+        qemu_chr_fe_deinit(&d->chrs[i], false);
+    }
+    d->be_cnt = 0;
 }
 
 static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend *backend,
diff --git a/chardev/char.c b/chardev/char.c
index ba847b6e9eff..2643c79e5749 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -333,7 +333,7 @@ static bool qemu_chr_is_busy(Chardev *s)
 {
     if (CHARDEV_IS_MUX(s)) {
         MuxChardev *d = MUX_CHARDEV(s);
-        return d->mux_cnt >= 0;
+        return d->fe_cnt >= 0;
     } else {
         return s->be != NULL;
     }
diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
index 4e03af31476c..72c2e4da7552 100644
--- a/chardev/chardev-internal.h
+++ b/chardev/chardev-internal.h
@@ -35,10 +35,13 @@
 
 struct MuxChardev {
     Chardev parent;
+    /* Linked frontends */
     CharBackend *backends[MAX_MUX];
-    CharBackend chr;
+    /* Linked backends */
+    CharBackend chrs[MAX_MUX];
     int focus;
-    int mux_cnt;
+    int fe_cnt;
+    int be_cnt;
     int term_got_escape;
     int max_size;
     /* Intermediate input buffer catches escape sequences even if the
-- 
2.34.1

Hi Roman

On Fri, Sep 13, 2024 at 8:37 PM Roman Penyaev <r.peniaev@gmail.com> wrote:
>
> This patch implements multiplexing capability of several backend
> devices, which opens up an opportunity to use a single frontend
> device on the guest, which can be manipulated from several
> backend devices.
>
> The idea of the change is trivial: keep list of backend devices
> (up to 4), init them on demand and forward data buffer back and
> forth.
>
> The following is QEMU command line example:
>
>   -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
>   -chardev vc,id=vc0 \
>   -chardev mux,id=mux0,chardev=vc0,,sock0 \
>   -device virtconsole,chardev=mux0 \
>   -vnc 0.0.0.0:0
>
> Which creates 2 backend devices: text virtual console (`vc0`)
> and a socket (`sock0`) connected to the single virtio hvc
> console with the multiplexer (`mux0`) help. `vc0` renders
> text to an image, which can be shared over the VNC protocol.
> `sock0` is a socket backend which provides biderectional
> communication to the virtio hvc console.

I think I would rather implement a new mux for this purpose, like
"mux-be" perhaps?

"mux" has been a bit hidden (behind mux=on) for various reasons, and
is probably not at production quality level.

I am not sure how CLI should handle option arrays these days
(especially with -chardev CLI not being json-friendly).

>
> Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>  chardev/char-fe.c          |  14 +++--
>  chardev/char-mux.c         | 120 +++++++++++++++++++++++++++++--------
>  chardev/char.c             |   2 +-
>  chardev/chardev-internal.h |   7 ++-
>  4 files changed, 111 insertions(+), 32 deletions(-)
>
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index b214ba3802b1..d1f67338084d 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -197,16 +197,22 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
>          if (CHARDEV_IS_MUX(s)) {
>              MuxChardev *d = MUX_CHARDEV(s);
>
> -            if (d->mux_cnt >= MAX_MUX) {
> +            if (d->fe_cnt >= MAX_MUX) {
>                  error_setg(errp,
>                             "too many uses of multiplexed chardev '%s'"
>                             " (maximum is " stringify(MAX_MUX) ")",
>                             s->label);
>                  return false;
>              }
> -
> -            d->backends[d->mux_cnt] = b;
> -            tag = d->mux_cnt++;
> +            if (d->fe_cnt > 0 && d->be_cnt > 1) {
> +                error_setg(errp,
> +                           "multiplexed chardev '%s' is already used "
> +                           "for backend multiplexing",
> +                           s->label);
> +                return false;
> +            }
> +            d->backends[d->fe_cnt] = b;
> +            tag = d->fe_cnt++;
>          } else if (s->be) {
>              error_setg(errp, "chardev '%s' is already in use", s->label);
>              return false;
> diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> index ee2d47b20d9b..82f728b5caf8 100644
> --- a/chardev/char-mux.c
> +++ b/chardev/char-mux.c
> @@ -26,6 +26,7 @@
>  #include "qapi/error.h"
>  #include "qemu/module.h"
>  #include "qemu/option.h"
> +#include "qemu/cutils.h"
>  #include "chardev/char.h"
>  #include "sysemu/block-backend.h"
>  #include "qapi/qapi-commands-control.h"
> @@ -40,13 +41,39 @@
>   */
>  static bool muxes_opened = true;
>
> +/* Write to all backends  */
> +static int mux_chr_fe_write(MuxChardev *mux, const uint8_t *buf, int len)
> +{
> +        int r, ret = -1, i;
> +
> +        for (i = 0; i < mux->be_cnt; i++) {
> +                r = qemu_chr_fe_write(&mux->chrs[i], buf, len);
> +                ret = ret < 0 ? r : MAX(r, ret);

I think it should be conservative and fail early if one of the backend
fails. Also if different frontends can write different amounts, there
needs to be some buffering... or it should always use write_all()
which has also a shortcoming since it blocks the thread.

> +        }
> +
> +        return ret;
> +}
> +
> +/* Write to all backends  */
> +static int mux_chr_fe_write_all(MuxChardev *mux, const uint8_t *buf, int len)
> +{
> +        int r, ret = -1, i;
> +
> +        for (i = 0; i < mux->be_cnt; i++) {
> +                r = qemu_chr_fe_write_all(&mux->chrs[i], buf, len);
> +                ret = ret < 0 ? r : MAX(r, ret);
> +        }
> +
> +        return ret;
> +}
> +
>  /* Called with chr_write_lock held.  */
>  static int mux_chr_write(Chardev *chr, const uint8_t *buf, int len)
>  {
>      MuxChardev *d = MUX_CHARDEV(chr);
>      int ret;
>      if (!d->timestamps) {
> -        ret = qemu_chr_fe_write(&d->chr, buf, len);
> +        ret = mux_chr_fe_write(d, buf, len);
>      } else {
>          int i;
>
> @@ -71,11 +98,11 @@ static int mux_chr_write(Chardev *chr, const uint8_t *buf, int len)
>                           (int)(ti % 1000));
>                  /* XXX this blocks entire thread. Rewrite to use
>                   * qemu_chr_fe_write and background I/O callbacks */
> -                qemu_chr_fe_write_all(&d->chr,
> +                mux_chr_fe_write_all(d,
>                                        (uint8_t *)buf1, strlen(buf1));
>                  d->linestart = 0;
>              }
> -            ret += qemu_chr_fe_write(&d->chr, buf + i, 1);
> +            ret += mux_chr_fe_write(d, buf + i, 1);
>              if (buf[i] == '\n') {
>                  d->linestart = 1;
>              }
> @@ -168,9 +195,9 @@ static int mux_proc_byte(Chardev *chr, MuxChardev *d, int ch)
>              qemu_chr_be_event(chr, CHR_EVENT_BREAK);
>              break;
>          case 'c':
> -            assert(d->mux_cnt > 0); /* handler registered with first fe */
> +            assert(d->fe_cnt > 0); /* handler registered with first fe */
>              /* Switch to the next registered device */
> -            mux_set_focus(chr, (d->focus + 1) % d->mux_cnt);
> +            mux_set_focus(chr, (d->focus + 1) % d->fe_cnt);
>              break;
>          case 't':
>              d->timestamps = !d->timestamps;
> @@ -248,8 +275,8 @@ void mux_chr_send_all_event(Chardev *chr, QEMUChrEvent event)
>          return;
>      }
>
> -    /* Send the event to all registered listeners */
> -    for (i = 0; i < d->mux_cnt; i++) {
> +    /* Send the event to all registered frontend listeners */
> +    for (i = 0; i < d->fe_cnt; i++) {
>          mux_chr_send_event(d, i, event);
>      }
>  }
> @@ -262,8 +289,16 @@ static void mux_chr_event(void *opaque, QEMUChrEvent event)
>  static GSource *mux_chr_add_watch(Chardev *s, GIOCondition cond)
>  {
>      MuxChardev *d = MUX_CHARDEV(s);
> -    Chardev *chr = qemu_chr_fe_get_driver(&d->chr);
> -    ChardevClass *cc = CHARDEV_GET_CLASS(chr);
> +    Chardev *chr;
> +    ChardevClass *cc;
> +
> +    if (d->be_cnt > 1) {
> +            /* TODO: multiple backends have to be combined on a single watch */

I think this must be done, otherwise there is a severe limitation.

> +            return NULL;
> +    }
> +
> +    chr = qemu_chr_fe_get_driver(&d->chrs[0]);
> +    cc = CHARDEV_GET_CLASS(chr);
>
>      if (!cc->chr_add_watch) {
>          return NULL;
> @@ -277,27 +312,32 @@ static void char_mux_finalize(Object *obj)
>      MuxChardev *d = MUX_CHARDEV(obj);
>      int i;
>
> -    for (i = 0; i < d->mux_cnt; i++) {
> +    for (i = 0; i < d->fe_cnt; i++) {
>          CharBackend *be = d->backends[i];
>          if (be) {
>              be->chr = NULL;
>          }
>      }
> -    qemu_chr_fe_deinit(&d->chr, false);
> +    for (i = 0; i < d->be_cnt; i++) {
> +        qemu_chr_fe_deinit(&d->chrs[i], false);
> +    }
>  }
>
>  static void mux_chr_update_read_handlers(Chardev *chr)
>  {
>      MuxChardev *d = MUX_CHARDEV(chr);
> +    int i;
>
> -    /* 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);
> +    for (i = 0; i < d->be_cnt; i++) {
> +        /* Fix up the real driver with mux routines */
> +        qemu_chr_fe_set_handlers_full(&d->chrs[i],
> +                                      mux_chr_can_read,
> +                                      mux_chr_read,
> +                                      mux_chr_event,
> +                                      NULL,
> +                                      chr,
> +                                      chr->gcontext, true, false);
> +    }
>  }
>
>  void mux_set_focus(Chardev *chr, int focus)
> @@ -305,7 +345,7 @@ void mux_set_focus(Chardev *chr, int focus)
>      MuxChardev *d = MUX_CHARDEV(chr);
>
>      assert(focus >= 0);
> -    assert(focus < d->mux_cnt);
> +    assert(focus < d->fe_cnt);
>
>      if (d->focus != -1) {
>          mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_OUT);
> @@ -324,19 +364,49 @@ static void qemu_chr_open_mux(Chardev *chr,
>      ChardevMux *mux = backend->u.mux.data;
>      Chardev *drv;
>      MuxChardev *d = MUX_CHARDEV(chr);
> -
> -    drv = qemu_chr_find(mux->chardev);
> -    if (drv == NULL) {
> -        error_setg(errp, "mux: base chardev %s not found", mux->chardev);
> +    const char *offset, *chardevs;
> +    int length, i;
> +
> +    if (d->fe_cnt > 1) {
> +        error_setg(errp,
> +                   "multiplexed chardev '%s' is already used "
> +                   "for frontend multiplexing",
> +                   chr->label);
>          return;
>      }
>
> +    chardevs = mux->chardev;
> +    for (i = 0; i < MAX_MUX; i++) {
> +        char *chardev;
> +
> +        offset = qemu_strchrnul(chardevs, ',');
> +        length = offset - chardevs;
> +        if (!length) {
> +            break;
> +        }
> +        chardev = strndupa(chardevs, length);
> +        chardevs += length + 1;
> +        drv = qemu_chr_find(chardev);
> +        if (drv == NULL) {
> +            error_setg(errp, "mux: base chardev %s not found", chardev);
> +            goto deinit_on_error;
> +        }
> +        qemu_chr_fe_init(&d->chrs[i], drv, errp);
> +        d->be_cnt += 1;
> +    }
>      d->focus = -1;
>      /* only default to opened state if we've realized the initial
>       * set of muxes
>       */
>      *be_opened = muxes_opened;
> -    qemu_chr_fe_init(&d->chr, drv, errp);
> +
> +    return;
> +
> +deinit_on_error:
> +    for (i = 0; i < d->be_cnt; i++) {
> +        qemu_chr_fe_deinit(&d->chrs[i], false);
> +    }
> +    d->be_cnt = 0;
>  }
>
>  static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend *backend,
> diff --git a/chardev/char.c b/chardev/char.c
> index ba847b6e9eff..2643c79e5749 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -333,7 +333,7 @@ static bool qemu_chr_is_busy(Chardev *s)
>  {
>      if (CHARDEV_IS_MUX(s)) {
>          MuxChardev *d = MUX_CHARDEV(s);
> -        return d->mux_cnt >= 0;
> +        return d->fe_cnt >= 0;
>      } else {
>          return s->be != NULL;
>      }
> diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
> index 4e03af31476c..72c2e4da7552 100644
> --- a/chardev/chardev-internal.h
> +++ b/chardev/chardev-internal.h
> @@ -35,10 +35,13 @@
>
>  struct MuxChardev {
>      Chardev parent;
> +    /* Linked frontends */
>      CharBackend *backends[MAX_MUX];
> -    CharBackend chr;
> +    /* Linked backends */
> +    CharBackend chrs[MAX_MUX];
>      int focus;
> -    int mux_cnt;
> +    int fe_cnt;
> +    int be_cnt;
>      int term_got_escape;
>      int max_size;
>      /* Intermediate input buffer catches escape sequences even if the
> --
> 2.34.1
>


It would also require some tests and QAPI support.

Hi Marc-André,

On Tue, Sep 17, 2024 at 2:31 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi Roman
>
> On Fri, Sep 13, 2024 at 8:37 PM Roman Penyaev <r.peniaev@gmail.com> wrote:
> >
> > This patch implements multiplexing capability of several backend
> > devices, which opens up an opportunity to use a single frontend
> > device on the guest, which can be manipulated from several
> > backend devices.
> >
> > The idea of the change is trivial: keep list of backend devices
> > (up to 4), init them on demand and forward data buffer back and
> > forth.
> >
> > The following is QEMU command line example:
> >
> >   -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
> >   -chardev vc,id=vc0 \
> >   -chardev mux,id=mux0,chardev=vc0,,sock0 \
> >   -device virtconsole,chardev=mux0 \
> >   -vnc 0.0.0.0:0
> >
> > Which creates 2 backend devices: text virtual console (`vc0`)
> > and a socket (`sock0`) connected to the single virtio hvc
> > console with the multiplexer (`mux0`) help. `vc0` renders
> > text to an image, which can be shared over the VNC protocol.
> > `sock0` is a socket backend which provides biderectional
> > communication to the virtio hvc console.
>
> I think I would rather implement a new mux for this purpose, like
> "mux-be" perhaps?

Do you mean a completely different implementation or just an alias
for the cli needs? Because code of the backend multiplexing nicely fits
current mux and I tried to avoid code duplication.

>
> "mux" has been a bit hidden (behind mux=on) for various reasons, and
> is probably not at production quality level.

Well, indeed creating "chardev mux" is not described in the doc, but you
can do this anyway :) Here I just followed the same pattern as we do for
other devices: "chardev NAME". Having a "mux-be" (or any other) alias
won't expose the default "mux", so configurations can be separated.
Is that what you meant?

Also, mux is used implicitly for various of configurations, this is
described in the manual:

   Note that some other command line options may implicitly create
   multiplexed character backends; for instance -serial mon:stdio creates
   a multiplexed stdio backend connected to the serial port and the QEMU
   monitor, and -nographic also multiplexes the console and the
monitor to stdio.

So it seems to me mux is used (tested) quite extensively.

>
> I am not sure how CLI should handle option arrays these days
> (especially with -chardev CLI not being json-friendly).

By CLI do you mean the default QEMU command line interface?
The command line with arrays (double commas ",,") I specified above
works fine. Can you suggest any other tool (or json formatting) you have
in mind so I can verify?

>
> >
> > Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
> > Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: qemu-devel@nongnu.org
> > ---
> >  chardev/char-fe.c          |  14 +++--
> >  chardev/char-mux.c         | 120 +++++++++++++++++++++++++++++--------
> >  chardev/char.c             |   2 +-
> >  chardev/chardev-internal.h |   7 ++-
> >  4 files changed, 111 insertions(+), 32 deletions(-)
> >
> > diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> > index b214ba3802b1..d1f67338084d 100644
> > --- a/chardev/char-fe.c
> > +++ b/chardev/char-fe.c
> > @@ -197,16 +197,22 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
> >          if (CHARDEV_IS_MUX(s)) {
> >              MuxChardev *d = MUX_CHARDEV(s);
> >
> > -            if (d->mux_cnt >= MAX_MUX) {
> > +            if (d->fe_cnt >= MAX_MUX) {
> >                  error_setg(errp,
> >                             "too many uses of multiplexed chardev '%s'"
> >                             " (maximum is " stringify(MAX_MUX) ")",
> >                             s->label);
> >                  return false;
> >              }
> > -
> > -            d->backends[d->mux_cnt] = b;
> > -            tag = d->mux_cnt++;
> > +            if (d->fe_cnt > 0 && d->be_cnt > 1) {
> > +                error_setg(errp,
> > +                           "multiplexed chardev '%s' is already used "
> > +                           "for backend multiplexing",
> > +                           s->label);
> > +                return false;
> > +            }
> > +            d->backends[d->fe_cnt] = b;
> > +            tag = d->fe_cnt++;
> >          } else if (s->be) {
> >              error_setg(errp, "chardev '%s' is already in use", s->label);
> >              return false;
> > diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> > index ee2d47b20d9b..82f728b5caf8 100644
> > --- a/chardev/char-mux.c
> > +++ b/chardev/char-mux.c
> > @@ -26,6 +26,7 @@
> >  #include "qapi/error.h"
> >  #include "qemu/module.h"
> >  #include "qemu/option.h"
> > +#include "qemu/cutils.h"
> >  #include "chardev/char.h"
> >  #include "sysemu/block-backend.h"
> >  #include "qapi/qapi-commands-control.h"
> > @@ -40,13 +41,39 @@
> >   */
> >  static bool muxes_opened = true;
> >
> > +/* Write to all backends  */
> > +static int mux_chr_fe_write(MuxChardev *mux, const uint8_t *buf, int len)
> > +{
> > +        int r, ret = -1, i;
> > +
> > +        for (i = 0; i < mux->be_cnt; i++) {
> > +                r = qemu_chr_fe_write(&mux->chrs[i], buf, len);
> > +                ret = ret < 0 ? r : MAX(r, ret);
>
> I think it should be conservative and fail early if one of the backend
> fails. Also if different frontends can write different amounts, there
> needs to be some buffering... or it should always use write_all()
> which has also a shortcoming since it blocks the thread.

Yes, early fail and buffers make sense.

>
> > +        }
> > +
> > +        return ret;
> > +}
> > +
> > +/* Write to all backends  */
> > +static int mux_chr_fe_write_all(MuxChardev *mux, const uint8_t *buf, int len)
> > +{
> > +        int r, ret = -1, i;
> > +
> > +        for (i = 0; i < mux->be_cnt; i++) {
> > +                r = qemu_chr_fe_write_all(&mux->chrs[i], buf, len);
> > +                ret = ret < 0 ? r : MAX(r, ret);
> > +        }
> > +
> > +        return ret;
> > +}
> > +
> >  /* Called with chr_write_lock held.  */
> >  static int mux_chr_write(Chardev *chr, const uint8_t *buf, int len)
> >  {
> >      MuxChardev *d = MUX_CHARDEV(chr);
> >      int ret;
> >      if (!d->timestamps) {
> > -        ret = qemu_chr_fe_write(&d->chr, buf, len);
> > +        ret = mux_chr_fe_write(d, buf, len);
> >      } else {
> >          int i;
> >
> > @@ -71,11 +98,11 @@ static int mux_chr_write(Chardev *chr, const uint8_t *buf, int len)
> >                           (int)(ti % 1000));
> >                  /* XXX this blocks entire thread. Rewrite to use
> >                   * qemu_chr_fe_write and background I/O callbacks */
> > -                qemu_chr_fe_write_all(&d->chr,
> > +                mux_chr_fe_write_all(d,
> >                                        (uint8_t *)buf1, strlen(buf1));
> >                  d->linestart = 0;
> >              }
> > -            ret += qemu_chr_fe_write(&d->chr, buf + i, 1);
> > +            ret += mux_chr_fe_write(d, buf + i, 1);
> >              if (buf[i] == '\n') {
> >                  d->linestart = 1;
> >              }
> > @@ -168,9 +195,9 @@ static int mux_proc_byte(Chardev *chr, MuxChardev *d, int ch)
> >              qemu_chr_be_event(chr, CHR_EVENT_BREAK);
> >              break;
> >          case 'c':
> > -            assert(d->mux_cnt > 0); /* handler registered with first fe */
> > +            assert(d->fe_cnt > 0); /* handler registered with first fe */
> >              /* Switch to the next registered device */
> > -            mux_set_focus(chr, (d->focus + 1) % d->mux_cnt);
> > +            mux_set_focus(chr, (d->focus + 1) % d->fe_cnt);
> >              break;
> >          case 't':
> >              d->timestamps = !d->timestamps;
> > @@ -248,8 +275,8 @@ void mux_chr_send_all_event(Chardev *chr, QEMUChrEvent event)
> >          return;
> >      }
> >
> > -    /* Send the event to all registered listeners */
> > -    for (i = 0; i < d->mux_cnt; i++) {
> > +    /* Send the event to all registered frontend listeners */
> > +    for (i = 0; i < d->fe_cnt; i++) {
> >          mux_chr_send_event(d, i, event);
> >      }
> >  }
> > @@ -262,8 +289,16 @@ static void mux_chr_event(void *opaque, QEMUChrEvent event)
> >  static GSource *mux_chr_add_watch(Chardev *s, GIOCondition cond)
> >  {
> >      MuxChardev *d = MUX_CHARDEV(s);
> > -    Chardev *chr = qemu_chr_fe_get_driver(&d->chr);
> > -    ChardevClass *cc = CHARDEV_GET_CLASS(chr);
> > +    Chardev *chr;
> > +    ChardevClass *cc;
> > +
> > +    if (d->be_cnt > 1) {
> > +            /* TODO: multiple backends have to be combined on a single watch */
>
> I think this must be done, otherwise there is a severe limitation.

Ok.

>
> > +            return NULL;
> > +    }
> > +
> > +    chr = qemu_chr_fe_get_driver(&d->chrs[0]);
> > +    cc = CHARDEV_GET_CLASS(chr);
> >
> >      if (!cc->chr_add_watch) {
> >          return NULL;
> > @@ -277,27 +312,32 @@ static void char_mux_finalize(Object *obj)
> >      MuxChardev *d = MUX_CHARDEV(obj);
> >      int i;
> >
> > -    for (i = 0; i < d->mux_cnt; i++) {
> > +    for (i = 0; i < d->fe_cnt; i++) {
> >          CharBackend *be = d->backends[i];
> >          if (be) {
> >              be->chr = NULL;
> >          }
> >      }
> > -    qemu_chr_fe_deinit(&d->chr, false);
> > +    for (i = 0; i < d->be_cnt; i++) {
> > +        qemu_chr_fe_deinit(&d->chrs[i], false);
> > +    }
> >  }
> >
> >  static void mux_chr_update_read_handlers(Chardev *chr)
> >  {
> >      MuxChardev *d = MUX_CHARDEV(chr);
> > +    int i;
> >
> > -    /* 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);
> > +    for (i = 0; i < d->be_cnt; i++) {
> > +        /* Fix up the real driver with mux routines */
> > +        qemu_chr_fe_set_handlers_full(&d->chrs[i],
> > +                                      mux_chr_can_read,
> > +                                      mux_chr_read,
> > +                                      mux_chr_event,
> > +                                      NULL,
> > +                                      chr,
> > +                                      chr->gcontext, true, false);
> > +    }
> >  }
> >
> >  void mux_set_focus(Chardev *chr, int focus)
> > @@ -305,7 +345,7 @@ void mux_set_focus(Chardev *chr, int focus)
> >      MuxChardev *d = MUX_CHARDEV(chr);
> >
> >      assert(focus >= 0);
> > -    assert(focus < d->mux_cnt);
> > +    assert(focus < d->fe_cnt);
> >
> >      if (d->focus != -1) {
> >          mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_OUT);
> > @@ -324,19 +364,49 @@ static void qemu_chr_open_mux(Chardev *chr,
> >      ChardevMux *mux = backend->u.mux.data;
> >      Chardev *drv;
> >      MuxChardev *d = MUX_CHARDEV(chr);
> > -
> > -    drv = qemu_chr_find(mux->chardev);
> > -    if (drv == NULL) {
> > -        error_setg(errp, "mux: base chardev %s not found", mux->chardev);
> > +    const char *offset, *chardevs;
> > +    int length, i;
> > +
> > +    if (d->fe_cnt > 1) {
> > +        error_setg(errp,
> > +                   "multiplexed chardev '%s' is already used "
> > +                   "for frontend multiplexing",
> > +                   chr->label);
> >          return;
> >      }
> >
> > +    chardevs = mux->chardev;
> > +    for (i = 0; i < MAX_MUX; i++) {
> > +        char *chardev;
> > +
> > +        offset = qemu_strchrnul(chardevs, ',');
> > +        length = offset - chardevs;
> > +        if (!length) {
> > +            break;
> > +        }
> > +        chardev = strndupa(chardevs, length);
> > +        chardevs += length + 1;
> > +        drv = qemu_chr_find(chardev);
> > +        if (drv == NULL) {
> > +            error_setg(errp, "mux: base chardev %s not found", chardev);
> > +            goto deinit_on_error;
> > +        }
> > +        qemu_chr_fe_init(&d->chrs[i], drv, errp);
> > +        d->be_cnt += 1;
> > +    }
> >      d->focus = -1;
> >      /* only default to opened state if we've realized the initial
> >       * set of muxes
> >       */
> >      *be_opened = muxes_opened;
> > -    qemu_chr_fe_init(&d->chr, drv, errp);
> > +
> > +    return;
> > +
> > +deinit_on_error:
> > +    for (i = 0; i < d->be_cnt; i++) {
> > +        qemu_chr_fe_deinit(&d->chrs[i], false);
> > +    }
> > +    d->be_cnt = 0;
> >  }
> >
> >  static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend *backend,
> > diff --git a/chardev/char.c b/chardev/char.c
> > index ba847b6e9eff..2643c79e5749 100644
> > --- a/chardev/char.c
> > +++ b/chardev/char.c
> > @@ -333,7 +333,7 @@ static bool qemu_chr_is_busy(Chardev *s)
> >  {
> >      if (CHARDEV_IS_MUX(s)) {
> >          MuxChardev *d = MUX_CHARDEV(s);
> > -        return d->mux_cnt >= 0;
> > +        return d->fe_cnt >= 0;
> >      } else {
> >          return s->be != NULL;
> >      }
> > diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
> > index 4e03af31476c..72c2e4da7552 100644
> > --- a/chardev/chardev-internal.h
> > +++ b/chardev/chardev-internal.h
> > @@ -35,10 +35,13 @@
> >
> >  struct MuxChardev {
> >      Chardev parent;
> > +    /* Linked frontends */
> >      CharBackend *backends[MAX_MUX];
> > -    CharBackend chr;
> > +    /* Linked backends */
> > +    CharBackend chrs[MAX_MUX];
> >      int focus;
> > -    int mux_cnt;
> > +    int fe_cnt;
> > +    int be_cnt;
> >      int term_got_escape;
> >      int max_size;
> >      /* Intermediate input buffer catches escape sequences even if the
> > --
> > 2.34.1
> >
>
>
> It would also require some tests and QAPI support.

I will take a look at tests and will try to come up with some extension
for backend multiplexing.

Since the mux object API was not changed and the current change heavily
relies on what is already in the code, do you think there should be any
QAPI related change? In my understanding this should work out of the box
(not tested though).

--
Roman

Hi

On Tue, Sep 17, 2024 at 6:17 PM Roman Penyaev <r.peniaev@gmail.com> wrote:

> Hi Marc-André,
>
> On Tue, Sep 17, 2024 at 2:31 PM Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> >
> > Hi Roman
> >
> > On Fri, Sep 13, 2024 at 8:37 PM Roman Penyaev <r.peniaev@gmail.com>
> wrote:
> > >
> > > This patch implements multiplexing capability of several backend
> > > devices, which opens up an opportunity to use a single frontend
> > > device on the guest, which can be manipulated from several
> > > backend devices.
> > >
> > > The idea of the change is trivial: keep list of backend devices
> > > (up to 4), init them on demand and forward data buffer back and
> > > forth.
> > >
> > > The following is QEMU command line example:
> > >
> > >   -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
> > >   -chardev vc,id=vc0 \
> > >   -chardev mux,id=mux0,chardev=vc0,,sock0 \
> > >   -device virtconsole,chardev=mux0 \
> > >   -vnc 0.0.0.0:0
> > >
> > > Which creates 2 backend devices: text virtual console (`vc0`)
> > > and a socket (`sock0`) connected to the single virtio hvc
> > > console with the multiplexer (`mux0`) help. `vc0` renders
> > > text to an image, which can be shared over the VNC protocol.
> > > `sock0` is a socket backend which provides biderectional
> > > communication to the virtio hvc console.
> >
> > I think I would rather implement a new mux for this purpose, like
> > "mux-be" perhaps?
>
> Do you mean a completely different implementation or just an alias
> for the cli needs? Because code of the backend multiplexing nicely fits
> current mux and I tried to avoid code duplication.
>

It's not the same behaviour though, and discoverability/compatibility would
be easier to handle. There is also various code in mux that isn't needed.

You can still factorize common code, or have a common base class.
Eventually, the two chardev can be from the same file. I don't know if that
makes sense.


> >
> > "mux" has been a bit hidden (behind mux=on) for various reasons, and
> > is probably not at production quality level.
>
> Well, indeed creating "chardev mux" is not described in the doc, but you
> can do this anyway :) Here I just followed the same pattern as we do for
> other devices: "chardev NAME". Having a "mux-be" (or any other) alias
> won't expose the default "mux", so configurations can be separated.
> Is that what you meant?
>

yes, this would help in general misconfiguration (although -chardev option
handling has a lot to be desired)


>
> Also, mux is used implicitly for various of configurations, this is
> described in the manual:
>
>    Note that some other command line options may implicitly create
>    multiplexed character backends; for instance -serial mon:stdio creates
>    a multiplexed stdio backend connected to the serial port and the QEMU
>    monitor, and -nographic also multiplexes the console and the
> monitor to stdio.
>
> So it seems to me mux is used (tested) quite extensively.
>

kinda, it's used by end-users who run qemu from the CLI. Probably less in
production systems.


>
> >
> > I am not sure how CLI should handle option arrays these days
> > (especially with -chardev CLI not being json-friendly).
>
> By CLI do you mean the default QEMU command line interface?
>

yes


> The command line with arrays (double commas ",,") I specified above
> works fine. Can you suggest any other tool (or json formatting) you have
> in mind so I can verify?
>

Indeed, this double commas stuff is weird, I don't know that "trick". Is it
used elsewhere and documented? I don't see a test in test-qemu-opts.c
either.

There is another ongoing discussion about json support for -chardev:
"-chardev with a JSON argument".


> >
> > >
> > > Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
> > > Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > Cc: qemu-devel@nongnu.org
> > > ---
> > >  chardev/char-fe.c          |  14 +++--
> > >  chardev/char-mux.c         | 120 +++++++++++++++++++++++++++++--------
> > >  chardev/char.c             |   2 +-
> > >  chardev/chardev-internal.h |   7 ++-
> > >  4 files changed, 111 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> > > index b214ba3802b1..d1f67338084d 100644
> > > --- a/chardev/char-fe.c
> > > +++ b/chardev/char-fe.c
> > > @@ -197,16 +197,22 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev
> *s, Error **errp)
> > >          if (CHARDEV_IS_MUX(s)) {
> > >              MuxChardev *d = MUX_CHARDEV(s);
> > >
> > > -            if (d->mux_cnt >= MAX_MUX) {
> > > +            if (d->fe_cnt >= MAX_MUX) {
> > >                  error_setg(errp,
> > >                             "too many uses of multiplexed chardev '%s'"
> > >                             " (maximum is " stringify(MAX_MUX) ")",
> > >                             s->label);
> > >                  return false;
> > >              }
> > > -
> > > -            d->backends[d->mux_cnt] = b;
> > > -            tag = d->mux_cnt++;
> > > +            if (d->fe_cnt > 0 && d->be_cnt > 1) {
> > > +                error_setg(errp,
> > > +                           "multiplexed chardev '%s' is already used "
> > > +                           "for backend multiplexing",
> > > +                           s->label);
> > > +                return false;
> > > +            }
> > > +            d->backends[d->fe_cnt] = b;
> > > +            tag = d->fe_cnt++;
> > >          } else if (s->be) {
> > >              error_setg(errp, "chardev '%s' is already in use",
> s->label);
> > >              return false;
> > > diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> > > index ee2d47b20d9b..82f728b5caf8 100644
> > > --- a/chardev/char-mux.c
> > > +++ b/chardev/char-mux.c
> > > @@ -26,6 +26,7 @@
> > >  #include "qapi/error.h"
> > >  #include "qemu/module.h"
> > >  #include "qemu/option.h"
> > > +#include "qemu/cutils.h"
> > >  #include "chardev/char.h"
> > >  #include "sysemu/block-backend.h"
> > >  #include "qapi/qapi-commands-control.h"
> > > @@ -40,13 +41,39 @@
> > >   */
> > >  static bool muxes_opened = true;
> > >
> > > +/* Write to all backends  */
> > > +static int mux_chr_fe_write(MuxChardev *mux, const uint8_t *buf, int
> len)
> > > +{
> > > +        int r, ret = -1, i;
> > > +
> > > +        for (i = 0; i < mux->be_cnt; i++) {
> > > +                r = qemu_chr_fe_write(&mux->chrs[i], buf, len);
> > > +                ret = ret < 0 ? r : MAX(r, ret);
> >
> > I think it should be conservative and fail early if one of the backend
> > fails. Also if different frontends can write different amounts, there
> > needs to be some buffering... or it should always use write_all()
> > which has also a shortcoming since it blocks the thread.
>
> Yes, early fail and buffers make sense.
>
> >
> > > +        }
> > > +
> > > +        return ret;
> > > +}
> > > +
> > > +/* Write to all backends  */
> > > +static int mux_chr_fe_write_all(MuxChardev *mux, const uint8_t *buf,
> int len)
> > > +{
> > > +        int r, ret = -1, i;
> > > +
> > > +        for (i = 0; i < mux->be_cnt; i++) {
> > > +                r = qemu_chr_fe_write_all(&mux->chrs[i], buf, len);
> > > +                ret = ret < 0 ? r : MAX(r, ret);
> > > +        }
> > > +
> > > +        return ret;
> > > +}
> > > +
> > >  /* Called with chr_write_lock held.  */
> > >  static int mux_chr_write(Chardev *chr, const uint8_t *buf, int len)
> > >  {
> > >      MuxChardev *d = MUX_CHARDEV(chr);
> > >      int ret;
> > >      if (!d->timestamps) {
> > > -        ret = qemu_chr_fe_write(&d->chr, buf, len);
> > > +        ret = mux_chr_fe_write(d, buf, len);
> > >      } else {
> > >          int i;
> > >
> > > @@ -71,11 +98,11 @@ static int mux_chr_write(Chardev *chr, const
> uint8_t *buf, int len)
> > >                           (int)(ti % 1000));
> > >                  /* XXX this blocks entire thread. Rewrite to use
> > >                   * qemu_chr_fe_write and background I/O callbacks */
> > > -                qemu_chr_fe_write_all(&d->chr,
> > > +                mux_chr_fe_write_all(d,
> > >                                        (uint8_t *)buf1, strlen(buf1));
> > >                  d->linestart = 0;
> > >              }
> > > -            ret += qemu_chr_fe_write(&d->chr, buf + i, 1);
> > > +            ret += mux_chr_fe_write(d, buf + i, 1);
> > >              if (buf[i] == '\n') {
> > >                  d->linestart = 1;
> > >              }
> > > @@ -168,9 +195,9 @@ static int mux_proc_byte(Chardev *chr, MuxChardev
> *d, int ch)
> > >              qemu_chr_be_event(chr, CHR_EVENT_BREAK);
> > >              break;
> > >          case 'c':
> > > -            assert(d->mux_cnt > 0); /* handler registered with first
> fe */
> > > +            assert(d->fe_cnt > 0); /* handler registered with first
> fe */
> > >              /* Switch to the next registered device */
> > > -            mux_set_focus(chr, (d->focus + 1) % d->mux_cnt);
> > > +            mux_set_focus(chr, (d->focus + 1) % d->fe_cnt);
> > >              break;
> > >          case 't':
> > >              d->timestamps = !d->timestamps;
> > > @@ -248,8 +275,8 @@ void mux_chr_send_all_event(Chardev *chr,
> QEMUChrEvent event)
> > >          return;
> > >      }
> > >
> > > -    /* Send the event to all registered listeners */
> > > -    for (i = 0; i < d->mux_cnt; i++) {
> > > +    /* Send the event to all registered frontend listeners */
> > > +    for (i = 0; i < d->fe_cnt; i++) {
> > >          mux_chr_send_event(d, i, event);
> > >      }
> > >  }
> > > @@ -262,8 +289,16 @@ static void mux_chr_event(void *opaque,
> QEMUChrEvent event)
> > >  static GSource *mux_chr_add_watch(Chardev *s, GIOCondition cond)
> > >  {
> > >      MuxChardev *d = MUX_CHARDEV(s);
> > > -    Chardev *chr = qemu_chr_fe_get_driver(&d->chr);
> > > -    ChardevClass *cc = CHARDEV_GET_CLASS(chr);
> > > +    Chardev *chr;
> > > +    ChardevClass *cc;
> > > +
> > > +    if (d->be_cnt > 1) {
> > > +            /* TODO: multiple backends have to be combined on a
> single watch */
> >
> > I think this must be done, otherwise there is a severe limitation.
>
> Ok.
>
> >
> > > +            return NULL;
> > > +    }
> > > +
> > > +    chr = qemu_chr_fe_get_driver(&d->chrs[0]);
> > > +    cc = CHARDEV_GET_CLASS(chr);
> > >
> > >      if (!cc->chr_add_watch) {
> > >          return NULL;
> > > @@ -277,27 +312,32 @@ static void char_mux_finalize(Object *obj)
> > >      MuxChardev *d = MUX_CHARDEV(obj);
> > >      int i;
> > >
> > > -    for (i = 0; i < d->mux_cnt; i++) {
> > > +    for (i = 0; i < d->fe_cnt; i++) {
> > >          CharBackend *be = d->backends[i];
> > >          if (be) {
> > >              be->chr = NULL;
> > >          }
> > >      }
> > > -    qemu_chr_fe_deinit(&d->chr, false);
> > > +    for (i = 0; i < d->be_cnt; i++) {
> > > +        qemu_chr_fe_deinit(&d->chrs[i], false);
> > > +    }
> > >  }
> > >
> > >  static void mux_chr_update_read_handlers(Chardev *chr)
> > >  {
> > >      MuxChardev *d = MUX_CHARDEV(chr);
> > > +    int i;
> > >
> > > -    /* 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);
> > > +    for (i = 0; i < d->be_cnt; i++) {
> > > +        /* Fix up the real driver with mux routines */
> > > +        qemu_chr_fe_set_handlers_full(&d->chrs[i],
> > > +                                      mux_chr_can_read,
> > > +                                      mux_chr_read,
> > > +                                      mux_chr_event,
> > > +                                      NULL,
> > > +                                      chr,
> > > +                                      chr->gcontext, true, false);
> > > +    }
> > >  }
> > >
> > >  void mux_set_focus(Chardev *chr, int focus)
> > > @@ -305,7 +345,7 @@ void mux_set_focus(Chardev *chr, int focus)
> > >      MuxChardev *d = MUX_CHARDEV(chr);
> > >
> > >      assert(focus >= 0);
> > > -    assert(focus < d->mux_cnt);
> > > +    assert(focus < d->fe_cnt);
> > >
> > >      if (d->focus != -1) {
> > >          mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_OUT);
> > > @@ -324,19 +364,49 @@ static void qemu_chr_open_mux(Chardev *chr,
> > >      ChardevMux *mux = backend->u.mux.data;
> > >      Chardev *drv;
> > >      MuxChardev *d = MUX_CHARDEV(chr);
> > > -
> > > -    drv = qemu_chr_find(mux->chardev);
> > > -    if (drv == NULL) {
> > > -        error_setg(errp, "mux: base chardev %s not found",
> mux->chardev);
> > > +    const char *offset, *chardevs;
> > > +    int length, i;
> > > +
> > > +    if (d->fe_cnt > 1) {
> > > +        error_setg(errp,
> > > +                   "multiplexed chardev '%s' is already used "
> > > +                   "for frontend multiplexing",
> > > +                   chr->label);
> > >          return;
> > >      }
> > >
> > > +    chardevs = mux->chardev;
> > > +    for (i = 0; i < MAX_MUX; i++) {
> > > +        char *chardev;
> > > +
> > > +        offset = qemu_strchrnul(chardevs, ',');
> > > +        length = offset - chardevs;
> > > +        if (!length) {
> > > +            break;
> > > +        }
> > > +        chardev = strndupa(chardevs, length);
> > > +        chardevs += length + 1;
> > > +        drv = qemu_chr_find(chardev);
> > > +        if (drv == NULL) {
> > > +            error_setg(errp, "mux: base chardev %s not found",
> chardev);
> > > +            goto deinit_on_error;
> > > +        }
> > > +        qemu_chr_fe_init(&d->chrs[i], drv, errp);
> > > +        d->be_cnt += 1;
> > > +    }
> > >      d->focus = -1;
> > >      /* only default to opened state if we've realized the initial
> > >       * set of muxes
> > >       */
> > >      *be_opened = muxes_opened;
> > > -    qemu_chr_fe_init(&d->chr, drv, errp);
> > > +
> > > +    return;
> > > +
> > > +deinit_on_error:
> > > +    for (i = 0; i < d->be_cnt; i++) {
> > > +        qemu_chr_fe_deinit(&d->chrs[i], false);
> > > +    }
> > > +    d->be_cnt = 0;
> > >  }
> > >
> > >  static void qemu_chr_parse_mux(QemuOpts *opts, ChardevBackend
> *backend,
> > > diff --git a/chardev/char.c b/chardev/char.c
> > > index ba847b6e9eff..2643c79e5749 100644
> > > --- a/chardev/char.c
> > > +++ b/chardev/char.c
> > > @@ -333,7 +333,7 @@ static bool qemu_chr_is_busy(Chardev *s)
> > >  {
> > >      if (CHARDEV_IS_MUX(s)) {
> > >          MuxChardev *d = MUX_CHARDEV(s);
> > > -        return d->mux_cnt >= 0;
> > > +        return d->fe_cnt >= 0;
> > >      } else {
> > >          return s->be != NULL;
> > >      }
> > > diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
> > > index 4e03af31476c..72c2e4da7552 100644
> > > --- a/chardev/chardev-internal.h
> > > +++ b/chardev/chardev-internal.h
> > > @@ -35,10 +35,13 @@
> > >
> > >  struct MuxChardev {
> > >      Chardev parent;
> > > +    /* Linked frontends */
> > >      CharBackend *backends[MAX_MUX];
> > > -    CharBackend chr;
> > > +    /* Linked backends */
> > > +    CharBackend chrs[MAX_MUX];
> > >      int focus;
> > > -    int mux_cnt;
> > > +    int fe_cnt;
> > > +    int be_cnt;
> > >      int term_got_escape;
> > >      int max_size;
> > >      /* Intermediate input buffer catches escape sequences even if the
> > > --
> > > 2.34.1
> > >
> >
> >
> > It would also require some tests and QAPI support.
>
> I will take a look at tests and will try to come up with some extension
> for backend multiplexing.
>
> Since the mux object API was not changed and the current change heavily
> relies on what is already in the code, do you think there should be any
> QAPI related change? In my understanding this should work out of the box
> (not tested though).
>
> --
> Roman
>
>

-- 
Marc-André Lureau

Hi,

Thanks for the review, I will try to come up with the next version of
this series.

On Wed, Sep 18, 2024 at 12:52 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
[cut]
>
>
> Indeed, this double commas stuff is weird, I don't know that "trick". Is it used elsewhere and documented? I don't see a test in test-qemu-opts.c either.

In few places, for example here:
https://www.qemu.org/docs/master/system/qemu-manpage.html#options
(look for "double")

The second quote acts as an escape symbol to avoid splitting of the
command line.
Then in the modified mux I used a single comma as a separator for an array of
backends.

>
> There is another ongoing discussion about json support for -chardev: "-chardev with a JSON argument".

Thanks, found.

--
Roman

On Tue, 17 Sept 2024 at 13:32, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi Roman
>
> On Fri, Sep 13, 2024 at 8:37 PM Roman Penyaev <r.peniaev@gmail.com> wrote:
> >
> > This patch implements multiplexing capability of several backend
> > devices, which opens up an opportunity to use a single frontend
> > device on the guest, which can be manipulated from several
> > backend devices.
> >
> > The idea of the change is trivial: keep list of backend devices
> > (up to 4), init them on demand and forward data buffer back and
> > forth.
> >
> > The following is QEMU command line example:
> >
> >   -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
> >   -chardev vc,id=vc0 \
> >   -chardev mux,id=mux0,chardev=vc0,,sock0 \
> >   -device virtconsole,chardev=mux0 \
> >   -vnc 0.0.0.0:0
> >
> > Which creates 2 backend devices: text virtual console (`vc0`)
> > and a socket (`sock0`) connected to the single virtio hvc
> > console with the multiplexer (`mux0`) help. `vc0` renders
> > text to an image, which can be shared over the VNC protocol.
> > `sock0` is a socket backend which provides biderectional
> > communication to the virtio hvc console.
>
> I think I would rather implement a new mux for this purpose, like
> "mux-be" perhaps?
>
> "mux" has been a bit hidden (behind mux=on) for various reasons, and
> is probably not at production quality level.

You get a mux by default (for serial vs HMP monitor), so
I think it's pretty heavily used and tested in that sense...

-- PMM

On Tue, 17 Sept 2024 at 13:40, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 17 Sept 2024 at 13:32, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> >
> > Hi Roman
> >
> > On Fri, Sep 13, 2024 at 8:37 PM Roman Penyaev <r.peniaev@gmail.com> wrote:
> > >
> > > This patch implements multiplexing capability of several backend
> > > devices, which opens up an opportunity to use a single frontend
> > > device on the guest, which can be manipulated from several
> > > backend devices.
> > >
> > > The idea of the change is trivial: keep list of backend devices
> > > (up to 4), init them on demand and forward data buffer back and
> > > forth.
> > >
> > > The following is QEMU command line example:
> > >
> > >   -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
> > >   -chardev vc,id=vc0 \
> > >   -chardev mux,id=mux0,chardev=vc0,,sock0 \
> > >   -device virtconsole,chardev=mux0 \
> > >   -vnc 0.0.0.0:0
> > >
> > > Which creates 2 backend devices: text virtual console (`vc0`)
> > > and a socket (`sock0`) connected to the single virtio hvc
> > > console with the multiplexer (`mux0`) help. `vc0` renders
> > > text to an image, which can be shared over the VNC protocol.
> > > `sock0` is a socket backend which provides biderectional
> > > communication to the virtio hvc console.
> >
> > I think I would rather implement a new mux for this purpose, like
> > "mux-be" perhaps?
> >
> > "mux" has been a bit hidden (behind mux=on) for various reasons, and
> > is probably not at production quality level.
>
> You get a mux by default (for serial vs HMP monitor), so
> I think it's pretty heavily used and tested in that sense...

I should have said "by default for any -nographic invocation";
that's still a pretty common usage pattern, though.

-- PMM