[Qemu-devel] [RFC v6 00/27] QMP: out-of-band (OOB) execution support

[Qemu-devel] [RFC v6 00/27] QMP: out-of-band (OOB) execution support

[Peter Xu]

This v6 kept most of the high level design untouched, however there
are quite a few of changes that try to make the series more robust and
safe.  My sincere thank to the reviewers (especially Stefan!) on all
review comments to previous one.  Let's see how this version goes.

v6:
- add r-bs
- s/negociate/negotiate/ [Dave]
- let mon_global be anonymous struct [Stefan]
- in monitor_init(): call qemu_chr_fe_set_handlers() later than init
  parser, then it's safe [Stefan]
- drop patch "qjson: add "opaque" field to JSONMessageParser",
  re-write the following one to use container_of(). [Stefan]
- keep get_qmp_greeting() the old way, add cap list [Stefan, Fam]
- fix the iothread_stop() comment since that's not really related to
  the glib bug [Stefan]
- when do qmp greeting, don't expose oob capability if iothread not
  used [Stefan, Fam]
- squash "qmp: introduce some capability helpers" into patch "monitor:
  separate QMP parser and dispatcher" [Fam]
- tune comments for monitor_qmp_respond(). [Stefan]
- use atomic_mb_{read|set}() where proper instead of no-mb ones
  [Stefan]
- add patch to let monitor suspend/resume really support QMP.  Notify
  iothread when needed, in both suspend/resume. [Stefan]
- add comment for qmp_queue_lock on lock taking ordering. [Stefan]
- refactor monitor_qmp_bh_dispatcher() to not loop, instead queue
  itself after each request. [Stefan]
- rename "request" into "command" in the new qmp event [Stefan]
- in handle_qmp_command() protect the queue length fetch with
  qmp_queue_lock [Stefan]
- one more s/2.11/2.12/.  [Fam]
- when isolating response queue, not only flush the queue, but also
  flush the monitor out buffer [Stefan]
- document rewrite [Stefan]
- some other touch-ups that I forgot to mention and some tiny new
  patches... anyway, rbs will be gone if any of the patch is touched
  up.  Please have a look!

v5
- rename "monitor_iothread" to "mon_iothread" [Dave]
- add comment in monitor_cleanup(), note that when the hacks can be
  removed. [Dan]
- add a note section in qmp-spec.txt, mentioning about how to migrate
  existing QMP command to oob-capable command. [Dave]
- drop patch "qmp: let migrate-incoming allow out-of-band".  All
  migration related changes will all be put into postcopy-recovery
  series.

v4:
- drop first patch to fix IOWatchPool [Stefan, Dan]
- add s-o-b where missing, and newly got r-bs
- fix English error in commit msg [Fam]
- some tunes on request-dropped event:  [Stefan]
  - firstly let 'id' be any type, meanwhile make sure "id" is there as
    long as OOB is enabled for the monitor.
  - some comments fix
- add new command "x-oob-test" for testing oob commands [Stefan]
- simplify the test codes to use new x-oob-test
- flush response queue before cleanup monitors

This series was born from this one:

  https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html

The idea comes from Markus Armbruster and the discussion we had in the
thread.  It's not a super ideal solution (I believe Markus had been
thinking hard to keep everything in order meanwhile trying to satisfy
the migration requirement), but AFAIU it's currently the best.

What is OOB?
============

It's the shortcut of Out-Of-Band execution, its name is given by
Markus.  It's a way to quickly execute a QMP request.  Say, originally
QMP is going throw these steps:

      JSON Parser --> QMP Dispatcher --> Respond
          /|\    (2)                (3)     |
       (1) |                               \|/ (4)
           +---------  main thread  --------+

The requests are executed by the so-called QMP-dispatcher after the
JSON is parsed.  If OOB is on, we run the command directly in the
parser and quickly returns.

This series changed the QMP handling logic by moving the command
parsing and responding phases into IOThreads, so to be more accurate,
after the series the above graph would change into this:

               queue/kick              queue/kick
     JSON Parser ======> QMP Dispatcher =====> Responder
         /|\ |     (3)       /|\  |      (4)      | /|\
      (1) |  | (2)            |   |               |  |
          |  |                |  \|/           (6)|  |(5)
          |  |            main thread             |  |
          |  |                                    |  |
          |  +--------> monitor IO thread <-------+  |
          +-----------/                   \----------+

New Interfaces
==============

QMP Introspection Changes
-------------------------

When do query-qmp-schema, we were getting something like:

  {"name": "migrate-incoming", "ret-type": "17",
   "meta-type": "command", "arg-type": "89"}

Now we will get a new key named "allow-oob":

  {"name": "migrate-incoming", "ret-type": "17", "allow-oob": true,
   "meta-type": "command", "arg-type": "89"}

Which shows whether a command supports OOB.

QMP Negociation Changes
-----------------------

We were running "qmp_capabilities" always without parameters like:

  {"execute": "qmp_capabilities"}

Now we can enable capabilities (we don't have any capability before
this series) like OOB using:

  {"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}}

Only after we explicitly enable OOB capability can we send requests in
OOB manner.  Otherwise we'll have exactly the same QMP session as
before, just like when OOB is not there.

When OOB is enabled, it's possible that OOB reply reaches faster than
previous command, so clients should be prepared.

Trigger OOB execution
---------------------

Let's take migrate-incoming as example.  The old command looks like:

  {"execute": "migrate-incoming", "arguments" : {"uri": "xxxxxx"}}

To execute a command with OOB execution, we need to specify it in the
QMP request in the extra "control" key:

  {"execute": "migrate-incoming", "arguments" : {"uri": "xxxxxx"},
   "control" { "run-oob": true } }

Then the command will be run in parser, and it can preempt other
commands.

Others
=================

The last patch of OOB test may need some attention.  I used
dump-guest-memory as a time-consuming command to test whether OOB is
working, and the only command I can test now is "migrate-incoming".  I
hope that is a "okay" solution for unit tests.  Any other suggestions
on that would be welcomed.

Please review.  Thanks.

Peter Xu (27):
  chardev: use backend chr context when watch for fe
  qobject: introduce qstring_get_try_str()
  qobject: introduce qobject_get_try_str()
  qobject: let object_property_get_str() use new API
  monitor: move skip_flush into monitor_data_init
  monitor: move the cur_mon hack deeper for QMP
  monitor: unify global init
  monitor: let mon_list be tail queue
  monitor: create monitor dedicate iothread
  monitor: allow to use IO thread for parsing
  qmp: introduce QMPCapability
  qmp: negotiate QMP capabilities
  monitor: introduce monitor_qmp_respond()
  monitor: let suspend_cnt be thread safe
  monitor: let suspend/resume work even with QMPs
  monitor: separate QMP parser and dispatcher
  qmp: add new event "command-dropped"
  monitor: send event when command queue full
  qapi: introduce new cmd option "allow-oob"
  qmp: export qmp_dispatch_check_obj and allow "id"
  qmp: support out-of-band (oob) execution
  qmp: isolate responses into io thread
  monitor: enable IO thread for (qmp & !mux) typed
  qmp: add command "x-oob-test"
  docs: update QMP documents for OOB commands
  tests: qmp-test: verify command batching
  tests: qmp-test: add oob test

 chardev/char-fe.c              |   2 +-
 docs/devel/qapi-code-gen.txt   |  70 ++++-
 docs/interop/qmp-spec.txt      |  30 +-
 include/monitor/monitor.h      |   2 +-
 include/qapi/qmp/dispatch.h    |   3 +
 include/qapi/qmp/qstring.h     |   2 +
 monitor.c                      | 650 +++++++++++++++++++++++++++++++++++------
 qapi-schema.json               |  83 +++++-
 qapi/introspect.json           |   6 +-
 qapi/qmp-dispatch.c            |  32 +-
 qmp.c                          |  16 +
 qobject/qstring.c              |  21 ++
 qom/object.c                   |   9 +-
 scripts/qapi-commands.py       |  19 +-
 scripts/qapi-introspect.py     |  10 +-
 scripts/qapi.py                |  15 +-
 scripts/qapi2texi.py           |   2 +-
 tests/qapi-schema/test-qapi.py |   2 +-
 tests/qmp-test.c               |  97 +++++-
 trace-events                   |   3 +
 vl.c                           |   7 +-
 21 files changed, 960 insertions(+), 121 deletions(-)

-- 
2.14.3

[Qemu-devel] [RFC v6 01/27] chardev: use backend chr context when watch for fe

[Peter Xu]

In commit 6bbb6c0644 ("chardev: use per-dev context for
io_add_watch_poll", 2017-09-22) all the chardev watches are converted to
use per-chardev gcontext to support chardev to be run outside default
main thread.  However that's still missing one call from the frontend
code.  Touch that up.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 chardev/char-fe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index ee6d596100..462c529f19 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -356,7 +356,7 @@ guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
     }
 
     g_source_set_callback(src, (GSourceFunc)func, user_data, NULL);
-    tag = g_source_attach(src, NULL);
+    tag = g_source_attach(src, be->chr->gcontext);
     g_source_unref(src);
 
     return tag;
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 01/27] chardev: use backend chr context when watch for fe

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:31PM +0800, Peter Xu wrote:
> In commit 6bbb6c0644 ("chardev: use per-dev context for
> io_add_watch_poll", 2017-09-22) all the chardev watches are converted to
> use per-chardev gcontext to support chardev to be run outside default
> main thread.  However that's still missing one call from the frontend
> code.  Touch that up.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-fe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index ee6d596100..462c529f19 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -356,7 +356,7 @@ guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
>      }
>  
>      g_source_set_callback(src, (GSourceFunc)func, user_data, NULL);
> -    tag = g_source_attach(src, NULL);
> +    tag = g_source_attach(src, be->chr->gcontext);

If you respin, it's a little nicer to use the s local variable instead:

    tag = g_source_attach(src, s->gcontext);

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

Re: [Qemu-devel] [RFC v6 01/27] chardev: use backend chr context when watch for fe

[Peter Xu]

On Wed, Dec 20, 2017 at 04:40:05PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 19, 2017 at 04:45:31PM +0800, Peter Xu wrote:
> > In commit 6bbb6c0644 ("chardev: use per-dev context for
> > io_add_watch_poll", 2017-09-22) all the chardev watches are converted to
> > use per-chardev gcontext to support chardev to be run outside default
> > main thread.  However that's still missing one call from the frontend
> > code.  Touch that up.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  chardev/char-fe.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> > index ee6d596100..462c529f19 100644
> > --- a/chardev/char-fe.c
> > +++ b/chardev/char-fe.c
> > @@ -356,7 +356,7 @@ guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
> >      }
> >  
> >      g_source_set_callback(src, (GSourceFunc)func, user_data, NULL);
> > -    tag = g_source_attach(src, NULL);
> > +    tag = g_source_attach(src, be->chr->gcontext);
> 
> If you respin, it's a little nicer to use the s local variable instead:
> 
>     tag = g_source_attach(src, s->gcontext);

Definitely.  R-b is taken as well.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 01/27] chardev: use backend chr context when watch for fe

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:31PM +0800, Peter Xu wrote:
> In commit 6bbb6c0644 ("chardev: use per-dev context for
> io_add_watch_poll", 2017-09-22) all the chardev watches are converted to
> use per-chardev gcontext to support chardev to be run outside default
> main thread.  However that's still missing one call from the frontend
> code.  Touch that up.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  chardev/char-fe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Either way, I'm happy with this patch:

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

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

[Qemu-devel] [RFC v6 02/27] qobject: introduce qstring_get_try_str()

[Peter Xu]

The only difference from qstring_get_str() is that it allows the qstring
to be NULL.  If so, NULL is returned.

CC: Eric Blake <eblake@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qapi/qmp/qstring.h |  1 +
 qobject/qstring.c          | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index 65c05a9be5..a145c8ca00 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -27,6 +27,7 @@ QString *qstring_from_str(const char *str);
 QString *qstring_from_substr(const char *str, int start, int end);
 size_t qstring_get_length(const QString *qstring);
 const char *qstring_get_str(const QString *qstring);
+const char *qstring_get_try_str(const QString *qstring);
 void qstring_append_int(QString *qstring, int64_t value);
 void qstring_append(QString *qstring, const char *str);
 void qstring_append_chr(QString *qstring, int c);
diff --git a/qobject/qstring.c b/qobject/qstring.c
index 74182a1c02..7638ab990d 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -128,6 +128,16 @@ const char *qstring_get_str(const QString *qstring)
     return qstring->string;
 }
 
+/**
+ * qstring_get_try_str(): Return a pointer to the stored string
+ *
+ * NOTE: will return NULL if qstring is not provided.
+ */
+const char *qstring_get_try_str(const QString *qstring)
+{
+    return qstring ? qstring_get_str(qstring) : NULL;
+}
+
 /**
  * qstring_is_equal(): Test whether the two QStrings are equal
  */
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 02/27] qobject: introduce qstring_get_try_str()

[Eric Blake]

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

On 12/19/2017 02:45 AM, Peter Xu wrote:
> The only difference from qstring_get_str() is that it allows the qstring
> to be NULL.  If so, NULL is returned.
> 
> CC: Eric Blake <eblake@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qapi/qmp/qstring.h |  1 +
>  qobject/qstring.c          | 10 ++++++++++
>  2 files changed, 11 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[Qemu-devel] [RFC v6 03/27] qobject: introduce qobject_get_try_str()

[Peter Xu]

A quick way to fetch string from qobject when it's a QString.

Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qapi/qmp/qstring.h |  1 +
 qobject/qstring.c          | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
index a145c8ca00..6517d8e377 100644
--- a/include/qapi/qmp/qstring.h
+++ b/include/qapi/qmp/qstring.h
@@ -28,6 +28,7 @@ QString *qstring_from_substr(const char *str, int start, int end);
 size_t qstring_get_length(const QString *qstring);
 const char *qstring_get_str(const QString *qstring);
 const char *qstring_get_try_str(const QString *qstring);
+const char *qobject_get_try_str(const QObject *qstring);
 void qstring_append_int(QString *qstring, int64_t value);
 void qstring_append(QString *qstring, const char *str);
 void qstring_append_chr(QString *qstring, int c);
diff --git a/qobject/qstring.c b/qobject/qstring.c
index 7638ab990d..fc6834baa6 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -138,6 +138,17 @@ const char *qstring_get_try_str(const QString *qstring)
     return qstring ? qstring_get_str(qstring) : NULL;
 }
 
+/**
+ * qobject_get_try_str(): Return a pointer of the backstore string
+ *
+ * NOTE: the string will only be returned if the object is valid, and
+ * its type is QString, otherwise NULL is returned.
+ */
+const char *qobject_get_try_str(const QObject *qstring)
+{
+    return qstring_get_try_str(qobject_to_qstring(qstring));
+}
+
 /**
  * qstring_is_equal(): Test whether the two QStrings are equal
  */
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 03/27] qobject: introduce qobject_get_try_str()

[Eric Blake]

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

On 12/19/2017 02:45 AM, Peter Xu wrote:
> A quick way to fetch string from qobject when it's a QString.
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qapi/qmp/qstring.h |  1 +
>  qobject/qstring.c          | 11 +++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
> index a145c8ca00..6517d8e377 100644
> --- a/include/qapi/qmp/qstring.h
> +++ b/include/qapi/qmp/qstring.h
> @@ -28,6 +28,7 @@ QString *qstring_from_substr(const char *str, int start, int end);
>  size_t qstring_get_length(const QString *qstring);
>  const char *qstring_get_str(const QString *qstring);
>  const char *qstring_get_try_str(const QString *qstring);
> +const char *qobject_get_try_str(const QObject *qstring);

The naming is consistent, so I won't reject the patch, but 'try_get_str'
reads better than 'get_try_str'.  Of course, fixing the code base to
read well AND be consistent is a much bigger task, and I'm not asking
you to tackle it.


>  
> +/**
> + * qobject_get_try_str(): Return a pointer of the backstore string

The word "backstore" doesn't appear anywhere in qemu.git, and flags as a
typo.  I'd prefer:

Return a pointer to the corresponding string

or maybe "backing string"

> + *
> + * NOTE: the string will only be returned if the object is valid, and
> + * its type is QString, otherwise NULL is returned.
> + */
> +const char *qobject_get_try_str(const QObject *qstring)
> +{
> +    return qstring_get_try_str(qobject_to_qstring(qstring));
> +}
> +
>  /**
>   * qstring_is_equal(): Test whether the two QStrings are equal
>   */
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC v6 03/27] qobject: introduce qobject_get_try_str()

[Peter Xu]

On Tue, Jan 09, 2018 at 04:50:39PM -0600, Eric Blake wrote:
> On 12/19/2017 02:45 AM, Peter Xu wrote:
> > A quick way to fetch string from qobject when it's a QString.
> > 
> > Reviewed-by: Fam Zheng <famz@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/qapi/qmp/qstring.h |  1 +
> >  qobject/qstring.c          | 11 +++++++++++
> >  2 files changed, 12 insertions(+)
> > 
> > diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
> > index a145c8ca00..6517d8e377 100644
> > --- a/include/qapi/qmp/qstring.h
> > +++ b/include/qapi/qmp/qstring.h
> > @@ -28,6 +28,7 @@ QString *qstring_from_substr(const char *str, int start, int end);
> >  size_t qstring_get_length(const QString *qstring);
> >  const char *qstring_get_str(const QString *qstring);
> >  const char *qstring_get_try_str(const QString *qstring);
> > +const char *qobject_get_try_str(const QObject *qstring);
> 
> The naming is consistent, so I won't reject the patch, but 'try_get_str'
> reads better than 'get_try_str'.  Of course, fixing the code base to
> read well AND be consistent is a much bigger task, and I'm not asking
> you to tackle it.

I agree.

> 
> 
> >  
> > +/**
> > + * qobject_get_try_str(): Return a pointer of the backstore string
> 
> The word "backstore" doesn't appear anywhere in qemu.git, and flags as a
> typo.  I'd prefer:
> 
> Return a pointer to the corresponding string

I'll take this one.  Thanks,

> 
> or maybe "backing string"

-- 
Peter Xu

[Qemu-devel] [RFC v6 04/27] qobject: let object_property_get_str() use new API

[Peter Xu]

We can simplify object_property_get_str() using the new
qobject_get_try_str().

Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 qom/object.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index c58c52d518..9cbeb51f0b 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1116,18 +1116,15 @@ char *object_property_get_str(Object *obj, const char *name,
                               Error **errp)
 {
     QObject *ret = object_property_get_qobject(obj, name, errp);
-    QString *qstring;
     char *retval;
 
     if (!ret) {
         return NULL;
     }
-    qstring = qobject_to_qstring(ret);
-    if (!qstring) {
+
+    retval = g_strdup(qobject_get_try_str(ret));
+    if (!retval) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name, "string");
-        retval = NULL;
-    } else {
-        retval = g_strdup(qstring_get_str(qstring));
     }
 
     qobject_decref(ret);
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 04/27] qobject: let object_property_get_str() use new API

[Eric Blake]

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

On 12/19/2017 02:45 AM, Peter Xu wrote:
> We can simplify object_property_get_str() using the new
> qobject_get_try_str().
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  qom/object.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

I'm not opposed to your patch split (particularly since it makes
backports easier if it just needs the new function and then your later
uses of the new function, without touching existing uses); but I might
have merged this with the previous patch so that the new API has a
client right away, proving why the new API is worthwhile as part of its
introduction.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC v6 04/27] qobject: let object_property_get_str() use new API

[Peter Xu]

On Tue, Jan 09, 2018 at 04:53:40PM -0600, Eric Blake wrote:
> On 12/19/2017 02:45 AM, Peter Xu wrote:
> > We can simplify object_property_get_str() using the new
> > qobject_get_try_str().
> > 
> > Reviewed-by: Fam Zheng <famz@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  qom/object.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> I'm not opposed to your patch split (particularly since it makes
> backports easier if it just needs the new function and then your later
> uses of the new function, without touching existing uses); but I might
> have merged this with the previous patch so that the new API has a
> client right away, proving why the new API is worthwhile as part of its
> introduction.

Sure, I'll follow the rule next time.

(I can simply squash it but I got a few r-bs for separate patches
 already, so I'll keep them separated for now)

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 04/27] qobject: let object_property_get_str() use new API

[Eric Blake]

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

On 01/10/2018 01:57 AM, Peter Xu wrote:
> On Tue, Jan 09, 2018 at 04:53:40PM -0600, Eric Blake wrote:
>> On 12/19/2017 02:45 AM, Peter Xu wrote:
>>> We can simplify object_property_get_str() using the new
>>> qobject_get_try_str().
>>>
>>> Reviewed-by: Fam Zheng <famz@redhat.com>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>  qom/object.c | 9 +++------
>>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> I'm not opposed to your patch split (particularly since it makes
>> backports easier if it just needs the new function and then your later
>> uses of the new function, without touching existing uses); but I might
>> have merged this with the previous patch so that the new API has a
>> client right away, proving why the new API is worthwhile as part of its
>> introduction.
> 
> Sure, I'll follow the rule next time.
> 
> (I can simply squash it but I got a few r-bs for separate patches
>  already, so I'll keep them separated for now)

Keeping it separate is fine, especially since you have r-b.

Another factor in my personal workflow - if I'm going to be converting
lots of users, but the users span over multiple areas of the code, then
introducing the helper in one patch, and then a series of conversions to
start using it, makes sense.  But if I'm converting just one existing
user, that's a refactoring, and doing it all at once is easier (prior to
adding the new code that becomes the second user).  I guess the moral of
the story is that patch splitting is an art form, and there's more than
one way to do things that still end up nice and reviewable.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC v6 04/27] qobject: let object_property_get_str() use new API

[Peter Xu]

On Wed, Jan 10, 2018 at 06:59:13AM -0600, Eric Blake wrote:
> On 01/10/2018 01:57 AM, Peter Xu wrote:
> > On Tue, Jan 09, 2018 at 04:53:40PM -0600, Eric Blake wrote:
> >> On 12/19/2017 02:45 AM, Peter Xu wrote:
> >>> We can simplify object_property_get_str() using the new
> >>> qobject_get_try_str().
> >>>
> >>> Reviewed-by: Fam Zheng <famz@redhat.com>
> >>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> >>> Signed-off-by: Peter Xu <peterx@redhat.com>
> >>> ---
> >>>  qom/object.c | 9 +++------
> >>>  1 file changed, 3 insertions(+), 6 deletions(-)
> >>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >>
> >> I'm not opposed to your patch split (particularly since it makes
> >> backports easier if it just needs the new function and then your later
> >> uses of the new function, without touching existing uses); but I might
> >> have merged this with the previous patch so that the new API has a
> >> client right away, proving why the new API is worthwhile as part of its
> >> introduction.
> > 
> > Sure, I'll follow the rule next time.
> > 
> > (I can simply squash it but I got a few r-bs for separate patches
> >  already, so I'll keep them separated for now)
> 
> Keeping it separate is fine, especially since you have r-b.
> 
> Another factor in my personal workflow - if I'm going to be converting
> lots of users, but the users span over multiple areas of the code, then
> introducing the helper in one patch, and then a series of conversions to
> start using it, makes sense.  But if I'm converting just one existing
> user, that's a refactoring, and doing it all at once is easier (prior to
> adding the new code that becomes the second user).  I guess the moral of
> the story is that patch splitting is an art form, and there's more than
> one way to do things that still end up nice and reviewable.

Agreed.  I started to learn how to split patches since when I started
to work on QEMU, and obvious now I'm still learning. :-)

Thanks Eric.

-- 
Peter Xu

[Qemu-devel] [RFC v6 05/27] monitor: move skip_flush into monitor_data_init

[Peter Xu]

It's part of the data init.  Collect it.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index d682eee2d8..0030ded8f1 100644
--- a/monitor.c
+++ b/monitor.c
@@ -567,13 +567,14 @@ static void monitor_qapi_event_init(void)
 
 static void handle_hmp_command(Monitor *mon, const char *cmdline);
 
-static void monitor_data_init(Monitor *mon)
+static void monitor_data_init(Monitor *mon, bool skip_flush)
 {
     memset(mon, 0, sizeof(Monitor));
     qemu_mutex_init(&mon->out_lock);
     mon->outbuf = qstring_new();
     /* Use *mon_cmds by default. */
     mon->cmd_table = mon_cmds;
+    mon->skip_flush = skip_flush;
 }
 
 static void monitor_data_destroy(Monitor *mon)
@@ -594,8 +595,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
     char *output = NULL;
     Monitor *old_mon, hmp;
 
-    monitor_data_init(&hmp);
-    hmp.skip_flush = true;
+    monitor_data_init(&hmp, true);
 
     old_mon = cur_mon;
     cur_mon = &hmp;
@@ -4088,7 +4088,7 @@ void monitor_init(Chardev *chr, int flags)
     }
 
     mon = g_malloc(sizeof(*mon));
-    monitor_data_init(mon);
+    monitor_data_init(mon, false);
 
     qemu_chr_fe_init(&mon->chr, chr, &error_abort);
     mon->flags = flags;
-- 
2.14.3

[Qemu-devel] [RFC v6 06/27] monitor: move the cur_mon hack deeper for QMP

[Peter Xu]

In monitor_qmp_read(), we have the hack to temporarily replace the
cur_mon pointer.  Now we move this hack deeper inside the QMP dispatcher
routine since the Monitor pointer can be actually obtained using
container_of() upon the parser object, just like most of the other JSON
parser users do.

This does not make much sense as a single patch.  However, this will be
a big step for the next patch, when the QMP dispatcher routine will be
split from the QMP parser.

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

diff --git a/monitor.c b/monitor.c
index 0030ded8f1..35a2aef0fe 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3811,7 +3811,9 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
 {
     QObject *req, *rsp = NULL, *id = NULL;
     QDict *qdict = NULL;
-    Monitor *mon = cur_mon;
+    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
+    Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
+
     Error *err = NULL;
 
     req = json_parser_parse_err(tokens, NULL, &err);
@@ -3836,8 +3838,13 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
         QDECREF(req_json);
     }
 
+    old_mon = cur_mon;
+    cur_mon = mon;
+
     rsp = qmp_dispatch(cur_mon->qmp.commands, req);
 
+    cur_mon = old_mon;
+
     if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
         qdict = qdict_get_qdict(qobject_to_qdict(rsp), "error");
         if (qdict
@@ -3874,13 +3881,9 @@ err_out:
 
 static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
 {
-    Monitor *old_mon = cur_mon;
-
-    cur_mon = opaque;
-
-    json_message_parser_feed(&cur_mon->qmp.parser, (const char *) buf, size);
+    Monitor *mon = opaque;
 
-    cur_mon = old_mon;
+    json_message_parser_feed(&mon->qmp.parser, (const char *) buf, size);
 }
 
 static void monitor_read(void *opaque, const uint8_t *buf, int size)
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 06/27] monitor: move the cur_mon hack deeper for QMP

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:36PM +0800, Peter Xu wrote:
> In monitor_qmp_read(), we have the hack to temporarily replace the
> cur_mon pointer.  Now we move this hack deeper inside the QMP dispatcher
> routine since the Monitor pointer can be actually obtained using
> container_of() upon the parser object, just like most of the other JSON
> parser users do.
> 
> This does not make much sense as a single patch.  However, this will be
> a big step for the next patch, when the QMP dispatcher routine will be
> split from the QMP parser.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)

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

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

Re: [Qemu-devel] [RFC v6 06/27] monitor: move the cur_mon hack deeper for QMP

[Eric Blake]

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

On 12/19/2017 02:45 AM, Peter Xu wrote:
> In monitor_qmp_read(), we have the hack to temporarily replace the
> cur_mon pointer.  Now we move this hack deeper inside the QMP dispatcher
> routine since the Monitor pointer can be actually obtained using
> container_of() upon the parser object, just like most of the other JSON
> parser users do.
> 
> This does not make much sense as a single patch.  However, this will be
> a big step for the next patch, when the QMP dispatcher routine will be
> split from the QMP parser.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[Qemu-devel] [RFC v6 07/27] monitor: unify global init

[Peter Xu]

There are many places for monitor init its globals, at least:

- monitor_init_qmp_commands() at the very beginning
- single function to init monitor_lock
- in the first entry of monitor_init() using "is_first_init"

Unify them a bit.

Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/monitor/monitor.h |  2 +-
 monitor.c                 | 25 ++++++++++---------------
 vl.c                      |  7 ++++++-
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 83ea4a1aaf..3a5128ab1b 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -16,7 +16,7 @@ extern Monitor *cur_mon;
 
 bool monitor_cur_is_qmp(void);
 
-void monitor_init_qmp_commands(void);
+void monitor_init_globals(void);
 void monitor_init(Chardev *chr, int flags);
 void monitor_cleanup(void);
 
diff --git a/monitor.c b/monitor.c
index 35a2aef0fe..bc0cb9f223 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1000,7 +1000,7 @@ static void qmp_unregister_commands_hack(void)
 #endif
 }
 
-void monitor_init_qmp_commands(void)
+static void monitor_init_qmp_commands(void)
 {
     /*
      * Two command lists:
@@ -4034,6 +4034,14 @@ static void sortcmdlist(void)
     qsort((void *)info_cmds, array_num, elem_size, compare_mon_cmd);
 }
 
+void monitor_init_globals(void)
+{
+    monitor_init_qmp_commands();
+    monitor_qapi_event_init();
+    sortcmdlist();
+    qemu_mutex_init(&monitor_lock);
+}
+
 /* These functions just adapt the readline interface in a typesafe way.  We
  * could cast function pointers but that discards compiler checks.
  */
@@ -4074,23 +4082,10 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap)
     }
 }
 
