[Qemu-devel] [PATCH for-2.12 0/4] Turn OOB off for 2.12-rc1, revert OOB tests

[Qemu-devel] [PATCH for-2.12 0/4] Turn OOB off for 2.12-rc1, revert OOB tests

[Peter Xu]

Temporarily disable OOB for 2.12 since it break (a lot of) things.
Luckily it's only a switch so not so hard. Meanwhile, revert all new
tests that checks against it.

Tests done: all target builds and "make check" passed, but only on
x86_64 host.  It can pass all "raw" iotests, but some "qcow2" iotests
are still failing.  I can even reproduce many of the qcow2 fails even
before the whole OOB series, so I suppose that's something already
exists so I'll ignore.

More tests are really welcomed.  Thanks,

Peter

Peter Xu (4):
  Revert "monitor: enable IO thread for (qmp & !mux) typed"
  Revert "tests: qmp-test: add oob test"
  Revert "tests: qmp-test: verify command batching"
  tests: partly revert "qmp: introduce QMPCapability"

 monitor.c        |  5 +--
 tests/qmp-test.c | 97 +-------------------------------------------------------
 2 files changed, 2 insertions(+), 100 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH for-2.12 1/4] Revert "monitor: enable IO thread for (qmp & !mux) typed"

[Peter Xu]

This reverts commit 3fd2457d18edf5736f713dfe1ada9c87a9badab1.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index 6ccd2fc089..77f4c41cfa 100644
--- a/monitor.c
+++ b/monitor.c
@@ -36,7 +36,6 @@
 #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"
@@ -4537,10 +4536,8 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
 void monitor_init(Chardev *chr, int flags)
 {
     Monitor *mon = g_malloc(sizeof(*mon));
-    /* Enable IOThread for QMPs that are not using MUX chardev backends. */
-    bool use_io_thr = (!CHARDEV_IS_MUX(chr)) && (flags & MONITOR_USE_CONTROL);
 
-    monitor_data_init(mon, false, use_io_thr);
+    monitor_data_init(mon, false, false);
 
     qemu_chr_fe_init(&mon->chr, chr, &error_abort);
     mon->flags = flags;
-- 
2.14.3

Re: [Qemu-devel] [PATCH for-2.12 1/4] Revert "monitor: enable IO thread for (qmp & !mux) typed"

[Eric Blake]

On 03/23/2018 09:08 AM, Peter Xu wrote:
> This reverts commit 3fd2457d18edf5736f713dfe1ada9c87a9badab1.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Not fatal to taking this patch as-is, but it's always nice to include a 
rationale when reverting, something along the lines of:

It turns out that enabling OOB caused several iotests to break; 
disabling OOB is the fastest and safest approach for reducing the risk 
of a broken release, while working on the fixes for the problems uncovered.

(Probably similar comments will be needed on the remaining patches...)

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

Re: [Qemu-devel] [PATCH for-2.12 1/4] Revert "monitor: enable IO thread for (qmp & !mux) typed"

[Peter Xu]

On Fri, Mar 23, 2018 at 10:49:31AM -0500, Eric Blake wrote:
> On 03/23/2018 09:08 AM, Peter Xu wrote:
> > This reverts commit 3fd2457d18edf5736f713dfe1ada9c87a9badab1.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Not fatal to taking this patch as-is, but it's always nice to include a
> rationale when reverting, something along the lines of:
> 
> It turns out that enabling OOB caused several iotests to break; disabling
> OOB is the fastest and safest approach for reducing the risk of a broken
> release, while working on the fixes for the problems uncovered.
> 
> (Probably similar comments will be needed on the remaining patches...)

Yes.  I saw the pull, thanks for adding those info into commit messages.

-- 
Peter Xu

[Qemu-devel] [PATCH for-2.12 2/4] Revert "tests: qmp-test: add oob test"

[Peter Xu]

This reverts commit d003f7a8f9cafe50119975844fa01afc2baf41fb.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/qmp-test.c | 65 --------------------------------------------------------
 1 file changed, 65 deletions(-)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 07c0b87e27..2e4b599a4c 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -164,70 +164,6 @@ static void test_qmp_protocol(void)
     qtest_quit(qts);
 }
 
