[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC] monitor: turn on Out-Of-Band by default again
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [RFC] monitor: turn on Out-Of-Band by default again |
Date: |
Mon, 21 May 2018 09:13:06 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 |
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