[Qemu-devel] [PATCH v4 0/7] monitor: enable OOB by default

[Qemu-devel] [PATCH v4 0/7] monitor: enable OOB by default

[Peter Xu]

v4:
- collect some r-bs
- remove one extra s-o-b of mine in one patch [Thomas, Markus]
- split the qmp response flush patch into two; apply changes to commit
  message [Markus]
- fix up the doc update patch [Markus]

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 (7):
  chardev: comment details for CLOSED event
  monitor: rename *_pop_one to *_pop_any
  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 | 17 +++++----
 include/chardev/char.h       | 11 +++++-
 include/monitor/monitor.h    |  1 -
 tests/libqtest.h             |  4 +--
 monitor.c                    | 70 ++++++++++++++++++++----------------
 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, 78 insertions(+), 57 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v4 1/7] 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>
Reviewed-by: Stefan Hajnoczi <stefanha@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

[Qemu-devel] [PATCH v4 2/7] monitor: rename *_pop_one to *_pop_any

[Peter Xu]

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

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

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/monitor.c b/monitor.c
index 6d0cec552e..d4a463f707 100644
--- a/monitor.c
+++ b/monitor.c
@@ -513,10 +513,10 @@ struct QMPResponse {
 typedef struct QMPResponse QMPResponse;
 
 /*
- * 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 false if all the queues are empty; else true.
  */
-static QMPResponse monitor_qmp_response_pop_one(void)
+static bool monitor_qmp_response_pop_any(QMPResponse *response)
 {
     Monitor *mon;
     QObject *data = NULL;
@@ -527,22 +527,20 @@ static QMPResponse monitor_qmp_response_pop_one(void)
         data = g_queue_pop_head(mon->qmp.qmp_responses);
         qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
         if (data) {
+            response->mon = mon;
+            response->data = data;
             break;
         }
     }
     qemu_mutex_unlock(&monitor_lock);
-    return (QMPResponse) { .mon = mon, .data = data };
+    return data != NULL;
 }
 
 static void monitor_qmp_bh_responder(void *opaque)
 {
     QMPResponse response;
 
-    while (true) {
-        response = monitor_qmp_response_pop_one();
-        if (!response.data) {
-            break;
-        }
+    while (monitor_qmp_response_pop_any(&response)) {
         monitor_json_emitter_raw(response.mon, response.data);
         qobject_unref(response.data);
     }
@@ -4107,7 +4105,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;
@@ -4139,7 +4137,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) ?: "");
-- 
2.17.1

Re: [Qemu-devel] [PATCH v4 2/7] monitor: rename *_pop_one to *_pop_any

[Eric Blake]

On 06/19/2018 12:34 AM, Peter Xu wrote:
> The old names are confusing since both of the old functions are poping

s/poping/popping/

> an item from multiple queues rather than a single queue.  In that
> sense, *_pop_any() suites better than *_pop_one().
> 
> Since at it, touch up the function monitor_qmp_response_pop_any() a bit
> to let the callers pass in a QMPResponse struct instead of returning a
> struct.  Change the return value to boolean to mark whether we have
> poped a valid response instead.

s/poped/popped/

> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   monitor.c | 20 +++++++++-----------
>   1 file changed, 9 insertions(+), 11 deletions(-)
> 

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

