monitor: enable OOB by default

The old names are confusing since both of the old functions are poping

an item from multiple queues rather than a single queue. In that sense, *_pop_any() suites better than *_pop_one().

Since at it, touch up the function monitor_qmp_response_pop_any() a bit to let the callers pass in a QMPResponse struct instead of returning a struct. Change the return value to boolean to mark whether we have poped a valid response instead.

Suggested-by Markus Armbruster <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
  monitor.c | 20 +++++++++-----------
  1 file changed, 9 insertions(+), 11 deletions(-)

The old names are confusing since both of the old functions are poping an item from multiple queues rather than a single queue. In that sense, *_pop_any() suites better than *_pop_one().

Since at it, touch up the function monitor_qmp_response_pop_any() a bit to let the callers pass in a QMPResponse struct instead of returning a struct. Change the return value to boolean to mark whether we have poped a valid response instead.

Suggested-by Markus Armbruster <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

monitor.c +0 -0

index 6d0cec552e..d4a463f707 100644
@@ -513,10 +513,10 @@ struct QMPResponse {
 typedef struct QMPResponse QMPResponse;
 
 /*

Fixes 6d2d563f8c ("qmp: cleanup qmp queues properly", 2018-03-27)

Suggested-by Markus Armbruster <[email protected]>

Signed-off-by Peter Xu <[email protected]>

Signed-off-by Peter Xu <[email protected]>

On Tue, Jun 19, 2018 at 01:34:22PM +0800, Peter Xu wrote:

[...]

Quoted 4 lines

Fixes 6d2d563f8c ("qmp: cleanup qmp queues properly", 2018-03-27)

Suggested-by Markus Armbruster <[email protected]>

Signed-off-by Peter Xu <[email protected]>

Quoted 1 line

Signed-off-by Peter Xu <[email protected]>

I am pretty sure this time that this 2nd line is not there in my local tree. :)

I think it's a git-format-patch bug, otherwise I must have misused it for a long time. Instead of figuring this out and repost again, I'll see how far the rest of the series can go.

Quoted 13 lines · Peter Xu

On Tue, Jun 19, 2018 at 01:34:22PM +0800, Peter Xu wrote:

[...]

Quoted 5 lines

Fixes 6d2d563f8c ("qmp: cleanup qmp queues properly", 2018-03-27)

Suggested-by Markus Armbruster <[email protected]>

Signed-off-by Peter Xu <[email protected]>

Signed-off-by Peter Xu <[email protected]>

I am pretty sure this time that this 2nd line is not there in my local tree. :)

Quoted 3 lines

I think it's a git-format-patch bug, otherwise I must have misused it for a long time. Instead of figuring this out and repost again, I'll see how far the rest of the series can go.

Do you use git-format-patch -s, or have format.signOff set in .git/config or ~/.gitconfig?

Previously we clean up the queues when we got CLOSED event. It was used to make sure we won't send leftover replies/events of a old client to a new client which makes perfect sense. However this will also drop the replies/events even if the output port of the previous chardev backend is still open, which can lead to missing of the last replies/events. Now this patch does an extra operation to flush the response queue before cleaning up.

In most cases, a QMP session will be based on a bidirectional channel (a TCP port, for example, we read/write to the same socket handle), so in port and out port of the backend chardev are fundamentally the same port. In these cases, it does not really matter much on whether we'll flush the response queue since flushing will fail anyway. However there can be cases where in & out ports of the QMP monitor's backend chardev are separated. Here is an example:

cat $QMP_COMMANDS | qemu -qmp stdio ... | filter_commands

In this case, the backend is fd-typed, and it is connected to stdio where in port is stdin and out port is stdout. Now if we drop all the events on the response queue then filter_command process might miss some events that it might expect. The thing is that, when stdin closes, stdout might still be there alive!

In practice, I encountered SHUTDOWN event missing when running test with iotest 087 with Out-Of-Band enabled. Here is one of the ways that this can happen (after "quit" command is executed and QEMU quits the main loop):

1. [main thread] QEMU queues a SHUTDOWN event into response queue.

2. "cat" terminates (to distinguish it from the animal, I quote it).

3. [monitor iothread] QEMU's monitor iothread reads EOF from stdin.

4. [monitor iothread] QEMU's monitor iothread calls the CLOSED event hook for the monitor, which will destroy the response queue of the monitor, then the SHUTDOWN event is dropped.

5. [main thread] QEMU's main thread cleans up the monitors in monitor_cleanup(). When trying to flush pending responses, it sees nothing. SHUTDOWN is lost forever.

Note that before the monitor iothread was introduced, step [4]/[5] could never happen since the main loop was the only place to detect the EOF event of stdin and run the CLOSED event hooks. Now things can happen in parallel in the iothread.

Without this patch, iotest 087 will have ~10% chance to miss the SHUTDOWN event and fail when with Out-Of-Band enabled (the output is manually touched up to suite line width requirement):

home/peterx/git/qemu/bin/tests/qemu-iotests/087.out.bad renamed from home/peterx/git/qemu/tests/qemu-iotests/087.out +0 -2

@@ -8,7 +8,6 @@
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "'node-name' must be
  specified for the root node"}}
 {"return": {}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
- "event": "SHUTDOWN", "data": {"guest": false}}
 
 === Duplicate ID ===

This patch fixes the problem.

Fixes 6d2d563f8c ("qmp: cleanup qmp queues properly", 2018-03-27)

Suggested-by Markus Armbruster <[email protected]>

Signed-off-by Peter Xu <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

monitor.c +22 -3

index d4a463f707..c9a02ee40c 100644
@@ -512,6 +512,27 @@ struct QMPResponse {
 };
 typedef struct QMPResponse QMPResponse;
 
+static QObject *monitor_qmp_response_pop_one(Monitor *mon)
+{
+    QObject *data;
+
+    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    data = g_queue_pop_head(mon->qmp.qmp_responses);
+    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+
+    return data;
+}
+
+static void monitor_qmp_response_flush(Monitor *mon)
+{
+    QObject *data;
+
+    while ((data = monitor_qmp_response_pop_one(mon))) {
+        monitor_json_emitter_raw(mon, data);
+        qobject_unref(data);
+    }
+}
+
 /*
  * Pop a QMPResponse from any monitor's response queue into @response.
  * Return false if all the queues are empty; else true.

@@ -523,9 +544,7 @@ static bool monitor_qmp_response_pop_any(QMPResponse *response)
 
     qemu_mutex_lock(&monitor_lock);
     QTAILQ_FOREACH(mon, &mon_list, entry) {
-        qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
-        data = g_queue_pop_head(mon->qmp.qmp_responses);
-        qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+        data = monitor_qmp_response_pop_one(mon);
         if (data) {
             response->mon = mon;
             response->data = data;

monitor.c +1 -0

+        monitor_qmp_response_flush(mon);
         monitor_qmp_cleanup_queues(mon);
         json_message_parser_destroy(&mon->qmp.parser);
         json_message_parser_init(&mon->qmp.parser, handle_qmp_command);

Quoted 43 lines · Peter Xu

Previously we clean up the queues when we got CLOSED event. It was used to make sure we won't send leftover replies/events of a old client to a new client which makes perfect sense. However this will also drop the replies/events even if the output port of the previous chardev backend is still open, which can lead to missing of the last replies/events. Now this patch does an extra operation to flush the response queue before cleaning up.

In most cases, a QMP session will be based on a bidirectional channel (a TCP port, for example, we read/write to the same socket handle), so in port and out port of the backend chardev are fundamentally the same port. In these cases, it does not really matter much on whether we'll flush the response queue since flushing will fail anyway. However there can be cases where in & out ports of the QMP monitor's backend chardev are separated. Here is an example:

cat $QMP_COMMANDS | qemu -qmp stdio ... | filter_commands

In this case, the backend is fd-typed, and it is connected to stdio where in port is stdin and out port is stdout. Now if we drop all the events on the response queue then filter_command process might miss some events that it might expect. The thing is that, when stdin closes, stdout might still be there alive!

In practice, I encountered SHUTDOWN event missing when running test with iotest 087 with Out-Of-Band enabled. Here is one of the ways that this can happen (after "quit" command is executed and QEMU quits the main loop):

1. [main thread] QEMU queues a SHUTDOWN event into response queue.

2. "cat" terminates (to distinguish it from the animal, I quote it).

3. [monitor iothread] QEMU's monitor iothread reads EOF from stdin.

4. [monitor iothread] QEMU's monitor iothread calls the CLOSED event hook for the monitor, which will destroy the response queue of the monitor, then the SHUTDOWN event is dropped.

5. [main thread] QEMU's main thread cleans up the monitors in monitor_cleanup(). When trying to flush pending responses, it sees nothing. SHUTDOWN is lost forever.

Quoted 8 lines

Note that before the monitor iothread was introduced, step [4]/[5] could never happen since the main loop was the only place to detect the EOF event of stdin and run the CLOSED event hooks. Now things can happen in parallel in the iothread.

Without this patch, iotest 087 will have ~10% chance to miss the SHUTDOWN event and fail when with Out-Of-Band enabled (the output is manually touched up to suite line width requirement):

I wouldn't wrap lines when quoting a diff.

Quoted 17 lines

home/peterx/git/qemu/bin/tests/qemu-iotests/087.out.bad renamed from home/peterx/git/qemu/tests/qemu-iotests/087.out +0 -2

@@ -8,7 +8,6 @@
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "'node-name' must be
  specified for the root node"}}
 {"return": {}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
- "event": "SHUTDOWN", "data": {"guest": false}}
 
 === Duplicate ID ===

@@ -53,7 +52,6 @@
 {"return": {}}
 {"return": {}}
 {"return": {}}
 

home/peterx/git/qemu/bin/tests/qemu-iotests/087.out.bad line 13

Quoted 2 lines

home/peterx/git/qemu/bin/tests/qemu-iotests/087.out.bad renamed from home/peterx/git/qemu/tests/qemu-iotests/087.out +0 -2

-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
- "event": "SHUTDOWN", "data": {"guest": false}}

Please indent the quoted diff a bit, so make it more obviously not part of the patch. In fact, git-am chokes on it for me.

Quoted 62 lines

This patch fixes the problem.

Fixes 6d2d563f8c ("qmp: cleanup qmp queues properly", 2018-03-27)

Suggested-by Markus Armbruster <[email protected]>

Signed-off-by Peter Xu <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

monitor.c +24 -3

index d4a463f707..c9a02ee40c 100644
@@ -512,6 +512,27 @@ struct QMPResponse {
 };
 typedef struct QMPResponse QMPResponse;
 
+static QObject *monitor_qmp_response_pop_one(Monitor *mon)
+{
+    QObject *data;
+
+    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    data = g_queue_pop_head(mon->qmp.qmp_responses);
+    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+
+    return data;
+}
+
+static void monitor_qmp_response_flush(Monitor *mon)
+{
+    QObject *data;
+
+    while ((data = monitor_qmp_response_pop_one(mon))) {
+        monitor_json_emitter_raw(mon, data);
+        qobject_unref(data);
+    }
+}
+
 /*
  * Pop a QMPResponse from any monitor's response queue into @response.
  * Return false if all the queues are empty; else true.

@@ -523,9 +544,7 @@ static bool monitor_qmp_response_pop_any(QMPResponse *response)
 
     qemu_mutex_lock(&monitor_lock);
     QTAILQ_FOREACH(mon, &mon_list, entry) {
-        qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
-        data = g_queue_pop_head(mon->qmp.qmp_responses);
-        qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+        data = monitor_qmp_response_pop_one(mon);
         if (data) {
             response->mon = mon;
             response->data = data;

@@ -4364,6 +4383,14 @@ static void monitor_qmp_event(void *opaque, int event)
         mon_refcount++;
         break;
     case CHR_EVENT_CLOSED:
+        /*
+         * Note: this is only useful when the output of the chardev

monitor.c static void monitor_qmp_event(void *opaque, int event) · line 4392

Quoted 5 lines

monitor.c +5 -0

+         * backend is still open.  For example, when the backend is
+         * stdio, it's possible that stdout is still open when stdin
+         * is closed.  After all, CHR_EVENT_CLOSED event is currently
+         * only bound to stdin.
+         */

I'm not sure I get the last sentence. What do you mean by "bound to stdin"?

monitor.c static void monitor_qmp_event(void *opaque, int event) · line 4396

Quoted 5 lines

monitor.c +1 -0

+        monitor_qmp_response_flush(mon);
         monitor_qmp_cleanup_queues(mon);
         json_message_parser_destroy(&mon->qmp.parser);
         json_message_parser_init(&mon->qmp.parser, handle_qmp_command);

Quoted 73 lines · Markus Armbruster

Quoted 43 lines · Peter Xu

Previously we clean up the queues when we got CLOSED event. It was used to make sure we won't send leftover replies/events of a old client to a new client which makes perfect sense. However this will also drop the replies/events even if the output port of the previous chardev backend is still open, which can lead to missing of the last replies/events. Now this patch does an extra operation to flush the response queue before cleaning up.

In most cases, a QMP session will be based on a bidirectional channel (a TCP port, for example, we read/write to the same socket handle), so in port and out port of the backend chardev are fundamentally the same port. In these cases, it does not really matter much on whether we'll flush the response queue since flushing will fail anyway. However there can be cases where in & out ports of the QMP monitor's backend chardev are separated. Here is an example:

cat $QMP_COMMANDS | qemu -qmp stdio ... | filter_commands

In this case, the backend is fd-typed, and it is connected to stdio where in port is stdin and out port is stdout. Now if we drop all the events on the response queue then filter_command process might miss some events that it might expect. The thing is that, when stdin closes, stdout might still be there alive!

In practice, I encountered SHUTDOWN event missing when running test with iotest 087 with Out-Of-Band enabled. Here is one of the ways that this can happen (after "quit" command is executed and QEMU quits the main loop):

1. [main thread] QEMU queues a SHUTDOWN event into response queue.

2. "cat" terminates (to distinguish it from the animal, I quote it).

3. [monitor iothread] QEMU's monitor iothread reads EOF from stdin.

4. [monitor iothread] QEMU's monitor iothread calls the CLOSED event hook for the monitor, which will destroy the response queue of the monitor, then the SHUTDOWN event is dropped.

5. [main thread] QEMU's main thread cleans up the monitors in monitor_cleanup(). When trying to flush pending responses, it sees nothing. SHUTDOWN is lost forever.

Quoted 8 lines

Note that before the monitor iothread was introduced, step [4]/[5] could never happen since the main loop was the only place to detect the EOF event of stdin and run the CLOSED event hooks. Now things can happen in parallel in the iothread.

Without this patch, iotest 087 will have ~10% chance to miss the SHUTDOWN event and fail when with Out-Of-Band enabled (the output is manually touched up to suite line width requirement):

I wouldn't wrap lines when quoting a diff.

Quoted 17 lines

home/peterx/git/qemu/bin/tests/qemu-iotests/087.out.bad renamed from home/peterx/git/qemu/tests/qemu-iotests/087.out +0 -2

@@ -8,7 +8,6 @@
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "'node-name' must be
  specified for the root node"}}
 {"return": {}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
- "event": "SHUTDOWN", "data": {"guest": false}}
 
 === Duplicate ID ===

@@ -53,7 +52,6 @@
 {"return": {}}
 {"return": {}}
 {"return": {}}
 

Quoted 5 lines

home/peterx/git/qemu/bin/tests/qemu-iotests/087.out.bad line 13

Quoted 2 lines

home/peterx/git/qemu/bin/tests/qemu-iotests/087.out.bad renamed from home/peterx/git/qemu/tests/qemu-iotests/087.out +0 -2

-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
- "event": "SHUTDOWN", "data": {"guest": false}}

