Re: [Qemu-devel] [PATCH v3] qemu-char: don't issue CHR_EVENT_OPEN in a BH

[Anthony Liguori <aliguori@us.ibm.com>]

Michael Roth <mdroth@linux.vnet.ibm.com> writes:

> When CHR_EVENT_OPENED was initially added, it was CHR_EVENT_RESET,
> and it was issued as a bottom-half:
>
> 86e94dea5b740dad65446c857f6959eae43e0ba6
>
> Which we basically used to print out a greeting/prompt for the
> monitor.
>
> AFAICT the only reason this was ever done in a BH was because in
> some cases we'd modify the chr_write handler for a new chardev
> backend *after* the site where we issued the reset (see:
> 86e94d:qemu_chr_open_stdio())
>
> At some point this event was renamed to CHR_EVENT_OPENED, and we've
> maintained the use of this BH ever since.
>
> However, due to 9f939df955a4152aad69a19a77e0898631bb2c18, we schedule
> the BH via g_idle_add(), which is causing events to sometimes be
> delivered after we've already begun processing data from backends,
> leading to:
>
>  known bugs:
>
>   QMP:
>     session negotation resets with OPENED event, in some cases this
>     is causing new sessions to get sporadically reset
>
>  potential bugs:
>
>   hw/usb/redirect.c:
>     can_read handler checks for dev->parser != NULL, which may be
>     true if CLOSED BH has not been executed yet. In the past, OPENED
>     quiesced outstanding CLOSED events prior to us reading client
>     data. If it's delayed, our check may allow reads to occur even
>     though we haven't processed the OPENED event yet, and when we
>     do finally get the OPENED event, our state may get reset.
>
>   qtest.c:
>     can begin session before OPENED event is processed, leading to
>     a spurious reset of the system and irq_levels
>
>   gdbstub.c:
>     may start a gdb session prior to the machine being paused
>
> To fix these, let's just drop the BH.
>
> Since the initial reasoning for using it still applies to an extent,
> work around that by deferring the delivery of CHR_EVENT_OPENED until
> after the chardevs have been fully initialized, toward the end of
> qmp_chardev_add() (or some cases, qemu_chr_new_from_opts()). This
> defers delivery long enough that we can be assured a CharDriverState
> is fully initialized before CHR_EVENT_OPENED is sent.
>
> Also, rather than requiring each chardev to do an explicit open, do it
> automatically, and allow the small few who don't desire such behavior to
> suppress the OPENED-on-init behavior by setting a
> 'suppress_be_open_on_init' flag.
>
> We additionally add missing OPENED events for stdio backends on w32,
> which were previously not being issued, causing us to not recieve the
> banner and initial prompts for qmp/hmp.
>
> Reported-by: Stefan Priebe <s.priebe@profihost.ag>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Thanks for digging into this Mike!

Regards,

Anthony Liguori

