QMP: out-of-band (OOB) execution support

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

CC Eric Blake <[email protected]>

CC Markus Armbruster <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 include/qapi/qmp/qstring.h |  1 +
 qobject/qstring.c          | 10 ++++++++++
 2 files changed, 11 insertions(+)

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

Reviewed-by Fam Zheng <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 include/qapi/qmp/qstring.h |  1 +
 qobject/qstring.c          | 11 +++++++++++
 2 files changed, 12 insertions(+)

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

Reviewed-by Fam Zheng <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 qom/object.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

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

Reviewed-by Dr. David Alan Gilbert <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

qga/main.c +2 -1

index 62a62755bd..3b5ebbc1ee 100644
@@ -593,7 +593,8 @@ static void process_command(GAState *s, QDict *req)
 }
 
 /* handle requests/control events coming in over the channel */
-static void process_event(JSONMessageParser *parser, GQueue *tokens)
+static void process_event(JSONMessageParser *parser, GQueue *tokens,
+                          void *opaque)
 {
     GAState *s = container_of(parser, GAState, parser);
     QDict *qdict;

Quoted 15 lines · Peter Xu

qga/main.c +2 -1

index 62a62755bd..3b5ebbc1ee 100644
@@ -593,7 +593,8 @@ static void process_command(GAState *s, QDict *req)
 }
 
 /* handle requests/control events coming in over the channel */
-static void process_event(JSONMessageParser *parser, GQueue *tokens)
+static void process_event(JSONMessageParser *parser, GQueue *tokens,
+                          void *opaque)
 {
     GAState *s = container_of(parser, GAState, parser);
     QDict *qdict;

qga/main.c static int run_agent(GAState *s, GAConfig *config, int socket_activation) · line 1324

Quoted 5 lines

qga/main.c +1 -1

@@ -1320,7 +1321,7 @@ static int run_agent(GAState *s, GAConfig *config, int socket_activation)
     s->command_state = ga_command_state_new();
     ga_command_state_init(s, s->command_state);
     ga_command_state_init_all(s->command_state);
-    json_message_parser_init(&s->parser, process_event);
+    json_message_parser_init(&s->parser, process_event, NULL);

This patch leaves the code with 2 ways of getting at state from the parser pointer:
1. Use container_of() like existing users.
2. Use the new (unused) opaque argument.

Given that #1 exists, is this patch really necessary?

Please explain in the commit description, don't make me look ahead in the patch series.

Quoted 27 lines · Stefan Hajnoczi

Quoted 15 lines · Peter Xu

qga/main.c +2 -1

index 62a62755bd..3b5ebbc1ee 100644
@@ -593,7 +593,8 @@ static void process_command(GAState *s, QDict *req)
 }
 
 /* handle requests/control events coming in over the channel */
-static void process_event(JSONMessageParser *parser, GQueue *tokens)
+static void process_event(JSONMessageParser *parser, GQueue *tokens,
+                          void *opaque)
 {
     GAState *s = container_of(parser, GAState, parser);
     QDict *qdict;

qga/main.c static int run_agent(GAState *s, GAConfig *config, int socket_activation) · line 1324

Quoted 5 lines

qga/main.c +1 -1

@@ -1320,7 +1321,7 @@ static int run_agent(GAState *s, GAConfig *config, int socket_activation)
     s->command_state = ga_command_state_new();
     ga_command_state_init(s, s->command_state);
     ga_command_state_init_all(s->command_state);
-    json_message_parser_init(&s->parser, process_event);
+    json_message_parser_init(&s->parser, process_event, NULL);

This patch leaves the code with 2 ways of getting at state from the parser pointer:
1. Use container_of() like existing users.
2. Use the new (unused) opaque argument.

Quoted 1 line

Given that #1 exists, is this patch really necessary?

I didn't really notice that. Thanks for pointing out.

However even if so I would still prefer the opaque way to do it if asked. Existing #1 of course works but IMHO is less flexible and has dependency between structure layouts.

How about I append another patch to convert existing users (or, I can post as separate patches after this series)? It's not really a lot, and the conversion would be obvious:

*** qga/main.c:
run_agent[1324] json_message_parser_init(&s->parser, process_event, NULL);
*** qobject/qjson.c:
qobject_from_jsonv[45] json_message_parser_init(&state.parser, parse_json, NULL);
*** tests/libqtest.c:
qmp_fd_receive[438] json_message_parser_init(&qmp.parser, qmp_response, NULL);

Though, if you still insist, I can drop it too.

Quoted 47 lines · Peter Xu

Quoted 27 lines · Stefan Hajnoczi

Quoted 15 lines · Peter Xu

qga/main.c +2 -1

index 62a62755bd..3b5ebbc1ee 100644
@@ -593,7 +593,8 @@ static void process_command(GAState *s, QDict *req)
 }
 
 /* handle requests/control events coming in over the channel */
