monitor: enable OOB by default

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/tests/qemu-iotests/087.out
+++ /home/peterx/git/qemu/bin/tests/qemu-iotests/087.out.bad
@@ -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": {}}

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

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]>

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

monitor.c +22 -3

index 9c89c8695c..ea93db4857 100644
@@ -540,6 +540,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.

@@ -551,9 +572,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 +2 -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 48 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.

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.

Quoted 3 lines

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 think the output is no longer wrapped. Can drop the parenthesis when I apply.

Quoted 72 lines

--- /home/peterx/git/qemu/tests/qemu-iotests/087.out
+++ /home/peterx/git/qemu/bin/tests/qemu-iotests/087.out.bad
@@ -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": {}}

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

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]>

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

monitor.c +22 -3

index 9c89c8695c..ea93db4857 100644
@@ -540,6 +540,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.

@@ -551,9 +572,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 static void monitor_qmp_event(void *opaque, int event) · line 4456

Quoted 9 lines

monitor.c +6 -0

@@ -4429,6 +4448,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
+         * 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.

Did you forget to delete the last sentence, or decide to keep it?

I could drop it when I apply.

Quoted 50 lines · Markus Armbruster

Quoted 48 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.

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.

Quoted 6 lines

Quoted 3 lines

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 think the output is no longer wrapped. Can drop the parenthesis when I apply.

Indeed.

Quoted 84 lines

Quoted 72 lines

--- /home/peterx/git/qemu/tests/qemu-iotests/087.out
+++ /home/peterx/git/qemu/bin/tests/qemu-iotests/087.out.bad
@@ -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": {}}

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

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]>

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

monitor.c +22 -3

index 9c89c8695c..ea93db4857 100644
@@ -540,6 +540,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.

@@ -551,9 +572,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 static void monitor_qmp_event(void *opaque, int event) · line 4456

Quoted 9 lines

monitor.c +6 -0

@@ -4429,6 +4448,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
+         * 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.

Did you forget to delete the last sentence, or decide to keep it?

Quoted 1 line

I could drop it when I apply.

I'm sorry; this one I forgot.

Please feel free to drop it.

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

--- /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": {}}
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]>

---
 tests/qemu-iotests/060     | 10 +++++++++-
 tests/qemu-iotests/060.out |  1 -
 2 files changed, 9 insertions(+), 2 deletions(-)

tests/qemu-iotests/060 +9 -1

index 7bdf609f3f..74ad371885 100755
@@ -33,6 +33,14 @@ _cleanup()
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
+# Sometimes the error line might be dumped before/after an event
+# randomly.  Mask it out for specific test that may trigger this
+# uncertainty for current test for now.
+_filter_io_error()
+{
+    sed '/Input\/output error/d'
+}
+
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter

@@ -464,7 +472,7 @@ echo "{'execute': 'qmp_capabilities'}
                         }}" \
             -incoming exec:'cat /dev/null' \
             2>&1 \
-    | _filter_qmp | _filter_qemu_io
+    | _filter_qmp | _filter_qemu_io | _filter_io_error
 
 echo
 # Image should not have been marked corrupt

tests/qemu-iotests/060.out +0 -1

index bff023d889..d67c6234a4 100644
@@ -428,7 +428,6 @@ 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 0x2a2a2a00 unaligned (L1 index: 0)", "node-name": "drive", "fatal": false}}
-read failed: Input/output error
 {"return": ""}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}

I fooled around a bit, and I think there are a few lose ends.