-/* Tests for Out-Of-Band support. */
-static void test_qmp_oob(void)
-{
-    QDict *resp;
-    int acks = 0;
-    const char *cmd_id;
-
-    global_qtest = qtest_init_without_qmp_handshake(common_args);
-
-    /* Ignore the greeting message. */
-    resp = qmp_receive();
-    g_assert(qdict_get_qdict(resp, "QMP"));
-    QDECREF(resp);
-
-    /* Try a fake capability, it should fail. */
-    resp = qmp("{ 'execute': 'qmp_capabilities', "
-               "  'arguments': { 'enable': [ 'cap-does-not-exist' ] } }");
-    g_assert(qdict_haskey(resp, "error"));
-    QDECREF(resp);
-
-    /* Now, enable OOB in current QMP session, it should succeed. */
-    resp = qmp("{ 'execute': 'qmp_capabilities', "
-               "  'arguments': { 'enable': [ 'oob' ] } }");
-    g_assert(qdict_haskey(resp, "return"));
-    QDECREF(resp);
-
-    /*
-     * Try any command that does not support OOB but with OOB flag. We
-     * should get failure.
-     */
-    resp = qmp("{ 'execute': 'query-cpus',"
-               "  'control': { 'run-oob': true } }");
-    g_assert(qdict_haskey(resp, "error"));
-    QDECREF(resp);
-
-    /*
-     * First send the "x-oob-test" command with lock=true and
-     * oob=false, it should hang the dispatcher and main thread;
-     * later, we send another lock=false with oob=true to continue
-     * that thread processing.  Finally we should receive replies from
-     * both commands.
-     */
-    qmp_async("{ 'execute': 'x-oob-test',"
-              "  'arguments': { 'lock': true }, "
-              "  'id': 'lock-cmd'}");
-    qmp_async("{ 'execute': 'x-oob-test', "
-              "  'arguments': { 'lock': false }, "
-              "  'control': { 'run-oob': true }, "
-              "  'id': 'unlock-cmd' }");
-
-    /* Ignore all events.  Wait for 2 acks */
-    while (acks < 2) {
-        resp = qmp_receive();
-        cmd_id = qdict_get_str(resp, "id");
-        if (!g_strcmp0(cmd_id, "lock-cmd") ||
-            !g_strcmp0(cmd_id, "unlock-cmd")) {
-            acks++;
-        }
-        QDECREF(resp);
-    }
-
-    qtest_end();
-}
-
 static int query_error_class(const char *cmd)
 {
     static struct {
@@ -412,7 +348,6 @@ int main(int argc, char *argv[])
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_func("qmp/protocol", test_qmp_protocol);
-    qtest_add_func("qmp/oob", test_qmp_oob);
     qmp_schema_init(&schema);
     add_query_tests(&schema);
 
-- 
2.14.3

[Qemu-devel] [PATCH for-2.12 3/4] Revert "tests: qmp-test: verify command batching"

[Peter Xu]

This reverts commit 91ad45061af0fe44ac5dadb5bedaf4d7a08077c8.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/qmp-test.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 2e4b599a4c..d1fa1cb217 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -82,7 +82,6 @@ static void test_qmp_protocol(void)
     QTestState *qts;
     const QListEntry *entry;
     QString *qstr;
-    int i;
 
     qts = qtest_init_without_qmp_handshake(common_args);
 
@@ -140,27 +139,6 @@ static void test_qmp_protocol(void)
     g_assert_cmpint(qdict_get_int(resp, "id"), ==, 2);
     QDECREF(resp);
 
-    /*
-     * Test command batching.  In current test OOB is not enabled, we
-     * should be able to run as many commands in batch as we like.
-     * Using 16 (>8, which is OOB queue length) to make sure OOB won't
-     * break existing clients.  Note: this test does not control the
-     * scheduling of QEMU's QMP command processing threads so it may
-     * not really trigger batching inside QEMU.  This is just a
-     * best-effort test.
-     */
-    for (i = 0; i < 16; i++) {
-        qtest_async_qmp(qts, "{ 'execute': 'query-version' }");
-    }
-    /* Verify the replies to make sure no command is dropped. */
-    for (i = 0; i < 16; i++) {
-        resp = qtest_qmp_receive(qts);
-        /* It should never be dropped.  Each of them should be a reply. */
-        g_assert(qdict_haskey(resp, "return"));
-        g_assert(!qdict_haskey(resp, "event"));
-        QDECREF(resp);
-    }
-
     qtest_quit(qts);
 }
 
-- 
2.14.3

[Qemu-devel] [PATCH for-2.12 4/4] tests: partly revert "qmp: introduce QMPCapability"

[Peter Xu]

Revert the tests in the patch 02130314d8 ("qmp: introduce
QMPCapability", 2018-03-19) since we'll disable OOB for now.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/qmp-test.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index d1fa1cb217..558e83540c 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -80,8 +80,6 @@ static void test_qmp_protocol(void)
     QDict *resp, *q, *ret;
     QList *capabilities;
     QTestState *qts;
-    const QListEntry *entry;
-    QString *qstr;
 
     qts = qtest_init_without_qmp_handshake(common_args);
 
@@ -91,13 +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);
-    entry = qlist_first(capabilities);
-    g_assert(entry);
-    qstr = qobject_to(QString, entry->value);
-    g_assert(qstr);
-    g_assert_cmpstr(qstring_get_str(qstr), ==, "oob");
-    QDECREF(resp);
+    g_assert(capabilities && qlist_empty(capabilities));
 
     /* Test valid command before handshake */
     resp = qtest_qmp(qts, "{ 'execute': 'query-version' }");
-- 
2.14.3

Re: [Qemu-devel] [PATCH for-2.12 0/4] Turn OOB off for 2.12-rc1, revert OOB tests

[Peter Xu]

On Fri, Mar 23, 2018 at 10:08:17PM +0800, Peter Xu wrote:
> Temporarily disable OOB for 2.12 since it break (a lot of) things.
> Luckily it's only a switch so not so hard. Meanwhile, revert all new
> tests that checks against it.
> 
> Tests done: all target builds and "make check" passed, but only on
> x86_64 host.  It can pass all "raw" iotests, but some "qcow2" iotests
> are still failing.  I can even reproduce many of the qcow2 fails even
> before the whole OOB series, so I suppose that's something already
> exists so I'll ignore.
> 
> More tests are really welcomed.  Thanks,

CC Eric Blake too.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH for-2.12 0/4] Turn OOB off for 2.12-rc1, revert OOB tests