-static void process_event(JSONMessageParser *parser, GQueue *tokens)
+static void process_event(JSONMessageParser *parser, GQueue *tokens,
+                          void *opaque)
 {
     GAState *s = container_of(parser, GAState, parser);
     QDict *qdict;

qga/main.c static int run_agent(GAState *s, GAConfig *config, int socket_activation) · line 1324

Quoted 5 lines

qga/main.c +1 -1

@@ -1320,7 +1321,7 @@ static int run_agent(GAState *s, GAConfig *config, int socket_activation)
     s->command_state = ga_command_state_new();
     ga_command_state_init(s, s->command_state);
     ga_command_state_init_all(s->command_state);
-    json_message_parser_init(&s->parser, process_event);
+    json_message_parser_init(&s->parser, process_event, NULL);

This patch leaves the code with 2 ways of getting at state from the parser pointer:
1. Use container_of() like existing users.
2. Use the new (unused) opaque argument.

Quoted 1 line

Given that #1 exists, is this patch really necessary?

I didn't really notice that. Thanks for pointing out.

However even if so I would still prefer the opaque way to do it if asked. Existing #1 of course works but IMHO is less flexible and has dependency between structure layouts.

How about I append another patch to convert existing users (or, I can post as separate patches after this series)? It's not really a lot, and the conversion would be obvious:

*** qga/main.c:
run_agent[1324] json_message_parser_init(&s->parser, process_event, NULL);
*** qobject/qjson.c:
qobject_from_jsonv[45] json_message_parser_init(&state.parser, parse_json, NULL);
*** tests/libqtest.c:
qmp_fd_receive[438] json_message_parser_init(&qmp.parser, qmp_response, NULL);

Quoted 1 line

Though, if you still insist, I can drop it too.

I prefer dropping it. Smaller patch series get reviewed quicker, cause fewer merge conflicts, are lower risk, etc.

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 passed in to that using the new JSON Parser opaque field now.

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.

Reviewed-by Eric Blake <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

monitor.c +0 -0

index ab80d32c70..322dfb5f31 100644

Quoted 21 lines · 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 passed in to that using the new JSON Parser opaque field now.

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.

Reviewed-by Eric Blake <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

monitor.c +0 -0

index ab80d32c70..322dfb5f31 100644

monitor.c static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens, · line 3816

Quoted 5 lines

monitor.c +1 -1

@@ -3813,7 +3813,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens,
 {
     QObject *req, *rsp = NULL, *id = NULL;
     QDict *qdict = NULL;
-    Monitor *mon = cur_mon;
+    Monitor *mon = opaque, *old_mon;

Or without the new opaque argument:

MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser); Monitor *mon = container_of(mon_qmp, Monitor, qmp);

vl.c +0 -1

index 1ad1c04637..1ec995a6ae 100644
@@ -3144,7 +3144,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);

Quoted 16 lines · Peter Xu

vl.c +0 -1

index 1ad1c04637..1ec995a6ae 100644
@@ -3144,7 +3144,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);

@@ -4690,6 +4689,8 @@ 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);
 

vl.c int main(int argc, char **argv, char **envp) · line 4692

Quoted 1 line

vl.c +1 -0

+    monitor_init_globals();

Why did you move monitor_init_qmp_commands() down here?

There are many function calls between the old position and the new position. Did you check all of them to make sure they don't touch the monitor which is now totally uninitialized?

Quoted 21 lines · Stefan Hajnoczi

Quoted 13 lines · Peter Xu

vl.c +0 -1

index 1ad1c04637..1ec995a6ae 100644
@@ -3144,7 +3144,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);

vl.c int main(int argc, char **argv, char **envp) · line 4692

Quoted 4 lines

vl.c +1 -0

@@ -4690,6 +4689,8 @@ 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);
 
+    monitor_init_globals();

Why did you move monitor_init_qmp_commands() down here?

Quoted 3 lines

There are many function calls between the old position and the new position. Did you check all of them to make sure they don't touch the monitor which is now totally uninitialized?

Yeh, this patch is a bit hairy, but I really think we should do it. Because there are too many places (as you have seen) that we inited monitor stuff in different places.

Quoted 26 lines · Peter Xu

Quoted 24 lines · Stefan Hajnoczi

Quoted 13 lines · Peter Xu

vl.c +0 -1

index 1ad1c04637..1ec995a6ae 100644
@@ -3144,7 +3144,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);

vl.c int main(int argc, char **argv, char **envp) · line 4692

Quoted 4 lines

vl.c +1 -0

@@ -4690,6 +4689,8 @@ 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);
 
+    monitor_init_globals();

Why did you move monitor_init_qmp_commands() down here?

There are many function calls between the old position and the new position. Did you check all of them to make sure they don't touch the monitor which is now totally uninitialized?

Quoted 3 lines

Yeh, this patch is a bit hairy, but I really think we should do it. Because there are too many places (as you have seen) that we inited monitor stuff in different places.

But why did you move it down here? Can you replace the monitor_init_qmp_commands() call instead?

Quoted 31 lines · Stefan Hajnoczi

Quoted 29 lines · Peter Xu

Quoted 17 lines · Stefan Hajnoczi

Quoted 16 lines · Peter Xu

vl.c +0 -1

index 1ad1c04637..1ec995a6ae 100644
@@ -3144,7 +3144,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);

@@ -4690,6 +4689,8 @@ 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);
 

Quoted 7 lines

vl.c int main(int argc, char **argv, char **envp) · line 4692

Quoted 1 line

vl.c +1 -0

+    monitor_init_globals();

Why did you move monitor_init_qmp_commands() down here?

There are many function calls between the old position and the new position. Did you check all of them to make sure they don't touch the monitor which is now totally uninitialized?