Talking to a QMP monitor that supports OOB:

    $ socat UNIX:test-qmp READLINE,history=$HOME/.qmp_history,prompt='QMP> '
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-1703-gb909799463"}, "capabilities": ["oob"]}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "oob": true } }
    {"error": {"class": "GenericError", "desc": "Parameter 'oob' is unexpected"}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
    {"return": {}}
    QMP> { "execute": "query-qmp-schema" }
    {"error": {"class": "GenericError", "desc": "Out-Of-Band capability requires that every command contains an 'id' field"}}

Why does every command require 'id'?

Quoted 1 line · Markus Armbruster

I fooled around a bit, and I think there are a few lose ends.

[...]

Quoted 11 lines

Talking to a QMP monitor that supports OOB:

    $ socat UNIX:test-qmp READLINE,history=$HOME/.qmp_history,prompt='QMP> '
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-1703-gb909799463"}, "capabilities": ["oob"]}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "oob": true } }
    {"error": {"class": "GenericError", "desc": "Parameter 'oob' is unexpected"}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
    {"return": {}}
    QMP> { "execute": "query-qmp-schema" }
    {"error": {"class": "GenericError", "desc": "Out-Of-Band capability requires that every command contains an 'id' field"}}

Quoted 1 line

Why does every command require 'id'?

I found one reason: event COMMAND_DROPPED wants it. Any other reason?

[...]

Quoted 19 lines · Markus Armbruster

Quoted 1 line · Markus Armbruster

I fooled around a bit, and I think there are a few lose ends.

[...]

Quoted 11 lines

Talking to a QMP monitor that supports OOB:

    $ socat UNIX:test-qmp READLINE,history=$HOME/.qmp_history,prompt='QMP> '
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-1703-gb909799463"}, "capabilities": ["oob"]}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "oob": true } }
    {"error": {"class": "GenericError", "desc": "Parameter 'oob' is unexpected"}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
    {"return": {}}
    QMP> { "execute": "query-qmp-schema" }
    {"error": {"class": "GenericError", "desc": "Out-Of-Band capability requires that every command contains an 'id' field"}}

Quoted 1 line

Why does every command require 'id'?

I found one reason: event COMMAND_DROPPED wants it. Any other reason?

Quoted 1 line

[...]

Apropos COMMAND_DROPPED: we send an event rather than an error response because we may send it out-of-order. Makes sense.

However, broadcasting it to all monitors doesn't make sense. We could use a way to send an event to just one monitor.

Quoted 21 lines · Markus Armbruster

Quoted 19 lines · Markus Armbruster

Quoted 1 line · Markus Armbruster

I fooled around a bit, and I think there are a few lose ends.

[...]

Quoted 11 lines

Talking to a QMP monitor that supports OOB:

    $ socat UNIX:test-qmp READLINE,history=$HOME/.qmp_history,prompt='QMP> '
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-1703-gb909799463"}, "capabilities": ["oob"]}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "oob": true } }
    {"error": {"class": "GenericError", "desc": "Parameter 'oob' is unexpected"}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
    {"return": {}}
    QMP> { "execute": "query-qmp-schema" }
    {"error": {"class": "GenericError", "desc": "Out-Of-Band capability requires that every command contains an 'id' field"}}

Quoted 1 line

Why does every command require 'id'?

I found one reason: event COMMAND_DROPPED wants it. Any other reason?

Quoted 7 lines

Quoted 1 line

[...]

Apropos COMMAND_DROPPED: we send an event rather than an error response because we may send it out-of-order. Makes sense.

However, broadcasting it to all monitors doesn't make sense. We could use a way to send an event to just one monitor.

Worse than that - broadcasting to all monitors is categorically broken. Different monitors make use the same "id" formatting scheme, so if you broadcast COMMAND_DROPPED to a different monitor you might have clashing "id" and thus incorrectly tell a client its command was dropped when in fact it was processed. You'd have to be fairly unlucky in timing, but it could happen.

Quoted 22 lines · Daniel P. Berrangé

Quoted 21 lines · Markus Armbruster

Quoted 19 lines · Markus Armbruster

Quoted 1 line · Markus Armbruster

I fooled around a bit, and I think there are a few lose ends.

[...]

Quoted 11 lines

Talking to a QMP monitor that supports OOB:

    $ socat UNIX:test-qmp READLINE,history=$HOME/.qmp_history,prompt='QMP> '
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-1703-gb909799463"}, "capabilities": ["oob"]}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "oob": true } }
    {"error": {"class": "GenericError", "desc": "Parameter 'oob' is unexpected"}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
    {"return": {}}
    QMP> { "execute": "query-qmp-schema" }
    {"error": {"class": "GenericError", "desc": "Out-Of-Band capability requires that every command contains an 'id' field"}}

Quoted 1 line

Why does every command require 'id'?

I found one reason: event COMMAND_DROPPED wants it. Any other reason?

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

[...]

Apropos COMMAND_DROPPED: we send an event rather than an error response because we may send it out-of-order. Makes sense.

However, broadcasting it to all monitors doesn't make sense. We could use a way to send an event to just one monitor.

Quoted 7 lines

Worse than that - broadcasting to all monitors is categorically broken. Different monitors make use the same "id" formatting scheme, so if you broadcast COMMAND_DROPPED to a different monitor you might have clashing "id" and thus incorrectly tell a client its command was dropped when in fact it was processed. You'd have to be fairly unlucky in timing, but it could happen.

Right. Must fix bug.

I'm glad I went over this one more time, and in public!

Quoted 8 lines

Quoted 6 lines

Worse than that - broadcasting to all monitors is categorically broken. Different monitors make use the same "id" formatting scheme, so if you broadcast COMMAND_DROPPED to a different monitor you might have clashing "id" and thus incorrectly tell a client its command was dropped when in fact it was processed. You'd have to be fairly unlucky in timing, but it could happen.

Right. Must fix bug.

My current plan is that I can touch up scripts/qapi/events.py and related stuff to allow QMPEventFuncEmit to take a monitor parameter, then we pass in NULL when we want to send the event to all monitors.

Would that work?

Quoted 15 lines · Peter Xu

Quoted 8 lines

Quoted 6 lines

Worse than that - broadcasting to all monitors is categorically broken. Different monitors make use the same "id" formatting scheme, so if you broadcast COMMAND_DROPPED to a different monitor you might have clashing "id" and thus incorrectly tell a client its command was dropped when in fact it was processed. You'd have to be fairly unlucky in timing, but it could happen.

Right. Must fix bug.

My current plan is that I can touch up scripts/qapi/events.py and related stuff to allow QMPEventFuncEmit to take a monitor parameter, then we pass in NULL when we want to send the event to all monitors.

Quoted 1 line

Would that work?

Makes sense to me. Also, right now, ALL callers of qapi_event_send_* pass &error_abort as their final parameter. If you're refactoring everything anyways, you could get rid of that parameter on the presumption that it doesn't buy us anything.

Quoted 17 lines · Eric Blake

Quoted 15 lines · Peter Xu

Quoted 8 lines

Quoted 6 lines

Worse than that - broadcasting to all monitors is categorically broken. Different monitors make use the same "id" formatting scheme, so if you broadcast COMMAND_DROPPED to a different monitor you might have clashing "id" and thus incorrectly tell a client its command was dropped when in fact it was processed. You'd have to be fairly unlucky in timing, but it could happen.

Right. Must fix bug.

My current plan is that I can touch up scripts/qapi/events.py and related stuff to allow QMPEventFuncEmit to take a monitor parameter, then we pass in NULL when we want to send the event to all monitors.

Quoted 6 lines

Quoted 1 line

Would that work?

Makes sense to me. Also, right now, ALL callers of qapi_event_send_* pass &error_abort as their final parameter. If you're refactoring everything anyways, you could get rid of that parameter on the presumption that it doesn't buy us anything.

Let me try to turn presumption into fact :)

The generated qapi_event_send_FOO() can detect the following error conditions:

* Visitor fails. But the QObject output visitor can't actually fail.

Since the errp parameter is part of the abstract visitor interface, we can't eliminate it. We can pass &error_abort.

* emit() function fails. Can't happen, either.

Let's eliminate the unused errp parameter of QMPEventFuncEmit.

If we can eliminate the somewhat silly indirection through qmp_event_get_func_emit(), even better.

Quoted 29 lines · Markus Armbruster

Quoted 27 lines · Daniel P. Berrangé

Quoted 26 lines · Markus Armbruster

Quoted 15 lines · Markus Armbruster

Quoted 1 line · Markus Armbruster

I fooled around a bit, and I think there are a few lose ends.

[...]

Quoted 11 lines

Talking to a QMP monitor that supports OOB:

    $ socat UNIX:test-qmp READLINE,history=$HOME/.qmp_history,prompt='QMP> '
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-1703-gb909799463"}, "capabilities": ["oob"]}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "oob": true } }
    {"error": {"class": "GenericError", "desc": "Parameter 'oob' is unexpected"}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
    {"return": {}}
    QMP> { "execute": "query-qmp-schema" }
    {"error": {"class": "GenericError", "desc": "Out-Of-Band capability requires that every command contains an 'id' field"}}

Quoted 5 lines

Quoted 1 line

Why does every command require 'id'?

I found one reason: event COMMAND_DROPPED wants it. Any other reason?

[...]

Apropos COMMAND_DROPPED: we send an event rather than an error response because we may send it out-of-order. Makes sense.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

However, broadcasting it to all monitors doesn't make sense. We could use a way to send an event to just one monitor.

True.

(Sorry for the late responses; I was on Linuxcon China in the past few days)

Quoted 9 lines

Quoted 7 lines

Worse than that - broadcasting to all monitors is categorically broken. Different monitors make use the same "id" formatting scheme, so if you broadcast COMMAND_DROPPED to a different monitor you might have clashing "id" and thus incorrectly tell a client its command was dropped when in fact it was processed. You'd have to be fairly unlucky in timing, but it could happen.

Right. Must fix bug.

Even more true.

Quoted 1 line

Quoted 1 line

I'm glad I went over this one more time, and in public!

I had a glance at current qmp-spec, it seems that we don't have any restriction currently on "we must send events to all the monitors". Does it mean that we should be free to have per-monitor events starting from this event?

My current plan is that I can touch up scripts/qapi/events.py and related stuff to allow QMPEventFuncEmit to take a monitor parameter, then we pass in NULL when we want to send the event to all monitors.

Would that work?

Changing an existing event from broadcast to unicast is an observable change in existing behavior. Compatibility break unless we can show nobody can use / uses the observation.

Creating a new event is not a compatibility break by itself[*], regardless of broadcast vs. unicast.

Quoted 5 lines

My current plan is that I can touch up scripts/qapi/events.py and related stuff to allow QMPEventFuncEmit to take a monitor parameter, then we pass in NULL when we want to send the event to all monitors.

Would that work?

Think so.

Alternatively, a pair of functions:

     void qapi_event_bcast_EVENT(... event params ..., Error **errp);
     void qapi_event_send_EVENT(Monitor *mon, ... event params ..., Error **errp);

Slightly more compact in the broadcast case, might be a bit easier to read.

[*] In the case of COMMAND_DROPPED, the compatibility break is dropping commands (the event's trigger), caused by the command queuing feature. That's why command queuing has to be opt-in. Details discussed elsewhere in this thread.

Quoted 30 lines · Peter Xu

Quoted 29 lines · Markus Armbruster

Quoted 27 lines · Daniel P. Berrangé

Quoted 26 lines · Markus Armbruster

Quoted 15 lines · Markus Armbruster

Quoted 1 line · Markus Armbruster

I fooled around a bit, and I think there are a few lose ends.

[...]

Quoted 11 lines

Talking to a QMP monitor that supports OOB:

    $ socat UNIX:test-qmp READLINE,history=$HOME/.qmp_history,prompt='QMP> '
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-1703-gb909799463"}, "capabilities": ["oob"]}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "oob": true } }
    {"error": {"class": "GenericError", "desc": "Parameter 'oob' is unexpected"}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
    {"return": {}}
    QMP> { "execute": "query-qmp-schema" }
    {"error": {"class": "GenericError", "desc": "Out-Of-Band capability requires that every command contains an 'id' field"}}

Quoted 5 lines

Quoted 1 line

Why does every command require 'id'?

I found one reason: event COMMAND_DROPPED wants it. Any other reason?

[...]

Apropos COMMAND_DROPPED: we send an event rather than an error response because we may send it out-of-order. Makes sense.

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

However, broadcasting it to all monitors doesn't make sense. We could use a way to send an event to just one monitor.

True.

(Sorry for the late responses; I was on Linuxcon China in the past few days)

No need to be sorry :)

Quoted 13 lines

Quoted 9 lines

Quoted 7 lines

Worse than that - broadcasting to all monitors is categorically broken. Different monitors make use the same "id" formatting scheme, so if you broadcast COMMAND_DROPPED to a different monitor you might have clashing "id" and thus incorrectly tell a client its command was dropped when in fact it was processed. You'd have to be fairly unlucky in timing, but it could happen.

Right. Must fix bug.

Even more true.

Quoted 1 line

Quoted 6 lines

Quoted 1 line

I'm glad I went over this one more time, and in public!

I had a glance at current qmp-spec, it seems that we don't have any restriction currently on "we must send events to all the monitors". Does it mean that we should be free to have per-monitor events starting from this event?

Changing an existing event from broadcast to unicast is an observable change in existing behavior. Compatibility break unless we can show nobody can use / uses the observation.

Creating a new event is not a compatibility break by itself[*], regardless of broadcast vs. unicast.

Quoted 6 lines

My current plan is that I can touch up scripts/qapi/events.py and related stuff to allow QMPEventFuncEmit to take a monitor parameter, then we pass in NULL when we want to send the event to all monitors.

Would that work?

Think so.

Alternatively, a pair of functions:

    void qapi_event_bcast_EVENT(... event params ..., Error **errp);
    void qapi_event_send_EVENT(Monitor *mon, ... event params ..., Error **errp);

Slightly more compact in the broadcast case, might be a bit easier to read.

[*] In the case of COMMAND_DROPPED, the compatibility break is dropping commands (the event's trigger), caused by the command queuing feature. That's why command queuing has to be opt-in. Details discussed elsewhere in this thread.

Monitor behavior changes even when the client rejects capability "oob".

Traditionally, the monitor reads, executes and responds to one command after the other. If the client sends in-band commands faster than the server can execute them, the kernel will eventually refuse to buffer more, and sending blocks or fails with EAGAIN.

To make OOB possible, we need to read and queue commands as we receive them. If the client sends in-band commands faster than the server can execute them, the server will eventually drop commands to limit the queue length. The sever sends event COMMAND_DROPPED then.

However, we get the new behavior even when the client rejects capability "oob". We get the traditional behavior only when the server doesn't offer "oob".

Is this what we want?

Quoted 12 lines · Markus Armbruster

Monitor behavior changes even when the client rejects capability "oob".

Traditionally, the monitor reads, executes and responds to one command after the other. If the client sends in-band commands faster than the server can execute them, the kernel will eventually refuse to buffer more, and sending blocks or fails with EAGAIN.

To make OOB possible, we need to read and queue commands as we receive them. If the client sends in-band commands faster than the server can execute them, the server will eventually drop commands to limit the queue length. The sever sends event COMMAND_DROPPED then.

Quoted 5 lines

However, we get the new behavior even when the client rejects capability "oob". We get the traditional behavior only when the server doesn't offer "oob".

Is this what we want?

IMHO the key benefit of allowing the client to reject the capability is to enable backwards compat support. So this behaviour feels wrong. Rejecting OOB should have same semantics as previous QEMU's without OOB available, otherwise we now have 3 distinct ways the monitor operates (no OOB, OOB rejected, OOB accepted). This can only ever lead to more bugs due to lack of testing of no OOB vs OOB rejected scenarios.

Monitor behavior changes even when the client rejects capability "oob".

Traditionally, the monitor reads, executes and responds to one command after the other. If the client sends in-band commands faster than the server can execute them, the kernel will eventually refuse to buffer more, and sending blocks or fails with EAGAIN.

To make OOB possible, we need to read and queue commands as we receive them. If the client sends in-band commands faster than the server can execute them, the server will eventually drop commands to limit the queue length. The sever sends event COMMAND_DROPPED then.

However, we get the new behavior even when the client rejects capability "oob". We get the traditional behavior only when the server doesn't offer "oob".

Is this what we want?

Quoted 12 lines · Markus Armbruster

Monitor behavior changes even when the client rejects capability "oob".

Traditionally, the monitor reads, executes and responds to one command after the other. If the client sends in-band commands faster than the server can execute them, the kernel will eventually refuse to buffer more, and sending blocks or fails with EAGAIN.

To make OOB possible, we need to read and queue commands as we receive them. If the client sends in-band commands faster than the server can execute them, the server will eventually drop commands to limit the queue length. The sever sends event COMMAND_DROPPED then.

Quoted 5 lines

However, we get the new behavior even when the client rejects capability "oob". We get the traditional behavior only when the server doesn't offer "oob".

Is this what we want?

Not really. But I thought we kept that old behavior, haven't we?

In handle_qmp_command() we have this:

    /*
     * If OOB is not enabled on the current monitor, we'll emulate the
     * old behavior that we won't process the current monitor any more
     * until it has responded.  This helps make sure that as long as
     * OOB is not enabled, the server will never drop any command.
     */
    if (!qmp_oob_enabled(mon)) {
        monitor_suspend(mon);
        req_obj->need_resume = true;
    } else {
        /* Drop the request if queue is full. */
        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
            qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
            qapi_event_send_command_dropped(id,
                                            COMMAND_DROP_REASON_QUEUE_FULL,
                                            &error_abort);
            qmp_request_free(req_obj);
            return;
        }
    }

Here assuming that we are not enabling OOB, then since we will suspend the monitor when we receive one command, then monitor_can_read() later will check with a result that "we should not read the chardev", then it'll leave all the data in the input buffer. Then AFAIU the QMP client that didn't enable OOB will have similar behavior as before.

Also, we will never receive a COMMAND_DROPPED event in that legacy
mode as well since it's in the "else" block. Am I right?

Quoted 44 lines · Peter Xu

Quoted 12 lines · Markus Armbruster

Monitor behavior changes even when the client rejects capability "oob".

Traditionally, the monitor reads, executes and responds to one command after the other. If the client sends in-band commands faster than the server can execute them, the kernel will eventually refuse to buffer more, and sending blocks or fails with EAGAIN.

To make OOB possible, we need to read and queue commands as we receive them. If the client sends in-band commands faster than the server can execute them, the server will eventually drop commands to limit the queue length. The sever sends event COMMAND_DROPPED then.

Quoted 5 lines

However, we get the new behavior even when the client rejects capability "oob". We get the traditional behavior only when the server doesn't offer "oob".

Is this what we want?

Not really. But I thought we kept that old behavior, haven't we?

In handle_qmp_command() we have this:

    /*
     * If OOB is not enabled on the current monitor, we'll emulate the
     * old behavior that we won't process the current monitor any more
     * until it has responded.  This helps make sure that as long as
     * OOB is not enabled, the server will never drop any command.
     */
    if (!qmp_oob_enabled(mon)) {
        monitor_suspend(mon);
        req_obj->need_resume = true;
    } else {
        /* Drop the request if queue is full. */
        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
            qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
            qapi_event_send_command_dropped(id,
                                            COMMAND_DROP_REASON_QUEUE_FULL,
                                            &error_abort);
            qmp_request_free(req_obj);
            return;
        }
    }

Quoted 8 lines

Here assuming that we are not enabling OOB, then since we will suspend the monitor when we receive one command, then monitor_can_read() later will check with a result that "we should not read the chardev", then it'll leave all the data in the input buffer. Then AFAIU the QMP client that didn't enable OOB will have similar behavior as before.

Also, we will never receive a COMMAND_DROPPED event in that legacy
mode as well since it's in the "else" block. Am I right?

Hmm. Did I get confused? Let me look again.

Some places check qmp_oob_enabled(), which is true when the server offered capability "oob" and the client accepted. In terms of my reply to Daniel, it distinguishes the two run time cases "OOB off" and "OOB on".

Other places check ->use_io_thr, which is true when

    (flags & MONITOR_USE_CONTROL) && !CHARDEV_IS_MUX(chr)

in monitor_init().

One of these places is get_qmp_greeting(). It makes the server offer "oob" when ->use_io_thr. Makes sense.

In terms of my reply to Daniel, ->use_io_thr distinguishes between "OOB not offered" and ("OOB offered, but rejected by client" or "OOB offered and accepted").

Uses could to violate 'may use "server offered OOB" only for configuration purposes', so let's check them:

* monitor_json_emitter()

If mon->use_io_thr, push to response queue. Else emit directly.

I'm afraid run time behavior differs for "OOB not offered" (emit directly) and "OOB offered by rejected" (queue).

* qmp_caps_check()

If !mon->use_io_thr, reject client's acceptance of "oob". Implicitly recomputing the set of offered capabilities here is less than elegant, but it's not wrong.

* monitor_resume()

If mon->use_io_thr(), kick the monitor I/O thread.

Again, different behavior for the two variations of "OOB off".

* get_qmp_greeting()

Covered above, looks good to me.

* monitor_qmp_setup_handlers_bh()

If mon->use_io_thr(), pass the monitor I/O thread's AIOContext to qemu_chr_fe_set_handlers(), else pass NULL.

Again, different behavior for the two variations of "OOB off".

* monitor_init()

If mon->use_io_thr, set up bottom half monitor_qmp_setup_handlers_bh(), else call qemu_chr_fe_set_handlers() directly.

Again, different behavior for the two variations of "OOB off".

Either I am confused now, or the two variations of "OOB off" execute different code at run time. Which is it?

If it's different code, are the differences observable for the client?

Observable or not, I suspect the differences should not exist.

Quoted 55 lines · Markus Armbruster

Quoted 52 lines · Peter Xu

Quoted 12 lines · Markus Armbruster

Monitor behavior changes even when the client rejects capability "oob".

Traditionally, the monitor reads, executes and responds to one command after the other. If the client sends in-band commands faster than the server can execute them, the kernel will eventually refuse to buffer more, and sending blocks or fails with EAGAIN.

To make OOB possible, we need to read and queue commands as we receive them. If the client sends in-band commands faster than the server can execute them, the server will eventually drop commands to limit the queue length. The sever sends event COMMAND_DROPPED then.

Quoted 5 lines

However, we get the new behavior even when the client rejects capability "oob". We get the traditional behavior only when the server doesn't offer "oob".

Is this what we want?

Not really. But I thought we kept that old behavior, haven't we?

In handle_qmp_command() we have this:

    /*
     * If OOB is not enabled on the current monitor, we'll emulate the
     * old behavior that we won't process the current monitor any more
     * until it has responded.  This helps make sure that as long as
     * OOB is not enabled, the server will never drop any command.
     */
    if (!qmp_oob_enabled(mon)) {
        monitor_suspend(mon);
        req_obj->need_resume = true;
    } else {
        /* Drop the request if queue is full. */
        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
            qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
            qapi_event_send_command_dropped(id,
                                            COMMAND_DROP_REASON_QUEUE_FULL,
                                            &error_abort);
            qmp_request_free(req_obj);
            return;
        }
    }

Here assuming that we are not enabling OOB, then since we will suspend the monitor when we receive one command, then monitor_can_read() later will check with a result that "we should not read the chardev", then it'll leave all the data in the input buffer. Then AFAIU the QMP client that didn't enable OOB will have similar behavior as before.

Also, we will never receive a COMMAND_DROPPED event in that legacy
mode as well since it's in the "else" block. Am I right?

Quoted 6 lines

Hmm. Did I get confused? Let me look again.

Some places check qmp_oob_enabled(), which is true when the server offered capability "oob" and the client accepted. In terms of my reply to Daniel, it distinguishes the two run time cases "OOB off" and "OOB on".

Exactly.

Quoted 6 lines

Other places check ->use_io_thr, which is true when

    (flags & MONITOR_USE_CONTROL) && !CHARDEV_IS_MUX(chr)

in monitor_init().

Quoted 6 lines

One of these places is get_qmp_greeting(). It makes the server offer "oob" when ->use_io_thr. Makes sense.

In terms of my reply to Daniel, ->use_io_thr distinguishes between "OOB not offered" and ("OOB offered, but rejected by client" or "OOB offered and accepted").

Exactly.

To be more clear, let's name the three cases (as you mentioned):

(A) OOB not offered
(B) OOB offered, but rejected by client
(C) OOB offered, and accepted

Then:

(1) qmp_oob_enabled() will only be return true if (C).
(2) ->use_io_thr will only be true if (B) or (C).

Quoted 4 lines

Uses could to violate 'may use "server offered OOB" only for configuration purposes', so let's check them:

* monitor_json_emitter()

Quoted 4 lines

If mon->use_io_thr, push to response queue. Else emit directly.

I'm afraid run time behavior differs for "OOB not offered" (emit directly) and "OOB offered by rejected" (queue).

Yeah it's different, but logically it's the same. IMHO it's only a fast path for main thread. In other word, I think it can still work correctly if we remove that else clause. In that sense, it's not really a "true" behavior change.

Quoted 5 lines

* qmp_caps_check()

If !mon->use_io_thr, reject client's acceptance of "oob". Implicitly recomputing the set of offered capabilities here is less than elegant, but it's not wrong.

Quoted 5 lines

* monitor_resume()

If mon->use_io_thr(), kick the monitor I/O thread.

Again, different behavior for the two variations of "OOB off".

This is another example like above - IMHO we can just kick it no matter what, but we just don't need to do that for main thread (since we are in main thread so we are sure it is not asleep). It's just another trivial enhancement on top of the main logic.

Quoted 3 lines

* get_qmp_greeting()

Covered above, looks good to me.

Quoted 5 lines

* monitor_qmp_setup_handlers_bh()

If mon->use_io_thr(), pass the monitor I/O thread's AIOContext to qemu_chr_fe_set_handlers(), else pass NULL.

Quoted 1 line

Again, different behavior for the two variations of "OOB off".

This is different indeed, but IMHO it's not really "behavior difference", it's just the context (thread to run the code) that is different. The major code path for case (A) and (B) (which are the two "off" cases) should be the same (when I say "major", I excluded those trivial enhancements that I mentioned).

Quoted 5 lines

* monitor_init()

If mon->use_io_thr, set up bottom half monitor_qmp_setup_handlers_bh(), else call qemu_chr_fe_set_handlers() directly.

This is indeed a bit tricky (to avoid a potential race that Stefan has pointed out), however after things are setup it'll be exactly the same code path for both OFF cases.

And actually when I read the code I noticed that we can actually simplify the code with this:

monitor.c +8 -12

index 0730a27172..5d6bff8d51 100644
@@ -4635,18 +4635,14 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
     Monitor *mon = opaque;
     GMainContext *context;
 
-    if (mon->use_io_thr) {
-        /*
-         * When use_io_thr is set, we use the global shared dedicated
-         * IO thread for this monitor to handle input/output.
-         */
-        context = monitor_get_io_context();
-        /* We should have inited globals before reaching here. */
-        assert(context);
-    } else {
-        /* The default main loop, which is the main thread */
-        context = NULL;
-    }
+    assert(mon->use_io_thr);
+    /*
+     * When use_io_thr is set, we use the global shared dedicated
+     * IO thread for this monitor to handle input/output.
+     */
+    context = monitor_get_io_context();
+    /* We should have inited globals before reaching here. */
+    assert(context);
 
     qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
                              monitor_qmp_event, NULL, mon, context, true);

Since monitor_qmp_setup_handlers_bh() will only be used for IOThread case. I can post a patch for that.

Quoted 2 lines

Again, different behavior for the two variations of "OOB off".

Quoted 2 lines

Either I am confused now, or the two variations of "OOB off" execute different code at run time. Which is it?

As mentioned, they should be running the same code at run time. Though with some trivial differences, but if you see above most of them should only be small enhancements which can actually be removed.

Quoted 1 line

If it's different code, are the differences observable for the client?

AFAIU since the code path should mostly be the same for the two OOF cases, IMHO there should have no difference observable from the client, otherwise it's buggy and I'd be willing to fix it.

Quoted 1 line

Quoted 1 line

Observable or not, I suspect the differences should not exist.

We can remove those "trivial enhancements" if we want to make sure the code paths are exactly the same, but I'm not sure whether that's really what we need (or we just need to make sure it's unobservable).

Thanks!

Marc-André, one question for you inline. Search for your name.

Quoted 56 lines · Peter Xu

Quoted 55 lines · Markus Armbruster

Quoted 52 lines · Peter Xu

Quoted 12 lines · Markus Armbruster

Monitor behavior changes even when the client rejects capability "oob".

Traditionally, the monitor reads, executes and responds to one command after the other. If the client sends in-band commands faster than the server can execute them, the kernel will eventually refuse to buffer more, and sending blocks or fails with EAGAIN.

To make OOB possible, we need to read and queue commands as we receive them. If the client sends in-band commands faster than the server can execute them, the server will eventually drop commands to limit the queue length. The sever sends event COMMAND_DROPPED then.

Quoted 5 lines

However, we get the new behavior even when the client rejects capability "oob". We get the traditional behavior only when the server doesn't offer "oob".

Is this what we want?

Not really. But I thought we kept that old behavior, haven't we?

In handle_qmp_command() we have this:

    /*
     * If OOB is not enabled on the current monitor, we'll emulate the
     * old behavior that we won't process the current monitor any more
     * until it has responded.  This helps make sure that as long as
     * OOB is not enabled, the server will never drop any command.
     */
    if (!qmp_oob_enabled(mon)) {
        monitor_suspend(mon);
        req_obj->need_resume = true;
    } else {
        /* Drop the request if queue is full. */
        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
            qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
            qapi_event_send_command_dropped(id,
                                            COMMAND_DROP_REASON_QUEUE_FULL,
                                            &error_abort);
            qmp_request_free(req_obj);
            return;
        }
    }

Here assuming that we are not enabling OOB, then since we will suspend the monitor when we receive one command, then monitor_can_read() later will check with a result that "we should not read the chardev", then it'll leave all the data in the input buffer. Then AFAIU the QMP client that didn't enable OOB will have similar behavior as before.

Also, we will never receive a COMMAND_DROPPED event in that legacy
mode as well since it's in the "else" block. Am I right?

Quoted 15 lines

Quoted 6 lines

Hmm. Did I get confused? Let me look again.

Some places check qmp_oob_enabled(), which is true when the server offered capability "oob" and the client accepted. In terms of my reply to Daniel, it distinguishes the two run time cases "OOB off" and "OOB on".

Exactly.

Quoted 6 lines

Other places check ->use_io_thr, which is true when

    (flags & MONITOR_USE_CONTROL) && !CHARDEV_IS_MUX(chr)

in monitor_init().

->use_io_thr is now spelled ->use_io_thread.

Quoted 11 lines

Quoted 6 lines

One of these places is get_qmp_greeting(). It makes the server offer "oob" when ->use_io_thr. Makes sense.

In terms of my reply to Daniel, ->use_io_thr distinguishes between "OOB not offered" and ("OOB offered, but rejected by client" or "OOB offered and accepted").

Exactly.

To be more clear, let's name the three cases (as you mentioned):

Quoted 8 lines

(A) OOB not offered
(B) OOB offered, but rejected by client
(C) OOB offered, and accepted

Then:

(1) qmp_oob_enabled() will only be return true if (C).
(2) ->use_io_thr will only be true if (B) or (C).

Notes on (A) to be kept in mind for the remainder of the discussion:

* I don't expect (A) to occur in production

(A) means the monitor has a chardev-mux backend. chardev-mux multiplexes multiple character backends, e.g. multiplex a monitor and a console on stdio. C-a c switches focus. While such multiplexing may be convenient for humans, it's an unnecessary complication for machines, which could just as well use two UNIX domain sockets and not have to worry about focus and escape sequences.

* I do expect (A) to go away eventually

(A) exists only because "not all the chardev frontends can run without main thread, or can run in multiple threads" [PATCH 6]. Hopefully, that'll be fixed eventually, so (A) can go away.

Marc-André, your "[PATCH 04/12] Revert "qmp: isolate responses into io thread" states the "chardev write path is thread safe". What's missing to make chardev-mux capable of supporting OOB?

Quoted 4 lines

Quoted 4 lines

Uses could to violate 'may use "server offered OOB" only for configuration purposes', so let's check them:

* monitor_json_emitter()

This is now qmp_queue_response()

Quoted 9 lines

Quoted 4 lines

If mon->use_io_thr, push to response queue. Else emit directly.

I'm afraid run time behavior differs for "OOB not offered" (emit directly) and "OOB offered by rejected" (queue).

Yeah it's different, but logically it's the same. IMHO it's only a fast path for main thread. In other word, I think it can still work correctly if we remove that else clause. In that sense, it's not really a "true" behavior change.

Remember that (A) should not occur in production, and should go away eventually. Maintaining a fast path just for that feels unjustified.

Quoted 5 lines

Quoted 5 lines

* qmp_caps_check()

If !mon->use_io_thr, reject client's acceptance of "oob". Implicitly recomputing the set of offered capabilities here is less than elegant, but it's not wrong.

This has been replaced by monitor_qmp_caps_reset().

If mon->use_io_thread, offer OOB. Makes sense.

Quoted 10 lines

Quoted 5 lines

* monitor_resume()

If mon->use_io_thr(), kick the monitor I/O thread.

Again, different behavior for the two variations of "OOB off".

This is another example like above - IMHO we can just kick it no matter what, but we just don't need to do that for main thread (since we are in main thread so we are sure it is not asleep). It's just another trivial enhancement on top of the main logic.

Again, maintaining a special case just for (A) feels unjustified.

Quoted 3 lines

Quoted 3 lines

* get_qmp_greeting()

Covered above, looks good to me.

Also replaced by monitor_qmp_caps_reset().

Quoted 5 lines

Quoted 5 lines

* monitor_qmp_setup_handlers_bh()

If mon->use_io_thr(), pass the monitor I/O thread's AIOContext to qemu_chr_fe_set_handlers(), else pass NULL.

Quoted 7 lines

Quoted 1 line

Again, different behavior for the two variations of "OOB off".

This is different indeed, but IMHO it's not really "behavior difference", it's just the context (thread to run the code) that is different. The major code path for case (A) and (B) (which are the two "off" cases) should be the same (when I say "major", I excluded those trivial enhancements that I mentioned).

As far as I can tell, monitor_qmp_setup_handlers_bh() can run only if scheduled by monitor_init() when mon->use_io_thread. If that's true, then the !mon->use_io_thread case is unreachable. Ah, I see you've also noticed that, and you're proposing a patch below.

Quoted 43 lines

Quoted 6 lines

* monitor_init()

If mon->use_io_thr, set up bottom half monitor_qmp_setup_handlers_bh(), else call qemu_chr_fe_set_handlers() directly.

This is indeed a bit tricky (to avoid a potential race that Stefan has pointed out), however after things are setup it'll be exactly the same code path for both OFF cases.

And actually when I read the code I noticed that we can actually simplify the code with this:

monitor.c +8 -12

index 0730a27172..5d6bff8d51 100644
@@ -4635,18 +4635,14 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
     Monitor *mon = opaque;
     GMainContext *context;
 
-    if (mon->use_io_thr) {
-        /*
-         * When use_io_thr is set, we use the global shared dedicated
-         * IO thread for this monitor to handle input/output.
-         */
-        context = monitor_get_io_context();
-        /* We should have inited globals before reaching here. */
-        assert(context);
-    } else {
-        /* The default main loop, which is the main thread */
-        context = NULL;
-    }
+    assert(mon->use_io_thr);
+    /*
+     * When use_io_thr is set, we use the global shared dedicated
+     * IO thread for this monitor to handle input/output.
+     */
+    context = monitor_get_io_context();
+    /* We should have inited globals before reaching here. */
+    assert(context);
 

Quoted 5 lines

monitor.c +0 -0

     qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
                              monitor_qmp_event, NULL, mon, context, true);

Since monitor_qmp_setup_handlers_bh() will only be used for IOThread case. I can post a patch for that.

Yes, please.

Quoted 3 lines

Quoted 3 lines

Again, different behavior for the two variations of "OOB off".

The difference is

* timing: right away for (A) vs. bottom half for (B) and (C)

* context argument for qemu_chr_fe_set_handlers(): NULL, i.e. main loop thread for (A), monitor_get_io_context(), i.e. the I/O thread for (B) and (C)

Okay, I think.

Quoted 6 lines

Quoted 2 lines

Either I am confused now, or the two variations of "OOB off" execute different code at run time. Which is it?

As mentioned, they should be running the same code at run time. Though with some trivial differences, but if you see above most of them should only be small enhancements which can actually be removed.

Please do.

Quoted 7 lines

Quoted 1 line

If it's different code, are the differences observable for the client?

AFAIU since the code path should mostly be the same for the two OOF cases, IMHO there should have no difference observable from the client, otherwise it's buggy and I'd be willing to fix it.

Quoted 1 line

Quoted 5 lines

Quoted 1 line

Observable or not, I suspect the differences should not exist.

We can remove those "trivial enhancements" if we want to make sure the code paths are exactly the same, but I'm not sure whether that's really what we need (or we just need to make sure it's unobservable).

As long as we're running separate code for (A), "unobservable difference" is a proposition we need to prove.

Reducing separate code should simplify the proof.

Eliminiating it will simplify it maximally :)

Quoted 2 lines

Thanks!

Thank *you* for persevering :)

Another lose end: event COMMAND_DROPPED seems to lack test coverage.

Quoted 1 line · Markus Armbruster

Another lose end: event COMMAND_DROPPED seems to lack test coverage.

Hmm, dropping commands serves to limit the request queue. What limits the response queue?

Before OOB, the monitor read at most one command, and wrote its response with monitor_puts().

For input, this leaves queueing to the kernel: if the client sends commands faster than the server can execute them, eventually the kernel refuses to buffer more, and the client's send either blocks or fails with EAGAIN.

Output is a mess. monitor_puts() uses an output buffer. It tries to flush at newline. Issues:

* If flushing runs into a partial write, the unwritten remainder remains in the output buffer until the next newline. That newline may take its own sweet time to arrive. Could even lead to deadlocks, where a client awaits complete output before it sends more input. Bug, predates OOB, doesn't block this series.

* If the client fails to read, the output buffer can grow without bound. Not a security issue; the client is trusted. Just bad workmanship.

OOB doesn't change this for monitors running in the main thread. Only mux chardevs run there.

Aside: keeping special case code around just for mux is a really bad idea. We need to get rid of it.

For monitors running in an I/O thread, we add another buffer: the response queue. It's drained by monitor_qmp_bh_responder(). I guess that means the response queue is effectively bounded by timely draining. Correct?

Buffering twice seems silly, but that could be addressed in follow-up patches.

Quoted 4 lines · Markus Armbruster

Quoted 1 line · Markus Armbruster

Another lose end: event COMMAND_DROPPED seems to lack test coverage.

Quoted 2 lines

Hmm, dropping commands serves to limit the request queue. What limits the response queue?

As long as we have a request queue limitation, that'll somehow be "part of" the limitation of response queue. Since the real responses (let's not consider the events first) should be no more than the maximum QMP requests that we allow in the request queue (one response for one request). In that sense it seems fine to me.