[Christian Borntraeger]

On 03/23/2018 03:15 PM, Peter Xu wrote:
> On Fri, Mar 23, 2018 at 10:08:17PM +0800, Peter Xu wrote:
>> Temporarily disable OOB for 2.12 since it break (a lot of) things.
>> Luckily it's only a switch so not so hard. Meanwhile, revert all new
>> tests that checks against it.
>>
>> Tests done: all target builds and "make check" passed, but only on
>> x86_64 host.  It can pass all "raw" iotests, but some "qcow2" iotests
>> are still failing.  I can even reproduce many of the qcow2 fails even
>> before the whole OOB series, so I suppose that's something already
>> exists so I'll ignore.
>>
>> More tests are really welcomed.  Thanks,

Series
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>


> 
> CC Eric Blake too.
> 

Re: [Qemu-devel] [PATCH for-2.12 0/4] Turn OOB off for 2.12-rc1, revert OOB tests

[Eric Blake]

On 03/23/2018 09:08 AM, Peter Xu wrote:
> Temporarily disable OOB for 2.12 since it break (a lot of) things.
> Luckily it's only a switch so not so hard. Meanwhile, revert all new
> tests that checks against it.
> 
> Tests done: all target builds and "make check" passed, but only on
> x86_64 host.  It can pass all "raw" iotests, but some "qcow2" iotests
> are still failing.  I can even reproduce many of the qcow2 fails even
> before the whole OOB series, so I suppose that's something already
> exists so I'll ignore.
> 
> More tests are really welcomed.  Thanks,
> 
> Peter

