Re: [Qemu-devel] [RFC] monitor: turn on Out-Of-Band by default again

[Eric Blake]

On 05/21/2018 03:42 AM, Peter Xu wrote:
> We turned Out-Of-Band feature of monitors off for 2.12 release.  Now we
> try to turn that on again.

"try to turn" sounds weak, like you aren't sure of this patch.  If you 
aren't sure, then why should we feel safe in applying it?  This text is 
going in the permanent git history, so sound bold, rather than hesitant!

"We have resolved the issues from last time (commit 3fd2457d reverted by 
commit a4f90923):
- issue 1 ...
- issue 2 ...
So now we are ready to enable advertisement of the feature by default"

with better descriptions of the issues that you fixed (I can think of at 
least the fixes adding thread-safety to the current monitor, and fixing 
early use of the monitor before qmp_capabilities completes; there may 
also be other issues that you want to call out).

> Signed-off-by: Peter Xu <address@hidden>
> --
> Now OOB should be okay with all known tests (except iotest qcow2, since
> it is still broken on master),

Which tests are still failing for you?  Ideally, you can still 
demonstrate that the tests not failing without this patch continue to 
pass with this patch, even if you call out the tests that have known 
issues to still be resolved.

> and AFAIK now we should also be okay with
> ARM+Libvirt (not testsed, but Eric Auger helped to verify that before
> the release).  So I think it's now safe to turn OOB on again.  Please
> feel free to test this against any of existing testsuites to see whether
> it'll still break any stuff.  Thanks,
>
> Signed-off-by: Peter Xu <address@hidden>
> ---
>   monitor.c        | 13 +++----------
>   tests/qmp-test.c |  2 +-
>   vl.c             |  9 ++++-----
>   3 files changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 46814af533..ce5cc5e34e 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4560,16 +4560,9 @@ void monitor_init(Chardev *chr, int flags)
>       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);
> -        }
> +    if (CHARDEV_IS_MUX(chr)) {
> +        /* MUX is still not supported for Out-Of-Band */
> +        use_oob = false;

This isn't a mere reinstatement of 3fd2457d, but is now advertising OOB 
when using readline (which presumably is a synonym for using HMP).  Is 
that intentional?  If so, the commit message should mention it.

>       }
>   
>       monitor_data_init(mon, false, use_oob);
> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
> index 88f867f8c0..c85a3964d9 100644
> --- a/tests/qmp-test.c
> +++ b/tests/qmp-test.c
> @@ -89,7 +89,7 @@ static void test_qmp_protocol(void)
>       g_assert(q);
>       test_version(qdict_get(q, "version"));
>       capabilities = qdict_get_qlist(q, "capabilities");
> -    g_assert(capabilities && qlist_empty(capabilities));
> +    g_assert(capabilities);
>       qobject_unref(resp);
>   
>       /* Test valid command before handshake */
> diff --git a/vl.c b/vl.c
> index 3b39bbd7a8..b71fb8eb25 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2394,6 +2394,10 @@ static int mon_init_func(void *opaque, QemuOpts *opts, 
> Error **errp)
>           flags = MONITOR_USE_READLINE;
>       } else if (strcmp(mode, "control") == 0) {
>           flags = MONITOR_USE_CONTROL;
> +        /* Out-Of-Band is on by default */
> +        if (qemu_opt_get_bool(opts, "x-oob", 1)) {
> +            flags |= MONITOR_USE_OOB;
> +        }

Do we really still need the x-oob property, vs. outright deletion of 
this bandaid?  Then again, I guess keeping it for one more release makes 
it easier to forcefully turn things off for temporary testing when 
isolating whether OOB is a culprit in something breaking.

>       } else {
>           error_report("unknown monitor mode \"%s\"", mode);
>           exit(1);
> @@ -2402,11 +2406,6 @@ 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) {

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org