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 | 51 +++++++++++++++++++++++++++++++++++++++-----
 docs/interop/qmp-spec.txt    | 49 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 89 insertions(+), 11 deletions(-)

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index f04c63fe82..8597fdb087 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -556,7 +556,8 @@ following example objects:
 
 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 +637,44 @@ possible, the command expression should include the optional key
 'success-response' with boolean value false.  So far, only QGA makes
 use of this member.
 
+Most of the QMP commands are handled sequentially in such a order:
+Firstly, the JSON Parser parses the command request into some internal
+message, delivers the message to QMP dispatchers. Secondly, the QMP
+dispatchers will handle the commands one by one in time order, respond
+when necessary.  For some commands that always complete "quickly" can
+instead be executed directly during parsing, at the QMP client's
+request.  This kind of commands that allow direct execution is called
+"out-of-band" ("oob" as shortcut) commands. The response can overtake
+prior in-band commands' responses.  By default, commands are always
+in-band.  We need to explicitly specify "allow-oob" to "True" to show
+that one command can be run out-of-band.
+
+One thing to mention for developers is that, although out-of-band
+execution of commands benefit from quick and asynchronous execution,
+it need to satisfy at least the following:
+
+(1) It is extremely quick and never blocks, so that its execution will
+    not block parsing routine of any other monitors.
+
+(2) It does not need BQL, since the parser can be run without BQL,
+    while the dispatcher is always with BQL held.
+
+If not, the command is not suitable to be allowed to run out-of-band,
+and it should set its "allow-oob" to "False".  Whether a command is
+allowed to run out-of-band can also be introspected using
+query-qmp-schema command.  Please see the section "Client JSON
+Protocol introspection" for more information.
+
+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 supports out-of-band execution will
+still be run in-band.
 
 === Events ===
 
@@ -739,10 +778,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..01a2df0c79 100644
--- a/docs/interop/qmp-spec.txt
+++ b/docs/interop/qmp-spec.txt
@@ -78,21 +78,44 @@ The greeting message format is:
 - The "capabilities" member specify the availability of features beyond the
   baseline specification; the order of elements in this array has no
   particular significance, so a client must search the entire array
-  when looking for a particular capability
+  when looking for a particular capability.
 
 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.
+
+  NOTE: Converting an existing QMP command to be OOB-capable can be
+  either very easy or extremely hard.  The most important thing is
+  that OOB-capable command should never be blocked for a long time.
+  Some bad examples: (1) doing IOs, especially when the backend can be
+  an NFS storage; or (2) accessing guest memories, which can be simply
+  blocked for a very long time when it triggers a page fault, which
+  may not be handled immediately.  It's still legal to take a mutex in
+  an OOB-capable command handler, however, we need to make sure that
+  all the other code paths that are protected by the same lock won't
+  be blocked very long as well, otherwise the OOB handler might be
+  blocked too when it tries to take the lock.
+
+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 +125,19 @@ 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 optionally, and currently only used for
+  "out-of-band" execution.  For some commands that always complete
+  "quickly" can be executed directly during parsing at the QMP
+  client's request.  This kind of commands that allow direct execution
+  is called "out-of-band" ("oob" as shortcut) commands. The response
+  of "oob" commands can overtake prior in-band commands' responses.
+  To enable "oob" feature, just provide a control field with:
+  { "control": { "run-oob": true } }
 
 2.4 Commands Responses
 ----------------------
@@ -113,6 +145,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

On Tue, Dec 05, 2017 at 01:51:58PM +0800, Peter Xu wrote:
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index f04c63fe82..8597fdb087 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -556,7 +556,8 @@ following example objects:
>  
>  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 +637,44 @@ possible, the command expression should include the optional key
>  'success-response' with boolean value false.  So far, only QGA makes
>  use of this member.
>  
> +Most of the QMP commands are handled sequentially in such a order:
> +Firstly, the JSON Parser parses the command request into some internal
> +message, delivers the message to QMP dispatchers. Secondly, the QMP
> +dispatchers will handle the commands one by one in time order, respond
> +when necessary.

The important points to cover about normal commands:
1. They execute in order
2. They run the main loop
3. They run under the BQL

