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

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

[Peter Xu]

Based-on: <20180828191048.29806-1-armbru@redhat.com>
Based-on: <20180901111716.1675-1-armbru@redhat.com>

(this series is based on Markus's monitor-next tree)

v9:
- add r-bs
- release the qmp queue lock before resume [Marc-Andre]

v8:
- remove patch 1 & 2 since already in the QAPI pull
- squash patch 3 & 4, use Markus's version of commit message (with
  some of my additions), make sure popping and reading queue length is
  in the same critical section [Markus]
- add one patch to cover test for queue full [Markus]
- add one patch to resume the monitor earlier when queue not full [Markus]

v7:
- use Markus's commit message for patch "qapi: Drop
  qapi_event_send_FOO()'s Error ** argument" [Markus]
- update commit message for "qapi: remove COMMAND_DROPPED event" since
  QEMU 3.0 is released [Eric/Dave]
- rebase to Markus's monitor-next tree:
  http://repo.or.cz/qemu/armbru.git/shortlog/refs/heads/monitor-next
  the patch "monitor: suspend monitor instead of send CMD_DROP"
  re-written since people prefer to drop need_resume flag so now I
  hand-made it.  Dropped a few patches since not appliable any more.

Please review.  Thanks,

Peter Xu (6):
  monitor: Suspend monitor instead dropping commands
  monitor: resume the monitor earlier if needed
  monitor: remove "x-oob", turn oob on by default
  Revert "tests: Add parameter to qtest_init_without_qmp_handshake"
  tests: add oob functional test for test-qmp-cmds
  tests: qmp-test: add queue full test

 docs/interop/qmp-spec.txt |  5 ++-
 include/monitor/monitor.h |  3 +-
 monitor.c                 | 82 ++++++++++++++++-----------------------
 qapi/misc.json            | 40 -------------------
 tests/libqtest.c          | 10 ++---
 tests/libqtest.h          |  4 +-
 tests/qmp-test.c          | 32 +++++++++++++--
 tests/test-qmp-cmds.c     | 16 ++++++++
 vl.c                      |  5 ---
 9 files changed, 89 insertions(+), 108 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v9 1/6] monitor: Suspend monitor instead dropping commands

[Peter Xu]

When a QMP client sends in-band commands more quickly that we can
process them, we can either queue them without limit (QUEUE), drop
commands when the queue is full (DROP), or suspend receiving commands
when the queue is full (SUSPEND).  None of them is ideal:

* QUEUE lets a misbehaving client make QEMU eat memory without bounds.
Not such a hot idea.

* With DROP, the client has to cope with dropped in-band commands.  To
inform the client, we send a COMMAND_DROPPED event then.  The event is
flawed by design in two ways: it's ambiguous (see commit d621cfe0a17),
and it brings back the "eat memory without bounds" problem.

* With SUSPEND, the client has to manage the flow of in-band commands to
keep the monitor available for out-of-band commands.

We currently DROP.  Switch to SUSPEND.

Managing the flow of in-band commands to keep the monitor available for
out-of-band commands isn't really hard: just count the number of
"outstanding" in-band commands (commands sent minus replies received),
and if it exceeds the limit, hold back additional ones until it drops
below the limit again.

Note that we need to be careful pairing the suspend with a resume, or
else the monitor will hang, possibly forever.  And here since we need to
make sure both:

     (1) popping request from the req queue, and
     (2) reading length of the req queue

will be in the same critical section, we let the pop function take the
corresponding queue lock when there is a request, then we release the
lock from the caller.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 docs/interop/qmp-spec.txt |  5 ++--
 include/monitor/monitor.h |  2 ++
 monitor.c                 | 52 +++++++++++++++++----------------------
 qapi/misc.json            | 40 ------------------------------
 4 files changed, 28 insertions(+), 71 deletions(-)

diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
index 8f7da0245d..67e44a8120 100644
--- a/docs/interop/qmp-spec.txt
+++ b/docs/interop/qmp-spec.txt
@@ -130,8 +130,9 @@ to pass "id" with out-of-band commands.  Passing it with all commands
 is recommended for clients that accept capability "oob".
 
 If the client sends in-band commands faster than the server can
-execute them, the server will eventually drop commands to limit the
-queue length.  The sever sends event COMMAND_DROPPED then.
+execute them, the server will stop reading the requests from the QMP
+channel until the request queue length is reduced to an acceptable
+range.
 
 Only a few commands support out-of-band execution.  The ones that do
 have "allow-oob": true in output of query-qmp-schema.
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 6fd2c53b09..0c0a37d8cb 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -15,6 +15,8 @@ extern __thread Monitor *cur_mon;
 #define MONITOR_USE_PRETTY    0x08
 #define MONITOR_USE_OOB       0x10
 