-static void __attribute__((constructor)) monitor_lock_init(void)
-{
-    qemu_mutex_init(&monitor_lock);
-}
-
 void monitor_init(Chardev *chr, int flags)
 {
-    static int is_first_init = 1;
-    Monitor *mon;
-
-    if (is_first_init) {
-        monitor_qapi_event_init();
-        sortcmdlist();
-        is_first_init = 0;
-    }
+    Monitor *mon = g_malloc(sizeof(*mon));
 
-    mon = g_malloc(sizeof(*mon));
     monitor_data_init(mon, false);
 
     qemu_chr_fe_init(&mon->chr, chr, &error_abort);
diff --git a/vl.c b/vl.c
index e9012bb009..2d89391351 100644
--- a/vl.c
+++ b/vl.c
@@ -3099,7 +3099,6 @@ int main(int argc, char **argv, char **envp)
     qemu_init_exec_dir(argv[0]);
 
     module_call_init(MODULE_INIT_QOM);
-    monitor_init_qmp_commands();
 
     qemu_add_opts(&qemu_drive_opts);
     qemu_add_drive_opts(&qemu_legacy_drive_opts);
@@ -4645,6 +4644,12 @@ int main(int argc, char **argv, char **envp)
     default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
     default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
 
+    /*
+     * Note: qtest_enabled() (which used in monitor_qapi_event_init())
+     * depend on configure_accelerator() above.
+     */
+    monitor_init_globals();
+
     if (qemu_opts_foreach(qemu_find_opts("mon"),
                           mon_init_func, NULL, NULL)) {
         exit(1);
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 07/27] monitor: unify global init

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:37PM +0800, Peter Xu wrote:
> There are many places for monitor init its globals, at least:
> 
> - monitor_init_qmp_commands() at the very beginning
> - single function to init monitor_lock
> - in the first entry of monitor_init() using "is_first_init"
> 
> Unify them a bit.
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/monitor/monitor.h |  2 +-
>  monitor.c                 | 25 ++++++++++---------------
>  vl.c                      |  7 ++++++-
>  3 files changed, 17 insertions(+), 17 deletions(-)

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

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

Re: [Qemu-devel] [RFC v6 07/27] monitor: unify global init

[Eric Blake]

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

On 12/19/2017 02:45 AM, Peter Xu wrote:
> There are many places for monitor init its globals, at least:

Reads awkwardly; maybe:

...many places where the monitor initializes its globals,...

> 
> - monitor_init_qmp_commands() at the very beginning
> - single function to init monitor_lock
> - in the first entry of monitor_init() using "is_first_init"
> 
> Unify them a bit.
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---

> +void monitor_init_globals(void)
> +{
> +    monitor_init_qmp_commands();
> +    monitor_qapi_event_init();
> +    sortcmdlist();
> +    qemu_mutex_init(&monitor_lock);
> +}
> +
>  /* These functions just adapt the readline interface in a typesafe way.  We
>   * could cast function pointers but that discards compiler checks.
>   */
> @@ -4074,23 +4082,10 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap)
>      }
>  }
>  
> -static void __attribute__((constructor)) monitor_lock_init(void)
> -{
> -    qemu_mutex_init(&monitor_lock);
> -}

The later initialization of the monitor_lock mutex is a potential
semantic change.  Please beef up the commit message to document why it
is safe.  In fact, I requested this back on my review of v1, but it
still hasn't been done. :(

https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg05421.html

If my read of history is correct, I think it is sufficient to point to
commit 05875687 as a place where we no longer care about constructor
semantics because we are no longer dealing with module_call_init().  But
you may find a better place to point to.  You already found that
d622cb587 was what introduced the constructor in the first place, but I
didn't spend time today auditing the state of qemu back at that time to
see if the constructor was really necessary back then or just a
convenience for lack of a better initialization point.

Alternatively, if you can't find a good commit message to point to, at
least document how you (and I) tested things, using gdb watchpoints, to
prove it is a safe delay.

Only if you improve the commit message, you may add:
Reviewed-by: Eric Blake <eblake@redhat.com>

> +++ b/vl.c
> @@ -3099,7 +3099,6 @@ int main(int argc, char **argv, char **envp)
>      qemu_init_exec_dir(argv[0]);
>  
>      module_call_init(MODULE_INIT_QOM);
> -    monitor_init_qmp_commands();
>  
>      qemu_add_opts(&qemu_drive_opts);
>      qemu_add_drive_opts(&qemu_legacy_drive_opts);
> @@ -4645,6 +4644,12 @@ int main(int argc, char **argv, char **envp)
>      default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
>      default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
>  
> +    /*
> +     * Note: qtest_enabled() (which used in monitor_qapi_event_init())

s/which/which is/

> +     * depend on configure_accelerator() above.

s/depend/depends/

> +     */
> +    monitor_init_globals();
> +
>      if (qemu_opts_foreach(qemu_find_opts("mon"),
>                            mon_init_func, NULL, NULL)) {
>          exit(1);
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC v6 07/27] monitor: unify global init

[Peter Xu]

On Tue, Jan 09, 2018 at 05:13:40PM -0600, Eric Blake wrote:
> On 12/19/2017 02:45 AM, Peter Xu wrote:
> > There are many places for monitor init its globals, at least:
> 
> Reads awkwardly; maybe:
> 
> ...many places where the monitor initializes its globals,...

Fixed.

> 
> > 
> > - monitor_init_qmp_commands() at the very beginning
> > - single function to init monitor_lock
> > - in the first entry of monitor_init() using "is_first_init"
> > 
> > Unify them a bit.
> > 
> > Reviewed-by: Fam Zheng <famz@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> 
> > +void monitor_init_globals(void)
> > +{
> > +    monitor_init_qmp_commands();
> > +    monitor_qapi_event_init();
> > +    sortcmdlist();
> > +    qemu_mutex_init(&monitor_lock);
> > +}
> > +
> >  /* These functions just adapt the readline interface in a typesafe way.  We
> >   * could cast function pointers but that discards compiler checks.
> >   */
> > @@ -4074,23 +4082,10 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap)
> >      }
> >  }
> >  
> > -static void __attribute__((constructor)) monitor_lock_init(void)
> > -{
> > -    qemu_mutex_init(&monitor_lock);
> > -}
> 
> The later initialization of the monitor_lock mutex is a potential
> semantic change.  Please beef up the commit message to document why it
> is safe.  In fact, I requested this back on my review of v1, but it
> still hasn't been done. :(
> 
> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg05421.html

Sorry for that! I thought you helped proved that somehow (which I
really appreciate)...

> 
> If my read of history is correct, I think it is sufficient to point to
> commit 05875687 as a place where we no longer care about constructor
> semantics because we are no longer dealing with module_call_init().  But
> you may find a better place to point to.  You already found that
> d622cb587 was what introduced the constructor in the first place, but I
> didn't spend time today auditing the state of qemu back at that time to
> see if the constructor was really necessary back then or just a
> convenience for lack of a better initialization point.
> 
> Alternatively, if you can't find a good commit message to point to, at
> least document how you (and I) tested things, using gdb watchpoints, to
> prove it is a safe delay.

I did that by observing all users of the lock in current repository:

*** monitor.c:
monitor_qmp_response_pop_one[457] qemu_mutex_lock(&monitor_lock);
monitor_qmp_response_pop_one[466] qemu_mutex_unlock(&monitor_lock);
monitor_qapi_event_queue[529]  qemu_mutex_lock(&monitor_lock);
monitor_qapi_event_queue[574]  qemu_mutex_unlock(&monitor_lock);
monitor_qapi_event_handler[588] qemu_mutex_lock(&monitor_lock);
monitor_qapi_event_handler[604] qemu_mutex_unlock(&monitor_lock);
monitor_qmp_requests_pop_one[4116] qemu_mutex_lock(&monitor_lock);
monitor_qmp_requests_pop_one[4136] qemu_mutex_unlock(&monitor_lock);
monitor_init[4559]             qemu_mutex_lock(&monitor_lock);
monitor_init[4561]             qemu_mutex_unlock(&monitor_lock);
monitor_cleanup[4596]          qemu_mutex_lock(&monitor_lock);
monitor_cleanup[4603]          qemu_mutex_unlock(&monitor_lock);

monitor_init() and monitor_cleanup() are called after global init, so
it should be fine (monitor_init() is called right after the global
init).

For the rest of the functions:

    monitor_qmp_response_pop_one
    monitor_qapi_event_queue
    monitor_qapi_event_handler
    monitor_qmp_requests_pop_one

AFAIK all of them are called even after monitor_init(), in other
words, they are all after global init too.

As a conclusion, we should be safe here.  Again, I may be wrong
somewhere, please correct me if so.

> 
> Only if you improve the commit message, you may add:
> Reviewed-by: Eric Blake <eblake@redhat.com>

Besides the English fix, how about I add one more paragraph to talk
about monitor_lock in commit message:

  monitor_lock won't be used before monitor_init().  So as long as we
  initialize the monitor globals before the first call to
  monitor_init(), we will be safe.

With that, could I take your r-b?

Thanks,

> 
> > +++ b/vl.c
> > @@ -3099,7 +3099,6 @@ int main(int argc, char **argv, char **envp)
> >      qemu_init_exec_dir(argv[0]);
> >  
> >      module_call_init(MODULE_INIT_QOM);
> > -    monitor_init_qmp_commands();
> >  
> >      qemu_add_opts(&qemu_drive_opts);
> >      qemu_add_drive_opts(&qemu_legacy_drive_opts);
> > @@ -4645,6 +4644,12 @@ int main(int argc, char **argv, char **envp)
> >      default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
> >      default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
> >  
> > +    /*
> > +     * Note: qtest_enabled() (which used in monitor_qapi_event_init())
> 
> s/which/which is/
> 
> > +     * depend on configure_accelerator() above.
> 
> s/depend/depends/
> 
> > +     */
> > +    monitor_init_globals();
> > +
> >      if (qemu_opts_foreach(qemu_find_opts("mon"),
> >                            mon_init_func, NULL, NULL)) {
> >          exit(1);
> > 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 




-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 07/27] monitor: unify global init

[Eric Blake]

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

On 01/10/2018 02:26 AM, Peter Xu wrote:

>> The later initialization of the monitor_lock mutex is a potential
>> semantic change.  Please beef up the commit message to document why it
>> is safe.  In fact, I requested this back on my review of v1, but it
>> still hasn't been done. :(
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg05421.html
> 
> Sorry for that! I thought you helped proved that somehow (which I
> really appreciate)...
> 
>>
>> If my read of history is correct, I think it is sufficient to point to
>> commit 05875687 as a place where we no longer care about constructor
>> semantics because we are no longer dealing with module_call_init().  But
>> you may find a better place to point to.  You already found that
>> d622cb587 was what introduced the constructor in the first place, but I
>> didn't spend time today auditing the state of qemu back at that time to
>> see if the constructor was really necessary back then or just a
>> convenience for lack of a better initialization point.
>>
>> Alternatively, if you can't find a good commit message to point to, at
>> least document how you (and I) tested things, using gdb watchpoints, to
>> prove it is a safe delay.
> 
> I did that by observing all users of the lock in current repository:


> AFAIK all of them are called even after monitor_init(), in other
> words, they are all after global init too.
> 
> As a conclusion, we should be safe here.  Again, I may be wrong
> somewhere, please correct me if so.

My gdb testing and your analysis match; we're safe.  So all that's
needed is the paragraph documenting that we thought about the issue:

> 
>>
>> Only if you improve the commit message, you may add:
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Besides the English fix, how about I add one more paragraph to talk
> about monitor_lock in commit message:
> 
>   monitor_lock won't be used before monitor_init().  So as long as we
>   initialize the monitor globals before the first call to
>   monitor_init(), we will be safe.

Or even:

monitor_lock is not used before monitor_init() (as confirmed by code
analysis and gdb watchpoints); so we are safe delaying what was a
constructor-time initialization of the mutex into the later first call
to monitor_init().

> 
> With that, could I take your r-b?

Yes.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC v6 07/27] monitor: unify global init

[Peter Xu]

On Wed, Jan 10, 2018 at 06:54:45AM -0600, Eric Blake wrote:
> On 01/10/2018 02:26 AM, Peter Xu wrote:
> 
> >> The later initialization of the monitor_lock mutex is a potential
> >> semantic change.  Please beef up the commit message to document why it
> >> is safe.  In fact, I requested this back on my review of v1, but it
> >> still hasn't been done. :(
> >>
> >> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg05421.html
> > 
> > Sorry for that! I thought you helped proved that somehow (which I
> > really appreciate)...
> > 
> >>
> >> If my read of history is correct, I think it is sufficient to point to
> >> commit 05875687 as a place where we no longer care about constructor
> >> semantics because we are no longer dealing with module_call_init().  But
> >> you may find a better place to point to.  You already found that
> >> d622cb587 was what introduced the constructor in the first place, but I
> >> didn't spend time today auditing the state of qemu back at that time to
> >> see if the constructor was really necessary back then or just a
> >> convenience for lack of a better initialization point.
> >>
> >> Alternatively, if you can't find a good commit message to point to, at
> >> least document how you (and I) tested things, using gdb watchpoints, to
> >> prove it is a safe delay.
> > 
> > I did that by observing all users of the lock in current repository:
> 
> 
> > AFAIK all of them are called even after monitor_init(), in other
> > words, they are all after global init too.
> > 
> > As a conclusion, we should be safe here.  Again, I may be wrong
> > somewhere, please correct me if so.
> 
> My gdb testing and your analysis match; we're safe.  So all that's
> needed is the paragraph documenting that we thought about the issue:
> 
> > 
> >>
> >> Only if you improve the commit message, you may add:
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> > 
> > Besides the English fix, how about I add one more paragraph to talk
> > about monitor_lock in commit message:
> > 
> >   monitor_lock won't be used before monitor_init().  So as long as we
> >   initialize the monitor globals before the first call to
> >   monitor_init(), we will be safe.
> 
> Or even:
> 
> monitor_lock is not used before monitor_init() (as confirmed by code
> analysis and gdb watchpoints); so we are safe delaying what was a
> constructor-time initialization of the mutex into the later first call
> to monitor_init().

Will take away this.

> 
> > 
> > With that, could I take your r-b?
> 
> Yes.

Thank you.

-- 
Peter Xu

[Qemu-devel] [RFC v6 08/27] monitor: let mon_list be tail queue

[Peter Xu]

It was QLIST.  I want to use this list to do monitor priority job later,
which need tail insertion ability.  So switching to a tail queue.

Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/monitor.c b/monitor.c
index bc0cb9f223..95fc54d97f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -204,7 +204,7 @@ struct Monitor {
     void *password_opaque;
     mon_cmd_t *cmd_table;
     QLIST_HEAD(,mon_fd_t) fds;
-    QLIST_ENTRY(Monitor) entry;
+    QTAILQ_ENTRY(Monitor) entry;
 };
 
 /* QMP checker flags */
@@ -213,7 +213,7 @@ struct Monitor {
 /* Protects mon_list, monitor_event_state.  */
 static QemuMutex monitor_lock;
 
-static QLIST_HEAD(mon_list, Monitor) mon_list;
+static QTAILQ_HEAD(mon_list, Monitor) mon_list;
 static QLIST_HEAD(mon_fdsets, MonFdset) mon_fdsets;
 static int mon_refcount;
 
@@ -414,7 +414,7 @@ static void monitor_qapi_event_emit(QAPIEvent event, QDict *qdict)
     Monitor *mon;
 
     trace_monitor_protocol_event_emit(event, qdict);
-    QLIST_FOREACH(mon, &mon_list, entry) {
+    QTAILQ_FOREACH(mon, &mon_list, entry) {
         if (monitor_is_qmp(mon)
             && mon->qmp.commands != &qmp_cap_negotiation_commands) {
             monitor_json_emitter(mon, QOBJECT(qdict));
@@ -4118,8 +4118,8 @@ void monitor_cleanup(void)
     Monitor *mon, *next;
 
     qemu_mutex_lock(&monitor_lock);
-    QLIST_FOREACH_SAFE(mon, &mon_list, entry, next) {
-        QLIST_REMOVE(mon, entry);
+    QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
+        QTAILQ_REMOVE(&mon_list, mon, entry);
         monitor_data_destroy(mon);
         g_free(mon);
     }
-- 
2.14.3

[Qemu-devel] [RFC v6 09/27] monitor: create monitor dedicate iothread

[Peter Xu]

Create one IOThread for the monitors, prepared to handle all the
input/output IOs using existing iothread framework.

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

diff --git a/monitor.c b/monitor.c
index 95fc54d97f..4ebd83fd94 100644
--- a/monitor.c
+++ b/monitor.c
@@ -75,6 +75,7 @@
 #include "qmp-introspect.h"
 #include "sysemu/qtest.h"
 #include "sysemu/cpus.h"
+#include "sysemu/iothread.h"
 #include "qemu/cutils.h"
 #include "qapi/qmp/dispatch.h"
 
@@ -207,6 +208,11 @@ struct Monitor {
     QTAILQ_ENTRY(Monitor) entry;
 };
 
+/* Let's add monitor global variables to this struct. */
+static struct {
+    IOThread *mon_iothread;
+} mon_global;
+
 /* QMP checker flags */
 #define QMP_ACCEPT_UNKNOWNS 1
 
@@ -4034,12 +4040,24 @@ static void sortcmdlist(void)
     qsort((void *)info_cmds, array_num, elem_size, compare_mon_cmd);
 }
 
+static GMainContext *monitor_io_context_get(void)
+{
+    return iothread_get_g_main_context(mon_global.mon_iothread);
+}
+
+static void monitor_iothread_init(void)
+{
+    mon_global.mon_iothread = iothread_create("mon_iothread",
+                                              &error_abort);
+}
+
 void monitor_init_globals(void)
 {
     monitor_init_qmp_commands();
     monitor_qapi_event_init();
     sortcmdlist();
     qemu_mutex_init(&monitor_lock);
+    monitor_iothread_init();
 }
 
 /* These functions just adapt the readline interface in a typesafe way.  We
@@ -4117,6 +4135,14 @@ void monitor_cleanup(void)
 {
     Monitor *mon, *next;
 
+    /*
+     * We need to explicitly stop the iothread (but not destroy it),
+     * cleanup the monitor resources, then destroy the iothread since
+     * we need to unregister from chardev below in
+     * monitor_data_destroy(), and chardev is not thread-safe yet
+     */
+    iothread_stop(mon_global.mon_iothread);
+
     qemu_mutex_lock(&monitor_lock);
     QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
         QTAILQ_REMOVE(&mon_list, mon, entry);
@@ -4124,6 +4150,9 @@ void monitor_cleanup(void)
         g_free(mon);
     }
     qemu_mutex_unlock(&monitor_lock);
+
+    iothread_destroy(mon_global.mon_iothread);
+    mon_global.mon_iothread = NULL;
 }
 
 QemuOptsList qemu_mon_opts = {
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 09/27] monitor: create monitor dedicate iothread

[Fam Zheng]

On Tue, 12/19 16:45, Peter Xu wrote:
> Create one IOThread for the monitors, prepared to handle all the
> input/output IOs using existing iothread framework.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/monitor.c b/monitor.c
> index 95fc54d97f..4ebd83fd94 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -75,6 +75,7 @@
>  #include "qmp-introspect.h"
>  #include "sysemu/qtest.h"
>  #include "sysemu/cpus.h"
> +#include "sysemu/iothread.h"
>  #include "qemu/cutils.h"
>  #include "qapi/qmp/dispatch.h"
>  
> @@ -207,6 +208,11 @@ struct Monitor {
>      QTAILQ_ENTRY(Monitor) entry;
>  };
>  
> +/* Let's add monitor global variables to this struct. */
> +static struct {
> +    IOThread *mon_iothread;
> +} mon_global;
> +
>  /* QMP checker flags */
>  #define QMP_ACCEPT_UNKNOWNS 1
>  
> @@ -4034,12 +4040,24 @@ static void sortcmdlist(void)
>      qsort((void *)info_cmds, array_num, elem_size, compare_mon_cmd);
>  }
>  
> +static GMainContext *monitor_io_context_get(void)
> +{
> +    return iothread_get_g_main_context(mon_global.mon_iothread);
> +}
> +
> +static void monitor_iothread_init(void)
> +{
> +    mon_global.mon_iothread = iothread_create("mon_iothread",
> +                                              &error_abort);
> +}
> +
>  void monitor_init_globals(void)
>  {
>      monitor_init_qmp_commands();
>      monitor_qapi_event_init();
>      sortcmdlist();
>      qemu_mutex_init(&monitor_lock);
> +    monitor_iothread_init();
>  }
>  
>  /* These functions just adapt the readline interface in a typesafe way.  We
> @@ -4117,6 +4135,14 @@ void monitor_cleanup(void)
>  {
>      Monitor *mon, *next;
>  
> +    /*
> +     * We need to explicitly stop the iothread (but not destroy it),
> +     * cleanup the monitor resources, then destroy the iothread since
> +     * we need to unregister from chardev below in
> +     * monitor_data_destroy(), and chardev is not thread-safe yet
> +     */
> +    iothread_stop(mon_global.mon_iothread);
> +
>      qemu_mutex_lock(&monitor_lock);
>      QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
>          QTAILQ_REMOVE(&mon_list, mon, entry);
> @@ -4124,6 +4150,9 @@ void monitor_cleanup(void)
>          g_free(mon);
>      }
>      qemu_mutex_unlock(&monitor_lock);
> +
> +    iothread_destroy(mon_global.mon_iothread);
> +    mon_global.mon_iothread = NULL;
>  }
>  
>  QemuOptsList qemu_mon_opts = {
> -- 
> 2.14.3
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [RFC v6 09/27] monitor: create monitor dedicate iothread

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:39PM +0800, Peter Xu wrote:
> Create one IOThread for the monitors, prepared to handle all the
> input/output IOs using existing iothread framework.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)

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

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

Re: [Qemu-devel] [RFC v6 09/27] monitor: create monitor dedicate iothread

[Eric Blake]

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

On 12/19/2017 02:45 AM, Peter Xu wrote:

Grammar in the subject:

s/monitor dedicate/a dedicated monitor/

> Create one IOThread for the monitors, prepared to handle all the
> input/output IOs using existing iothread framework.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 

> @@ -207,6 +208,11 @@ struct Monitor {
>      QTAILQ_ENTRY(Monitor) entry;
>  };
>  
> +/* Let's add monitor global variables to this struct. */

Is this comment a placeholder for future additions in later patches of
the series?  If so, a temporary TODO or FIXME might make it easier to
track that this patch is part of a series; if not, you could delete the
comment altogether.

> +static struct {
> +    IOThread *mon_iothread;
> +} mon_global;
> +

Up to you what to do about the comment; tweaking it (or leaving it
unchanged) is not a semantic change, so I'm fine with:

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC v6 09/27] monitor: create monitor dedicate iothread

[Peter Xu]

On Tue, Jan 09, 2018 at 05:31:18PM -0600, Eric Blake wrote:
> On 12/19/2017 02:45 AM, Peter Xu wrote:
> 
> Grammar in the subject:
> 
> s/monitor dedicate/a dedicated monitor/
> 
> > Create one IOThread for the monitors, prepared to handle all the
> > input/output IOs using existing iothread framework.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  monitor.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> 
> > @@ -207,6 +208,11 @@ struct Monitor {
> >      QTAILQ_ENTRY(Monitor) entry;
> >  };
> >  
> > +/* Let's add monitor global variables to this struct. */
> 
> Is this comment a placeholder for future additions in later patches of
> the series?  If so, a temporary TODO or FIXME might make it easier to
> track that this patch is part of a series; if not, you could delete the
> comment altogether.

mon_iothread is the first element of struct already so IMHO it's not
really a TODO.  The series will add some more things inside though
(and hoping that it can be a good place to unify global monitor stuff
in the future).  Actually the comment is a suggestion from Stefan and
either with/without it works for me.

> 
> > +static struct {
> > +    IOThread *mon_iothread;
> > +} mon_global;
> > +
> 
> Up to you what to do about the comment; tweaking it (or leaving it
> unchanged) is not a semantic change, so I'm fine with:
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

So I'm taking the R-b if you won't disagree and keep everything
untouched.  Thanks,

-- 
Peter Xu

[Qemu-devel] [RFC v6 10/27] monitor: allow to use IO thread for parsing

[Peter Xu]

For each Monitor, add one field "use_io_thr" to show whether it will be
using the dedicated monitor IO thread to handle input/output.  When set,
monitor IO parsing work will be offloaded to dedicated monitor IO
thread, rather than the original main loop thread.

This only works for QMP.  HMP will always be run on main loop thread.

Currently we're still keeping use_io_thr to off always.  Will turn it on
later at some point.

One thing to mention is that we cannot set use_io_thr for every QMP
monitors.  The problem is that MUXed typed chardevs may not work well
with it now. When MUX is used, frontend of chardev can be the monitor
plus something else.  The only thing we know would be safe to be run
outside main thread so far is the monitor frontend. All the rest of the
frontends should still be run in main thread only.

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

diff --git a/monitor.c b/monitor.c
index 4ebd83fd94..4b2bee773f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -190,6 +190,7 @@ struct Monitor {
     int flags;
     int suspend_cnt;
     bool skip_flush;
+    bool use_io_thr;
 
     QemuMutex out_lock;
     QString *outbuf;
@@ -573,7 +574,8 @@ static void monitor_qapi_event_init(void)
 
 static void handle_hmp_command(Monitor *mon, const char *cmdline);
 
-static void monitor_data_init(Monitor *mon, bool skip_flush)
+static void monitor_data_init(Monitor *mon, bool skip_flush,
+                              bool use_io_thr)
 {
     memset(mon, 0, sizeof(Monitor));
     qemu_mutex_init(&mon->out_lock);
@@ -581,6 +583,7 @@ static void monitor_data_init(Monitor *mon, bool skip_flush)
     /* Use *mon_cmds by default. */
     mon->cmd_table = mon_cmds;
     mon->skip_flush = skip_flush;
+    mon->use_io_thr = use_io_thr;
 }
 
 static void monitor_data_destroy(Monitor *mon)
@@ -601,7 +604,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
     char *output = NULL;
     Monitor *old_mon, hmp;
 
-    monitor_data_init(&hmp, true);
+    monitor_data_init(&hmp, true, false);
 
     old_mon = cur_mon;
     cur_mon = &hmp;
@@ -4103,8 +4106,9 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap)
 void monitor_init(Chardev *chr, int flags)
 {
     Monitor *mon = g_malloc(sizeof(*mon));
+    GMainContext *context;
 
-    monitor_data_init(mon, false);
+    monitor_data_init(mon, false, false);
 
     qemu_chr_fe_init(&mon->chr, chr, &error_abort);
     mon->flags = flags;
@@ -4116,19 +4120,38 @@ void monitor_init(Chardev *chr, int flags)
         monitor_read_command(mon, 0);
     }
 
+    if (mon->use_io_thr) {
+        /*
+         * When use_io_thr is set, we use the global shared dedicated
+         * IO thread for this monitor to handle input/output.
+         */
+        context = monitor_io_context_get();
+        /* We should have inited globals before reaching here. */
+        assert(context);
+    } else {
+        /* The default main loop, which is the main thread */
+        context = NULL;
+    }
+
+    /*
+     * Add the monitor before running it (which is triggered by
+     * qemu_chr_fe_set_handlers), otherwise one monitor may find
+     * itself not on the mon_list when running.
+     */
+    qemu_mutex_lock(&monitor_lock);
+    QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
+    qemu_mutex_unlock(&monitor_lock);
+
     if (monitor_is_qmp(mon)) {
-        qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
-                                 monitor_qmp_event, NULL, mon, NULL, true);
         qemu_chr_fe_set_echo(&mon->chr, true);
         json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
+        qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
+                                 monitor_qmp_event, NULL, mon, context, true);
     } else {
+        assert(!context);       /* HMP does not support IOThreads */
         qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_read,
                                  monitor_event, NULL, mon, NULL, true);
     }
-
-    qemu_mutex_lock(&monitor_lock);
-    QLIST_INSERT_HEAD(&mon_list, mon, entry);
-    qemu_mutex_unlock(&monitor_lock);
 }
 
 void monitor_cleanup(void)
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 10/27] monitor: allow to use IO thread for parsing

[Fam Zheng]