Acked-by: Eric Blake <eblake@redhat.com>

if Peter Maydell wants to pick this up directly on mainline (the smaller 
the window with broken tests, the easier it is to bisect other issues). 
Otherwise, I'll queue it in my QAPI pull request for Monday.

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

Re: [Qemu-devel] [PATCH for-2.12 0/4] Turn OOB off for 2.12-rc1, revert OOB tests

[Eric Blake]

On 03/23/2018 10:42 AM, Eric Blake wrote:
> On 03/23/2018 09:08 AM, Peter Xu wrote:
>> Temporarily disable OOB for 2.12 since it break (a lot of) things.
>> Luckily it's only a switch so not so hard. Meanwhile, revert all new
>> tests that checks against it.
>>
>> Tests done: all target builds and "make check" passed, but only on
>> x86_64 host.  It can pass all "raw" iotests, but some "qcow2" iotests
>> are still failing.  I can even reproduce many of the qcow2 fails even
>> before the whole OOB series, so I suppose that's something already
>> exists so I'll ignore.
>>
>> More tests are really welcomed.  Thanks,
>>
>> Peter
> 
> Acked-by: Eric Blake <eblake@redhat.com>
> 
> if Peter Maydell wants to pick this up directly on mainline (the smaller 
> the window with broken tests, the easier it is to bisect other issues). 
> Otherwise, I'll queue it in my QAPI pull request for Monday.
> 

Actually, we should revert things in reverse order of the original 
commits, so that we aren't introducing yet more temporary breakage.

Since you reverted:

$ git describe 3fd2457 d003f7a 91ad450 0213031 --match=v\*
v2.11.0-2595-g3fd2457d18e
v2.11.0-2598-gd003f7a8f9c
v2.11.0-2597-g91ad45061af
v2.11.0-2585-g02130314d8c

where the higher the middle number represents the further distance from 
2.11 (aka newer patch), this series should be applied in the order:

2/4 (revert the newest patch first)
3/4
1/4
4/4

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

Re: [Qemu-devel] [PATCH for-2.12 0/4] Turn OOB off for 2.12-rc1, revert OOB tests

[Eric Blake]

On 03/23/2018 10:53 AM, Eric Blake wrote:

> Actually, we should revert things in reverse order of the original 
> commits, so that we aren't introducing yet more temporary breakage.
> 
> Since you reverted:
> 
> $ git describe 3fd2457 d003f7a 91ad450 0213031 --match=v\*
> v2.11.0-2595-g3fd2457d18e
> v2.11.0-2598-gd003f7a8f9c
> v2.11.0-2597-g91ad45061af
> v2.11.0-2585-g02130314d8c
> 
> where the higher the middle number represents the further distance from 
> 2.11 (aka newer patch), this series should be applied in the order:
> 
> 2/4 (revert the newest patch first)
> 3/4
> 1/4
> 4/4

Even that didn't work - 'make check' fails with either 1/4 or 4/4 
applied in isolation, so I'm squashing them into a single patch.

The failure was either:

   GTESTER check-qtest-x86_64
**
ERROR:tests/qmp-test.c:96:test_qmp_protocol: assertion failed: (entry)
GTester: last random seed: R02Sa02aeb0d46d3092524914cd9be2b57f7
make: *** [/home/eblake/qemu/tests/Makefile.include:880: 
check-qtest-x86_64] Error 1


or:

   GTESTER check-qtest-x86_64
**
ERROR:tests/qmp-test.c:92:test_qmp_protocol: assertion failed: 
(capabilities && qlist_empty(capabilities))
GTester: last random seed: R02S621a9dbfcf7bed132c614953e93ecb37
make: *** [/home/eblake/qemu/tests/Makefile.include:880: 
check-qtest-x86_64] Error 1


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

