Re: [Qemu-devel] [PATCH v7 09/23] monitor: allow using IO thread for parsing

[Stefan Hajnoczi]

On Wed, Jan 24, 2018 at 01:39:43PM +0800, Peter Xu wrote:
> @@ -4034,12 +4044,29 @@ static void sortcmdlist(void)
>      qsort((void *)info_cmds, array_num, elem_size, compare_mon_cmd);
>  }
>  
> +static GMainContext *monitor_io_context_get(void)
> +{
> +    return iothread_get_g_main_context(mon_global.mon_iothread);
> +}
> +
> +static AioContext *monitor_aio_context_get(void)
> +{
> +    return iothread_get_aio_context(mon_global.mon_iothread);
> +}

Please follow the X_get_Y() naming convention instead of X_Y_get().  For
example, see qemu_get_aio_context() and iothread_get_aio_context().

> @@ -4082,11 +4109,41 @@ void error_vprintf_unless_qmp(const char *fmt, 
> va_list ap)
>      }
>  }
>  
> +static void monitor_list_append(Monitor *mon)
> +{
> +    qemu_mutex_lock(&monitor_lock);
> +    QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
> +    qemu_mutex_unlock(&monitor_lock);
> +}
> +
> +static void monitor_qmp_setup_handlers(void *data)

BH functions are usually declared like this:

  static void X_bh(void *opaque)

This way it's immediately clear that this function is invoked as a BH.

I suggest renaming the function to monitor_qmp_setup_handlers_bh().
Using 'opaque' instead of 'data' is common, too.

> @@ -4099,24 +4156,55 @@ void monitor_init(Chardev *chr, int flags)
>      }
>  
>      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);
> +        if (mon->use_io_thr) {
> +            /*
> +             * It's possible that we already have an IOWatchPoll
> +             * registered for the Chardev during chardev_init_func().

When does this happen?

This seems like a hack that breaks when certain -chardev options are
used.  For example, what happens if the chardev is a TCP connection with
reconnect=5.  In that case the socket will be connecting asynchronously
and we cannot just remove the fd watch.

How does this interact with TCP listen chardevs?  It looks like the
listener socket uses the main loop (see tcp_chr_disconnect()).

I'm worried that the chardev layer isn't thread-safe and you haven't
added anything to protect it or at least refuse to run in unsafe
conditions.

signature.asc
Description: PGP signature