Hi Roman
On Fri, Sep 13, 2024 at 8:37 PM Roman Penyaev <r.peni...@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.peni...@gmail.com>
> Cc: "Marc-André Lureau" <marcandre.lur...@redhat.com>
> Cc: Paolo Bonzini <pbonz...@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.
