Re: [Qemu-devel] [PATCH] chardev: fix mess in OPENED/CLOSED events when muxed

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

Hi
On Thu, Nov 1, 2018 at 6:55 PM Artem Pisarenko
<artem.k.pisarenko@gmail.com> 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 <artem.k.pisarenko@gmail.com>
> ---
>
> 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!