Please indent the quoted diff a bit, so make it more obviously not part of the patch. In fact, git-am chokes on it for me.

To make it even simpler, I plan to remove the whole chunk of the diff from the commit message if you won't disagree.

Quoted 16 lines

Quoted 16 lines

This patch fixes the problem.

Fixes 6d2d563f8c ("qmp: cleanup qmp queues properly", 2018-03-27)

Suggested-by Markus Armbruster <[email protected]>

Signed-off-by Peter Xu <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

monitor.c +0 -0

index d4a463f707..c9a02ee40c 100644

Quoted 6 lines

Quoted 6 lines

monitor.c +2 -0

@@ -4364,6 +4383,14 @@ static void monitor_qmp_event(void *opaque, int event)
         mon_refcount++;
         break;
     case CHR_EVENT_CLOSED:
+        /*
+         * Note: this is only useful when the output of the chardev

Quoted 8 lines

monitor.c static void monitor_qmp_event(void *opaque, int event) · line 4392

Quoted 5 lines

monitor.c +5 -0

+         * backend is still open.  For example, when the backend is
+         * stdio, it's possible that stdout is still open when stdin
+         * is closed.  After all, CHR_EVENT_CLOSED event is currently
+         * only bound to stdin.
+         */

I'm not sure I get the last sentence. What do you mean by "bound to stdin"?

