Re: [Qemu-devel] [PATCH for-2.12 3/8] monitor: new parameter "x-oob"

[Marc-André Lureau]

Hi

On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <address@hidden> wrote:
> Add new parameter to optionally enable Out-Of-Band for a QMP server.
>
> An example command line:
>
>   ./qemu-system-x86_64 -chardev stdio,id=char0 \
>                        -mon chardev=char0,mode=control,x-oob=on
>
> By default, Out-Of-Band is off.
>
> It is not allowed if either MUX or non-QMP is detected, since
> Out-Of-Band is currently only for QMP, and non-MUX chardev backends.
>
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  include/monitor/monitor.h |  1 +
>  monitor.c                 | 22 ++++++++++++++++++++--
>  vl.c                      |  5 +++++
>  3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 0cb0538a31..d6ab70cae2 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -13,6 +13,7 @@ extern Monitor *cur_mon;
>  #define MONITOR_USE_READLINE  0x02
>  #define MONITOR_USE_CONTROL   0x04
>  #define MONITOR_USE_PRETTY    0x08
> +#define MONITOR_USE_OOB       0x10
>
>  bool monitor_cur_is_qmp(void);
>
> diff --git a/monitor.c b/monitor.c
> index eba98df9da..d77ccc8785 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -36,6 +36,7 @@
>  #include "net/slirp.h"
>  #include "chardev/char-fe.h"
>  #include "chardev/char-io.h"
> +#include "chardev/char-mux.h"
>  #include "ui/qemu-spice.h"
>  #include "sysemu/numa.h"
>  #include "monitor/monitor.h"
> @@ -4568,12 +4569,26 @@ static void monitor_qmp_setup_handlers_bh(void 
> *opaque)
>  void monitor_init(Chardev *chr, int flags)
>  {
>      Monitor *mon = g_malloc(sizeof(*mon));
> +    bool use_readline = flags & MONITOR_USE_READLINE;
> +    bool use_oob = flags & MONITOR_USE_OOB;
> +
> +    if (use_oob) {
> +        if (CHARDEV_IS_MUX(chr)) {
> +            error_report("Monitor Out-Of-Band is not supported with "
> +                         "MUX typed chardev backend");
> +            exit(1);
> +        }
> +        if (use_readline) {
> +            error_report("Monitor Out-Of-band is only supported by QMP");
> +            exit(1);
> +        }
> +    }

I would rather see the error reporting / exit in vl.c:mon_init_func()
function, to have a single place for exit()

>
> -    monitor_data_init(mon, false, false);
> +    monitor_data_init(mon, false, use_oob);
>
>      qemu_chr_fe_init(&mon->chr, chr, &error_abort);
>      mon->flags = flags;
> -    if (flags & MONITOR_USE_READLINE) {
> +    if (use_readline) {
>          mon->rs = readline_init(monitor_readline_printf,
>                                  monitor_readline_flush,
>                                  mon,
> @@ -4669,6 +4684,9 @@ QemuOptsList qemu_mon_opts = {
>          },{
>              .name = "pretty",
>              .type = QEMU_OPT_BOOL,
> +        },{
> +            .name = "x-oob",
> +            .type = QEMU_OPT_BOOL,
>          },
>          { /* end of list */ }
>      },
> diff --git a/vl.c b/vl.c
> index c81cc86607..5fd01bd5f6 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2404,6 +2404,11 @@ static int mon_init_func(void *opaque, QemuOpts *opts, 
> Error **errp)
>      if (qemu_opt_get_bool(opts, "pretty", 0))
>          flags |= MONITOR_USE_PRETTY;
>
> +    /* OOB is off by default */
> +    if (qemu_opt_get_bool(opts, "x-oob", 0)) {
> +        flags |= MONITOR_USE_OOB;
> +    }
> +
>      chardev = qemu_opt_get(opts, "chardev");
>      chr = qemu_chr_find(chardev);
>      if (chr == NULL) {
> --
> 2.14.3
>

Other than that, it looks fine.
Reviewed-by: Marc-André Lureau <address@hidden>