[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] chardev: fix mess in OPENED/CLOSED events when
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH] chardev: fix mess in OPENED/CLOSED events when muxed |
Date: |
Sun, 4 Nov 2018 18:00:11 +0400 |
Hi
On Thu, Nov 1, 2018 at 6:55 PM Artem Pisarenko
<address@hidden> wrote:
>
> When chardev is multiplexed (mux=on) there are a lot of cases, when
> CHR_EVENT_OPENED/CHR_EVENT_CLOSED events pairing (expected from
> frontend side) is broken. There are either generation of multiple
> repeated or extra CHR_EVENT_OPENED events, or CHR_EVENT_CLOSED just
> isn't generated at all (when it does with mux=off).
> Fix that.
>
> Signed-off-by: Artem Pisarenko <address@hidden>
> ---
>
> Notes:
> This issue actually more complex. Idea of generating events from inside
> function called '*_set_handlers' isn't good, at least its implicit nature,
> and especially a fact, that function decides about open state (see 'fe_open'
> variable), but generates event only in one direction. Combined with
> 'mux_chr_set_handlers()' hack this makes things even worse.
> Better solution is to change fe interface and rewrite all frontends code
> (a lot of stuff in hw/char/* and somewhere else).
> Although this patch doesn't fix any issue/bug (known to me), it prevents
> them in future. Also it optimizes emulation performance by avoiding extra
> activity.
> I did several trivial tests on x86_64 target and seems like nothing
> broken.
I am a bit reluctant to take patches that don't actually "fix" things.
Could you add some tests to demonstrate the problems?
>
> chardev/char-fe.c | 9 ++++++---
> chardev/char-mux.c | 13 ++++++++-----
> include/chardev/char-mux.h | 2 +-
> 3 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index a8931f7..31cf7f0 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -257,6 +257,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
> {
> Chardev *s;
> int fe_open;
> + static __thread bool mux_reentered;
Not very elegant. Maybe mux_chr_set_handlers() could call a refactored
internal chr_fe_set_handlers() with an extra arg "no_open_event" ?
>
> s = b->chr;
> if (!s) {
> @@ -284,14 +285,16 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
> if (fe_open) {
> qemu_chr_fe_take_focus(b);
> /* We're connecting to an already opened device, so let's make sure
> we
> - also get the open event */
> - if (s->be_open) {
> + also get the open event (hack: except when chardev is muxed) */
> + if (s->be_open && !mux_reentered) {
> qemu_chr_be_event(s, CHR_EVENT_OPENED);
> }
> }
>
> if (CHARDEV_IS_MUX(s)) {
> - mux_chr_set_handlers(s, context);
> + mux_reentered = true;
> + mux_chr_set_handlers(s, fe_open, context);
> + mux_reentered = false;
> }
> }
>
> diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> index 6055e76..9244802 100644
> --- a/chardev/char-mux.c
> +++ b/chardev/char-mux.c
> @@ -272,21 +272,24 @@ static void char_mux_finalize(Object *obj)
> for (i = 0; i < d->mux_cnt; i++) {
> CharBackend *be = d->backends[i];
> if (be) {
> + if (be->chr && be->chr->be_open) {
> + qemu_chr_be_event(be->chr, CHR_EVENT_CLOSED);
> + }
It looks like this could be a seperate patch, with a seperate test.
> be->chr = NULL;
> }
> }
> qemu_chr_fe_deinit(&d->chr, false);
> }
>
> -void mux_chr_set_handlers(Chardev *chr, GMainContext *context)
> +void mux_chr_set_handlers(Chardev *chr, bool is_open, GMainContext *context)
> {
> MuxChardev *d = MUX_CHARDEV(chr);
>
> /* Fix up the real driver with mux routines */
> qemu_chr_fe_set_handlers(&d->chr,
> - mux_chr_can_read,
> - mux_chr_read,
> - mux_chr_event,
> + is_open ? mux_chr_can_read : NULL,
> + is_open ? mux_chr_read : NULL,
> + is_open ? mux_chr_event : NULL,
same
> NULL,
> chr,
> context, true);
> @@ -367,7 +370,7 @@ static int open_muxes(Chardev *chr)
> * mark mux as OPENED so any new FEs will immediately receive
> * OPENED event
> */
> - qemu_chr_be_event(chr, CHR_EVENT_OPENED);
> + chr->be_open = 1;
>
> return 0;
> }
> diff --git a/include/chardev/char-mux.h b/include/chardev/char-mux.h
> index 1e13187..4b4df6e 100644
> --- a/include/chardev/char-mux.h
> +++ b/include/chardev/char-mux.h
> @@ -55,7 +55,7 @@ typedef struct MuxChardev {
> #define CHARDEV_IS_MUX(chr) \
> object_dynamic_cast(OBJECT(chr), TYPE_CHARDEV_MUX)
>
> -void mux_chr_set_handlers(Chardev *chr, GMainContext *context);
> +void mux_chr_set_handlers(Chardev *chr, bool is_open, GMainContext *context);
> void mux_set_focus(Chardev *chr, int focus);
> void mux_chr_send_all_event(Chardev *chr, int event);
>
> --
> 2.7.4
thanks!