Re: [Qemu-devel] [PATCH v4 2/7] monitor: rename *_pop_one to *_pop_any

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> The old names are confusing since both of the old functions are poping
> an item from multiple queues rather than a single queue.  In that
> sense, *_pop_any() suites better than *_pop_one().
>
> Since at it, touch up the function monitor_qmp_response_pop_any() a bit
> to let the callers pass in a QMPResponse struct instead of returning a
> struct.  Change the return value to boolean to mark whether we have
> poped a valid response instead.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 6d0cec552e..d4a463f707 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -513,10 +513,10 @@ struct QMPResponse {
>  typedef struct QMPResponse QMPResponse;
>  
>  /*
> - * 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 false if all the queues are empty; else true.
>   */
> -static QMPResponse monitor_qmp_response_pop_one(void)
> +static bool monitor_qmp_response_pop_any(QMPResponse *response)
>  {
>      Monitor *mon;
>      QObject *data = NULL;

I'd return @response.  Matter of taste.

With Eric's spelling fixes:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

[...]

[Qemu-devel] [PATCH v4 3/7] 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 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 <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>

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

diff --git a/monitor.c b/monitor.c
index d4a463f707..c9a02ee40c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -512,6 +512,27 @@ struct QMPResponse {
 };
 typedef struct QMPResponse QMPResponse;
 
+static QObject *monitor_qmp_response_pop_one(Monitor *mon)
+{
+    QObject *data;
+
+    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    data = g_queue_pop_head(mon->qmp.qmp_responses);
+    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+
+    return data;
+}
+
+static void monitor_qmp_response_flush(Monitor *mon)
+{
+    QObject *data;
+
+    while ((data = monitor_qmp_response_pop_one(mon))) {
+        monitor_json_emitter_raw(mon, data);
+        qobject_unref(data);
+    }
+}
+
 /*
  * Pop a QMPResponse from any monitor's response queue into @response.
  * Return false if all the queues are empty; else true.
@@ -523,9 +544,7 @@ static bool monitor_qmp_response_pop_any(QMPResponse *response)
 
     qemu_mutex_lock(&monitor_lock);
     QTAILQ_FOREACH(mon, &mon_list, entry) {
-        qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
-        data = g_queue_pop_head(mon->qmp.qmp_responses);
-        qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+        data = monitor_qmp_response_pop_one(mon);
         if (data) {
             response->mon = mon;
             response->data = data;
@@ -4364,6 +4383,14 @@ static void monitor_qmp_event(void *opaque, int event)
         mon_refcount++;
         break;
     case CHR_EVENT_CLOSED:
+        /*
+         * Note: this is only useful when the output of the chardev
+         * 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.
+         */
+        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 v4 3/7] monitor: flush qmp responses when CLOSED

[Peter Xu]

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

[...]

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

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

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

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v4 3/7] monitor: flush qmp responses when CLOSED

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Jun 19, 2018 at 01:34:22PM +0800, Peter Xu wrote:
>
> [...]
>
>> 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>
>
> I am pretty sure this time that this 2nd line is not there in my local
> tree. :)
>
> I think it's a git-format-patch bug, otherwise I must have misused it
> for a long time.  Instead of figuring this out and repost again, I'll
> see how far the rest of the series can go.

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

Re: [Qemu-devel] [PATCH v4 3/7] monitor: flush qmp responses when CLOSED

[Peter Xu]

On Tue, Jun 19, 2018 at 03:55:12PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Tue, Jun 19, 2018 at 01:34:22PM +0800, Peter Xu wrote:
> >
> > [...]
> >
> >> 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>
> >
> > I am pretty sure this time that this 2nd line is not there in my local
> > tree. :)
> >
> > I think it's a git-format-patch bug, otherwise I must have misused it
> > for a long time.  Instead of figuring this out and repost again, I'll
> > see how far the rest of the series can go.
> 
> Do you use git-format-patch -s, or have format.signOff set in
> .git/config or ~/.gitconfig?

Ah it's in my ~/.gitconfig!  Removing that fixes the issue.

Though I'm still not sure why the problem doesn't happen with other
patches.  After all, due to the line wrapping mess I still prefer to
drop that chunk in commit message directly.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v4 3/7] 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 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):

I wouldn't wrap lines when quoting a diff.

>
> --- /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}}

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

