[Qemu-devel] [PATCH v3 0/6] monitor: enable OOB by default

[Qemu-devel] [PATCH v3 0/6] monitor: enable OOB by default

[Peter Xu]

Patches 1-3 are new. I am not sure about patch 3; I hope current hack
works for us.

v3:
- drop patch "tests: iotests: don't compare SHUTDOWN event", replace
  it with "monitor: flush qmp responses when CLOSED" to fix up the
  race. [Eric, Markus]
- tweak the oob revert patch to not break qmp-test [Eric]
- quite a few comment and commit message fix-ups [Eric]
- add more comment in commit message, mention about why MUX can't be
  used for Out-Of-Band [Markus]
- one new patch to comment on chardev CLOSED event [Stefan]
- one new patch to fix iotest breakage on 060 specifically

Tests: make check, iotests

Please review.  Thanks,

Peter Xu (6):
  chardev: comment details for CLOSED event
  monitor: flush qmp responses when CLOSED
  tests: iotests: drop some stderr line
  docs: mention shared state protect for OOB
  monitor: remove "x-oob", turn oob on by default
  Revert "tests: Add parameter to qtest_init_without_qmp_handshake"

 docs/devel/qapi-code-gen.txt |  3 +++
 include/chardev/char.h       | 11 ++++++++-
 include/monitor/monitor.h    |  1 -
 tests/libqtest.h             |  4 +--
 monitor.c                    | 47 ++++++++++++++++++++----------------
 tests/libqtest.c             | 10 +++-----
 tests/qmp-test.c             |  6 ++---
 vl.c                         |  5 ----
 tests/qemu-iotests/060       | 10 +++++++-
 tests/qemu-iotests/060.out   |  1 -
 10 files changed, 56 insertions(+), 42 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v3 1/6] chardev: comment details for CLOSED event

[Peter Xu]

It was unclear before on what does the CLOSED event mean.  Meanwhile we
add a TODO to fix up the CLOSED event in the future when the in/out
ports are different for a chardev.

CC: Paolo Bonzini <pbonzini@redhat.com>
CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/chardev/char.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/chardev/char.h b/include/chardev/char.h
index 04de45795e..6f0576e214 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -22,7 +22,16 @@ typedef enum {
     CHR_EVENT_OPENED, /* new connection established */
     CHR_EVENT_MUX_IN, /* mux-focus was set to this terminal */
     CHR_EVENT_MUX_OUT, /* mux-focus will move on */
-    CHR_EVENT_CLOSED /* connection closed */
+    CHR_EVENT_CLOSED /* connection closed.  NOTE: currently this event
+                      * is only bound to the read port of the chardev.
+                      * Normally the read port and write port of a
+                      * chardev should be the same, but it can be
+                      * different, e.g., for fd chardevs, when the two
+                      * fds are different.  So when we received the
+                      * CLOSED event it's still possible that the out
+                      * port is still open.  TODO: we should only send
+                      * the CLOSED event when both ports are closed.
+                      */
 } QEMUChrEvent;
 
 #define CHR_READ_BUF_LEN 4096
-- 
2.17.1

Re: [Qemu-devel] [PATCH v3 1/6] chardev: comment details for CLOSED event

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> It was unclear before on what does the CLOSED event mean.  Meanwhile we
> add a TODO to fix up the CLOSED event in the future when the in/out
> ports are different for a chardev.
>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/chardev/char.h | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 04de45795e..6f0576e214 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -22,7 +22,16 @@ typedef enum {
>      CHR_EVENT_OPENED, /* new connection established */
>      CHR_EVENT_MUX_IN, /* mux-focus was set to this terminal */
>      CHR_EVENT_MUX_OUT, /* mux-focus will move on */
> -    CHR_EVENT_CLOSED /* connection closed */
> +    CHR_EVENT_CLOSED /* connection closed.  NOTE: currently this event
> +                      * is only bound to the read port of the chardev.
> +                      * Normally the read port and write port of a
> +                      * chardev should be the same, but it can be
> +                      * different, e.g., for fd chardevs, when the two
> +                      * fds are different.  So when we received the
> +                      * CLOSED event it's still possible that the out
> +                      * port is still open.  TODO: we should only send
> +                      * the CLOSED event when both ports are closed.
> +                      */
>  } QEMUChrEvent;
>  
>  #define CHR_READ_BUF_LEN 4096

Undefined terms "read port" and "write port".  But the header is full of
undefined terms, like "front end", "back end", "data channel", "chardev
peer", ...  It could use a file comment to tie things together.  Clearly
out of scope for this series.

