Re: [Qemu-devel] [RFC v6 10/27] monitor: allow to use IO thread for parsing

[Stefan Hajnoczi]

On Tue, Dec 19, 2017 at 04:45:40PM +0800, Peter Xu wrote:
>      if (monitor_is_qmp(mon)) {
> -        qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, 
> monitor_qmp_read,
> -                                 monitor_qmp_event, NULL, mon, NULL, true);
>          qemu_chr_fe_set_echo(&mon->chr, true);
>          json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
> +        qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, 
> monitor_qmp_read,
> +                                 monitor_qmp_event, NULL, mon, context, 
> true);
>      } else {
> +        assert(!context);       /* HMP does not support IOThreads */
>          qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_read,
>                                   monitor_event, NULL, mon, NULL, true);
>      }

qemu_chr_fe_set_handlers() isn't thread-safe.  It looks like
monitor_can_read()/monitor_qmp_read() can run in the IOThread at the
same time as monitor_qmp_event() runs in the main loop:

  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_be_update_read_handlers(s, context);
        ^-- The IOThread may invoke handler functions from now on!

      <-- Everything below races with the IOThread!

      if (set_open) {
          qemu_chr_fe_set_open(b, fe_open);
      }

      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) {
              qemu_chr_be_event(s, CHR_EVENT_OPENED);
          }
      }

      if (CHARDEV_IS_MUX(s)) {
          mux_chr_set_handlers(s, context);
      }
  }

It looks like chr_*() functions must be called from the event loop
thread that monitors the chardev.  You can use aio_bh_schedule_oneshot()
or g_idle_source_new() to execute the last part of monitor_init() in the
GMainContext.

That way there is no race condition because qemu_chr_fe_set_handlers()
is called from within the event loop thread.

signature.asc
Description: PGP signature