On Tue, 12/19 16:45, Peter Xu wrote:
> For each Monitor, add one field "use_io_thr" to show whether it will be
> using the dedicated monitor IO thread to handle input/output.  When set,
> monitor IO parsing work will be offloaded to dedicated monitor IO
> thread, rather than the original main loop thread.
> 
> This only works for QMP.  HMP will always be run on main loop thread.
> 
> Currently we're still keeping use_io_thr to off always.  Will turn it on
> later at some point.
> 
> One thing to mention is that we cannot set use_io_thr for every QMP
> monitors.  The problem is that MUXed typed chardevs may not work well
> with it now. When MUX is used, frontend of chardev can be the monitor
> plus something else.  The only thing we know would be safe to be run
> outside main thread so far is the monitor frontend. All the rest of the
> frontends should still be run in main thread only.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 41 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 4ebd83fd94..4b2bee773f 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -190,6 +190,7 @@ struct Monitor {
>      int flags;
>      int suspend_cnt;
>      bool skip_flush;
> +    bool use_io_thr;
>  
>      QemuMutex out_lock;
>      QString *outbuf;
> @@ -573,7 +574,8 @@ static void monitor_qapi_event_init(void)
>  
>  static void handle_hmp_command(Monitor *mon, const char *cmdline);
>  
> -static void monitor_data_init(Monitor *mon, bool skip_flush)
> +static void monitor_data_init(Monitor *mon, bool skip_flush,
> +                              bool use_io_thr)
>  {
>      memset(mon, 0, sizeof(Monitor));
>      qemu_mutex_init(&mon->out_lock);
> @@ -581,6 +583,7 @@ static void monitor_data_init(Monitor *mon, bool skip_flush)
>      /* Use *mon_cmds by default. */
>      mon->cmd_table = mon_cmds;
>      mon->skip_flush = skip_flush;
> +    mon->use_io_thr = use_io_thr;
>  }
>  
>  static void monitor_data_destroy(Monitor *mon)
> @@ -601,7 +604,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
>      char *output = NULL;
>      Monitor *old_mon, hmp;
>  
> -    monitor_data_init(&hmp, true);
> +    monitor_data_init(&hmp, true, false);
>  
>      old_mon = cur_mon;
>      cur_mon = &hmp;
> @@ -4103,8 +4106,9 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap)
>  void monitor_init(Chardev *chr, int flags)
>  {
>      Monitor *mon = g_malloc(sizeof(*mon));
> +    GMainContext *context;
>  
> -    monitor_data_init(mon, false);
> +    monitor_data_init(mon, false, false);
>  
>      qemu_chr_fe_init(&mon->chr, chr, &error_abort);
>      mon->flags = flags;
> @@ -4116,19 +4120,38 @@ void monitor_init(Chardev *chr, int flags)
>          monitor_read_command(mon, 0);
>      }
>  
> +    if (mon->use_io_thr) {
> +        /*
> +         * When use_io_thr is set, we use the global shared dedicated
> +         * IO thread for this monitor to handle input/output.
> +         */
> +        context = monitor_io_context_get();
> +        /* We should have inited globals before reaching here. */
> +        assert(context);
> +    } else {
> +        /* The default main loop, which is the main thread */
> +        context = NULL;
> +    }
> +
> +    /*
> +     * Add the monitor before running it (which is triggered by
> +     * qemu_chr_fe_set_handlers), otherwise one monitor may find
> +     * itself not on the mon_list when running.
> +     */
> +    qemu_mutex_lock(&monitor_lock);
> +    QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
> +    qemu_mutex_unlock(&monitor_lock);
> +
>      if (monitor_is_qmp(mon)) {
> -        qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
> -                                 monitor_qmp_event, NULL, mon, NULL, true);
>          qemu_chr_fe_set_echo(&mon->chr, true);
>          json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
> +        qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
> +                                 monitor_qmp_event, NULL, mon, context, true);
>      } else {
> +        assert(!context);       /* HMP does not support IOThreads */
>          qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_read,
>                                   monitor_event, NULL, mon, NULL, true);
>      }
> -
> -    qemu_mutex_lock(&monitor_lock);
> -    QLIST_INSERT_HEAD(&mon_list, mon, entry);
> -    qemu_mutex_unlock(&monitor_lock);
>  }
>  
>  void monitor_cleanup(void)
> -- 
> 2.14.3
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [RFC v6 10/27] monitor: allow to use IO thread for parsing

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:40PM +0800, Peter Xu wrote:
>      if (monitor_is_qmp(mon)) {
> -        qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
> -                                 monitor_qmp_event, NULL, mon, NULL, true);
>          qemu_chr_fe_set_echo(&mon->chr, true);
>          json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
> +        qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
> +                                 monitor_qmp_event, NULL, mon, context, true);
>      } else {
> +        assert(!context);       /* HMP does not support IOThreads */
>          qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_read,
>                                   monitor_event, NULL, mon, NULL, true);
>      }

qemu_chr_fe_set_handlers() isn't thread-safe.  It looks like
monitor_can_read()/monitor_qmp_read() can run in the IOThread at the
same time as monitor_qmp_event() runs in the main loop:

  void qemu_chr_fe_set_handlers(CharBackend *b,
                                IOCanReadHandler *fd_can_read,
                                IOReadHandler *fd_read,
                                IOEventHandler *fd_event,
                                BackendChangeHandler *be_change,
                                void *opaque,
                                GMainContext *context,
                                bool set_open)
  {
      ...

      qemu_chr_be_update_read_handlers(s, context);
        ^-- The IOThread may invoke handler functions from now on!

      <-- Everything below races with the IOThread!

      if (set_open) {
          qemu_chr_fe_set_open(b, fe_open);
      }

      if (fe_open) {
          qemu_chr_fe_take_focus(b);
          /* We're connecting to an already opened device, so let's make sure we
             also get the open event */
          if (s->be_open) {
              qemu_chr_be_event(s, CHR_EVENT_OPENED);
          }
      }

      if (CHARDEV_IS_MUX(s)) {
          mux_chr_set_handlers(s, context);
      }
  }

It looks like chr_*() functions must be called from the event loop
thread that monitors the chardev.  You can use aio_bh_schedule_oneshot()
or g_idle_source_new() to execute the last part of monitor_init() in the
GMainContext.

That way there is no race condition because qemu_chr_fe_set_handlers()
is called from within the event loop thread.

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

Re: [Qemu-devel] [RFC v6 10/27] monitor: allow to use IO thread for parsing

[Peter Xu]

On Fri, Jan 05, 2018 at 05:22:26PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 19, 2017 at 04:45:40PM +0800, Peter Xu wrote:
> >      if (monitor_is_qmp(mon)) {
> > -        qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
> > -                                 monitor_qmp_event, NULL, mon, NULL, true);
> >          qemu_chr_fe_set_echo(&mon->chr, true);
> >          json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
> > +        qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
> > +                                 monitor_qmp_event, NULL, mon, context, true);
> >      } else {
> > +        assert(!context);       /* HMP does not support IOThreads */
> >          qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_read,
> >                                   monitor_event, NULL, mon, NULL, true);
> >      }
> 
> qemu_chr_fe_set_handlers() isn't thread-safe.  It looks like
> monitor_can_read()/monitor_qmp_read() can run in the IOThread at the
> same time as monitor_qmp_event() runs in the main loop:
> 
>   void qemu_chr_fe_set_handlers(CharBackend *b,
>                                 IOCanReadHandler *fd_can_read,
>                                 IOReadHandler *fd_read,
>                                 IOEventHandler *fd_event,
>                                 BackendChangeHandler *be_change,
>                                 void *opaque,
>                                 GMainContext *context,
>                                 bool set_open)
>   {
>       ...
> 
>       qemu_chr_be_update_read_handlers(s, context);
>         ^-- The IOThread may invoke handler functions from now on!
> 
>       <-- Everything below races with the IOThread!
> 
>       if (set_open) {
>           qemu_chr_fe_set_open(b, fe_open);
>       }
> 
>       if (fe_open) {
>           qemu_chr_fe_take_focus(b);
>           /* We're connecting to an already opened device, so let's make sure we
>              also get the open event */
>           if (s->be_open) {
>               qemu_chr_be_event(s, CHR_EVENT_OPENED);
>           }
>       }
> 
>       if (CHARDEV_IS_MUX(s)) {
>           mux_chr_set_handlers(s, context);
>       }
>   }
> 
> It looks like chr_*() functions must be called from the event loop
> thread that monitors the chardev.  You can use aio_bh_schedule_oneshot()
> or g_idle_source_new() to execute the last part of monitor_init() in the
> GMainContext.
> 
> That way there is no race condition because qemu_chr_fe_set_handlers()
> is called from within the event loop thread.

Good catch.  I'll take your suggestion.  Thanks!

-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 10/27] monitor: allow to use IO thread for parsing

[Marc-André Lureau]

Hi

On Fri, Jan 5, 2018 at 6:22 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Tue, Dec 19, 2017 at 04:45:40PM +0800, Peter Xu wrote:
> >      if (monitor_is_qmp(mon)) {
> > -        qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
> > -                                 monitor_qmp_event, NULL, mon, NULL, true);
> >          qemu_chr_fe_set_echo(&mon->chr, true);
> >          json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
> > +        qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
> > +                                 monitor_qmp_event, NULL, mon, context, true);
> >      } else {
> > +        assert(!context);       /* HMP does not support IOThreads */
> >          qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_read,
> >                                   monitor_event, NULL, mon, NULL, true);
> >      }
>
> qemu_chr_fe_set_handlers() isn't thread-safe.  It looks like
> monitor_can_read()/monitor_qmp_read() can run in the IOThread at the
> same time as monitor_qmp_event() runs in the main loop:
>
>   void qemu_chr_fe_set_handlers(CharBackend *b,
>                                 IOCanReadHandler *fd_can_read,
>                                 IOReadHandler *fd_read,
>                                 IOEventHandler *fd_event,
>                                 BackendChangeHandler *be_change,
>                                 void *opaque,
>                                 GMainContext *context,
>                                 bool set_open)
>   {
>       ...
>
>       qemu_chr_be_update_read_handlers(s, context);
>         ^-- The IOThread may invoke handler functions from now on!
>
>       <-- Everything below races with the IOThread!
>
>       if (set_open) {
>           qemu_chr_fe_set_open(b, fe_open);
>       }
>
>       if (fe_open) {
>           qemu_chr_fe_take_focus(b);
>           /* We're connecting to an already opened device, so let's make sure we
>              also get the open event */
>           if (s->be_open) {
>               qemu_chr_be_event(s, CHR_EVENT_OPENED);
>           }
>       }
>
>       if (CHARDEV_IS_MUX(s)) {
>           mux_chr_set_handlers(s, context);
>       }
>   }
>
> It looks like chr_*() functions must be called from the event loop
> thread that monitors the chardev.  You can use aio_bh_schedule_oneshot()
> or g_idle_source_new() to execute the last part of monitor_init() in the
> GMainContext.

Sadly, it looks like moving the qemu_chr_fe_set_handlers() call to the
target thread isn't yet safe either.

Even if remove_fd_in_watch() is called from the source thread, there
might be other sources that are still running in the main thread. For
example, the socket telnet_source. This will create races in
telnet_init & telnet_init_io callback between source & target threads.
We have to review this more carefully.

(btw, there are other code paths in the monitor chardev callbacks that
look unsafe to me, mon_refcount for ex)


>
> That way there is no race condition because qemu_chr_fe_set_handlers()
> is called from within the event loop thread.



-- 
Marc-André Lureau

Re: [Qemu-devel] [RFC v6 10/27] monitor: allow to use IO thread for parsing

[Eric Blake]

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

On 12/19/2017 02:45 AM, Peter Xu wrote:

Grammar in the subject:

"allow to $VERB" is not idiomatic English; correct is either "allow
${VERB}ing" or "allow $SUBJECT to $VERB".  Concretely, s/to use/using/

> For each Monitor, add one field "use_io_thr" to show whether it will be
> using the dedicated monitor IO thread to handle input/output.  When set,
> monitor IO parsing work will be offloaded to dedicated monitor IO

s/to/to the/

> thread, rather than the original main loop thread.
> 
> This only works for QMP.  HMP will always be run on main loop thread.

s/on/on the/

> 
> Currently we're still keeping use_io_thr to off always.  Will turn it on

s/to off/off/

> later at some point.
> 
> One thing to mention is that we cannot set use_io_thr for every QMP
> monitors.  The problem is that MUXed typed chardevs may not work well

s/monitors/monitor/

> with it now. When MUX is used, frontend of chardev can be the monitor
> plus something else.  The only thing we know would be safe to be run
> outside main thread so far is the monitor frontend. All the rest of the
> frontends should still be run in main thread only.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 41 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 32 insertions(+), 9 deletions(-)
> 
I have nothing to add on the code review, but it looks like Stefan had
valid comments that need addressing.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC v6 10/27] monitor: allow to use IO thread for parsing

[Peter Xu]

On Tue, Jan 09, 2018 at 05:37:39PM -0600, Eric Blake wrote:
> On 12/19/2017 02:45 AM, Peter Xu wrote:
> 
> Grammar in the subject:
> 
> "allow to $VERB" is not idiomatic English; correct is either "allow
> ${VERB}ing" or "allow $SUBJECT to $VERB".  Concretely, s/to use/using/
> 
> > For each Monitor, add one field "use_io_thr" to show whether it will be
> > using the dedicated monitor IO thread to handle input/output.  When set,
> > monitor IO parsing work will be offloaded to dedicated monitor IO
> 
> s/to/to the/
> 
> > thread, rather than the original main loop thread.
> > 
> > This only works for QMP.  HMP will always be run on main loop thread.
> 
> s/on/on the/
> 
> > 
> > Currently we're still keeping use_io_thr to off always.  Will turn it on
> 
> s/to off/off/
> 
> > later at some point.
> > 
> > One thing to mention is that we cannot set use_io_thr for every QMP
> > monitors.  The problem is that MUXed typed chardevs may not work well
> 
> s/monitors/monitor/
> 
> > with it now. When MUX is used, frontend of chardev can be the monitor
> > plus something else.  The only thing we know would be safe to be run
> > outside main thread so far is the monitor frontend. All the rest of the
> > frontends should still be run in main thread only.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  monitor.c | 41 ++++++++++++++++++++++++++++++++---------
> >  1 file changed, 32 insertions(+), 9 deletions(-)
> > 
> I have nothing to add on the code review, but it looks like Stefan had
> valid comments that need addressing.

Yes, will address it with all the grammar fixes above.  Thanks,

-- 
Peter Xu

[Qemu-devel] [RFC v6 11/27] qmp: introduce QMPCapability

[Peter Xu]

There was no QMP capabilities defined.  Define the first "oob" as
capability to allow out-of-band messages.

Also, touch up qmp-test.c to test the new bits.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c        | 10 ++++++++--
 qapi-schema.json | 13 +++++++++++++
 tests/qmp-test.c | 10 +++++++++-
 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index 4b2bee773f..81fb0a42b4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3943,12 +3943,18 @@ void monitor_resume(Monitor *mon)
 
 static QObject *get_qmp_greeting(void)
 {
+    QList *cap_list = qlist_new();
     QObject *ver = NULL;
+    QMPCapability cap;
 
     qmp_marshal_query_version(NULL, &ver, NULL);
 
-    return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': []}}",
-                              ver);
+    for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
+        qlist_append(cap_list, qstring_from_str(QMPCapability_str(cap)));
+    }
+
+    return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': %p}}",
+                              ver, cap_list);
 }
 
 static void monitor_qmp_event(void *opaque, int event)
diff --git a/qapi-schema.json b/qapi-schema.json
index 18457954a8..03201578b4 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -118,6 +118,19 @@
 ##
 { 'command': 'qmp_capabilities' }
 
+##
+# @QMPCapability:
+#
+# QMP supported capabilities to be broadcasted to the clients.
+#
+# @oob:   QMP ability to support Out-Of-Band requests.
+#
+# Since: 2.12
+#
+##
+{ 'enum': 'QMPCapability',
+  'data': [ 'oob' ] }
+
 ##
 # @VersionTriple:
 #
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index c5a5c10b41..292c5f135a 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -17,6 +17,7 @@
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/util.h"
 #include "qapi/visitor.h"
+#include "qapi/qmp/qstring.h"
 
 const char common_args[] = "-nodefaults -machine none";
 
@@ -75,6 +76,8 @@ static void test_qmp_protocol(void)
 {
     QDict *resp, *q, *ret;
     QList *capabilities;
+    const QListEntry *entry;
+    QString *qstr;
 
     global_qtest = qtest_init_without_qmp_handshake(common_args);
 
@@ -84,7 +87,12 @@ 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);
+    entry = qlist_first(capabilities);
+    g_assert(entry);
+    qstr = qobject_to_qstring(entry->value);
+    g_assert(qstr);
+    g_assert_cmpstr(qstring_get_str(qstr), ==, "oob");
     QDECREF(resp);
 
     /* Test valid command before handshake */
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 11/27] qmp: introduce QMPCapability

[Fam Zheng]

On Tue, 12/19 16:45, Peter Xu wrote:
> diff --git a/monitor.c b/monitor.c
> index 4b2bee773f..81fb0a42b4 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3943,12 +3943,18 @@ void monitor_resume(Monitor *mon)
>  
>  static QObject *get_qmp_greeting(void)
>  {
> +    QList *cap_list = qlist_new();
>      QObject *ver = NULL;
> +    QMPCapability cap;
>  
>      qmp_marshal_query_version(NULL, &ver, NULL);
>  
> -    return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': []}}",
> -                              ver);
> +    for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
> +        qlist_append(cap_list, qstring_from_str(QMPCapability_str(cap)));

Did we want to not include "oob" if current monitor doesn't have use_io_thr?

Fam

Re: [Qemu-devel] [RFC v6 11/27] qmp: introduce QMPCapability

[Peter Xu]

On Thu, Dec 21, 2017 at 05:56:37PM +0800, Fam Zheng wrote:
> On Tue, 12/19 16:45, Peter Xu wrote:
> > diff --git a/monitor.c b/monitor.c
> > index 4b2bee773f..81fb0a42b4 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -3943,12 +3943,18 @@ void monitor_resume(Monitor *mon)
> >  
> >  static QObject *get_qmp_greeting(void)
> >  {
> > +    QList *cap_list = qlist_new();
> >      QObject *ver = NULL;
> > +    QMPCapability cap;
> >  
> >      qmp_marshal_query_version(NULL, &ver, NULL);
> >  
> > -    return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': []}}",
> > -                              ver);
> > +    for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
> > +        qlist_append(cap_list, qstring_from_str(QMPCapability_str(cap)));
> 
> Did we want to not include "oob" if current monitor doesn't have use_io_thr?

Yes.  As you have already noticed, it's in next patch. :)

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 11/27] qmp: introduce QMPCapability

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:41PM +0800, Peter Xu wrote:
> There was no QMP capabilities defined.  Define the first "oob" as
> capability to allow out-of-band messages.
> 
> Also, touch up qmp-test.c to test the new bits.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c        | 10 ++++++++--
>  qapi-schema.json | 13 +++++++++++++
>  tests/qmp-test.c | 10 +++++++++-
>  3 files changed, 30 insertions(+), 3 deletions(-)

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

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

Re: [Qemu-devel] [RFC v6 11/27] qmp: introduce QMPCapability

[Eric Blake]

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

On 12/19/2017 02:45 AM, Peter Xu wrote:
> There was no QMP capabilities defined.  Define the first "oob" as

s/was/were/

> capability to allow out-of-band messages.
> 
> Also, touch up qmp-test.c to test the new bits.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c        | 10 ++++++++--
>  qapi-schema.json | 13 +++++++++++++
>  tests/qmp-test.c | 10 +++++++++-
>  3 files changed, 30 insertions(+), 3 deletions(-)

I'm assuming later patches will document this?

I'm somewhat a fan of documentation alongside or before implementation,
as getting the general overview right and then checking that the code
matches is a bit nicer than random coding then documenting what we ended
up with.  But I don't know if reordering patches in your series is
necessary, as long as the end product is properly documented.

> +++ b/qapi-schema.json
> @@ -118,6 +118,19 @@
>  ##
>  { 'command': 'qmp_capabilities' }

The client can't request a particular feature alongside the command?  Or
is that in later patches?  With just this patch, the enum QMPCapability
is not introspected, because it is not referenced by any command
(although introspection is a bit moot, since the client will learn what
the host advertises from the initial handshake before the client can
even request introspection).

>  
> +##
> +# @QMPCapability:
> +#
> +# QMP supported capabilities to be broadcasted to the clients.

'broadcast' is one of those weird verbs that doesn't change spelling
when constructing its past tense (there is no 'broadcasted').  However,
I think this description is a bit nicer (and avoids the problematic word
altogether):

Enumeration of capabilities to be advertised during initial client
connection, used for agreeing on particular QMP extension behaviors.

> +#
> +# @oob:   QMP ability to support Out-Of-Band requests.

Rather terse (it doesn't say what Out-Of-Band requests are); even a
pointer to the QMP spec (where OOB is more fully documented) might be
nice (of course, that means we need a patch to docs/interop/qmp-spec.txt
somewhere in the series, especially since this patch renders 2.2.1 in
that document obsolete...)

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC v6 11/27] qmp: introduce QMPCapability

[Peter Xu]

On Thu, Jan 11, 2018 at 05:07:11PM -0600, Eric Blake wrote:
> On 12/19/2017 02:45 AM, Peter Xu wrote:
> > There was no QMP capabilities defined.  Define the first "oob" as
> 
> s/was/were/

Fixed.

Just to confirm: is "There was no QMP capability" also correct?

> 
> > capability to allow out-of-band messages.
> > 
> > Also, touch up qmp-test.c to test the new bits.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  monitor.c        | 10 ++++++++--
> >  qapi-schema.json | 13 +++++++++++++
> >  tests/qmp-test.c | 10 +++++++++-
> >  3 files changed, 30 insertions(+), 3 deletions(-)
> 
> I'm assuming later patches will document this?
> 
> I'm somewhat a fan of documentation alongside or before implementation,
> as getting the general overview right and then checking that the code
> matches is a bit nicer than random coding then documenting what we ended
> up with.  But I don't know if reordering patches in your series is
> necessary, as long as the end product is properly documented.

Yes, I put the whole document at the end of the series.  I can put it
as the first patch too.

> 
> > +++ b/qapi-schema.json
> > @@ -118,6 +118,19 @@
> >  ##
> >  { 'command': 'qmp_capabilities' }
> 
> The client can't request a particular feature alongside the command?  Or
> is that in later patches?  With just this patch, the enum QMPCapability
> is not introspected, because it is not referenced by any command
> (although introspection is a bit moot, since the client will learn what
> the host advertises from the initial handshake before the client can
> even request introspection).

Yes, client can't if without further patches.

> 
> >  
> > +##
> > +# @QMPCapability:
> > +#
> > +# QMP supported capabilities to be broadcasted to the clients.
> 
> 'broadcast' is one of those weird verbs that doesn't change spelling
> when constructing its past tense (there is no 'broadcasted').  However,
> I think this description is a bit nicer (and avoids the problematic word
> altogether):
> 
> Enumeration of capabilities to be advertised during initial client
> connection, used for agreeing on particular QMP extension behaviors.

I'll take your advise.

> 
> > +#
> > +# @oob:   QMP ability to support Out-Of-Band requests.
> 
> Rather terse (it doesn't say what Out-Of-Band requests are); even a
> pointer to the QMP spec (where OOB is more fully documented) might be
> nice (of course, that means we need a patch to docs/interop/qmp-spec.txt
> somewhere in the series, especially since this patch renders 2.2.1 in
> that document obsolete...)

Sorry for the inconvenience.  Please refer to the last doc patch for
details.

I thought the doc patch would explain itself but obviously I should be
more careful on the ordering next time to ease reviewers.  I'll move
the doc patch to front when repost, and I'll note here to refer to
qmp-spec.txt for more information.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 11/27] qmp: introduce QMPCapability

[Eric Blake]

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

On 01/11/2018 10:28 PM, Peter Xu wrote:
> On Thu, Jan 11, 2018 at 05:07:11PM -0600, Eric Blake wrote:
>> On 12/19/2017 02:45 AM, Peter Xu wrote:
>>> There was no QMP capabilities defined.  Define the first "oob" as
>>
>> s/was/were/
> 
> Fixed.
> 
> Just to confirm: is "There was no QMP capability" also correct?

Yes, that also matches singular vs. plural between the verb and object.


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

[Qemu-devel] [RFC v6 12/27] qmp: negotiate QMP capabilities

[Peter Xu]

After this patch, we will allow QMP clients to enable QMP capabilities
when sending the first "qmp_capabilities" command.  Originally we are
starting QMP session with no arguments like:

  { "execute": "qmp_capabilities" }

Now we can enable some QMP capabilities using (take OOB as example,
which is the only one capability that we support):

  { "execute": "qmp_capabilities",
    "argument": { "enable": [ "oob" ] } }

When the "argument" key is not provided, no capability is enabled.

For capability "oob", the monitor needs to be run on dedicated IO
thread, otherwise the command will fail.  For example, trying to enable
OOB on a MUXed typed QMP monitor will fail.

One thing to mention is that, QMP capabilities are per-monitor, and also
when the connection is closed due to some reason, the capabilities will
be reset.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c        | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 qapi-schema.json | 15 ++++++++++---
 2 files changed, 74 insertions(+), 6 deletions(-)

diff --git a/monitor.c b/monitor.c
index 81fb0a42b4..c88a11cd69 100644
--- a/monitor.c
+++ b/monitor.c
@@ -166,6 +166,7 @@ typedef struct {
      * mode.
      */
     QmpCommandList *commands;
+    bool qmp_caps[QMP_CAPABILITY__MAX];
 } MonitorQMP;
 
 /*
@@ -1035,8 +1036,42 @@ static void monitor_init_qmp_commands(void)
                          qmp_marshal_qmp_capabilities, QCO_NO_OPTIONS);
 }
 
-void qmp_qmp_capabilities(Error **errp)
+static void qmp_caps_check(Monitor *mon, QMPCapabilityList *list,
+                           Error **errp)
+{
+    for (; list; list = list->next) {
+        assert(list->value < QMP_CAPABILITY__MAX);
+        switch (list->value) {
+        case QMP_CAPABILITY_OOB:
+            if (!mon->use_io_thr) {
+                /*
+                 * Out-Of-Band only works with monitors that are
+                 * running on dedicated IOThread.
+                 */
+                error_setg(errp, "This monitor does not support "
+                           "Out-Of-Band (OOB)");
+                return;
+            }
+            break;
+        default:
+            break;
+        }
+    }
+}
+
+/* This function should only be called after capabilities are checked. */
+static void qmp_caps_apply(Monitor *mon, QMPCapabilityList *list)
+{
+    for (; list; list = list->next) {
+        mon->qmp.qmp_caps[list->value] = true;
+    }
+}
+
+void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
+                          Error **errp)
 {
+    Error *local_err = NULL;
+
     if (cur_mon->qmp.commands == &qmp_commands) {
         error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
                   "Capabilities negotiation is already complete, command "
@@ -1044,6 +1079,20 @@ void qmp_qmp_capabilities(Error **errp)
         return;
     }
 
+    /* Enable QMP capabilities provided by the guest if applicable. */
+    if (has_enable) {
+        qmp_caps_check(cur_mon, enable, &local_err);
+        if (local_err) {
+            /*
+             * Failed check on either of the capabilities will fail
+             * the apply of all.
+             */
+            error_propagate(errp, local_err);
+            return;
+        }
+        qmp_caps_apply(cur_mon, enable);
+    }
+
     cur_mon->qmp.commands = &qmp_commands;
 }
 
@@ -3941,7 +3990,7 @@ void monitor_resume(Monitor *mon)
         readline_show_prompt(mon->rs);
 }
 
-static QObject *get_qmp_greeting(void)
+static QObject *get_qmp_greeting(Monitor *mon)
 {
     QList *cap_list = qlist_new();
     QObject *ver = NULL;
@@ -3950,6 +3999,10 @@ static QObject *get_qmp_greeting(void)
     qmp_marshal_query_version(NULL, &ver, NULL);
 
     for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
+        if (!mon->use_io_thr && cap == QMP_CAPABILITY_OOB) {
+            /* Monitors that are not using IOThread won't support OOB */
+            continue;
+        }
         qlist_append(cap_list, qstring_from_str(QMPCapability_str(cap)));
     }
 
@@ -3957,6 +4010,11 @@ static QObject *get_qmp_greeting(void)
                               ver, cap_list);
 }
 
