[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: |
John Snow |
Subject: |
Re: [Qemu-devel] [RFC] monitor: turn on Out-Of-Band by default again |
Date: |
Tue, 22 May 2018 14:40:26 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 |
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 <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.
>
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 <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) {
>>
>