I want to express that the event is only related to the state of stdin, meanwhile it's never related to the state of stdout. How about I remove the sentence after "after all"? I think it's clear enough with the rest of the comments.

In my Out-Of-Band test, "check -qcow2 060" fail with this (the output is manually changed due to line width requirement):

060 5s ... - output mismatch (see 060.out.bad)

home/peterx/git/qemu/bin/tests/qemu-iotests/060.out.bad renamed from home/peterx/git/qemu/tests/qemu-iotests/060.out +3 -3

@@ -427,8 +427,8 @@
 QMP_VERSION
 {"return": {}}
 qcow2: Image is corrupt: L2 table offset 0x2a2a2a00 unaligned (L1
  index: 0); further non-fatal corruption events will be suppressed
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
- "event": "BLOCK_IMAGE_CORRUPTED", "data": {"device": "", "msg": "L2
- table offset 0x2a2a2a0
 0 unaligned (L1 index: 0)", "node-name": "drive", "fatal": false}}
 read failed: Input/output error
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
+ "event": "BLOCK_IMAGE_CORRUPTED", "data": {"device": "", "msg": "L2
+ table offset 0x2a2a2a0

0 unaligned (L1 index: 0)", "node-name": "drive", "fatal": false}}
{"return": ""}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
"event": "SHUTDOWN", "data": {"guest": false}}

The order of the event and the in/out error line is swapped. I didn't dig up the reason, but AFAIU what we want to verify is the event rather than stderr. Let's drop the stderr line directly for this test.

Signed-off-by Peter Xu <[email protected]>

Quoted 18 lines · Peter Xu

In my Out-Of-Band test, "check -qcow2 060" fail with this (the output is manually changed due to line width requirement):

060 5s ... - output mismatch (see 060.out.bad)

home/peterx/git/qemu/bin/tests/qemu-iotests/060.out.bad renamed from home/peterx/git/qemu/tests/qemu-iotests/060.out +2 -3

@@ -427,8 +427,8 @@
 QMP_VERSION
 {"return": {}}
 qcow2: Image is corrupt: L2 table offset 0x2a2a2a00 unaligned (L1
  index: 0); further non-fatal corruption events will be suppressed
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
- "event": "BLOCK_IMAGE_CORRUPTED", "data": {"device": "", "msg": "L2
- table offset 0x2a2a2a0
 0 unaligned (L1 index: 0)", "node-name": "drive", "fatal": false}}
 read failed: Input/output error
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
+ "event": "BLOCK_IMAGE_CORRUPTED", "data": {"device": "", "msg": "L2

Quoted 6 lines

+ table offset 0x2a2a2a0
0 unaligned (L1 index: 0)", "node-name": "drive", "fatal": false}}
{"return": ""}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
"event": "SHUTDOWN", "data": {"guest": false}}

Please indent this diff; I'd expect git-am to choke on it.

Quoted 7 lines

The order of the event and the in/out error line is swapped. I didn't dig up the reason, but AFAIU what we want to verify is the event rather than stderr. Let's drop the stderr line directly for this test.

Signed-off-by Peter Xu <[email protected]>

Quoted 20 lines · Markus Armbruster

Quoted 18 lines · Peter Xu

In my Out-Of-Band test, "check -qcow2 060" fail with this (the output is manually changed due to line width requirement):

060 5s ... - output mismatch (see 060.out.bad)

home/peterx/git/qemu/bin/tests/qemu-iotests/060.out.bad renamed from home/peterx/git/qemu/tests/qemu-iotests/060.out +2 -3

@@ -427,8 +427,8 @@
 QMP_VERSION
 {"return": {}}
 qcow2: Image is corrupt: L2 table offset 0x2a2a2a00 unaligned (L1
  index: 0); further non-fatal corruption events will be suppressed
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
- "event": "BLOCK_IMAGE_CORRUPTED", "data": {"device": "", "msg": "L2
- table offset 0x2a2a2a0
 0 unaligned (L1 index: 0)", "node-name": "drive", "fatal": false}}
 read failed: Input/output error
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
+ "event": "BLOCK_IMAGE_CORRUPTED", "data": {"device": "", "msg": "L2

Quoted 8 lines

Quoted 6 lines

+ table offset 0x2a2a2a0
0 unaligned (L1 index: 0)", "node-name": "drive", "fatal": false}}
{"return": ""}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
"event": "SHUTDOWN", "data": {"guest": false}}

Please indent this diff; I'd expect git-am to choke on it.

Do you mean something like pretty-JSON?

How about I remove this chunk too? What do you prefer?

Quoted 30 lines · Peter Xu

Quoted 20 lines · Markus Armbruster

Quoted 18 lines · Peter Xu

In my Out-Of-Band test, "check -qcow2 060" fail with this (the output is manually changed due to line width requirement):

060 5s ... - output mismatch (see 060.out.bad)

home/peterx/git/qemu/bin/tests/qemu-iotests/060.out.bad renamed from home/peterx/git/qemu/tests/qemu-iotests/060.out +2 -3

@@ -427,8 +427,8 @@
 QMP_VERSION
 {"return": {}}
 qcow2: Image is corrupt: L2 table offset 0x2a2a2a00 unaligned (L1
  index: 0); further non-fatal corruption events will be suppressed
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
- "event": "BLOCK_IMAGE_CORRUPTED", "data": {"device": "", "msg": "L2
- table offset 0x2a2a2a0
 0 unaligned (L1 index: 0)", "node-name": "drive", "fatal": false}}
 read failed: Input/output error
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
+ "event": "BLOCK_IMAGE_CORRUPTED", "data": {"device": "", "msg": "L2

Quoted 8 lines

Quoted 6 lines

+ table offset 0x2a2a2a0
0 unaligned (L1 index: 0)", "node-name": "drive", "fatal": false}}
{"return": ""}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
"event": "SHUTDOWN", "data": {"guest": false}}

Please indent this diff; I'd expect git-am to choke on it.

Quoted 1 line

Do you mean something like pretty-JSON?

Instead of

    In my Out-Of-Band test, "check -qcow2 060" fail with this (the output is
    manually changed due to line width requirement):
    
    060 5s ... - output mismatch (see 060.out.bad)
    --- /home/peterx/git/qemu/tests/qemu-iotests/060.out
    +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/060.out.bad
    @@ -427,8 +427,8 @@
     QMP_VERSION
     {"return": {}}

write

    In my Out-Of-Band test, "check -qcow2 060" fail with this (the output is
    manually changed due to line width requirement):
    
        060 5s ... - output mismatch (see 060.out.bad)
        --- /home/peterx/git/qemu/tests/qemu-iotests/060.out
        +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/060.out.bad
        @@ -427,8 +427,8 @@
         QMP_VERSION
         {"return": {}}

The former confuses git-am.

In other words, quote diff output verbatim, but indent your quote.

Quoted 1 line

How about I remove this chunk too? What do you prefer?

I rather like having the test failure details in the commit message.

Out-Of-Band handlers need to protect shared state if there is any. Mention it in the document. Meanwhile, touch up some other places too, either with better English, or reordering of bullets.

Suggested-by Markus Armbruster <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 docs/devel/qapi-code-gen.txt | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

docs/devel/qapi-code-gen.txt +3 -6

index 1366228b2a..fb486d4050 100644
@@ -664,22 +664,27 @@ command:
 
 - They are executed in order,
 - They run only in main thread of QEMU,
-- They have the BQL taken during execution.
+- They run with the BQL held.
 
 When a command is executed with OOB, the following changes occur:
 
 - They can be completed before a pending in-band command,
 - They run in a dedicated monitor thread,
-- They do not take the BQL during execution.
+- They run with the BQL not held.
 
 OOB command handlers must satisfy the following conditions:
 
-- It executes extremely fast,
-- It does not take any lock, or, it can take very small locks if all
-  critical regions also follow the rules for OOB command handler code,
+- It terminates quickly,
 - It does not invoke system calls that may block,
 - It does not access guest RAM that may block when userfaultfd is
-  enabled for postcopy live migration.

There was a regression reported by Eric Auger before with OOB:

http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html

It is fixed in 951702f39c ("monitor: bind dispatch bh to iohandler context", 2018-04-10).

For the bug, we turned Out-Of-Band feature of monitors off for 2.12 release. Now we turn that on again after the 2.12 release.

This patch partly reverts be933ffc23 (monitor: new parameter "x-oob"), meanwhile turn it on again by default for non-MUX QMPs. Note that we

can't enable Out-Of-Band for monitors with MUX-typed chardev backends, because not all the chardev frontends can run without main thread, or can run in multiple threads.

Some trivial touch-up in the test code is required to make sure qmp-test won't broke.

Signed-off-by Peter Xu <[email protected]>

---
 include/monitor/monitor.h |  1 -
 monitor.c                 | 17 +----------------
 tests/libqtest.c          |  2 +-
 tests/qmp-test.c          |  2 +-
 vl.c                      |  5 -----
 5 files changed, 3 insertions(+), 24 deletions(-)

include/monitor/monitor.h +0 -1

index d6ab70cae2..0cb0538a31 100644
@@ -13,7 +13,6 @@ extern Monitor *cur_mon;
 #define MONITOR_USE_READLINE  0x02
 #define MONITOR_USE_CONTROL   0x04
 #define MONITOR_USE_PRETTY    0x08
-#define MONITOR_USE_OOB       0x10
 
 bool monitor_cur_is_qmp(void);
 

monitor.c +0 -4

index c9a02ee40c..7fbcf84b02 100644
@@ -4587,19 +4587,7 @@ void monitor_init(Chardev *chr, int flags)
 {
     Monitor *mon = g_malloc(sizeof(*mon));
     bool use_readline = flags & MONITOR_USE_READLINE;
-    bool use_oob = flags & MONITOR_USE_OOB;
-
-    if (use_oob) {
-        if (CHARDEV_IS_MUX(chr)) {

monitor.c +0 -3

     monitor_data_init(mon, false, use_oob);
 

@@ -4701,9 +4689,6 @@ QemuOptsList qemu_mon_opts = {
         },{
             .name = "pretty",
             .type = QEMU_OPT_BOOL,
-        },{
-            .name = "x-oob",
-            .type = QEMU_OPT_BOOL,
         },
         { /* end of list */ }
     },

tests/libqtest.c +1 -1

index 098af6aec4..c5cb3f925c 100644
@@ -213,7 +213,7 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
                                   "-display none "
                                   "%s", qemu_binary, socket_path,
                                   getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null",
-                                  qmp_socket_path, use_oob ? ",x-oob=on" : "",
+                                  qmp_socket_path, "",
                                   extra_args ?: "");
         execlp("/bin/sh", "sh", "-c", command, NULL);
         exit(1);

tests/qmp-test.c +1 -1

index a49cbc6fde..3747bf7fbb 100644
@@ -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 */

vl.c +0 -0

index 6e34fb348d..26a0bb3f0f 100644
@@ -2307,11 +2307,6 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
     if (qemu_opt_get_bool(opts, "pretty", 0))

Quoted 9 lines · Peter Xu

There was a regression reported by Eric Auger before with OOB:

http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html

It is fixed in 951702f39c ("monitor: bind dispatch bh to iohandler context", 2018-04-10).

For the bug, we turned Out-Of-Band feature of monitors off for 2.12 release. Now we turn that on again after the 2.12 release.

Relating what happened in the order it happened could be easier to understand. Perhaps:

OOB commands were introduced in commit cf869d53172. Unfortunately, we ran into a regression, and had to disable them by default for 2.12 (commit be933ffc23).

The regression has since been fixed (commit 951702f39c7 "monitor: bind dispatch bh to iohandler context"). Time to re-enable OOB.

Quoted 3 lines

This patch partly reverts be933ffc23 (monitor: new parameter "x-oob"), meanwhile turn it on again by default for non-MUX QMPs. Note that we

"by default"?

Quoted 7 lines

can't enable Out-Of-Band for monitors with MUX-typed chardev backends, because not all the chardev frontends can run without main thread, or can run in multiple threads.

Some trivial touch-up in the test code is required to make sure qmp-test won't broke.

"won't break"

Quoted 40 lines

Signed-off-by Peter Xu <[email protected]>

---
 include/monitor/monitor.h |  1 -
 monitor.c                 | 17 +----------------
 tests/libqtest.c          |  2 +-
 tests/qmp-test.c          |  2 +-
 vl.c                      |  5 -----
 5 files changed, 3 insertions(+), 24 deletions(-)

include/monitor/monitor.h +0 -1

index d6ab70cae2..0cb0538a31 100644
@@ -13,7 +13,6 @@ extern Monitor *cur_mon;
 #define MONITOR_USE_READLINE  0x02
 #define MONITOR_USE_CONTROL   0x04
 #define MONITOR_USE_PRETTY    0x08
-#define MONITOR_USE_OOB       0x10
 
 bool monitor_cur_is_qmp(void);
 

monitor.c +0 -9

index c9a02ee40c..7fbcf84b02 100644
@@ -4587,19 +4587,7 @@ void monitor_init(Chardev *chr, int flags)
 {
     Monitor *mon = g_malloc(sizeof(*mon));
     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) {

monitor.c void monitor_init(Chardev *chr, int flags) · line 4590

Quoted 5 lines

monitor.c +1 -4

-            error_report("Monitor Out-Of-band is only supported by QMP");
-            exit(1);
-        }
-    }
+    bool use_oob = (flags & MONITOR_USE_CONTROL) && !CHARDEV_IS_MUX(chr);

A comment explaining (briefly!) why MUX prevents oob would be useful here. Fortunately, you can simply steal from your commit message.

Quoted 46 lines

monitor.c +0 -3

     monitor_data_init(mon, false, use_oob);
 

@@ -4701,9 +4689,6 @@ QemuOptsList qemu_mon_opts = {
         },{
             .name = "pretty",
             .type = QEMU_OPT_BOOL,
-        },{
-            .name = "x-oob",
-            .type = QEMU_OPT_BOOL,
         },
         { /* end of list */ }
     },

tests/libqtest.c +1 -1

index 098af6aec4..c5cb3f925c 100644
@@ -213,7 +213,7 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
                                   "-display none "
                                   "%s", qemu_binary, socket_path,
                                   getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null",
-                                  qmp_socket_path, use_oob ? ",x-oob=on" : "",
+                                  qmp_socket_path, "",
                                   extra_args ?: "");
         execlp("/bin/sh", "sh", "-c", command, NULL);
         exit(1);

tests/qmp-test.c +1 -1

index a49cbc6fde..3747bf7fbb 100644
@@ -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 */

vl.c +0 -0

index 6e34fb348d..26a0bb3f0f 100644
@@ -2307,11 +2307,6 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
     if (qemu_opt_get_bool(opts, "pretty", 0))

vl.c static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp) · line 2312

Quoted 10 lines

vl.c +0 -5

         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) {

Preferrably with the commit message touched up, and a common on MUX:

Reviewed-by Markus Armbruster <[email protected]>

Quoted 15 lines · Markus Armbruster

Quoted 9 lines · Peter Xu

There was a regression reported by Eric Auger before with OOB:

http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html

It is fixed in 951702f39c ("monitor: bind dispatch bh to iohandler context", 2018-04-10).

For the bug, we turned Out-Of-Band feature of monitors off for 2.12 release. Now we turn that on again after the 2.12 release.

Relating what happened in the order it happened could be easier to understand. Perhaps:

Quoted 6 lines

OOB commands were introduced in commit cf869d53172. Unfortunately, we ran into a regression, and had to disable them by default for 2.12 (commit be933ffc23).

The regression has since been fixed (commit 951702f39c7 "monitor: bind dispatch bh to iohandler context"). Time to re-enable OOB.

This indeed looks much nicer.

Quoted 1 line

Quoted 4 lines

Quoted 2 lines

This patch partly reverts be933ffc23 (monitor: new parameter "x-oob"), meanwhile turn it on again by default for non-MUX QMPs. Note that we

"by default"?

Did I mis-spell somewhere?

Quoted 26 lines · Peter Xu

Quoted 15 lines · Markus Armbruster

Quoted 9 lines · Peter Xu

There was a regression reported by Eric Auger before with OOB:

http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html

It is fixed in 951702f39c ("monitor: bind dispatch bh to iohandler context", 2018-04-10).

For the bug, we turned Out-Of-Band feature of monitors off for 2.12 release. Now we turn that on again after the 2.12 release.

Relating what happened in the order it happened could be easier to understand. Perhaps:

Quoted 6 lines

OOB commands were introduced in commit cf869d53172. Unfortunately, we ran into a regression, and had to disable them by default for 2.12 (commit be933ffc23).

The regression has since been fixed (commit 951702f39c7 "monitor: bind dispatch bh to iohandler context"). Time to re-enable OOB.

This indeed looks much nicer.

Quoted 1 line

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

This patch partly reverts be933ffc23 (monitor: new parameter "x-oob"), meanwhile turn it on again by default for non-MUX QMPs. Note that we

"by default"?

Did I mis-spell somewhere?

I was too terse, sorry. Let me try again.

"By default" suggests there's a way for the user to switch it off. That's not the case. I guess you mean something like

This patch partly reverts be933ffc23 (monitor: new parameter "x-oob"), and turns OOB on again for non-MUX QMPs.

This reverts commit ddee57e0176f6ab53b13c6c97605b62737a8fd7a.

Meanwhile, revert one line from fa198ad9bdef to make sure qtest_init_without_qmp_handshake() will only pass in one parameter.

Signed-off-by Peter Xu <[email protected]>

v4:
- collect some r-bs
- remove one extra s-o-b of mine in one patch [Thomas, Markus]

Quoted 3 lines · Peter Xu

v4:
- collect some r-bs
- remove one extra s-o-b of mine in one patch [Thomas, Markus]

FYI: You've still got a duplicated s-o-b line in patch 3 and 4.

Thomas