Re: [PATCH v6 0/8] chardev: implement backend chardev multiplexing

Re: [PATCH v6 0/8] chardev: implement backend chardev multiplexing

[Marc-André Lureau]

Hi

On Mon, Dec 23, 2024 at 5:29 PM Roman Penyaev <r.peniaev@gmail.com> wrote:
>
> Mux is a character backend (host side) device, which multiplexes
> multiple frontends with one backend device. The following is a
> few lines from the QEMU manpage [1]:
>
>   A multiplexer is a "1:N" device, and here the "1" end is your
>   specified chardev backend, and the "N" end is the various parts
>   of QEMU that can talk to a chardev.
>
> But sadly multiple backends are not supported.
>
> This work 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 motivation is the EVE project [2], where it would be very
> convenient to have a virtio console frontend device on the guest that
> can be controlled from multiple backend devices, namely VNC and local
> TTY emulator. The following is an example of the QEMU command line:
>
>     -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
>     -chardev vc,id=vc0 \
>     -chardev mux-be,id=mux0,chardevs.0=sock0,chardevs.1=vc0 \
>     -device virtconsole,chardev=mux0 \
>     -vnc 0.0.0.0:0
>
> Which creates two backend devices:
>
> * Text virtual console (`vc0`)
> * A socket (`sock0`) connected to the single virtio hvc console with the
>   help of the backend multiplexer (`mux0`)
>
> `vc0` renders text to an image, which can be shared over the VNC protocol.
> `sock0` is a socket backend which provides bidirectional communication to
> the virtio hvc console.
>
> Once QEMU starts, the VNC client and any TTY emulator can be used to
> control a single hvc console. For example, these two different
> consoles should have similar input and output due to the buffer
> multiplexing:
>
>     # Start TTY emulator
>     socat unix-connect:/tmp/sock pty,link=/tmp/pty & \
>     tio /tmp/pty
>
>     # Start VNC client and switch to virtual console Ctrl-Alt-2
>     vncviewer :0
>
> v5 .. v6:
>
> * Rebased on latest master
> * Changed how chardev is attached to a multiplexer: with version 6
> mux should specify list elements with ID of chardevs:
>
>     chardevs.0=ID[,chardevs.N=ID]
>
> 'chardevs.N' list syntax is used for the sake of compatibility with
> the representation of JSON lists in 'key=val' pairs format of the
> util/keyval.c, despite the fact that modern QAPI way of parsing,
> namely qobject_input_visitor_new_str(), is not used. Choice of keeping
> QAPI list sytax may help to smoothly switch to modern parsing in the
> future.
>

That looks like a good compromise to me, thanks. Markus, wdyt?

In your patches, please change version 9.3 for 10.0 (next release).

btw, for some reason your series is not caught by patchew or
patchwork.. it's a bit annoying

> v4 .. v5:
>
> * Spelling fixes in qemu-options description
> * Memory leaks fixes in mux-be tests
> * Add sanity checks to chardev to avoid stacking of mux devices
> * Add corresponding unit test case to cover the creation of stacked
>   muxers: `-chardev mux-be,mux-id-be=ID`, which is forbidden
> * Reflect the fact that stacking is not supported in the documentation
>
> v3 .. v4:
>
> * Rebase on latest chardev changes
> * Add unit tests which test corner cases:
>    * Inability to remove mux with active frontend
>    * Inability to add more chardevs to a mux than `MUX_MAX`
>    * Inability to mix mux-fe and mux-be for the same chardev
>
> v2 .. v3:
>
> * Split frontend and backend multiplexer implementations and
>   move them to separate files: char-mux-fe.c and char-mux-be.c
>
> v1 .. v2:
>
> * Separate type for the backend multiplexer `mux-be`
> * Handle EAGAIN on write to the backend device
> * Support of watch of previously failed backend device
> * Proper json support of the `mux-be-id` option
> * Unit test for the `mux-be` multiplexer
>
> [1] https://www.qemu.org/docs/master/system/qemu-manpage.html#hxtool-6
> [2] https://github.com/lf-edge/eve
>
> Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: qemu-devel@nongnu.org
>
> Roman Penyaev (8):
>   chardev/char: rename `MuxChardev` struct to `MuxFeChardev`
>   chardev/char: rename `char-mux.c` to `char-mux-fe.c`
>   chardev/char: move away mux suspend/resume calls
>   chardev/char: rename frontend mux calls
>   chardev/char: rename `bid` to `mux_fe_id`
>   chardev/char-mux: implement backend chardev multiplexing
>   tests/unit/test-char: add unit test for the `mux-be` multiplexer
>   qemu-options.hx: describe multiplexing of several backend devices
>
>  chardev/char-fe.c                     |  25 +-
>  chardev/char-mux-be.c                 | 321 ++++++++++++++++++++
>  chardev/{char-mux.c => char-mux-fe.c} | 157 ++++------
>  chardev/char.c                        | 117 +++++++-
>  chardev/chardev-internal.h            |  59 +++-
>  chardev/meson.build                   |   3 +-
>  include/chardev/char.h                |   8 +-
>  qapi/char.json                        |  27 ++
>  qemu-options.hx                       |  48 ++-
>  system/vl.c                           |   4 +-
>  tests/unit/test-char.c                | 403 +++++++++++++++++++++++++-
>  11 files changed, 1023 insertions(+), 149 deletions(-)
>  create mode 100644 chardev/char-mux-be.c
>  rename chardev/{char-mux.c => char-mux-fe.c} (71%)
>
> --
> 2.34.1
>

Re: [PATCH v6 6/8] chardev/char-mux: implement backend chardev multiplexing

[Marc-André Lureau]

Hi