Re: [Qemu-devel] [PATCH v3 1/6] chardev: comment details for CLOSED event

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 641 bytes --]

On Fri, Jun 15, 2018 at 09:42:44AM +0800, Peter Xu wrote:
> It was unclear before on what does the CLOSED event mean.  Meanwhile we
> add a TODO to fix up the CLOSED event in the future when the in/out
> ports are different for a chardev.
> 
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/chardev/char.h | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH v3 2/6] monitor: flush qmp responses when CLOSED

[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.  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 should possibly fail
anyways (just imagine to write to a socket that has already closed).
However there can be cases where in & out ports of the QMP monitor's
backend chardev are separated.  One true 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 a very strange SHUTDOWN event missing when
running test with iotest 087 with Out-Of-Band enabled. One condition
could be this (after "quit" command is executed and QEMU quits the main
loop):

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

2. "cat" terminates (to distinguish it from the animal, I quote it).
   Logically it can terminate even earlier, but let's just assume it's
   here.

3. [monitor iothread] QEMU reads EOF from stdin, which connects to the
   "cat" process

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

5. [main thread] clean 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:

087 8s ... - output mismatch (see 087.out.bad)
--- /home/peterx/git/qemu/tests/qemu-iotests/087.out    2018-06-01 18:44:22.378982462 +0800
+++ /home/peterx/git/qemu/bin/tests/qemu-iotests/087.out.bad    2018-06-01 18:53:44.267840928 +0800
@@ -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 <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>

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

diff --git a/monitor.c b/monitor.c
index 6d0cec552e..e59d4f09ac 100644
--- a/monitor.c
+++ b/monitor.c
@@ -512,20 +512,39 @@ 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);
+    }
+}
+
 /*
  * Return one QMPResponse.  The response is only valid if
  * response.data is not NULL.
  */
-static QMPResponse monitor_qmp_response_pop_one(void)
+static QMPResponse monitor_qmp_response_pop(void)
 {
     Monitor *mon;
     QObject *data = NULL;
 
     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) {
             break;
         }
@@ -539,7 +558,7 @@ static void monitor_qmp_bh_responder(void *opaque)
     QMPResponse response;
 
     while (true) {
-        response = monitor_qmp_response_pop_one();
+        response = monitor_qmp_response_pop();
         if (!response.data) {
             break;
         }
@@ -4366,6 +4385,7 @@ static void monitor_qmp_event(void *opaque, int event)
         mon_refcount++;
         break;
     case CHR_EVENT_CLOSED:
+        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);
-- 
2.17.1

Re: [Qemu-devel] [PATCH v3 2/6] monitor: flush qmp responses when CLOSED

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> 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.  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 should possibly fail

s/should possibly fail anyways/will fail anyway/

> anyways (just imagine to write to a socket that has already closed).
> However there can be cases where in & out ports of the QMP monitor's
> backend chardev are separated.  One true example:

Suggest to drop "true", or say "Here's 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 a very strange SHUTDOWN event missing when

The event isn't strange, its missing is (sort of).  Suggest to drop
"very strange".

> running test with iotest 087 with Out-Of-Band enabled. One condition
> could be this (after "quit" command is executed and QEMU quits the main
> loop):

"could be" suggests this may or may not demonstrate the loss of the
event.  Misleading.  "Here's one of the ways this can happen" would be
clearer.

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

s/one/a/

>
> 2. "cat" terminates (to distinguish it from the animal, I quote it).
>    Logically it can terminate even earlier, but let's just assume it's
>    here.

Suggest to drop the second sentence.  You're describing one way for the
bug to bite here, not all possible ways.

>
> 3. [monitor iothread] QEMU reads EOF from stdin, which connects to the
>    "cat" process

Suggest "QEMU's monitor iothread reads".

Suggest to drop ", which connects...", because that should be obvious.

> 4. [monitor iothread] QEMU calls the CLOSED event hook for the monitor,

"QEMU's monitor iothread" again.

>    which will clean up the response queue of the monitor, then the
>    SHUTDOWN event is dropped

Suggest to say "destroy" instead of "clean up", to make perfectly clear
that we're throwing away the queue.
>
> 5. [main thread] clean up the monitors in monitor_cleanup(), when

"QEMU's main thread cleans up the monitors in monitor_cleanup().  When"

>    trying to flush pending responses, it sees nothing.  SHUTDOWN is
>    lost forever

Missing a period.