+#define QMP_REQ_QUEUE_LEN_MAX 8
+
 bool monitor_cur_is_qmp(void);
 
 void monitor_init_globals(void);
diff --git a/monitor.c b/monitor.c
index b9258a7438..1f83775fff 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4097,8 +4097,12 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id)
  * processing commands only on a very busy monitor.  To achieve that,
  * when we process one request on a specific monitor, we put that
  * monitor to the end of mon_list queue.
+ *
+ * Note: if the function returned with non-NULL, then the caller will
+ * be with mon->qmp.qmp_queue_lock held, and the caller is responsible
+ * to release it.
  */
-static QMPRequest *monitor_qmp_requests_pop_any(void)
+static QMPRequest *monitor_qmp_requests_pop_any_with_lock(void)
 {
     QMPRequest *req_obj = NULL;
     Monitor *mon;
@@ -4108,10 +4112,11 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
     QTAILQ_FOREACH(mon, &mon_list, entry) {
         qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
         req_obj = g_queue_pop_head(mon->qmp.qmp_requests);
-        qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
         if (req_obj) {
+            /* With the lock of corresponding queue held */
             break;
         }
+        qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
     }
 
     if (req_obj) {
@@ -4130,30 +4135,34 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
 
 static void monitor_qmp_bh_dispatcher(void *data)
 {
-    QMPRequest *req_obj = monitor_qmp_requests_pop_any();
+    QMPRequest *req_obj = monitor_qmp_requests_pop_any_with_lock();
     QDict *rsp;
     bool need_resume;
+    Monitor *mon;
 
     if (!req_obj) {
         return;
     }
 
+    mon = req_obj->mon;
     /*  qmp_oob_enabled() might change after "qmp_capabilities" */
-    need_resume = !qmp_oob_enabled(req_obj->mon);
+    need_resume = !qmp_oob_enabled(mon) ||
+        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
+    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
     if (req_obj->req) {
         trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
-        monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
+        monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
     } else {
         assert(req_obj->err);
         rsp = qmp_error_response(req_obj->err);
         req_obj->err = NULL;
-        monitor_qmp_respond(req_obj->mon, rsp, NULL);
+        monitor_qmp_respond(mon, rsp, NULL);
         qobject_unref(rsp);
     }
 
     if (need_resume) {
         /* Pairs with the monitor_suspend() in handle_qmp_command() */
-        monitor_resume(req_obj->mon);
+        monitor_resume(mon);
     }
     qmp_request_free(req_obj);
 
@@ -4161,8 +4170,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
     qemu_bh_schedule(qmp_dispatcher_bh);
 }
 
-#define  QMP_REQ_QUEUE_LEN_MAX  (8)
-
 static void handle_qmp_command(void *opaque, QObject *req, Error *err)
 {
     Monitor *mon = opaque;
@@ -4204,28 +4211,14 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
     qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
 
     /*
-     * If OOB is not enabled on the current monitor, we'll emulate the
-     * old behavior that we won't process the current monitor any more
-     * until it has responded.  This helps make sure that as long as
-     * OOB is not enabled, the server will never drop any command.
+     * Suspend the monitor when we can't queue more requests after
+     * this one.  Dequeuing in monitor_qmp_bh_dispatcher() will resume
+     * it.  Note that when OOB is disabled, we queue at most one
+     * command, for backward compatibility.
      */
-    if (!qmp_oob_enabled(mon)) {
+    if (!qmp_oob_enabled(mon) ||
+        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
         monitor_suspend(mon);
-    } else {
-        /* Drop the request if queue is full. */
-        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
-            qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
-            /*
-             * FIXME @id's scope is just @mon, and broadcasting it is
-             * wrong.  If another monitor's client has a command with
-             * the same ID in flight, the event will incorrectly claim
-             * that command was dropped.
-             */
-            qapi_event_send_command_dropped(id,
-                                            COMMAND_DROP_REASON_QUEUE_FULL);
-            qmp_request_free(req_obj);
-            return;
-        }
     }
 
     /*
@@ -4233,6 +4226,7 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
      * handled in time order.  Ownership for req_obj, req, id,
      * etc. will be delivered to the handler side.
      */
+    assert(mon->qmp.qmp_requests->length < QMP_REQ_QUEUE_LEN_MAX);
     g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
     qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
 
diff --git a/qapi/misc.json b/qapi/misc.json
index ada9af5add..f149651a98 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -3437,46 +3437,6 @@
 ##
 { 'command': 'query-sev-capabilities', 'returns': 'SevCapability' }
 
-##
-# @CommandDropReason:
-#
-# Reasons that caused one command to be dropped.
-#
-# @queue-full: the command queue is full. This can only occur when
-#              the client sends a new non-oob command before the
-#              response to the previous non-oob command has been
-#              received.
-#
-# Since: 2.12
-##
-{ 'enum': 'CommandDropReason',
-  'data': [ 'queue-full' ] }
-
-##
-# @COMMAND_DROPPED:
-#
-# Emitted when a command is dropped due to some reason.  Commands can
-# only be dropped when the oob capability is enabled.
-#
-# @id: The dropped command's "id" field.
-# FIXME Broken by design.  Events are broadcast to all monitors.  If
-# another monitor's client has a command with the same ID in flight,
-# the event will incorrectly claim that command was dropped.
-#
-# @reason: The reason why the command is dropped.
-#
-# Since: 2.12
-#
-# Example:
-#
-# { "event": "COMMAND_DROPPED",
-#   "data": {"result": {"id": "libvirt-102",
-#                       "reason": "queue-full" } } }
-#
-##
-{ 'event': 'COMMAND_DROPPED' ,
-  'data': { 'id': 'any', 'reason': 'CommandDropReason' } }
-
 ##
 # @set-numa-node:
 #
-- 
2.17.1

Re: [Qemu-devel] [PATCH v9 1/6] monitor: Suspend monitor instead dropping commands

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> When a QMP client sends in-band commands more quickly that we can
> process them, we can either queue them without limit (QUEUE), drop
> commands when the queue is full (DROP), or suspend receiving commands
> when the queue is full (SUSPEND).  None of them is ideal:
>
> * QUEUE lets a misbehaving client make QEMU eat memory without bounds.
> Not such a hot idea.
>
> * With DROP, the client has to cope with dropped in-band commands.  To
> inform the client, we send a COMMAND_DROPPED event then.  The event is
> flawed by design in two ways: it's ambiguous (see commit d621cfe0a17),
> and it brings back the "eat memory without bounds" problem.
>
> * With SUSPEND, the client has to manage the flow of in-band commands to
> keep the monitor available for out-of-band commands.
>
> We currently DROP.  Switch to SUSPEND.
>
> Managing the flow of in-band commands to keep the monitor available for
> out-of-band commands isn't really hard: just count the number of
> "outstanding" in-band commands (commands sent minus replies received),
> and if it exceeds the limit, hold back additional ones until it drops
> below the limit again.
>
> Note that we need to be careful pairing the suspend with a resume, or
> else the monitor will hang, possibly forever.  And here since we need to
> make sure both:
>
>      (1) popping request from the req queue, and
>      (2) reading length of the req queue
>
> will be in the same critical section, we let the pop function take the
> corresponding queue lock when there is a request, then we release the
> lock from the caller.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  docs/interop/qmp-spec.txt |  5 ++--
>  include/monitor/monitor.h |  2 ++
>  monitor.c                 | 52 +++++++++++++++++----------------------
>  qapi/misc.json            | 40 ------------------------------
>  4 files changed, 28 insertions(+), 71 deletions(-)
>
> diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
> index 8f7da0245d..67e44a8120 100644
> --- a/docs/interop/qmp-spec.txt
> +++ b/docs/interop/qmp-spec.txt
> @@ -130,8 +130,9 @@ to pass "id" with out-of-band commands.  Passing it with all commands
>  is recommended for clients that accept capability "oob".
>  
>  If the client sends in-band commands faster than the server can
> -execute them, the server will eventually drop commands to limit the
> -queue length.  The sever sends event COMMAND_DROPPED then.
> +execute them, the server will stop reading the requests from the QMP
> +channel until the request queue length is reduced to an acceptable
> +range.
>  
>  Only a few commands support out-of-band execution.  The ones that do
>  have "allow-oob": true in output of query-qmp-schema.

Clients need to know the queue length limit to manage the flow.  We
better document it.  Can be done on top.

> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 6fd2c53b09..0c0a37d8cb 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -15,6 +15,8 @@ extern __thread Monitor *cur_mon;
>  #define MONITOR_USE_PRETTY    0x08
>  #define MONITOR_USE_OOB       0x10
>  
> +#define QMP_REQ_QUEUE_LEN_MAX 8
> +
>  bool monitor_cur_is_qmp(void);
>  
>  void monitor_init_globals(void);
> diff --git a/monitor.c b/monitor.c
> index b9258a7438..1f83775fff 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4097,8 +4097,12 @@ static void monitor_qmp_dispatch(Monitor *mon, QObject *req, QObject *id)
>   * processing commands only on a very busy monitor.  To achieve that,
>   * when we process one request on a specific monitor, we put that
>   * monitor to the end of mon_list queue.
> + *
> + * Note: if the function returned with non-NULL, then the caller will
> + * be with mon->qmp.qmp_queue_lock held, and the caller is responsible
> + * to release it.

The English could use some polish.  We can do that on top as well.

[...]

[Qemu-devel] [PATCH v9 2/6] monitor: resume the monitor earlier if needed

[Peter Xu]

Currently when QMP request queue full we won't resume the monitor until
we have completely handled the current command.  It's not necessary
since even before it's handled the queue is already non-full.  Moving
the resume logic earlier before the command execution.

Note that now monitor_resume() is heavy weighted after 8af6bb14a3a8 and
it's even possible (as pointed out by Marc-André) that the function
itself may try to take the monitor lock again, so let's do the resume
after the monitor lock is released to avoid possible dead lock.

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

diff --git a/monitor.c b/monitor.c
index 1f83775fff..f5911399d8 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4149,6 +4149,12 @@ static void monitor_qmp_bh_dispatcher(void *data)
     need_resume = !qmp_oob_enabled(mon) ||
         mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
     qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+
+    if (need_resume) {
+        /* Pairs with the monitor_suspend() in handle_qmp_command() */
+        monitor_resume(mon);
+    }
+
     if (req_obj->req) {
         trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
         monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
@@ -4160,10 +4166,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
         qobject_unref(rsp);
     }
 
-    if (need_resume) {
-        /* Pairs with the monitor_suspend() in handle_qmp_command() */
-        monitor_resume(mon);
-    }
     qmp_request_free(req_obj);
 
     /* Reschedule instead of looping so the main loop stays responsive */
-- 
2.17.1

Re: [Qemu-devel] [PATCH v9 2/6] monitor: resume the monitor earlier if needed

[Marc-André Lureau]

Hi
On Tue, Oct 9, 2018 at 10:28 AM Peter Xu <peterx@redhat.com> wrote:
>
> Currently when QMP request queue full we won't resume the monitor until
> we have completely handled the current command.  It's not necessary
> since even before it's handled the queue is already non-full.  Moving
> the resume logic earlier before the command execution.
>
> Note that now monitor_resume() is heavy weighted after 8af6bb14a3a8 and
> it's even possible (as pointed out by Marc-André) that the function
> itself may try to take the monitor lock again, so let's do the resume
> after the monitor lock is released to avoid possible dead lock.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 1f83775fff..f5911399d8 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4149,6 +4149,12 @@ static void monitor_qmp_bh_dispatcher(void *data)
>      need_resume = !qmp_oob_enabled(mon) ||
>          mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
>      qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> +
> +    if (need_resume) {
> +        /* Pairs with the monitor_suspend() in handle_qmp_command() */
> +        monitor_resume(mon);
> +    }

"need_resume" is only set on non-oob enabled monitors.

On monitor_resume(), monitor_qmp_read() may end up being called, which
may call handle_qmp_command().

With regular commands, a new command is queued. But if the command is
"exec-oob", it will dispatch immediately, thus not following the QMP
reply ordering constrain.

Shouldn't it be an error to call exec-oob on non-oob enabled monitors?

> +
>      if (req_obj->req) {
>          trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
>          monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
> @@ -4160,10 +4166,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
>          qobject_unref(rsp);
>      }
>
> -    if (need_resume) {
> -        /* Pairs with the monitor_suspend() in handle_qmp_command() */
> -        monitor_resume(mon);
> -    }
>      qmp_request_free(req_obj);
>
>      /* Reschedule instead of looping so the main loop stays responsive */
> --
> 2.17.1
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v9 2/6] monitor: resume the monitor earlier if needed

[Peter Xu]

On Tue, Oct 09, 2018 at 12:54:37PM +0400, Marc-André Lureau wrote:
> Hi
> On Tue, Oct 9, 2018 at 10:28 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > Currently when QMP request queue full we won't resume the monitor until
> > we have completely handled the current command.  It's not necessary
> > since even before it's handled the queue is already non-full.  Moving
> > the resume logic earlier before the command execution.
> >
> > Note that now monitor_resume() is heavy weighted after 8af6bb14a3a8 and
> > it's even possible (as pointed out by Marc-André) that the function
> > itself may try to take the monitor lock again, so let's do the resume
> > after the monitor lock is released to avoid possible dead lock.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  monitor.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 1f83775fff..f5911399d8 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4149,6 +4149,12 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >      need_resume = !qmp_oob_enabled(mon) ||
> >          mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
> >      qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> > +
> > +    if (need_resume) {
> > +        /* Pairs with the monitor_suspend() in handle_qmp_command() */
> > +        monitor_resume(mon);
> > +    }
> 
> "need_resume" is only set on non-oob enabled monitors.

... or on oob-enabled monitors when queue full?

> 
> On monitor_resume(), monitor_qmp_read() may end up being called, which
> may call handle_qmp_command().
> 
> With regular commands, a new command is queued. But if the command is
> "exec-oob", it will dispatch immediately, thus not following the QMP
> reply ordering constrain.
> 
> Shouldn't it be an error to call exec-oob on non-oob enabled monitors?

Do you mean a "qmp_capabilities" command to enable oob, and a
continuous "exec-oob" command?

I can't say I fully understand the scenario you mentioned, but I think
it does violate the rule if we resume the monitor before we finish
executing the "qmp_capabilities" command since that command should
still be run with "non-oob" context.  So in that case we should do the
resume after dispatching.  For the other queue-full case we shouldn't
need to, as Markus suggested.

Instead of bothering with all these, how about I drop this patch?  We
might resume the monitor a little bit later when queue full, but I
don't think that's a big deal for now.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v9 2/6] monitor: resume the monitor earlier if needed

[Marc-André Lureau]

Hi

On Wed, Oct 10, 2018 at 7:22 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Oct 09, 2018 at 12:54:37PM +0400, Marc-André Lureau wrote:
> > Hi
> > On Tue, Oct 9, 2018 at 10:28 AM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > Currently when QMP request queue full we won't resume the monitor until
> > > we have completely handled the current command.  It's not necessary
> > > since even before it's handled the queue is already non-full.  Moving
> > > the resume logic earlier before the command execution.
> > >
> > > Note that now monitor_resume() is heavy weighted after 8af6bb14a3a8 and
> > > it's even possible (as pointed out by Marc-André) that the function
> > > itself may try to take the monitor lock again, so let's do the resume
> > > after the monitor lock is released to avoid possible dead lock.
> > >
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  monitor.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/monitor.c b/monitor.c
> > > index 1f83775fff..f5911399d8 100644
> > > --- a/monitor.c
> > > +++ b/monitor.c
> > > @@ -4149,6 +4149,12 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > >      need_resume = !qmp_oob_enabled(mon) ||
> > >          mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
> > >      qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> > > +
> > > +    if (need_resume) {
> > > +        /* Pairs with the monitor_suspend() in handle_qmp_command() */
> > > +        monitor_resume(mon);
> > > +    }
> >
> > "need_resume" is only set on non-oob enabled monitors.
>
> ... or on oob-enabled monitors when queue full?

yes, after patch 1

>
> >
> > On monitor_resume(), monitor_qmp_read() may end up being called, which
> > may call handle_qmp_command().
> >
> > With regular commands, a new command is queued. But if the command is
> > "exec-oob", it will dispatch immediately, thus not following the QMP
> > reply ordering constrain.
> >
> > Shouldn't it be an error to call exec-oob on non-oob enabled monitors?
>
> Do you mean a "qmp_capabilities" command to enable oob, and a
> continuous "exec-oob" command?

No I meant calling "exec-oob" on a monitor without oob capability. I
checked again, and it does already fail with "QMP input member
'exec-oob' is unexpected". So nevermind.

>
> I can't say I fully understand the scenario you mentioned, but I think
> it does violate the rule if we resume the monitor before we finish
> executing the "qmp_capabilities" command since that command should
> still be run with "non-oob" context.  So in that case we should do the
> resume after dispatching.  For the other queue-full case we shouldn't
> need to, as Markus suggested.
>
> Instead of bothering with all these, how about I drop this patch?  We
> might resume the monitor a little bit later when queue full, but I
> don't think that's a big deal for now.

Yes, if it's a minor optimization, we can postpone it for now.

thanks

>
> Regards,
>
> --
> Peter Xu



-- 
Marc-André Lureau

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

[Peter Xu]

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

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

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"),
and turns it on again 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 break.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/monitor/monitor.h |  1 -
 monitor.c                 | 22 ++++++----------------
 tests/libqtest.c          |  2 +-
 tests/qmp-test.c          |  2 +-
 vl.c                      |  5 -----
 5 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 0c0a37d8cb..c1b40a9cac 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -13,7 +13,6 @@ extern __thread Monitor *cur_mon;
 #define MONITOR_USE_READLINE  0x02
 #define MONITOR_USE_CONTROL   0x04
 #define MONITOR_USE_PRETTY    0x08
-#define MONITOR_USE_OOB       0x10
 
 #define QMP_REQ_QUEUE_LEN_MAX 8
 
diff --git a/monitor.c b/monitor.c
index f5911399d8..65a5e5f41a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4545,19 +4545,12 @@ 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);
-        }
-    }
+    /*
+     * Note: 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.
+     */
+    bool use_oob = (flags & MONITOR_USE_CONTROL) && !CHARDEV_IS_MUX(chr);
 
     monitor_data_init(mon, false, use_oob);
 