Re: [Qemu-devel] [PATCH for-2.12 0/4] Turn OOB off for 2.12-rc1, revert OOB tests

[Christian Borntraeger]

On 03/23/2018 06:17 PM, Eric Blake wrote:
> On 03/23/2018 10:53 AM, Eric Blake wrote:
> 
>> Actually, we should revert things in reverse order of the original commits, so that we aren't introducing yet more temporary breakage.
>>
>> Since you reverted:
>>
>> $ git describe 3fd2457 d003f7a 91ad450 0213031 --match=v\*
>> v2.11.0-2595-g3fd2457d18e
>> v2.11.0-2598-gd003f7a8f9c
>> v2.11.0-2597-g91ad45061af
>> v2.11.0-2585-g02130314d8c
>>
>> where the higher the middle number represents the further distance from 2.11 (aka newer patch), this series should be applied in the order:
>>
>> 2/4 (revert the newest patch first)
>> 3/4
>> 1/4
>> 4/4
> 
> Even that didn't work - 'make check' fails with either 1/4 or 4/4 applied in isolation, so I'm squashing them into a single patch.

Yes, "qmp: introduce QMPCapability" added an assert, that was fixed by a later on patch. So
the original series was not bisectable. Anyway with Peters 4 patches all applied things are fine
for me.

Re: [Qemu-devel] [PATCH for-2.12 0/4] Turn OOB off for 2.12-rc1, revert OOB tests

[Eric Blake]

On 03/23/2018 01:30 PM, Christian Borntraeger wrote:

>> Even that didn't work - 'make check' fails with either 1/4 or 4/4 applied in isolation, so I'm squashing them into a single patch.
> 
> Yes, "qmp: introduce QMPCapability" added an assert, that was fixed by a later on patch. So
> the original series was not bisectable. Anyway with Peters 4 patches all applied things are fine
> for me.

Weird, since I seem to recall running 'make check' on every patch in 
order in that series without seeing failures, prior to preparing the 
pull request (where I fell short was running iotests, which is what 
sparked this whole revert thread).  Anyways, even if my memory is faulty 
and the original series has a broken bisection window, it's now water 
under the bridge; the focus at this point is improving the known bugs to 
see if we can re-enable OOB prior to -rc2.

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

Re: [Qemu-devel] [PATCH for-2.12 0/4] Turn OOB off for 2.12-rc1, revert OOB tests

[Peter Xu]

On Fri, Mar 23, 2018 at 01:36:28PM -0500, Eric Blake wrote:
> On 03/23/2018 01:30 PM, Christian Borntraeger wrote:
> 
> > > Even that didn't work - 'make check' fails with either 1/4 or 4/4 applied in isolation, so I'm squashing them into a single patch.
> > 
> > Yes, "qmp: introduce QMPCapability" added an assert, that was fixed by a later on patch. So
> > the original series was not bisectable. Anyway with Peters 4 patches all applied things are fine
> > for me.
> 
> Weird, since I seem to recall running 'make check' on every patch in order
> in that series without seeing failures, prior to preparing the pull request
> (where I fell short was running iotests, which is what sparked this whole
> revert thread).  Anyways, even if my memory is faulty and the original
> series has a broken bisection window, it's now water under the bridge; the
> focus at this point is improving the known bugs to see if we can re-enable
> OOB prior to -rc2.

I think each of the commit in original series should pass "make check"
before.  But for this revert series, it can't.  I'll take this into
consideration next time even reverting patches.

Regarding to the revert ordering itself: IMHO as long as we do the
first patch last (put "Revert monitor: enable IO thread for (qmp &
!mux) typed" at the end), we should be able to pass "make check" too.
So the correct order should be:

patch 2-4 (in any order, since each of them is only a test case)
patch 1

And, since at it, I just noticed that we could even keep one of the
test ("tests: qmp-test: verify command batching"), but anyway it won't
hurt if we revert that too now.

-- 
Peter Xu