Re: [Qemu-devel] [PATCH v3 02/15] monitor: Split monitor_init in HMP and QMP function

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Instead of mixing HMP and QMP monitors in the same function, separate
> the monitor creation function for both.
>
> While in theory, one could pass both MONITOR_USE_CONTROL and
> MONITOR_USE_READLINE before this patch and both flags would do
> something, readline support is tightly coupled with HMP: QMP never feeds
> its input to readline, and the tab completion function treats the input
> as an HMP command. Therefore, this configuration is useless.
>
> After this patch, the QMP path asserts that MONITOR_USE_READLINE is not
> set. The HMP path can be used with or without MONITOR_USE_READLINE, like
> before.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 89 ++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 52 insertions(+), 37 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 9846a5623b..a70c1283b1 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -704,7 +704,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline);
>  
>  static void monitor_iothread_init(void);
>  
> -static void monitor_data_init(Monitor *mon, bool skip_flush,
> +static void monitor_data_init(Monitor *mon, int flags, bool skip_flush,
>                                bool use_io_thread)
>  {
>      if (use_io_thread && !mon_iothread) {
           monitor_iothread_init();
       }
       memset(mon, 0, sizeof(Monitor));

I'd like to replace this memset() by ...

       qemu_mutex_init(&mon->mon_lock);
       qemu_mutex_init(&mon->qmp.qmp_queue_lock);
       mon->outbuf = qstring_new();
       /* Use *mon_cmds by default. */
       mon->cmd_table = mon_cmds;
> @@ -719,6 +719,7 @@ static void monitor_data_init(Monitor *mon, bool skip_flush,
>      mon->skip_flush = skip_flush;
>      mon->use_io_thread = use_io_thread;
>      mon->qmp.qmp_requests = g_queue_new();
> +    mon->flags = flags;
>  }
>  
>  static void monitor_data_destroy(Monitor *mon)
> @@ -742,7 +743,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
>      char *output = NULL;
>      Monitor *old_mon, hmp;

... hmp = {} here (moved from PATCH 04), and ...

>  
> -    monitor_data_init(&hmp, true, false);
> +    monitor_data_init(&hmp, 0, true, false);
>  
>      old_mon = cur_mon;
>      cur_mon = &hmp;
> @@ -4605,19 +4606,51 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
>      monitor_list_append(mon);
>  }
>  
> -void monitor_init(Chardev *chr, int flags)
> +static void monitor_init_qmp(Chardev *chr, int flags)
>  {
>      Monitor *mon = g_malloc(sizeof(*mon));

... g_malloc0() here (moved from PATCH 03), and ...

> -    bool use_readline = flags & MONITOR_USE_READLINE;
> +
> +    /* Only HMP supports readline */
> +    assert(!(flags & MONITOR_USE_READLINE));
>  
>      /* Note: we run QMP monitor in I/O thread when @chr supports that */
> -    monitor_data_init(mon, false,
> -                      (flags & MONITOR_USE_CONTROL)
> -                      && qemu_chr_has_feature(chr,
> -                                              QEMU_CHAR_FEATURE_GCONTEXT));
> +    monitor_data_init(mon, flags, false,
> +                      qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT));
>  
>      qemu_chr_fe_init(&mon->chr, chr, &error_abort);
> -    mon->flags = flags;
> +    qemu_chr_fe_set_echo(&mon->chr, true);
> +
> +    json_message_parser_init(&mon->qmp.parser, handle_qmp_command, mon, NULL);
> +    if (mon->use_io_thread) {
> +        /*
> +         * Make sure the old iowatch is gone.  It's possible when
> +         * e.g. the chardev is in client mode, with wait=on.
> +         */
> +        remove_fd_in_watch(chr);
> +        /*
> +         * We can't call qemu_chr_fe_set_handlers() directly here
> +         * since chardev might be running in the monitor I/O
> +         * thread.  Schedule a bottom half.
> +         */
> +        aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
> +                                monitor_qmp_setup_handlers_bh, mon);
> +        /* The bottom half will add @mon to @mon_list */
> +    } else {
> +        qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read,
> +                                 monitor_qmp_read, monitor_qmp_event,
> +                                 NULL, mon, NULL, true);
> +        monitor_list_append(mon);
> +    }
> +}
> +
> +static void monitor_init_hmp(Chardev *chr, int flags)
> +{
> +    Monitor *mon = g_malloc(sizeof(*mon));

... and g_malloc0() here (moved from PATCH 04).

This way, the responsibility for zeroing moves just once, right when its
new owners get created.  Saves us explaining in PATCH 03+04 why we make
these changes.  They were confusing enough for me to ask for an
explanation in my review of v2.

Happy do to it in my tree.

I'd be tempted to assert(!(flags & MONITOR_USE_CONTROL)) here, and the
opposite in monitor_init_qmp(), if your PATCH 14 didn't get rid of
@flags.  Okay as it is.

> +    bool use_readline = flags & MONITOR_USE_READLINE;
> +
> +    monitor_data_init(mon, flags, false, false);
> +    qemu_chr_fe_init(&mon->chr, chr, &error_abort);
> +
>      if (use_readline) {
>          mon->rs = readline_init(monitor_readline_printf,
>                                  monitor_readline_flush,
[...]