On Mon, Dec 23, 2024 at 5:24 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.
>
> Patch implements another multiplexer type `mux-be`. The following
> is QEMU command line example:
>
>    -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
>    -chardev vc,id=vc0 \
>    -chardev mux-be,id=mux0,chardevs.0=sock0,chardevs.1=vc0 \
>    -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
> backend 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.
>
> 'chardevs.N' list syntax is used for the sake of compatibility with
> the representation of JSON lists in 'key=val' pairs format of the
> util/keyval.c, despite the fact that modern QAPI way of parsing,
> namely qobject_input_visitor_new_str(), is not used. Choice of keeping
> QAPI list sytax may help to smoothly switch to modern parsing in the
> future.
>
> Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>  chardev/char-fe.c          |   9 ++
>  chardev/char-mux-be.c      | 321 +++++++++++++++++++++++++++++++++++++
>  chardev/char.c             |  30 +++-
>  chardev/chardev-internal.h |  38 ++++-
>  chardev/meson.build        |   1 +
>  include/chardev/char.h     |   1 +
>  qapi/char.json             |  27 ++++
>  7 files changed, 423 insertions(+), 4 deletions(-)
>  create mode 100644 chardev/char-mux-be.c
>
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index 8853f7fb4686..a156053ebef3 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -200,6 +200,12 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
>              if (!mux_fe_chr_attach_frontend(d, b, &tag, errp)) {
>                  return false;
>              }
> +        } else if (CHARDEV_IS_MUX_BE(s)) {
> +            MuxBeChardev *d = MUX_BE_CHARDEV(s);
> +
> +            if (!mux_be_chr_attach_frontend(d, b, errp)) {
> +                return false;
> +            }
>          } else if (s->be) {
>              error_setg(errp, "chardev '%s' is already in use", s->label);
>              return false;
> @@ -226,6 +232,9 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del)
>          if (CHARDEV_IS_MUX_FE(b->chr)) {
>              MuxFeChardev *d = MUX_FE_CHARDEV(b->chr);
>              mux_fe_chr_detach_frontend(d, b->tag);
> +        } else if (CHARDEV_IS_MUX_BE(b->chr)) {
> +            MuxBeChardev *d = MUX_BE_CHARDEV(b->chr);
> +            mux_be_chr_detach_frontend(d);
>          }
>          if (del) {
>              Object *obj = OBJECT(b->chr);
> diff --git a/chardev/char-mux-be.c b/chardev/char-mux-be.c
> new file mode 100644
> index 000000000000..f24c00dee2fe
> --- /dev/null
> +++ b/chardev/char-mux-be.c
> @@ -0,0 +1,321 @@
> +/*
> + * QEMU Character Backend Multiplexer
> + *
> + * Author: Roman Penyaev <r.peniaev@gmail.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/option.h"
> +#include "chardev/char.h"
> +#include "chardev-internal.h"
> +
> +/*
> + * MUX-BE driver for multiplexing 1 frontend device with N backend devices
> + */
> +
> +/*
> + * Write to all backends. Different backend devices accept data with
> + * various rate, so it is quite possible that one device returns less,
> + * then others. In this case we return minimum to the caller,
> + * expecting caller will repeat operation soon. When repeat happens
> + * send to the devices which consume data faster must be avoided
> + * for obvious reasons not to send data, which was already sent.
> + */
> +static int mux_be_chr_write_to_all(MuxBeChardev *d, const uint8_t *buf, int len)
> +{
> +    int r, i, ret = len;
> +    unsigned int written;
> +
> +    /* Invalidate index on every write */
> +    d->be_eagain_ind = -1;
> +
> +    for (i = 0; i < d->be_cnt; i++) {
> +        written = d->be_written[i] - d->be_min_written;
> +        if (written) {
> +            /* Written in the previous call so take into account */
> +            ret = MIN(written, ret);
> +            continue;
> +        }
> +        r = qemu_chr_fe_write(&d->backends[i], buf, len);
> +        if (r < 0 && errno == EAGAIN) {
> +            /*
> +             * Fail immediately if write would block. Expect to be called
> +             * soon on watch wake up.
> +             */
> +            d->be_eagain_ind = i;
> +            return r;

But next attempt to write will loop over the same backend again, which
will see the "same" write multiple times.

> +        } else if (r < 0) {
> +            /*
> +             * Ignore all other errors and pretend the entire buffer is
> +             * written to avoid this chardev being watched. This device
> +             * becomes disabled until the following write succeeds, but
> +             * writing continues to others.
> +             */
> +            r = len;
> +        }
> +        d->be_written[i] += r;
> +        ret = MIN(r, ret);
> +    }
> +    d->be_min_written += ret;
> +
> +    return ret;
> +}

I am not sure what is the correct way to handle write here. This
mux-be behaviour is different from mux-fe, since it handles all
backend I/Os, and does not select one... it's more of a "mixer",
right, Is this wanted?

> +
> +/* Called with chr_write_lock held.  */
> +static int mux_be_chr_write(Chardev *chr, const uint8_t *buf, int len)
> +{
> +    MuxBeChardev *d = MUX_BE_CHARDEV(chr);
> +    return mux_be_chr_write_to_all(d, buf, len);
> +}
> +
> +static void mux_be_chr_send_event(MuxBeChardev *d, QEMUChrEvent event)
> +{
> +    CharBackend *fe = d->frontend;
> +
> +    if (fe && fe->chr_event) {
> +        fe->chr_event(fe->opaque, event);
> +    }
> +}
> +
> +static void mux_be_chr_be_event(Chardev *chr, QEMUChrEvent event)
> +{
> +    MuxBeChardev *d = MUX_BE_CHARDEV(chr);
> +
> +    mux_be_chr_send_event(d, event);
> +}
> +
> +static int mux_be_chr_can_read(void *opaque)
> +{
> +    MuxBeChardev *d = MUX_BE_CHARDEV(opaque);
> +    CharBackend *fe = d->frontend;
> +
> +    if (fe && fe->chr_can_read) {
> +        return fe->chr_can_read(fe->opaque);
> +    }
> +
> +    return 0;
> +}
> +
> +static void mux_be_chr_read(void *opaque, const uint8_t *buf, int size)
> +{
> +    MuxBeChardev *d = MUX_BE_CHARDEV(opaque);
> +    CharBackend *fe = d->frontend;
> +
> +    if (fe && fe->chr_read) {
> +        fe->chr_read(fe->opaque, buf, size);
> +    }
> +}
> +
> +void mux_be_chr_send_all_event(MuxBeChardev *d, QEMUChrEvent event)
> +{
> +    mux_be_chr_send_event(d, event);
> +}
> +
> +static void mux_be_chr_event(void *opaque, QEMUChrEvent event)
> +{
> +    mux_chr_send_all_event(CHARDEV(opaque), event);
> +}
> +
> +static GSource *mux_be_chr_add_watch(Chardev *s, GIOCondition cond)
> +{
> +    MuxBeChardev *d = MUX_BE_CHARDEV(s);
> +    Chardev *chr;
> +    ChardevClass *cc;
> +
> +    if (d->be_eagain_ind == -1)
> +        return NULL;
> +
> +    assert(d->be_eagain_ind < d->be_cnt);
> +    chr = qemu_chr_fe_get_driver(&d->backends[d->be_eagain_ind]);
> +    cc = CHARDEV_GET_CLASS(chr);
> +    if (!cc->chr_add_watch) {
> +        return NULL;
> +    }
> +
> +    return cc->chr_add_watch(chr, cond);
> +}
> +
> +static bool mux_be_chr_attach_chardev(MuxBeChardev *d, Chardev *chr,
> +                                      Error **errp)
> +{
> +    bool ret;
> +
> +    if (d->be_cnt >= MAX_MUX) {
> +        error_setg(errp, "mux-be: too many uses of multiplexed chardev '%s'"
> +                   " (maximum is " stringify(MAX_MUX) ")",
> +                   d->parent.label);
> +        return false;
> +    }
> +    ret = qemu_chr_fe_init(&d->backends[d->be_cnt], chr, errp);
> +    if (ret) {
> +        /* Catch up with what was already written */
> +        d->be_written[d->be_cnt] = d->be_min_written;
> +        d->be_cnt += 1;
> +    }
> +
> +    return ret;
> +}
> +
> +static void char_mux_be_finalize(Object *obj)
> +{
> +    MuxBeChardev *d = MUX_BE_CHARDEV(obj);
> +    CharBackend *fe = d->frontend;
> +    int i;
> +
> +    if (fe) {
> +        fe->chr = NULL;
> +    }
> +    for (i = 0; i < d->be_cnt; i++) {
> +        qemu_chr_fe_deinit(&d->backends[i], false);
> +    }
> +}
> +
> +static void mux_be_chr_update_read_handlers(Chardev *chr)
> +{
> +    MuxBeChardev *d = MUX_BE_CHARDEV(chr);
> +    int i;
> +
> +    for (i = 0; i < d->be_cnt; i++) {
> +        /* Fix up the real driver with mux routines */
> +        qemu_chr_fe_set_handlers_full(&d->backends[i],
> +                                      mux_be_chr_can_read,
> +                                      mux_be_chr_read,
> +                                      mux_be_chr_event,
> +                                      NULL,
> +                                      chr,
> +                                      chr->gcontext, true, false);
> +    }
> +}
> +
> +bool mux_be_chr_attach_frontend(MuxBeChardev *d, CharBackend *b, Error **errp)
> +{
> +    if (d->frontend) {
> +        error_setg(errp, "mux-be: multiplexed chardev '%s' is already used "
> +                   "for multiplexing", d->parent.label);
> +        return false;
> +    }
> +    d->frontend = b;
> +
> +    return true;
> +}
> +
> +void mux_be_chr_detach_frontend(MuxBeChardev *d)
> +{
> +    d->frontend = NULL;
> +}
> +
> +static void qemu_chr_open_mux_be(Chardev *chr,
> +                                 ChardevBackend *backend,
> +                                 bool *be_opened,
> +                                 Error **errp)
> +{
> +    ChardevMuxBe *mux = backend->u.mux_be.data;
> +    MuxBeChardev *d = MUX_BE_CHARDEV(chr);
> +    strList *list = mux->chardevs;
> +
> +    d->be_eagain_ind = -1;
> +
> +    if (list == NULL) {
> +        error_setg(errp, "mux-be: 'chardevs' list is not defined");
> +        return;
> +    }
> +
> +    while (list) {
> +        Chardev *s;
> +
> +        s = qemu_chr_find(list->value);
> +        if (s == NULL) {
> +            error_setg(errp, "mux-be: chardev can't be found by id '%s'",
> +                       list->value);
> +            return;
> +        }
> +        if (CHARDEV_IS_MUX_BE(s) || CHARDEV_IS_MUX_FE(s)) {
> +            error_setg(errp, "mux-be: multiplexers can't be stacked, check "
> +                       "chardev '%s', chardev should not be a multiplexer or "
> +                       "have 'mux=on' enabled", list->value);
> +            return;
> +        }
> +        if (!mux_be_chr_attach_chardev(d, s, errp)) {
> +            return;
> +        }
> +        list = list->next;
> +    }
> +
> +    /*
> +     * Only default to opened state if we've realized the initial
> +     * set of muxes
> +     */
> +    *be_opened = mux_is_opened();
> +}
> +
> +static void qemu_chr_parse_mux_be(QemuOpts *opts, ChardevBackend *backend,
> +                                  Error **errp)
> +{
> +    ChardevMuxBe *mux;
> +    strList **tail;
> +    int i;
> +
> +    backend->type = CHARDEV_BACKEND_KIND_MUX_BE;
> +    mux = backend->u.mux_be.data = g_new0(ChardevMuxBe, 1);
> +    qemu_chr_parse_common(opts, qapi_ChardevMuxBe_base(mux));
> +
> +    tail = &mux->chardevs;
> +
> +    for (i = 0; i < MAX_MUX; i++) {
> +        char optbuf[16];
> +        const char *dev;
> +
> +        snprintf(optbuf, sizeof(optbuf), "chardevs.%u", i);
> +        dev = qemu_opt_get(opts, optbuf);
> +        if (!dev)
> +            break;
> +
> +        QAPI_LIST_APPEND(tail, g_strdup(dev));
> +    }
> +}
> +
> +static void char_mux_be_class_init(ObjectClass *oc, void *data)
> +{
> +    ChardevClass *cc = CHARDEV_CLASS(oc);
> +
> +    cc->parse = qemu_chr_parse_mux_be;
> +    cc->open = qemu_chr_open_mux_be;
> +    cc->chr_write = mux_be_chr_write;
> +    cc->chr_add_watch = mux_be_chr_add_watch;
> +    cc->chr_be_event = mux_be_chr_be_event;
> +    cc->chr_update_read_handler = mux_be_chr_update_read_handlers;
> +}
> +
> +static const TypeInfo char_mux_be_type_info = {
> +    .name = TYPE_CHARDEV_MUX_BE,
> +    .parent = TYPE_CHARDEV,
> +    .class_init = char_mux_be_class_init,
> +    .instance_size = sizeof(MuxBeChardev),
> +    .instance_finalize = char_mux_be_finalize,
> +};
> +
> +static void register_types(void)
> +{
> +    type_register_static(&char_mux_be_type_info);
> +}
> +
> +type_init(register_types);
> diff --git a/chardev/char.c b/chardev/char.c
> index 08e7f23bddb2..3e85cfc5a4ba 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -341,6 +341,9 @@ static bool qemu_chr_is_busy(Chardev *s)
>      if (CHARDEV_IS_MUX_FE(s)) {
>          MuxFeChardev *d = MUX_FE_CHARDEV(s);
>          return d->mux_bitset != 0;
> +    } else if (CHARDEV_IS_MUX_BE(s)) {
> +        MuxBeChardev *d = MUX_BE_CHARDEV(s);
> +        return d->frontend != NULL;
>      } else {
>          return s->be != NULL;
>      }
> @@ -949,7 +952,26 @@ QemuOptsList qemu_chardev_opts = {
>          },{
>              .name = "chardev",
>              .type = QEMU_OPT_STRING,
> +        },
> +        /*
> +         * Multiplexer options. Follows QAPI array syntax.
> +         * See MAX_MUX macro to obtain array capacity.
> +         */
> +        {
> +            .name = "chardevs.0",
> +            .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "chardevs.1",
> +            .type = QEMU_OPT_STRING,
>          },{
> +            .name = "chardevs.2",
> +            .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "chardevs.3",
> +            .type = QEMU_OPT_STRING,
> +        },
> +
> +        {
>              .name = "append",
>              .type = QEMU_OPT_BOOL,
>          },{
> @@ -1112,7 +1134,7 @@ ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
>          return NULL;
>      }
>
> -    if (CHARDEV_IS_MUX_FE(chr)) {
> +    if (CHARDEV_IS_MUX_FE(chr) || CHARDEV_IS_MUX_BE(chr)) {
>          error_setg(errp, "Mux device hotswap not supported yet");
>          return NULL;
>      }
> @@ -1300,7 +1322,7 @@ static int chardev_options_parsed_cb(Object *child, void *opaque)
>  {
>      Chardev *chr = (Chardev *)child;
>
> -    if (!chr->be_open && CHARDEV_IS_MUX_FE(chr)) {
> +    if (!chr->be_open && (CHARDEV_IS_MUX_FE(chr) || CHARDEV_IS_MUX_BE(chr))) {
>          open_muxes(chr);
>      }
>
> @@ -1327,8 +1349,10 @@ void mux_chr_send_all_event(Chardev *chr, QEMUChrEvent event)
>
>      if (CHARDEV_IS_MUX_FE(chr)) {
>          MuxFeChardev *d = MUX_FE_CHARDEV(chr);
> -
>          mux_fe_chr_send_all_event(d, event);
> +    } else if (CHARDEV_IS_MUX_BE(chr)) {
> +        MuxBeChardev *d = MUX_BE_CHARDEV(chr);
> +        mux_be_chr_send_all_event(d, event);
>      }
>  }
>
> diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
> index 94c8d07ac235..8c98a3fb0767 100644
> --- a/chardev/chardev-internal.h
> +++ b/chardev/chardev-internal.h
> @@ -35,7 +35,9 @@
>
>  struct MuxFeChardev {
>      Chardev parent;
> +    /* Linked frontends */
>      CharBackend *backends[MAX_MUX];
> +    /* Linked backend */
>      CharBackend chr;
>      unsigned long mux_bitset;
>      int focus;
> @@ -54,10 +56,41 @@ struct MuxFeChardev {
>  };
>  typedef struct MuxFeChardev MuxFeChardev;
>
> +struct MuxBeChardev {
> +    Chardev parent;
> +    /* Linked frontend */
> +    CharBackend *frontend;
> +    /* Linked backends */
> +    CharBackend backends[MAX_MUX];
> +    /*
> +     * Number of backends attached to this mux. Once attached, a
> +     * backend can't be detached, so the counter is only increasing.
> +     * To safely remove a backend, mux has to be removed first.
> +     */
> +    unsigned int be_cnt;
> +    /*
> +     * Counters of written bytes from a single frontend device
> +     * to multiple backend devices.
> +     */
> +    unsigned int be_written[MAX_MUX];
> +    unsigned int be_min_written;
> +    /*
> +     * Index of a backend device which got EAGAIN on last write,
> +     * -1 is invalid index.
> +     */
> +    int be_eagain_ind;
> +};
> +typedef struct MuxBeChardev MuxBeChardev;
> +
>  DECLARE_INSTANCE_CHECKER(MuxFeChardev, MUX_FE_CHARDEV,
>                           TYPE_CHARDEV_MUX_FE)
> -#define CHARDEV_IS_MUX_FE(chr)                             \
> +DECLARE_INSTANCE_CHECKER(MuxBeChardev, MUX_BE_CHARDEV,
> +                         TYPE_CHARDEV_MUX_BE)
> +
> +#define CHARDEV_IS_MUX_FE(chr)                              \
>      object_dynamic_cast(OBJECT(chr), TYPE_CHARDEV_MUX_FE)
> +#define CHARDEV_IS_MUX_BE(chr)                              \
> +    object_dynamic_cast(OBJECT(chr), TYPE_CHARDEV_MUX_BE)
>
>  void mux_chr_send_all_event(Chardev *chr, QEMUChrEvent event);
>
> @@ -67,6 +100,9 @@ void mux_fe_chr_send_all_event(MuxFeChardev *d, QEMUChrEvent event);
>  bool mux_fe_chr_attach_frontend(MuxFeChardev *d, CharBackend *b,
>                                  unsigned int *tag, Error **errp);
>  bool mux_fe_chr_detach_frontend(MuxFeChardev *d, unsigned int tag);
> +void mux_be_chr_send_all_event(MuxBeChardev *d, QEMUChrEvent event);
> +bool mux_be_chr_attach_frontend(MuxBeChardev *d, CharBackend *b, Error **errp);
> +void mux_be_chr_detach_frontend(MuxBeChardev *d);
>
>  Object *get_chardevs_root(void);
>
> diff --git a/chardev/meson.build b/chardev/meson.build
> index 778444a00ca6..3a9f5565372b 100644
> --- a/chardev/meson.build
> +++ b/chardev/meson.build
> @@ -3,6 +3,7 @@ chardev_ss.add(files(
>    'char-file.c',
>    'char-io.c',
>    'char-mux-fe.c',
> +  'char-mux-be.c',
>    'char-null.c',
>    'char-pipe.c',
>    'char-ringbuf.c',
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 0bec974f9d73..c58c11c4eeaf 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -232,6 +232,7 @@ OBJECT_DECLARE_TYPE(Chardev, ChardevClass, CHARDEV)
>
>  #define TYPE_CHARDEV_NULL "chardev-null"
>  #define TYPE_CHARDEV_MUX_FE "chardev-mux"
> +#define TYPE_CHARDEV_MUX_BE "chardev-mux-be"
>  #define TYPE_CHARDEV_RINGBUF "chardev-ringbuf"
>  #define TYPE_CHARDEV_PTY "chardev-pty"
>  #define TYPE_CHARDEV_CONSOLE "chardev-console"
> diff --git a/qapi/char.json b/qapi/char.json
> index e04535435034..2ad77a7f6435 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -332,6 +332,19 @@
>    'data': { 'chardev': 'str' },
>    'base': 'ChardevCommon' }
>
> +##
> +# @ChardevMuxBe:
> +#
> +# Configuration info for mux-be chardevs.
> +#
> +# @chardevs: List of chardev IDs, which should be added to this mux
> +#
> +# Since: 9.3
> +##
> +{ 'struct': 'ChardevMuxBe',
> +  'data': { 'chardevs': ['str'] },
> +  'base': 'ChardevCommon' }
> +
>  ##
>  # @ChardevStdio:
>  #
> @@ -479,6 +492,8 @@
>  #
>  # @mux: (since 1.5)
>  #
> +# @mux-be: (since 9.3)
> +#
>  # @msmouse: emulated Microsoft serial mouse (since 1.5)
>  #
>  # @wctablet: emulated Wacom Penpartner serial tablet (since 2.9)
> @@ -521,6 +536,7 @@
>              'pty',
>              'null',
>              'mux',
> +            'mux-be',
>              'msmouse',
>              'wctablet',
>              { 'name': 'braille', 'if': 'CONFIG_BRLAPI' },
> @@ -595,6 +611,16 @@
>  { 'struct': 'ChardevMuxWrapper',
>    'data': { 'data': 'ChardevMux' } }
>
> +##
> +# @ChardevMuxBeWrapper:
> +#
> +# @data: Configuration info for mux-be chardevs
> +#
> +# Since: 9.3
> +##
> +{ 'struct': 'ChardevMuxBeWrapper',
> +  'data': { 'data': 'ChardevMuxBe' } }
> +
>  ##
>  # @ChardevStdioWrapper:
>  #
> @@ -703,6 +729,7 @@
>              'pty': 'ChardevPtyWrapper',
>              'null': 'ChardevCommonWrapper',
>              'mux': 'ChardevMuxWrapper',
> +            'mux-be': 'ChardevMuxBeWrapper',
>              'msmouse': 'ChardevCommonWrapper',
>              'wctablet': 'ChardevCommonWrapper',
>              'braille': { 'type': 'ChardevCommonWrapper',
> --
> 2.34.1
>

Re: [PATCH v6 6/8] chardev/char-mux: implement backend chardev multiplexing

[Roman Penyaev]

Hi,

First of all Happy New Year :)

On Mon, Dec 30, 2024 at 12:41 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:

[cut]

> > +
> > +    for (i = 0; i < d->be_cnt; i++) {
> > +        written = d->be_written[i] - d->be_min_written;
> > +        if (written) {
> > +            /* Written in the previous call so take into account */
> > +            ret = MIN(written, ret);
> > +            continue;
> > +        }
> > +        r = qemu_chr_fe_write(&d->backends[i], buf, len);
> > +        if (r < 0 && errno == EAGAIN) {
> > +            /*
> > +             * Fail immediately if write would block. Expect to be called
> > +             * soon on watch wake up.
> > +             */
> > +            d->be_eagain_ind = i;
> > +            return r;
>
> But next attempt to write will loop over the same backend again, which
> will see the "same" write multiple times.

This case is handled by checking the difference between counters
`d->be_written[i]` and `d->be_min_written`. The idea is that device, which
already "swallowed" some portion of data, will be skipped from writing to it,
until it catches up with the stream.

Please take a look into the `char_mux_be_test()` test case, where the
EAGAIN scenario is tested. The line test-char.c:716 explicitly shows the
repeat of the write procedure after EAGAIN was received.

>
> > +        } else if (r < 0) {
> > +            /*
> > +             * Ignore all other errors and pretend the entire buffer is
> > +             * written to avoid this chardev being watched. This device
> > +             * becomes disabled until the following write succeeds, but
> > +             * writing continues to others.
> > +             */
> > +            r = len;
> > +        }
> > +        d->be_written[i] += r;
> > +        ret = MIN(r, ret);
> > +    }
> > +    d->be_min_written += ret;
> > +
> > +    return ret;
> > +}
>
> I am not sure what is the correct way to handle write here. This
> mux-be behaviour is different from mux-fe, since it handles all
> backend I/Os, and does not select one... it's more of a "mixer",
> right, Is this wanted?

Right. The intention is to have both consoles simultaneously
working, for example having char-based tio (over a socket chardev)
and image-based vnc (over a vc chardev):

    -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
    -chardev vc,id=vc0 \

and both are connected to the same frontend device.

I agree with you on the "mixer" naming concern, this did not come to
my mind. As far as I understand the logic of `mux-fe`, it just doesn't seem
possible to have both frontends running at the same time, because they
will both generate output, at least that's the case for virtual consoles:
imagine you have two virtual console frontends working at the same time
and one backend. Any command you enter from a backend causes the two
separate frontends to output completely different data.

On the other hand, several backend devices can easily be simultaneously
attached to one frontend, the analogy is simple: several monitors, several
keyboards, etc work perfectly fine with a single PC. At least this is how
I see this, please correct me if I'm wrong.

Do you think we need to artificially introduce multiplexing logic to be fully
compliant with multiplexer naming? It's not hard to do, repeating
`mux_proc_byte()` from `mux-fe`. In my use-case, I'll still need to disable
multiplexing in favor of 'mixing', for example with the 'mixer=on' option,
i.e. '-chardev mux-be,mixer=on,...`. Or do you think it should be some
completely different beast, something like mixer chardev?

--
Roman

Re: [PATCH v6 0/8] chardev: implement backend chardev multiplexing

[Roman Penyaev]

On Mon, Dec 30, 2024 at 12:35 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> That looks like a good compromise to me, thanks. Markus, wdyt?
>
> In your patches, please change version 9.3 for 10.0 (next release).

Ok.

> btw, for some reason your series is not caught by patchew or
> patchwork.. it's a bit annoying

That's odd. v6 has disappeared. Will re-send once we discuss the mixer
matters in another thread.

--
Roman

Re: [PATCH v6 6/8] chardev/char-mux: implement backend chardev multiplexing

[Marc-André Lureau]

Hi

On Thu, Jan 2, 2025 at 2:22 PM Roman Penyaev <r.peniaev@gmail.com> wrote:
>
> Hi,
>
> First of all Happy New Year :)
>
> On Mon, Dec 30, 2024 at 12:41 PM Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>
> [cut]
>
> > > +
> > > +    for (i = 0; i < d->be_cnt; i++) {
> > > +        written = d->be_written[i] - d->be_min_written;
> > > +        if (written) {
> > > +            /* Written in the previous call so take into account */
> > > +            ret = MIN(written, ret);
> > > +            continue;
> > > +        }
> > > +        r = qemu_chr_fe_write(&d->backends[i], buf, len);
> > > +        if (r < 0 && errno == EAGAIN) {
> > > +            /*
> > > +             * Fail immediately if write would block. Expect to be called
> > > +             * soon on watch wake up.
> > > +             */
> > > +            d->be_eagain_ind = i;
> > > +            return r;
> >
> > But next attempt to write will loop over the same backend again, which
> > will see the "same" write multiple times.
>
> This case is handled by checking the difference between counters
> `d->be_written[i]` and `d->be_min_written`. The idea is that device, which
> already "swallowed" some portion of data, will be skipped from writing to it,
> until it catches up with the stream.

ok, I see. This looks fragile though, I/one will need to do a more
thorough review.

>
> Please take a look into the `char_mux_be_test()` test case, where the
> EAGAIN scenario is tested. The line test-char.c:716 explicitly shows the
> repeat of the write procedure after EAGAIN was received.
>
> >
> > > +        } else if (r < 0) {
> > > +            /*
> > > +             * Ignore all other errors and pretend the entire buffer is
> > > +             * written to avoid this chardev being watched. This device
> > > +             * becomes disabled until the following write succeeds, but
> > > +             * writing continues to others.
> > > +             */
> > > +            r = len;
> > > +        }
> > > +        d->be_written[i] += r;
> > > +        ret = MIN(r, ret);
> > > +    }
> > > +    d->be_min_written += ret;
> > > +
> > > +    return ret;
> > > +}
> >
> > I am not sure what is the correct way to handle write here. This
> > mux-be behaviour is different from mux-fe, since it handles all
> > backend I/Os, and does not select one... it's more of a "mixer",
> > right, Is this wanted?
>
> Right. The intention is to have both consoles simultaneously
> working, for example having char-based tio (over a socket chardev)
> and image-based vnc (over a vc chardev):
>
>     -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
>     -chardev vc,id=vc0 \
>
> and both are connected to the same frontend device.
>
> I agree with you on the "mixer" naming concern, this did not come to
> my mind. As far as I understand the logic of `mux-fe`, it just doesn't seem
> possible to have both frontends running at the same time, because they
> will both generate output, at least that's the case for virtual consoles:
> imagine you have two virtual console frontends working at the same time
> and one backend. Any command you enter from a backend causes the two
> separate frontends to output completely different data.
>
> On the other hand, several backend devices can easily be simultaneously
> attached to one frontend, the analogy is simple: several monitors, several
> keyboards, etc work perfectly fine with a single PC. At least this is how
> I see this, please correct me if I'm wrong.

Whether we talk about multiplexing front-end or back-end, the issues
are similar. In general, mixing input will create issues. Teeing
output is less problematic, except to handle the buffering...

>
> Do you think we need to artificially introduce multiplexing logic to be fully
> compliant with multiplexer naming? It's not hard to do, repeating
> `mux_proc_byte()` from `mux-fe`. In my use-case, I'll still need to disable
> multiplexing in favor of 'mixing', for example with the 'mixer=on' option,
> i.e. '-chardev mux-be,mixer=on,...`. Or do you think it should be some
> completely different beast, something like mixer chardev?

I think it would be saner to have the muxer be selectors: only work
with one selected be or fe. Otherwise, we can run into various issues.

I hope some qemu maintainers can comment too.

Re: [PATCH v6 6/8] chardev/char-mux: implement backend chardev multiplexing

[Roman Penyaev]

Hi,

On Tue, Jan 7, 2025 at 3:57 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi

[cut]

> > > But next attempt to write will loop over the same backend again, which
> > > will see the "same" write multiple times.
> >
> > This case is handled by checking the difference between counters
> > `d->be_written[i]` and `d->be_min_written`. The idea is that device, which
> > already "swallowed" some portion of data, will be skipped from writing to it,
> > until it catches up with the stream.
>
> ok, I see. This looks fragile though, I/one will need to do a more
> thorough review.

Yes, please, a thorough review would be great. I'm really keen to improve
what needs to be improved and hopefully finish this.

Do you want me to resend this as v7 to make patchew to grab this series
for further review?

>
> >
> > Please take a look into the `char_mux_be_test()` test case, where the
> > EAGAIN scenario is tested. The line test-char.c:716 explicitly shows the
> > repeat of the write procedure after EAGAIN was received.
> >
> > >
> > > > +        } else if (r < 0) {
> > > > +            /*
> > > > +             * Ignore all other errors and pretend the entire buffer is
> > > > +             * written to avoid this chardev being watched. This device
> > > > +             * becomes disabled until the following write succeeds, but
> > > > +             * writing continues to others.
> > > > +             */
> > > > +            r = len;
> > > > +        }
> > > > +        d->be_written[i] += r;
> > > > +        ret = MIN(r, ret);
> > > > +    }
> > > > +    d->be_min_written += ret;
> > > > +
> > > > +    return ret;
> > > > +}
> > >
> > > I am not sure what is the correct way to handle write here. This
> > > mux-be behaviour is different from mux-fe, since it handles all
> > > backend I/Os, and does not select one... it's more of a "mixer",
> > > right, Is this wanted?
> >
> > Right. The intention is to have both consoles simultaneously
> > working, for example having char-based tio (over a socket chardev)
> > and image-based vnc (over a vc chardev):
> >
> >     -chardev socket,path=/tmp/sock,server=on,wait=off,id=sock0 \
> >     -chardev vc,id=vc0 \
> >
> > and both are connected to the same frontend device.
> >
> > I agree with you on the "mixer" naming concern, this did not come to
> > my mind. As far as I understand the logic of `mux-fe`, it just doesn't seem
> > possible to have both frontends running at the same time, because they
> > will both generate output, at least that's the case for virtual consoles:
> > imagine you have two virtual console frontends working at the same time
> > and one backend. Any command you enter from a backend causes the two
> > separate frontends to output completely different data.
> >
> > On the other hand, several backend devices can easily be simultaneously
> > attached to one frontend, the analogy is simple: several monitors, several
> > keyboards, etc work perfectly fine with a single PC. At least this is how
> > I see this, please correct me if I'm wrong.
>
> Whether we talk about multiplexing front-end or back-end, the issues
> are similar. In general, mixing input will create issues. Teeing
> output is less problematic, except to handle the buffering...