> 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:
>
> 087 8s ... - output mismatch (see 087.out.bad)
> --- /home/peterx/git/qemu/tests/qemu-iotests/087.out    2018-06-01 18:44:22.378982462 +0800
> +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/087.out.bad    2018-06-01 18:53:44.267840928 +0800
> @@ -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}}

Suggest to 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.

> This patch fixes the problem.
>
> Fixes: 6d2d563f8c ("qmp: cleanup qmp queues properly", 2018-03-27)
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Don't let my nitpicking deceive you; this is a really nice commit
message.

One more suggestion.  Your first paragraph describes the solution.  I
like to start with the problem instead.  More so when the problem
description is long, as it is here, because by the time I'm done reading
the problem, I'm prone to have forgotten the solution, and go "okay, and
how's the patch addressing the problem?"  But please use your judgement.

> ---
>  monitor.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 6d0cec552e..e59d4f09ac 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -512,20 +512,39 @@ 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;
> +}

Note for later: returns null only when the queue is empty (we never push
null onto this queue).

> +
> +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);
> +    }
> +}

This flushes the queue to @mon.  Good.

> +
>  /*
>   * Return one QMPResponse.  The response is only valid if
>   * response.data is not NULL.
>   */
> -static QMPResponse monitor_qmp_response_pop_one(void)
> +static QMPResponse monitor_qmp_response_pop(void)
>  {
>      Monitor *mon;
>      QObject *data = NULL;
>  
>      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) {
>              break;
>          }
       }
       qemu_mutex_unlock(&monitor_lock);
       return (QMPResponse) { .mon = mon, .data = data };
   }

Not this patch's fault, but this function gave me pause until I got the
loop's purpose: the function returns a response from *any* monitor.

Moreover, your naming creates an inconsistency with
monitor_qmp_requests_pop_one().

I append my attempt to make it easier to understand, and restore
consistency.  I guess I'd split it into a preparatory patch and a fixup
for this one.