Yeh, this patch is a bit hairy, but I really think we should do it. Because there are too many places (as you have seen) that we inited monitor stuff in different places.

Quoted 2 lines

But why did you move it down here? Can you replace the monitor_init_qmp_commands() call instead?

I just moved it closer to (actually, right above) initializations of monitors to be clear that these globals will never be touched until this point. Or do you want me to move this call exactly back to the place where monitor_init_qmp_commands() was called before? Thanks,

Quoted 32 lines · Peter Xu

Quoted 31 lines · Stefan Hajnoczi

Quoted 29 lines · Peter Xu

Quoted 17 lines · Stefan Hajnoczi

Quoted 16 lines · Peter Xu

vl.c +0 -1

index 1ad1c04637..1ec995a6ae 100644
@@ -3144,7 +3144,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);

@@ -4690,6 +4689,8 @@ 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);
 

Quoted 7 lines

vl.c int main(int argc, char **argv, char **envp) · line 4692

Quoted 1 line

vl.c +1 -0

+    monitor_init_globals();

Why did you move monitor_init_qmp_commands() down here?

There are many function calls between the old position and the new position. Did you check all of them to make sure they don't touch the monitor which is now totally uninitialized?

Yeh, this patch is a bit hairy, but I really think we should do it. Because there are too many places (as you have seen) that we inited monitor stuff in different places.

Quoted 7 lines

Quoted 2 lines

But why did you move it down here? Can you replace the monitor_init_qmp_commands() call instead?

I just moved it closer to (actually, right above) initializations of monitors to be clear that these globals will never be touched until this point. Or do you want me to move this call exactly back to the place where monitor_init_qmp_commands() was called before? Thanks,

Hmm...now that we're discussing this patch in detail I found an ordering dependency:

qtest_enabled() only works after configure_accelerator(current_machine), so the monitor_qapi_event_init() cannot be moved to the monitor_init_qmp_commands() location.

Everything else has no dependencies. I would be best to remove the self-contained init from vl.c:main() so we don't have to worry about dependencies again. How about:

}

Now vl.c:main() doesn't need to initialize the monitor.

We still need to handle the qtest_enabled() dependency:

(quoted diff) +4 -1

-static QEMUClockType event_clock_type = QEMU_CLOCK_REALTIME;
+static inline QEMUClockType event_clock_type(void)
+{
+    return qtest_enabled() ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME;
+}

This way the qtest_enabled() call is deferred until later when the accelerators have been initialized.

Quoted 17 lines · Stefan Hajnoczi

Quoted 4 lines · Peter Xu

Quoted 3 lines · Stefan Hajnoczi

Quoted 2 lines · Peter Xu

Quoted 1 line · Stefan Hajnoczi

On Tue, Dec 05, 2017 at 01:51:41PM +0800, Peter Xu wrote:

}

Now vl.c:main() doesn't need to initialize the monitor.

We still need to handle the qtest_enabled() dependency:

(quoted diff) +4 -1

-static QEMUClockType event_clock_type = QEMU_CLOCK_REALTIME;
+static inline QEMUClockType event_clock_type(void)
+{
+    return qtest_enabled() ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME;
+}

Quoted 2 lines

This way the qtest_enabled() call is deferred until later when the accelerators have been initialized.

Or... to be much simpler... How about we just use my patch? IMHO it calls monitor_init_globals() after configure_accelerator() so monitor_qapi_event_init() and everything else would just work as expected?

Quoted 18 lines · Peter Xu

Quoted 17 lines · Stefan Hajnoczi

Quoted 4 lines · Peter Xu

Quoted 3 lines · Stefan Hajnoczi

Quoted 2 lines · Peter Xu

Quoted 1 line · Stefan Hajnoczi

On Tue, Dec 05, 2017 at 01:51:41PM +0800, Peter Xu wrote:

}

Now vl.c:main() doesn't need to initialize the monitor.

We still need to handle the qtest_enabled() dependency:

(quoted diff) +4 -1

-static QEMUClockType event_clock_type = QEMU_CLOCK_REALTIME;
+static inline QEMUClockType event_clock_type(void)
+{
+    return qtest_enabled() ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME;
+}

Quoted 7 lines

Quoted 2 lines

This way the qtest_enabled() call is deferred until later when the accelerators have been initialized.

Or... to be much simpler... How about we just use my patch? IMHO it calls monitor_init_globals() after configure_accelerator() so monitor_qapi_event_init() and everything else would just work as expected?

Ordering is complex in vl.c:main() so I think it's more maintainable to put initialization in a monitor.c constructor function and/or monitor_init().

If you want to keep your approach please add a comment about the qtest_enabled() dependency in vl.c:main().

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 <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

monitor.c +5 -0