I understand your concerns. What exact issues do you have in mind?
Are these issues related to the input buffer handling, so technical issues?
Or issues with usability?

>
> >
> > Do you think we need to artificially introduce multiplexing logic to be fully
> > compliant with multiplexer naming? It's not hard to do, repeating
> > `mux_proc_byte()` from `mux-fe`. In my use-case, I'll still need to disable
> > multiplexing in favor of 'mixing', for example with the 'mixer=on' option,
> > i.e. '-chardev mux-be,mixer=on,...`. Or do you think it should be some
> > completely different beast, something like mixer chardev?
>
> I think it would be saner to have the muxer be selectors: only work
> with one selected be or fe. Otherwise, we can run into various issues.

In multiplexing (not mixing) for the use-case that I am describing, there is one
serious drawback: as soon as you switch the "focus" to another input device
(for example from vnc to socket chardev), you will not be able to switch back
from the same input console - the input now works on another device. This looks
strange and does not add convenience to the final user. Perhaps, for a case
other than console, this would be reasonable, but for console input -
I would like
to keep the mixer option: the front-end receives input from both back-ends.

> I hope some qemu maintainers can comment too.

Do I need to add someone else in CC to gain more feedback?

--
Roman

Re: [PATCH v6 6/8] chardev/char-mux: implement backend chardev multiplexing