Quoted 8 lines

Before OOB, the monitor read at most one command, and wrote its response with monitor_puts().

For input, this leaves queueing to the kernel: if the client sends commands faster than the server can execute them, eventually the kernel refuses to buffer more, and the client's send either blocks or fails with EAGAIN.

Quoted 6 lines

Output is a mess. monitor_puts() uses an output buffer. It tries to flush at newline. Issues:

* If flushing runs into a partial write, the unwritten remainder remains in the output buffer until the next newline. That newline may take its own sweet time to arrive.

Quoted 3 lines

Could even lead to deadlocks, where a
client awaits complete output before it sends more input. Bug, predates OOB, doesn't block this series.

True. Though I noticed that we have a "hackish" line in monitor_json_emitter_raw():

    qstring_append_chr(json, '\n');

So it seems that at least we should never encounter a deadlock, after all there will always be a newline there. But I'd say I agree with you on that it's at least not that "beautiful". :-)

Quoted 2 lines

* If the client fails to read, the output buffer can grow without bound. Not a security issue; the client is trusted. Just bad workmanship.

True.

Quoted 4 lines

OOB doesn't change this for monitors running in the main thread. Only mux chardevs run there.

Quoted 2 lines

Aside: keeping special case code around just for mux is a really bad idea. We need to get rid of it.