+static void monitor_qmp_caps_reset(Monitor *mon)
+{
+    memset(mon->qmp.qmp_caps, 0, sizeof(mon->qmp.qmp_caps));
+}
+
 static void monitor_qmp_event(void *opaque, int event)
 {
     QObject *data;
@@ -3965,7 +4023,8 @@ static void monitor_qmp_event(void *opaque, int event)
     switch (event) {
     case CHR_EVENT_OPENED:
         mon->qmp.commands = &qmp_cap_negotiation_commands;
-        data = get_qmp_greeting();
+        monitor_qmp_caps_reset(mon);
+        data = get_qmp_greeting(mon);
         monitor_json_emitter(mon, data);
         qobject_decref(data);
         mon_refcount++;
diff --git a/qapi-schema.json b/qapi-schema.json
index 03201578b4..531fd4c0db 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -102,21 +102,30 @@
 #
 # Enable QMP capabilities.
 #
-# Arguments: None.
+# Arguments:
+#
+# @enable:    List of QMPCapabilities to enable, which is optional.
+#             If not provided, it means no QMP capabilities will be
+#             enabled.  (since 2.12)
 #
 # Example:
 #
-# -> { "execute": "qmp_capabilities" }
+# -> { "execute": "qmp_capabilities",
+#      "arguments": { "enable": [ "oob" ] } }
 # <- { "return": {} }
 #
 # Notes: This command is valid exactly when first connecting: it must be
 # issued before any other command will be accepted, and will fail once the
 # monitor is accepting other commands. (see qemu docs/interop/qmp-spec.txt)
 #
+# QMP client needs to explicitly enable QMP capabilities, otherwise
+# all the QMP capabilities will be turned off by default.
+#
 # Since: 0.13
 #
 ##
-{ 'command': 'qmp_capabilities' }
+{ 'command': 'qmp_capabilities',
+  'data': { '*enable': [ 'QMPCapability' ] } }
 
 ##
 # @QMPCapability:
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 12/27] qmp: negotiate QMP capabilities

[Fam Zheng]

On Tue, 12/19 16:45, Peter Xu wrote:
> After this patch, we will allow QMP clients to enable QMP capabilities
> when sending the first "qmp_capabilities" command.  Originally we are
> starting QMP session with no arguments like:
> 
>   { "execute": "qmp_capabilities" }
> 
> Now we can enable some QMP capabilities using (take OOB as example,
> which is the only one capability that we support):
> 
>   { "execute": "qmp_capabilities",
>     "argument": { "enable": [ "oob" ] } }
> 
> When the "argument" key is not provided, no capability is enabled.
> 
> For capability "oob", the monitor needs to be run on dedicated IO
> thread, otherwise the command will fail.  For example, trying to enable
> OOB on a MUXed typed QMP monitor will fail.
> 
> One thing to mention is that, QMP capabilities are per-monitor, and also
> when the connection is closed due to some reason, the capabilities will
> be reset.
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c        | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  qapi-schema.json | 15 ++++++++++---
>  2 files changed, 74 insertions(+), 6 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 81fb0a42b4..c88a11cd69 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -166,6 +166,7 @@ typedef struct {
>       * mode.
>       */
>      QmpCommandList *commands;
> +    bool qmp_caps[QMP_CAPABILITY__MAX];
>  } MonitorQMP;
>  
>  /*
> @@ -1035,8 +1036,42 @@ static void monitor_init_qmp_commands(void)
>                           qmp_marshal_qmp_capabilities, QCO_NO_OPTIONS);
>  }
>  
> -void qmp_qmp_capabilities(Error **errp)
> +static void qmp_caps_check(Monitor *mon, QMPCapabilityList *list,
> +                           Error **errp)
> +{
> +    for (; list; list = list->next) {
> +        assert(list->value < QMP_CAPABILITY__MAX);
> +        switch (list->value) {
> +        case QMP_CAPABILITY_OOB:
> +            if (!mon->use_io_thr) {
> +                /*
> +                 * Out-Of-Band only works with monitors that are
> +                 * running on dedicated IOThread.
> +                 */
> +                error_setg(errp, "This monitor does not support "
> +                           "Out-Of-Band (OOB)");
> +                return;
> +            }
> +            break;
> +        default:
> +            break;
> +        }
> +    }
> +}
> +
> +/* This function should only be called after capabilities are checked. */
> +static void qmp_caps_apply(Monitor *mon, QMPCapabilityList *list)
> +{
> +    for (; list; list = list->next) {
> +        mon->qmp.qmp_caps[list->value] = true;
> +    }
> +}
> +
> +void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
> +                          Error **errp)
>  {
> +    Error *local_err = NULL;
> +
>      if (cur_mon->qmp.commands == &qmp_commands) {
>          error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
>                    "Capabilities negotiation is already complete, command "
> @@ -1044,6 +1079,20 @@ void qmp_qmp_capabilities(Error **errp)
>          return;
>      }
>  
> +    /* Enable QMP capabilities provided by the guest if applicable. */

s/guest/client/ ?

> +    if (has_enable) {
> +        qmp_caps_check(cur_mon, enable, &local_err);
> +        if (local_err) {
> +            /*
> +             * Failed check on either of the capabilities will fail
> +             * the apply of all.
> +             */
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +        qmp_caps_apply(cur_mon, enable);
> +    }
> +
>      cur_mon->qmp.commands = &qmp_commands;
>  }
>  
> @@ -3941,7 +3990,7 @@ void monitor_resume(Monitor *mon)
>          readline_show_prompt(mon->rs);
>  }
>  
> -static QObject *get_qmp_greeting(void)
> +static QObject *get_qmp_greeting(Monitor *mon)
>  {
>      QList *cap_list = qlist_new();
>      QObject *ver = NULL;
> @@ -3950,6 +3999,10 @@ static QObject *get_qmp_greeting(void)
>      qmp_marshal_query_version(NULL, &ver, NULL);
>  
>      for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
> +        if (!mon->use_io_thr && cap == QMP_CAPABILITY_OOB) {
> +            /* Monitors that are not using IOThread won't support OOB */
> +            continue;
> +        }

OK, I thought this could better go to the previous patch, but it may be fine
here, together with monitor_qmp_caps_reset().

>          qlist_append(cap_list, qstring_from_str(QMPCapability_str(cap)));
>      }
>  
> @@ -3957,6 +4010,11 @@ static QObject *get_qmp_greeting(void)
>                                ver, cap_list);
>  }
>  
> +static void monitor_qmp_caps_reset(Monitor *mon)
> +{
> +    memset(mon->qmp.qmp_caps, 0, sizeof(mon->qmp.qmp_caps));
> +}
> +
>  static void monitor_qmp_event(void *opaque, int event)
>  {
>      QObject *data;
> @@ -3965,7 +4023,8 @@ static void monitor_qmp_event(void *opaque, int event)
>      switch (event) {
>      case CHR_EVENT_OPENED:
>          mon->qmp.commands = &qmp_cap_negotiation_commands;
> -        data = get_qmp_greeting();
> +        monitor_qmp_caps_reset(mon);
> +        data = get_qmp_greeting(mon);
>          monitor_json_emitter(mon, data);
>          qobject_decref(data);
>          mon_refcount++;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 03201578b4..531fd4c0db 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -102,21 +102,30 @@
>  #
>  # Enable QMP capabilities.
>  #
> -# Arguments: None.
> +# Arguments:
> +#
> +# @enable:    List of QMPCapabilities to enable, which is optional.
> +#             If not provided, it means no QMP capabilities will be
> +#             enabled.  (since 2.12)
>  #
>  # Example:
>  #
> -# -> { "execute": "qmp_capabilities" }
> +# -> { "execute": "qmp_capabilities",
> +#      "arguments": { "enable": [ "oob" ] } }
>  # <- { "return": {} }
>  #
>  # Notes: This command is valid exactly when first connecting: it must be
>  # issued before any other command will be accepted, and will fail once the
>  # monitor is accepting other commands. (see qemu docs/interop/qmp-spec.txt)
>  #
> +# QMP client needs to explicitly enable QMP capabilities, otherwise
> +# all the QMP capabilities will be turned off by default.
> +#
>  # Since: 0.13
>  #
>  ##
> -{ 'command': 'qmp_capabilities' }
> +{ 'command': 'qmp_capabilities',
> +  'data': { '*enable': [ 'QMPCapability' ] } }
>  
>  ##
>  # @QMPCapability:
> -- 
> 2.14.3
> 

Fam

Re: [Qemu-devel] [RFC v6 12/27] qmp: negotiate QMP capabilities

[Peter Xu]

On Thu, Dec 21, 2017 at 06:01:00PM +0800, Fam Zheng wrote:

[...]

> > +void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
> > +                          Error **errp)
> >  {
> > +    Error *local_err = NULL;
> > +
> >      if (cur_mon->qmp.commands == &qmp_commands) {
> >          error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
> >                    "Capabilities negotiation is already complete, command "
> > @@ -1044,6 +1079,20 @@ void qmp_qmp_capabilities(Error **errp)
> >          return;
> >      }
> >  
> > +    /* Enable QMP capabilities provided by the guest if applicable. */
> 
> s/guest/client/ ?

Fixed.

[...]

> > -static QObject *get_qmp_greeting(void)
> > +static QObject *get_qmp_greeting(Monitor *mon)
> >  {
> >      QList *cap_list = qlist_new();
> >      QObject *ver = NULL;
> > @@ -3950,6 +3999,10 @@ static QObject *get_qmp_greeting(void)
> >      qmp_marshal_query_version(NULL, &ver, NULL);
> >  
> >      for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
> > +        if (!mon->use_io_thr && cap == QMP_CAPABILITY_OOB) {
> > +            /* Monitors that are not using IOThread won't support OOB */
> > +            continue;
> > +        }
> 
> OK, I thought this could better go to the previous patch, but it may be fine
> here, together with monitor_qmp_caps_reset().

Yes.  I kept all OOB things in this patch, and for previous patch it
only starts to offer all the capabilities to clients.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 12/27] qmp: negotiate QMP capabilities

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:42PM +0800, Peter Xu wrote:
> After this patch, we will allow QMP clients to enable QMP capabilities
> when sending the first "qmp_capabilities" command.  Originally we are
> starting QMP session with no arguments like:
> 
>   { "execute": "qmp_capabilities" }
> 
> Now we can enable some QMP capabilities using (take OOB as example,
> which is the only one capability that we support):
> 
>   { "execute": "qmp_capabilities",
>     "argument": { "enable": [ "oob" ] } }
> 
> When the "argument" key is not provided, no capability is enabled.
> 
> For capability "oob", the monitor needs to be run on dedicated IO
> thread, otherwise the command will fail.  For example, trying to enable
> OOB on a MUXed typed QMP monitor will fail.
> 
> One thing to mention is that, QMP capabilities are per-monitor, and also
> when the connection is closed due to some reason, the capabilities will
> be reset.
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c        | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  qapi-schema.json | 15 ++++++++++---
>  2 files changed, 74 insertions(+), 6 deletions(-)

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

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

Re: [Qemu-devel] [RFC v6 12/27] qmp: negotiate QMP capabilities

[Eric Blake]

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

On 12/19/2017 02:45 AM, Peter Xu wrote:
> After this patch, we will allow QMP clients to enable QMP capabilities
> when sending the first "qmp_capabilities" command.  Originally we are
> starting QMP session with no arguments like:
> 
>   { "execute": "qmp_capabilities" }
> 
> Now we can enable some QMP capabilities using (take OOB as example,
> which is the only one capability that we support):
> 
>   { "execute": "qmp_capabilities",
>     "argument": { "enable": [ "oob" ] } }
> 
> When the "argument" key is not provided, no capability is enabled.
> 
> For capability "oob", the monitor needs to be run on dedicated IO
> thread, otherwise the command will fail.  For example, trying to enable
> OOB on a MUXed typed QMP monitor will fail.


> 
> One thing to mention is that, QMP capabilities are per-monitor, and also
> when the connection is closed due to some reason, the capabilities will
> be reset.
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c        | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  qapi-schema.json | 15 ++++++++++---
>  2 files changed, 74 insertions(+), 6 deletions(-)
> 

> @@ -1044,6 +1079,20 @@ void qmp_qmp_capabilities(Error **errp)
>          return;
>      }
>  
> +    /* Enable QMP capabilities provided by the guest if applicable. */
> +    if (has_enable) {
> +        qmp_caps_check(cur_mon, enable, &local_err);
> +        if (local_err) {
> +            /*
> +             * Failed check on either of the capabilities will fail

s/either/any/

> +             * the apply of all.

s/apply/application/

or even a more verbose

will fail the entire command (and thus not apply any of the other
capabilities that were also requested).

> @@ -3950,6 +3999,10 @@ static QObject *get_qmp_greeting(void)
>      qmp_marshal_query_version(NULL, &ver, NULL);
>  
>      for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
> +        if (!mon->use_io_thr && cap == QMP_CAPABILITY_OOB) {
> +            /* Monitors that are not using IOThread won't support OOB */
> +            continue;
> +        }
>          qlist_append(cap_list, qstring_from_str(QMPCapability_str(cap)));

I think this hunk belongs in the previous patch - the server should not
advertise 'oob' in the greeting if it will not be able to honor it.


> +++ b/qapi-schema.json
> @@ -102,21 +102,30 @@
>  #
>  # Enable QMP capabilities.
>  #
> -# Arguments: None.
> +# Arguments:
> +#
> +# @enable:    List of QMPCapabilities to enable, which is optional.

Maybe document that this list must not include any capabilities that
were not mentioned in the server's initial greeting.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC v6 12/27] qmp: negotiate QMP capabilities

[Peter Xu]

On Fri, Jan 12, 2018 at 02:57:23PM -0600, Eric Blake wrote:
> On 12/19/2017 02:45 AM, Peter Xu wrote:
> > After this patch, we will allow QMP clients to enable QMP capabilities
> > when sending the first "qmp_capabilities" command.  Originally we are
> > starting QMP session with no arguments like:
> > 
> >   { "execute": "qmp_capabilities" }
> > 
> > Now we can enable some QMP capabilities using (take OOB as example,
> > which is the only one capability that we support):
> > 
> >   { "execute": "qmp_capabilities",
> >     "argument": { "enable": [ "oob" ] } }
> > 
> > When the "argument" key is not provided, no capability is enabled.
> > 
> > For capability "oob", the monitor needs to be run on dedicated IO
> > thread, otherwise the command will fail.  For example, trying to enable
> > OOB on a MUXed typed QMP monitor will fail.
> 
> 
> > 
> > One thing to mention is that, QMP capabilities are per-monitor, and also
> > when the connection is closed due to some reason, the capabilities will
> > be reset.
> > 
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  monitor.c        | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  qapi-schema.json | 15 ++++++++++---
> >  2 files changed, 74 insertions(+), 6 deletions(-)
> > 
> 
> > @@ -1044,6 +1079,20 @@ void qmp_qmp_capabilities(Error **errp)
> >          return;
> >      }
> >  
> > +    /* Enable QMP capabilities provided by the guest if applicable. */
> > +    if (has_enable) {
> > +        qmp_caps_check(cur_mon, enable, &local_err);
> > +        if (local_err) {
> > +            /*
> > +             * Failed check on either of the capabilities will fail
> 
> s/either/any/

Fixed.

> 
> > +             * the apply of all.
> 
> s/apply/application/
> 
> or even a more verbose
> 
> will fail the entire command (and thus not apply any of the other
> capabilities that were also requested).

Sure.

> 
> > @@ -3950,6 +3999,10 @@ static QObject *get_qmp_greeting(void)
> >      qmp_marshal_query_version(NULL, &ver, NULL);
> >  
> >      for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
> > +        if (!mon->use_io_thr && cap == QMP_CAPABILITY_OOB) {
> > +            /* Monitors that are not using IOThread won't support OOB */
> > +            continue;
> > +        }
> >          qlist_append(cap_list, qstring_from_str(QMPCapability_str(cap)));
> 
> I think this hunk belongs in the previous patch - the server should not
> advertise 'oob' in the greeting if it will not be able to honor it.

Yes, Fam asked the same question.  How about I squash these two
patches?  After all chunk moving between commits are even more
error-prone to me...  and even moving the chunk will need me to drop
all r-bs for the patches.

Let me squash them.

> 
> 
> > +++ b/qapi-schema.json
> > @@ -102,21 +102,30 @@
> >  #
> >  # Enable QMP capabilities.
> >  #
> > -# Arguments: None.
> > +# Arguments:
> > +#
> > +# @enable:    List of QMPCapabilities to enable, which is optional.
> 
> Maybe document that this list must not include any capabilities that
> were not mentioned in the server's initial greeting.

Ok.  Thanks,

-- 
Peter Xu

[Qemu-devel] [RFC v6 13/27] monitor: introduce monitor_qmp_respond()

[Peter Xu]

A tiny refactoring, preparing to split the QMP dispatcher away.

Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 50 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/monitor.c b/monitor.c
index c88a11cd69..cf1e5d881c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3865,6 +3865,38 @@ static int monitor_can_read(void *opaque)
     return (mon->suspend_cnt == 0) ? 1 : 0;
 }
 
+/*
+ * 1. This function takes ownership of rsp, err, and id.
+ * 2. rsp, err, and id may be NULL.
+ * 3. If err != NULL then rsp must be NULL.
+ */
+static void monitor_qmp_respond(Monitor *mon, QObject *rsp,
+                                Error *err, QObject *id)
+{
+    QDict *qdict = NULL;
+
+    if (err) {
+        assert(!rsp);
+        qdict = qdict_new();
+        qdict_put_obj(qdict, "error", qmp_build_error_object(err));
+        error_free(err);
+        rsp = QOBJECT(qdict);
+    }
+
+    if (rsp) {
+        if (id) {
+            /* This is for the qdict below. */
+            qobject_incref(id);
+            qdict_put_obj(qobject_to_qdict(rsp), "id", id);
+        }
+
+        monitor_json_emitter(mon, rsp);
+    }
+
+    qobject_decref(id);
+    qobject_decref(rsp);
+}
+
 static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
 {
     QObject *req, *rsp = NULL, *id = NULL;
@@ -3916,24 +3948,8 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     }
 
 err_out:
-    if (err) {
-        qdict = qdict_new();
-        qdict_put_obj(qdict, "error", qmp_build_error_object(err));
-        error_free(err);
-        rsp = QOBJECT(qdict);
-    }
+    monitor_qmp_respond(mon, rsp, err, id);
 
-    if (rsp) {
-        if (id) {
-            qdict_put_obj(qobject_to_qdict(rsp), "id", id);
-            id = NULL;
-        }
-
-        monitor_json_emitter(mon, rsp);
-    }
-
-    qobject_decref(id);
-    qobject_decref(rsp);
     qobject_decref(req);
 }
 
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 13/27] monitor: introduce monitor_qmp_respond()

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:43PM +0800, Peter Xu wrote:
> A tiny refactoring, preparing to split the QMP dispatcher away.
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 50 +++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 17 deletions(-)

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

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

[Qemu-devel] [RFC v6 14/27] monitor: let suspend_cnt be thread safe

[Peter Xu]

Monitor code now can be run in more than one thread.  Let it be thread
safe when accessing suspend_cnt counter.

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

diff --git a/monitor.c b/monitor.c
index cf1e5d881c..844508d134 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3862,7 +3862,7 @@ static int monitor_can_read(void *opaque)
 {
     Monitor *mon = opaque;
 
-    return (mon->suspend_cnt == 0) ? 1 : 0;
+    return (atomic_mb_read(&mon->suspend_cnt) == 0) ? 1 : 0;
 }
 
 /*
@@ -3994,7 +3994,7 @@ int monitor_suspend(Monitor *mon)
 {
     if (!mon->rs)
         return -ENOTTY;
-    mon->suspend_cnt++;
+    atomic_inc(&mon->suspend_cnt);
     return 0;
 }
 
@@ -4002,8 +4002,9 @@ void monitor_resume(Monitor *mon)
 {
     if (!mon->rs)
         return;
-    if (--mon->suspend_cnt == 0)
+    if (atomic_dec_fetch(&mon->suspend_cnt) == 0) {
         readline_show_prompt(mon->rs);
+    }
 }
 
 static QObject *get_qmp_greeting(Monitor *mon)
@@ -4068,19 +4069,19 @@ static void monitor_event(void *opaque, int event)
             monitor_resume(mon);
             monitor_flush(mon);
         } else {
-            mon->suspend_cnt = 0;
+            atomic_mb_set(&mon->suspend_cnt, 0);
         }
         break;
 
     case CHR_EVENT_MUX_OUT:
         if (mon->reset_seen) {
-            if (mon->suspend_cnt == 0) {
+            if (atomic_mb_read(&mon->suspend_cnt) == 0) {
                 monitor_printf(mon, "\n");
             }
             monitor_flush(mon);
             monitor_suspend(mon);
         } else {
-            mon->suspend_cnt++;
+            atomic_inc(&mon->suspend_cnt);
         }
         qemu_mutex_lock(&mon->out_lock);
         mon->mux_out = 1;
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 14/27] monitor: let suspend_cnt be thread safe

[Eric Blake]

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

On 12/19/2017 02:45 AM, Peter Xu wrote:
> Monitor code now can be run in more than one thread.  Let it be thread
> safe when accessing suspend_cnt counter.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index cf1e5d881c..844508d134 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3862,7 +3862,7 @@ static int monitor_can_read(void *opaque)
>  {
>      Monitor *mon = opaque;
>  
> -    return (mon->suspend_cnt == 0) ? 1 : 0;
> +    return (atomic_mb_read(&mon->suspend_cnt) == 0) ? 1 : 0;

Worth a comment at the declaration of suspend_cnt that it must be
accessed atomically?

I find anything that does '(boolexpr) ? 1 : 0' to be a pointless waste
of typing.  It is semantically equivalent and shorter to write either of:

return atomic_mb_read(&mon->suspend_cnt) == 0;
return !atomic_mb_read(&mon->suspend_cnt);

At any rate, I checked that all uses of suspend_cnt were converted over
to atomic API, so this looks correct, whether or not you add my tweaks.

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC v6 14/27] monitor: let suspend_cnt be thread safe

[Peter Xu]

On Fri, Jan 12, 2018 at 03:48:10PM -0600, Eric Blake wrote:
> On 12/19/2017 02:45 AM, Peter Xu wrote:
> > Monitor code now can be run in more than one thread.  Let it be thread
> > safe when accessing suspend_cnt counter.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  monitor.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index cf1e5d881c..844508d134 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -3862,7 +3862,7 @@ static int monitor_can_read(void *opaque)
> >  {
> >      Monitor *mon = opaque;
> >  
> > -    return (mon->suspend_cnt == 0) ? 1 : 0;
> > +    return (atomic_mb_read(&mon->suspend_cnt) == 0) ? 1 : 0;
> 
> Worth a comment at the declaration of suspend_cnt that it must be
> accessed atomically?

Sure.

> 
> I find anything that does '(boolexpr) ? 1 : 0' to be a pointless waste
> of typing.  It is semantically equivalent and shorter to write either of:
> 
> return atomic_mb_read(&mon->suspend_cnt) == 0;
> return !atomic_mb_read(&mon->suspend_cnt);

Sure, will use the latter.

> 
> At any rate, I checked that all uses of suspend_cnt were converted over
> to atomic API, so this looks correct, whether or not you add my tweaks.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks for reviewing.

-- 
Peter Xu

[Qemu-devel] [RFC v6 15/27] monitor: let suspend/resume work even with QMPs

[Peter Xu]

One thing to mention is that for QMPs that are using IOThreads, we need
an explicit kick for the IOThread in case it is sleeping.

Since at it, add traces for the operations.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c    | 26 +++++++++++++++++++++-----
 trace-events |  1 +
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/monitor.c b/monitor.c
index 844508d134..5f05f2e9da 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3992,19 +3992,35 @@ static void monitor_command_cb(void *opaque, const char *cmdline,
 
 int monitor_suspend(Monitor *mon)
 {
-    if (!mon->rs)
-        return -ENOTTY;
     atomic_inc(&mon->suspend_cnt);
+    if (monitor_is_qmp(mon)) {
+        /*
+         * Kick iothread to make sure this takes effect.  It'll be
+         * evaluated again in prepare() of the watch object.
+         */
+        aio_notify(iothread_get_aio_context(mon_global.mon_iothread));
+    }
+    trace_monitor_suspend(mon, 1);
     return 0;
 }
 
 void monitor_resume(Monitor *mon)
 {
-    if (!mon->rs)
-        return;
     if (atomic_dec_fetch(&mon->suspend_cnt) == 0) {
-        readline_show_prompt(mon->rs);
+        if (monitor_is_qmp(mon)) {
+            /*
+             * For QMP monitors that are running in IOThread, let's
+             * kick the thread in case it's sleeping.
+             */
+            if (mon->use_io_thr) {
+                aio_notify(iothread_get_aio_context(mon_global.mon_iothread));
+            }
+        } else {
+            assert(mon->rs);
+            readline_show_prompt(mon->rs);
+        }
     }
+    trace_monitor_suspend(mon, -1);
 }
 
 static QObject *get_qmp_greeting(Monitor *mon)
diff --git a/trace-events b/trace-events
index 1d2eb5d3e4..2646241a0f 100644
--- a/trace-events
+++ b/trace-events
@@ -47,6 +47,7 @@ monitor_protocol_event_emit(uint32_t event, void *data) "event=%d data=%p"
 monitor_protocol_event_queue(uint32_t event, void *qdict, uint64_t rate) "event=%d data=%p rate=%" PRId64
 handle_hmp_command(void *mon, const char *cmdline) "mon %p cmdline: %s"
 handle_qmp_command(void *mon, const char *req) "mon %p req: %s"
+monitor_suspend(void *ptr, int cnt) "mon %p: %d"
 
 # dma-helpers.c
 dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d"
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 15/27] monitor: let suspend/resume work even with QMPs

[Fam Zheng]