[Daniel P. Berrangé]

On Thu, Jan 09, 2025 at 01:56:40PM +0100, Roman Penyaev wrote:
> Hi,
> 
> On Tue, Jan 7, 2025 at 3:57 PM Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> > Whether we talk about multiplexing front-end or back-end, the issues
> > are similar. In general, mixing input will create issues. Teeing
> > output is less problematic, except to handle the buffering...
> 
> I understand your concerns. What exact issues do you have in mind?
> Are these issues related to the input buffer handling, so technical issues?
> Or issues with usability?

While the design / impl technically allows for concurrent input to be
sent to the frontend, from multiple backends, in practice I don't think
we need to be particularly concerned about it.

I don't see this as being a way for multiple different users to interact
concurrently. Rather I'd see 1 user of the VM just deciding to switch
from one backend to the other on the fly. IOW, although technically
possible, the user will only be leveraging one at a time to send input.

We very definitely do need all backends to receive output from the guest
concurrently too, as you'd want the historical output context to be
visible on whatever backend you choose to use at any given point in time.

If a user decides to be crazy and send input from multiple backends
concurrently, then they get to keep the mess.

> > > Do you think we need to artificially introduce multiplexing logic to be fully
> > > compliant with multiplexer naming? It's not hard to do, repeating
> > > `mux_proc_byte()` from `mux-fe`. In my use-case, I'll still need to disable
> > > multiplexing in favor of 'mixing', for example with the 'mixer=on' option,
> > > i.e. '-chardev mux-be,mixer=on,...`. Or do you think it should be some
> > > completely different beast, something like mixer chardev?
> >
> > I think it would be saner to have the muxer be selectors: only work
> > with one selected be or fe. Otherwise, we can run into various issues.
> 
> In multiplexing (not mixing) for the use-case that I am describing, there is one
> serious drawback: as soon as you switch the "focus" to another input device
> (for example from vnc to socket chardev), you will not be able to s]witch back
> from the same input console - the input now works on another device. This looks
> strange and does not add convenience to the final user. Perhaps, for a case
> other than console, this would be reasonable, but for console input -
> I would like
> to keep the mixer option: the front-end receives input from both back-ends.