> @@ -539,7 +558,7 @@ static void monitor_qmp_bh_responder(void *opaque)
>      QMPResponse response;
>  
>      while (true) {
> -        response = monitor_qmp_response_pop_one();
> +        response = monitor_qmp_response_pop();
>          if (!response.data) {
>              break;
>          }
> @@ -4366,6 +4385,7 @@ static void monitor_qmp_event(void *opaque, int event)
>          mon_refcount++;
>          break;
>      case CHR_EVENT_CLOSED:
> +        monitor_qmp_response_flush(mon);

This wastes work in the common case when @mon's output fd has been
closed already.  I think that's just fine.

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


diff --git a/monitor.c b/monitor.c
index e59d4f09ac..af28977549 100644
--- a/monitor.c
+++ b/monitor.c
@@ -512,7 +512,7 @@ struct QMPResponse {
 };
 typedef struct QMPResponse QMPResponse;
 
-static QObject *monitor_qmp_response_pop_one(Monitor *mon)
+static QObject *monitor_qmp_requests_pop(Monitor *mon)
 {
     QObject *data;
 
@@ -527,41 +527,39 @@ static void monitor_qmp_response_flush(Monitor *mon)
 {
     QObject *data;
 
-    while ((data = monitor_qmp_response_pop_one(mon))) {
+    while ((data = monitor_qmp_requests_pop(mon))) {
         monitor_json_emitter_raw(mon, data);
         qobject_unref(data);
     }
 }
 
 /*
- * Return one QMPResponse.  The response is only valid if
- * response.data is not NULL.
+ * Pop a QMPResponse from any monitor's response queue into @response.
+ * Return NULL when all queues are empty, else @response.
  */
-static QMPResponse monitor_qmp_response_pop(void)
+static QMPResponse *monitor_qmp_response_pop_any(QMPResponse *response)
 {
     Monitor *mon;
     QObject *data = NULL;
 
     qemu_mutex_lock(&monitor_lock);
     QTAILQ_FOREACH(mon, &mon_list, entry) {
-        data = monitor_qmp_response_pop_one(mon);
+        data = monitor_qmp_requests_pop(mon);
         if (data) {
+            response->mon = mon;
+            response->data = data;
             break;
         }
     }
     qemu_mutex_unlock(&monitor_lock);
-    return (QMPResponse) { .mon = mon, .data = data };
+    return data ? response : NULL;
 }
 
 static void monitor_qmp_bh_responder(void *opaque)
 {
     QMPResponse response;
 
-    while (true) {
-        response = monitor_qmp_response_pop();
-        if (!response.data) {
-            break;
-        }
+    while (monitor_qmp_response_pop_any(&response)) {
         monitor_json_emitter_raw(response.mon, response.data);
         qobject_unref(response.data);
     }
@@ -4126,7 +4124,7 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
  * when we process one request on a specific monitor, we put that
  * monitor to the end of mon_list queue.
  */
-static QMPRequest *monitor_qmp_requests_pop_one(void)
+static QMPRequest *monitor_qmp_requests_pop_any(void)
 {
     QMPRequest *req_obj = NULL;
     Monitor *mon;
@@ -4158,7 +4156,7 @@ static QMPRequest *monitor_qmp_requests_pop_one(void)
 
 static void monitor_qmp_bh_dispatcher(void *data)
 {
-    QMPRequest *req_obj = monitor_qmp_requests_pop_one();
+    QMPRequest *req_obj = monitor_qmp_requests_pop_any();
 
     if (req_obj) {
         trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");

Re: [Qemu-devel] [PATCH v3 2/6] monitor: flush qmp responses when CLOSED

[Peter Xu]

On Fri, Jun 15, 2018 at 10:11:34AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > 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.  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 should possibly fail
> 
> s/should possibly fail anyways/will fail anyway/
> 
> > anyways (just imagine to write to a socket that has already closed).
> > However there can be cases where in & out ports of the QMP monitor's
> > backend chardev are separated.  One true example:
> 
> Suggest to drop "true", or say "Here's 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 a very strange SHUTDOWN event missing when
> 
> The event isn't strange, its missing is (sort of).  Suggest to drop
> "very strange".
> 
> > running test with iotest 087 with Out-Of-Band enabled. One condition
> > could be this (after "quit" command is executed and QEMU quits the main
> > loop):
> 
> "could be" suggests this may or may not demonstrate the loss of the
> event.  Misleading.  "Here's one of the ways this can happen" would be
> clearer.
> 
> >
> > 1. [main thread] QEMU queues one SHUTDOWN event into response queue
> 
> s/one/a/
> 
> >
> > 2. "cat" terminates (to distinguish it from the animal, I quote it).
> >    Logically it can terminate even earlier, but let's just assume it's
> >    here.
> 
> Suggest to drop the second sentence.  You're describing one way for the
> bug to bite here, not all possible ways.
> 
> >
> > 3. [monitor iothread] QEMU reads EOF from stdin, which connects to the
> >    "cat" process
> 
> Suggest "QEMU's monitor iothread reads".
> 
> Suggest to drop ", which connects...", because that should be obvious.
> 
> > 4. [monitor iothread] QEMU calls the CLOSED event hook for the monitor,
> 
> "QEMU's monitor iothread" again.
> 
> >    which will clean up the response queue of the monitor, then the
> >    SHUTDOWN event is dropped
> 
> Suggest to say "destroy" instead of "clean up", to make perfectly clear
> that we're throwing away the queue.
> >
> > 5. [main thread] clean up the monitors in monitor_cleanup(), when
> 
> "QEMU's main thread cleans up the monitors in monitor_cleanup().  When"
> 
> >    trying to flush pending responses, it sees nothing.  SHUTDOWN is
> >    lost forever
> 
> Missing a period.
> 
> > 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:
> >
> > 087 8s ... - output mismatch (see 087.out.bad)
> > --- /home/peterx/git/qemu/tests/qemu-iotests/087.out    2018-06-01 18:44:22.378982462 +0800
> > +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/087.out.bad    2018-06-01 18:53:44.267840928 +0800
> > @@ -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}}
> 
> Suggest to 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.
> 
> > This patch fixes the problem.
> >
> > Fixes: 6d2d563f8c ("qmp: cleanup qmp queues properly", 2018-03-27)
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Don't let my nitpicking deceive you; this is a really nice commit
> message.

Thanks.  I'll apply all the suggestions until here in my next post.

> 
> One more suggestion.  Your first paragraph describes the solution.  I
> like to start with the problem instead.  More so when the problem
> description is long, as it is here, because by the time I'm done reading
> the problem, I'm prone to have forgotten the solution, and go "okay, and
> how's the patch addressing the problem?"  But please use your judgement.

Yes, talking about the problem first makes sense.  But I'll try to
make it even easier for readers: I'll summarize the problem and
solution first, then I'll keep the rest of the commit message as
detailed description of the problem.  So I'll rewrite my first
paragraph as:

  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.

Hope this works.

> 
> > ---
> >  monitor.c | 30 +++++++++++++++++++++++++-----
> >  1 file changed, 25 insertions(+), 5 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 6d0cec552e..e59d4f09ac 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -512,20 +512,39 @@ 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;
> > +}
> 
> Note for later: returns null only when the queue is empty (we never push
> null onto this queue).
> 
> > +
> > +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);
> > +    }
> > +}
> 
> This flushes the queue to @mon.  Good.
> 
> > +
> >  /*
> >   * Return one QMPResponse.  The response is only valid if
> >   * response.data is not NULL.
> >   */
> > -static QMPResponse monitor_qmp_response_pop_one(void)
> > +static QMPResponse monitor_qmp_response_pop(void)
> >  {
> >      Monitor *mon;
> >      QObject *data = NULL;
> >  
> >      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) {
> >              break;
> >          }
>        }
>        qemu_mutex_unlock(&monitor_lock);
>        return (QMPResponse) { .mon = mon, .data = data };
>    }
> 
> Not this patch's fault, but this function gave me pause until I got the
> loop's purpose: the function returns a response from *any* monitor.
> 
> Moreover, your naming creates an inconsistency with
> monitor_qmp_requests_pop_one().
> 
> I append my attempt to make it easier to understand, and restore
> consistency.  I guess I'd split it into a preparatory patch and a fixup
> for this one.