@@ -4647,9 +4640,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 2cd5736642..8bf9fd1ae3 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -231,7 +231,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 6c419f6023..030d813f77 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -116,7 +116,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 4e25c78bff..db766f5295 100644
--- a/vl.c
+++ b/vl.c
@@ -2282,11 +2282,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 v9 4/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.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/libqtest.c | 10 ++++------
 tests/libqtest.h |  4 +---
 tests/qmp-test.c |  4 ++--
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 8bf9fd1ae3..e0bb032c5e 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -191,8 +191,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;
@@ -225,13 +224,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);
@@ -266,7 +264,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);
     QDict *greeting;
 
     /* Read the QMP greeting and then do the handshake */
diff --git a/tests/libqtest.h b/tests/libqtest.h
index ed88ff99d5..96a6c01352 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -55,14 +55,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/qmp-test.c b/tests/qmp-test.c
index 030d813f77..cc9907b525 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -108,7 +108,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);
@@ -219,7 +219,7 @@ static void test_qmp_oob(void)
     QList *capabilities;
     QString *qstr;
 
-    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

[Qemu-devel] [PATCH v9 5/6] tests: add oob functional test for test-qmp-cmds

[Peter Xu]

Straightforward test just to let the test-qmp-cmds be complete.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/test-qmp-cmds.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index 4ab2b6e5ce..481cb069ca 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -126,6 +126,21 @@ static void test_dispatch_cmd(void)
     qobject_unref(req);
 }
 