>
> 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 | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index d4a463f707..c9a02ee40c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -512,6 +512,27 @@ struct QMPResponse {
>  };
>  typedef struct QMPResponse QMPResponse;
>  
> +static QObject *monitor_qmp_response_pop_one(Monitor *mon)
> +{
> +    QObject *data;
> +
> +    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> +    data = g_queue_pop_head(mon->qmp.qmp_responses);
> +    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> +
> +    return data;
> +}
> +
> +static void monitor_qmp_response_flush(Monitor *mon)
> +{
> +    QObject *data;
> +
> +    while ((data = monitor_qmp_response_pop_one(mon))) {
> +        monitor_json_emitter_raw(mon, data);
> +        qobject_unref(data);
> +    }
> +}
> +
>  /*
>   * Pop a QMPResponse from any monitor's response queue into @response.
>   * Return false if all the queues are empty; else true.
> @@ -523,9 +544,7 @@ static bool monitor_qmp_response_pop_any(QMPResponse *response)
>  
>      qemu_mutex_lock(&monitor_lock);
>      QTAILQ_FOREACH(mon, &mon_list, entry) {
> -        qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> -        data = g_queue_pop_head(mon->qmp.qmp_responses);
> -        qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> +        data = monitor_qmp_response_pop_one(mon);
>          if (data) {
>              response->mon = mon;
>              response->data = data;
> @@ -4364,6 +4383,14 @@ static void monitor_qmp_event(void *opaque, int event)
>          mon_refcount++;
>          break;
>      case CHR_EVENT_CLOSED:
> +        /*
> +         * Note: this is only useful when the output of the chardev
> +         * backend is still open.  For example, when the backend is
> +         * stdio, it's possible that stdout is still open when stdin
> +         * is closed.  After all, CHR_EVENT_CLOSED event is currently
> +         * only bound to stdin.
> +         */

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

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

Re: [Qemu-devel] [PATCH v4 3/7] monitor: flush qmp responses when CLOSED

[Peter Xu]

On Tue, Jun 19, 2018 at 03:53:11PM +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 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):
> 
> I wouldn't wrap lines when quoting a diff.
> 
> >
> > --- /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}}
> 
> Please indent the quoted diff a bit, so make it more obviously not part
> of the patch.  In fact, git-am chokes on it for me.

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

> 
> >
> > 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 | 33 ++++++++++++++++++++++++++++++---
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index d4a463f707..c9a02ee40c 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -512,6 +512,27 @@ struct QMPResponse {
> >  };
> >  typedef struct QMPResponse QMPResponse;
> >  
> > +static QObject *monitor_qmp_response_pop_one(Monitor *mon)
> > +{
> > +    QObject *data;
> > +
> > +    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> > +    data = g_queue_pop_head(mon->qmp.qmp_responses);
> > +    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> > +
> > +    return data;
> > +}
> > +
> > +static void monitor_qmp_response_flush(Monitor *mon)
> > +{
> > +    QObject *data;
> > +
> > +    while ((data = monitor_qmp_response_pop_one(mon))) {
> > +        monitor_json_emitter_raw(mon, data);
> > +        qobject_unref(data);
> > +    }
> > +}
> > +
> >  /*
> >   * Pop a QMPResponse from any monitor's response queue into @response.
> >   * Return false if all the queues are empty; else true.
> > @@ -523,9 +544,7 @@ static bool monitor_qmp_response_pop_any(QMPResponse *response)
> >  
> >      qemu_mutex_lock(&monitor_lock);
> >      QTAILQ_FOREACH(mon, &mon_list, entry) {
> > -        qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> > -        data = g_queue_pop_head(mon->qmp.qmp_responses);
> > -        qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> > +        data = monitor_qmp_response_pop_one(mon);
> >          if (data) {
> >              response->mon = mon;
> >              response->data = data;
> > @@ -4364,6 +4383,14 @@ static void monitor_qmp_event(void *opaque, int event)
> >          mon_refcount++;
> >          break;
> >      case CHR_EVENT_CLOSED:
> > +        /*
> > +         * Note: this is only useful when the output of the chardev
> > +         * backend is still open.  For example, when the backend is
> > +         * stdio, it's possible that stdout is still open when stdin
> > +         * is closed.  After all, CHR_EVENT_CLOSED event is currently
> > +         * only bound to stdin.
> > +         */
> 
> I'm not sure I get the last sentence.  What do you mean by "bound to
> stdin"?

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

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

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v4 3/7] monitor: flush qmp responses when CLOSED

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Jun 19, 2018 at 03:53:11PM +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 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):
>> 
>> I wouldn't wrap lines when quoting a diff.
>> 
>> >
>> > --- /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}}
>> 
>> Please indent the quoted diff a bit, so make it more obviously not part
>> of the patch.  In fact, git-am chokes on it for me.
>
> To make it even simpler, I plan to remove the whole chunk of the diff
> from the commit message if you won't disagree.

In general, I rather like having test failure details in the commit
message.  In this particular case, however, the diff is probably not
necessary, as "iotest 087 will have ~10% chance to miss the SHUTDOWN
event" is clear enough.

