[Qemu-devel] [PULL 0/2] QAPI and monitor patches for 2018-07-23 (3.0.0-rc2)

[Qemu-devel] [PULL 0/2] QAPI and monitor patches for 2018-07-23 (3.0.0-rc2)

[Markus Armbruster]

The following changes since commit 6598f0cdad6acc6674c4f060fa46e537228c2c47:

  po: Don't include comments with location (2018-07-23 10:50:54 +0100)

are available in the Git repository at:

  git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2018-07-23

for you to fetch changes up to 62aa1d887ff9fc76adb488d31447d126a78f4b8f:

  monitor: Fix unsafe sharing of @cur_mon among threads (2018-07-23 14:00:03 +0200)

----------------------------------------------------------------
QAPI and monitor patches for 2018-07-23 (3.0.0-rc2)

This pull request consists of a late external interface tweak and a
thread safety fix, both related to QMP out-of-band execution.

----------------------------------------------------------------
Markus Armbruster (1):
      qapi: Make 'allow-oob' optional in SchemaInfoCommand

Peter Xu (1):
      monitor: Fix unsafe sharing of @cur_mon among threads

 include/monitor/monitor.h  |  2 +-
 monitor.c                  |  2 +-
 qapi/introspect.json       |  6 +++---
 scripts/qapi/introspect.py | 10 +++++-----
 stubs/monitor.c            |  2 +-
 tests/test-util-sockets.c  |  2 +-
 6 files changed, 12 insertions(+), 12 deletions(-)
armbru@dusky:~/work/qemu$ rm *patch
armbru@dusky:~/work/qemu$ git-format-patch --subject-prefix PULL master
0000-cover-letter.patch
0001-qapi-Make-allow-oob-optional-in-SchemaInfoCommand.patch
0002-monitor-Fix-unsafe-sharing-of-cur_mon-among-threads.patch

-- 
2.17.1

[Qemu-devel] [PULL 1/2] qapi: Make 'allow-oob' optional in SchemaInfoCommand

[Markus Armbruster]

Making 'allow-oob' optional in SchemaInfoCommand permits omitting it
in the common case.  Shrinks query-qmp-schema's output from 122.1KiB
to 118.6KiB for me.

Note that out-of-band execution is still experimental (you have to
configure the monitor with x-oob=on to use it).

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20180718090557.17248-1-armbru@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 qapi/introspect.json       |  6 +++---
 scripts/qapi/introspect.py | 10 +++++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/qapi/introspect.json b/qapi/introspect.json
index c7f67b7d78..137b39b992 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -259,8 +259,8 @@
 #
 # @ret-type: the name of the command's result type.
 #
-# @allow-oob: whether the command allows out-of-band execution.
-#             (Since: 2.12)
+# @allow-oob: whether the command allows out-of-band execution,
+#             defaults to false (Since: 2.12)
 #
 # TODO: @success-response (currently irrelevant, because it's QGA, not QMP)
 #
@@ -268,7 +268,7 @@
 ##
 { 'struct': 'SchemaInfoCommand',
   'data': { 'arg-type': 'str', 'ret-type': 'str',
-            'allow-oob': 'bool' } }
+            '*allow-oob': 'bool' } }
 
 ##
 # @SchemaInfoEvent:
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 70ca5dd876..189a4edaba 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -184,11 +184,11 @@ const QLitObject %(c_name)s = %(c_string)s;
                       success_response, boxed, allow_oob, allow_preconfig):
         arg_type = arg_type or self._schema.the_empty_object_type
         ret_type = ret_type or self._schema.the_empty_object_type
-        self._gen_qlit(name, 'command',
-                       {'arg-type': self._use_type(arg_type),
-                        'ret-type': self._use_type(ret_type),
-                        'allow-oob': allow_oob},
-                       ifcond)
+        obj = {'arg-type': self._use_type(arg_type),
+               'ret-type': self._use_type(ret_type) }
+        if allow_oob:
+            obj['allow-oob'] = allow_oob
+        self._gen_qlit(name, 'command', obj, ifcond)
 
     def visit_event(self, name, info, ifcond, arg_type, boxed):
         arg_type = arg_type or self._schema.the_empty_object_type
-- 
2.17.1

[Qemu-devel] [PULL 2/2] monitor: Fix unsafe sharing of @cur_mon among threads

[Markus Armbruster]

From: Peter Xu <peterx@redhat.com>

@cur_mon is null unless the main thread is running monitor code, either
HMP code within monitor_read(), or QMP code within
monitor_qmp_dispatch().

Use of @cur_mon outside the main thread is therefore unsafe.

Most of its uses are in monitor command handlers.  These run in the main
thread.