> ---
> v2->v3:
>  * removed artifact in from v1 in backends/baum.c, test build with
>    BRLAPI=y (Anthony)
>  * rebased on latest origin/master
>
> v1->v2:
>  * default to sending OPENED on backend init, add flag to suppress
>    it (Anthony)
>  * fix missing OPENED for stdio backends on w32
>  * fix missing OPENED when qemu_chr_new_from_opts() doesn't use
>    qmp_chardev_add()
>  * clean up/update commit message
>
>  backends/baum.c       |    2 --
>  include/sysemu/char.h |    2 +-
>  qemu-char.c           |   38 +++++++++++++++++---------------------
>  ui/console.c          |    1 -
>  ui/gtk.c              |    1 -
>  5 files changed, 18 insertions(+), 26 deletions(-)
>
> diff --git a/backends/baum.c b/backends/baum.c
> index 4cba79f..62aa784 100644
> --- a/backends/baum.c
> +++ b/backends/baum.c
> @@ -611,8 +611,6 @@ CharDriverState *chr_baum_init(void)
>  
>      qemu_set_fd_handler(baum->brlapi_fd, baum_chr_read, NULL, baum);
>  
> -    qemu_chr_be_generic_open(chr);
> -
>      return chr;
>  
>  fail:
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 5e42c90..b0ae749 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -70,13 +70,13 @@ struct CharDriverState {
>      void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
>      void (*chr_set_fe_open)(struct CharDriverState *chr, int fe_open);
>      void *opaque;
> -    int idle_tag;
>      char *label;
>      char *filename;
>      int be_open;
>      int fe_open;
>      int explicit_fe_open;
>      int avail_connections;
> +    bool suppress_be_open_on_init;
>      QemuOpts *opts;
>      QTAILQ_ENTRY(CharDriverState) next;
>  };
> diff --git a/qemu-char.c b/qemu-char.c
> index d04b429..8043f86 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -110,19 +110,9 @@ void qemu_chr_be_event(CharDriverState *s, int event)
>      s->chr_event(s->handler_opaque, event);
>  }
>  
> -static gboolean qemu_chr_be_generic_open_bh(gpointer opaque)
> -{
> -    CharDriverState *s = opaque;
> -    qemu_chr_be_event(s, CHR_EVENT_OPENED);
> -    s->idle_tag = 0;
> -    return FALSE;
> -}
> -
>  void qemu_chr_be_generic_open(CharDriverState *s)
>  {
> -    if (s->idle_tag == 0) {
> -        s->idle_tag = g_idle_add(qemu_chr_be_generic_open_bh, s);
> -    }
> +    qemu_chr_be_event(s, CHR_EVENT_OPENED);
>  }
>  
>  int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
> @@ -247,6 +237,7 @@ static CharDriverState *qemu_chr_open_null(void)
>  
>      chr = g_malloc0(sizeof(CharDriverState));
>      chr->chr_write = null_chr_write;
> +    chr->suppress_be_open_on_init = true;
>      return chr;
>  }
>  
> @@ -504,9 +495,6 @@ static CharDriverState *qemu_chr_open_mux(CharDriverState *drv)
>      /* Frontend guest-open / -close notification is not support with muxes */
>      chr->chr_set_fe_open = NULL;
>  
> -    /* Muxes are always open on creation */
> -    qemu_chr_be_generic_open(chr);
> -
>      return chr;
>  }
>  
> @@ -883,8 +871,6 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out)
>      chr->chr_update_read_handler = fd_chr_update_read_handler;
>      chr->chr_close = fd_chr_close;
>  
> -    qemu_chr_be_generic_open(chr);
> -
>      return chr;
>  }
>  
> @@ -1243,6 +1229,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
>      chr->chr_update_read_handler = pty_chr_update_read_handler;
>      chr->chr_close = pty_chr_close;
>      chr->chr_add_watch = pty_chr_add_watch;
> +    chr->suppress_be_open_on_init = true;
>  
>      s->fd = io_channel_from_fd(master_fd);
>      s->timer_tag = 0;
> @@ -1595,8 +1582,6 @@ static CharDriverState *qemu_chr_open_pp_fd(int fd)
>      chr->chr_close = pp_close;
>      chr->opaque = drv;
>  
> -    qemu_chr_be_generic_open(chr);
> -
>      return chr;
>  }
>  #endif /* __linux__ */
> @@ -1650,6 +1635,7 @@ static CharDriverState *qemu_chr_open_pp_fd(int fd)
>      chr->opaque = (void *)(intptr_t)fd;
>      chr->chr_write = null_chr_write;
>      chr->chr_ioctl = pp_ioctl;
> +    chr->suppress_be_open_on_init = true;
>      return chr;
>  }
>  #endif
> @@ -1880,7 +1866,6 @@ static CharDriverState *qemu_chr_open_win_path(const char *filename)
>          g_free(chr);
>          return NULL;
>      }
> -    qemu_chr_be_generic_open(chr);
>      return chr;
>  }
>  
> @@ -1980,7 +1965,6 @@ static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
>          g_free(chr);
>          return NULL;
>      }
> -    qemu_chr_be_generic_open(chr);
>      return chr;
>  }
>  
> @@ -1994,7 +1978,6 @@ static CharDriverState *qemu_chr_open_win_file(HANDLE fd_out)
>      s->hcom = fd_out;
>      chr->opaque = s;
>      chr->chr_write = win_chr_write;
> -    qemu_chr_be_generic_open(chr);
>      return chr;
>  }
>  
> @@ -2329,6 +2312,8 @@ static CharDriverState *qemu_chr_open_udp_fd(int fd)
>      chr->chr_write = udp_chr_write;
>      chr->chr_update_read_handler = udp_chr_update_read_handler;
>      chr->chr_close = udp_chr_close;
> +    /* be isn't opened until we get a connection */
> +    chr->suppress_be_open_on_init = true;
>      return chr;
>  }
>  
> @@ -2731,6 +2716,8 @@ static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay,
>      chr->get_msgfd = tcp_get_msgfd;
>      chr->chr_add_client = tcp_chr_add_client;
>      chr->chr_add_watch = tcp_chr_add_watch;
> +    /* be isn't opened until we get a connection */
> +    chr->suppress_be_open_on_init = true;
>  
>      if (is_listen) {
>          s->listen_fd = fd;
> @@ -3325,6 +3312,12 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
>      if (!chr->filename)
>          chr->filename = g_strdup(qemu_opt_get(opts, "backend"));
>      chr->init = init;
> +    /* if we didn't create the chardev via qmp_chardev_add, we
> +     * need to send the OPENED event here
> +     */
> +    if (!chr->suppress_be_open_on_init) {
> +        qemu_chr_be_event(chr, CHR_EVENT_OPENED);
> +    }
>      QTAILQ_INSERT_TAIL(&chardevs, chr, next);
>  
>      if (qemu_opt_get_bool(opts, "mux", 0)) {
> @@ -3804,6 +3797,9 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
>          if (!chr->filename) {
>              chr->filename = g_strdup(ChardevBackendKind_lookup[backend->kind]);
>          }
> +        if (!chr->suppress_be_open_on_init) {
> +            qemu_chr_be_event(chr, CHR_EVENT_OPENED);
> +        }
>          QTAILQ_INSERT_TAIL(&chardevs, chr, next);
>          return ret;
>      } else {
> diff --git a/ui/console.c b/ui/console.c
> index b30853f..54b192d 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1746,7 +1746,6 @@ static void text_console_do_init(CharDriverState *chr, DisplayState *ds)
>          s->t_attrib = s->t_attrib_default;
>      }
>  
> -    qemu_chr_be_generic_open(chr);
>      if (chr->init)
>          chr->init(chr);
>  }
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 52c3f95..71b2497 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -1214,7 +1214,6 @@ static GSList *gd_vc_init(GtkDisplayState *s, VirtualConsole *vc, int index, GSL
>  
>      gtk_menu_shell_append(GTK_MENU_SHELL(view_menu), vc->menu_item);
>  
> -    qemu_chr_be_generic_open(vc->chr);
>      if (vc->chr->init) {
>          vc->chr->init(vc->chr);
>      }
> -- 
> 1.7.9.5