On 05/21/2018 10:13 AM, Eric Blake wrote: > 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 <pet...@redhat.com> >> -- >> 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. >
Probably 91 and 169. If any others fail that's news to me. >> 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 <pet...@redhat.com> >> --- >> 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) { >> >