On Tue, 12/19 16:45, Peter Xu wrote:
> One thing to mention is that for QMPs that are using IOThreads, we need
> an explicit kick for the IOThread in case it is sleeping.
> 
> Since at it, add traces for the operations.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c    | 26 +++++++++++++++++++++-----
>  trace-events |  1 +
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 844508d134..5f05f2e9da 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3992,19 +3992,35 @@ static void monitor_command_cb(void *opaque, const char *cmdline,
>  
>  int monitor_suspend(Monitor *mon)
>  {
> -    if (!mon->rs)
> -        return -ENOTTY;

Please add to the commit message why the mon->rs check is dropped, for this and
the other one.

Fam

>      atomic_inc(&mon->suspend_cnt);
> +    if (monitor_is_qmp(mon)) {
> +        /*
> +         * Kick iothread to make sure this takes effect.  It'll be
> +         * evaluated again in prepare() of the watch object.
> +         */
> +        aio_notify(iothread_get_aio_context(mon_global.mon_iothread));
> +    }
> +    trace_monitor_suspend(mon, 1);
>      return 0;
>  }
>  
>  void monitor_resume(Monitor *mon)
>  {
> -    if (!mon->rs)
> -        return;
>      if (atomic_dec_fetch(&mon->suspend_cnt) == 0) {
> -        readline_show_prompt(mon->rs);
> +        if (monitor_is_qmp(mon)) {
> +            /*
> +             * For QMP monitors that are running in IOThread, let's
> +             * kick the thread in case it's sleeping.
> +             */
> +            if (mon->use_io_thr) {
> +                aio_notify(iothread_get_aio_context(mon_global.mon_iothread));
> +            }
> +        } else {
> +            assert(mon->rs);
> +            readline_show_prompt(mon->rs);
> +        }
>      }
> +    trace_monitor_suspend(mon, -1);
>  }
>  
>  static QObject *get_qmp_greeting(Monitor *mon)
> diff --git a/trace-events b/trace-events
> index 1d2eb5d3e4..2646241a0f 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -47,6 +47,7 @@ monitor_protocol_event_emit(uint32_t event, void *data) "event=%d data=%p"
>  monitor_protocol_event_queue(uint32_t event, void *qdict, uint64_t rate) "event=%d data=%p rate=%" PRId64
>  handle_hmp_command(void *mon, const char *cmdline) "mon %p cmdline: %s"
>  handle_qmp_command(void *mon, const char *req) "mon %p req: %s"
> +monitor_suspend(void *ptr, int cnt) "mon %p: %d"
>  
>  # dma-helpers.c
>  dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d"
> -- 
> 2.14.3
> 

Re: [Qemu-devel] [RFC v6 15/27] monitor: let suspend/resume work even with QMPs

[Peter Xu]

On Thu, Dec 21, 2017 at 07:27:38PM +0800, Fam Zheng wrote:
> On Tue, 12/19 16:45, Peter Xu wrote:
> > One thing to mention is that for QMPs that are using IOThreads, we need
> > an explicit kick for the IOThread in case it is sleeping.
> > 
> > Since at it, add traces for the operations.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  monitor.c    | 26 +++++++++++++++++++++-----
> >  trace-events |  1 +
> >  2 files changed, 22 insertions(+), 5 deletions(-)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index 844508d134..5f05f2e9da 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -3992,19 +3992,35 @@ static void monitor_command_cb(void *opaque, const char *cmdline,
> >  
> >  int monitor_suspend(Monitor *mon)
> >  {
> > -    if (!mon->rs)
> > -        return -ENOTTY;
> 
> Please add to the commit message why the mon->rs check is dropped, for this and
> the other one.

I thought it would be clear enough since mon->rs is only used by HMP
and the subject tells us that this patch is adding support for QMP.
But... sure I can add one more sentence for that!

-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 15/27] monitor: let suspend/resume work even with QMPs

[Stefan Hajnoczi]

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

On Mon, Dec 25, 2017 at 11:26:13AM +0800, Peter Xu wrote:
> On Thu, Dec 21, 2017 at 07:27:38PM +0800, Fam Zheng wrote:
> > On Tue, 12/19 16:45, Peter Xu wrote:
> > > One thing to mention is that for QMPs that are using IOThreads, we need
> > > an explicit kick for the IOThread in case it is sleeping.
> > > 
> > > Since at it, add traces for the operations.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  monitor.c    | 26 +++++++++++++++++++++-----
> > >  trace-events |  1 +
> > >  2 files changed, 22 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/monitor.c b/monitor.c
> > > index 844508d134..5f05f2e9da 100644
> > > --- a/monitor.c
> > > +++ b/monitor.c
> > > @@ -3992,19 +3992,35 @@ static void monitor_command_cb(void *opaque, const char *cmdline,
> > >  
> > >  int monitor_suspend(Monitor *mon)
> > >  {
> > > -    if (!mon->rs)
> > > -        return -ENOTTY;
> > 
> > Please add to the commit message why the mon->rs check is dropped, for this and
> > the other one.
> 
> I thought it would be clear enough since mon->rs is only used by HMP
> and the subject tells us that this patch is adding support for QMP.
> But... sure I can add one more sentence for that!

This change breaks hmp.c:hmp_migrate():

  if (monitor_suspend(mon) < 0) {
      monitor_printf(mon, "terminal does not allow synchronous "
                     "migration, continuing detached\n");
      return;
  }

mon->rs is used by HMP code to determine whether this is an interactive
terminal.  Both live migration and password prompting rely on this
because they must be forbidden when doing so would be impossible (e.g.
GDB stub tunneling HMP commands).

Suspending the QMP monitor is useful, but please don't break the HMP
code.

Stefan

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

Re: [Qemu-devel] [RFC v6 15/27] monitor: let suspend/resume work even with QMPs

[Peter Xu]

On Mon, Jan 08, 2018 at 04:49:36PM +0000, Stefan Hajnoczi wrote:
> On Mon, Dec 25, 2017 at 11:26:13AM +0800, Peter Xu wrote:
> > On Thu, Dec 21, 2017 at 07:27:38PM +0800, Fam Zheng wrote:
> > > On Tue, 12/19 16:45, Peter Xu wrote:
> > > > One thing to mention is that for QMPs that are using IOThreads, we need
> > > > an explicit kick for the IOThread in case it is sleeping.
> > > > 
> > > > Since at it, add traces for the operations.
> > > > 
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >  monitor.c    | 26 +++++++++++++++++++++-----
> > > >  trace-events |  1 +
> > > >  2 files changed, 22 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/monitor.c b/monitor.c
> > > > index 844508d134..5f05f2e9da 100644
> > > > --- a/monitor.c
> > > > +++ b/monitor.c
> > > > @@ -3992,19 +3992,35 @@ static void monitor_command_cb(void *opaque, const char *cmdline,
> > > >  
> > > >  int monitor_suspend(Monitor *mon)
> > > >  {
> > > > -    if (!mon->rs)
> > > > -        return -ENOTTY;
> > > 
> > > Please add to the commit message why the mon->rs check is dropped, for this and
> > > the other one.
> > 
> > I thought it would be clear enough since mon->rs is only used by HMP
> > and the subject tells us that this patch is adding support for QMP.
> > But... sure I can add one more sentence for that!
> 
> This change breaks hmp.c:hmp_migrate():
> 
>   if (monitor_suspend(mon) < 0) {
>       monitor_printf(mon, "terminal does not allow synchronous "
>                      "migration, continuing detached\n");
>       return;
>   }
> 
> mon->rs is used by HMP code to determine whether this is an interactive
> terminal.  Both live migration and password prompting rely on this
> because they must be forbidden when doing so would be impossible (e.g.
> GDB stub tunneling HMP commands).
> 
> Suspending the QMP monitor is useful, but please don't break the HMP
> code.

Could you elaborate a bit on how this patch breaks anything?

IMHO HMP monitors always have Monitor.rs setup, so here in
hmp_migrate() I don't really understand why we need this check, since
IIUC monitor_suspend() for a HMP monitor will never fail before.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 15/27] monitor: let suspend/resume work even with QMPs

[Stefan Hajnoczi]

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

On Fri, Jan 12, 2018 at 12:51:49PM +0800, Peter Xu wrote:
> On Mon, Jan 08, 2018 at 04:49:36PM +0000, Stefan Hajnoczi wrote:
> > On Mon, Dec 25, 2017 at 11:26:13AM +0800, Peter Xu wrote:
> > > On Thu, Dec 21, 2017 at 07:27:38PM +0800, Fam Zheng wrote:
> > > > On Tue, 12/19 16:45, Peter Xu wrote:
> > > > > One thing to mention is that for QMPs that are using IOThreads, we need
> > > > > an explicit kick for the IOThread in case it is sleeping.
> > > > > 
> > > > > Since at it, add traces for the operations.
> > > > > 
> > > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > > ---
> > > > >  monitor.c    | 26 +++++++++++++++++++++-----
> > > > >  trace-events |  1 +
> > > > >  2 files changed, 22 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/monitor.c b/monitor.c
> > > > > index 844508d134..5f05f2e9da 100644
> > > > > --- a/monitor.c
> > > > > +++ b/monitor.c
> > > > > @@ -3992,19 +3992,35 @@ static void monitor_command_cb(void *opaque, const char *cmdline,
> > > > >  
> > > > >  int monitor_suspend(Monitor *mon)
> > > > >  {
> > > > > -    if (!mon->rs)
> > > > > -        return -ENOTTY;
> > > > 
> > > > Please add to the commit message why the mon->rs check is dropped, for this and
> > > > the other one.
> > > 
> > > I thought it would be clear enough since mon->rs is only used by HMP
> > > and the subject tells us that this patch is adding support for QMP.
> > > But... sure I can add one more sentence for that!
> > 
> > This change breaks hmp.c:hmp_migrate():
> > 
> >   if (monitor_suspend(mon) < 0) {
> >       monitor_printf(mon, "terminal does not allow synchronous "
> >                      "migration, continuing detached\n");
> >       return;
> >   }
> > 
> > mon->rs is used by HMP code to determine whether this is an interactive
> > terminal.  Both live migration and password prompting rely on this
> > because they must be forbidden when doing so would be impossible (e.g.
> > GDB stub tunneling HMP commands).
> > 
> > Suspending the QMP monitor is useful, but please don't break the HMP
> > code.
> 
> Could you elaborate a bit on how this patch breaks anything?
> 
> IMHO HMP monitors always have Monitor.rs setup, so here in
> hmp_migrate() I don't really understand why we need this check, since
> IIUC monitor_suspend() for a HMP monitor will never fail before.

No, HMP monitors do not always have ->rs.  I mentioned that the GDB stub
uses a non-interactive HMP monitor.

Here is the code from gdbstub.c:

  /* Initialize a monitor terminal for gdb */
  mon_chr = qemu_chardev_new(NULL, TYPE_CHARDEV_GDB,
                             NULL, &error_abort);
  monitor_init(mon_chr, 0);

flags=0 means !MONITOR_USE_CONTROL (HMP) and !MONITOR_USE_READLINE
(non-interactive).

Your patch breaks this feature.

Stefan

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

Re: [Qemu-devel] [RFC v6 15/27] monitor: let suspend/resume work even with QMPs

[Peter Xu]

On Fri, Jan 12, 2018 at 02:28:32PM +0000, Stefan Hajnoczi wrote:
> On Fri, Jan 12, 2018 at 12:51:49PM +0800, Peter Xu wrote:
> > On Mon, Jan 08, 2018 at 04:49:36PM +0000, Stefan Hajnoczi wrote:
> > > On Mon, Dec 25, 2017 at 11:26:13AM +0800, Peter Xu wrote:
> > > > On Thu, Dec 21, 2017 at 07:27:38PM +0800, Fam Zheng wrote:
> > > > > On Tue, 12/19 16:45, Peter Xu wrote:
> > > > > > One thing to mention is that for QMPs that are using IOThreads, we need
> > > > > > an explicit kick for the IOThread in case it is sleeping.
> > > > > > 
> > > > > > Since at it, add traces for the operations.
> > > > > > 
> > > > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > > > ---
> > > > > >  monitor.c    | 26 +++++++++++++++++++++-----
> > > > > >  trace-events |  1 +
> > > > > >  2 files changed, 22 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/monitor.c b/monitor.c
> > > > > > index 844508d134..5f05f2e9da 100644
> > > > > > --- a/monitor.c
> > > > > > +++ b/monitor.c
> > > > > > @@ -3992,19 +3992,35 @@ static void monitor_command_cb(void *opaque, const char *cmdline,
> > > > > >  
> > > > > >  int monitor_suspend(Monitor *mon)
> > > > > >  {
> > > > > > -    if (!mon->rs)
> > > > > > -        return -ENOTTY;
> > > > > 
> > > > > Please add to the commit message why the mon->rs check is dropped, for this and
> > > > > the other one.
> > > > 
> > > > I thought it would be clear enough since mon->rs is only used by HMP
> > > > and the subject tells us that this patch is adding support for QMP.
> > > > But... sure I can add one more sentence for that!
> > > 
> > > This change breaks hmp.c:hmp_migrate():
> > > 
> > >   if (monitor_suspend(mon) < 0) {
> > >       monitor_printf(mon, "terminal does not allow synchronous "
> > >                      "migration, continuing detached\n");
> > >       return;
> > >   }
> > > 
> > > mon->rs is used by HMP code to determine whether this is an interactive
> > > terminal.  Both live migration and password prompting rely on this
> > > because they must be forbidden when doing so would be impossible (e.g.
> > > GDB stub tunneling HMP commands).
> > > 
> > > Suspending the QMP monitor is useful, but please don't break the HMP
> > > code.
> > 
> > Could you elaborate a bit on how this patch breaks anything?
> > 
> > IMHO HMP monitors always have Monitor.rs setup, so here in
> > hmp_migrate() I don't really understand why we need this check, since
> > IIUC monitor_suspend() for a HMP monitor will never fail before.
> 
> No, HMP monitors do not always have ->rs.  I mentioned that the GDB stub
> uses a non-interactive HMP monitor.
> 
> Here is the code from gdbstub.c:
> 
>   /* Initialize a monitor terminal for gdb */
>   mon_chr = qemu_chardev_new(NULL, TYPE_CHARDEV_GDB,
>                              NULL, &error_abort);
>   monitor_init(mon_chr, 0);
> 
> flags=0 means !MONITOR_USE_CONTROL (HMP) and !MONITOR_USE_READLINE
> (non-interactive).
> 
> Your patch breaks this feature.

Ah I see the point.  I'll fix that up.  So Fam's questioning is valid,
mon->rs needs to be checked.

Even, I would prefer to check the flags - they are more understandable
than checking against mon->rs.

Thanks for explaining.

-- 
Peter Xu

[Qemu-devel] [RFC v6 16/27] monitor: separate QMP parser and dispatcher

[Peter Xu]

Originally QMP goes throw these steps:

  JSON Parser --> QMP Dispatcher --> Respond
      /|\    (2)                (3)     |
   (1) |                               \|/ (4)
       +---------  main thread  --------+

This patch does this:

  JSON Parser     QMP Dispatcher --> Respond
      /|\ |           /|\       (4)     |
       |  | (2)        | (3)            |  (5)
   (1) |  +----->      |               \|/
       +---------  main thread  <-------+

So the parsing job and the dispatching job is isolated now.  It gives us
a chance in following up patches to totally move the parser outside.

The isloation is done using one QEMUBH. Only one dispatcher QEMUBH is
used for all the monitors.

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

diff --git a/monitor.c b/monitor.c
index 5f05f2e9da..ed9a741d06 100644
--- a/monitor.c
+++ b/monitor.c
@@ -167,6 +167,13 @@ typedef struct {
      */
     QmpCommandList *commands;
     bool qmp_caps[QMP_CAPABILITY__MAX];
+    /*
+     * Protects qmp request/response queue.  Please take monitor_lock
+     * first when used together.
+     */
+    QemuMutex qmp_queue_lock;
+    /* Input queue that holds all the parsed QMP requests */
+    GQueue *qmp_requests;
 } MonitorQMP;
 
 /*
@@ -213,6 +220,8 @@ struct Monitor {
 /* Let's add monitor global variables to this struct. */
 static struct {
     IOThread *mon_iothread;
+    /* Bottom half to dispatch the requests received from IO thread */
+    QEMUBH *qmp_dispatcher_bh;
 } mon_global;
 
 /* QMP checker flags */
@@ -580,11 +589,13 @@ static void monitor_data_init(Monitor *mon, bool skip_flush,
 {
     memset(mon, 0, sizeof(Monitor));
     qemu_mutex_init(&mon->out_lock);
+    qemu_mutex_init(&mon->qmp.qmp_queue_lock);
     mon->outbuf = qstring_new();
     /* Use *mon_cmds by default. */
     mon->cmd_table = mon_cmds;
     mon->skip_flush = skip_flush;
     mon->use_io_thr = use_io_thr;
+    mon->qmp.qmp_requests = g_queue_new();
 }
 
 static void monitor_data_destroy(Monitor *mon)
@@ -597,6 +608,8 @@ static void monitor_data_destroy(Monitor *mon)
     g_free(mon->rs);
     QDECREF(mon->outbuf);
     qemu_mutex_destroy(&mon->out_lock);
+    qemu_mutex_destroy(&mon->qmp.qmp_queue_lock);
+    g_queue_free(mon->qmp.qmp_requests);
 }
 
 char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
@@ -1036,6 +1049,16 @@ static void monitor_init_qmp_commands(void)
                          qmp_marshal_qmp_capabilities, QCO_NO_OPTIONS);
 }
 
+static bool qmp_cap_enabled(Monitor *mon, QMPCapability cap)
+{
+    return mon->qmp.qmp_caps[cap];
+}
+
+static bool qmp_oob_enabled(Monitor *mon)
+{
+    return qmp_cap_enabled(mon, QMP_CAPABILITY_OOB);
+}
+
 static void qmp_caps_check(Monitor *mon, QMPCapabilityList *list,
                            Error **errp)
 {
@@ -3897,30 +3920,39 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp,
     qobject_decref(rsp);
 }
 
-static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
+struct QMPRequest {
+    /* Owner of the request */
+    Monitor *mon;
+    /* "id" field of the request */
+    QObject *id;
+    /* Request object to be handled */
+    QObject *req;
+    /*
+     * Whether we need to resume the monitor afterward.  This flag is
+     * used to emulate the old QMP server behavior that only one
+     * command is executed for each time.
+     */
+    bool need_resume;
+};
+typedef struct QMPRequest QMPRequest;
+
+/*
+ * Dispatch one single QMP request. The function will free the req_obj
+ * and objects inside it before return.
+ */
+static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
 {
-    QObject *req, *rsp = NULL, *id = NULL;
+    Monitor *mon, *old_mon;
+    QObject *req, *rsp = NULL, *id;
     QDict *qdict = NULL;
-    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
-    Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
-
-    Error *err = NULL;
+    bool need_resume;
 
-    req = json_parser_parse_err(tokens, NULL, &err);
-    if (!req && !err) {
-        /* json_parser_parse_err() sucks: can fail without setting @err */
-        error_setg(&err, QERR_JSON_PARSING);
-    }
-    if (err) {
-        goto err_out;
-    }
+    req = req_obj->req;
+    mon = req_obj->mon;
+    id = req_obj->id;
+    need_resume = req_obj->need_resume;
 
-    qdict = qobject_to_qdict(req);
-    if (qdict) {
-        id = qdict_get(qdict, "id");
-        qobject_incref(id);
-        qdict_del(qdict, "id");
-    } /* else will fail qmp_dispatch() */
+    g_free(req_obj);
 
     if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
         QString *req_json = qobject_to_json(req);
@@ -3931,7 +3963,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     old_mon = cur_mon;
     cur_mon = mon;
 
-    rsp = qmp_dispatch(cur_mon->qmp.commands, req);
+    rsp = qmp_dispatch(mon->qmp.commands, req);
 
     cur_mon = old_mon;
 
@@ -3947,12 +3979,122 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
         }
     }
 
-err_out:
-    monitor_qmp_respond(mon, rsp, err, id);
+    /* Respond if necessary */
+    monitor_qmp_respond(mon, rsp, NULL, id);
+
+    /* This pairs with the monitor_suspend() in handle_qmp_command(). */
+    if (need_resume) {
+        monitor_resume(mon);
+    }
 
     qobject_decref(req);
 }
 
+/*
+ * Pop one QMP request from monitor queues, return NULL if not found.
+ * We are using round-robin fasion to pop the request, to avoid
+ * processing command only on a very busy monitor.  To achieve that,
+ * when we processed one request on specific monitor, we put that
+ * monitor to the end of mon_list queue.
+ */
+static QMPRequest *monitor_qmp_requests_pop_one(void)
+{
+    QMPRequest *req_obj = NULL;
+    Monitor *mon;
+
+    qemu_mutex_lock(&monitor_lock);
+
+    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) {
+            break;
+        }
+    }
+
+    if (req_obj) {
+        /*
+         * We found one request on the monitor. Degrade this monitor's
+         * priority to lowest by re-inserting it to end of queue.
+         */
+        QTAILQ_REMOVE(&mon_list, mon, entry);
+        QTAILQ_INSERT_TAIL(&mon_list, mon, entry);
+    }
+
+    qemu_mutex_unlock(&monitor_lock);
+
+    return req_obj;
+}
+
+static void monitor_qmp_bh_dispatcher(void *data)
+{
+    QMPRequest *req_obj = monitor_qmp_requests_pop_one();
+
+    if (req_obj) {
+        monitor_qmp_dispatch_one(req_obj);
+        /* Reschedule instead of looping so the main loop stays responsive */
+        qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
+    }
+}
+
+static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
+{
+    QObject *req, *id = NULL;
+    QDict *qdict = NULL;
+    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
+    Monitor *mon = container_of(mon_qmp, Monitor, qmp);
+    Error *err = NULL;
+    QMPRequest *req_obj;
+
+    req = json_parser_parse_err(tokens, NULL, &err);
+    if (!req && !err) {
+        /* json_parser_parse_err() sucks: can fail without setting @err */
+        error_setg(&err, QERR_JSON_PARSING);
+    }
+    if (err) {
+        monitor_qmp_respond(mon, NULL, err, NULL);
+        qobject_decref(req);
+        return;
+    }
+
+    qdict = qobject_to_qdict(req);
+    if (qdict) {
+        id = qdict_get(qdict, "id");
+        qobject_incref(id);
+        qdict_del(qdict, "id");
+    } /* else will fail qmp_dispatch() */
+
+    req_obj = g_new0(QMPRequest, 1);
+    req_obj->mon = mon;
+    req_obj->id = id;
+    req_obj->req = req;
+    req_obj->need_resume = false;
+
+    /*
+     * If OOB is not enabled on current monitor, we'll emulate the old
+     * behavior that we won't process current monitor any more until
+     * it is responded.  This helps make sure that as long as OOB is
+     * not enabled, the server will never drop any command.
+     */
+    if (!qmp_oob_enabled(mon)) {
+        monitor_suspend(mon);
+        req_obj->need_resume = true;
+    }
+
+    /*
+     * Put the request to the end of queue so that requests will be
+     * handled in time order.  Ownership for req_obj, req, id,
+     * etc. will be delivered to the handler side.
+     */
+    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
+    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+
+    /* Kick the dispatcher routine */
+    qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
+}
+
 static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
 {
     Monitor *mon = opaque;
@@ -4150,6 +4292,15 @@ static void monitor_iothread_init(void)
 {
     mon_global.mon_iothread = iothread_create("mon_iothread",
                                               &error_abort);
+
+    /*
+     * This MUST be on main loop thread since we have commands that
+     * have assumption to be run on main loop thread (Yeah, we'd
+     * better remove this assumption in the future).
+     */
+    mon_global.qmp_dispatcher_bh = aio_bh_new(qemu_get_aio_context(),
+                                              monitor_qmp_bh_dispatcher,
+                                              NULL);
 }
 
 void monitor_init_globals(void)
@@ -4272,6 +4423,10 @@ void monitor_cleanup(void)
     }
     qemu_mutex_unlock(&monitor_lock);
 
+    /* QEMUBHs needs to be deleted before destroying the IOThread. */
+    qemu_bh_delete(mon_global.qmp_dispatcher_bh);
+    mon_global.qmp_dispatcher_bh = NULL;
+
     iothread_destroy(mon_global.mon_iothread);
     mon_global.mon_iothread = NULL;
 }
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 16/27] monitor: separate QMP parser and dispatcher

[Fam Zheng]

On Tue, 12/19 16:45, Peter Xu wrote:
> Originally QMP goes throw these steps:
> 
>   JSON Parser --> QMP Dispatcher --> Respond
>       /|\    (2)                (3)     |
>    (1) |                               \|/ (4)
>        +---------  main thread  --------+
> 
> This patch does this:
> 
>   JSON Parser     QMP Dispatcher --> Respond
>       /|\ |           /|\       (4)     |
>        |  | (2)        | (3)            |  (5)
>    (1) |  +----->      |               \|/
>        +---------  main thread  <-------+
> 
> So the parsing job and the dispatching job is isolated now.  It gives us
> a chance in following up patches to totally move the parser outside.
> 
> The isloation is done using one QEMUBH. Only one dispatcher QEMUBH is
> used for all the monitors.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 178 insertions(+), 23 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 5f05f2e9da..ed9a741d06 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -167,6 +167,13 @@ typedef struct {
>       */
>      QmpCommandList *commands;
>      bool qmp_caps[QMP_CAPABILITY__MAX];
> +    /*
> +     * Protects qmp request/response queue.  Please take monitor_lock
> +     * first when used together.
> +     */
> +    QemuMutex qmp_queue_lock;
> +    /* Input queue that holds all the parsed QMP requests */
> +    GQueue *qmp_requests;
>  } MonitorQMP;
>  
>  /*
> @@ -213,6 +220,8 @@ struct Monitor {
>  /* Let's add monitor global variables to this struct. */
>  static struct {
>      IOThread *mon_iothread;
> +    /* Bottom half to dispatch the requests received from IO thread */
> +    QEMUBH *qmp_dispatcher_bh;
>  } mon_global;
>  
>  /* QMP checker flags */
> @@ -580,11 +589,13 @@ static void monitor_data_init(Monitor *mon, bool skip_flush,
>  {
>      memset(mon, 0, sizeof(Monitor));
>      qemu_mutex_init(&mon->out_lock);
> +    qemu_mutex_init(&mon->qmp.qmp_queue_lock);
>      mon->outbuf = qstring_new();
>      /* Use *mon_cmds by default. */
>      mon->cmd_table = mon_cmds;
>      mon->skip_flush = skip_flush;
>      mon->use_io_thr = use_io_thr;
> +    mon->qmp.qmp_requests = g_queue_new();
>  }
>  
>  static void monitor_data_destroy(Monitor *mon)
> @@ -597,6 +608,8 @@ static void monitor_data_destroy(Monitor *mon)
>      g_free(mon->rs);
>      QDECREF(mon->outbuf);
>      qemu_mutex_destroy(&mon->out_lock);
> +    qemu_mutex_destroy(&mon->qmp.qmp_queue_lock);
> +    g_queue_free(mon->qmp.qmp_requests);
>  }
>  
>  char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
> @@ -1036,6 +1049,16 @@ static void monitor_init_qmp_commands(void)
>                           qmp_marshal_qmp_capabilities, QCO_NO_OPTIONS);
>  }
>  
> +static bool qmp_cap_enabled(Monitor *mon, QMPCapability cap)
> +{
> +    return mon->qmp.qmp_caps[cap];
> +}
> +
> +static bool qmp_oob_enabled(Monitor *mon)
> +{
> +    return qmp_cap_enabled(mon, QMP_CAPABILITY_OOB);
> +}
> +
>  static void qmp_caps_check(Monitor *mon, QMPCapabilityList *list,
>                             Error **errp)
>  {
> @@ -3897,30 +3920,39 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp,
>      qobject_decref(rsp);
>  }
>  
> -static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> +struct QMPRequest {
> +    /* Owner of the request */
> +    Monitor *mon;
> +    /* "id" field of the request */
> +    QObject *id;
> +    /* Request object to be handled */
> +    QObject *req;
> +    /*
> +     * Whether we need to resume the monitor afterward.  This flag is
> +     * used to emulate the old QMP server behavior that only one
> +     * command is executed for each time.
> +     */
> +    bool need_resume;
> +};
> +typedef struct QMPRequest QMPRequest;
> +
> +/*
> + * Dispatch one single QMP request. The function will free the req_obj
> + * and objects inside it before return.
> + */
> +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
>  {
> -    QObject *req, *rsp = NULL, *id = NULL;
> +    Monitor *mon, *old_mon;
> +    QObject *req, *rsp = NULL, *id;
>      QDict *qdict = NULL;
> -    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
> -    Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
> -
> -    Error *err = NULL;
> +    bool need_resume;
>  
> -    req = json_parser_parse_err(tokens, NULL, &err);
> -    if (!req && !err) {
> -        /* json_parser_parse_err() sucks: can fail without setting @err */
> -        error_setg(&err, QERR_JSON_PARSING);
> -    }
> -    if (err) {
> -        goto err_out;
> -    }
> +    req = req_obj->req;
> +    mon = req_obj->mon;
> +    id = req_obj->id;
> +    need_resume = req_obj->need_resume;
>  
> -    qdict = qobject_to_qdict(req);
> -    if (qdict) {
> -        id = qdict_get(qdict, "id");
> -        qobject_incref(id);
> -        qdict_del(qdict, "id");
> -    } /* else will fail qmp_dispatch() */
> +    g_free(req_obj);
>  
>      if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
>          QString *req_json = qobject_to_json(req);
> @@ -3931,7 +3963,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>      old_mon = cur_mon;
>      cur_mon = mon;
>  
> -    rsp = qmp_dispatch(cur_mon->qmp.commands, req);
> +    rsp = qmp_dispatch(mon->qmp.commands, req);
>  
>      cur_mon = old_mon;
>  
> @@ -3947,12 +3979,122 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>          }
>      }
>  
> -err_out:
> -    monitor_qmp_respond(mon, rsp, err, id);
> +    /* Respond if necessary */
> +    monitor_qmp_respond(mon, rsp, NULL, id);
> +
> +    /* This pairs with the monitor_suspend() in handle_qmp_command(). */
> +    if (need_resume) {
> +        monitor_resume(mon);
> +    }
>  
>      qobject_decref(req);
>  }
>  
> +/*
> + * Pop one QMP request from monitor queues, return NULL if not found.
> + * We are using round-robin fasion to pop the request, to avoid

s/fasion/fashion/

> + * processing command only on a very busy monitor.  To achieve that,
> + * when we processed one request on specific monitor, we put that
> + * monitor to the end of mon_list queue.
> + */
> +static QMPRequest *monitor_qmp_requests_pop_one(void)
> +{
> +    QMPRequest *req_obj = NULL;
> +    Monitor *mon;
> +
> +    qemu_mutex_lock(&monitor_lock);
> +
> +    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) {
> +            break;
> +        }
> +    }
> +
> +    if (req_obj) {
> +        /*
> +         * We found one request on the monitor. Degrade this monitor's
> +         * priority to lowest by re-inserting it to end of queue.
> +         */
> +        QTAILQ_REMOVE(&mon_list, mon, entry);
> +        QTAILQ_INSERT_TAIL(&mon_list, mon, entry);
> +    }
> +
> +    qemu_mutex_unlock(&monitor_lock);
> +
> +    return req_obj;
> +}
> +
> +static void monitor_qmp_bh_dispatcher(void *data)
> +{
> +    QMPRequest *req_obj = monitor_qmp_requests_pop_one();
> +
> +    if (req_obj) {
> +        monitor_qmp_dispatch_one(req_obj);
> +        /* Reschedule instead of looping so the main loop stays responsive */
> +        qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> +    }
> +}
> +
> +static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> +{
> +    QObject *req, *id = NULL;
> +    QDict *qdict = NULL;
> +    MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
> +    Monitor *mon = container_of(mon_qmp, Monitor, qmp);
> +    Error *err = NULL;
> +    QMPRequest *req_obj;
> +
> +    req = json_parser_parse_err(tokens, NULL, &err);
> +    if (!req && !err) {
> +        /* json_parser_parse_err() sucks: can fail without setting @err */
> +        error_setg(&err, QERR_JSON_PARSING);
> +    }
> +    if (err) {
> +        monitor_qmp_respond(mon, NULL, err, NULL);
> +        qobject_decref(req);
> +        return;
> +    }
> +
> +    qdict = qobject_to_qdict(req);
> +    if (qdict) {
> +        id = qdict_get(qdict, "id");
> +        qobject_incref(id);
> +        qdict_del(qdict, "id");
> +    } /* else will fail qmp_dispatch() */
> +
> +    req_obj = g_new0(QMPRequest, 1);
> +    req_obj->mon = mon;
> +    req_obj->id = id;
> +    req_obj->req = req;
> +    req_obj->need_resume = false;
> +
> +    /*
> +     * If OOB is not enabled on current monitor, we'll emulate the old
> +     * behavior that we won't process current monitor any more until
> +     * it is responded.  This helps make sure that as long as OOB is
> +     * not enabled, the server will never drop any command.
> +     */
> +    if (!qmp_oob_enabled(mon)) {
> +        monitor_suspend(mon);
> +        req_obj->need_resume = true;
> +    }
> +
> +    /*
> +     * Put the request to the end of queue so that requests will be
> +     * handled in time order.  Ownership for req_obj, req, id,
> +     * etc. will be delivered to the handler side.
> +     */
> +    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> +    g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
> +    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> +
> +    /* Kick the dispatcher routine */
> +    qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> +}
> +
>  static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
>  {
>      Monitor *mon = opaque;
> @@ -4150,6 +4292,15 @@ static void monitor_iothread_init(void)
>  {
>      mon_global.mon_iothread = iothread_create("mon_iothread",
>                                                &error_abort);
> +
> +    /*
> +     * This MUST be on main loop thread since we have commands that
> +     * have assumption to be run on main loop thread (Yeah, we'd
> +     * better remove this assumption in the future).

Assert and move this comment in monitor_qmp_bh_dispatcher?

> +     */
> +    mon_global.qmp_dispatcher_bh = aio_bh_new(qemu_get_aio_context(),
> +                                              monitor_qmp_bh_dispatcher,
> +                                              NULL);
>  }
>  

Fam

Re: [Qemu-devel] [RFC v6 16/27] monitor: separate QMP parser and dispatcher

[Peter Xu]

On Thu, Dec 21, 2017 at 07:40:13PM +0800, Fam Zheng wrote:

[...]

> > +/*
> > + * Pop one QMP request from monitor queues, return NULL if not found.
> > + * We are using round-robin fasion to pop the request, to avoid
> 
> s/fasion/fashion/

Fixed.

[...]

> >  static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
> >  {
> >      Monitor *mon = opaque;
> > @@ -4150,6 +4292,15 @@ static void monitor_iothread_init(void)
> >  {
> >      mon_global.mon_iothread = iothread_create("mon_iothread",
> >                                                &error_abort);
> > +
> > +    /*
> > +     * This MUST be on main loop thread since we have commands that
> > +     * have assumption to be run on main loop thread (Yeah, we'd
> > +     * better remove this assumption in the future).
> 
> Assert and move this comment in monitor_qmp_bh_dispatcher?

I would still prefer to keep it here.  IMHO what really matters is the
first parameter that passed in to aio_bh_new() below, rather than
monitor_qmp_bh_dispatcher itself?  Thanks,

> 
> > +     */
> > +    mon_global.qmp_dispatcher_bh = aio_bh_new(qemu_get_aio_context(),
> > +                                              monitor_qmp_bh_dispatcher,
> > +                                              NULL);
> >  }
> >  
> 
> Fam

-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 16/27] monitor: separate QMP parser and dispatcher

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:46PM +0800, Peter Xu wrote:
> Originally QMP goes throw these steps:

s/throw/through/

> 
>   JSON Parser --> QMP Dispatcher --> Respond
>       /|\    (2)                (3)     |
>    (1) |                               \|/ (4)
>        +---------  main thread  --------+
> 
> This patch does this:
> 
>   JSON Parser     QMP Dispatcher --> Respond
>       /|\ |           /|\       (4)     |
>        |  | (2)        | (3)            |  (5)
>    (1) |  +----->      |               \|/
>        +---------  main thread  <-------+
> 
> So the parsing job and the dispatching job is isolated now.  It gives us
> a chance in following up patches to totally move the parser outside.
> 
> The isloation is done using one QEMUBH. Only one dispatcher QEMUBH is

s/isloation/isolation/