The other stuff about parsing requests into internal messages,
dispatchers, etc is not relevant.  It's better not to include it in
documentation because it can change and could also confuse readers
(since they don't need this info).

> For some commands that always complete "quickly" can

I've mentioned before that "quickly" is misleading and not what oob
commands are about.  I suggest changing this to:

  Certain urgent commands can

I've made similar comments further down where I think the text focusses
on words like "quickly" or "asynchronous" too much.

> +instead be executed directly during parsing, at the QMP client's
> +request.  This kind of commands that allow direct execution is called
> +"out-of-band" ("oob" as shortcut) commands. The response can overtake
> +prior in-band commands' responses.  By default, commands are always
> +in-band.  We need to explicitly specify "allow-oob" to "True" to show

s/"True"/true/  (JSON is case-sensitive)

> +that one command can be run out-of-band.
> +
> +One thing to mention for developers is that, although out-of-band
> +execution of commands benefit from quick and asynchronous execution,
> +it need to satisfy at least the following:
> +
> +(1) It is extremely quick and never blocks, so that its execution will
> +    not block parsing routine of any other monitors.
> +
> +(2) It does not need BQL, since the parser can be run without BQL,
> +    while the dispatcher is always with BQL held.

These conditions are not sufficient for postcopy recovery.  I suggest
rephrasing this section as follows:

  An out-of-band command must:

  1. Complete extremely quickly
  2. Not take locks
  3. Not invoke blocking system calls
  4. Not access guest RAM or memory that blocks when userfaultfd is
     enabled for postcopy live migration

We could make #2 less strict by saying "Not take locks except for
spinlocks (or mutexes that could be spinlocks because the critical
regions are tiny) or indirectly via g_malloc()".

> Whether a command is
> +allowed to run out-of-band can also be introspected using
> +query-qmp-schema command.  Please see the section "Client JSON
> +Protocol introspection" for more information.

This is relevant to client authors, not QEMU developers learning about
qapi.  I suggest dropping it.

> +
> +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 supports out-of-band execution will
> +still be run in-band.

This is relevant to client authors, not QEMU developers learning about
qapi.  I suggest instead explaining how qmp functions need to test for
qmp_is_oob() so that they know which mode they are executing in.

>  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.
> +
> +  NOTE: Converting an existing QMP command to be OOB-capable can be
> +  either very easy or extremely hard.  The most important thing is
> +  that OOB-capable command should never be blocked for a long time.
> +  Some bad examples: (1) doing IOs, especially when the backend can be
> +  an NFS storage; or (2) accessing guest memories, which can be simply
> +  blocked for a very long time when it triggers a page fault, which
> +  may not be handled immediately.  It's still legal to take a mutex in
> +  an OOB-capable command handler, however, we need to make sure that
> +  all the other code paths that are protected by the same lock won't
> +  be blocked very long as well, otherwise the OOB handler might be
> +  blocked too when it tries to take the lock.

Please drop this paragraph, this is the qmp-spec.txt document so the
implementation of QEMU's QMP commands shouldn't be discussed here.

> For some commands that always complete
> +  "quickly" can be executed directly during parsing at the QMP
> +  client's request.

Please drop this sentence.  "quickly" isn't the important quality, it's
urgency.  Also the description of execution in the QMP parser isn't
relevant for qmp-spec.txt, what matters is that oob commands can execute
while a normal monitor command is still running (this is described next
so this whole sentence can be dropped).

On Thu, Dec 14, 2017 at 02:30:19PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 05, 2017 at 01:51:58PM +0800, Peter Xu wrote:
> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> > index f04c63fe82..8597fdb087 100644
> > --- a/docs/devel/qapi-code-gen.txt
> > +++ b/docs/devel/qapi-code-gen.txt
> > @@ -556,7 +556,8 @@ following example objects:
> >  
> >  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 +637,44 @@ possible, the command expression should include the optional key
> >  'success-response' with boolean value false.  So far, only QGA makes
> >  use of this member.
> >  
> > +Most of the QMP commands are handled sequentially in such a order:
> > +Firstly, the JSON Parser parses the command request into some internal
> > +message, delivers the message to QMP dispatchers. Secondly, the QMP
> > +dispatchers will handle the commands one by one in time order, respond
> > +when necessary.
> 
> The important points to cover about normal commands:
> 1. They execute in order
> 2. They run the main loop
> 3. They run under the BQL
> 
> The other stuff about parsing requests into internal messages,
> dispatchers, etc is not relevant.  It's better not to include it in
> documentation because it can change and could also confuse readers
> (since they don't need this info).

Ah yes.  I'm thinking whether I should just remove most of the changes
to this file (qapi-code-gen.txt) since most of them are really not
related to code gen at all...  Maybe somewhere in qmp-spec.txt as
well?

> 
> > For some commands that always complete "quickly" can
> 
> I've mentioned before that "quickly" is misleading and not what oob
> commands are about.  I suggest changing this to:
> 
>   Certain urgent commands can
> 
> I've made similar comments further down where I think the text focusses
> on words like "quickly" or "asynchronous" too much.

I'll be more careful on the wording in next version on this.

> 
> > +instead be executed directly during parsing, at the QMP client's
> > +request.  This kind of commands that allow direct execution is called
> > +"out-of-band" ("oob" as shortcut) commands. The response can overtake
> > +prior in-band commands' responses.  By default, commands are always
> > +in-band.  We need to explicitly specify "allow-oob" to "True" to show
> 
> s/"True"/true/  (JSON is case-sensitive)

Ok.

> 
> > +that one command can be run out-of-band.
> > +
> > +One thing to mention for developers is that, although out-of-band
> > +execution of commands benefit from quick and asynchronous execution,
> > +it need to satisfy at least the following:
> > +
> > +(1) It is extremely quick and never blocks, so that its execution will
> > +    not block parsing routine of any other monitors.
> > +
> > +(2) It does not need BQL, since the parser can be run without BQL,
> > +    while the dispatcher is always with BQL held.
> 
> These conditions are not sufficient for postcopy recovery.  I suggest
> rephrasing this section as follows:
> 
>   An out-of-band command must:
> 
>   1. Complete extremely quickly
>   2. Not take locks
>   3. Not invoke blocking system calls
>   4. Not access guest RAM or memory that blocks when userfaultfd is
>      enabled for postcopy live migration
> 
> We could make #2 less strict by saying "Not take locks except for
> spinlocks (or mutexes that could be spinlocks because the critical
> regions are tiny) or indirectly via g_malloc()".

Ok.  I'm thinking whether I should also move these lines out too.

> 
> > Whether a command is
> > +allowed to run out-of-band can also be introspected using
> > +query-qmp-schema command.  Please see the section "Client JSON
> > +Protocol introspection" for more information.
> 
> This is relevant to client authors, not QEMU developers learning about
> qapi.  I suggest dropping it.

Ok.

> 
> > +
> > +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 supports out-of-band execution will
> > +still be run in-band.
> 
> This is relevant to client authors, not QEMU developers learning about
> qapi.  I suggest instead explaining how qmp functions need to test for
> qmp_is_oob() so that they know which mode they are executing in.
> 
> >  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.
> > +
> > +  NOTE: Converting an existing QMP command to be OOB-capable can be
> > +  either very easy or extremely hard.  The most important thing is
> > +  that OOB-capable command should never be blocked for a long time.
> > +  Some bad examples: (1) doing IOs, especially when the backend can be
> > +  an NFS storage; or (2) accessing guest memories, which can be simply
> > +  blocked for a very long time when it triggers a page fault, which
> > +  may not be handled immediately.  It's still legal to take a mutex in
> > +  an OOB-capable command handler, however, we need to make sure that
> > +  all the other code paths that are protected by the same lock won't
> > +  be blocked very long as well, otherwise the OOB handler might be
> > +  blocked too when it tries to take the lock.
> 
> Please drop this paragraph, this is the qmp-spec.txt document so the
> implementation of QEMU's QMP commands shouldn't be discussed here.

Ok.

> 
> > For some commands that always complete
> > +  "quickly" can be executed directly during parsing at the QMP
> > +  client's request.
> 
> Please drop this sentence.  "quickly" isn't the important quality, it's
> urgency.  Also the description of execution in the QMP parser isn't
> relevant for qmp-spec.txt, what matters is that oob commands can execute
> while a normal monitor command is still running (this is described next
> so this whole sentence can be dropped).

Ok.

I'll try to re-arrange the sentences better in my next post.  Thanks,

-- 
Peter Xu

On Mon, Dec 18, 2017 at 05:44:19PM +0800, Peter Xu wrote:
> On Thu, Dec 14, 2017 at 02:30:19PM +0000, Stefan Hajnoczi wrote:
> > On Tue, Dec 05, 2017 at 01:51:58PM +0800, Peter Xu wrote:
> > > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> > > index f04c63fe82..8597fdb087 100644
> > > --- a/docs/devel/qapi-code-gen.txt
> > > +++ b/docs/devel/qapi-code-gen.txt
> > > @@ -556,7 +556,8 @@ following example objects:
> > >  
> > >  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 +637,44 @@ possible, the command expression should include the optional key
> > >  'success-response' with boolean value false.  So far, only QGA makes
> > >  use of this member.
> > >  
> > > +Most of the QMP commands are handled sequentially in such a order:
> > > +Firstly, the JSON Parser parses the command request into some internal
> > > +message, delivers the message to QMP dispatchers. Secondly, the QMP
> > > +dispatchers will handle the commands one by one in time order, respond
> > > +when necessary.
> > 
> > The important points to cover about normal commands:
> > 1. They execute in order
> > 2. They run the main loop
> > 3. They run under the BQL
> > 
> > The other stuff about parsing requests into internal messages,
> > dispatchers, etc is not relevant.  It's better not to include it in
> > documentation because it can change and could also confuse readers
> > (since they don't need this info).
> 
> Ah yes.  I'm thinking whether I should just remove most of the changes
> to this file (qapi-code-gen.txt) since most of them are really not
> related to code gen at all...  Maybe somewhere in qmp-spec.txt as
> well?

qmp-spec.txt is the wire protocol that QMP clients and servers follow.
QEMU details like the BQL do not belong in qmp-spec.txt.

qapi-code-gen.txt is not a bad place to document the QEMU details that
are relevant to people writing QMP commands with allow-oob: true.  What
I was getting at is that this document should focus on things that QMP
command authors need to know, not on monitor implementation details.

On Mon, Dec 18, 2017 at 02:09:36PM +0000, Stefan Hajnoczi wrote:
> On Mon, Dec 18, 2017 at 05:44:19PM +0800, Peter Xu wrote:
> > On Thu, Dec 14, 2017 at 02:30:19PM +0000, Stefan Hajnoczi wrote:
> > > On Tue, Dec 05, 2017 at 01:51:58PM +0800, Peter Xu wrote:
> > > > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> > > > index f04c63fe82..8597fdb087 100644
> > > > --- a/docs/devel/qapi-code-gen.txt
> > > > +++ b/docs/devel/qapi-code-gen.txt
> > > > @@ -556,7 +556,8 @@ following example objects:
> > > >  
> > > >  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 +637,44 @@ possible, the command expression should include the optional key
> > > >  'success-response' with boolean value false.  So far, only QGA makes
> > > >  use of this member.
> > > >  
> > > > +Most of the QMP commands are handled sequentially in such a order:
> > > > +Firstly, the JSON Parser parses the command request into some internal
> > > > +message, delivers the message to QMP dispatchers. Secondly, the QMP
> > > > +dispatchers will handle the commands one by one in time order, respond
> > > > +when necessary.
> > > 
> > > The important points to cover about normal commands:
> > > 1. They execute in order
> > > 2. They run the main loop
> > > 3. They run under the BQL
> > > 
> > > The other stuff about parsing requests into internal messages,
> > > dispatchers, etc is not relevant.  It's better not to include it in
> > > documentation because it can change and could also confuse readers
> > > (since they don't need this info).
> > 
> > Ah yes.  I'm thinking whether I should just remove most of the changes
> > to this file (qapi-code-gen.txt) since most of them are really not
> > related to code gen at all...  Maybe somewhere in qmp-spec.txt as
> > well?
> 
> qmp-spec.txt is the wire protocol that QMP clients and servers follow.
> QEMU details like the BQL do not belong in qmp-spec.txt.
> 
> qapi-code-gen.txt is not a bad place to document the QEMU details that
> are relevant to people writing QMP commands with allow-oob: true.  What
> I was getting at is that this document should focus on things that QMP
> command authors need to know, not on monitor implementation details.

Fair enough.  I'll try to avoid talking about QMP internals in either
of the documents.

Let me cook a better one.  Thanks,

-- 
Peter Xu