[PULL 0/9] QMP patches patches for 2021-02-04

[PULL 0/9] QMP patches patches for 2021-02-04

[Markus Armbruster]

The following changes since commit db754f8ccaf2f073c9aed46a4389e9c0c2080399:

  Merge remote-tracking branch 'remotes/rth-gitlab/tags/pull-tcg-20210202' into staging (2021-02-03 19:35:57 +0000)

are available in the Git repository at:

  git://repo.or.cz/qemu/armbru.git tags/pull-qmp-2021-02-04

for you to fetch changes up to 88daf0996cd0488e93e67bcb0af258f2c24f117a:

  qmp: Resume OOB-enabled monitor before processing the request (2021-02-04 13:20:29 +0100)

----------------------------------------------------------------
QMP patches patches for 2021-02-04

----------------------------------------------------------------
Markus Armbruster (4):
      docs/interop/qmp-spec: Document the request queue limit
      qmp: Fix up comments after commit 9ce44e2ce2
      qmp: Add more tracepoints
      qmp: Resume OOB-enabled monitor before processing the request

Peter Maydell (1):
      monitor/qmp-cmds.c: Don't include ui/vnc.h

Zhang Han (4):
      qobject: open brace '{' following struct go on the same line
      qobject: code indent should never use tabs
      qobject: spaces required around that operators
      qobject: braces {} are necessary for all arms of this statement

 docs/interop/qmp-spec.txt |  8 +++++---
 monitor/qmp-cmds.c        |  2 +-
 monitor/qmp.c             | 44 ++++++++++++++++++++++++++++++++++++--------
 qobject/json-parser.c     |  3 +--
 qobject/qdict.c           | 12 +++++++-----
 qobject/qjson.c           |  3 +--
 monitor/trace-events      |  4 ++++
 7 files changed, 55 insertions(+), 21 deletions(-)

-- 
2.26.2

[PULL 1/9] monitor/qmp-cmds.c: Don't include ui/vnc.h

[Markus Armbruster]

From: Peter Maydell <peter.maydell@linaro.org>

The qmp-cmds.c file currently includes ui/vnc.h, which (being located
in the ui/ directory rather than include) is really supposed to be
for use only by the ui subsystem.  In fact the function prototypes we
need (vnc_display_password(), etc) are all declared in
include/ui/console.h, so we can switch to including that instead.