>> >
>> > 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 | 33 ++++++++++++++++++++++++++++++---
>> >  1 file changed, 30 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/monitor.c b/monitor.c
>> > index d4a463f707..c9a02ee40c 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
[...]
>> > @@ -4364,6 +4383,14 @@ static void monitor_qmp_event(void *opaque, int event)
>> >          mon_refcount++;
>> >          break;
>> >      case CHR_EVENT_CLOSED:
>> > +        /*
>> > +         * Note: this is only useful when the output of the chardev
>> > +         * backend is still open.  For example, when the backend is
>> > +         * stdio, it's possible that stdout is still open when stdin
>> > +         * is closed.  After all, CHR_EVENT_CLOSED event is currently
>> > +         * only bound to stdin.
>> > +         */
>> 
>> I'm not sure I get the last sentence.  What do you mean by "bound to
>> stdin"?
>
> I want to express that the event is only related to the state of
> stdin, meanwhile it's never related to the state of stdout.  How about
> I remove the sentence after "after all"?  I think it's clear enough
> with the rest of the comments.

Works for me.

[...]

[Qemu-devel] [PATCH v4 4/7] tests: iotests: drop some stderr line

[Peter Xu]

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

060 5s ... - output mismatch (see 060.out.bad)
--- /home/peterx/git/qemu/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 <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 v4 4/7] 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 (the output is
> manually changed due to line width requirement):
>
> 060 5s ... - output mismatch (see 060.out.bad)
> --- /home/peterx/git/qemu/tests/qemu-iotests/060.out
> +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/060.out.bad
> @@ -427,8 +427,8 @@
>  QMP_VERSION
>  {"return": {}}
>  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}}

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

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

Re: [Qemu-devel] [PATCH v4 4/7] tests: iotests: drop some stderr line

[Peter Xu]

On Tue, Jun 19, 2018 at 03:57:07PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > In my Out-Of-Band test, "check -qcow2 060" fail with this (the output is
> > manually changed due to line width requirement):
> >
> > 060 5s ... - output mismatch (see 060.out.bad)
> > --- /home/peterx/git/qemu/tests/qemu-iotests/060.out
> > +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/060.out.bad
> > @@ -427,8 +427,8 @@
> >  QMP_VERSION
> >  {"return": {}}
> >  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}}
> 
> Please indent this diff; I'd expect git-am to choke on it.

Do you mean something like pretty-JSON?

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

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

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v4 4/7] tests: iotests: drop some stderr line

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Jun 19, 2018 at 03:57:07PM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > In my Out-Of-Band test, "check -qcow2 060" fail with this (the output is
>> > manually changed due to line width requirement):
>> >
>> > 060 5s ... - output mismatch (see 060.out.bad)
>> > --- /home/peterx/git/qemu/tests/qemu-iotests/060.out
>> > +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/060.out.bad
>> > @@ -427,8 +427,8 @@
>> >  QMP_VERSION
>> >  {"return": {}}
>> >  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}}
>> 
>> Please indent this diff; I'd expect git-am to choke on it.
>
> Do you mean something like pretty-JSON?

Instead of

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

write

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

The former confuses git-am.

In other words, quote diff output verbatim, but indent your quote.
    
> How about I remove this chunk too?  What do you prefer?

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

[...]

Re: [Qemu-devel] [PATCH v4 4/7] tests: iotests: drop some stderr line

[Peter Xu]