Agreed, I think this is desirable. If you did the exclusive access mode,
it'd complicate things as you now need a way to switch between active
backends, while also reducing the usefulness of it.

The main thing I'm not a fan of here is the naming 'mux-fe', as I think we
should have something distinct from current 'mux', to reduce confusion
when we're talking about it.

How about 'overlay' or 'replicator' ? 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v6 6/8] chardev/char-mux: implement backend chardev multiplexing

[Roman Penyaev]

On Thu, Jan 9, 2025 at 7:07 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Jan 09, 2025 at 01:56:40PM +0100, Roman Penyaev wrote:
> > Hi,
> >
> > On Tue, Jan 7, 2025 at 3:57 PM Marc-André Lureau
> > <marcandre.lureau@redhat.com> wrote:
> > > Whether we talk about multiplexing front-end or back-end, the issues
> > > are similar. In general, mixing input will create issues. Teeing
> > > output is less problematic, except to handle the buffering...
> >
> > I understand your concerns. What exact issues do you have in mind?
> > Are these issues related to the input buffer handling, so technical issues?
> > Or issues with usability?
>
> While the design / impl technically allows for concurrent input to be
> sent to the frontend, from multiple backends, in practice I don't think
> we need to be particularly concerned about it.
>
> I don't see this as being a way for multiple different users to interact
> concurrently. Rather I'd see 1 user of the VM just deciding to switch
> from one backend to the other on the fly. IOW, although technically
> possible, the user will only be leveraging one at a time to send input.
>
> We very definitely do need all backends to receive output from the guest
> concurrently too, as you'd want the historical output context to be
> visible on whatever backend you choose to use at any given point in time.
>
> If a user decides to be crazy and send input from multiple backends
> concurrently, then they get to keep the mess.
>
> > > > Do you think we need to artificially introduce multiplexing logic to be fully
> > > > compliant with multiplexer naming? It's not hard to do, repeating
> > > > `mux_proc_byte()` from `mux-fe`. In my use-case, I'll still need to disable
> > > > multiplexing in favor of 'mixing', for example with the 'mixer=on' option,
> > > > i.e. '-chardev mux-be,mixer=on,...`. Or do you think it should be some
> > > > completely different beast, something like mixer chardev?
> > >
> > > I think it would be saner to have the muxer be selectors: only work
> > > with one selected be or fe. Otherwise, we can run into various issues.
> >
> > In multiplexing (not mixing) for the use-case that I am describing, there is one
> > serious drawback: as soon as you switch the "focus" to another input device
> > (for example from vnc to socket chardev), you will not be able to s]witch back
> > from the same input console - the input now works on another device. This looks
> > strange and does not add convenience to the final user. Perhaps, for a case
> > other than console, this would be reasonable, but for console input -
> > I would like
> > to keep the mixer option: the front-end receives input from both back-ends.
>
> Agreed, I think this is desirable. If you did the exclusive access mode,
> it'd complicate things as you now need a way to switch between active
> backends, while also reducing the usefulness of it.
>
> The main thing I'm not a fan of here is the naming 'mux-fe', as I think we
> should have something distinct from current 'mux', to reduce confusion
> when we're talking about it.

