[for-2.12,3/8] monitor: new parameter "x-oob"

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 <peterx@redhat.com>
---
 include/monitor/monitor.h |  1 +
 monitor.c                 | 22 ++++++++++++++++++++--
 vl.c                      |  5 +++++
 3 files changed, 26 insertions(+), 2 deletions(-)

Hi

On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> 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 <peterx@redhat.com>
> ---
>  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 <marcandre.lureau@redhat.com>

On Mon, Mar 26, 2018 at 11:10:30AM +0200, Marc-André Lureau wrote:

[...]

> > @@ -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()

But there can be other callers for monitor_init() so I would assume
checking it here would be safer (though for now indeed mon_init_func()
is the only one that can pass OOB flag in).

[...]

> Other than that, it looks fine.
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Thanks for your quick reviews!

On 03/26/2018 04:10 AM, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Mar 26, 2018 at 8:38 AM, Peter Xu <peterx@redhat.com> 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.

Worth documenting in the commit message at least that even when OOB is 
enabled, the client must STILL opt-in to using it by replying correctly 
to qmp_capabilities, as well as mention that in the future, we may 
remove x-oob and rely on JUST qmp_capabilities for enabling OOB.


>> @@ -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);

Should these two checks be swapped?  Otherwise, if you use a MUX-typed 
chardev for HMP, the message implies that switching chardev backend 
might make it work, even though if you actually do that you'd then get 
the failure for not being QMP.

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

To do that, monitor_init() should change signatures to take Error 
**errp.  Probably worth doing if you spin a v2 of this series (adding 
the parameter can be done as a separate patch, although there are only 5 
callers in the tree so adjusting the callers at the same time is 
probably not that hard to review).

> 
> Other than that, it looks fine.
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Okay, I'll see how my review goes on the rest of the series before 
deciding whether to request a v2.