On Wed, Jun 20, 2018 at 09:12:57AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Tue, Jun 19, 2018 at 03:57:07PM +0200, Markus Armbruster wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > In my Out-Of-Band test, "check -qcow2 060" fail with this (the output is
> >> > manually changed due to line width requirement):
> >> >
> >> > 060 5s ... - output mismatch (see 060.out.bad)
> >> > --- /home/peterx/git/qemu/tests/qemu-iotests/060.out
> >> > +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/060.out.bad
> >> > @@ -427,8 +427,8 @@
> >> >  QMP_VERSION
> >> >  {"return": {}}
> >> >  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}}
> >> 
> >> Please indent this diff; I'd expect git-am to choke on it.
> >
> > Do you mean something like pretty-JSON?
> 
> Instead of
> 
>     In my Out-Of-Band test, "check -qcow2 060" fail with this (the output is
>     manually changed due to line width requirement):
>     
>     060 5s ... - output mismatch (see 060.out.bad)
>     --- /home/peterx/git/qemu/tests/qemu-iotests/060.out
>     +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/060.out.bad
>     @@ -427,8 +427,8 @@
>      QMP_VERSION
>      {"return": {}}
> 
> write
> 
>     In my Out-Of-Band test, "check -qcow2 060" fail with this (the output is
>     manually changed due to line width requirement):
>     
>         060 5s ... - output mismatch (see 060.out.bad)
>         --- /home/peterx/git/qemu/tests/qemu-iotests/060.out
>         +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/060.out.bad
>         @@ -427,8 +427,8 @@
>          QMP_VERSION
>          {"return": {}}
> 
> The former confuses git-am.
> 
> In other words, quote diff output verbatim, but indent your quote.
>     
> > How about I remove this chunk too?  What do you prefer?
> 
> I rather like having the test failure details in the commit message.

Ah I see.  I'll keep them then.

Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH v4 5/7] 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.  Meanwhile, touch up some other places too,
either with better English, or reordering of bullets.

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

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