+    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 +4153,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 = {

Signature · 1 line

2.14.3

monitor.c struct Monitor { · line 216

Quoted 9 lines · Peter Xu

monitor.c +5 -0

@@ -208,6 +209,12 @@ struct Monitor {
     QTAILQ_ENTRY(Monitor) entry;
 };
 
+struct MonitorGlobal {
+    IOThread *mon_iothread;
+};
+
+static struct MonitorGlobal mon_global;

structs can be anonymous. That avoids the QEMU coding style violation (structs must be typedefed):

  static struct {
      IOThread *mon_iothread;
  } mon_global;

In general global variables are usually top-level variables in QEMU. I'm not sure why wrapping globals in a struct is useful.

monitor.c void monitor_cleanup(void) · line 4146

Quoted 13 lines

monitor.c +8 -0

@@ -4117,6 +4136,16 @@ 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.  See
+     * again on the glib bug mentioned in 2b316774f6 for a reason.
+     *
+     * TODO: the bug is fixed in glib 2.28, so we can remove this hack
+     * as long as we won't support glib versions older than it.
+     */

I find this comment confusing. There is no GSource .finalize() in monitor.c so why does monitor_cleanup() need to work around the bug?

I see that monitor_data_destroy() is not thread-safe so the IOThread must be stopped first. That is unrelated to glib.

Quoted 19 lines

monitor.c +5 -0

+    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 +4153,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 = {

Signature · 1 line

2.14.3

Quoted 14 lines · Stefan Hajnoczi

monitor.c struct Monitor { · line 216

Quoted 9 lines · Peter Xu

monitor.c +5 -0

@@ -208,6 +209,12 @@ struct Monitor {
     QTAILQ_ENTRY(Monitor) entry;
 };
 
+struct MonitorGlobal {
+    IOThread *mon_iothread;
+};
+
+static struct MonitorGlobal mon_global;

structs can be anonymous. That avoids the QEMU coding style violation (structs must be typedefed):

Quoted 3 lines

  static struct {
      IOThread *mon_iothread;
  } mon_global;

Will fix this up. Thanks.

Quoted 1 line

Quoted 2 lines

In general global variables are usually top-level variables in QEMU. I'm not sure why wrapping globals in a struct is useful.

Because I see too many global variables for monitor code, and from this patch I wanted to start moving them altogether into this global struct. I didn't really do that in current series because it's more like a clean up, but if you see future patches,

I can add a comment in the commit message, like: "Let's start to create a struct to keep monitor global variables together". Would that help?

Quoted 17 lines

monitor.c void monitor_cleanup(void) · line 4146

Quoted 13 lines

monitor.c +8 -0

@@ -4117,6 +4136,16 @@ 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.  See
+     * again on the glib bug mentioned in 2b316774f6 for a reason.
+     *
+     * TODO: the bug is fixed in glib 2.28, so we can remove this hack
+     * as long as we won't support glib versions older than it.
+     */

I find this comment confusing. There is no GSource .finalize() in monitor.c so why does monitor_cleanup() need to work around the bug?

Quoted 2 lines

I see that monitor_data_destroy() is not thread-safe so the IOThread must be stopped first. That is unrelated to glib.

Yeah actually that's a suggestion by Dave and Dan in previous review comments which makes sense to me:

http://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04344.html

I'm fine with either way: keep it as it is,

or instead saying
"monitor_data_destroy() is not thread-safe" (which finally will still root cause to that glib bug).

But how about we just keep it in case
it may be helpful some day?

Quoted 25 lines · Peter Xu

Quoted 14 lines · Stefan Hajnoczi

monitor.c struct Monitor { · line 216

Quoted 9 lines · Peter Xu

monitor.c +5 -0

@@ -208,6 +209,12 @@ struct Monitor {
     QTAILQ_ENTRY(Monitor) entry;
 };
 
+struct MonitorGlobal {
+    IOThread *mon_iothread;
+};
+
+static struct MonitorGlobal mon_global;

structs can be anonymous. That avoids the QEMU coding style violation (structs must be typedefed):

Quoted 3 lines

  static struct {
      IOThread *mon_iothread;
  } mon_global;

Will fix this up. Thanks.

Quoted 3 lines

In general global variables are usually top-level variables in QEMU. I'm not sure why wrapping globals in a struct is useful.

Quoted 4 lines

Because I see too many global variables for monitor code, and from this patch I wanted to start moving them altogether into this global struct. I didn't really do that in current series because it's more like a clean up, but if you see future patches,

You cannot expect reviewers to jump around a 26 patch series to check for possible future changes. Each patch must be self-contained and the changes need to be justified.

Quoted 4 lines

I can add a comment in the commit message, like: "Let's start to create a struct to keep monitor global variables together". Would that help?

It's better to add a comment in the code:

/* Add monitor global variables to this struct */

so that other people modifying the code know what this is about and can participate.

Other people might not want to do it since it leads to repetitive and long names like mon_global.mon_iothread. Or they might just not see the struct when defining a global.

The chance of the struct being used consistently is low and therefore I wouldn't do it.

Quoted 23 lines

Quoted 17 lines

monitor.c void monitor_cleanup(void) · line 4146

Quoted 13 lines

monitor.c +8 -0

@@ -4117,6 +4136,16 @@ 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.  See
+     * again on the glib bug mentioned in 2b316774f6 for a reason.
+     *
+     * TODO: the bug is fixed in glib 2.28, so we can remove this hack
+     * as long as we won't support glib versions older than it.
+     */

I find this comment confusing. There is no GSource .finalize() in monitor.c so why does monitor_cleanup() need to work around the bug?

Quoted 2 lines

I see that monitor_data_destroy() is not thread-safe so the IOThread must be stopped first. That is unrelated to glib.

Yeah actually that's a suggestion by Dave and Dan in previous review comments which makes sense to me:

Quoted 3 lines

http://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04344.html

I'm fine with either way: keep it as it is,

The meaning of the comment is unclear to me and you haven't been able to explain it. Therefore, merging this comment isn't justified.

Quoted 4 lines

or instead saying
"monitor_data_destroy() is not thread-safe" (which finally will still root cause to that glib bug).

This is incorrect. The problem is that the IOThread may run chardev handler functions while the main loop thread invokes monitor_data_destroy(). There is nothing protecting the chardev itself (it's not thread-safe!) nor the monitor state that is being freed, so a running chardev handler function could crash.

This means that even without the glib bug, it's not safe to call monitor_data_destroy() while the chardev is still attached to the IOThread event loop.

Quoted 2 lines

But how about we just keep it in case
it may be helpful some day?

Please see above.

Stefan

Quoted 26 lines · Stefan Hajnoczi

Quoted 25 lines · Peter Xu

Quoted 14 lines · Stefan Hajnoczi

monitor.c struct Monitor { · line 216

Quoted 9 lines · Peter Xu

monitor.c +5 -0

@@ -208,6 +209,12 @@ struct Monitor {
     QTAILQ_ENTRY(Monitor) entry;
 };
 
+struct MonitorGlobal {
+    IOThread *mon_iothread;
+};
+
+static struct MonitorGlobal mon_global;

structs can be anonymous. That avoids the QEMU coding style violation (structs must be typedefed):

Quoted 3 lines

  static struct {
      IOThread *mon_iothread;
  } mon_global;

Will fix this up. Thanks.

Quoted 3 lines

In general global variables are usually top-level variables in QEMU. I'm not sure why wrapping globals in a struct is useful.

Quoted 8 lines

Quoted 4 lines

Because I see too many global variables for monitor code, and from this patch I wanted to start moving them altogether into this global struct. I didn't really do that in current series because it's more like a clean up, but if you see future patches,

You cannot expect reviewers to jump around a 26 patch series to check for possible future changes. Each patch must be self-contained and the changes need to be justified.

Noted.

Quoted 11 lines

Quoted 4 lines

I can add a comment in the commit message, like: "Let's start to create a struct to keep monitor global variables together". Would that help?

It's better to add a comment in the code:

/* Add monitor global variables to this struct */

so that other people modifying the code know what this is about and can participate.

Will do.

Quoted 31 lines

Other people might not want to do it since it leads to repetitive and long names like mon_global.mon_iothread. Or they might just not see the struct when defining a global.

The chance of the struct being used consistently is low and therefore I wouldn't do it.

Quoted 23 lines

Quoted 17 lines

monitor.c void monitor_cleanup(void) · line 4146

Quoted 13 lines

monitor.c +8 -0

@@ -4117,6 +4136,16 @@ 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.  See
+     * again on the glib bug mentioned in 2b316774f6 for a reason.
+     *
+     * TODO: the bug is fixed in glib 2.28, so we can remove this hack
+     * as long as we won't support glib versions older than it.
+     */

I find this comment confusing. There is no GSource .finalize() in monitor.c so why does monitor_cleanup() need to work around the bug?

Quoted 2 lines

I see that monitor_data_destroy() is not thread-safe so the IOThread must be stopped first. That is unrelated to glib.

Yeah actually that's a suggestion by Dave and Dan in previous review comments which makes sense to me:

Quoted 6 lines

Quoted 3 lines

http://lists.gnu.org/archive/html/qemu-devel/2017-11/msg04344.html

I'm fine with either way: keep it as it is,

The meaning of the comment is unclear to me and you haven't been able to explain it. Therefore, merging this comment isn't justified.

(Please see below)

Quoted 10 lines

Quoted 4 lines

or instead saying
"monitor_data_destroy() is not thread-safe" (which finally will still root cause to that glib bug).

This is incorrect. The problem is that the IOThread may run chardev handler functions while the main loop thread invokes monitor_data_destroy(). There is nothing protecting the chardev itself (it's not thread-safe!) nor the monitor state that is being freed, so a running chardev handler function could crash.

It's only about a single line of comment, but since we are at this, I think it would still be good to discuss it in case I was wrong.

Firstly, I agree that chardevs are not thread-safe. But IMHO monitors are thread-safe. There is the big monitor_lock to protect. There can be bug though, but generally speaking that lock should be doing the thread safety work.

Next, basically when destroying the monitors logically we should never touch the chardev if without that glib bug. Or say, if without the bug we should not call qemu_chr_fe_deinit() in monitor_data_destroy().

monitor.c +18 -0

@@ -4117,19 +4121,37 @@ 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.
+     */

monitor.c +0 -4

         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)

Signature · 1 line

2.14.3

monitor.c static void monitor_data_init(Monitor *mon, bool skip_flush) · line 588

Quoted 5 lines · Peter Xu

monitor.c +1 -0

@@ -583,6 +585,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;

Why is mon->use_io_thr is a field instead of a monitor_init() argument.

Quoted 23 lines

monitor.c +18 -0

@@ -4117,19 +4121,37 @@ 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.
+     */

monitor.c void monitor_init(Chardev *chr, int flags) · line 4150

Quoted 10 lines

monitor.c +5 -1

+    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);
+                                 monitor_qmp_event, NULL, mon, context, true);
         qemu_chr_fe_set_echo(&mon->chr, true);
         json_message_parser_init(&mon->qmp.parser, handle_qmp_command, mon);

The comment above mentions the race condition between qemu_chr_fe_set_handlers() and mon_list. I think that means the order must be:

  json_message_parser_init(&mon->qmp.parser, handle_qmp_command, mon);
  qemu_chr_fe_set_echo(&mon->chr, true);
  qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
                           monitor_qmp_event, NULL, mon, context, true);

monitor.c void monitor_init(Chardev *chr, int flags) · line 4151

Quoted 2 lines

monitor.c +0 -0

     } else {

assert(!context); /* HMP does not support IOThreads */

Quoted 14 lines

monitor.c +0 -4

         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)

Signature · 1 line

2.14.3

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 <[email protected]>

---
 monitor.c        | 15 +++++++++++++--
 qapi-schema.json | 13 +++++++++++++
 tests/qmp-test.c | 10 +++++++++-
 3 files changed, 35 insertions(+), 3 deletions(-)

monitor.c +7 -0

index e8f5a586e4..bad6ee8dd1 100644
@@ -3944,12 +3944,23 @@ void monitor_resume(Monitor *mon)
 
 static QObject *get_qmp_greeting(void)
 {
+    QDict *result = qdict_new(), *qmp = qdict_new();
+    QList *cap_list = qlist_new();
     QObject *ver = NULL;
+    QMPCapability cap;
+
+    qdict_put(result, "QMP", qmp);
 
     qmp_marshal_query_version(NULL, &ver, NULL);
+    qdict_put_obj(qmp, "version", ver);
+

Quoted 30 lines · 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 <[email protected]>

---
 monitor.c        | 15 +++++++++++++--
 qapi-schema.json | 13 +++++++++++++
 tests/qmp-test.c | 10 +++++++++-
 3 files changed, 35 insertions(+), 3 deletions(-)

monitor.c +7 -0

index e8f5a586e4..bad6ee8dd1 100644
@@ -3944,12 +3944,23 @@ void monitor_resume(Monitor *mon)
 
 static QObject *get_qmp_greeting(void)
 {
+    QDict *result = qdict_new(), *qmp = qdict_new();
+    QList *cap_list = qlist_new();
     QObject *ver = NULL;
+    QMPCapability cap;
+
+    qdict_put(result, "QMP", qmp);
 
     qmp_marshal_query_version(NULL, &ver, NULL);
+    qdict_put_obj(qmp, "version", ver);
+

monitor.c void monitor_resume(Monitor *mon) · line 3964

Quoted 10 lines

monitor.c +6 -2

+    for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
+        qlist_append(cap_list, qstring_from_str(
+                         QMPCapability_str(cap)));
+    }
+    qdict_put(qmp, "capabilities", cap_list);
 
-    return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': []}}",
-                              ver);
+    return QOBJECT(result);
 }

Why did you replace qobject_from_jsonf() with manual qdict_*() calls?

I was expecting this (it's shorter and easier to read):

  static QObject *get_qmp_greeting(void)
  {
      QList *cap_list = qlist_new();
      QObject *ver = NULL;
      QMPCapability cap;

      qmp_marshal_query_version(NULL, &ver, NULL);

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

  }

Quoted 52 lines · Stefan Hajnoczi

Quoted 30 lines · 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 <[email protected]>

---
 monitor.c        | 15 +++++++++++++--
 qapi-schema.json | 13 +++++++++++++
 tests/qmp-test.c | 10 +++++++++-
 3 files changed, 35 insertions(+), 3 deletions(-)

monitor.c +7 -0

index e8f5a586e4..bad6ee8dd1 100644
@@ -3944,12 +3944,23 @@ void monitor_resume(Monitor *mon)
 
 static QObject *get_qmp_greeting(void)
 {
+    QDict *result = qdict_new(), *qmp = qdict_new();
+    QList *cap_list = qlist_new();
     QObject *ver = NULL;
+    QMPCapability cap;
+
+    qdict_put(result, "QMP", qmp);
 
     qmp_marshal_query_version(NULL, &ver, NULL);
+    qdict_put_obj(qmp, "version", ver);
+

monitor.c void monitor_resume(Monitor *mon) · line 3964

Quoted 10 lines

monitor.c +6 -2

+    for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
+        qlist_append(cap_list, qstring_from_str(
+                         QMPCapability_str(cap)));
+    }
+    qdict_put(qmp, "capabilities", cap_list);
 
-    return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': []}}",
-                              ver);
+    return QOBJECT(result);
 }

Why did you replace qobject_from_jsonf() with manual qdict_*() calls?

I was expecting this (it's shorter and easier to read):

  static QObject *get_qmp_greeting(void)
  {
      QList *cap_list = qlist_new();
      QObject *ver = NULL;
      QMPCapability cap;

Quoted 5 lines

      qmp_marshal_query_version(NULL, &ver, NULL);

      for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
          qlist_append(cap_list, qstring_from_str(
                           QMPCapability_str(cap)));

Quoted 4 lines

      }

      return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': %p}}",
                                ver, cap);

(I believe you mean s/cap/cap_list/ here?)

Quoted 1 line

  }