> @@ -3897,30 +3920,39 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp,
>      qobject_decref(rsp);
>  }
>  
> -static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> +struct QMPRequest {
> +    /* Owner of the request */
> +    Monitor *mon;
> +    /* "id" field of the request */
> +    QObject *id;
> +    /* Request object to be handled */
> +    QObject *req;
> +    /*
> +     * Whether we need to resume the monitor afterward.  This flag is
> +     * used to emulate the old QMP server behavior that only one
> +     * command is executed for each time.

s/only one command is executed for each time/the current command must
complete before the next one executes/

> +     */
> +    bool need_resume;

This isn't really a per-request decision so a QMPRequest field is not
necessary.  If "oob" is enabled then we dispatch commands without
waiting.  If "oob" is disabled then we complete the current command
before dispatching the next one.

If you want to keep this, I don't mind, but the field isn't necessary.

> @@ -4150,6 +4292,15 @@ static void monitor_iothread_init(void)
>  {
>      mon_global.mon_iothread = iothread_create("mon_iothread",
>                                                &error_abort);
> +
> +    /*
> +     * This MUST be on main loop thread since we have commands that
> +     * have assumption to be run on main loop thread (Yeah, we'd
> +     * better remove this assumption in the future).

"we'd better ..." usually means that it will be necessary.  It is
stronger than "it would be nice to ...".  I'm not sure which one you
mean here.

There doesn't seem to be any immediate need or plan to run regular
commands outside the main loop.  I'm not aware of active work to do
that.  So what is this comment trying to say?

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

Re: [Qemu-devel] [RFC v6 16/27] monitor: separate QMP parser and dispatcher

[Peter Xu]

On Mon, Jan 08, 2018 at 05:09:16PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 19, 2017 at 04:45:46PM +0800, Peter Xu wrote:
> > Originally QMP goes throw these steps:
> 
> s/throw/through/
> 
> > 
> >   JSON Parser --> QMP Dispatcher --> Respond
> >       /|\    (2)                (3)     |
> >    (1) |                               \|/ (4)
> >        +---------  main thread  --------+
> > 
> > This patch does this:
> > 
> >   JSON Parser     QMP Dispatcher --> Respond
> >       /|\ |           /|\       (4)     |
> >        |  | (2)        | (3)            |  (5)
> >    (1) |  +----->      |               \|/
> >        +---------  main thread  <-------+
> > 
> > So the parsing job and the dispatching job is isolated now.  It gives us
> > a chance in following up patches to totally move the parser outside.
> > 
> > The isloation is done using one QEMUBH. Only one dispatcher QEMUBH is
> 
> s/isloation/isolation/
> 
> > @@ -3897,30 +3920,39 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp,
> >      qobject_decref(rsp);
> >  }
> >  
> > -static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> > +struct QMPRequest {
> > +    /* Owner of the request */
> > +    Monitor *mon;
> > +    /* "id" field of the request */
> > +    QObject *id;
> > +    /* Request object to be handled */
> > +    QObject *req;
> > +    /*
> > +     * Whether we need to resume the monitor afterward.  This flag is
> > +     * used to emulate the old QMP server behavior that only one
> > +     * command is executed for each time.
> 
> s/only one command is executed for each time/the current command must
> complete before the next one executes/

Will address all the grammar issues.

> 
> > +     */
> > +    bool need_resume;
> 
> This isn't really a per-request decision so a QMPRequest field is not
> necessary.  If "oob" is enabled then we dispatch commands without
> waiting.  If "oob" is disabled then we complete the current command
> before dispatching the next one.

I explicitly added this to make sure the suspend and resume will be
paired up.  Actually there is at least one exception, which is the
initial "qmp_capabilities" command especially when used to enable the
OOB capability. For that case, OOB is not enabled before execution,
while OOB is enabled during the execution and after running the
command.  If without this field, monitor can hang.

> 
> If you want to keep this, I don't mind, but the field isn't necessary.

According to above, I would still like to keep it.

> 
> > @@ -4150,6 +4292,15 @@ static void monitor_iothread_init(void)
> >  {
> >      mon_global.mon_iothread = iothread_create("mon_iothread",
> >                                                &error_abort);
> > +
> > +    /*
> > +     * This MUST be on main loop thread since we have commands that
> > +     * have assumption to be run on main loop thread (Yeah, we'd
> > +     * better remove this assumption in the future).
> 
> "we'd better ..." usually means that it will be necessary.  It is
> stronger than "it would be nice to ...".  I'm not sure which one you
> mean here.
> 
> There doesn't seem to be any immediate need or plan to run regular
> commands outside the main loop.  I'm not aware of active work to do
> that.  So what is this comment trying to say?

Yeah I mean "it would be nice to", I'll switch.

This comment explains why dispatcher bottom half is bound to the
default main thread and it cannot be bound to anything else.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 16/27] monitor: separate QMP parser and dispatcher

[Stefan Hajnoczi]

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

On Fri, Jan 12, 2018 at 02:05:15PM +0800, Peter Xu wrote:
> On Mon, Jan 08, 2018 at 05:09:16PM +0000, Stefan Hajnoczi wrote:
> > On Tue, Dec 19, 2017 at 04:45:46PM +0800, Peter Xu wrote:
> > 
> > > +     */
> > > +    bool need_resume;
> > 
> > This isn't really a per-request decision so a QMPRequest field is not
> > necessary.  If "oob" is enabled then we dispatch commands without
> > waiting.  If "oob" is disabled then we complete the current command
> > before dispatching the next one.
> 
> I explicitly added this to make sure the suspend and resume will be
> paired up.  Actually there is at least one exception, which is the
> initial "qmp_capabilities" command especially when used to enable the
> OOB capability. For that case, OOB is not enabled before execution,
> while OOB is enabled during the execution and after running the
> command.  If without this field, monitor can hang.

Good point, your approach avoids bugs.  It's safest to leave it as is.

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

[Qemu-devel] [RFC v6 17/27] qmp: add new event "command-dropped"

[Peter Xu]

This event will be emitted if one QMP command is dropped.  Along,
declare an enum for the reasons.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 qapi-schema.json | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index 531fd4c0db..447ab25572 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3222,3 +3222,40 @@
 # Since: 2.11
 ##
 { 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} }
+
+##
+# @CommandDropReason:
+#
+# Reasons that caused one command to be dropped.
+#
+# @queue-full: the queue of command 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.
+#
+# @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' } }
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 17/27] qmp: add new event "command-dropped"

[Fam Zheng]

On Tue, 12/19 16:45, Peter Xu wrote:
> This event will be emitted if one QMP command is dropped.  Along,
> declare an enum for the reasons.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [RFC v6 17/27] qmp: add new event "command-dropped"

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:47PM +0800, Peter Xu wrote:
> This event will be emitted if one QMP command is dropped.  Along,
> declare an enum for the reasons.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  qapi-schema.json | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 531fd4c0db..447ab25572 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3222,3 +3222,40 @@
>  # Since: 2.11
>  ##
>  { 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} }
> +
> +##
> +# @CommandDropReason:
> +#
> +# Reasons that caused one command to be dropped.
> +#
> +# @queue-full: the queue of command is full. This can only occur when

s/the queue of command is full/the command queue is full/

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

Re: [Qemu-devel] [RFC v6 17/27] qmp: add new event "command-dropped"

[Peter Xu]

On Tue, Jan 09, 2018 at 01:20:20PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 19, 2017 at 04:45:47PM +0800, Peter Xu wrote:
> > This event will be emitted if one QMP command is dropped.  Along,
> > declare an enum for the reasons.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  qapi-schema.json | 37 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 531fd4c0db..447ab25572 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -3222,3 +3222,40 @@
> >  # Since: 2.11
> >  ##
> >  { 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} }
> > +
> > +##
> > +# @CommandDropReason:
> > +#
> > +# Reasons that caused one command to be dropped.
> > +#
> > +# @queue-full: the queue of command is full. This can only occur when
> 
> s/the queue of command is full/the command queue is full/

Fixed.  Thanks,

-- 
Peter Xu

[Qemu-devel] [RFC v6 18/27] monitor: send event when command queue full

[Peter Xu]

Set maximum QMP command queue length to 8.  If queue full, instead of
queue the command, we directly return a "command-dropped" event, telling
client that specific command is dropped.

Note that this flow control mechanism is only valid if OOB is enabled.
If it's not, the effective queue length will always be 1, which strictly
follows original behavior of QMP command handling (which never drop
messages).

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/monitor.c b/monitor.c
index ed9a741d06..b571866659 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4038,6 +4038,8 @@ static void monitor_qmp_bh_dispatcher(void *data)
     }
 }
 
+#define  QMP_REQ_QUEUE_LEN_MAX  (8)
+
 static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
 {
     QObject *req, *id = NULL;
@@ -4071,6 +4073,9 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     req_obj->req = req;
     req_obj->need_resume = false;
 
+    /* Protect qmp_requests and fetching its length. */
+    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+
     /*
      * If OOB is not enabled on current monitor, we'll emulate the old
      * behavior that we won't process current monitor any more until
@@ -4080,6 +4085,17 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     if (!qmp_oob_enabled(mon)) {
         monitor_suspend(mon);
         req_obj->need_resume = true;
+    } else {
+        /* Drop the request if queue is full. */
+        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
+            qapi_event_send_command_dropped(id,
+                                            COMMAND_DROP_REASON_QUEUE_FULL,
+                                            NULL);
+            qobject_decref(id);
+            qobject_decref(req);
+            g_free(req_obj);
+            return;
+        }
     }
 
     /*
@@ -4087,7 +4103,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
      * handled in time order.  Ownership for req_obj, req, id,
      * etc. will be delivered to the handler side.
      */
-    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
     g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
     qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
 
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 18/27] monitor: send event when command queue full

[Fam Zheng]

On Tue, 12/19 16:45, Peter Xu wrote:
> Set maximum QMP command queue length to 8.  If queue full, instead of
> queue the command, we directly return a "command-dropped" event, telling
> client that specific command is dropped.
> 
> Note that this flow control mechanism is only valid if OOB is enabled.
> If it's not, the effective queue length will always be 1, which strictly
> follows original behavior of QMP command handling (which never drop
> messages).
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/monitor.c b/monitor.c
> index ed9a741d06..b571866659 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4038,6 +4038,8 @@ static void monitor_qmp_bh_dispatcher(void *data)
>      }
>  }
>  
> +#define  QMP_REQ_QUEUE_LEN_MAX  (8)

Is this limit introspectable on QMP?

Fam

Re: [Qemu-devel] [RFC v6 18/27] monitor: send event when command queue full

[Peter Xu]

On Thu, Dec 21, 2017 at 07:42:46PM +0800, Fam Zheng wrote:
> On Tue, 12/19 16:45, Peter Xu wrote:
> > Set maximum QMP command queue length to 8.  If queue full, instead of
> > queue the command, we directly return a "command-dropped" event, telling
> > client that specific command is dropped.
> > 
> > Note that this flow control mechanism is only valid if OOB is enabled.
> > If it's not, the effective queue length will always be 1, which strictly
> > follows original behavior of QMP command handling (which never drop
> > messages).
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  monitor.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index ed9a741d06..b571866659 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4038,6 +4038,8 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >      }
> >  }
> >  
> > +#define  QMP_REQ_QUEUE_LEN_MAX  (8)
> 
> Is this limit introspectable on QMP?

Not yet.  IMHO it's really QMP internal stuff, and I see no benefit so
far for a client to know about this...

-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 18/27] monitor: send event when command queue full

[Fam Zheng]

On Mon, 12/25 13:18, Peter Xu wrote:
> On Thu, Dec 21, 2017 at 07:42:46PM +0800, Fam Zheng wrote:
> > On Tue, 12/19 16:45, Peter Xu wrote:
> > > Set maximum QMP command queue length to 8.  If queue full, instead of
> > > queue the command, we directly return a "command-dropped" event, telling
> > > client that specific command is dropped.
> > > 
> > > Note that this flow control mechanism is only valid if OOB is enabled.
> > > If it's not, the effective queue length will always be 1, which strictly
> > > follows original behavior of QMP command handling (which never drop
> > > messages).
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  monitor.c | 17 ++++++++++++++++-
> > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/monitor.c b/monitor.c
> > > index ed9a741d06..b571866659 100644
> > > --- a/monitor.c
> > > +++ b/monitor.c
> > > @@ -4038,6 +4038,8 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > >      }
> > >  }
> > >  
> > > +#define  QMP_REQ_QUEUE_LEN_MAX  (8)
> > 
> > Is this limit introspectable on QMP?
> 
> Not yet.  IMHO it's really QMP internal stuff, and I see no benefit so
> far for a client to know about this...

A client may need this number to batch (non-oob) commands without worrying about
getting command-dropped event.

Fam

Re: [Qemu-devel] [RFC v6 18/27] monitor: send event when command queue full

[Peter Xu]

On Mon, Dec 25, 2017 at 01:55:56PM +0800, Fam Zheng wrote:
> On Mon, 12/25 13:18, Peter Xu wrote:
> > On Thu, Dec 21, 2017 at 07:42:46PM +0800, Fam Zheng wrote:
> > > On Tue, 12/19 16:45, Peter Xu wrote:
> > > > Set maximum QMP command queue length to 8.  If queue full, instead of
> > > > queue the command, we directly return a "command-dropped" event, telling
> > > > client that specific command is dropped.
> > > > 
> > > > Note that this flow control mechanism is only valid if OOB is enabled.
> > > > If it's not, the effective queue length will always be 1, which strictly
> > > > follows original behavior of QMP command handling (which never drop
> > > > messages).
> > > > 
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >  monitor.c | 17 ++++++++++++++++-
> > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/monitor.c b/monitor.c
> > > > index ed9a741d06..b571866659 100644
> > > > --- a/monitor.c
> > > > +++ b/monitor.c
> > > > @@ -4038,6 +4038,8 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > > >      }
> > > >  }
> > > >  
> > > > +#define  QMP_REQ_QUEUE_LEN_MAX  (8)
> > > 
> > > Is this limit introspectable on QMP?
> > 
> > Not yet.  IMHO it's really QMP internal stuff, and I see no benefit so
> > far for a client to know about this...
> 
> A client may need this number to batch (non-oob) commands without worrying about
> getting command-dropped event.

IMHO QMP batching will only be useful when performance matters, but
for now IMHO we don't need to worry much about QMP performance?  When
we do, I suspect we need to do more things to make sure of it, and
exposing this single parameter may not really help much, say, for now
even the client batches the stuff, they still need to wait for the
completion of commands.

A rule of mine for this (which may not be exactly correct, but that's
what I'm following) is that we only export things that we are very
sure to export.  For the rest, I would prefer to hide them, so that we
will make ourselves easier for compatibilities.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 18/27] monitor: send event when command queue full

[Fam Zheng]

On Mon, 12/25 14:18, Peter Xu wrote:
> On Mon, Dec 25, 2017 at 01:55:56PM +0800, Fam Zheng wrote:
> > On Mon, 12/25 13:18, Peter Xu wrote:
> > > On Thu, Dec 21, 2017 at 07:42:46PM +0800, Fam Zheng wrote:
> > > > On Tue, 12/19 16:45, Peter Xu wrote:
> > > > > Set maximum QMP command queue length to 8.  If queue full, instead of
> > > > > queue the command, we directly return a "command-dropped" event, telling
> > > > > client that specific command is dropped.
> > > > > 
> > > > > Note that this flow control mechanism is only valid if OOB is enabled.
> > > > > If it's not, the effective queue length will always be 1, which strictly
> > > > > follows original behavior of QMP command handling (which never drop
> > > > > messages).
> > > > > 
> > > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > > ---
> > > > >  monitor.c | 17 ++++++++++++++++-
> > > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/monitor.c b/monitor.c
> > > > > index ed9a741d06..b571866659 100644
> > > > > --- a/monitor.c
> > > > > +++ b/monitor.c
> > > > > @@ -4038,6 +4038,8 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > > > >      }
> > > > >  }
> > > > >  
> > > > > +#define  QMP_REQ_QUEUE_LEN_MAX  (8)
> > > > 
> > > > Is this limit introspectable on QMP?
> > > 
> > > Not yet.  IMHO it's really QMP internal stuff, and I see no benefit so
> > > far for a client to know about this...
> > 
> > A client may need this number to batch (non-oob) commands without worrying about
> > getting command-dropped event.
> 
> IMHO QMP batching will only be useful when performance matters, but
> for now IMHO we don't need to worry much about QMP performance?  When
> we do, I suspect we need to do more things to make sure of it, and
> exposing this single parameter may not really help much, say, for now
> even the client batches the stuff, they still need to wait for the
> completion of commands.

I think when we declare that commands can be dropped when the queue is full, we
should be clear about in what condition a queue is full, and don't make users
guess. If we want is this incompleteness, and I'm the only one who doesn't like
it, that may be fine. It's just that, like you said, this seems a bit useless.

Fam

Re: [Qemu-devel] [RFC v6 18/27] monitor: send event when command queue full

[Peter Xu]

On Mon, Dec 25, 2017 at 03:13:49PM +0800, Fam Zheng wrote:
> On Mon, 12/25 14:18, Peter Xu wrote:
> > On Mon, Dec 25, 2017 at 01:55:56PM +0800, Fam Zheng wrote:
> > > On Mon, 12/25 13:18, Peter Xu wrote:
> > > > On Thu, Dec 21, 2017 at 07:42:46PM +0800, Fam Zheng wrote:
> > > > > On Tue, 12/19 16:45, Peter Xu wrote:
> > > > > > Set maximum QMP command queue length to 8.  If queue full, instead of
> > > > > > queue the command, we directly return a "command-dropped" event, telling
> > > > > > client that specific command is dropped.
> > > > > > 
> > > > > > Note that this flow control mechanism is only valid if OOB is enabled.
> > > > > > If it's not, the effective queue length will always be 1, which strictly
> > > > > > follows original behavior of QMP command handling (which never drop
> > > > > > messages).
> > > > > > 
> > > > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > > > ---
> > > > > >  monitor.c | 17 ++++++++++++++++-
> > > > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/monitor.c b/monitor.c
> > > > > > index ed9a741d06..b571866659 100644
> > > > > > --- a/monitor.c
> > > > > > +++ b/monitor.c
> > > > > > @@ -4038,6 +4038,8 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > > > > >      }
> > > > > >  }
> > > > > >  
> > > > > > +#define  QMP_REQ_QUEUE_LEN_MAX  (8)
> > > > > 
> > > > > Is this limit introspectable on QMP?
> > > > 
> > > > Not yet.  IMHO it's really QMP internal stuff, and I see no benefit so
> > > > far for a client to know about this...
> > > 
> > > A client may need this number to batch (non-oob) commands without worrying about
> > > getting command-dropped event.
> > 
> > IMHO QMP batching will only be useful when performance matters, but
> > for now IMHO we don't need to worry much about QMP performance?  When
> > we do, I suspect we need to do more things to make sure of it, and
> > exposing this single parameter may not really help much, say, for now
> > even the client batches the stuff, they still need to wait for the
> > completion of commands.
> 
> I think when we declare that commands can be dropped when the queue is full, we
> should be clear about in what condition a queue is full, and don't make users
> guess. If we want is this incompleteness, and I'm the only one who doesn't like
> it, that may be fine. It's just that, like you said, this seems a bit useless.

Just to clarify that this maximum queue length is still useful to us
(not the clients) in the sense that we'll have limited memory usage
for QMP command queues.  Otherwise we have risk on using up all the
memory of the host or reach the limitation of existing QEMU process,
either of which is bad.

-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 18/27] monitor: send event when command queue full

[Stefan Hajnoczi]

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

On Mon, Dec 25, 2017 at 03:22:24PM +0800, Peter Xu wrote:
> On Mon, Dec 25, 2017 at 03:13:49PM +0800, Fam Zheng wrote:
> > On Mon, 12/25 14:18, Peter Xu wrote:
> > > On Mon, Dec 25, 2017 at 01:55:56PM +0800, Fam Zheng wrote:
> > > > On Mon, 12/25 13:18, Peter Xu wrote:
> > > > > On Thu, Dec 21, 2017 at 07:42:46PM +0800, Fam Zheng wrote:
> > > > > > On Tue, 12/19 16:45, Peter Xu wrote:
> > > > > > > Set maximum QMP command queue length to 8.  If queue full, instead of
> > > > > > > queue the command, we directly return a "command-dropped" event, telling
> > > > > > > client that specific command is dropped.
> > > > > > > 
> > > > > > > Note that this flow control mechanism is only valid if OOB is enabled.
> > > > > > > If it's not, the effective queue length will always be 1, which strictly
> > > > > > > follows original behavior of QMP command handling (which never drop
> > > > > > > messages).
> > > > > > > 
> > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > > > > ---
> > > > > > >  monitor.c | 17 ++++++++++++++++-
> > > > > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/monitor.c b/monitor.c
> > > > > > > index ed9a741d06..b571866659 100644
> > > > > > > --- a/monitor.c
> > > > > > > +++ b/monitor.c
> > > > > > > @@ -4038,6 +4038,8 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > > > > > >      }
> > > > > > >  }
> > > > > > >  
> > > > > > > +#define  QMP_REQ_QUEUE_LEN_MAX  (8)
> > > > > > 
> > > > > > Is this limit introspectable on QMP?
> > > > > 
> > > > > Not yet.  IMHO it's really QMP internal stuff, and I see no benefit so
> > > > > far for a client to know about this...
> > > > 
> > > > A client may need this number to batch (non-oob) commands without worrying about
> > > > getting command-dropped event.
> > > 
> > > IMHO QMP batching will only be useful when performance matters, but
> > > for now IMHO we don't need to worry much about QMP performance?  When
> > > we do, I suspect we need to do more things to make sure of it, and
> > > exposing this single parameter may not really help much, say, for now
> > > even the client batches the stuff, they still need to wait for the
> > > completion of commands.
> > 
> > I think when we declare that commands can be dropped when the queue is full, we
> > should be clear about in what condition a queue is full, and don't make users
> > guess. If we want is this incompleteness, and I'm the only one who doesn't like
> > it, that may be fine. It's just that, like you said, this seems a bit useless.
> 
> Just to clarify that this maximum queue length is still useful to us
> (not the clients) in the sense that we'll have limited memory usage
> for QMP command queues.  Otherwise we have risk on using up all the
> memory of the host or reach the limitation of existing QEMU process,
> either of which is bad.

I think batching doesn't make much sense when "oob" is enabled.  There
are only very narrow use cases where it's reasonable to batch:

Batched commands must be independent of each other because any batched
command can be dropped:

  A B C

A can be dropped if there are already one or more commands pending.  If
this happens then B or C might run or be dropped too, depending on
timing.

Possible combinations are:

  <none>
  A
  A B
  A C
  A B C
  B
  B C
  C

The client has very little control and needs to implement retry logic if
commands are dropped.  I bet this is going to be very error prone for
client developers...

One reasonable use case is batching query-* commands.  I can't think of
many other use cases where it makes sense.

I'd be perfectly happy if QEMU did no batching at all when "oob" is
enabled (i.e. hardcoded 1 command lookahead).

Stefan

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

Re: [Qemu-devel] [RFC v6 18/27] monitor: send event when command queue full

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:48PM +0800, Peter Xu wrote:
> @@ -4071,6 +4073,9 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>      req_obj->req = req;
>      req_obj->need_resume = false;
>  
> +    /* Protect qmp_requests and fetching its length. */
> +    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> +
>      /*
>       * If OOB is not enabled on current monitor, we'll emulate the old
>       * behavior that we won't process current monitor any more until
> @@ -4080,6 +4085,17 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>      if (!qmp_oob_enabled(mon)) {
>          monitor_suspend(mon);
>          req_obj->need_resume = true;
> +    } else {
> +        /* Drop the request if queue is full. */
> +        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
> +            qapi_event_send_command_dropped(id,
> +                                            COMMAND_DROP_REASON_QUEUE_FULL,
> +                                            NULL);
> +            qobject_decref(id);
> +            qobject_decref(req);
> +            g_free(req_obj);
> +            return;

qmp_queue_lock is still locked!  I suggest releasing the lock before
calling qapi_event_send_command_dropped().

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

Re: [Qemu-devel] [RFC v6 18/27] monitor: send event when command queue full

[Peter Xu]

On Tue, Jan 09, 2018 at 01:42:08PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 19, 2017 at 04:45:48PM +0800, Peter Xu wrote:
> > @@ -4071,6 +4073,9 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> >      req_obj->req = req;
> >      req_obj->need_resume = false;
> >  
> > +    /* Protect qmp_requests and fetching its length. */
> > +    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> > +
> >      /*
> >       * If OOB is not enabled on current monitor, we'll emulate the old
> >       * behavior that we won't process current monitor any more until
> > @@ -4080,6 +4085,17 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> >      if (!qmp_oob_enabled(mon)) {
> >          monitor_suspend(mon);
> >          req_obj->need_resume = true;
> > +    } else {
> > +        /* Drop the request if queue is full. */
> > +        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
> > +            qapi_event_send_command_dropped(id,
> > +                                            COMMAND_DROP_REASON_QUEUE_FULL,
> > +                                            NULL);
> > +            qobject_decref(id);
> > +            qobject_decref(req);
> > +            g_free(req_obj);
> > +            return;
> 
> qmp_queue_lock is still locked!  I suggest releasing the lock before
> calling qapi_event_send_command_dropped().

Yes.  It took me some time to rebase and move that lock out, even if
so I still got one more bug...  Thanks for catching it.

And yes, I agree I should unlock before sending the event.

-- 
Peter Xu

[Qemu-devel] [RFC v6 19/27] qapi: introduce new cmd option "allow-oob"

[Peter Xu]

Here "oob" stands for "Out-Of-Band".  When "allow-oob" is set, it means
the command allows out-of-band execution.

The "oob" idea is proposed by Markus Armbruster in following thread:

  https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg02057.html

This new "allow-oob" boolean will be exposed by "query-qmp-schema" as
well for command entries, so that QMP clients can know which command can
be used as out-of-band calls. For example the command "migrate"
originally looks like:

  {"name": "migrate", "ret-type": "17", "meta-type": "command",
   "arg-type": "86"}

And it'll be changed into:

  {"name": "migrate", "ret-type": "17", "allow-oob": false,
   "meta-type": "command", "arg-type": "86"}

This patch only provides the QMP interface level changes.  It does not
contains the real out-of-band execution implementation yet.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qapi/qmp/dispatch.h    |  1 +
 qapi/introspect.json           |  6 +++++-
 scripts/qapi-commands.py       | 19 ++++++++++++++-----
 scripts/qapi-introspect.py     | 10 ++++++++--
 scripts/qapi.py                | 15 ++++++++++-----
 scripts/qapi2texi.py           |  2 +-
 tests/qapi-schema/test-qapi.py |  2 +-
 7 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 20578dcd48..b76798800c 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -23,6 +23,7 @@ typedef enum QmpCommandOptions
 {
     QCO_NO_OPTIONS = 0x0,
     QCO_NO_SUCCESS_RESP = 0x1,
+    QCO_ALLOW_OOB = 0x2,
 } QmpCommandOptions;
 
 typedef struct QmpCommand
diff --git a/qapi/introspect.json b/qapi/introspect.json
index 5b3e6e9d78..c7f67b7d78 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -259,12 +259,16 @@
 #
 # @ret-type: the name of the command's result type.
 #
+# @allow-oob: whether the command allows out-of-band execution.
+#             (Since: 2.12)
+#
 # TODO: @success-response (currently irrelevant, because it's QGA, not QMP)
 #
 # Since: 2.5
 ##
 { 'struct': 'SchemaInfoCommand',
-  'data': { 'arg-type': 'str', 'ret-type': 'str' } }
+  'data': { 'arg-type': 'str', 'ret-type': 'str',
+            'allow-oob': 'bool' } }
 
 ##
 # @SchemaInfoEvent:
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 974d0a4a80..b2b0bc0510 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -192,10 +192,18 @@ out:
     return ret
 
 
-def gen_register_command(name, success_response):
-    options = 'QCO_NO_OPTIONS'
+def gen_register_command(name, success_response, allow_oob):
+    options = []
+
     if not success_response:
-        options = 'QCO_NO_SUCCESS_RESP'
+        options += ['QCO_NO_SUCCESS_RESP']
+    if allow_oob:
+        options += ['QCO_ALLOW_OOB']
+
+    if not options:
+        options = ['QCO_NO_OPTIONS']
+
+    options = " | ".join(options)
 
     ret = mcgen('''
     qmp_register_command(cmds, "%(name)s",
@@ -241,7 +249,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
         self._visited_ret_types = None
 
     def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed):
+                      gen, success_response, boxed, allow_oob):
         if not gen:
             return
         self.decl += gen_command_decl(name, arg_type, boxed, ret_type)
@@ -250,7 +258,8 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
             self.defn += gen_marshal_output(ret_type)
         self.decl += gen_marshal_decl(name)
         self.defn += gen_marshal(name, arg_type, boxed, ret_type)
-        self._regy += gen_register_command(name, success_response)
+        self._regy += gen_register_command(name, success_response,
+                                           allow_oob)
 
 
 (input_file, output_dir, do_c, do_h, prefix, opts) = parse_command_line()
diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
index 032bcea491..9fbf88b644 100644
--- a/scripts/qapi-introspect.py
+++ b/scripts/qapi-introspect.py
@@ -28,6 +28,11 @@ def to_json(obj, level=0):
                               to_json(obj[key], level + 1))
                 for key in sorted(obj.keys())]
         ret = '{' + ', '.join(elts) + '}'
+    elif isinstance(obj, bool):
+        if obj:
+            ret = 'true'
+        else:
+            ret = 'false'
     else:
         assert False                # not implemented
     if level == 1:
@@ -154,12 +159,13 @@ const char %(c_name)s[] = %(c_string)s;
                                     for m in variants.variants]})
 
     def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed):
+                      gen, success_response, boxed, allow_oob):
         arg_type = arg_type or self._schema.the_empty_object_type
         ret_type = ret_type or self._schema.the_empty_object_type
         self._gen_json(name, 'command',
                        {'arg-type': self._use_type(arg_type),
-                        'ret-type': self._use_type(ret_type)})
+                        'ret-type': self._use_type(ret_type),
+                        'allow-oob': allow_oob})
 
     def visit_event(self, name, info, arg_type, boxed):
         arg_type = arg_type or self._schema.the_empty_object_type
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 62dc52ed6e..f411b8fc91 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -920,7 +920,8 @@ def check_exprs(exprs):
         elif 'command' in expr:
             meta = 'command'
             check_keys(expr_elem, 'command', [],
-                       ['data', 'returns', 'gen', 'success-response', 'boxed'])
+                       ['data', 'returns', 'gen', 'success-response',
+                        'boxed', 'allow-oob'])
         elif 'event' in expr:
             meta = 'event'
             check_keys(expr_elem, 'event', [], ['data', 'boxed'])
@@ -1031,7 +1032,7 @@ class QAPISchemaVisitor(object):
         pass
 
     def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed):
+                      gen, success_response, boxed, allow_oob):
         pass
 
     def visit_event(self, name, info, arg_type, boxed):
@@ -1398,7 +1399,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
 
 class QAPISchemaCommand(QAPISchemaEntity):
     def __init__(self, name, info, doc, arg_type, ret_type,
-                 gen, success_response, boxed):
+                 gen, success_response, boxed, allow_oob):
         QAPISchemaEntity.__init__(self, name, info, doc)
         assert not arg_type or isinstance(arg_type, str)
         assert not ret_type or isinstance(ret_type, str)
@@ -1409,6 +1410,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
         self.gen = gen
         self.success_response = success_response
         self.boxed = boxed
+        self.allow_oob = allow_oob
 
     def check(self, schema):
         if self._arg_type_name:
@@ -1432,7 +1434,8 @@ class QAPISchemaCommand(QAPISchemaEntity):
     def visit(self, visitor):
         visitor.visit_command(self.name, self.info,
                               self.arg_type, self.ret_type,
-                              self.gen, self.success_response, self.boxed)
+                              self.gen, self.success_response,
+                              self.boxed, self.allow_oob)
 
 
 class QAPISchemaEvent(QAPISchemaEntity):
@@ -1640,6 +1643,7 @@ class QAPISchema(object):
         gen = expr.get('gen', True)
         success_response = expr.get('success-response', True)
         boxed = expr.get('boxed', False)
+        allow_oob = expr.get('allow-oob', False)
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
                 name, info, doc, 'arg', self._make_members(data, info))
@@ -1647,7 +1651,8 @@ class QAPISchema(object):
             assert len(rets) == 1
             rets = self._make_array_type(rets[0], info)
         self._def_entity(QAPISchemaCommand(name, info, doc, data, rets,
-                                           gen, success_response, boxed))
+                                           gen, success_response,
+                                           boxed, allow_oob))
 
     def _def_event(self, expr, info, doc):
         name = expr['event']
diff --git a/scripts/qapi2texi.py b/scripts/qapi2texi.py
index a317526e51..0ac0df517a 100755
--- a/scripts/qapi2texi.py
+++ b/scripts/qapi2texi.py
@@ -236,7 +236,7 @@ class QAPISchemaGenDocVisitor(qapi.QAPISchemaVisitor):
                              body=texi_entity(doc, 'Members'))
 
     def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed):
+                      gen, success_response, boxed, allow_oob):
         doc = self.cur_doc
         if self.out:
             self.out += '\n'
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index c7724d3437..6749e9e397 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -36,7 +36,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
         self._print_variants(variants)
 
     def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed):
+                      gen, success_response, boxed, allow_oob):
         print 'command %s %s -> %s' % \
             (name, arg_type and arg_type.name, ret_type and ret_type.name)
         print '   gen=%s success_response=%s boxed=%s' % \
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 19/27] qapi: introduce new cmd option "allow-oob"

[Fam Zheng]

On Tue, 12/19 16:45, Peter Xu wrote:
> Here "oob" stands for "Out-Of-Band".  When "allow-oob" is set, it means
> the command allows out-of-band execution.
> 
> The "oob" idea is proposed by Markus Armbruster in following thread:
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg02057.html
> 
> This new "allow-oob" boolean will be exposed by "query-qmp-schema" as
> well for command entries, so that QMP clients can know which command can
> be used as out-of-band calls. For example the command "migrate"
> originally looks like:
> 
>   {"name": "migrate", "ret-type": "17", "meta-type": "command",
>    "arg-type": "86"}
> 
> And it'll be changed into:
> 
>   {"name": "migrate", "ret-type": "17", "allow-oob": false,
>    "meta-type": "command", "arg-type": "86"}
> 
> This patch only provides the QMP interface level changes.  It does not
> contains the real out-of-band execution implementation yet.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

[Qemu-devel] [RFC v6 20/27] qmp: export qmp_dispatch_check_obj and allow "id"

[Peter Xu]

We need this for earlier check for OOB, before reaching
do_qmp_dispatch().  Meanwhile, a small touch-up to allow "id" field,
after all we allow that for sure in the spec (in any form).

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qapi/qmp/dispatch.h | 1 +
 qapi/qmp-dispatch.c         | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index b76798800c..cf2657b5c9 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -49,6 +49,7 @@ bool qmp_command_is_enabled(const QmpCommand *cmd);
 const char *qmp_command_name(const QmpCommand *cmd);
 bool qmp_has_success_response(const QmpCommand *cmd);
 QObject *qmp_build_error_object(Error *err);
+QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp);
 
 typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
 
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index b41fa174fe..b16f2f1d3f 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -20,7 +20,7 @@
 #include "qapi-types.h"
 #include "qapi/qmp/qerror.h"
 
-static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
+QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
 {
     const QDictEntry *ent;
     const char *arg_name;
@@ -52,6 +52,8 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
                            "QMP input member 'arguments' must be an object");
                 return NULL;
             }
+        } else if (!strcmp(arg_name, "id")) {
+            continue;
         } else {
             error_setg(errp, "QMP input member '%s' is unexpected",
                        arg_name);
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 20/27] qmp: export qmp_dispatch_check_obj and allow "id"

[Fam Zheng]

On Tue, 12/19 16:45, Peter Xu wrote:
> We need this for earlier check for OOB, before reaching
> do_qmp_dispatch().  Meanwhile, a small touch-up to allow "id" field,
> after all we allow that for sure in the spec (in any form).
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [RFC v6 20/27] qmp: export qmp_dispatch_check_obj and allow "id"

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:50PM +0800, Peter Xu wrote:
> We need this for earlier check for OOB, before reaching
> do_qmp_dispatch().  Meanwhile, a small touch-up to allow "id" field,
> after all we allow that for sure in the spec (in any form).

This change would be better as part of the patch that needs it.  I don't
understand the commit description and there is no context in this patch
showing how this change is used.

> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qapi/qmp/dispatch.h | 1 +
>  qapi/qmp-dispatch.c         | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index b76798800c..cf2657b5c9 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -49,6 +49,7 @@ bool qmp_command_is_enabled(const QmpCommand *cmd);
>  const char *qmp_command_name(const QmpCommand *cmd);
>  bool qmp_has_success_response(const QmpCommand *cmd);
>  QObject *qmp_build_error_object(Error *err);
> +QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp);
>  
>  typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
>  
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index b41fa174fe..b16f2f1d3f 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -20,7 +20,7 @@
>  #include "qapi-types.h"
>  #include "qapi/qmp/qerror.h"
>  
> -static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
> +QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
>  {
>      const QDictEntry *ent;
>      const char *arg_name;
> @@ -52,6 +52,8 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
>                             "QMP input member 'arguments' must be an object");
>                  return NULL;
>              }
> +        } else if (!strcmp(arg_name, "id")) {
> +            continue;
>          } else {
>              error_setg(errp, "QMP input member '%s' is unexpected",
>                         arg_name);
> -- 
> 2.14.3
> 

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

Re: [Qemu-devel] [RFC v6 20/27] qmp: export qmp_dispatch_check_obj and allow "id"

[Peter Xu]

On Tue, Jan 09, 2018 at 01:45:28PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 19, 2017 at 04:45:50PM +0800, Peter Xu wrote:
> > We need this for earlier check for OOB, before reaching
> > do_qmp_dispatch().  Meanwhile, a small touch-up to allow "id" field,
> > after all we allow that for sure in the spec (in any form).
> 
> This change would be better as part of the patch that needs it.  I don't
> understand the commit description and there is no context in this patch
> showing how this change is used.

Maybe I should mention it explicitly that "it'll be used in the
following up patch".

Logically I think this has nothing to do with OOB so I splitted it
out.  I would prefer it to be a separate patch (though I can enrich
the commit message).  Or, I'm also fine if you think squashing this
into next would be nicer.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 20/27] qmp: export qmp_dispatch_check_obj and allow "id"

[Stefan Hajnoczi]

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

On Fri, Jan 12, 2018 at 02:16:10PM +0800, Peter Xu wrote:
> On Tue, Jan 09, 2018 at 01:45:28PM +0000, Stefan Hajnoczi wrote:
> > On Tue, Dec 19, 2017 at 04:45:50PM +0800, Peter Xu wrote:
> > > We need this for earlier check for OOB, before reaching
> > > do_qmp_dispatch().  Meanwhile, a small touch-up to allow "id" field,
> > > after all we allow that for sure in the spec (in any form).
> > 
> > This change would be better as part of the patch that needs it.  I don't
> > understand the commit description and there is no context in this patch
> > showing how this change is used.
> 
> Maybe I should mention it explicitly that "it'll be used in the
> following up patch".
> 
> Logically I think this has nothing to do with OOB so I splitted it
> out.  I would prefer it to be a separate patch (though I can enrich
> the commit message).  Or, I'm also fine if you think squashing this
> into next would be nicer.  Thanks,

I didn't understand the patch in isolation.  Maybe rephrasing the commit
description would help.  My suggestion to squash it means there will be
more context to review this change and hopefully it will become
self-explanatory.

Stefan

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

Re: [Qemu-devel] [RFC v6 20/27] qmp: export qmp_dispatch_check_obj and allow "id"

[Peter Xu]

On Fri, Jan 12, 2018 at 02:20:36PM +0000, Stefan Hajnoczi wrote:
> On Fri, Jan 12, 2018 at 02:16:10PM +0800, Peter Xu wrote:
> > On Tue, Jan 09, 2018 at 01:45:28PM +0000, Stefan Hajnoczi wrote:
> > > On Tue, Dec 19, 2017 at 04:45:50PM +0800, Peter Xu wrote:
> > > > We need this for earlier check for OOB, before reaching
> > > > do_qmp_dispatch().  Meanwhile, a small touch-up to allow "id" field,
> > > > after all we allow that for sure in the spec (in any form).
> > > 
> > > This change would be better as part of the patch that needs it.  I don't
> > > understand the commit description and there is no context in this patch
> > > showing how this change is used.
> > 
> > Maybe I should mention it explicitly that "it'll be used in the
> > following up patch".
> > 
> > Logically I think this has nothing to do with OOB so I splitted it
> > out.  I would prefer it to be a separate patch (though I can enrich
> > the commit message).  Or, I'm also fine if you think squashing this
> > into next would be nicer.  Thanks,
> 
> I didn't understand the patch in isolation.  Maybe rephrasing the commit
> description would help.  My suggestion to squash it means there will be
> more context to review this change and hopefully it will become
> self-explanatory.

Let me squash it. :-) And, sorry for the confusion.  Thanks,

-- 
Peter Xu

[Qemu-devel] [RFC v6 21/27] qmp: support out-of-band (oob) execution

[Peter Xu]

Having "allow-oob" to true for a command does not mean that this command
will always be run in out-of-band mode.  The out-of-band quick path will
only be executed if we specify the extra "run-oob" flag when sending the
QMP request:

    { "execute":   "command-that-allows-oob",
      "arguments": { ... },
      "control":   { "run-oob": true } }

The "control" key is introduced to store this extra flag.  "control"
field is used to store arguments that are shared by all the commands,
rather than command specific arguments.  Let "run-oob" be the first.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qapi/qmp/dispatch.h |  1 +
 monitor.c                   | 83 ++++++++++++++++++++++++++++++++++++++++-----
 qapi/qmp-dispatch.c         | 28 +++++++++++++++
 trace-events                |  2 ++
 4 files changed, 105 insertions(+), 9 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index cf2657b5c9..d8d913b338 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -50,6 +50,7 @@ const char *qmp_command_name(const QmpCommand *cmd);
 bool qmp_has_success_response(const QmpCommand *cmd);
 QObject *qmp_build_error_object(Error *err);
 QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp);
+bool qmp_is_oob(QDict *dict);
 
 typedef void (*qmp_cmd_callback_fn)(QmpCommand *cmd, void *opaque);
 
diff --git a/monitor.c b/monitor.c
index b571866659..505db439d8 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1090,6 +1090,44 @@ static void qmp_caps_apply(Monitor *mon, QMPCapabilityList *list)
     }
 }
 
+/*
+ * Return true if check successful, or false otherwise.  When false is
+ * returned, detailed error will be in errp if provided.
+ */
+static bool qmp_cmd_oob_check(Monitor *mon, QDict *req, Error **errp)
+{
+    const char *command;
+    QmpCommand *cmd;
+
+    command = qdict_get_try_str(req, "execute");
+    if (!command) {
+        error_setg(errp, "Command field 'execute' missing");
+        return false;
+    }
+
+    cmd = qmp_find_command(mon->qmp.commands, command);
+    if (!cmd) {
+        error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
+                  "The command %s has not been found", command);
+        return false;
+    }
+
+    if (qmp_is_oob(req)) {
+        if (!qmp_oob_enabled(mon)) {
+            error_setg(errp, "Please enable Out-Of-Band first "
+                       "for the session during handshake");
+            return false;
+        }
+        if (!(cmd->options & QCO_ALLOW_OOB)) {
+            error_setg(errp, "The command %s does not support OOB",
+                       command);
+            return false;
+        }
+    }
+
+    return true;
+}
+
 void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
                           Error **errp)
 {
@@ -4032,6 +4070,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
     QMPRequest *req_obj = monitor_qmp_requests_pop_one();
 
     if (req_obj) {
+        trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id));
         monitor_qmp_dispatch_one(req_obj);
         /* Reschedule instead of looping so the main loop stays responsive */
         qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
@@ -4055,17 +4094,31 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
         error_setg(&err, QERR_JSON_PARSING);
     }
     if (err) {
-        monitor_qmp_respond(mon, NULL, err, NULL);
-        qobject_decref(req);
-        return;
+        goto err;
+    }
+
+    /* Check against the request in general layout */
+    qdict = qmp_dispatch_check_obj(req, &err);
+    if (!qdict) {
+        goto err;
     }
 
-    qdict = qobject_to_qdict(req);
-    if (qdict) {
-        id = qdict_get(qdict, "id");
-        qobject_incref(id);
-        qdict_del(qdict, "id");
-    } /* else will fail qmp_dispatch() */
+    /* Check against OOB specific */
+    if (!qmp_cmd_oob_check(mon, qdict, &err)) {
+        goto err;
+    }
+
+    id = qdict_get(qdict, "id");
+
+    /* When OOB is enabled, the "id" field is mandatory. */
+    if (qmp_oob_enabled(mon) && !id) {
+        error_setg(&err, "Out-Of-Band capability requires that "
+                   "every command contains an 'id' field.");
+        goto err;
+    }
+
+    qobject_incref(id);
+    qdict_del(qdict, "id");
 
     req_obj = g_new0(QMPRequest, 1);
     req_obj->mon = mon;
@@ -4073,6 +4126,13 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
     req_obj->req = req;
     req_obj->need_resume = false;
 
+    if (qmp_is_oob(qdict)) {
+        /* Out-Of-Band (OOB) requests are executed directly in parser. */
+        trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id));
+        monitor_qmp_dispatch_one(req_obj);
+        return;
+    }
+
     /* Protect qmp_requests and fetching its length. */
     qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
 
@@ -4108,6 +4168,11 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
 
     /* Kick the dispatcher routine */
     qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
+    return;
+
+err:
+    monitor_qmp_respond(mon, NULL, err, NULL);
+    qobject_decref(req);
 }
 
 static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index b16f2f1d3f..675223e0ec 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -54,6 +54,12 @@ QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
             }
         } else if (!strcmp(arg_name, "id")) {
             continue;
+        } else if (!strcmp(arg_name, "control")) {
+            if (qobject_type(arg_obj) != QTYPE_QDICT) {
+                error_setg(errp,
+                           "QMP input member 'control' must be a dict");
+                return NULL;
+            }
         } else {
             error_setg(errp, "QMP input member '%s' is unexpected",
                        arg_name);
@@ -124,6 +130,28 @@ QObject *qmp_build_error_object(Error *err)
                               error_get_pretty(err));
 }
 
+/*
+ * Detect whether a request should be run out-of-band, by quickly
+ * peeking at whether we have: { "control": { "run-oob": True } }. By
+ * default commands are run in-band.
+ */
+bool qmp_is_oob(QDict *dict)
+{
+    QBool *bool_obj;
+
+    dict = qdict_get_qdict(dict, "control");
+    if (!dict) {
+        return false;
+    }
+
+    bool_obj = qobject_to_qbool(qdict_get(dict, "run-oob"));
+    if (!bool_obj) {
+        return false;
+    }
+
+    return qbool_get_bool(bool_obj);
+}
+
 QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request)
 {
     Error *err = NULL;
diff --git a/trace-events b/trace-events
index 2646241a0f..1fb9ea7df2 100644
--- a/trace-events
+++ b/trace-events
@@ -48,6 +48,8 @@ monitor_protocol_event_queue(uint32_t event, void *qdict, uint64_t rate) "event=
 handle_hmp_command(void *mon, const char *cmdline) "mon %p cmdline: %s"
 handle_qmp_command(void *mon, const char *req) "mon %p req: %s"
 monitor_suspend(void *ptr, int cnt) "mon %p: %d"
+monitor_qmp_cmd_in_band(const char *id) "%s"
+monitor_qmp_cmd_out_of_band(const char *id) "%s"
 
 # dma-helpers.c
 dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d"
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 21/27] qmp: support out-of-band (oob) execution

[Fam Zheng]

On Tue, 12/19 16:45, Peter Xu wrote:
> Having "allow-oob" to true for a command does not mean that this command
> will always be run in out-of-band mode.  The out-of-band quick path will
> only be executed if we specify the extra "run-oob" flag when sending the
> QMP request:
> 
>     { "execute":   "command-that-allows-oob",
>       "arguments": { ... },
>       "control":   { "run-oob": true } }
> 
> The "control" key is introduced to store this extra flag.  "control"
> field is used to store arguments that are shared by all the commands,
> rather than command specific arguments.  Let "run-oob" be the first.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [RFC v6 21/27] qmp: support out-of-band (oob) execution

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:51PM +0800, Peter Xu wrote:
> diff --git a/monitor.c b/monitor.c
> index b571866659..505db439d8 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1090,6 +1090,44 @@ static void qmp_caps_apply(Monitor *mon, QMPCapabilityList *list)
>      }
>  }
>  
> +/*
> + * Return true if check successful, or false otherwise.  When false is
> + * returned, detailed error will be in errp if provided.
> + */
> +static bool qmp_cmd_oob_check(Monitor *mon, QDict *req, Error **errp)
> +{
> +    const char *command;
> +    QmpCommand *cmd;
> +
> +    command = qdict_get_try_str(req, "execute");
> +    if (!command) {
> +        error_setg(errp, "Command field 'execute' missing");
> +        return false;
> +    }
> +
> +    cmd = qmp_find_command(mon->qmp.commands, command);
> +    if (!cmd) {
> +        error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
> +                  "The command %s has not been found", command);
> +        return false;
> +    }
> +
> +    if (qmp_is_oob(req)) {
> +        if (!qmp_oob_enabled(mon)) {
> +            error_setg(errp, "Please enable Out-Of-Band first "
> +                       "for the session during handshake");

The qmp-spec.txt document calls it "capabilities negotiation".  I don't
see "handshake" mentioned.  Please use the terminology from the QMP spec
so it's clear what this error message means.

> +            return false;
> +        }
> +        if (!(cmd->options & QCO_ALLOW_OOB)) {
> +            error_setg(errp, "The command %s does not support OOB",
> +                       command);
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
>  void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
>                            Error **errp)
>  {
> @@ -4032,6 +4070,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
>      QMPRequest *req_obj = monitor_qmp_requests_pop_one();
>  
>      if (req_obj) {
> +        trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id));

Not all tracers on all platforms support NULL string pointers.  Please
use qobject_get_try_str(req_obj->id) ?: "".

>          monitor_qmp_dispatch_one(req_obj);
>          /* Reschedule instead of looping so the main loop stays responsive */
>          qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> @@ -4055,17 +4094,31 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>          error_setg(&err, QERR_JSON_PARSING);
>      }
>      if (err) {
> -        monitor_qmp_respond(mon, NULL, err, NULL);
> -        qobject_decref(req);
> -        return;
> +        goto err;
> +    }
> +
> +    /* Check against the request in general layout */
> +    qdict = qmp_dispatch_check_obj(req, &err);
> +    if (!qdict) {
> +        goto err;
>      }
>  
> -    qdict = qobject_to_qdict(req);
> -    if (qdict) {
> -        id = qdict_get(qdict, "id");
> -        qobject_incref(id);
> -        qdict_del(qdict, "id");
> -    } /* else will fail qmp_dispatch() */
> +    /* Check against OOB specific */
> +    if (!qmp_cmd_oob_check(mon, qdict, &err)) {
> +        goto err;
> +    }
> +
> +    id = qdict_get(qdict, "id");
> +
> +    /* When OOB is enabled, the "id" field is mandatory. */
> +    if (qmp_oob_enabled(mon) && !id) {
> +        error_setg(&err, "Out-Of-Band capability requires that "
> +                   "every command contains an 'id' field.");

error_setg() messages shouldn't end with a period ('.').

> +        goto err;
> +    }
> +
> +    qobject_incref(id);
> +    qdict_del(qdict, "id");
>  
>      req_obj = g_new0(QMPRequest, 1);
>      req_obj->mon = mon;
> @@ -4073,6 +4126,13 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>      req_obj->req = req;
>      req_obj->need_resume = false;
>  
> +    if (qmp_is_oob(qdict)) {
> +        /* Out-Of-Band (OOB) requests are executed directly in parser. */
> +        trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id));

Same NULL "%s" issue here.

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

Re: [Qemu-devel] [RFC v6 21/27] qmp: support out-of-band (oob) execution

[Peter Xu]

On Tue, Jan 09, 2018 at 02:08:10PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 19, 2017 at 04:45:51PM +0800, Peter Xu wrote:
> > diff --git a/monitor.c b/monitor.c
> > index b571866659..505db439d8 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -1090,6 +1090,44 @@ static void qmp_caps_apply(Monitor *mon, QMPCapabilityList *list)
> >      }
> >  }
> >  
> > +/*
> > + * Return true if check successful, or false otherwise.  When false is
> > + * returned, detailed error will be in errp if provided.
> > + */
> > +static bool qmp_cmd_oob_check(Monitor *mon, QDict *req, Error **errp)
> > +{
> > +    const char *command;
> > +    QmpCommand *cmd;
> > +
> > +    command = qdict_get_try_str(req, "execute");
> > +    if (!command) {
> > +        error_setg(errp, "Command field 'execute' missing");
> > +        return false;
> > +    }
> > +
> > +    cmd = qmp_find_command(mon->qmp.commands, command);
> > +    if (!cmd) {
> > +        error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
> > +                  "The command %s has not been found", command);
> > +        return false;
> > +    }
> > +
> > +    if (qmp_is_oob(req)) {
> > +        if (!qmp_oob_enabled(mon)) {
> > +            error_setg(errp, "Please enable Out-Of-Band first "
> > +                       "for the session during handshake");
> 
> The qmp-spec.txt document calls it "capabilities negotiation".  I don't
> see "handshake" mentioned.  Please use the terminology from the QMP spec
> so it's clear what this error message means.
> 
> > +            return false;
> > +        }
> > +        if (!(cmd->options & QCO_ALLOW_OOB)) {
> > +            error_setg(errp, "The command %s does not support OOB",
> > +                       command);
> > +            return false;
> > +        }
> > +    }
> > +
> > +    return true;
> > +}
> > +
> >  void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
> >                            Error **errp)
> >  {
> > @@ -4032,6 +4070,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >      QMPRequest *req_obj = monitor_qmp_requests_pop_one();
> >  
> >      if (req_obj) {
> > +        trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id));
> 
> Not all tracers on all platforms support NULL string pointers.  Please
> use qobject_get_try_str(req_obj->id) ?: "".
> 
> >          monitor_qmp_dispatch_one(req_obj);
> >          /* Reschedule instead of looping so the main loop stays responsive */
> >          qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> > @@ -4055,17 +4094,31 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> >          error_setg(&err, QERR_JSON_PARSING);
> >      }
> >      if (err) {
> > -        monitor_qmp_respond(mon, NULL, err, NULL);
> > -        qobject_decref(req);
> > -        return;
> > +        goto err;
> > +    }
> > +
> > +    /* Check against the request in general layout */
> > +    qdict = qmp_dispatch_check_obj(req, &err);
> > +    if (!qdict) {
> > +        goto err;
> >      }
> >  
> > -    qdict = qobject_to_qdict(req);
> > -    if (qdict) {
> > -        id = qdict_get(qdict, "id");
> > -        qobject_incref(id);
> > -        qdict_del(qdict, "id");
> > -    } /* else will fail qmp_dispatch() */
> > +    /* Check against OOB specific */
> > +    if (!qmp_cmd_oob_check(mon, qdict, &err)) {
> > +        goto err;
> > +    }
> > +
> > +    id = qdict_get(qdict, "id");
> > +
> > +    /* When OOB is enabled, the "id" field is mandatory. */
> > +    if (qmp_oob_enabled(mon) && !id) {
> > +        error_setg(&err, "Out-Of-Band capability requires that "
> > +                   "every command contains an 'id' field.");
> 
> error_setg() messages shouldn't end with a period ('.').
> 
> > +        goto err;
> > +    }
> > +
> > +    qobject_incref(id);
> > +    qdict_del(qdict, "id");
> >  
> >      req_obj = g_new0(QMPRequest, 1);
> >      req_obj->mon = mon;
> > @@ -4073,6 +4126,13 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> >      req_obj->req = req;
> >      req_obj->need_resume = false;
> >  
> > +    if (qmp_is_oob(qdict)) {
> > +        /* Out-Of-Band (OOB) requests are executed directly in parser. */
> > +        trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id));
> 
> Same NULL "%s" issue here.

Fixing them all.

Good to know the "?:" syntax...  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 21/27] qmp: support out-of-band (oob) execution

[Stefan Hajnoczi]

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

On Fri, Jan 12, 2018 at 02:23:51PM +0800, Peter Xu wrote:
> Good to know the "?:" syntax...  Thanks,

It's a gcc extension and not part of the C standard:

https://gcc.gnu.org/onlinedocs/gcc/Conditionals.html#Conditionals

However, QEMU already uses it in several places so we can take advantage
of it.

Stefan

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

[Qemu-devel] [RFC v6 22/27] qmp: isolate responses into io thread

[Peter Xu]

For those monitors who has enabled IO thread, we'll offload the
responding procedure into IO thread.  The main reason is that chardev is
not thread safe, and we need to do all the read/write IOs in the same
thread.  For use_io_thr=true monitors, that thread is the IO thread.

We do this isolation in similar pattern as what we have done to the
request queue: we first create one response queue for each monitor, then
instead of reply directly in main thread, we queue the responses and
kick the IO thread to do the rest of the job for us.

A funny thing after doing this is that, when the QMP clients send "quit"
to QEMU, it's possible that we close the IOThread even earlier than
replying to that "quit".  So another thing we need to do before cleaning
up the monitors is that we need to flush the response queue (we don't
need to do that for command queue; after all we are quitting) to make
sure replies for handled commands are always flushed back to clients.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 99 insertions(+), 1 deletion(-)

diff --git a/monitor.c b/monitor.c
index 505db439d8..0c6403bc65 100644
--- a/monitor.c
+++ b/monitor.c
@@ -174,6 +174,8 @@ typedef struct {
     QemuMutex qmp_queue_lock;
     /* Input queue that holds all the parsed QMP requests */
     GQueue *qmp_requests;
+    /* Output queue contains all the QMP responses in order */
+    GQueue *qmp_responses;
 } MonitorQMP;
 
 /*
@@ -222,6 +224,8 @@ static struct {
     IOThread *mon_iothread;
     /* Bottom half to dispatch the requests received from IO thread */
     QEMUBH *qmp_dispatcher_bh;
+    /* Bottom half to deliver the responses back to clients */
+    QEMUBH *qmp_respond_bh;
 } mon_global;
 
 /* QMP checker flags */
@@ -396,7 +400,8 @@ int monitor_fprintf(FILE *stream, const char *fmt, ...)
     return 0;
 }
 
-static void monitor_json_emitter(Monitor *mon, const QObject *data)
+static void monitor_json_emitter_raw(Monitor *mon,
+                                     QObject *data)
 {
     QString *json;
 
@@ -410,6 +415,71 @@ static void monitor_json_emitter(Monitor *mon, const QObject *data)
     QDECREF(json);
 }
 
+static void monitor_json_emitter(Monitor *mon, QObject *data)
+{
+    if (mon->use_io_thr) {
+        /*
+         * If using IO thread, we need to queue the item so that IO
+         * thread will do the rest for us.  Take refcount so that
+         * caller won't free the data (which will be finally freed in
+         * responder thread).
+         */
+        qobject_incref(data);
+        qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+        g_queue_push_tail(mon->qmp.qmp_responses, (void *)data);
+        qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+        qemu_bh_schedule(mon_global.qmp_respond_bh);
+    } else {
+        /*
+         * If not using monitor IO thread, then we are in main thread.
+         * Do the emission right away.
+         */
+        monitor_json_emitter_raw(mon, data);
+    }
+}
+
+struct QMPResponse {
+    Monitor *mon;
+    QObject *data;
+};
+typedef struct QMPResponse QMPResponse;
+
+/*
+ * Return one QMPResponse.  The response is only valid if
+ * response.data is not NULL.
+ */
+static QMPResponse monitor_qmp_response_pop_one(void)
+{
+    Monitor *mon;
+    QObject *data = NULL;
+
+    qemu_mutex_lock(&monitor_lock);
+    QTAILQ_FOREACH(mon, &mon_list, entry) {
+        qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+        data = g_queue_pop_head(mon->qmp.qmp_responses);
+        qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+        if (data) {
+            break;
+        }
+    }
+    qemu_mutex_unlock(&monitor_lock);
+    return (QMPResponse) { .mon = mon, .data = data };
+}
+
+static void monitor_qmp_bh_responder(void *opaque)
+{
+    QMPResponse response;
+
+    while (true) {
+        response = monitor_qmp_response_pop_one();
+        if (!response.data) {
+            break;
+        }
+        monitor_json_emitter_raw(response.mon, response.data);
+        qobject_decref(response.data);
+    }
+}
+
 static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
     /* Limit guest-triggerable events to 1 per second */
     [QAPI_EVENT_RTC_CHANGE]        = { 1000 * SCALE_MS },
@@ -596,6 +666,7 @@ static void monitor_data_init(Monitor *mon, bool skip_flush,
     mon->skip_flush = skip_flush;
     mon->use_io_thr = use_io_thr;
     mon->qmp.qmp_requests = g_queue_new();
+    mon->qmp.qmp_responses = g_queue_new();
 }
 
 static void monitor_data_destroy(Monitor *mon)
@@ -610,6 +681,7 @@ static void monitor_data_destroy(Monitor *mon)
     qemu_mutex_destroy(&mon->out_lock);
     qemu_mutex_destroy(&mon->qmp.qmp_queue_lock);
     g_queue_free(mon->qmp.qmp_requests);
+    g_queue_free(mon->qmp.qmp_responses);
 }
 
 char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
@@ -4368,6 +4440,11 @@ static GMainContext *monitor_io_context_get(void)
     return iothread_get_g_main_context(mon_global.mon_iothread);
 }
 
+static AioContext *monitor_aio_context_get(void)
+{
+    return iothread_get_aio_context(mon_global.mon_iothread);
+}
+
 static void monitor_iothread_init(void)
 {
     mon_global.mon_iothread = iothread_create("mon_iothread",
@@ -4381,6 +4458,15 @@ static void monitor_iothread_init(void)
     mon_global.qmp_dispatcher_bh = aio_bh_new(qemu_get_aio_context(),
                                               monitor_qmp_bh_dispatcher,
                                               NULL);
+
+    /*
+     * Unlike the dispatcher BH, this must be run on the monitor IO
+     * thread, so that monitors that are using IO thread will make
+     * sure read/write operations are all done on the IO thread.
+     */
+    mon_global.qmp_respond_bh = aio_bh_new(monitor_aio_context_get(),
+                                           monitor_qmp_bh_responder,
+                                           NULL);
 }
 
 void monitor_init_globals(void)
@@ -4495,9 +4581,19 @@ void monitor_cleanup(void)
      */
     iothread_stop(mon_global.mon_iothread);
 
+    /*
+     * After we have IOThread to send responses, it's possible that
+     * when we stop the IOThread there are still replies queued in the
+     * responder queue.  Flush all of them.  Note that even after this
+     * flush it's still possible that out buffer is not flushed.
+     * It'll be done in below monitor_flush() as the last resort.
+     */
+    monitor_qmp_bh_responder(NULL);
+
     qemu_mutex_lock(&monitor_lock);
     QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
         QTAILQ_REMOVE(&mon_list, mon, entry);
+        monitor_flush(mon);
         monitor_data_destroy(mon);
         g_free(mon);
     }
@@ -4506,6 +4602,8 @@ void monitor_cleanup(void)
     /* QEMUBHs needs to be deleted before destroying the IOThread. */
     qemu_bh_delete(mon_global.qmp_dispatcher_bh);
     mon_global.qmp_dispatcher_bh = NULL;
+    qemu_bh_delete(mon_global.qmp_respond_bh);
+    mon_global.qmp_respond_bh = NULL;
 
     iothread_destroy(mon_global.mon_iothread);
     mon_global.mon_iothread = NULL;
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 22/27] qmp: isolate responses into io thread

[Fam Zheng]

On Tue, 12/19 16:45, Peter Xu wrote:
> For those monitors who has enabled IO thread, we'll offload the

s/has/have/

> responding procedure into IO thread.  The main reason is that chardev is
> not thread safe, and we need to do all the read/write IOs in the same
> thread.  For use_io_thr=true monitors, that thread is the IO thread.
> 
> We do this isolation in similar pattern as what we have done to the
> request queue: we first create one response queue for each monitor, then
> instead of reply directly in main thread, we queue the responses and

s/reply/replying/

s/main thread/the main thread/

> kick the IO thread to do the rest of the job for us.
> 
> A funny thing after doing this is that, when the QMP clients send "quit"
> to QEMU, it's possible that we close the IOThread even earlier than
> replying to that "quit".  So another thing we need to do before cleaning
> up the monitors is that we need to flush the response queue (we don't
> need to do that for command queue; after all we are quitting) to make
> sure replies for handled commands are always flushed back to clients.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [RFC v6 22/27] qmp: isolate responses into io thread

[Peter Xu]

On Thu, Dec 21, 2017 at 08:57:02PM +0800, Fam Zheng wrote:
> On Tue, 12/19 16:45, Peter Xu wrote:
> > For those monitors who has enabled IO thread, we'll offload the
> 
> s/has/have/
> 
> > responding procedure into IO thread.  The main reason is that chardev is
> > not thread safe, and we need to do all the read/write IOs in the same
> > thread.  For use_io_thr=true monitors, that thread is the IO thread.
> > 
> > We do this isolation in similar pattern as what we have done to the
> > request queue: we first create one response queue for each monitor, then
> > instead of reply directly in main thread, we queue the responses and
> 
> s/reply/replying/
> 
> s/main thread/the main thread/

Fixed all.

> 
> > kick the IO thread to do the rest of the job for us.
> > 
> > A funny thing after doing this is that, when the QMP clients send "quit"
> > to QEMU, it's possible that we close the IOThread even earlier than
> > replying to that "quit".  So another thing we need to do before cleaning
> > up the monitors is that we need to flush the response queue (we don't
> > need to do that for command queue; after all we are quitting) to make
> > sure replies for handled commands are always flushed back to clients.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>

Thanks!

-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 22/27] qmp: isolate responses into io thread

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:52PM +0800, Peter Xu wrote:
> +static void monitor_qmp_bh_responder(void *opaque)
> +{
> +    QMPResponse response;
> +
> +    while (true) {
> +        response = monitor_qmp_response_pop_one();
> +        if (!response.data) {
> +            break;
> +        }
> +        monitor_json_emitter_raw(response.mon, response.data);

Have you audited all mon->out_lock users to ensure that guest memory is
never touched while the lock is held?

If guest memory is touched then the main loop could be blocked due to
postcopy and when the IOThread executes monitor_qmp_bh_responder() ->
monitor_json_emitter_raw() -> monitor_puts() it will also hang!

Please add a comment above the out_lock declaration letting users know
that they must not touch guest memory while holding the lock.

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

Re: [Qemu-devel] [RFC v6 22/27] qmp: isolate responses into io thread

[Peter Xu]

On Tue, Jan 09, 2018 at 02:24:35PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 19, 2017 at 04:45:52PM +0800, Peter Xu wrote:
> > +static void monitor_qmp_bh_responder(void *opaque)
> > +{
> > +    QMPResponse response;
> > +
> > +    while (true) {
> > +        response = monitor_qmp_response_pop_one();
> > +        if (!response.data) {
> > +            break;
> > +        }
> > +        monitor_json_emitter_raw(response.mon, response.data);
> 
> Have you audited all mon->out_lock users to ensure that guest memory is
> never touched while the lock is held?
> 
> If guest memory is touched then the main loop could be blocked due to
> postcopy and when the IOThread executes monitor_qmp_bh_responder() ->
> monitor_json_emitter_raw() -> monitor_puts() it will also hang!

Yes, I am mostly certain that it never touches guest memory.  IMHO
it's a pure lock for monitor's output buffer since day one of its
occurence in 6cff3e8594c.

An extreme case I can think of is when someone wants to dump some data
from guest memory in QMP (I suspect who will use it though...).  In
that case we will still be safe, since the guest memory access should
be happening in g_strdup_vprintf() rather than monitor_puts(), so even
if it happens we will hang at g_strdup_vprintf() without the lock taken.

> 
> Please add a comment above the out_lock declaration letting users know
> that they must not touch guest memory while holding the lock.

I'm not sure whether adding a comment like this for the lock would be
a bit strange, but I agree it should be better than nothing.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [RFC v6 22/27] qmp: isolate responses into io thread

[Stefan Hajnoczi]

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

On Fri, Jan 12, 2018 at 02:44:55PM +0800, Peter Xu wrote:
> On Tue, Jan 09, 2018 at 02:24:35PM +0000, Stefan Hajnoczi wrote:
> > On Tue, Dec 19, 2017 at 04:45:52PM +0800, Peter Xu wrote:
> > > +static void monitor_qmp_bh_responder(void *opaque)
> > > +{
> > > +    QMPResponse response;
> > > +
> > > +    while (true) {
> > > +        response = monitor_qmp_response_pop_one();
> > > +        if (!response.data) {
> > > +            break;
> > > +        }
> > > +        monitor_json_emitter_raw(response.mon, response.data);
> > 
> > Have you audited all mon->out_lock users to ensure that guest memory is
> > never touched while the lock is held?
> > 
> > If guest memory is touched then the main loop could be blocked due to
> > postcopy and when the IOThread executes monitor_qmp_bh_responder() ->
> > monitor_json_emitter_raw() -> monitor_puts() it will also hang!
> 
> Yes, I am mostly certain that it never touches guest memory.  IMHO
> it's a pure lock for monitor's output buffer since day one of its
> occurence in 6cff3e8594c.
> 
> An extreme case I can think of is when someone wants to dump some data
> from guest memory in QMP (I suspect who will use it though...).  In
> that case we will still be safe, since the guest memory access should
> be happening in g_strdup_vprintf() rather than monitor_puts(), so even
> if it happens we will hang at g_strdup_vprintf() without the lock taken.
> 
> > 
> > Please add a comment above the out_lock declaration letting users know
> > that they must not touch guest memory while holding the lock.
> 
> I'm not sure whether adding a comment like this for the lock would be
> a bit strange, but I agree it should be better than nothing.  Thanks,

This is a case where out-of-band code depends on a mutex that non-OOB
code is used.  According to the documentation added in this patch series
all critical sections must also follow the OOB code rules.

Adding this comment informs people working with the code to follow OOB
code rules.  It certainly is safer than leaving this assumption
undocumented.

Stefan

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

[Qemu-devel] [RFC v6 23/27] monitor: enable IO thread for (qmp & !mux) typed

[Peter Xu]

Start to use dedicate IO thread for QMP monitors that are not using
MUXed chardev.

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

diff --git a/monitor.c b/monitor.c
index 0c6403bc65..7183061c2b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -35,6 +35,7 @@
 #include "net/net.h"
 #include "net/slirp.h"
 #include "chardev/char-fe.h"
+#include "chardev/char-mux.h"
 #include "ui/qemu-spice.h"
 #include "sysemu/numa.h"
 #include "monitor/monitor.h"
@@ -4522,8 +4523,10 @@ void monitor_init(Chardev *chr, int flags)
 {
     Monitor *mon = g_malloc(sizeof(*mon));
     GMainContext *context;
+    /* Enable IOThread for QMPs that are not using MUX chardev backends. */
+    bool use_io_thr = (!CHARDEV_IS_MUX(chr)) && (flags & MONITOR_USE_CONTROL);
 
-    monitor_data_init(mon, false, false);
+    monitor_data_init(mon, false, use_io_thr);
 
     qemu_chr_fe_init(&mon->chr, chr, &error_abort);
     mon->flags = flags;
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 23/27] monitor: enable IO thread for (qmp & !mux) typed

[Fam Zheng]

On Tue, 12/19 16:45, Peter Xu wrote:
> Start to use dedicate IO thread for QMP monitors that are not using
> MUXed chardev.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 0c6403bc65..7183061c2b 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -35,6 +35,7 @@
>  #include "net/net.h"
>  #include "net/slirp.h"
>  #include "chardev/char-fe.h"
> +#include "chardev/char-mux.h"
>  #include "ui/qemu-spice.h"
>  #include "sysemu/numa.h"
>  #include "monitor/monitor.h"
> @@ -4522,8 +4523,10 @@ void monitor_init(Chardev *chr, int flags)
>  {
>      Monitor *mon = g_malloc(sizeof(*mon));
>      GMainContext *context;
> +    /* Enable IOThread for QMPs that are not using MUX chardev backends. */
> +    bool use_io_thr = (!CHARDEV_IS_MUX(chr)) && (flags & MONITOR_USE_CONTROL);
>  
> -    monitor_data_init(mon, false, false);
> +    monitor_data_init(mon, false, use_io_thr);
>  
>      qemu_chr_fe_init(&mon->chr, chr, &error_abort);
>      mon->flags = flags;
> -- 
> 2.14.3
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [RFC v6 23/27] monitor: enable IO thread for (qmp & !mux) typed

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:53PM +0800, Peter Xu wrote:
> Start to use dedicate IO thread for QMP monitors that are not using
> MUXed chardev.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  monitor.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

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

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

[Qemu-devel] [RFC v6 24/27] qmp: add command "x-oob-test"

[Peter Xu]

This command is only used to test OOB functionality.  It should not be
used for any other purposes.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 qapi-schema.json | 18 ++++++++++++++++++
 qmp.c            | 16 ++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index 447ab25572..a63cd42cdd 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3259,3 +3259,21 @@
 ##
 { 'event': 'COMMAND_DROPPED' ,
   'data': { 'id': 'any', 'reason': 'CommandDropReason' } }
+
+##
+# @x-oob-test:
+#
+# Test OOB functionality.  When send this command with lock=true,
+# it'll try to hang the dispatcher.  When send it with lock=false,
+# it'll try to notify the locked thread to continue.  Note: it should
+# only be used by QMP test program rather than anything else.
+#
+# Since: 2.12
+#
+# Example:
+#
+# { "execute": "x-oob-test",
+#   "arguments": { "lock": true } }
+##
+{ 'command': 'x-oob-test', 'data' : { 'lock': 'bool' },
+  'allow-oob': true }
diff --git a/qmp.c b/qmp.c
index e8c303116a..ca2968aad2 100644
--- a/qmp.c
+++ b/qmp.c
@@ -722,3 +722,19 @@ MemoryInfo *qmp_query_memory_size_summary(Error **errp)
 
     return mem_info;
 }
+
+static QemuSemaphore x_oob_test_sem;
+
+static void __attribute__((constructor)) x_oob_test_init(void)
+{
+    qemu_sem_init(&x_oob_test_sem, 0);
+}
+
+void qmp_x_oob_test(bool lock, Error **errp)
+{
+    if (lock) {
+        qemu_sem_wait(&x_oob_test_sem);
+    } else {
+        qemu_sem_post(&x_oob_test_sem);
+    }
+}
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 24/27] qmp: add command "x-oob-test"

[Fam Zheng]

On Tue, 12/19 16:45, Peter Xu wrote:
> This command is only used to test OOB functionality.  It should not be
> used for any other purposes.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  qapi-schema.json | 18 ++++++++++++++++++
>  qmp.c            | 16 ++++++++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 447ab25572..a63cd42cdd 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3259,3 +3259,21 @@
>  ##
>  { 'event': 'COMMAND_DROPPED' ,
>    'data': { 'id': 'any', 'reason': 'CommandDropReason' } }
> +
> +##
> +# @x-oob-test:
> +#
> +# Test OOB functionality.  When send this command with lock=true,
> +# it'll try to hang the dispatcher.  When send it with lock=false,
> +# it'll try to notify the locked thread to continue.  Note: it should
> +# only be used by QMP test program rather than anything else.
> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# { "execute": "x-oob-test",
> +#   "arguments": { "lock": true } }
> +##
> +{ 'command': 'x-oob-test', 'data' : { 'lock': 'bool' },
> +  'allow-oob': true }
> diff --git a/qmp.c b/qmp.c
> index e8c303116a..ca2968aad2 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -722,3 +722,19 @@ MemoryInfo *qmp_query_memory_size_summary(Error **errp)
>  
>      return mem_info;
>  }
> +
> +static QemuSemaphore x_oob_test_sem;
> +
> +static void __attribute__((constructor)) x_oob_test_init(void)
> +{
> +    qemu_sem_init(&x_oob_test_sem, 0);
> +}
> +
> +void qmp_x_oob_test(bool lock, Error **errp)
> +{
> +    if (lock) {
> +        qemu_sem_wait(&x_oob_test_sem);
> +    } else {
> +        qemu_sem_post(&x_oob_test_sem);
> +    }
> +}
> -- 
> 2.14.3
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

[Qemu-devel] [RFC v6 25/27] docs: update QMP documents for OOB commands

[Peter Xu]

Update both the developer and spec for the new QMP OOB (Out-Of-Band)
command.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 docs/devel/qapi-code-gen.txt | 70 ++++++++++++++++++++++++++++++++++++++++----
 docs/interop/qmp-spec.txt    | 30 ++++++++++++++++---
 2 files changed, 91 insertions(+), 9 deletions(-)

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index f04c63fe82..02cc1a4075 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -554,9 +554,12 @@ following example objects:
 
 === Commands ===
 
+--- General Command Layout ---
+
 Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
          '*returns': TYPE-NAME, '*boxed': true,
-         '*gen': false, '*success-response': false }
+         '*gen': false, '*success-response': false,
+         '*allow-oob': false }
 
 Commands are defined by using a dictionary containing several members,
 where three members are most common.  The 'command' member is a
@@ -636,6 +639,61 @@ possible, the command expression should include the optional key
 'success-response' with boolean value false.  So far, only QGA makes
 use of this member.
 
+A command can be declared to support Out-Of-Band (OOB) execution.  By
+default, commands do not support OOB.  To declare a command to support
+it, we need an extra 'allow-oob' field.  For example:
+
+ { 'command': 'migrate_recover',
+   'data': { 'uri': 'str' }, 'allow-oob': true }
+
+To execute a command in Out-Of-Band way, we need to specify the
+"control" field in the request, with "run-oob" set to true. Example:
+
+ => { "execute": "command-support-oob",
+      "arguments": { ... },
+      "control": { "run-oob": true } }
+ <= { "return": { } }
+
+Without it, even the commands that support out-of-band execution will
+still be run In-Band.
+
+Please read the "Out-Of-Band Command Execution" section below for more
+information on how OOB execution works.
+
+--- About Out-Of-Band (OOB) Command Execution ---
+
+Out-Of-Band does not mean a special kind of command. Instead, it's a
+special way to execute the command.  One normal command can be
+declared to support Out-Of-Band execution when 'allow-oob' field is
+set to true when defining the command.  With that, it can be run in an
+Out-Of-Band way if 'run-oob' is specified in 'control' field of
+command request.
+
+When we say normal QMP command executions, it means basically the
+following:
+
+- They are executed in order,
+- They run only in main thread of QEMU,
+- They have the BQL taken during execution.
+
+For OOB command executions, they differ in the following:
+
+- They can be executed before an existing command,
+- They run in a monitor dedicated thread,
+- They do not take the BQL during execution.
+
+To add OOB execution support for a command, we need to make sure the
+command handler satisfies at least the following:
+
+- It executes extremely fast,
+- It does not take any lock (or, it can take very small locks, but in
+  very predictable ways),
+- 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.
+
+If someone is unsure about whether a command handler can be run in OOB
+way, then it possibly means that it does not suite for OOB execution.
 
 === Events ===
 
@@ -739,10 +797,12 @@ references by name.
 QAPI schema definitions not reachable that way are omitted.
 
 The SchemaInfo for a command has meta-type "command", and variant
-members "arg-type" and "ret-type".  On the wire, the "arguments"
-member of a client's "execute" command must conform to the object type
-named by "arg-type".  The "return" member that the server passes in a
-success response conforms to the type named by "ret-type".
+members "arg-type", "ret-type" and "allow-oob".  On the wire, the
+"arguments" member of a client's "execute" command must conform to the
+object type named by "arg-type".  The "return" member that the server
+passes in a success response conforms to the type named by
+"ret-type".  When "allow-oob" is set, it means the command supports
+out-of-band execution.
 
 If the command takes no arguments, "arg-type" names an object type
 without members.  Likewise, if the command returns nothing, "ret-type"
diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
index f8b5356015..e20163c138 100644
--- a/docs/interop/qmp-spec.txt
+++ b/docs/interop/qmp-spec.txt
@@ -83,16 +83,27 @@ The greeting message format is:
 2.2.1 Capabilities
 ------------------
 
-As of the date this document was last revised, no server or client
-capability strings have been defined.
+Currently supported capabilities are:
 
+- "oob": it means the QMP server supports "Out-Of-Band" command
+  execution.  For more detail, please see "run-oob" parameter in
+  "Issuing Commands" section below.  Not all commands allow this "oob"
+  execution.  One can know whether one command supports "oob" by
+  "query-qmp-schema" command.
+
+QMP clients can get a list of supported QMP capabilities of the QMP
+server in the greeting message mentioned above.  By default, all the
+capabilities are off.  To enable a specific or multiple of QMP
+capabilities, QMP client needs to send "qmp_capabilities" command with
+extra parameter for the capabilities.
 
 2.3 Issuing Commands
 --------------------
 
 The format for command execution is:
 
-{ "execute": json-string, "arguments": json-object, "id": json-value }
+{ "execute": json-string, "arguments": json-object, "id": json-value,
+  "control": json-dict }
 
  Where,
 
@@ -102,10 +113,16 @@ The format for command execution is:
   required. Each command documents what contents will be considered
   valid when handling the json-argument
 - The "id" member is a transaction identification associated with the
-  command execution, it is optional and will be part of the response if
+  command execution.  It is required if OOB is enabled, and optional
+  if not.  The same "id" field will be part of the response if
   provided. The "id" member can be any json-value, although most
   clients merely use a json-number incremented for each successive
   command
+- The "control" member is optional, and currently only used for
+  "out-of-band" execution ("oob" as shortcut). The handling or
+  response of an "oob" command can overtake prior in-band commands.
+  To enable "oob" feature, just provide a control field with: {
+  "control": { "run-oob": true } }
 
 2.4 Commands Responses
 ----------------------
@@ -113,6 +130,11 @@ The format for command execution is:
 There are two possible responses which the Server will issue as the result
 of a command execution: success or error.
 
+As long as the commands were issued with a proper "id" field, then the
+same "id" field will be attached in the corresponding response message
+so that requests and responses can match.  Clients should drop all the
+responses that are with unknown "id" field.
+
 2.4.1 success
 -------------
 
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 25/27] docs: update QMP documents for OOB commands

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:55PM +0800, Peter Xu wrote:
> +To add OOB execution support for a command, we need to make sure the
> +command handler satisfies at least the following:

It would help to rephrase this in the imperative mood (telling the
reader what to do rather than describing what would need to be done):

"OOB command handlers must satisfy the following conditions:"

That way it's shorter and easier to read, and it communicates that these
conditions are absolutely necessary.

> +
> +- It executes extremely fast,
> +- It does not take any lock (or, it can take very small locks, but in
> +  very predictable ways),

"it can take very small locks, but in very predictable ways" does not
explain what is allowed and what isn't.

I suggest: "it can take very small locks if all critical regions also
follow the rules for OOB command handler code".

> +- 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.
> +
> +If someone is unsure about whether a command handler can be run in OOB
> +way, then it possibly means that it does not suite for OOB execution.

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

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

Re: [Qemu-devel] [RFC v6 25/27] docs: update QMP documents for OOB commands

[Peter Xu]

On Tue, Jan 09, 2018 at 02:52:25PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 19, 2017 at 04:45:55PM +0800, Peter Xu wrote:
> > +To add OOB execution support for a command, we need to make sure the
> > +command handler satisfies at least the following:
> 
> It would help to rephrase this in the imperative mood (telling the
> reader what to do rather than describing what would need to be done):
> 
> "OOB command handlers must satisfy the following conditions:"
> 
> That way it's shorter and easier to read, and it communicates that these
> conditions are absolutely necessary.
> 
> > +
> > +- It executes extremely fast,
> > +- It does not take any lock (or, it can take very small locks, but in
> > +  very predictable ways),
> 
> "it can take very small locks, but in very predictable ways" does not
> explain what is allowed and what isn't.
> 
> I suggest: "it can take very small locks if all critical regions also
> follow the rules for OOB command handler code".
> 
> > +- 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.
> > +
> > +If someone is unsure about whether a command handler can be run in OOB
> > +way, then it possibly means that it does not suite for OOB execution.
> 
> "If in doubt, do not implement OOB execution support."

All fixed.  Thanks,

-- 
Peter Xu

[Qemu-devel] [RFC v6 26/27] tests: qmp-test: verify command batching

[Peter Xu]

OOB introduced DROP event for flow control.  This should not affect old
QMP clients.  Add a command batching check to make sure of it.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/qmp-test.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 292c5f135a..6bc66b0a01 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -78,6 +78,7 @@ static void test_qmp_protocol(void)
     QList *capabilities;
     const QListEntry *entry;
     QString *qstr;
+    int i;
 
     global_qtest = qtest_init_without_qmp_handshake(common_args);
 
@@ -135,6 +136,27 @@ static void test_qmp_protocol(void)
     g_assert_cmpint(qdict_get_int(resp, "id"), ==, 2);
     QDECREF(resp);
 
+    /*
+     * Test command batching.  In current test OOB is not enabled, we
+     * should be able to run as many commands in batch as we like.
+     * Using 16 (>8, which is OOB queue length) to make sure OOB won't
+     * break existing clients.  Note: this test does not control the
+     * scheduling of QEMU's QMP command processing threads so it may
+     * not really trigger batching inside QEMU.  This is just a
+     * best-effort test.
+     */
+    for (i = 0; i < 16; i++) {
+        qmp_async("{ 'execute': 'query-version' }");
+    }
+    /* Verify the replies to make sure no command is dropped. */
+    for (i = 0; i < 16; i++) {
+        resp = qmp_receive();
+        /* It should never be dropped.  Each of them should be a reply. */
+        g_assert(qdict_haskey(resp, "return"));
+        g_assert(!qdict_haskey(resp, "event"));
+        QDECREF(resp);
+    }
+
     qtest_end();
 }
 
-- 
2.14.3

[Qemu-devel] [RFC v6 27/27] tests: qmp-test: add oob test

[Peter Xu]

Test the new OOB capability.  Here we used the new "x-oob-test" command.
Firstly, we send a lock=true and oob=false command to hang the main
thread.  Then send another lock=false and oob=true command (which will
be run inside parser this time) to free that hanged command.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/qmp-test.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 6bc66b0a01..6165911aca 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -160,6 +160,70 @@ static void test_qmp_protocol(void)
     qtest_end();
 }
 
+/* Tests for Out-Of-Band support. */
+static void test_qmp_oob(void)
+{
+    QDict *resp;
+    int acks = 0;
+    const char *cmd_id;
+
+    global_qtest = qtest_init_without_qmp_handshake(common_args);
+
+    /* Ignore the greeting message. */
+    resp = qmp_receive();
+    g_assert(qdict_get_qdict(resp, "QMP"));
+    QDECREF(resp);
+
+    /* Try a fake capability, it should fail. */
+    resp = qmp("{ 'execute': 'qmp_capabilities', "
+               "  'arguments': { 'enable': [ 'cap-does-not-exist' ] } }");
+    g_assert(qdict_haskey(resp, "error"));
+    QDECREF(resp);
+
+    /* Now, enable OOB in current QMP session, it should success. */
+    resp = qmp("{ 'execute': 'qmp_capabilities', "
+               "  'arguments': { 'enable': [ 'oob' ] } }");
+    g_assert(qdict_haskey(resp, "return"));
+    QDECREF(resp);
+
+    /*
+     * Try any command that does not support OOB but with OOB flag. We
+     * should get failure.
+     */
+    resp = qmp("{ 'execute': 'query-cpus',"
+               "  'control': { 'run-oob': true } }");
+    g_assert(qdict_haskey(resp, "error"));
+    QDECREF(resp);
+
+    /*
+     * Firstly send the "x-oob-test" command with lock=true and
+     * oob=false, it should hang the dispatcher and main thread;
+     * later, we send another lock=false with oob=true to continue
+     * that thread processing.  Finally we should receive replies from
+     * both commands.
+     */
+    qmp_async("{ 'execute': 'x-oob-test',"
+              "  'arguments': { 'lock': true }, "
+              "  'id': 'lock-cmd'}");
+    qmp_async("{ 'execute': 'x-oob-test', "
+              "  'arguments': { 'lock': false }, "
+              "  'control': { 'run-oob': true }, "
+              "  'id': 'unlock-cmd' }");
+
+    /* Ignore all events.  Wait for 2 acks */
+    while (acks < 2) {
+        resp = qmp_receive();
+        cmd_id = qdict_get_str(resp, "id");
+        if (!g_strcmp0(cmd_id, "lock-cmd") ||
+            !g_strcmp0(cmd_id, "unlock-cmd")) {
+            acks++;
+        }
+        QDECREF(resp);
+    }
+
+    qtest_end();
+}
+
 static int query_error_class(const char *cmd)
 {
     static struct {
@@ -338,6 +402,7 @@ int main(int argc, char *argv[])
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_func("qmp/protocol", test_qmp_protocol);
+    qtest_add_func("qmp/oob", test_qmp_oob);
     qmp_schema_init(&schema);
     add_query_tests(&schema);
 
-- 
2.14.3

Re: [Qemu-devel] [RFC v6 00/27] QMP: out-of-band (OOB) execution support

[Stefan Hajnoczi]

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

On Tue, Dec 19, 2017 at 04:45:30PM +0800, Peter Xu wrote:
> This v6 kept most of the high level design untouched, however there
> are quite a few of changes that try to make the series more robust and
> safe.  My sincere thank to the reviewers (especially Stefan!) on all
> review comments to previous one.  Let's see how this version goes.
> 
> v6:

I've finished my review of this revision and posted comments.

Stefan

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

Re: [Qemu-devel] [RFC v6 00/27] QMP: out-of-band (OOB) execution support

[Peter Xu]

On Tue, Jan 09, 2018 at 02:52:49PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 19, 2017 at 04:45:30PM +0800, Peter Xu wrote:
> > This v6 kept most of the high level design untouched, however there
> > are quite a few of changes that try to make the series more robust and
> > safe.  My sincere thank to the reviewers (especially Stefan!) on all
> > review comments to previous one.  Let's see how this version goes.
> > 
> > v6:
> 
> I've finished my review of this revision and posted comments.

Thanks Stefan (and all the reviewers)!  I'll revisit the series in one
or two days.

-- 
Peter Xu