We should be running the same code path even for MUX-ed typed, right? Do you mean to put MUX-ed typed handling onto iothreads as well when you say "get rid of it"?

Quoted 4 lines

For monitors running in an I/O thread, we add another buffer: the response queue. It's drained by monitor_qmp_bh_responder(). I guess that means the response queue is effectively bounded by timely draining. Correct?

I don't see a timely manner to flush it, but as long as we queue anything (including events) onto the response queue, we'll poke the bottom half (in monitor_json_emitter() we call qemu_bh_schedule()) so we'll possibly drain the queue very soon, and there should be no chance to have a stale message in that queue.

Quoted 2 lines

Buffering twice seems silly, but that could be addressed in follow-up patches.

Do you mean that we can write the response immediately into Monitor.outbuf, then only flush it in iothread? IMHO that's fine - after all, the response queue, as mentioned above, should have a natural restriction as well due to the request queue, then we won't waste too much resources for that. Meanwhile using a queue with QMP response objects seems to be a bit more cleaner to me from design pov (though I might be wrong).

Quoted 5 lines · Peter Xu

Quoted 4 lines · Markus Armbruster

Quoted 1 line · Markus Armbruster

Another lose end: event COMMAND_DROPPED seems to lack test coverage.

Quoted 8 lines

Quoted 2 lines