The idea to have mux-fe and mux-be (current implementation) was born to
distinguish what exactly we multiplex: front-ends or back-ends.

As Mark-Andre rightly noted, input from back-end devices is not multiplexed,
but rather mixed.

>
> How about 'overlay' or 'replicator' ?

Overlay for me has a strong association with the filesystem concept. This
would work for me if combined back-end inputs function by layering one
on top of another, with potentially higher-priority inputs overriding
lower-priority ones. It implies a hierarchical or layered merging approach.
Not quite well describes a simple mixing strategy.

Replicator - this can be a good name from front-end device point of view:
suggests a mechanism for distributing the same input (front-end) to different
destinations (back-ends).

Two more: what about 'aggregator' or even 'hub' ?
Also 'mixer'? So we have '-chardev mux' and '-chardev mix' (try not to get
confused :)

--
Roman

Re: [PATCH v6 6/8] chardev/char-mux: implement backend chardev multiplexing

[Daniel P. Berrangé]

On Fri, Jan 10, 2025 at 09:43:52AM +0100, Roman Penyaev wrote:
> On Thu, Jan 9, 2025 at 7:07 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Thu, Jan 09, 2025 at 01:56:40PM +0100, Roman Penyaev wrote:
> > > Hi,
> > >
> > > On Tue, Jan 7, 2025 at 3:57 PM Marc-André Lureau
> > > <marcandre.lureau@redhat.com> wrote:
> > > > Whether we talk about multiplexing front-end or back-end, the issues
> > > > are similar. In general, mixing input will create issues. Teeing
> > > > output is less problematic, except to handle the buffering...
> > >
> > > I understand your concerns. What exact issues do you have in mind?
> > > Are these issues related to the input buffer handling, so technical issues?
> > > Or issues with usability?
> >
> > While the design / impl technically allows for concurrent input to be
> > sent to the frontend, from multiple backends, in practice I don't think
> > we need to be particularly concerned about it.
> >
> > I don't see this as being a way for multiple different users to interact
> > concurrently. Rather I'd see 1 user of the VM just deciding to switch
> > from one backend to the other on the fly. IOW, although technically
> > possible, the user will only be leveraging one at a time to send input.
> >
> > We very definitely do need all backends to receive output from the guest
> > concurrently too, as you'd want the historical output context to be
> > visible on whatever backend you choose to use at any given point in time.
> >
> > If a user decides to be crazy and send input from multiple backends
> > concurrently, then they get to keep the mess.
> >
> > > > > Do you think we need to artificially introduce multiplexing logic to be fully
> > > > > compliant with multiplexer naming? It's not hard to do, repeating
> > > > > `mux_proc_byte()` from `mux-fe`. In my use-case, I'll still need to disable
> > > > > multiplexing in favor of 'mixing', for example with the 'mixer=on' option,
> > > > > i.e. '-chardev mux-be,mixer=on,...`. Or do you think it should be some
> > > > > completely different beast, something like mixer chardev?
> > > >
> > > > I think it would be saner to have the muxer be selectors: only work
> > > > with one selected be or fe. Otherwise, we can run into various issues.
> > >
> > > In multiplexing (not mixing) for the use-case that I am describing, there is one
> > > serious drawback: as soon as you switch the "focus" to another input device
> > > (for example from vnc to socket chardev), you will not be able to s]witch back
> > > from the same input console - the input now works on another device. This looks
> > > strange and does not add convenience to the final user. Perhaps, for a case
> > > other than console, this would be reasonable, but for console input -
> > > I would like
> > > to keep the mixer option: the front-end receives input from both back-ends.
> >
> > Agreed, I think this is desirable. If you did the exclusive access mode,
> > it'd complicate things as you now need a way to switch between active
> > backends, while also reducing the usefulness of it.
> >
> > The main thing I'm not a fan of here is the naming 'mux-fe', as I think we
> > should have something distinct from current 'mux', to reduce confusion
> > when we're talking about it.
> 
> The idea to have mux-fe and mux-be (current implementation) was born to
> distinguish what exactly we multiplex: front-ends or back-ends.
> 
> As Mark-Andre rightly noted, input from back-end devices is not multiplexed,
> but rather mixed.
> 
> >
> > How about 'overlay' or 'replicator' ?
> 
> Overlay for me has a strong association with the filesystem concept. This
> would work for me if combined back-end inputs function by layering one
> on top of another, with potentially higher-priority inputs overriding
> lower-priority ones. It implies a hierarchical or layered merging approach.
> Not quite well describes a simple mixing strategy.
> 
> Replicator - this can be a good name from front-end device point of view:
> suggests a mechanism for distributing the same input (front-end) to different
> destinations (back-ends).
> 
> Two more: what about 'aggregator' or even 'hub' ?