Oh I just didn't notice that "%p" magic at all... :(

I think for me it's fine in either way. Frankly speaking creating the objects explicitly would be even easier to understand for me instead of using a mixture of two ways... But just let me know if you want me to do it your way. I can switch. Thanks,

Quoted 53 lines · Peter Xu

Quoted 52 lines · Stefan Hajnoczi

Quoted 30 lines · 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 <[email protected]>

---
 monitor.c        | 15 +++++++++++++--
 qapi-schema.json | 13 +++++++++++++
 tests/qmp-test.c | 10 +++++++++-
 3 files changed, 35 insertions(+), 3 deletions(-)

monitor.c +7 -0

index e8f5a586e4..bad6ee8dd1 100644
@@ -3944,12 +3944,23 @@ void monitor_resume(Monitor *mon)
 
 static QObject *get_qmp_greeting(void)
 {
+    QDict *result = qdict_new(), *qmp = qdict_new();
+    QList *cap_list = qlist_new();
     QObject *ver = NULL;
+    QMPCapability cap;
+
+    qdict_put(result, "QMP", qmp);
 
     qmp_marshal_query_version(NULL, &ver, NULL);
+    qdict_put_obj(qmp, "version", ver);
+

monitor.c void monitor_resume(Monitor *mon) · line 3964

Quoted 10 lines

monitor.c +6 -2

+    for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
+        qlist_append(cap_list, qstring_from_str(
+                         QMPCapability_str(cap)));
+    }
+    qdict_put(qmp, "capabilities", cap_list);
 