+static void test_dispatch_cmd_oob(void)
+{
+    QDict *req = qdict_new();
+    QDict *resp;
+
+    qdict_put_str(req, "exec-oob", "test-flags-command");
+
+    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), true);
+    assert(resp != NULL);
+    assert(!qdict_haskey(resp, "error"));
+
+    qobject_unref(resp);
+    qobject_unref(req);
+}
+
 /* test commands that return an error due to invalid parameters */
 static void test_dispatch_cmd_failure(void)
 {
@@ -302,6 +317,7 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
 
     g_test_add_func("/qmp/dispatch_cmd", test_dispatch_cmd);
+    g_test_add_func("/qmp/dispatch_cmd_oob", test_dispatch_cmd_oob);
     g_test_add_func("/qmp/dispatch_cmd_failure", test_dispatch_cmd_failure);
     g_test_add_func("/qmp/dispatch_cmd_io", test_dispatch_cmd_io);
     g_test_add_func("/qmp/dispatch_cmd_success_response",
-- 
2.17.1

[Qemu-devel] [PATCH v9 6/6] tests: qmp-test: add queue full test

[Peter Xu]

We'll need to include "monitor/monitor.h" for the queue length macro,
then we don't need to hard code it.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/qmp-test.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index cc9907b525..6989acbca4 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -18,6 +18,7 @@
 #include "qapi/qmp/qlist.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qmp/qstring.h"
+#include "monitor/monitor.h"
 
 const char common_args[] = "-nodefaults -machine none";
 
@@ -218,6 +219,8 @@ static void test_qmp_oob(void)
     const QListEntry *entry;
     QList *capabilities;
     QString *qstr;
+    gchar *id;
+    int i;
 
     qts = qtest_init_without_qmp_handshake(common_args);
 
@@ -272,6 +275,29 @@ static void test_qmp_oob(void)
     unblock_blocked_cmd();
     recv_cmd_id(qts, "blocks-2");
     recv_cmd_id(qts, "err-2");
+
+    /*
+     * Test queue full.  When that happens, the out-of-band command
+     * will only be able to be handled after the queue is shrinked, so
+     * it'll be processed only after one existing in-band command
+     * finishes.
+     */
+    for (i = 1; i <= QMP_REQ_QUEUE_LEN_MAX; i++) {
+        id = g_strdup_printf("queue-blocks-%d", i);
+        send_cmd_that_blocks(qts, id);
+        g_free(id);
+    }
+    send_oob_cmd_that_fails(qts, "oob-1");
+    unblock_blocked_cmd();
+    recv_cmd_id(qts, "queue-blocks-1");
+    recv_cmd_id(qts, "oob-1");
+    for (i = 2; i <= QMP_REQ_QUEUE_LEN_MAX; i++) {
+        unblock_blocked_cmd();
+        id = g_strdup_printf("queue-blocks-%d", i);
+        recv_cmd_id(qts, id);
+        g_free(id);
+    }
+
     cleanup_blocking_cmd();
 
     qtest_quit(qts);
-- 
2.17.1

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

[Eric Blake]

On 10/9/18 1:27 AM, Peter Xu wrote:
> Based-on: <20180828191048.29806-1-armbru@redhat.com>
> Based-on: <20180901111716.1675-1-armbru@redhat.com>
> 
> (this series is based on Markus's monitor-next tree)
> 
> v9:
> - add r-bs
> - release the qmp queue lock before resume [Marc-Andre]

I haven't reviewed closely, but did want to report that I tested that 
with your patches applied, there is no way to trigger OOB of the initial 
capability handshake (good). It's a bit odd that the initial error 
(input member unexpected) is different from the later error (does not 
support OOB), but not a show-stopper, so I don't think you need to worry 
about it:

{"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 3}, 
"package": "v3.0.0-1150-g7d932cd3d53"}, "capabilities": ["oob"]}}
{"exec-oob":"qmp_capabilities","arguments":{"enable":["oob"]}}
{"error": {"class": "GenericError", "desc": "QMP input member 'exec-oob' 
is unexpected"}}
{"execute":"qmp_capabilities","arguments":{"enable":["oob"]}}
{"return": {}}
{"exec-oob":"qmp_capabilities"}
{"error": {"class": "GenericError", "desc": "The command 
qmp_capabilities does not support OOB"}}

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

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

[Eric Blake]

On 10/10/18 11:26 AM, Eric Blake wrote:
> On 10/9/18 1:27 AM, Peter Xu wrote:
>> Based-on: <20180828191048.29806-1-armbru@redhat.com>
>> Based-on: <20180901111716.1675-1-armbru@redhat.com>
>>
>> (this series is based on Markus's monitor-next tree)
>>
>> v9:
>> - add r-bs
>> - release the qmp queue lock before resume [Marc-Andre]
> 
> I haven't reviewed closely, but did want to report that I tested that 
> with your patches applied, there is no way to trigger OOB of the initial 
> capability handshake (good). It's a bit odd that the initial error 
> (input member unexpected) is different from the later error (does not 
> support OOB), but not a show-stopper, so I don't think you need to worry 
> about it:
> 
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 3}, 
> "package": "v3.0.0-1150-g7d932cd3d53"}, "capabilities": ["oob"]}}
> {"exec-oob":"qmp_capabilities","arguments":{"enable":["oob"]}}
> {"error": {"class": "GenericError", "desc": "QMP input member 'exec-oob' 
> is unexpected"}}
> {"execute":"qmp_capabilities","arguments":{"enable":["oob"]}}
> {"return": {}}
> {"exec-oob":"qmp_capabilities"}
> {"error": {"class": "GenericError", "desc": "The command 
> qmp_capabilities does not support OOB"}}
> 

On the other hand, when I'm trying to use a qemu binary with these 
patches applied, libvirt is hanging when trying to probe the 
capabilities of the binary, waiting for a response to 
"qmp_capabilities". I'll try and bisect which patch is causing the 
problem, and figure out why it is happening for libvirt and not running 
by hand (perhaps is it a tty vs. Unix socket thing?)

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

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

[Eric Blake]

On 10/10/18 2:26 PM, Eric Blake wrote:

> 
> On the other hand, when I'm trying to use a qemu binary with these 
> patches applied, libvirt is hanging when trying to probe the 
> capabilities of the binary, waiting for a response to 
> "qmp_capabilities". I'll try and bisect which patch is causing the 
> problem, and figure out why it is happening for libvirt and not running 
> by hand (perhaps is it a tty vs. Unix socket thing?)

Bisect didn't help much; it landed on:

     monitor: remove "x-oob", turn oob on by default

as the cause of libvirt hanging. I didn't have time to investigate 
further, other than the command line that is hanging:

/home/eblake/qemu/x86_64-softmmu/qemu-system-x86_64 -S -no-user-config 
-nodefaults -nographic -machine none,accel=kvm:tcg -qmp 
unix:/var/lib/libvirt/qemu/capabilities.monitor.sock,server,nowait 
-pidfile /var/lib/libvirt/qemu/capabilities.pidfile -daemonize

And I suspect it is the -daemonize that is causing the hang I'm seeing 
when run by libvirt.

> Based-on: <20180828191048.29806-1-armbru@redhat.com>
> Based-on: <20180901111716.1675-1-armbru@redhat.com>
> 
> (this series is based on Markus's monitor-next tree)


Am I missing any prerequisite patches? Markus' monitor-next tree is 
currently a subset of git master (merge efd1d522).

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

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

[Peter Xu]

On Wed, Oct 10, 2018 at 03:27:34PM -0500, Eric Blake wrote:
> On 10/10/18 2:26 PM, Eric Blake wrote:
> 
> > 
> > On the other hand, when I'm trying to use a qemu binary with these
> > patches applied, libvirt is hanging when trying to probe the
> > capabilities of the binary, waiting for a response to
> > "qmp_capabilities". I'll try and bisect which patch is causing the
> > problem, and figure out why it is happening for libvirt and not running
> > by hand (perhaps is it a tty vs. Unix socket thing?)
> 
> Bisect didn't help much; it landed on:
> 
>     monitor: remove "x-oob", turn oob on by default
> 
> as the cause of libvirt hanging. I didn't have time to investigate further,
> other than the command line that is hanging:
> 
> /home/eblake/qemu/x86_64-softmmu/qemu-system-x86_64 -S -no-user-config
> -nodefaults -nographic -machine none,accel=kvm:tcg -qmp
> unix:/var/lib/libvirt/qemu/capabilities.monitor.sock,server,nowait -pidfile
> /var/lib/libvirt/qemu/capabilities.pidfile -daemonize
> 
> And I suspect it is the -daemonize that is causing the hang I'm seeing when
> run by libvirt.
> 
> > Based-on: <20180828191048.29806-1-armbru@redhat.com>
> > Based-on: <20180901111716.1675-1-armbru@redhat.com>
> > 
> > (this series is based on Markus's monitor-next tree)
> 
> 
> Am I missing any prerequisite patches? Markus' monitor-next tree is
> currently a subset of git master (merge efd1d522).

Sorry for the confusion. I should have removed these lines from the
old cover letter.

It's very possible the daemonize thing, actually Wolfgang Bumiller has
posted patches to fix this up (it's not the problem of this series,
but it just exposed this to libvirt by the series since it only
happens when oob and daemonize are both enabled).  The fixes are:

  [PATCH v2 0/2] delay monitor iothread creation

Since it cannot be applied cleanly onto this series, I resolved the
conflicts and pushed a tree here in case you wanna try with these two
extra patches applied:

  https://github.com/xzpeter/qemu/tree/test-oob

Please feel free to test with libvirt again with that.

Thanks for playing with the tree and further investigation!  I will
also mention this in the next version of cover letter (if there is one).

Regards,

-- 
Peter Xu

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

[Eric Blake]

On 10/10/18 7:05 PM, Peter Xu wrote:

>> other than the command line that is hanging:
>>
>> /home/eblake/qemu/x86_64-softmmu/qemu-system-x86_64 -S -no-user-config
>> -nodefaults -nographic -machine none,accel=kvm:tcg -qmp
>> unix:/var/lib/libvirt/qemu/capabilities.monitor.sock,server,nowait -pidfile
>> /var/lib/libvirt/qemu/capabilities.pidfile -daemonize
>>
>> And I suspect it is the -daemonize that is causing the hang I'm seeing when
>> run by libvirt.
>>

> 
> It's very possible the daemonize thing, actually Wolfgang Bumiller has
> posted patches to fix this up (it's not the problem of this series,
> but it just exposed this to libvirt by the series since it only
> happens when oob and daemonize are both enabled).  The fixes are:
> 
>    [PATCH v2 0/2] delay monitor iothread creation
> 
> Since it cannot be applied cleanly onto this series, I resolved the
> conflicts and pushed a tree here in case you wanna try with these two
> extra patches applied:
> 
>    https://github.com/xzpeter/qemu/tree/test-oob

Technically, we should apply the patches in the opposite order 
(Wolfgang's first, then yours), so that bisection does not land on a 
known-bad hang situation.

> 
> Please feel free to test with libvirt again with that.
> 
> Thanks for playing with the tree and further investigation!  I will
> also mention this in the next version of cover letter (if there is one).

Confirmed that libvirt no longer hung with Wolfgang's patches added in.

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

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

[Peter Xu]

On Wed, Oct 10, 2018 at 08:17:41PM -0500, Eric Blake wrote:
> On 10/10/18 7:05 PM, Peter Xu wrote:
> 
> > > other than the command line that is hanging:
> > > 
> > > /home/eblake/qemu/x86_64-softmmu/qemu-system-x86_64 -S -no-user-config
> > > -nodefaults -nographic -machine none,accel=kvm:tcg -qmp
> > > unix:/var/lib/libvirt/qemu/capabilities.monitor.sock,server,nowait -pidfile
> > > /var/lib/libvirt/qemu/capabilities.pidfile -daemonize
> > > 
> > > And I suspect it is the -daemonize that is causing the hang I'm seeing when
> > > run by libvirt.
> > > 
> 
> > 
> > It's very possible the daemonize thing, actually Wolfgang Bumiller has
> > posted patches to fix this up (it's not the problem of this series,
> > but it just exposed this to libvirt by the series since it only
> > happens when oob and daemonize are both enabled).  The fixes are:
> > 
> >    [PATCH v2 0/2] delay monitor iothread creation
> > 
> > Since it cannot be applied cleanly onto this series, I resolved the
> > conflicts and pushed a tree here in case you wanna try with these two
> > extra patches applied:
> > 
> >    https://github.com/xzpeter/qemu/tree/test-oob
> 
> Technically, we should apply the patches in the opposite order (Wolfgang's
> first, then yours), so that bisection does not land on a known-bad hang
> situation.

Agreed, that patch actually fixes bug of current master (current
master could possibly hit the same issue when with x-oob=on and
-daemonize), so should be applied earlier when proper.

> 
> > 
> > Please feel free to test with libvirt again with that.
> > 
> > Thanks for playing with the tree and further investigation!  I will
> > also mention this in the next version of cover letter (if there is one).
> 
> Confirmed that libvirt no longer hung with Wolfgang's patches added in.

Thanks!

-- 
Peter Xu

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

[Markus Armbruster]

Queued except for PATCH 2/6.  Thanks!

We'll want Marc-André's fixes on top.