Yes, those are ok

> Also 'mixer'? So we have '-chardev mux' and '-chardev mix' (try not to get
> confused :)

AFAIR, users would not use '-chardev mux', but instead set 'mux=on' on the
real chardev backend.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v6 6/8] chardev/char-mux: implement backend chardev multiplexing

[Roman Penyaev]

On Fri, Jan 10, 2025 at 9:58 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Fri, Jan 10, 2025 at 09:43:52AM +0100, Roman Penyaev wrote:
> > On Thu, Jan 9, 2025 at 7:07 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >
> > > On Thu, Jan 09, 2025 at 01:56:40PM +0100, Roman Penyaev wrote:
> > > > Hi,
> > > >
> > > > On Tue, Jan 7, 2025 at 3:57 PM Marc-André Lureau
> > > > <marcandre.lureau@redhat.com> wrote:
> > > > > Whether we talk about multiplexing front-end or back-end, the issues
> > > > > are similar. In general, mixing input will create issues. Teeing
> > > > > output is less problematic, except to handle the buffering...
> > > >
> > > > I understand your concerns. What exact issues do you have in mind?
> > > > Are these issues related to the input buffer handling, so technical issues?
> > > > Or issues with usability?
> > >
> > > While the design / impl technically allows for concurrent input to be
> > > sent to the frontend, from multiple backends, in practice I don't think
> > > we need to be particularly concerned about it.
> > >
> > > I don't see this as being a way for multiple different users to interact
> > > concurrently. Rather I'd see 1 user of the VM just deciding to switch
> > > from one backend to the other on the fly. IOW, although technically
> > > possible, the user will only be leveraging one at a time to send input.
> > >
> > > We very definitely do need all backends to receive output from the guest
> > > concurrently too, as you'd want the historical output context to be
> > > visible on whatever backend you choose to use at any given point in time.
> > >
> > > If a user decides to be crazy and send input from multiple backends
> > > concurrently, then they get to keep the mess.
> > >
> > > > > > Do you think we need to artificially introduce multiplexing logic to be fully
> > > > > > compliant with multiplexer naming? It's not hard to do, repeating
> > > > > > `mux_proc_byte()` from `mux-fe`. In my use-case, I'll still need to disable
> > > > > > multiplexing in favor of 'mixing', for example with the 'mixer=on' option,
> > > > > > i.e. '-chardev mux-be,mixer=on,...`. Or do you think it should be some
> > > > > > completely different beast, something like mixer chardev?
> > > > >
> > > > > I think it would be saner to have the muxer be selectors: only work
> > > > > with one selected be or fe. Otherwise, we can run into various issues.
> > > >
> > > > In multiplexing (not mixing) for the use-case that I am describing, there is one
> > > > serious drawback: as soon as you switch the "focus" to another input device
> > > > (for example from vnc to socket chardev), you will not be able to s]witch back
> > > > from the same input console - the input now works on another device. This looks
> > > > strange and does not add convenience to the final user. Perhaps, for a case
> > > > other than console, this would be reasonable, but for console input -
> > > > I would like
> > > > to keep the mixer option: the front-end receives input from both back-ends.
> > >
> > > Agreed, I think this is desirable. If you did the exclusive access mode,
> > > it'd complicate things as you now need a way to switch between active
> > > backends, while also reducing the usefulness of it.
> > >
> > > The main thing I'm not a fan of here is the naming 'mux-fe', as I think we
> > > should have something distinct from current 'mux', to reduce confusion
> > > when we're talking about it.
> >
> > The idea to have mux-fe and mux-be (current implementation) was born to
> > distinguish what exactly we multiplex: front-ends or back-ends.
> >
> > As Mark-Andre rightly noted, input from back-end devices is not multiplexed,
> > but rather mixed.
> >
> > >
> > > How about 'overlay' or 'replicator' ?
> >
> > Overlay for me has a strong association with the filesystem concept. This
> > would work for me if combined back-end inputs function by layering one
> > on top of another, with potentially higher-priority inputs overriding
> > lower-priority ones. It implies a hierarchical or layered merging approach.
> > Not quite well describes a simple mixing strategy.
> >
> > Replicator - this can be a good name from front-end device point of view:
> > suggests a mechanism for distributing the same input (front-end) to different
> > destinations (back-ends).
> >
> > Two more: what about 'aggregator' or even 'hub' ?
>
> Yes, those are ok
>
> > Also 'mixer'? So we have '-chardev mux' and '-chardev mix' (try not to get
> > confused :)
>
> AFAIR, users would not use '-chardev mux', but instead set 'mux=on' on the
> real chardev backend.