Hmm, dropping commands serves to limit the request queue. What limits the response queue?

As long as we have a request queue limitation, that'll somehow be "part of" the limitation of response queue. Since the real responses (let's not consider the events first) should be no more than the maximum QMP requests that we allow in the request queue (one response for one request). In that sense it seems fine to me.

"Normal" flow of the request through the server (QEMU):

receive
-> handle_qmp_command()

-> request queue: mon->qmp.qmp_requests

-> monitor_qmp_bh_dispatcher()
-> monitor_qmp_dispatch_one()
-> monitor_qmp_respond()
-> monitor_json_emitter()

-> response queue: mon->qmp.qmp_requests

-> monitor_qmp_response_pop_one()
-> monitor_qmp_bh_responder()
-> monitor_json_emitter_raw()
-> monitor_puts()

-> output buffer: mon->outbuf

-> monitor_flush_locked()

The purpose of event COMMAND_DROPPED is flow control notification: when the client sends more than we're willing to buffer, we drop the excess and notify the client.

If the client sends too many requests too quickly, the request queue fills up, and flow control kicks in. Good.

As long as the client sends requests at a moderate pace, the request queue never fills up: the dispatch & execute code sitting between request queue and response queue drains it just fine.

The response queue proper also doesn't fill up: the emitter code sitting between response queue and output buffer drains it just fine.