Will do.

> 
> > @@ -539,7 +558,7 @@ static void monitor_qmp_bh_responder(void *opaque)
> >      QMPResponse response;
> >  
> >      while (true) {
> > -        response = monitor_qmp_response_pop_one();
> > +        response = monitor_qmp_response_pop();
> >          if (!response.data) {
> >              break;
> >          }
> > @@ -4366,6 +4385,7 @@ static void monitor_qmp_event(void *opaque, int event)
> >          mon_refcount++;
> >          break;
> >      case CHR_EVENT_CLOSED:
> > +        monitor_qmp_response_flush(mon);
> 
> This wastes work in the common case when @mon's output fd has been
> closed already.  I think that's just fine.

Yeah.  Since I'll repost, I consider to add some comment for that.

> 
> >          monitor_qmp_cleanup_queues(mon);
> >          json_message_parser_destroy(&mon->qmp.parser);
> >          json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
> 
> 
> diff --git a/monitor.c b/monitor.c
> index e59d4f09ac..af28977549 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -512,7 +512,7 @@ struct QMPResponse {
>  };
>  typedef struct QMPResponse QMPResponse;
>  
> -static QObject *monitor_qmp_response_pop_one(Monitor *mon)
> +static QObject *monitor_qmp_requests_pop(Monitor *mon)

I guess it's a typo; I'll keep the "response" word.

>  {
>      QObject *data;
>  
> @@ -527,41 +527,39 @@ static void monitor_qmp_response_flush(Monitor *mon)
>  {
>      QObject *data;
>  
> -    while ((data = monitor_qmp_response_pop_one(mon))) {
> +    while ((data = monitor_qmp_requests_pop(mon))) {

Same here.

>          monitor_json_emitter_raw(mon, data);
>          qobject_unref(data);
>      }
>  }
>  
>  /*
> - * Return one QMPResponse.  The response is only valid if
> - * response.data is not NULL.
> + * Pop a QMPResponse from any monitor's response queue into @response.
> + * Return NULL when all queues are empty, else @response.

I'll change the return value to a boolean directly if it's okay, since
after all we'll pass the QMPResponse in now.

I'll put the rest of the content into a standalone patch.

Thanks,

>   */
> -static QMPResponse monitor_qmp_response_pop(void)
> +static QMPResponse *monitor_qmp_response_pop_any(QMPResponse *response)
>  {
>      Monitor *mon;
>      QObject *data = NULL;
>  
>      qemu_mutex_lock(&monitor_lock);
>      QTAILQ_FOREACH(mon, &mon_list, entry) {
> -        data = monitor_qmp_response_pop_one(mon);
> +        data = monitor_qmp_requests_pop(mon);
>          if (data) {
> +            response->mon = mon;
> +            response->data = data;
>              break;
>          }
>      }
>      qemu_mutex_unlock(&monitor_lock);
> -    return (QMPResponse) { .mon = mon, .data = data };
> +    return data ? response : NULL;
>  }
>  
>  static void monitor_qmp_bh_responder(void *opaque)
>  {
>      QMPResponse response;
>  
> -    while (true) {
> -        response = monitor_qmp_response_pop();
> -        if (!response.data) {
> -            break;
> -        }
> +    while (monitor_qmp_response_pop_any(&response)) {
>          monitor_json_emitter_raw(response.mon, response.data);
>          qobject_unref(response.data);
>      }
> @@ -4126,7 +4124,7 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
>   * when we process one request on a specific monitor, we put that
>   * monitor to the end of mon_list queue.
>   */
> -static QMPRequest *monitor_qmp_requests_pop_one(void)
> +static QMPRequest *monitor_qmp_requests_pop_any(void)
>  {
>      QMPRequest *req_obj = NULL;
>      Monitor *mon;
> @@ -4158,7 +4156,7 @@ static QMPRequest *monitor_qmp_requests_pop_one(void)
>  
>  static void monitor_qmp_bh_dispatcher(void *data)
>  {
> -    QMPRequest *req_obj = monitor_qmp_requests_pop_one();
> +    QMPRequest *req_obj = monitor_qmp_requests_pop_any();
>  
>      if (req_obj) {
>          trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 2/6] monitor: flush qmp responses when CLOSED

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Jun 15, 2018 at 10:11:34AM +0200, Markus Armbruster wrote:
[...]
>> diff --git a/monitor.c b/monitor.c
>> index e59d4f09ac..af28977549 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -512,7 +512,7 @@ struct QMPResponse {
>>  };
>>  typedef struct QMPResponse QMPResponse;
>>  
>> -static QObject *monitor_qmp_response_pop_one(Monitor *mon)
>> +static QObject *monitor_qmp_requests_pop(Monitor *mon)
>
> I guess it's a typo; I'll keep the "response" word.

You're right.

>>  {
>>      QObject *data;
>>  
>> @@ -527,41 +527,39 @@ static void monitor_qmp_response_flush(Monitor *mon)
>>  {
>>      QObject *data;
>>  
>> -    while ((data = monitor_qmp_response_pop_one(mon))) {
>> +    while ((data = monitor_qmp_requests_pop(mon))) {
>
> Same here.
>
>>          monitor_json_emitter_raw(mon, data);
>>          qobject_unref(data);
>>      }
>>  }
>>  
>>  /*
>> - * Return one QMPResponse.  The response is only valid if
>> - * response.data is not NULL.
>> + * Pop a QMPResponse from any monitor's response queue into @response.
>> + * Return NULL when all queues are empty, else @response.
>
> I'll change the return value to a boolean directly if it's okay, since
> after all we'll pass the QMPResponse in now.

I routinely return the buffer argument in cases like this.  It's quite
traditional, e.g. fgets(), memcpy(), strcpy() do it, too.  Occasionally
permits more concise code, but not here.

[...]

[Qemu-devel] [PATCH v3 3/6] tests: iotests: drop some stderr line

[Peter Xu]

In my Out-Of-Band test, "check -qcow2 060" fail with this:

060 5s ... - output mismatch (see 060.out.bad)
--- /home/peterx/git/qemu/tests/qemu-iotests/060.out    2018-06-15 08:31:14.607411950 +0800
+++ /home/peterx/git/qemu/bin/tests/qemu-iotests/060.out.bad    2018-06-15 08:33:09.679880113 +0800
@@ -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}}
Failures: 060
Failed 1 of 1 tests

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 <peterx@redhat.com>

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/qemu-iotests/060     | 10 +++++++++-
 tests/qemu-iotests/060.out |  1 -
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 7bdf609f3f..74ad371885 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -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
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index bff023d889..d67c6234a4 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -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}}
-- 
2.17.1

Re: [Qemu-devel] [PATCH v3 3/6] tests: iotests: drop some stderr line

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> In my Out-Of-Band test, "check -qcow2 060" fail with this:
>
> 060 5s ... - output mismatch (see 060.out.bad)
> --- /home/peterx/git/qemu/tests/qemu-iotests/060.out    2018-06-15 08:31:14.607411950 +0800
> +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/060.out.bad    2018-06-15 08:33:09.679880113 +0800
> @@ -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}}
> Failures: 060
> Failed 1 of 1 tests

Please indent this diff; git-am chokes on it for me.

> 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 <peterx@redhat.com>
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Lose one S-o-b, please :)

Re: [Qemu-devel] [PATCH v3 3/6] tests: iotests: drop some stderr line

[Peter Xu]

On Fri, Jun 15, 2018 at 10:13:07AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > In my Out-Of-Band test, "check -qcow2 060" fail with this:
> >
> > 060 5s ... - output mismatch (see 060.out.bad)
> > --- /home/peterx/git/qemu/tests/qemu-iotests/060.out    2018-06-15 08:31:14.607411950 +0800
> > +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/060.out.bad    2018-06-15 08:33:09.679880113 +0800
> > @@ -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}}
> > Failures: 060
> > Failed 1 of 1 tests
> 
> Please indent this diff; git-am chokes on it for me.

Will do.

> 
> > 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 <peterx@redhat.com>
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Lose one S-o-b, please :)

Yeah. :)

-- 
Peter Xu

[Qemu-devel] [PATCH v3 4/6] docs: mention shared state protect for OOB

[Peter Xu]

Out-Of-Band handlers need to protect shared state if there is any.
Mention it in the document.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 docs/devel/qapi-code-gen.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 1366228b2a..bee9de35df 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -680,6 +680,9 @@ OOB command handlers must satisfy the following conditions:
 - 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.
+- It needs to protect any shared state, since as long as a command
+  supports Out-Of-Band it means the handler can be run in parallel
+  with the same handler running in the other thread.
 
 If in doubt, do not implement OOB execution support.
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH v3 4/6] docs: mention shared state protect for OOB

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Out-Of-Band handlers need to protect shared state if there is any.
> Mention it in the document.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  docs/devel/qapi-code-gen.txt | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 1366228b2a..bee9de35df 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -680,6 +680,9 @@ OOB command handlers must satisfy the following conditions:
   Under normal QMP command execution, the following apply to each
   command:

   - They are executed in order,
   - They run only in main thread of QEMU,
   - They have the BQL taken during execution.

Not this patch's fault, but this sounds awkward.  Perhaps "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 terminates quickly"

   - 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 takes only "fast" locks, i.e. all critical sections protected by any
lock it takes also satisfy the conditions for OOB command handler code."
Maybe make it the last item.

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

All these are corollaries of the first item.  But that's okay.

> +- It needs to protect any shared state, since as long as a command
> +  supports Out-Of-Band it means the handler can be run in parallel
> +  with the same handler running in the other thread.

"in another thread"

Not just the same handler is a potential problem.  Any code accessing
shared state from another thread is.

"It needs" is not really a condition.

Perhaps we can make this a separate paragraph rather than an additional
item:

   The restrictions on locking limit access to shared state.  Such
   access requires synchronization, but OOB commands can't take the BQL
   or any other "slow" lock.

>  If in doubt, do not implement OOB execution support.

Re: [Qemu-devel] [PATCH v3 4/6] docs: mention shared state protect for OOB

[Peter Xu]

On Fri, Jun 15, 2018 at 02:37:49PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Out-Of-Band handlers need to protect shared state if there is any.
> > Mention it in the document.
> >
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  docs/devel/qapi-code-gen.txt | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> > index 1366228b2a..bee9de35df 100644
> > --- a/docs/devel/qapi-code-gen.txt
> > +++ b/docs/devel/qapi-code-gen.txt
> > @@ -680,6 +680,9 @@ OOB command handlers must satisfy the following conditions:
>    Under normal QMP command execution, the following apply to each
>    command:
> 
>    - They are executed in order,
>    - They run only in main thread of QEMU,
>    - They have the BQL taken during execution.
> 
> Not this patch's fault, but this sounds awkward.  Perhaps "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 terminates quickly"
> 
>    - 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 takes only "fast" locks, i.e. all critical sections protected by any
> lock it takes also satisfy the conditions for OOB command handler code."
> Maybe make it the last item.
> 
> >  - 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.
> 
> All these are corollaries of the first item.  But that's okay.
> 
> > +- It needs to protect any shared state, since as long as a command
> > +  supports Out-Of-Band it means the handler can be run in parallel
> > +  with the same handler running in the other thread.
> 
> "in another thread"
> 
> Not just the same handler is a potential problem.  Any code accessing
> shared state from another thread is.
> 
> "It needs" is not really a condition.
> 
> Perhaps we can make this a separate paragraph rather than an additional
> item:
> 
>    The restrictions on locking limit access to shared state.  Such
>    access requires synchronization, but OOB commands can't take the BQL
>    or any other "slow" lock.

Yes this looks good to me.  I'll apply the rest of suggestions in my
next post along with this patch.  I'll touch up the commit message a
bit too.

Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH v3 5/6] monitor: remove "x-oob", turn oob on by default

[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.

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 <peterx@redhat.com>
---
 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(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index d6ab70cae2..0cb0538a31 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -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);
 
diff --git a/monitor.c b/monitor.c
index e59d4f09ac..860c9588b7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4582,19 +4582,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) {
-            error_report("Monitor Out-Of-band is only supported by QMP");
-            exit(1);
-        }
-    }
+    bool use_oob = (flags & MONITOR_USE_CONTROL) && !CHARDEV_IS_MUX(chr);
 
     monitor_data_init(mon, false, use_oob);
 
@@ -4696,9 +4684,6 @@ QemuOptsList qemu_mon_opts = {
         },{
             .name = "pretty",
             .type = QEMU_OPT_BOOL,
-        },{
-            .name = "x-oob",
-            .type = QEMU_OPT_BOOL,
         },
         { /* end of list */ }
     },
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 098af6aec4..c5cb3f925c 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -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);
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index a49cbc6fde..3747bf7fbb 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -89,7 +89,7 @@ static void test_qmp_protocol(void)
     g_assert(q);
     test_version(qdict_get(q, "version"));
     capabilities = qdict_get_qlist(q, "capabilities");
-    g_assert(capabilities && qlist_empty(capabilities));
+    g_assert(capabilities);
     qobject_unref(resp);
 
     /* Test valid command before handshake */
diff --git a/vl.c b/vl.c
index 6e34fb348d..26a0bb3f0f 100644
--- a/vl.c
+++ b/vl.c
@@ -2307,11 +2307,6 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
     if (qemu_opt_get_bool(opts, "pretty", 0))
         flags |= MONITOR_USE_PRETTY;
 
-    /* OOB is off by default */
-    if (qemu_opt_get_bool(opts, "x-oob", 0)) {
-        flags |= MONITOR_USE_OOB;
-    }
-
     chardev = qemu_opt_get(opts, "chardev");
     chr = qemu_chr_find(chardev);
     if (chr == NULL) {
-- 
2.17.1

[Qemu-devel] [PATCH v3 6/6] Revert "tests: Add parameter to qtest_init_without_qmp_handshake"

[Peter Xu]

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 <peterx@redhat.com>
---
 tests/libqtest.h |  4 +---
 tests/libqtest.c | 10 ++++------
 tests/qmp-test.c |  4 ++--
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index ac52872cbe..180d2cc6ff 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -56,14 +56,12 @@ QTestState *qtest_init(const char *extra_args);
 
 /**
  * qtest_init_without_qmp_handshake:
- * @use_oob: true to have the server advertise OOB support
  * @extra_args: other arguments to pass to QEMU.  CAUTION: these
  * arguments are subject to word splitting and shell evaluation.
  *
  * Returns: #QTestState instance.
  */
-QTestState *qtest_init_without_qmp_handshake(bool use_oob,
-                                             const char *extra_args);
+QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
 
 /**
  * qtest_quit:
diff --git a/tests/libqtest.c b/tests/libqtest.c
index c5cb3f925c..51af4a289e 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -173,8 +173,7 @@ static const char *qtest_qemu_binary(void)
     return qemu_bin;
 }
 
-QTestState *qtest_init_without_qmp_handshake(bool use_oob,
-                                             const char *extra_args)
+QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
 {
     QTestState *s;
     int sock, qmpsock, i;
@@ -207,13 +206,12 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
         command = g_strdup_printf("exec %s "
                                   "-qtest unix:%s,nowait "
                                   "-qtest-log %s "
-                                  "-chardev socket,path=%s,nowait,id=char0 "
-                                  "-mon chardev=char0,mode=control%s "
+                                  "-qmp unix:%s,nowait "
                                   "-machine accel=qtest "
                                   "-display none "
                                   "%s", qemu_binary, socket_path,
                                   getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null",
-                                  qmp_socket_path, "",
+                                  qmp_socket_path,
                                   extra_args ?: "");
         execlp("/bin/sh", "sh", "-c", command, NULL);
         exit(1);
@@ -248,7 +246,7 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,
 
 QTestState *qtest_init(const char *extra_args)
 {
-    QTestState *s = qtest_init_without_qmp_handshake(false, extra_args);
+    QTestState *s = qtest_init_without_qmp_handshake(extra_args);
 
     /* Read the QMP greeting and then do the handshake */
     qtest_qmp_discard_response(s, "");
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 3747bf7fbb..7d23daba28 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -81,7 +81,7 @@ static void test_qmp_protocol(void)
     QList *capabilities;
     QTestState *qts;
 
-    qts = qtest_init_without_qmp_handshake(false, common_args);
+    qts = qtest_init_without_qmp_handshake(common_args);
 
     /* Test greeting */
     resp = qtest_qmp_receive(qts);
@@ -146,7 +146,7 @@ static void test_qmp_oob(void)
     QString *qstr;
     const char *cmd_id;
 
-    qts = qtest_init_without_qmp_handshake(true, common_args);
+    qts = qtest_init_without_qmp_handshake(common_args);
 
     /* Check the greeting message. */
     resp = qtest_qmp_receive(qts);
-- 
2.17.1