-    return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': []}}",
-                              ver);
+    return QOBJECT(result);
 }

Why did you replace qobject_from_jsonf() with manual qdict_*() calls?

I was expecting this (it's shorter and easier to read):

  static QObject *get_qmp_greeting(void)
  {
      QList *cap_list = qlist_new();
      QObject *ver = NULL;
      QMPCapability cap;

Quoted 5 lines

Quoted 5 lines

      qmp_marshal_query_version(NULL, &ver, NULL);

      for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
          qlist_append(cap_list, qstring_from_str(
                           QMPCapability_str(cap)));

And aligning the parameters would be even nicer.

Quoted 11 lines

Quoted 4 lines

      }

      return qobject_from_jsonf("{'QMP': {'version': %p, 'capabilities': %p}}",
                                ver, cap);

(I believe you mean s/cap/cap_list/ here?)

Quoted 1 line

  }

Oh I just didn't notice that "%p" magic at all... :(

Quoted 4 lines

I think for me it's fine in either way. Frankly speaking creating the objects explicitly would be even easier to understand for me instead of using a mixture of two ways... But just let me know if you want me to do it your way. I can switch. Thanks,

I agree with Stefan here. (Readability is not judged based on how low level the code goes when there is a higher level interface available, it's exactly the opposite, and this doesn't change even when you happen to not know it.)

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.

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c        | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 qapi-schema.json | 15 ++++++++++++---
 2 files changed, 68 insertions(+), 4 deletions(-)