However, there are also uses hiding elsewhere, such as in
error_vprintf(), and thus error_report(), making these functions unsafe
outside the main thread.  No such unsafe uses are known at this time.
Regardless, this is an unnecessary trap.  It's an ancient trap, though.

More recently, commit cf869d53172 "qmp: support out-of-band (oob)
execution" spiced things up: the monitor I/O thread assigns to @cur_mon
when executing commands out-of-band.  Having two threads save, set and
restore @cur_mon without synchronization is definitely unsafe.  We can
end up with @cur_mon null while the main thread runs monitor code, or
non-null while it runs non-monitor code.

We could fix this by making the I/O thread not mess with @cur_mon, but
that would leave the trap armed and ready.

Instead, make @cur_mon thread-local.  It's now reliably null unless the
thread is running monitor code.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[peterx: update subject and commit message written by Markus]
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
Message-Id: <20180720033451.32710-1-peterx@redhat.com>
---
 include/monitor/monitor.h | 2 +-
 monitor.c                 | 2 +-
 stubs/monitor.c           | 2 +-
 tests/test-util-sockets.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index d6ab70cae2..2ef5e04b37 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -6,7 +6,7 @@
 #include "qapi/qapi-types-misc.h"
 #include "qemu/readline.h"
 
-extern Monitor *cur_mon;
+extern __thread Monitor *cur_mon;
 
 /* flags for monitor_init */
 /* 0x01 unused */
diff --git a/monitor.c b/monitor.c
index be29634a00..f75027b09e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -290,7 +290,7 @@ static mon_cmd_t info_cmds[];
 
 QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
 
-Monitor *cur_mon;
+__thread Monitor *cur_mon;
 
 static void monitor_command_cb(void *opaque, const char *cmdline,
                                void *readline_opaque);
diff --git a/stubs/monitor.c b/stubs/monitor.c
index e018c8f594..3890771bb5 100644
--- a/stubs/monitor.c
+++ b/stubs/monitor.c
@@ -3,7 +3,7 @@
 #include "qemu-common.h"
 #include "monitor/monitor.h"
 
-Monitor *cur_mon = NULL;
+__thread Monitor *cur_mon;
 
 int monitor_get_fd(Monitor *mon, const char *name, Error **errp)
 {
diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index acadd85e8f..6195a3ac36 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -69,7 +69,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
  * stubs/monitor.c is defined, to make sure monitor.o is discarded
  * otherwise we get duplicate syms at link time.
  */
-Monitor *cur_mon;
+__thread Monitor *cur_mon;
 void monitor_init(Chardev *chr, int flags) {}
 
 
-- 
2.17.1

Re: [Qemu-devel] [PULL 0/2] QAPI and monitor patches for 2018-07-23 (3.0.0-rc2)

[Peter Maydell]

On 23 July 2018 at 14:13, Markus Armbruster <armbru@redhat.com> wrote:
> The following changes since commit 6598f0cdad6acc6674c4f060fa46e537228c2c47:
>
>   po: Don't include comments with location (2018-07-23 10:50:54 +0100)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2018-07-23
>
> for you to fetch changes up to 62aa1d887ff9fc76adb488d31447d126a78f4b8f:
>
>   monitor: Fix unsafe sharing of @cur_mon among threads (2018-07-23 14:00:03 +0200)
>
> ----------------------------------------------------------------
> QAPI and monitor patches for 2018-07-23 (3.0.0-rc2)
>
> This pull request consists of a late external interface tweak and a
> thread safety fix, both related to QMP out-of-band execution.
>
> ----------------------------------------------------------------
> Markus Armbruster (1):
>       qapi: Make 'allow-oob' optional in SchemaInfoCommand
>
> Peter Xu (1):
>       monitor: Fix unsafe sharing of @cur_mon among threads
>
>  include/monitor/monitor.h  |  2 +-
>  monitor.c                  |  2 +-
>  qapi/introspect.json       |  6 +++---
>  scripts/qapi/introspect.py | 10 +++++-----
>  stubs/monitor.c            |  2 +-
>  tests/test-util-sockets.c  |  2 +-
>  6 files changed, 12 insertions(+), 12 deletions(-)

Applied, thanks.

> armbru@dusky:~/work/qemu$ rm *patch
> armbru@dusky:~/work/qemu$ git-format-patch --subject-prefix PULL master
> 0000-cover-letter.patch
> 0001-qapi-Make-allow-oob-optional-in-SchemaInfoCommand.patch
> 0002-monitor-Fix-unsafe-sharing-of-cur_mon-among-threads.patch

...stray cut-n-paste? :-)

-- PMM