However, output buffer can still grow without bounds! monitor_flush_locked() requires the client to keep up to make progress. Our flow control fails then.

Extreme case: a (misbehaving!) client that keeps sending requests at a moderate pace while not reading any responses. output buffer grows without bounds.

Less extreme case: a client sends a small number of requests quickly, then reads responses very slowly or not at all for some reason, say because the network goes bad right at this time. Here, the size of the request queue does limit the size of output buffer, as you proposed, but the size multiplier isn't really known.

My point is: the idea "limiting the request queue also limits the response queue + output buffer" isn't entirely wrong, but it's not entirely right, either.

Can we improve flow control to cover the complete flow, not just the flow into the request queue?

Quoted 9 lines

Quoted 9 lines

Before OOB, the monitor read at most one command, and wrote its response with monitor_puts().

For input, this leaves queueing to the kernel: if the client sends commands faster than the server can execute them, eventually the kernel refuses to buffer more, and the client's send either blocks or fails with EAGAIN.

Quoted 6 lines

Quoted 6 lines

Output is a mess. monitor_puts() uses an output buffer. It tries to flush at newline. Issues:

* If flushing runs into a partial write, the unwritten remainder remains in the output buffer until the next newline. That newline may take its own sweet time to arrive.

Hmm, it's also flushed via mon->out_watch. If that works how I guess it does, there's no deadlock.

Quoted 4 lines

Quoted 3 lines

Could even lead to deadlocks, where a
client awaits complete output before it sends more input. Bug, predates OOB, doesn't block this series.

Quoted 8 lines

True. Though I noticed that we have a "hackish" line in monitor_json_emitter_raw():

    qstring_append_chr(json, '\n');

So it seems that at least we should never encounter a deadlock, after all there will always be a newline there. But I'd say I agree with you on that it's at least not that "beautiful". :-)

