[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/2] chardev: fix mess in OPENED/CLOSED event
|
From: |
Marc-André Lureau |
|
Subject: |
Re: [Qemu-devel] [PATCH v3 1/2] chardev: fix mess in OPENED/CLOSED events when muxed |
|
Date: |
Mon, 26 Nov 2018 17:42:33 +0400 |
On Tue, Nov 6, 2018 at 4:41 PM Artem Pisarenko
<address@hidden> wrote:
>
> When chardev is multiplexed (mux=on) there are a lot of cases where
> CHR_EVENT_OPENED/CHR_EVENT_CLOSED events pairing (expected from
> frontend side) is broken. There are either generation of multiple
> repeated or extra CHR_EVENT_OPENED events, or CHR_EVENT_CLOSED just
> isn't generated at all.
> This is mostly because 'qemu_chr_fe_set_handlers()' function makes its
> own (and often wrong) implicit decision on updated frontend state and
> invokes 'fd_event' callback with 'CHR_EVENT_OPENED'. And even worse,
> it doesn't do symmetric action in opposite direction, as someone may
> expect (i.e. it doesn't invoke previously set 'fd_event' with
> 'CHR_EVENT_CLOSED'). Muxed chardev uses trick by calling this function
> again to replace callback handlers with its own ones, but it doesn't
> account for such side effect.
> Fix that using extended version of this function with added argument
> for disabling side effect and keep original function for compatibility
> with lots of frontends already using this interface and being
> "tolerant" to its side effects.
> One more source of event duplication is just line of code in
> char-mux.c, which does far more than comment above says (obvious fix).
>
> Signed-off-by: Artem Pisarenko <address@hidden>
Reviewed-by: Marc-André Lureau <address@hidden>
> ---
>
> Notes:
> v3:
> - extended and improved commit message with 'why' and 'how' explanation
>
> chardev/char-fe.c | 33 ++++++++++++++++++++++++---------
> chardev/char-mux.c | 16 ++++++++--------
> include/chardev/char-fe.h | 18 +++++++++++++++++-
> 3 files changed, 49 insertions(+), 18 deletions(-)
>
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index a8931f7..b7bcbd5 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -246,14 +246,15 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del)
> }
> }
>
> -void qemu_chr_fe_set_handlers(CharBackend *b,
> - IOCanReadHandler *fd_can_read,
> - IOReadHandler *fd_read,
> - IOEventHandler *fd_event,
> - BackendChangeHandler *be_change,
> - void *opaque,
> - GMainContext *context,
> - bool set_open)
> +void qemu_chr_fe_set_handlers_full(CharBackend *b,
> + IOCanReadHandler *fd_can_read,
> + IOReadHandler *fd_read,
> + IOEventHandler *fd_event,
> + BackendChangeHandler *be_change,
> + void *opaque,
> + GMainContext *context,
> + bool set_open,
> + bool sync_state)
> {
> Chardev *s;
> int fe_open;
> @@ -285,7 +286,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
> qemu_chr_fe_take_focus(b);
> /* We're connecting to an already opened device, so let's make sure
> we
> also get the open event */
> - if (s->be_open) {
> + if (sync_state && s->be_open) {
> qemu_chr_be_event(s, CHR_EVENT_OPENED);
> }
> }
> @@ -295,6 +296,20 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
> }
> }
>
> +void qemu_chr_fe_set_handlers(CharBackend *b,
> + IOCanReadHandler *fd_can_read,
> + IOReadHandler *fd_read,
> + IOEventHandler *fd_event,
> + BackendChangeHandler *be_change,
> + void *opaque,
> + GMainContext *context,
> + bool set_open)
> +{
> + qemu_chr_fe_set_handlers_full(b, fd_can_read, fd_read, fd_event,
> be_change,
> + opaque, context, set_open,
> + true);
> +}
> +
> void qemu_chr_fe_take_focus(CharBackend *b)
> {
> if (!b->chr) {
> diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> index 6055e76..1199d32 100644
> --- a/chardev/char-mux.c
> +++ b/chardev/char-mux.c
> @@ -283,13 +283,13 @@ void mux_chr_set_handlers(Chardev *chr, GMainContext
> *context)
> MuxChardev *d = MUX_CHARDEV(chr);
>
> /* Fix up the real driver with mux routines */
> - qemu_chr_fe_set_handlers(&d->chr,
> - mux_chr_can_read,
> - mux_chr_read,
> - mux_chr_event,
> - NULL,
> - chr,
> - context, true);
> + qemu_chr_fe_set_handlers_full(&d->chr,
> + mux_chr_can_read,
> + mux_chr_read,
> + mux_chr_event,
> + NULL,
> + chr,
> + context, true, false);
> }
>
> void mux_set_focus(Chardev *chr, int focus)
> @@ -367,7 +367,7 @@ static int open_muxes(Chardev *chr)
> * mark mux as OPENED so any new FEs will immediately receive
> * OPENED event
> */
> - qemu_chr_be_event(chr, CHR_EVENT_OPENED);
> + chr->be_open = 1;
>
> return 0;
> }
> diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
> index 46c997d..c1b7fd9 100644
> --- a/include/chardev/char-fe.h
> +++ b/include/chardev/char-fe.h
> @@ -67,7 +67,7 @@ bool qemu_chr_fe_backend_connected(CharBackend *be);
> bool qemu_chr_fe_backend_open(CharBackend *be);
>
> /**
> - * qemu_chr_fe_set_handlers:
> + * qemu_chr_fe_set_handlers_full:
> * @b: a CharBackend
> * @fd_can_read: callback to get the amount of data the frontend may
> * receive
> @@ -79,12 +79,28 @@ bool qemu_chr_fe_backend_open(CharBackend *be);
> * @context: a main loop context or NULL for the default
> * @set_open: whether to call qemu_chr_fe_set_open() implicitely when
> * any of the handler is non-NULL
> + * @sync_state: whether to issue event callback with updated state
> *
> * Set the front end char handlers. The front end takes the focus if
> * any of the handler is non-NULL.
> *
> * Without associated Chardev, nothing is changed.
> */
> +void qemu_chr_fe_set_handlers_full(CharBackend *b,
> + IOCanReadHandler *fd_can_read,
> + IOReadHandler *fd_read,
> + IOEventHandler *fd_event,
> + BackendChangeHandler *be_change,
> + void *opaque,
> + GMainContext *context,
> + bool set_open,
> + bool sync_state);
> +
> +/**
> + * qemu_chr_fe_set_handlers:
> + *
> + * Version of qemu_chr_fe_set_handlers_full() with sync_state = true.
> + */
> void qemu_chr_fe_set_handlers(CharBackend *b,
> IOCanReadHandler *fd_can_read,
> IOReadHandler *fd_read,
> --
> 2.7.4
>