monitor.c +56 -1

index bad6ee8dd1..097312e65f 100644
@@ -167,6 +167,7 @@ typedef struct {
      * mode.
      */
     QmpCommandList *commands;
+    bool qmp_caps[QMP_CAPABILITY__MAX];
 } MonitorQMP;
 
 /*

@@ -1037,8 +1038,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 "

@@ -1046,6 +1081,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;
 }
 

@@ -3963,6 +4012,11 @@ static QObject *get_qmp_greeting(void)
     return QOBJECT(result);
 }
 
+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;

@@ -3971,6 +4025,7 @@ static void monitor_qmp_event(void *opaque, int event)
     switch (event) {
     case CHR_EVENT_OPENED:
         mon->qmp.commands = &qmp_cap_negotiation_commands;
+        monitor_qmp_caps_reset(mon);
         data = get_qmp_greeting();
         monitor_json_emitter(mon, data);
         qobject_decref(data);

qapi-schema.json +12 -3

index 03201578b4..531fd4c0db 100644
@@ -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:

Signature · 1 line

2.14.3

monitor.c +7 -1

@@ -1037,8 +1038,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:

Quoted 18 lines · Peter Xu

monitor.c +13 -1

@@ -1037,8 +1038,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 "

monitor.c static void monitor_init_qmp_commands(void) · line 1057

Quoted 4 lines

monitor.c +4 -0

+                           "Out-Of-Band (OOB)");
+                return;
+            }
+            break;

QEMU always offers the 'oob' capability, even if the monitor does not support it. Should it send 'oob' only when mon->use_io_thr to make things easier for clients?

Quoted 19 lines · Stefan Hajnoczi

Quoted 18 lines · Peter Xu

monitor.c +13 -1

@@ -1037,8 +1038,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 "

Quoted 8 lines

monitor.c static void monitor_init_qmp_commands(void) · line 1057

Quoted 4 lines

monitor.c +4 -0

+                           "Out-Of-Band (OOB)");
+                return;
+            }
+            break;

QEMU always offers the 'oob' capability, even if the monitor does not support it. Should it send 'oob' only when mon->use_io_thr to make things easier for clients?

So should we firstly agree on whether the capabilities is on the current monitor connection or QEMU as a whole?

Quoted 6 lines

Quoted 3 lines

QEMU always offers the 'oob' capability, even if the monitor does not support it. Should it send 'oob' only when mon->use_io_thr to make things easier for clients?

So should we firstly agree on whether the capabilities is on the current monitor connection or QEMU as a whole?

It's more flexible to allow per-connection capabilities. Is there a reason against it?

Quoted 3 lines · Stefan Hajnoczi

Quoted 3 lines

Quoted 3 lines

QEMU always offers the 'oob' capability, even if the monitor does not support it. Should it send 'oob' only when mon->use_io_thr to make things easier for clients?

Quoted 6 lines

Quoted 3 lines

So should we firstly agree on whether the capabilities is on the current monitor connection or QEMU as a whole?

It's more flexible to allow per-connection capabilities. Is there a reason against it?

No, I just think either way we should document it. So if we define it per-connection, like you said, "oob" shouldn't be sent in the greeting message, and patch 11 need to be updated.

Introduce qmp_cap_enabled() and qmp_oob_enabled() helpers.

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

monitor.c +10 -0

index 097312e65f..9666115259 100644
@@ -1038,6 +1038,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)
 {

Signature · 1 line

2.14.3

Introduce qmp_cap_enabled() and qmp_oob_enabled() helpers.

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Introduce qmp_cap_enabled() and qmp_oob_enabled() helpers.

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

monitor.c +10 -0

index 097312e65f..9666115259 100644
@@ -1038,6 +1038,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)
 {

Signature · 1 line

2.14.3

Quoted 31 lines · Peter Xu

Introduce qmp_cap_enabled() and qmp_oob_enabled() helpers.

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

monitor.c +10 -0

index 097312e65f..9666115259 100644
@@ -1038,6 +1038,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)
 {

Signature · 1 line

2.14.3

NACK, this commit doesn't commit for me:

/home/fam/work/qemu/monitor.c:1046:13: error: ‘qmp_oob_enabled’ defined but not used [-Werror=unused-function]
static bool qmp_oob_enabled(Monitor *mon)

             ^~~~~~~~~~~~~~~

cc1: all warnings being treated as errors
CC x86_64-softmmu/accel/stubs/hax-stub.o

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

Reviewed-by Fam Zheng <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 48 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 17 deletions(-)

monitor.c +0 -0

index 9666115259..35d8925636 100644

Quoted 13 lines · Peter Xu

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

Reviewed-by Fam Zheng <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 48 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 17 deletions(-)

monitor.c +0 -0

index 9666115259..35d8925636 100644

monitor.c static int monitor_can_read(void *opaque) · line 3883

Quoted 7 lines

monitor.c +4 -0

@@ -3877,6 +3877,36 @@ static int monitor_can_read(void *opaque)
     return (mon->suspend_cnt == 0) ? 1 : 0;
 }
 
+/*
+ * When rsp/err/id is passed in, the function will be responsible for
+ * the cleanup.
+ */