Yeah, right, I forgot about this peculiarity.

If Mark-Andre or anyone else has no objections, I'll drop all changes
to the original front-end 'mux' (I tried to make all names and variables reflect
the 'front-end' nature to reduce confusion with the back-end mux) and will
resend modified series introducing a new 'hub' chardev. I will keep the
possibility to send input from several back-ends to a single front-end, as
in the current implementation.

--
Roman

Re: [PATCH v6 6/8] chardev/char-mux: implement backend chardev multiplexing

[Marc-André Lureau]

Hi

On Fri, Jan 10, 2025 at 7:39 PM Roman Penyaev <r.peniaev@gmail.com> wrote:
>
> On Fri, Jan 10, 2025 at 9:58 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Fri, Jan 10, 2025 at 09:43:52AM +0100, Roman Penyaev wrote:
> > > On Thu, Jan 9, 2025 at 7:07 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > >
> > > > On Thu, Jan 09, 2025 at 01:56:40PM +0100, Roman Penyaev wrote:
> > > > > Hi,
> > > > >
> > > > > On Tue, Jan 7, 2025 at 3:57 PM Marc-André Lureau
> > > > > <marcandre.lureau@redhat.com> wrote:
> > > > > > Whether we talk about multiplexing front-end or back-end, the issues
> > > > > > are similar. In general, mixing input will create issues. Teeing
> > > > > > output is less problematic, except to handle the buffering...
> > > > >
> > > > > I understand your concerns. What exact issues do you have in mind?
> > > > > Are these issues related to the input buffer handling, so technical issues?
> > > > > Or issues with usability?
> > > >
> > > > While the design / impl technically allows for concurrent input to be
> > > > sent to the frontend, from multiple backends, in practice I don't think
> > > > we need to be particularly concerned about it.
> > > >
> > > > I don't see this as being a way for multiple different users to interact
> > > > concurrently. Rather I'd see 1 user of the VM just deciding to switch
> > > > from one backend to the other on the fly. IOW, although technically
> > > > possible, the user will only be leveraging one at a time to send input.
> > > >
> > > > We very definitely do need all backends to receive output from the guest
> > > > concurrently too, as you'd want the historical output context to be
> > > > visible on whatever backend you choose to use at any given point in time.
> > > >
> > > > If a user decides to be crazy and send input from multiple backends
> > > > concurrently, then they get to keep the mess.
> > > >
> > > > > > > Do you think we need to artificially introduce multiplexing logic to be fully
> > > > > > > compliant with multiplexer naming? It's not hard to do, repeating
> > > > > > > `mux_proc_byte()` from `mux-fe`. In my use-case, I'll still need to disable
> > > > > > > multiplexing in favor of 'mixing', for example with the 'mixer=on' option,
> > > > > > > i.e. '-chardev mux-be,mixer=on,...`. Or do you think it should be some
> > > > > > > completely different beast, something like mixer chardev?
> > > > > >
> > > > > > I think it would be saner to have the muxer be selectors: only work
> > > > > > with one selected be or fe. Otherwise, we can run into various issues.
> > > > >
> > > > > In multiplexing (not mixing) for the use-case that I am describing, there is one
> > > > > serious drawback: as soon as you switch the "focus" to another input device
> > > > > (for example from vnc to socket chardev), you will not be able to s]witch back
> > > > > from the same input console - the input now works on another device. This looks
> > > > > strange and does not add convenience to the final user. Perhaps, for a case
> > > > > other than console, this would be reasonable, but for console input -
> > > > > I would like
> > > > > to keep the mixer option: the front-end receives input from both back-ends.
> > > >
> > > > Agreed, I think this is desirable. If you did the exclusive access mode,
> > > > it'd complicate things as you now need a way to switch between active
> > > > backends, while also reducing the usefulness of it.
> > > >
> > > > The main thing I'm not a fan of here is the naming 'mux-fe', as I think we
> > > > should have something distinct from current 'mux', to reduce confusion
> > > > when we're talking about it.
> > >
> > > The idea to have mux-fe and mux-be (current implementation) was born to
> > > distinguish what exactly we multiplex: front-ends or back-ends.
> > >
> > > As Mark-Andre rightly noted, input from back-end devices is not multiplexed,
> > > but rather mixed.
> > >
> > > >
> > > > How about 'overlay' or 'replicator' ?
> > >
> > > Overlay for me has a strong association with the filesystem concept. This
> > > would work for me if combined back-end inputs function by layering one
> > > on top of another, with potentially higher-priority inputs overriding
> > > lower-priority ones. It implies a hierarchical or layered merging approach.
> > > Not quite well describes a simple mixing strategy.
> > >
> > > Replicator - this can be a good name from front-end device point of view:
> > > suggests a mechanism for distributing the same input (front-end) to different
> > > destinations (back-ends).
> > >
> > > Two more: what about 'aggregator' or even 'hub' ?
> >
> > Yes, those are ok
> >
> > > Also 'mixer'? So we have '-chardev mux' and '-chardev mix' (try not to get
> > > confused :)
> >
> > AFAIR, users would not use '-chardev mux', but instead set 'mux=on' on the
> > real chardev backend.
>
> Yeah, right, I forgot about this peculiarity.
>
> If Mark-Andre or anyone else has no objections, I'll drop all changes
> to the original front-end 'mux' (I tried to make all names and variables reflect
> the 'front-end' nature to reduce confusion with the back-end mux) and will
> resend modified series introducing a new 'hub' chardev. I will keep the
> possibility to send input from several back-ends to a single front-end, as
> in the current implementation.
>

Based on the above discussion, I am okay with this plan.
thanks


-- 
Marc-André Lureau