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

[Peter Xu <peterx@redhat.com>]

On Fri, Jan 05, 2018 at 05:22:26PM +0000, Stefan Hajnoczi wrote:
> 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.

Good catch.  I'll take your suggestion.  Thanks!

-- 
Peter Xu