(ui/vnc.h includes include/ui/console.h, so this change strictly
reduces the quantity of headers qmp-cmds.c pulls in.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20210104161200.15068-1-peter.maydell@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor/qmp-cmds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 990936136c..c7df8c0ee2 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -23,7 +23,7 @@
 #include "qemu/uuid.h"
 #include "chardev/char.h"
 #include "ui/qemu-spice.h"
-#include "ui/vnc.h"
+#include "ui/console.h"
 #include "sysemu/kvm.h"
 #include "sysemu/runstate.h"
 #include "sysemu/runstate-action.h"
-- 
2.26.2

[PULL 2/9] qobject: open brace '{' following struct go on the same line

[Markus Armbruster]

From: Zhang Han <zhanghan64@huawei.com>

Put open brace '{' on the same line of struct.

Signed-off-by: Zhang Han <zhanghan64@huawei.com>
Message-Id: <20201228071129.24563-2-zhanghan64@huawei.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qobject/json-parser.c | 3 +--
 qobject/qjson.c       | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/qobject/json-parser.c b/qobject/json-parser.c
index d351039b10..008b326fb8 100644
--- a/qobject/json-parser.c
+++ b/qobject/json-parser.c
@@ -31,8 +31,7 @@ struct JSONToken {
     char str[];
 };
 
-typedef struct JSONParserContext
-{
+typedef struct JSONParserContext {
     Error *err;
     JSONToken *current;
     GQueue *buf;
diff --git a/qobject/qjson.c b/qobject/qjson.c
index bcc376e626..167fcb429c 100644
--- a/qobject/qjson.c
+++ b/qobject/qjson.c
@@ -22,8 +22,7 @@
 #include "qapi/qmp/qnum.h"
 #include "qapi/qmp/qstring.h"
 
-typedef struct JSONParsingState
-{
+typedef struct JSONParsingState {
     JSONMessageParser parser;
     QObject *result;
     Error *err;
-- 
2.26.2

[PULL 3/9] qobject: code indent should never use tabs

[Markus Armbruster]

From: Zhang Han <zhanghan64@huawei.com>

Transfer tabs to spaces.

Signed-off-by: Zhang Han <zhanghan64@huawei.com>
Message-Id: <20201228071129.24563-3-zhanghan64@huawei.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qobject/qdict.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qobject/qdict.c b/qobject/qdict.c
index d84443391e..d4d016ad69 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -39,8 +39,8 @@ QDict *qdict_new(void)
  */
 static unsigned int tdb_hash(const char *name)
 {
-    unsigned value;	/* Used to compute the hash value.  */
-    unsigned   i;	/* Used to cycle through random values. */
+    unsigned value;    /* Used to compute the hash value.  */
+    unsigned   i;      /* Used to cycle through random values. */
 
     /* Set the initial value from the key size. */
     for (value = 0x238F13AF * strlen(name), i=0; name[i]; i++)
-- 
2.26.2

[PULL 4/9] qobject: spaces required around that operators

[Markus Armbruster]

From: Zhang Han <zhanghan64@huawei.com>

Add spaces around operators.

Signed-off-by: Zhang Han <zhanghan64@huawei.com>
Message-Id: <20201228071129.24563-4-zhanghan64@huawei.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qobject/qdict.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qobject/qdict.c b/qobject/qdict.c
index d4d016ad69..6c15ed14c1 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -43,8 +43,8 @@ static unsigned int tdb_hash(const char *name)
     unsigned   i;      /* Used to cycle through random values. */
 
     /* Set the initial value from the key size. */
-    for (value = 0x238F13AF * strlen(name), i=0; name[i]; i++)
-        value = (value + (((const unsigned char *)name)[i] << (i*5 % 24)));
+    for (value = 0x238F13AF * strlen(name), i = 0; name[i]; i++)
+        value = (value + (((const unsigned char *)name)[i] << (i * 5 % 24)));
 
     return (1103515243 * value + 12345);
 }
-- 
2.26.2

[PULL 5/9] qobject: braces {} are necessary for all arms of this statement

[Markus Armbruster]

From: Zhang Han <zhanghan64@huawei.com>

Add braces {} for arms of if/for statement

Signed-off-by: Zhang Han <zhanghan64@huawei.com>
Message-Id: <20201228071129.24563-5-zhanghan64@huawei.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qobject/qdict.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qobject/qdict.c b/qobject/qdict.c
index 6c15ed14c1..0216ca7ac1 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -43,8 +43,9 @@ static unsigned int tdb_hash(const char *name)
     unsigned   i;      /* Used to cycle through random values. */
 
     /* Set the initial value from the key size. */
-    for (value = 0x238F13AF * strlen(name), i = 0; name[i]; i++)
+    for (value = 0x238F13AF * strlen(name), i = 0; name[i]; i++) {
         value = (value + (((const unsigned char *)name)[i] << (i * 5 % 24)));
+    }
 
     return (1103515243 * value + 12345);
 }
@@ -93,8 +94,9 @@ static QDictEntry *qdict_find(const QDict *qdict,
     QDictEntry *entry;
 
     QLIST_FOREACH(entry, &qdict->table[bucket], next)
-        if (!strcmp(entry->key, key))
+        if (!strcmp(entry->key, key)) {
             return entry;
+        }
 
     return NULL;
 }
-- 
2.26.2

[PULL 6/9] docs/interop/qmp-spec: Document the request queue limit

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20210127144734.2367693-1-armbru@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 docs/interop/qmp-spec.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
index cdf5842555..b0e8351d5b 100644
--- a/docs/interop/qmp-spec.txt
+++ b/docs/interop/qmp-spec.txt
@@ -133,9 +133,11 @@ to pass "id" with out-of-band commands.  Passing it with all commands
 is recommended for clients that accept capability "oob".
 
 If the client sends in-band commands faster than the server can
-execute them, the server will stop reading the requests from the QMP
-channel until the request queue length is reduced to an acceptable
-range.
+execute them, the server will stop reading requests until the request
+queue length is reduced to an acceptable range.
+
+To ensure commands to be executed out-of-band get read and executed,
+the client should have at most eight in-band commands in flight.
 
 Only a few commands support out-of-band execution.  The ones that do
 have "allow-oob": true in output of query-qmp-schema.
-- 
2.26.2

[PULL 7/9] qmp: Fix up comments after commit 9ce44e2ce2

[Markus Armbruster]

Commit 9ce44e2ce2 "qmp: Move dispatcher to a coroutine" replaced
monitor_qmp_bh_dispatcher() by monitor_qmp_dispatcher_co(), but
neglected to update comments.  Do that now.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20210201161504.1976989-2-armbru@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 monitor/qmp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/monitor/qmp.c b/monitor/qmp.c
index 8f91af32be..f6a1e7783b 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -79,7 +79,7 @@ static void monitor_qmp_cleanup_queue_and_resume(MonitorQMP *mon)
     qemu_mutex_lock(&mon->qmp_queue_lock);
 
     /*
-     * Same condition as in monitor_qmp_bh_dispatcher(), but before
+     * Same condition as in monitor_qmp_dispatcher_co(), but before
      * removing an element from the queue (hence no `- 1`).
      * Also, the queue should not be empty either, otherwise the
      * monitor hasn't been suspended yet (or was already resumed).
@@ -349,7 +349,7 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
 
     /*
      * Suspend the monitor when we can't queue more requests after
-     * this one.  Dequeuing in monitor_qmp_bh_dispatcher() or
+     * this one.  Dequeuing in monitor_qmp_dispatcher_co() or
      * monitor_qmp_cleanup_queue_and_resume() will resume it.
      * Note that when OOB is disabled, we queue at most one command,
      * for backward compatibility.
-- 
2.26.2

[PULL 8/9] qmp: Add more tracepoints

[Markus Armbruster]

Add tracepoints for in-band request enqueue and dequeue, processing of
queued in-band errors, and responses.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20210201161504.1976989-3-armbru@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 monitor/qmp.c        | 7 +++++++
 monitor/trace-events | 4 ++++
 2 files changed, 11 insertions(+)

diff --git a/monitor/qmp.c b/monitor/qmp.c
index f6a1e7783b..e37b047c8a 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -113,6 +113,7 @@ void qmp_send_response(MonitorQMP *mon, const QDict *rsp)
 
     json = qobject_to_json_pretty(data, mon->pretty);
     assert(json != NULL);
+    trace_monitor_qmp_respond(mon, json->str);
 
     g_string_append_c(json, '\n');
     monitor_puts(&mon->common, json->str);
@@ -251,6 +252,9 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
             }
         }
 
+        trace_monitor_qmp_in_band_dequeue(req_obj,
+                                          req_obj->mon->qmp_requests->length);
+
         if (qatomic_xchg(&qmp_dispatcher_co_busy, true) == true) {
             /*
              * Someone rescheduled us (probably because a new requests
@@ -287,6 +291,7 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
             monitor_qmp_dispatch(mon, req_obj->req);
         } else {
             assert(req_obj->err);
+            trace_monitor_qmp_err_in_band(error_get_pretty(req_obj->err));
             rsp = qmp_error_response(req_obj->err);
             req_obj->err = NULL;
             monitor_qmp_respond(mon, rsp);
@@ -364,6 +369,8 @@ static void handle_qmp_command(void *opaque, QObject *req, Error *err)
      * handled in time order.  Ownership for req_obj, req,
      * etc. will be delivered to the handler side.
      */
+    trace_monitor_qmp_in_band_enqueue(req_obj, mon,
+                                      mon->qmp_requests->length);
     assert(mon->qmp_requests->length < QMP_REQ_QUEUE_LEN_MAX);
     g_queue_push_tail(mon->qmp_requests, req_obj);
     qemu_mutex_unlock(&mon->qmp_queue_lock);
diff --git a/monitor/trace-events b/monitor/trace-events
index 0365ac4d99..348dcfca9b 100644
--- a/monitor/trace-events
+++ b/monitor/trace-events
@@ -10,6 +10,10 @@ monitor_protocol_event_queue(uint32_t event, void *qdict, uint64_t rate) "event=
 monitor_suspend(void *ptr, int cnt) "mon %p: %d"
 
 # qmp.c
+monitor_qmp_in_band_enqueue(void *req, void *mon, unsigned len) "%p mon %p len %u"
+monitor_qmp_in_band_dequeue(void *req, unsigned len) "%p len %u"
 monitor_qmp_cmd_in_band(const char *id) "%s"
+monitor_qmp_err_in_band(const char *desc) "%s"
 monitor_qmp_cmd_out_of_band(const char *id) "%s"
+monitor_qmp_respond(void *mon, const char *json) "mon %p resp: %s"
 handle_qmp_command(void *mon, const char *req) "mon %p req: %s"
-- 
2.26.2

[PULL 9/9] qmp: Resume OOB-enabled monitor before processing the request

[Markus Armbruster]

monitor_qmp_dispatcher_co() needs to resume the monitor if
handle_qmp_command() suspended it.  Two cases:

1. OOB enabled: suspended if mon->qmp_requests has no more space

2. OOB disabled: suspended always

We resume only after we processed the request.  Which can take a long
time.

Resume the monitor right when the queue has space to keep the monitor
available for out-of-band commands even in this corner case.

Leave the "OOB disabled" case alone.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20210201161504.1976989-4-armbru@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
[Trailing whitespace tidied up]
---
 monitor/qmp.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/monitor/qmp.c b/monitor/qmp.c
index e37b047c8a..43880fa623 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -214,7 +214,7 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
 {
     QMPRequest *req_obj = NULL;
     QDict *rsp;
-    bool need_resume;
+    bool oob_enabled;
     MonitorQMP *mon;
 
     while (true) {
@@ -273,11 +273,32 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
         aio_co_schedule(qemu_get_aio_context(), qmp_dispatcher_co);
         qemu_coroutine_yield();
 
+        /*
+         * @req_obj has a request, we hold req_obj->mon->qmp_queue_lock
+         */
+
         mon = req_obj->mon;
-        /* qmp_oob_enabled() might change after "qmp_capabilities" */
-        need_resume = !qmp_oob_enabled(mon) ||
-            mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
+
+        /*
+         * We need to resume the monitor if handle_qmp_command()
+         * suspended it.  Two cases:
+         * 1. OOB enabled: mon->qmp_requests has no more space
+         *    Resume right away, so that OOB commands can get executed while
+         *    this request is being processed.
+         * 2. OOB disabled: always
+         *    Resume only after we're done processing the request,
+         * We need to save qmp_oob_enabled() for later, because
+         * qmp_qmp_capabilities() can change it.
+         */
+        oob_enabled = qmp_oob_enabled(mon);
+        if (oob_enabled
+            && mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
+            monitor_resume(&mon->common);
+        }
+
         qemu_mutex_unlock(&mon->qmp_queue_lock);
+
+        /* Process request */
         if (req_obj->req) {
             if (trace_event_get_state(TRACE_MONITOR_QMP_CMD_IN_BAND)) {
                 QDict *qdict = qobject_to(QDict, req_obj->req);
@@ -298,10 +319,10 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
             qobject_unref(rsp);
         }
 
-        if (need_resume) {
-            /* Pairs with the monitor_suspend() in handle_qmp_command() */
+        if (!oob_enabled) {
             monitor_resume(&mon->common);
         }
+
         qmp_request_free(req_obj);
 
         /*
-- 
2.26.2

Re: [PULL 0/9] QMP patches patches for 2021-02-04

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210204140136.2769065-1-armbru@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210204140136.2769065-1-armbru@redhat.com
Subject: [PULL 0/9] QMP patches patches for 2021-02-04

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20210128003223.3561108-1-f4bug@amsat.org -> patchew/20210128003223.3561108-1-f4bug@amsat.org
 * [new tag]         patchew/20210204140136.2769065-1-armbru@redhat.com -> patchew/20210204140136.2769065-1-armbru@redhat.com
Switched to a new branch 'test'
6db5e4e qmp: Resume OOB-enabled monitor before processing the request
6a67c5fb qmp: Add more tracepoints
4661e67 qmp: Fix up comments after commit 9ce44e2ce2
764b192 docs/interop/qmp-spec: Document the request queue limit
57fcaf0 qobject: braces {} are necessary for all arms of this statement
e8ad6c0 qobject: spaces required around that operators
d083c84 qobject: code indent should never use tabs
2d7dc45 qobject: open brace '{' following struct go on the same line
b107051 monitor/qmp-cmds.c: Don't include ui/vnc.h

=== OUTPUT BEGIN ===
1/9 Checking commit b107051cffb4 (monitor/qmp-cmds.c: Don't include ui/vnc.h)
2/9 Checking commit 2d7dc454821b (qobject: open brace '{' following struct go on the same line)
3/9 Checking commit d083c8413d95 (qobject: code indent should never use tabs)
4/9 Checking commit e8ad6c0fb2b7 (qobject: spaces required around that operators)
ERROR: braces {} are necessary for all arms of this statement
#28: FILE: qobject/qdict.c:46:
+    for (value = 0x238F13AF * strlen(name), i = 0; name[i]; i++)
[...]

total: 1 errors, 0 warnings, 10 lines checked

Patch 4/9 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/9 Checking commit 57fcaf03568f (qobject: braces {} are necessary for all arms of this statement)
6/9 Checking commit 764b1924d5b4 (docs/interop/qmp-spec: Document the request queue limit)
7/9 Checking commit 4661e67c592f (qmp: Fix up comments after commit 9ce44e2ce2)
8/9 Checking commit 6a67c5fb8c1a (qmp: Add more tracepoints)
9/9 Checking commit 6db5e4ef9c3d (qmp: Resume OOB-enabled monitor before processing the request)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210204140136.2769065-1-armbru@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PULL 0/9] QMP patches patches for 2021-02-04

[Peter Maydell]

On Thu, 4 Feb 2021 at 14:01, Markus Armbruster <armbru@redhat.com> wrote:
>
> The following changes since commit db754f8ccaf2f073c9aed46a4389e9c0c2080399:
>
>   Merge remote-tracking branch 'remotes/rth-gitlab/tags/pull-tcg-20210202' into staging (2021-02-03 19:35:57 +0000)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/armbru.git tags/pull-qmp-2021-02-04
>
> for you to fetch changes up to 88daf0996cd0488e93e67bcb0af258f2c24f117a:
>
>   qmp: Resume OOB-enabled monitor before processing the request (2021-02-04 13:20:29 +0100)
>
> ----------------------------------------------------------------
> QMP patches patches for 2021-02-04
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0
for any user-visible changes.

-- PMM