Please document this function fully. I had to look at the implementation to learn how the arguments work:

1. This function takes ownership of rsp, err, and id. (Please use this exact wording because the function does not perform the "cleanup" when refcount > 1.)
2. rsp, err, and id may be NULL.
3. If err != NULL then rsp must be NULL.

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 <[email protected]>

---
 monitor.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Quoted 2 lines · Peter Xu

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

Please add doc comments to the functions explaining which thread they may be called from (especially public functions). Without this information other people will have a hard time keeping code thread-safe.

Quoted 5 lines

Signed-off-by Peter Xu <[email protected]>

---
 monitor.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

From docs/devel/atomics.txt:

Macros defined by qemu/atomic.h fall in three camps:

- compiler barriers: barrier();

- weak atomic access and manual memory barriers: atomic_read(),

    atomic_set(), smp_rmb(), smp_wmb(), smp_mb(), smp_mb_acquire(),
    smp_mb_release(), smp_read_barrier_depends();

- sequentially consistent atomic access: everything else.

The atomic_read() and atomic_set() calls in this patch aren't effective without memory barriers. Please use atomic_mb_read() and atomic_mb_set() instead.

Quoted 7 lines · Stefan Hajnoczi

Quoted 2 lines · Peter Xu

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

Please add doc comments to the functions explaining which thread they may be called from (especially public functions). Without this information other people will have a hard time keeping code thread-safe.

The two public functions would be monitor_resume() and monitor_suspend(). IIUC as long as these functions are using atomic operations correctly they should be able to be called anywhere?

Could you help point out what I missed?

Quoted 9 lines · Peter Xu

Quoted 7 lines · Stefan Hajnoczi

Quoted 2 lines · Peter Xu

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

Please add doc comments to the functions explaining which thread they may be called from (especially public functions). Without this information other people will have a hard time keeping code thread-safe.

Quoted 5 lines

The two public functions would be monitor_resume() and monitor_suspend(). IIUC as long as these functions are using atomic operations correctly they should be able to be called anywhere?

Could you help point out what I missed?

I don't mean just the public functions. How are other people supposed to know which monitor function executes in which thread without first studying all the monitor code? This is where comments, naming schemes, or grouping functions helps.

Not every function needs to be documented in detail, but there needs to be some structure to help people understand what happens in the IOThread and what happens in the main loop.

Just adding threading to the existing code without leaving clues for people who come after you is likely to result in future bugs.