The newline ensures responses arrive on their own line. JSON doesn't care about lines (makes sense), and qmp-spec.txt doesn't care, either (that's wrong, in my opinion). Anyone playing with QMP by hand will definitely care. Even QMP clients might.

The newline also triggers the actual write(), because monitor_puts() is line-buffered. That buffering makes sense for HMP, but it's useless for QMP. Let's not worry about that right now.

The flushing, however, is not guaranteed to write anything! If qemu_chr_fe_write() fails with EAGAIN, mon->outbuf remains unchanged.

Quoted 10 lines

Quoted 3 lines

* If the client fails to read, the output buffer can grow without bound. Not a security issue; the client is trusted. Just bad workmanship.

True.

Quoted 4 lines

OOB doesn't change this for monitors running in the main thread. Only mux chardevs run there.

Quoted 6 lines

Quoted 2 lines

Aside: keeping special case code around just for mux is a really bad idea. We need to get rid of it.

We should be running the same code path even for MUX-ed typed, right? Do you mean to put MUX-ed typed handling onto iothreads as well when you say "get rid of it"?

I figure I'll cover this in my reply to Daniel. If not, I'll reply to this one again.

Quoted 10 lines

Quoted 4 lines

For monitors running in an I/O thread, we add another buffer: the response queue. It's drained by monitor_qmp_bh_responder(). I guess that means the response queue is effectively bounded by timely draining. Correct?

I don't see a timely manner to flush it, but as long as we queue anything (including events) onto the response queue, we'll poke the bottom half (in monitor_json_emitter() we call qemu_bh_schedule()) so we'll possibly drain the queue very soon, and there should be no chance to have a stale message in that queue.

As long as bottom halves work, the response queue remains small. That's okay.

Quoted 10 lines

Quoted 2 lines

Buffering twice seems silly, but that could be addressed in follow-up patches.

Do you mean that we can write the response immediately into Monitor.outbuf, then only flush it in iothread? IMHO that's fine - after all, the response queue, as mentioned above, should have a natural restriction as well due to the request queue, then we won't waste too much resources for that. Meanwhile using a queue with QMP response objects seems to be a bit more cleaner to me from design pov (though I might be wrong).

Again, let's not worry about this right now.

Another lose end: event COMMAND_DROPPED seems to lack test coverage.

I fooled around a bit, and I think there are a few lose ends.

Lets update the examples in docs/interop/qmp-spec.txt to show the current greeting (section 3.1) and how to accept a capability (section 3.2). The capability negotiation documentation could use some polish. I'll post a patch.

Talking to a QMP monitor that supports OOB:

    $ socat UNIX:test-qmp READLINE,history=$HOME/.qmp_history,prompt='QMP> '
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-1703-gb909799463"}, "capabilities": ["oob"]}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "oob": true } }
    {"error": {"class": "GenericError", "desc": "Parameter 'oob' is unexpected"}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
    {"return": {}}
    QMP> { "execute": "query-qmp-schema" }
    {"error": {"class": "GenericError", "desc": "Out-Of-Band capability requires that every command contains an 'id' field"}}

Why does every command require 'id'?

Talking to a QMP monitor that doesn't support OOB:

    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-1703-gb909799463"}, "capabilities": []}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
    {"error": {"class": "GenericError", "desc": "This monitor does not support Out-Of-Band (OOB)"}}
    QMP> { "execute": "qmp_capabilities" }
    {"return": {}}
    QMP> { "execute": "query-kvm" }
    {"return": {"enabled": true, "present": true}}
    QMP> { "execute": "query-kvm", "control": { "run-oob": true } }
    {"error": {"class": "GenericError", "desc": "Please enable Out-Of-Band first for the session during capabilities negotiation"}}

Telling people to enable OOB when that cannot be done is suboptimal. More so when it cannot be used here anyway. I'll post a patch.

Quoted 1 line · Markus Armbruster

I fooled around a bit, and I think there are a few lose ends.

Quoted 11 lines

Talking to a QMP monitor that supports OOB:

    $ socat UNIX:test-qmp READLINE,history=$HOME/.qmp_history,prompt='QMP> '
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-1703-gb909799463"}, "capabilities": ["oob"]}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "oob": true } }
    {"error": {"class": "GenericError", "desc": "Parameter 'oob' is unexpected"}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
    {"return": {}}
    QMP> { "execute": "query-qmp-schema" }
    {"error": {"class": "GenericError", "desc": "Out-Of-Band capability requires that every command contains an 'id' field"}}

Quoted 1 line

Why does every command require 'id'?

The COMMAND_DROPPED event is one reason, as you mentioned in the other thread. Meanwhile as long as we have OOB command it means that we can have more than one commands running in parallel, then it's a must that we can mark that out in the response to mark out which command we are replying to.

Quoted 2 lines · Peter Xu

Quoted 1 line · Markus Armbruster

I fooled around a bit, and I think there are a few lose ends.

[...]

Quoted 11 lines

Quoted 11 lines

Talking to a QMP monitor that supports OOB:

     $ socat UNIX:test-qmp READLINE,history=$HOME/.qmp_history,prompt='QMP> '
     {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-1703-gb909799463"}, "capabilities": ["oob"]}}
     QMP> { "execute": "qmp_capabilities", "arguments": { "oob": true } }
     {"error": {"class": "GenericError", "desc": "Parameter 'oob' is unexpected"}}
     QMP> { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
     {"return": {}}
     QMP> { "execute": "query-qmp-schema" }
     {"error": {"class": "GenericError", "desc": "Out-Of-Band capability requires that every command contains an 'id' field"}}

Quoted 1 line

Quoted 1 line

Why does every command require 'id'?

Why am I asking? Requiring 'id' is rather onerous when you play with QMP by hand.

Possible solutions other than requiring "id":

1. Make out-of-band responses recognizable

Just like we copy "id" to the response, we could copy "run-oob", or maybe "control".

Solves the "match response to command" problem, doesn't solve the "match COMMAND_DROPPED event to command" problem.

Permit me a digression. "control": { "run-oob": true } is awfully verbose. Back when we hashed out the OOB design, I proposed to use "exec-oob" instead of "execute" to run a command OOB. I missed when that morphed into flag "run-oob" wrapped in object "control". Was it in response to reviewer feedback? Can you give me a pointer?

The counterpart to "exec-oob" would be "return-oob" and "error-oob". Hmm.

2. Do nothing; it's the client's problem

If the client needs to match responses and events to commands, it should use the feature that was expressly designed for helping with
that: "id".

Note that QMP's initial design assumed asynchronous commands, i.e. respones that may arrive in any order. Nevertheless, it did not require "id". Same reasoning: if the client needs to match, it can use "id".

As so often when "do nothing" is a viable solution, it looks mighty attractive to me :)

Quoted 2 lines · Peter Xu

Quoted 1 line · Markus Armbruster

I fooled around a bit, and I think there are a few lose ends.

[...]

Quoted 11 lines

Quoted 11 lines

Talking to a QMP monitor that supports OOB:

    $ socat UNIX:test-qmp READLINE,history=$HOME/.qmp_history,prompt='QMP> '
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-1703-gb909799463"}, "capabilities": ["oob"]}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "oob": true } }
    {"error": {"class": "GenericError", "desc": "Parameter 'oob' is unexpected"}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
    {"return": {}}
    QMP> { "execute": "query-qmp-schema" }
    {"error": {"class": "GenericError", "desc": "Out-Of-Band capability requires that every command contains an 'id' field"}}

Quoted 1 line

Quoted 1 line

Why does every command require 'id'?

Why am I asking? Requiring 'id' is rather onerous when you play with QMP by hand.

Quoted 5 lines

The COMMAND_DROPPED event is one reason, as you mentioned in the other thread. Meanwhile as long as we have OOB command it means that we can have more than one commands running in parallel, then it's a must that we can mark that out in the response to mark out which command we are replying to.

Without OOB, the client can match response to command, because it'll receive responses in command issue order.

Example 1: after sending

    { "execute": "cmd1", ... }
    { "execute": "cmd2", ... }
    { "execute": "cmd3", ... }

the client will receive three responses, first one is cmd1's, second one is cmd2's, third one is cmd3's.

With OOB, we have to independent command streams, in-band and out-of-band. Each separate stream's responses arrive in-order, but the two streams may be interleaved.

Example 2: after sending

    { "execute": "cmd1", ... }
    { "execute": "cmd2", ... }
    { "execute": "cmd3", ... }
    { "execute": "oob1", "control": { "run-oob": true }, ... }
    { "execute": "oob2", "control": { "run-oob": true }, ... }
    { "execute": "oob3", "control": { "run-oob": true }, ... }

the client will receive six responses: cmd1's before cmd2's before cmd3's, and oob1's before oob2's before oob3's.

If the client sends "id" with each command, it can match responses to commands by id.

But that's not the only way to match. Imagine the client had a perfect oracle to tell it whether a response is in-band or out-of-band. Then matching can work pretty much as in example 1: the first in-band response is cmd1's, the second one is cmd2's, and the third one is cmd3's; the first out-of-band response is oob1's, the second one is oob2's and the third one is oob3's.

Possible solutions other than requiring "id":

1. Make out-of-band responses recognizable

Just like we copy "id" to the response, we could copy "run-oob", or maybe "control".

Solves the "match response to command" problem, doesn't solve the "match COMMAND_DROPPED event to command" problem.

Permit me a digression. "control": { "run-oob": true } is awfully verbose. Back when we hashed out the OOB design, I proposed to use "exec-oob" instead of "execute" to run a command OOB. I missed when that morphed into flag "run-oob" wrapped in object "control". Was it in response to reviewer feedback? Can you give me a pointer?

The counterpart to "exec-oob" would be "return-oob" and "error-oob". Hmm.

2. Do nothing; it's the client's problem

If the client needs to match responses and events to commands, it should use the feature that was expressly designed for helping with
that: "id".

Note that QMP's initial design assumed asynchronous commands, i.e. respones that may arrive in any order. Nevertheless, it did not require "id". Same reasoning: if the client needs to match, it can use "id".

As so often when "do nothing" is a viable solution, it looks mighty attractive to me :)

[...]

Quoted 52 lines · Markus Armbruster

Quoted 2 lines · Peter Xu

Quoted 1 line · Markus Armbruster

I fooled around a bit, and I think there are a few lose ends.

[...]

Quoted 11 lines

Quoted 11 lines

Talking to a QMP monitor that supports OOB:

    $ socat UNIX:test-qmp READLINE,history=$HOME/.qmp_history,prompt='QMP> '
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-1703-gb909799463"}, "capabilities": ["oob"]}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "oob": true } }
    {"error": {"class": "GenericError", "desc": "Parameter 'oob' is unexpected"}}
    QMP> { "execute": "qmp_capabilities", "arguments": { "enable": ["oob"] } }
    {"return": {}}
    QMP> { "execute": "query-qmp-schema" }
    {"error": {"class": "GenericError", "desc": "Out-Of-Band capability requires that every command contains an 'id' field"}}

Quoted 1 line

Quoted 1 line

Why does every command require 'id'?

Why am I asking? Requiring 'id' is rather onerous when you play with QMP by hand.

Quoted 5 lines

The COMMAND_DROPPED event is one reason, as you mentioned in the other thread. Meanwhile as long as we have OOB command it means that we can have more than one commands running in parallel, then it's a must that we can mark that out in the response to mark out which command we are replying to.

Without OOB, the client can match response to command, because it'll receive responses in command issue order.

Example 1: after sending

    { "execute": "cmd1", ... }
    { "execute": "cmd2", ... }
    { "execute": "cmd3", ... }

the client will receive three responses, first one is cmd1's, second one is cmd2's, third one is cmd3's.

With OOB, we have to independent command streams, in-band and out-of-band. Each separate stream's responses arrive in-order, but the two streams may be interleaved.

Example 2: after sending

    { "execute": "cmd1", ... }
    { "execute": "cmd2", ... }
    { "execute": "cmd3", ... }
    { "execute": "oob1", "control": { "run-oob": true }, ... }
    { "execute": "oob2", "control": { "run-oob": true }, ... }
    { "execute": "oob3", "control": { "run-oob": true }, ... }

Quoted 2 lines

the client will receive six responses: cmd1's before cmd2's before cmd3's, and oob1's before oob2's before oob3's.

I thought the intention was that oob1, oob2, and oob3 are defined to return in any order. It just happens that we only hve a single oob processing stream right now, but we may have more in future.

Quoted 10 lines

If the client sends "id" with each command, it can match responses to commands by id.

But that's not the only way to match. Imagine the client had a perfect oracle to tell it whether a response is in-band or out-of-band. Then matching can work pretty much as in example 1: the first in-band response is cmd1's, the second one is cmd2's, and the third one is cmd3's; the first out-of-band response is oob1's, the second one is oob2's and the third one is oob3's.

Not if oob1/2/3 can retunr in any order in future, which I think we should be allowing fore.

IMHO 'id' should be mandatory for oob usage.

Quoted 15 lines

Possible solutions other than requiring "id":

1. Make out-of-band responses recognizable

Just like we copy "id" to the response, we could copy "run-oob", or maybe "control".

Solves the "match response to command" problem, doesn't solve the "match COMMAND_DROPPED event to command" problem.

Permit me a digression. "control": { "run-oob": true } is awfully verbose. Back when we hashed out the OOB design, I proposed to use "exec-oob" instead of "execute" to run a command OOB. I missed when that morphed into flag "run-oob" wrapped in object "control". Was it in response to reviewer feedback? Can you give me a pointer?

Quoted 3 lines

The counterpart to "exec-oob" would be "return-oob" and "error-oob". Hmm.

I do prefer the less verbose proposal too.

Quoted 11 lines

2. Do nothing; it's the client's problem

If the client needs to match responses and events to commands, it should use the feature that was expressly designed for helping with
that: "id".

Note that QMP's initial design assumed asynchronous commands, i.e. respones that may arrive in any order. Nevertheless, it did not require "id". Same reasoning: if the client needs to match, it can use "id".

IMHO we should just mandate 'id' when using "exec-oob", as it is conceptually broken to use OOB without a way to match responses. We don't want clients relying on all OOB replies coming in sequence now, and then breaking when we allow multiple OOB processing threads. That would require us to add a eec-oob-no-really-i-mean-it command to allow overlapping OOB responses later.

Quoted 5 lines

Quoted 5 lines

Permit me a digression. "control": { "run-oob": true } is awfully verbose. Back when we hashed out the OOB design, I proposed to use "exec-oob" instead of "execute" to run a command OOB. I missed when that morphed into flag "run-oob" wrapped in object "control". Was it in response to reviewer feedback? Can you give me a pointer?

It's me who proposed this, not from any reviewer's feedback. It just happened that no one was against it.

Quoted 6 lines

Quoted 4 lines

The counterpart to "exec-oob" would be "return-oob" and "error-oob". Hmm.

I do prefer the less verbose proposal too.

But frankly speaking I still prefer current way. QMP is majorly for clients (computer programs) rather than users to use it. Comparing to verbosity, I would care more about coherency for how we define the interface. Say, OOB execution is still one way to execute a command, so naturally it should still be using the same way that we execute a QMP command, we just need some extra fields to tell the server that "this message is more urgent than the other ones".

If we are discussing HMP interfaces, I'll have the same preference with both of you to consider more about whether it's user-friendly or not. But now we are talking about QMP, then I'll prefer "control".

In the future, it's also possible to add some more sub-fields into the "control" field. When that happens, do we want to introduce another standalone wording for that? I would assume the answer is no.

More lose ends:

* scripts/qmp/ doesn't support OOB, yet. qmp-shell.py in particular

* test-qmp-cmds neglects to cover the OOB additions to qmp-dispatch.c

Quoted 4 lines · Markus Armbruster

More lose ends:

* scripts/qmp/ doesn't support OOB, yet. qmp-shell.py in particular

Quoted 1 line

* test-qmp-cmds neglects to cover the OOB additions to qmp-dispatch.c

Would you mind I put these aside for now?

I'm afraid things grow then we lose control, so not sure whether I can just focus on the bugs first (e.g., the COMMAND_DROP event broadcasting, and also the response queue flow control issue).