Re: [Qemu-devel] [PATCH v4 5/7] 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.  Meanwhile, touch up some other places too,
> either with better English, or reordering of bullets.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  docs/devel/qapi-code-gen.txt | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 1366228b2a..fb486d4050 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -664,22 +664,27 @@ command:
>  
>  - They are executed in order,
>  - They run only in main thread of QEMU,
> -- They have the BQL taken during execution.
> +- They run with the BQL held.
>  
>  When a command is executed with OOB, the following changes occur:
>  
>  - They can be completed before a pending in-band command,
>  - They run in a dedicated monitor thread,
> -- They do not take the BQL during execution.
> +- They run with the BQL not held.
>  
>  OOB command handlers must satisfy the following conditions:
>  
> -- It executes extremely fast,
> -- It does not take any lock, or, it can take very small locks if all
> -  critical regions also follow the rules for OOB command handler code,
> +- It terminates quickly,
>  - It does not invoke system calls that may block,
>  - It does not access guest RAM that may block when userfaultfd is
> -  enabled for postcopy live migration.
> +  enabled for postcopy live migration,
> +- 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.
> +
> +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.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v4 6/7] 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 c9a02ee40c..7fbcf84b02 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4587,19 +4587,7 @@ void monitor_init(Chardev *chr, int flags)
 {
     Monitor *mon = g_malloc(sizeof(*mon));
     bool use_readline = flags & MONITOR_USE_READLINE;
-    bool use_oob = flags & MONITOR_USE_OOB;
-
-    if (use_oob) {
-        if (CHARDEV_IS_MUX(chr)) {
-            error_report("Monitor Out-Of-Band is not supported with "
-                         "MUX typed chardev backend");
-            exit(1);
-        }
-        if (use_readline) {
-            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);
 
@@ -4701,9 +4689,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

Re: [Qemu-devel] [PATCH v4 6/7] monitor: remove "x-oob", turn oob on by default

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> There was a regression reported by Eric Auger before with OOB:
>
>   http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html
>
> It is fixed in 951702f39c ("monitor: bind dispatch bh to iohandler
> context", 2018-04-10).
>
> For the bug, we turned Out-Of-Band feature of monitors off for 2.12
> release.  Now we turn that on again after the 2.12 release.

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

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

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

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

"by default"?

> can't enable Out-Of-Band for monitors with MUX-typed chardev backends,
> because not all the chardev frontends can run without main thread, or
> can run in multiple threads.
>
> Some trivial touch-up in the test code is required to make sure qmp-test
> won't broke.

"won't break"

>
> 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 c9a02ee40c..7fbcf84b02 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4587,19 +4587,7 @@ void monitor_init(Chardev *chr, int flags)
>  {
>      Monitor *mon = g_malloc(sizeof(*mon));
>      bool use_readline = flags & MONITOR_USE_READLINE;
> -    bool use_oob = flags & MONITOR_USE_OOB;
> -
> -    if (use_oob) {
> -        if (CHARDEV_IS_MUX(chr)) {
> -            error_report("Monitor Out-Of-Band is not supported with "
> -                         "MUX typed chardev backend");
> -            exit(1);
> -        }
> -        if (use_readline) {
> -            error_report("Monitor Out-Of-band is only supported by QMP");
> -            exit(1);
> -        }
> -    }
> +    bool use_oob = (flags & MONITOR_USE_CONTROL) && !CHARDEV_IS_MUX(chr);

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

>  
>      monitor_data_init(mon, false, use_oob);
>  
> @@ -4701,9 +4689,6 @@ QemuOptsList qemu_mon_opts = {
>          },{
>              .name = "pretty",
>              .type = QEMU_OPT_BOOL,
> -        },{
> -            .name = "x-oob",
> -            .type = QEMU_OPT_BOOL,
>          },
>          { /* end of list */ }
>      },
> 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) {

Preferrably with the commit message touched up, and a common on MUX:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v4 6/7] monitor: remove "x-oob", turn oob on by default

[Peter Xu]

On Tue, Jun 19, 2018 at 04:16:49PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > There was a regression reported by Eric Auger before with OOB:
> >
> >   http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html
> >
> > It is fixed in 951702f39c ("monitor: bind dispatch bh to iohandler
> > context", 2018-04-10).
> >
> > For the bug, we turned Out-Of-Band feature of monitors off for 2.12
> > release.  Now we turn that on again after the 2.12 release.
> 
> Relating what happened in the order it happened could be easier to
> understand.  Perhaps:
> 
>   OOB commands were introduced in commit cf869d53172.  Unfortunately, we
>   ran into a regression, and had to disable them by default for 2.12
>   (commit be933ffc23).
> 
>   The regression has since been fixed (commit 951702f39c7 "monitor: bind
>   dispatch bh to iohandler context").  Time to re-enable OOB.

This indeed looks much nicer.

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

Did I mis-spell somewhere?

> 
> > can't enable Out-Of-Band for monitors with MUX-typed chardev backends,
> > because not all the chardev frontends can run without main thread, or
> > can run in multiple threads.
> >
> > Some trivial touch-up in the test code is required to make sure qmp-test
> > won't broke.
> 
> "won't break"

This one I did.

> 
> >
> > 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 c9a02ee40c..7fbcf84b02 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4587,19 +4587,7 @@ void monitor_init(Chardev *chr, int flags)
> >  {
> >      Monitor *mon = g_malloc(sizeof(*mon));
> >      bool use_readline = flags & MONITOR_USE_READLINE;
> > -    bool use_oob = flags & MONITOR_USE_OOB;
> > -
> > -    if (use_oob) {
> > -        if (CHARDEV_IS_MUX(chr)) {
> > -            error_report("Monitor Out-Of-Band is not supported with "
> > -                         "MUX typed chardev backend");
> > -            exit(1);
> > -        }
> > -        if (use_readline) {
> > -            error_report("Monitor Out-Of-band is only supported by QMP");
> > -            exit(1);
> > -        }
> > -    }
> > +    bool use_oob = (flags & MONITOR_USE_CONTROL) && !CHARDEV_IS_MUX(chr);
> 
> A comment explaining (briefly!) why MUX prevents oob would be useful
> here.  Fortunately, you can simply steal from your commit message.

Done.

> 
> >  
> >      monitor_data_init(mon, false, use_oob);
> >  
> > @@ -4701,9 +4689,6 @@ QemuOptsList qemu_mon_opts = {
> >          },{
> >              .name = "pretty",
> >              .type = QEMU_OPT_BOOL,
> > -        },{
> > -            .name = "x-oob",
> > -            .type = QEMU_OPT_BOOL,
> >          },
> >          { /* end of list */ }
> >      },
> > 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) {
> 
> Preferrably with the commit message touched up, and a common on MUX:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Picked.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v4 6/7] monitor: remove "x-oob", turn oob on by default

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Jun 19, 2018 at 04:16:49PM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > There was a regression reported by Eric Auger before with OOB:
>> >
>> >   http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html
>> >
>> > It is fixed in 951702f39c ("monitor: bind dispatch bh to iohandler
>> > context", 2018-04-10).
>> >
>> > For the bug, we turned Out-Of-Band feature of monitors off for 2.12
>> > release.  Now we turn that on again after the 2.12 release.
>> 
>> Relating what happened in the order it happened could be easier to
>> understand.  Perhaps:
>> 
>>   OOB commands were introduced in commit cf869d53172.  Unfortunately, we
>>   ran into a regression, and had to disable them by default for 2.12
>>   (commit be933ffc23).
>> 
>>   The regression has since been fixed (commit 951702f39c7 "monitor: bind
>>   dispatch bh to iohandler context").  Time to re-enable OOB.
>
> This indeed looks much nicer.
>
>> 
>> > This patch partly reverts be933ffc23 (monitor: new parameter "x-oob"),
>> > meanwhile turn it on again by default for non-MUX QMPs.  Note that we
>> 
>> "by default"?
>
> Did I mis-spell somewhere?

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

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

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

[...]

Re: [Qemu-devel] [PATCH v4 6/7] monitor: remove "x-oob", turn oob on by default

[Peter Xu]

On Wed, Jun 20, 2018 at 09:20:30AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Tue, Jun 19, 2018 at 04:16:49PM +0200, Markus Armbruster wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > There was a regression reported by Eric Auger before with OOB:
> >> >
> >> >   http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html
> >> >
> >> > It is fixed in 951702f39c ("monitor: bind dispatch bh to iohandler
> >> > context", 2018-04-10).
> >> >
> >> > For the bug, we turned Out-Of-Band feature of monitors off for 2.12
> >> > release.  Now we turn that on again after the 2.12 release.
> >> 
> >> Relating what happened in the order it happened could be easier to
> >> understand.  Perhaps:
> >> 
> >>   OOB commands were introduced in commit cf869d53172.  Unfortunately, we
> >>   ran into a regression, and had to disable them by default for 2.12
> >>   (commit be933ffc23).
> >> 
> >>   The regression has since been fixed (commit 951702f39c7 "monitor: bind
> >>   dispatch bh to iohandler context").  Time to re-enable OOB.
> >
> > This indeed looks much nicer.
> >
> >> 
> >> > This patch partly reverts be933ffc23 (monitor: new parameter "x-oob"),
> >> > meanwhile turn it on again by default for non-MUX QMPs.  Note that we
> >> 
> >> "by default"?
> >
> > Did I mis-spell somewhere?
> 
> I was too terse, sorry.  Let me try again.
> 
> "By default" suggests there's a way for the user to switch it off.
> That's not the case.  I guess you mean something like
> 
>     This patch partly reverts be933ffc23 (monitor: new parameter "x-oob"),
>     and turns OOB on again for non-MUX QMPs.

I see!  Fixed, with your r-b kept.

Hmm.  A new version is coming.

Thanks,

-- 
Peter Xu

[Qemu-devel] [PATCH v4 7/7] 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

Re: [Qemu-devel] [PATCH v4 7/7] Revert "tests: Add parameter to qtest_init_without_qmp_handshake"

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

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

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v4 0/7] monitor: enable OOB by default

[Thomas Huth]

On 19.06.2018 07:34, Peter Xu wrote:
> v4:
> - collect some r-bs
> - remove one extra s-o-b of mine in one patch [Thomas, Markus]

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

 Thomas

Re: [Qemu-devel] [PATCH v4 0/7] monitor: enable OOB by default

[Peter Xu]

On Tue, Jun 19, 2018 at 07:51:47AM +0200, Thomas Huth wrote:
> On 19.06.2018 07:34, Peter Xu wrote:
> > v4:
> > - collect some r-bs
> > - remove one extra s-o-b of mine in one patch [Thomas, Markus]
> 
> FYI: You've still got a duplicated s-o-b line in patch 3 and 4.
> 
>  Thomas

Thanks.  I think it's caused by the lines in commit messages like

"""
--- /home/peterx/git/qemu/tests/qemu-iotests/087.out
+++ /home/peterx/git/qemu/bin/tests/qemu-iotests/087.out.bad
"""

Then git-format-patch does not work as expected (so the extra s-o-bs
are not in my local tree).

I remove these lines then git-format-patch works fine.

If to repost, I'll double confirm with these matters.

Regards,

-- 
Peter Xu