On Wed, Dec 13, 2017 at 04:35:00PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 05, 2017 at 01:51:44PM +0800, Peter Xu wrote:
> > @@ -583,6 +585,7 @@ static void monitor_data_init(Monitor *mon, bool
> > skip_flush)
> > /* Use *mon_cmds by default. */
> > mon->cmd_table = mon_cmds;
> > mon->skip_flush = skip_flush;
> > + mon->use_io_thr = use_io_thr;
>
> Why is mon->use_io_thr is a field instead of a monitor_init() argument.
It's used in other part of code when we want to know whether IOThread
is enabled for a monitor.
>
> > @@ -4117,19 +4121,37 @@ void monitor_init(Chardev *chr, int flags)
> > monitor_read_command(mon, 0);
> > }
> >
> > + if (mon->use_io_thr) {
> > + /*
> > + * When use_io_thr is set, we use the global shared dedicated
> > + * IO thread for this monitor to handle input/output.
> > + */
> > + context = monitor_io_context_get();
> > + /* We should have inited globals before reaching here. */
> > + assert(context);
> > + } else {
> > + /* The default main loop, which is the main thread */
> > + context = NULL;
> > + }
> > +
> > + /*
> > + * Add the monitor before running it (which is triggered by
> > + * qemu_chr_fe_set_handlers), otherwise one monitor may find
> > + * itself not on the mon_list when running.
> > + */
> > + qemu_mutex_lock(&monitor_lock);
> > + QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
> > + qemu_mutex_unlock(&monitor_lock);
> > +
> > 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);
> > + monitor_qmp_event, NULL, mon, context,
> > true);
> > qemu_chr_fe_set_echo(&mon->chr, true);
> > json_message_parser_init(&mon->qmp.parser, handle_qmp_command,
> > mon);
>
> The comment above mentions the race condition between
> qemu_chr_fe_set_handlers() and mon_list. I think that means the order
> must be:
>
> json_message_parser_init(&mon->qmp.parser, handle_qmp_command, mon);
> qemu_chr_fe_set_echo(&mon->chr, true);
> qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
> monitor_qmp_event, NULL, mon, context, true);
Yeh this looks safer!
>
> > } else {
>
> assert(!context); /* HMP does not support IOThreads */
I can add this. Thanks,
>
> > qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_read,
> > monitor_event, NULL, mon, NULL, true);
> > }
> > -
> > - qemu_mutex_lock(&monitor_lock);
> > - QLIST_INSERT_HEAD(&mon_list, mon, entry);
> > - qemu_mutex_unlock(&monitor_lock);
> > }
> >
> > void monitor_cleanup(void)
> > --
> > 2.14.3
> >
--
Peter Xu
