Originally QMP goes through these steps:
JSON Parser --> QMP Dispatcher --> Respond
/|\ (2) (3) |
(1) | \|/ (4)
+--------- main thread --------+
This patch does this:
JSON Parser QMP Dispatcher --> Respond
/|\ | /|\ (4) |
| | (2) | (3) | (5)
(1) | +-----> | \|/
+--------- main thread <-------+
So the parsing job and the dispatching job is isolated now. It gives us
a chance in following up patches to totally move the parser outside.
The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is
used for all the monitors.
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
monitor.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 178 insertions(+), 23 deletions(-)
diff --git a/monitor.c b/monitor.c
index de9343be87..5104e5db07 100644
--- a/monitor.c
+++ b/monitor.c
@@ -172,6 +172,13 @@ typedef struct {
*/
QmpCommandList *commands;
bool qmp_caps[QMP_CAPABILITY__MAX];
+ /*
+ * Protects qmp request/response queue. Please take monitor_lock
+ * first when used together.
+ */
+ QemuMutex qmp_queue_lock;
+ /* Input queue that holds all the parsed QMP requests */
+ GQueue *qmp_requests;
} MonitorQMP;
/*
@@ -218,6 +225,8 @@ struct Monitor {
/* Let's add monitor global variables to this struct. */
static struct {
IOThread *mon_iothread;
+ /* Bottom half to dispatch the requests received from IO thread */
+ QEMUBH *qmp_dispatcher_bh;
} mon_global;
/* QMP checker flags */
@@ -600,11 +609,13 @@ static void monitor_data_init(Monitor *mon, bool skip_flush,
{
memset(mon, 0, sizeof(Monitor));
qemu_mutex_init(&mon->out_lock);
+ qemu_mutex_init(&mon->qmp.qmp_queue_lock);
mon->outbuf = qstring_new();
/* Use *mon_cmds by default. */
mon->cmd_table = mon_cmds;
mon->skip_flush = skip_flush;
mon->use_io_thr = use_io_thr;
+ mon->qmp.qmp_requests = g_queue_new();
}
static void monitor_data_destroy(Monitor *mon)
@@ -617,6 +628,8 @@ static void monitor_data_destroy(Monitor *mon)
readline_free(mon->rs);
QDECREF(mon->outbuf);
qemu_mutex_destroy(&mon->out_lock);
+ qemu_mutex_destroy(&mon->qmp.qmp_queue_lock);
+ g_queue_free(mon->qmp.qmp_requests);
}
char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
@@ -1056,6 +1069,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)
{
@@ -3866,30 +3889,39 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp,
qobject_decref(rsp);
}
-static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
+struct QMPRequest {
+ /* Owner of the request */
+ Monitor *mon;
+ /* "id" field of the request */
+ QObject *id;
+ /* Request object to be handled */
+ QObject *req;
+ /*
+ * Whether we need to resume the monitor afterward. This flag is
+ * used to emulate the old QMP server behavior that the current
+ * command must be completed before execution of the next one.
+ */
+ bool need_resume;
+};
+typedef struct QMPRequest QMPRequest;
+
+/*
+ * Dispatch one single QMP request. The function will free the req_obj
+ * and objects inside it before return.
+ */
+static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
{
- QObject *req, *rsp = NULL, *id = NULL;
+ Monitor *mon, *old_mon;
+ QObject *req, *rsp = NULL, *id;
QDict *qdict = NULL;
- MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
- Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
-
- Error *err = NULL;
+ bool need_resume;
- req = json_parser_parse_err(tokens, NULL, &err);
- if (!req && !err) {
- /* json_parser_parse_err() sucks: can fail without setting @err */
- error_setg(&err, QERR_JSON_PARSING);
- }
- if (err) {
- goto err_out;
- }
+ req = req_obj->req;
+ mon = req_obj->mon;
+ id = req_obj->id;
+ need_resume = req_obj->need_resume;
- qdict = qobject_to_qdict(req);
- if (qdict) {
- id = qdict_get(qdict, "id");
- qobject_incref(id);
- qdict_del(qdict, "id");
- } /* else will fail qmp_dispatch() */
+ g_free(req_obj);
if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
QString *req_json = qobject_to_json(req);
@@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
old_mon = cur_mon;
cur_mon = mon;
- rsp = qmp_dispatch(cur_mon->qmp.commands, req);
+ rsp = qmp_dispatch(mon->qmp.commands, req);
cur_mon = old_mon;
@@ -3916,12 +3948,122 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
}
}
-err_out:
- monitor_qmp_respond(mon, rsp, err, id);
+ /* Respond if necessary */
+ monitor_qmp_respond(mon, rsp, NULL, id);
+
+ /* This pairs with the monitor_suspend() in handle_qmp_command(). */
+ if (need_resume) {
+ monitor_resume(mon);
+ }
qobject_decref(req);
}
+/*
+ * Pop one QMP request from monitor queues, return NULL if not found.
+ * We are using round-robin fashion to pop the request, to avoid
+ * processing command only on a very busy monitor. To achieve that,
+ * when we processed one request on specific monitor, we put that
+ * monitor to the end of mon_list queue.
+ */
+static QMPRequest *monitor_qmp_requests_pop_one(void)
+{
+ QMPRequest *req_obj = NULL;
+ Monitor *mon;
+
+ qemu_mutex_lock(&monitor_lock);
+
+ QTAILQ_FOREACH(mon, &mon_list, entry) {
+ qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+ req_obj = g_queue_pop_head(mon->qmp.qmp_requests);
+ qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+ if (req_obj) {
+ break;
+ }
+ }
+
+ if (req_obj) {
+ /*
+ * We found one request on the monitor. Degrade this monitor's
+ * priority to lowest by re-inserting it to end of queue.
+ */
+ QTAILQ_REMOVE(&mon_list, mon, entry);
+ QTAILQ_INSERT_TAIL(&mon_list, mon, entry);
+ }
+
+ qemu_mutex_unlock(&monitor_lock);
+
+ return req_obj;
+}
+
+static void monitor_qmp_bh_dispatcher(void *data)
+{
+ QMPRequest *req_obj = monitor_qmp_requests_pop_one();
+
+ if (req_obj) {
+ monitor_qmp_dispatch_one(req_obj);
+ /* Reschedule instead of looping so the main loop stays responsive */
+ qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
+ }
+}
+
+static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
+{
+ QObject *req, *id = NULL;
+ QDict *qdict = NULL;
+ MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
+ Monitor *mon = container_of(mon_qmp, Monitor, qmp);
+ Error *err = NULL;
+ QMPRequest *req_obj;
+
+ req = json_parser_parse_err(tokens, NULL, &err);
+ if (!req && !err) {
+ /* json_parser_parse_err() sucks: can fail without setting @err */
+ error_setg(&err, QERR_JSON_PARSING);
+ }
+ if (err) {
+ monitor_qmp_respond(mon, NULL, err, NULL);
+ qobject_decref(req);
+ return;
+ }
+
+ qdict = qobject_to_qdict(req);
+ if (qdict) {
+ id = qdict_get(qdict, "id");
+ qobject_incref(id);
+ qdict_del(qdict, "id");
+ } /* else will fail qmp_dispatch() */
+
+ req_obj = g_new0(QMPRequest, 1);
+ req_obj->mon = mon;
+ req_obj->id = id;
+ req_obj->req = req;
+ req_obj->need_resume = false;
+
+ /*
+ * If OOB is not enabled on current monitor, we'll emulate the old
+ * behavior that we won't process current monitor any more until
+ * it is responded. This helps make sure that as long as OOB is
+ * not enabled, the server will never drop any command.
+ */
+ if (!qmp_oob_enabled(mon)) {
+ monitor_suspend(mon);
+ req_obj->need_resume = true;
+ }
+
+ /*
+ * Put the request to the end of queue so that requests will be
+ * handled in time order. Ownership for req_obj, req, id,
+ * etc. will be delivered to the handler side.
+ */
+ qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+ g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
+ qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+
+ /* Kick the dispatcher routine */
+ qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
+}
+
static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
{
Monitor *mon = opaque;
@@ -4134,6 +4276,15 @@ static void monitor_iothread_init(void)
{
mon_global.mon_iothread = iothread_create("mon_iothread",
&error_abort);
+
+ /*
+ * This MUST be on main loop thread since we have commands that
+ * have assumption to be run on main loop thread. It would be
+ * nice that one day we can remove this assumption in the future.
+ */
+ mon_global.qmp_dispatcher_bh = aio_bh_new(qemu_get_aio_context(),
+ monitor_qmp_bh_dispatcher,
+ NULL);
}
void monitor_init_globals(void)
@@ -4280,6 +4431,10 @@ void monitor_cleanup(void)
}
qemu_mutex_unlock(&monitor_lock);
+ /* QEMUBHs needs to be deleted before destroying the IOThread. */
+ qemu_bh_delete(mon_global.qmp_dispatcher_bh);
+ mon_global.qmp_dispatcher_bh = NULL;
+
iothread_destroy(mon_global.mon_iothread);
mon_global.mon_iothread = NULL;
}
--
2.14.3
On 03/09/2018 02:59 AM, Peter Xu wrote:
> Originally QMP goes through these steps:
>
> JSON Parser --> QMP Dispatcher --> Respond
> /|\ (2) (3) |
> (1) | \|/ (4)
> +--------- main thread --------+
>
> This patch does this:
>
> JSON Parser QMP Dispatcher --> Respond
> /|\ | /|\ (4) |
> | | (2) | (3) | (5)
> (1) | +-----> | \|/
> +--------- main thread <-------+
>
> So the parsing job and the dispatching job is isolated now. It gives us
> a chance in following up patches to totally move the parser outside.
s/following/follow/
>
> The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is
> used for all the monitors.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> monitor.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 178 insertions(+), 23 deletions(-)
>
>
> if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
> QString *req_json = qobject_to_json(req);
More context conflicts.
> +/*
> + * Pop one QMP request from monitor queues, return NULL if not found.
> + * We are using round-robin fashion to pop the request, to avoid
> + * processing command only on a very busy monitor. To achieve that,
s/command/commands/
> + * when we processed one request on specific monitor, we put that
s/processed/process/
s/on/on a/
> + * monitor to the end of mon_list queue.
> + */
> +static QMPRequest *monitor_qmp_requests_pop_one(void)
> +
> + /*
> + * If OOB is not enabled on current monitor, we'll emulate the old
> + * behavior that we won't process current monitor any more until
s/current/the current/g
> + * it is responded. This helps make sure that as long as OOB is
/is responded/has responded/
> + * not enabled, the server will never drop any command.
> + */
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Hi
On Fri, Mar 9, 2018 at 9:59 AM, Peter Xu <peterx@redhat.com> wrote:
> Originally QMP goes through these steps:
>
> JSON Parser --> QMP Dispatcher --> Respond
> /|\ (2) (3) |
> (1) | \|/ (4)
> +--------- main thread --------+
>
> This patch does this:
>
> JSON Parser QMP Dispatcher --> Respond
> /|\ | /|\ (4) |
> | | (2) | (3) | (5)
> (1) | +-----> | \|/
> +--------- main thread <-------+
>
> So the parsing job and the dispatching job is isolated now. It gives us
> a chance in following up patches to totally move the parser outside.
>
> The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is
> used for all the monitors.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> monitor.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 178 insertions(+), 23 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index de9343be87..5104e5db07 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -172,6 +172,13 @@ typedef struct {
> */
> QmpCommandList *commands;
> bool qmp_caps[QMP_CAPABILITY__MAX];
> + /*
> + * Protects qmp request/response queue. Please take monitor_lock
> + * first when used together.
> + */
> + QemuMutex qmp_queue_lock;
> + /* Input queue that holds all the parsed QMP requests */
> + GQueue *qmp_requests;
> } MonitorQMP;
>
> /*
> @@ -218,6 +225,8 @@ struct Monitor {
> /* Let's add monitor global variables to this struct. */
> static struct {
> IOThread *mon_iothread;
> + /* Bottom half to dispatch the requests received from IO thread */
> + QEMUBH *qmp_dispatcher_bh;
> } mon_global;
>
> /* QMP checker flags */
> @@ -600,11 +609,13 @@ static void monitor_data_init(Monitor *mon, bool skip_flush,
> {
> memset(mon, 0, sizeof(Monitor));
> qemu_mutex_init(&mon->out_lock);
> + qemu_mutex_init(&mon->qmp.qmp_queue_lock);
> mon->outbuf = qstring_new();
> /* Use *mon_cmds by default. */
> mon->cmd_table = mon_cmds;
> mon->skip_flush = skip_flush;
> mon->use_io_thr = use_io_thr;
> + mon->qmp.qmp_requests = g_queue_new();
> }
>
> static void monitor_data_destroy(Monitor *mon)
> @@ -617,6 +628,8 @@ static void monitor_data_destroy(Monitor *mon)
> readline_free(mon->rs);
> QDECREF(mon->outbuf);
> qemu_mutex_destroy(&mon->out_lock);
> + qemu_mutex_destroy(&mon->qmp.qmp_queue_lock);
> + g_queue_free(mon->qmp.qmp_requests);
> }
>
> char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
> @@ -1056,6 +1069,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)
> {
> @@ -3866,30 +3889,39 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp,
> qobject_decref(rsp);
> }
>
> -static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> +struct QMPRequest {
> + /* Owner of the request */
> + Monitor *mon;
> + /* "id" field of the request */
> + QObject *id;
> + /* Request object to be handled */
> + QObject *req;
> + /*
> + * Whether we need to resume the monitor afterward. This flag is
> + * used to emulate the old QMP server behavior that the current
> + * command must be completed before execution of the next one.
> + */
> + bool need_resume;
> +};
> +typedef struct QMPRequest QMPRequest;
> +
> +/*
> + * Dispatch one single QMP request. The function will free the req_obj
> + * and objects inside it before return.
> + */
> +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> {
> - QObject *req, *rsp = NULL, *id = NULL;
> + Monitor *mon, *old_mon;
> + QObject *req, *rsp = NULL, *id;
> QDict *qdict = NULL;
> - MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
> - Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
> -
> - Error *err = NULL;
> + bool need_resume;
>
> - req = json_parser_parse_err(tokens, NULL, &err);
> - if (!req && !err) {
> - /* json_parser_parse_err() sucks: can fail without setting @err */
> - error_setg(&err, QERR_JSON_PARSING);
> - }
> - if (err) {
> - goto err_out;
> - }
> + req = req_obj->req;
> + mon = req_obj->mon;
> + id = req_obj->id;
> + need_resume = req_obj->need_resume;
>
> - qdict = qobject_to_qdict(req);
> - if (qdict) {
> - id = qdict_get(qdict, "id");
> - qobject_incref(id);
> - qdict_del(qdict, "id");
> - } /* else will fail qmp_dispatch() */
> + g_free(req_obj);
>
> if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
> QString *req_json = qobject_to_json(req);
> @@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> old_mon = cur_mon;
> cur_mon = mon;
>
> - rsp = qmp_dispatch(cur_mon->qmp.commands, req);
> + rsp = qmp_dispatch(mon->qmp.commands, req);
>
> cur_mon = old_mon;
>
> @@ -3916,12 +3948,122 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> }
> }
>
> -err_out:
> - monitor_qmp_respond(mon, rsp, err, id);
> + /* Respond if necessary */
> + monitor_qmp_respond(mon, rsp, NULL, id);
> +
> + /* This pairs with the monitor_suspend() in handle_qmp_command(). */
> + if (need_resume) {
> + monitor_resume(mon);
> + }
>
> qobject_decref(req);
> }
>
> +/*
> + * Pop one QMP request from monitor queues, return NULL if not found.
> + * We are using round-robin fashion to pop the request, to avoid
> + * processing command only on a very busy monitor. To achieve that,
> + * when we processed one request on specific monitor, we put that
> + * monitor to the end of mon_list queue.
> + */
> +static QMPRequest *monitor_qmp_requests_pop_one(void)
> +{
> + QMPRequest *req_obj = NULL;
> + Monitor *mon;
> +
> + qemu_mutex_lock(&monitor_lock);
> +
> + QTAILQ_FOREACH(mon, &mon_list, entry) {
> + qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> + req_obj = g_queue_pop_head(mon->qmp.qmp_requests);
> + qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> + if (req_obj) {
> + break;
> + }
> + }
> +
> + if (req_obj) {
> + /*
> + * We found one request on the monitor. Degrade this monitor's
> + * priority to lowest by re-inserting it to end of queue.
> + */
> + QTAILQ_REMOVE(&mon_list, mon, entry);
> + QTAILQ_INSERT_TAIL(&mon_list, mon, entry);
> + }
> +
> + qemu_mutex_unlock(&monitor_lock);
> +
> + return req_obj;
> +}
> +
> +static void monitor_qmp_bh_dispatcher(void *data)
> +{
> + QMPRequest *req_obj = monitor_qmp_requests_pop_one();
> +
> + if (req_obj) {
> + monitor_qmp_dispatch_one(req_obj);
> + /* Reschedule instead of looping so the main loop stays responsive */
> + qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> + }
> +}
> +
> +static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> +{
> + QObject *req, *id = NULL;
> + QDict *qdict = NULL;
> + MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
> + Monitor *mon = container_of(mon_qmp, Monitor, qmp);
> + Error *err = NULL;
> + QMPRequest *req_obj;
> +
> + req = json_parser_parse_err(tokens, NULL, &err);
> + if (!req && !err) {
> + /* json_parser_parse_err() sucks: can fail without setting @err */
> + error_setg(&err, QERR_JSON_PARSING);
> + }
> + if (err) {
> + monitor_qmp_respond(mon, NULL, err, NULL);
> + qobject_decref(req);
> + return;
> + }
> +
> + qdict = qobject_to_qdict(req);
> + if (qdict) {
> + id = qdict_get(qdict, "id");
> + qobject_incref(id);
> + qdict_del(qdict, "id");
> + } /* else will fail qmp_dispatch() */
> +
> + req_obj = g_new0(QMPRequest, 1);
> + req_obj->mon = mon;
> + req_obj->id = id;
> + req_obj->req = req;
> + req_obj->need_resume = false;
> +
> + /*
> + * If OOB is not enabled on current monitor, we'll emulate the old
> + * behavior that we won't process current monitor any more until
> + * it is responded. This helps make sure that as long as OOB is
> + * not enabled, the server will never drop any command.
> + */
> + if (!qmp_oob_enabled(mon)) {
> + monitor_suspend(mon);
> + req_obj->need_resume = true;
> + }
> +
> + /*
> + * Put the request to the end of queue so that requests will be
> + * handled in time order. Ownership for req_obj, req, id,
I think the order is not respected if subsequent messages have errors
(in either json parsing, dispatch_check_obj, oob_check). So if I
enable oob, and queue a few command, then send a bad command/message,
I won't be able to tell for which command.
> + * etc. will be delivered to the handler side.
> + */
> + qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> + g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
> + qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> +
> + /* Kick the dispatcher routine */
> + qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> +}
> +
> static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
> {
> Monitor *mon = opaque;
> @@ -4134,6 +4276,15 @@ static void monitor_iothread_init(void)
> {
> mon_global.mon_iothread = iothread_create("mon_iothread",
> &error_abort);
> +
> + /*
> + * This MUST be on main loop thread since we have commands that
> + * have assumption to be run on main loop thread. It would be
> + * nice that one day we can remove this assumption in the future.
> + */
> + mon_global.qmp_dispatcher_bh = aio_bh_new(qemu_get_aio_context(),
> + monitor_qmp_bh_dispatcher,
> + NULL);
> }
>
> void monitor_init_globals(void)
> @@ -4280,6 +4431,10 @@ void monitor_cleanup(void)
> }
> qemu_mutex_unlock(&monitor_lock);
>
> + /* QEMUBHs needs to be deleted before destroying the IOThread. */
> + qemu_bh_delete(mon_global.qmp_dispatcher_bh);
> + mon_global.qmp_dispatcher_bh = NULL;
> +
> iothread_destroy(mon_global.mon_iothread);
> mon_global.mon_iothread = NULL;
> }
> --
> 2.14.3
>
>
--
Marc-André Lureau
* Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> Hi
>
> On Fri, Mar 9, 2018 at 9:59 AM, Peter Xu <peterx@redhat.com> wrote:
> > Originally QMP goes through these steps:
> >
> > JSON Parser --> QMP Dispatcher --> Respond
> > /|\ (2) (3) |
> > (1) | \|/ (4)
> > +--------- main thread --------+
> >
> > This patch does this:
> >
> > JSON Parser QMP Dispatcher --> Respond
> > /|\ | /|\ (4) |
> > | | (2) | (3) | (5)
> > (1) | +-----> | \|/
> > +--------- main thread <-------+
> >
> > So the parsing job and the dispatching job is isolated now. It gives us
> > a chance in following up patches to totally move the parser outside.
> >
> > The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is
> > used for all the monitors.
> >
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > monitor.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 178 insertions(+), 23 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index de9343be87..5104e5db07 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -172,6 +172,13 @@ typedef struct {
> > */
> > QmpCommandList *commands;
> > bool qmp_caps[QMP_CAPABILITY__MAX];
> > + /*
> > + * Protects qmp request/response queue. Please take monitor_lock
> > + * first when used together.
> > + */
> > + QemuMutex qmp_queue_lock;
> > + /* Input queue that holds all the parsed QMP requests */
> > + GQueue *qmp_requests;
> > } MonitorQMP;
> >
> > /*
> > @@ -218,6 +225,8 @@ struct Monitor {
> > /* Let's add monitor global variables to this struct. */
> > static struct {
> > IOThread *mon_iothread;
> > + /* Bottom half to dispatch the requests received from IO thread */
> > + QEMUBH *qmp_dispatcher_bh;
> > } mon_global;
> >
> > /* QMP checker flags */
> > @@ -600,11 +609,13 @@ static void monitor_data_init(Monitor *mon, bool skip_flush,
> > {
> > memset(mon, 0, sizeof(Monitor));
> > qemu_mutex_init(&mon->out_lock);
> > + qemu_mutex_init(&mon->qmp.qmp_queue_lock);
> > mon->outbuf = qstring_new();
> > /* Use *mon_cmds by default. */
> > mon->cmd_table = mon_cmds;
> > mon->skip_flush = skip_flush;
> > mon->use_io_thr = use_io_thr;
> > + mon->qmp.qmp_requests = g_queue_new();
> > }
> >
> > static void monitor_data_destroy(Monitor *mon)
> > @@ -617,6 +628,8 @@ static void monitor_data_destroy(Monitor *mon)
> > readline_free(mon->rs);
> > QDECREF(mon->outbuf);
> > qemu_mutex_destroy(&mon->out_lock);
> > + qemu_mutex_destroy(&mon->qmp.qmp_queue_lock);
> > + g_queue_free(mon->qmp.qmp_requests);
> > }
> >
> > char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
> > @@ -1056,6 +1069,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)
> > {
> > @@ -3866,30 +3889,39 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp,
> > qobject_decref(rsp);
> > }
> >
> > -static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> > +struct QMPRequest {
> > + /* Owner of the request */
> > + Monitor *mon;
> > + /* "id" field of the request */
> > + QObject *id;
> > + /* Request object to be handled */
> > + QObject *req;
> > + /*
> > + * Whether we need to resume the monitor afterward. This flag is
> > + * used to emulate the old QMP server behavior that the current
> > + * command must be completed before execution of the next one.
> > + */
> > + bool need_resume;
> > +};
> > +typedef struct QMPRequest QMPRequest;
> > +
> > +/*
> > + * Dispatch one single QMP request. The function will free the req_obj
> > + * and objects inside it before return.
> > + */
> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> > {
> > - QObject *req, *rsp = NULL, *id = NULL;
> > + Monitor *mon, *old_mon;
> > + QObject *req, *rsp = NULL, *id;
> > QDict *qdict = NULL;
> > - MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
> > - Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
> > -
> > - Error *err = NULL;
> > + bool need_resume;
> >
> > - req = json_parser_parse_err(tokens, NULL, &err);
> > - if (!req && !err) {
> > - /* json_parser_parse_err() sucks: can fail without setting @err */
> > - error_setg(&err, QERR_JSON_PARSING);
> > - }
> > - if (err) {
> > - goto err_out;
> > - }
> > + req = req_obj->req;
> > + mon = req_obj->mon;
> > + id = req_obj->id;
> > + need_resume = req_obj->need_resume;
> >
> > - qdict = qobject_to_qdict(req);
> > - if (qdict) {
> > - id = qdict_get(qdict, "id");
> > - qobject_incref(id);
> > - qdict_del(qdict, "id");
> > - } /* else will fail qmp_dispatch() */
> > + g_free(req_obj);
> >
> > if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
> > QString *req_json = qobject_to_json(req);
> > @@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> > old_mon = cur_mon;
> > cur_mon = mon;
> >
> > - rsp = qmp_dispatch(cur_mon->qmp.commands, req);
> > + rsp = qmp_dispatch(mon->qmp.commands, req);
> >
> > cur_mon = old_mon;
> >
> > @@ -3916,12 +3948,122 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> > }
> > }
> >
> > -err_out:
> > - monitor_qmp_respond(mon, rsp, err, id);
> > + /* Respond if necessary */
> > + monitor_qmp_respond(mon, rsp, NULL, id);
> > +
> > + /* This pairs with the monitor_suspend() in handle_qmp_command(). */
> > + if (need_resume) {
> > + monitor_resume(mon);
> > + }
> >
> > qobject_decref(req);
> > }
> >
> > +/*
> > + * Pop one QMP request from monitor queues, return NULL if not found.
> > + * We are using round-robin fashion to pop the request, to avoid
> > + * processing command only on a very busy monitor. To achieve that,
> > + * when we processed one request on specific monitor, we put that
> > + * monitor to the end of mon_list queue.
> > + */
> > +static QMPRequest *monitor_qmp_requests_pop_one(void)
> > +{
> > + QMPRequest *req_obj = NULL;
> > + Monitor *mon;
> > +
> > + qemu_mutex_lock(&monitor_lock);
> > +
> > + QTAILQ_FOREACH(mon, &mon_list, entry) {
> > + qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> > + req_obj = g_queue_pop_head(mon->qmp.qmp_requests);
> > + qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> > + if (req_obj) {
> > + break;
> > + }
> > + }
> > +
> > + if (req_obj) {
> > + /*
> > + * We found one request on the monitor. Degrade this monitor's
> > + * priority to lowest by re-inserting it to end of queue.
> > + */
> > + QTAILQ_REMOVE(&mon_list, mon, entry);
> > + QTAILQ_INSERT_TAIL(&mon_list, mon, entry);
> > + }
> > +
> > + qemu_mutex_unlock(&monitor_lock);
> > +
> > + return req_obj;
> > +}
> > +
> > +static void monitor_qmp_bh_dispatcher(void *data)
> > +{
> > + QMPRequest *req_obj = monitor_qmp_requests_pop_one();
> > +
> > + if (req_obj) {
> > + monitor_qmp_dispatch_one(req_obj);
> > + /* Reschedule instead of looping so the main loop stays responsive */
> > + qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> > + }
> > +}
> > +
> > +static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> > +{
> > + QObject *req, *id = NULL;
> > + QDict *qdict = NULL;
> > + MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
> > + Monitor *mon = container_of(mon_qmp, Monitor, qmp);
> > + Error *err = NULL;
> > + QMPRequest *req_obj;
> > +
> > + req = json_parser_parse_err(tokens, NULL, &err);
> > + if (!req && !err) {
> > + /* json_parser_parse_err() sucks: can fail without setting @err */
> > + error_setg(&err, QERR_JSON_PARSING);
> > + }
> > + if (err) {
> > + monitor_qmp_respond(mon, NULL, err, NULL);
> > + qobject_decref(req);
> > + return;
> > + }
> > +
> > + qdict = qobject_to_qdict(req);
> > + if (qdict) {
> > + id = qdict_get(qdict, "id");
> > + qobject_incref(id);
> > + qdict_del(qdict, "id");
> > + } /* else will fail qmp_dispatch() */
> > +
> > + req_obj = g_new0(QMPRequest, 1);
> > + req_obj->mon = mon;
> > + req_obj->id = id;
> > + req_obj->req = req;
> > + req_obj->need_resume = false;
> > +
> > + /*
> > + * If OOB is not enabled on current monitor, we'll emulate the old
> > + * behavior that we won't process current monitor any more until
> > + * it is responded. This helps make sure that as long as OOB is
> > + * not enabled, the server will never drop any command.
> > + */
> > + if (!qmp_oob_enabled(mon)) {
> > + monitor_suspend(mon);
> > + req_obj->need_resume = true;
> > + }
> > +
> > + /*
> > + * Put the request to the end of queue so that requests will be
> > + * handled in time order. Ownership for req_obj, req, id,
>
> I think the order is not respected if subsequent messages have errors
> (in either json parsing, dispatch_check_obj, oob_check). So if I
> enable oob, and queue a few command, then send a bad command/message,
> I won't be able to tell for which command.
Doesn't OOB insist on having an ID field with the command?
Dave
> > + * etc. will be delivered to the handler side.
> > + */
> > + qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> > + g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
> > + qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> > +
> > + /* Kick the dispatcher routine */
> > + qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> > +}
> > +
> > static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
> > {
> > Monitor *mon = opaque;
> > @@ -4134,6 +4276,15 @@ static void monitor_iothread_init(void)
> > {
> > mon_global.mon_iothread = iothread_create("mon_iothread",
> > &error_abort);
> > +
> > + /*
> > + * This MUST be on main loop thread since we have commands that
> > + * have assumption to be run on main loop thread. It would be
> > + * nice that one day we can remove this assumption in the future.
> > + */
> > + mon_global.qmp_dispatcher_bh = aio_bh_new(qemu_get_aio_context(),
> > + monitor_qmp_bh_dispatcher,
> > + NULL);
> > }
> >
> > void monitor_init_globals(void)
> > @@ -4280,6 +4431,10 @@ void monitor_cleanup(void)
> > }
> > qemu_mutex_unlock(&monitor_lock);
> >
> > + /* QEMUBHs needs to be deleted before destroying the IOThread. */
> > + qemu_bh_delete(mon_global.qmp_dispatcher_bh);
> > + mon_global.qmp_dispatcher_bh = NULL;
> > +
> > iothread_destroy(mon_global.mon_iothread);
> > mon_global.mon_iothread = NULL;
> > }
> > --
> > 2.14.3
> >
> >
>
>
>
> --
> Marc-André Lureau
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 03/21/2018 03:09 PM, Dr. David Alan Gilbert wrote:
>>>
>>> So the parsing job and the dispatching job is isolated now. It gives us
>>> a chance in following up patches to totally move the parser outside.
>>>
>>> The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is
>>> used for all the monitors.
>>>
>>> +
>>> + /*
>>> + * If OOB is not enabled on current monitor, we'll emulate the old
>>> + * behavior that we won't process current monitor any more until
>>> + * it is responded. This helps make sure that as long as OOB is
>>> + * not enabled, the server will never drop any command.
>>> + */
>>> + if (!qmp_oob_enabled(mon)) {
>>> + monitor_suspend(mon);
>>> + req_obj->need_resume = true;
>>> + }
>>> +
>>> + /*
>>> + * Put the request to the end of queue so that requests will be
>>> + * handled in time order. Ownership for req_obj, req, id,
>>
>> I think the order is not respected if subsequent messages have errors
>> (in either json parsing, dispatch_check_obj, oob_check). So if I
>> enable oob, and queue a few command, then send a bad command/message,
>> I won't be able to tell for which command.
>
> Doesn't OOB insist on having an ID field with the command?
OOB insists on an id field - but there is the situation that SOME errors
occur even before the id field has been encountered (for example, if you
send non-JSON, the parser gets all confused - it has to emit SOME error,
but that error can't refer to an id because it wasn't able to parse one
yet). A well-formed client will never do this, but we MUST be robust
even in the face of a malicious client (or even a well-intentioned
client but a noisy communication medium that manages to corrupt bytes).
I'm not sure if the testsuite adequately covers what happens in this
scenario. Peter?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Hi
On Wed, Mar 21, 2018 at 9:33 PM, Eric Blake <eblake@redhat.com> wrote:
> On 03/21/2018 03:09 PM, Dr. David Alan Gilbert wrote:
>
>>>>
>>>> So the parsing job and the dispatching job is isolated now. It gives us
>>>> a chance in following up patches to totally move the parser outside.
>>>>
>>>> The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is
>>>> used for all the monitors.
>>>>
>
>>>> +
>>>> + /*
>>>> + * If OOB is not enabled on current monitor, we'll emulate the old
>>>> + * behavior that we won't process current monitor any more until
>>>> + * it is responded. This helps make sure that as long as OOB is
>>>> + * not enabled, the server will never drop any command.
>>>> + */
>>>> + if (!qmp_oob_enabled(mon)) {
>>>> + monitor_suspend(mon);
>>>> + req_obj->need_resume = true;
>>>> + }
>>>> +
>>>> + /*
>>>> + * Put the request to the end of queue so that requests will be
>>>> + * handled in time order. Ownership for req_obj, req, id,
>>>
>>>
>>> I think the order is not respected if subsequent messages have errors
>>> (in either json parsing, dispatch_check_obj, oob_check). So if I
>>> enable oob, and queue a few command, then send a bad command/message,
>>> I won't be able to tell for which command.
>>
>>
>> Doesn't OOB insist on having an ID field with the command?
>
>
> OOB insists on an id field - but there is the situation that SOME errors
> occur even before the id field has been encountered (for example, if you
> send non-JSON, the parser gets all confused - it has to emit SOME error, but
> that error can't refer to an id because it wasn't able to parse one yet). A
> well-formed client will never do this, but we MUST be robust even in the
> face of a malicious client (or even a well-intentioned client but a noisy
> communication medium that manages to corrupt bytes). I'm not sure if the
> testsuite adequately covers what happens in this scenario. Peter?
I think a solution would be to queue the error reply (the reply only)
instead of replying directly.
Another problem I see with the current solution is that pending
commands are not discarded when a new client connects. So I suppose a
new client can receive replies for commands it didn't make, with id
namespace that may conflict with its own. breaking ordering etc. A
possible solution is to mark the pending request to not send the reply
somehow?
--
Marc-André Lureau
On Thu, Mar 22, 2018 at 12:32:36AM +0100, Marc-André Lureau wrote:
> Hi
>
> On Wed, Mar 21, 2018 at 9:33 PM, Eric Blake <eblake@redhat.com> wrote:
> > On 03/21/2018 03:09 PM, Dr. David Alan Gilbert wrote:
> >
> >>>>
> >>>> So the parsing job and the dispatching job is isolated now. It gives us
> >>>> a chance in following up patches to totally move the parser outside.
> >>>>
> >>>> The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is
> >>>> used for all the monitors.
> >>>>
> >
> >>>> +
> >>>> + /*
> >>>> + * If OOB is not enabled on current monitor, we'll emulate the old
> >>>> + * behavior that we won't process current monitor any more until
> >>>> + * it is responded. This helps make sure that as long as OOB is
> >>>> + * not enabled, the server will never drop any command.
> >>>> + */
> >>>> + if (!qmp_oob_enabled(mon)) {
> >>>> + monitor_suspend(mon);
> >>>> + req_obj->need_resume = true;
> >>>> + }
> >>>> +
> >>>> + /*
> >>>> + * Put the request to the end of queue so that requests will be
> >>>> + * handled in time order. Ownership for req_obj, req, id,
> >>>
> >>>
> >>> I think the order is not respected if subsequent messages have errors
> >>> (in either json parsing, dispatch_check_obj, oob_check). So if I
> >>> enable oob, and queue a few command, then send a bad command/message,
> >>> I won't be able to tell for which command.
> >>
> >>
> >> Doesn't OOB insist on having an ID field with the command?
> >
> >
> > OOB insists on an id field - but there is the situation that SOME errors
> > occur even before the id field has been encountered (for example, if you
> > send non-JSON, the parser gets all confused - it has to emit SOME error, but
> > that error can't refer to an id because it wasn't able to parse one yet). A
> > well-formed client will never do this, but we MUST be robust even in the
> > face of a malicious client (or even a well-intentioned client but a noisy
> > communication medium that manages to corrupt bytes). I'm not sure if the
> > testsuite adequately covers what happens in this scenario. Peter?
>
> I think a solution would be to queue the error reply (the reply only)
> instead of replying directly.
IMHO this should be fine.
Note that to be compatible with existing QMP we'll suspend the monitor
if OOB is not enabled in the session on receiving the first command.
It means even if another faulty command is sent after the good one,
it'll not be read by QMP parser until the first one is fully complete.
Since I'm working on some test patches after all, I'll try to add a
test case for this to see whether Eric would like them too.
>
> Another problem I see with the current solution is that pending
> commands are not discarded when a new client connects. So I suppose a
> new client can receive replies for commands it didn't make, with id
> namespace that may conflict with its own. breaking ordering etc. A
> possible solution is to mark the pending request to not send the reply
> somehow?
Yeah, to be simpler - maybe we can even drop the commands that have
not yet been dispatched?
I'll treat a command as "complete" only until the client receives a
response, otherwise if a client disconnects before receiving a reply
then I think it's fine the behavior is undefined - IMHO it's fine
either the QMP server executes the command or not (and no matter what,
we drop the responses). Would that work?
Thanks,
--
Peter Xu
On 03/22/2018 12:00 AM, Peter Xu wrote: >>>> Doesn't OOB insist on having an ID field with the command? >>> >>> >>> OOB insists on an id field - but there is the situation that SOME errors >>> occur even before the id field has been encountered (for example, if you >>> send non-JSON, the parser gets all confused - it has to emit SOME error, but >>> that error can't refer to an id because it wasn't able to parse one yet). A >>> well-formed client will never do this, but we MUST be robust even in the >>> face of a malicious client (or even a well-intentioned client but a noisy >>> communication medium that manages to corrupt bytes). I'm not sure if the >>> testsuite adequately covers what happens in this scenario. Peter? >> >> I think a solution would be to queue the error reply (the reply only) >> instead of replying directly. > > IMHO this should be fine. Seems reasonable - conceptually, the error reply comes no sooner than pre-patch (the client wouldn't see a parse error message until after all earlier successful parsed messages completed), making a parse error a sort of synchronization barrier (if the client gets one, then all outstanding message prior to the parse error have finished sending replies to the client, and nothing after the parse error has been processed at the time the error message was sent). > > Note that to be compatible with existing QMP we'll suspend the monitor > if OOB is not enabled in the session on receiving the first command. > It means even if another faulty command is sent after the good one, > it'll not be read by QMP parser until the first one is fully complete. > > Since I'm working on some test patches after all, I'll try to add a > test case for this to see whether Eric would like them too. Yes, these are the sorts of bug fixes and test suite coverage that we want to include during freeze, for better robustness. > >> >> Another problem I see with the current solution is that pending >> commands are not discarded when a new client connects. So I suppose a >> new client can receive replies for commands it didn't make, with id >> namespace that may conflict with its own. breaking ordering etc. A >> possible solution is to mark the pending request to not send the reply >> somehow? > > Yeah, to be simpler - maybe we can even drop the commands that have > not yet been dispatched? > > I'll treat a command as "complete" only until the client receives a > response, otherwise if a client disconnects before receiving a reply > then I think it's fine the behavior is undefined - IMHO it's fine > either the QMP server executes the command or not (and no matter what, > we drop the responses). Would that work? If a client disconnects before receiving a reply, I'm fine with either approach (the command gets discarded from the queue and never run - which matches if the client disconnect raced with the server actually getting the command, or the command ran but the response is dropped - which matches pre-patch behavior), while agreeing that sending the response to the next client is probably not a good idea (clients are supposed to ignore unknown id's, but we can't guarantee an 'id' collision will not confuse the client). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Fri, Mar 9, 2018 at 9:59 AM, Peter Xu <peterx@redhat.com> wrote:
> Originally QMP goes through these steps:
>
> JSON Parser --> QMP Dispatcher --> Respond
> /|\ (2) (3) |
> (1) | \|/ (4)
> +--------- main thread --------+
>
> This patch does this:
>
> JSON Parser QMP Dispatcher --> Respond
> /|\ | /|\ (4) |
> | | (2) | (3) | (5)
> (1) | +-----> | \|/
> +--------- main thread <-------+
>
> So the parsing job and the dispatching job is isolated now. It gives us
> a chance in following up patches to totally move the parser outside.
>
> The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is
> used for all the monitors.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> monitor.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 178 insertions(+), 23 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index de9343be87..5104e5db07 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -172,6 +172,13 @@ typedef struct {
> */
> QmpCommandList *commands;
> bool qmp_caps[QMP_CAPABILITY__MAX];
> + /*
> + * Protects qmp request/response queue. Please take monitor_lock
> + * first when used together.
> + */
> + QemuMutex qmp_queue_lock;
> + /* Input queue that holds all the parsed QMP requests */
> + GQueue *qmp_requests;
> } MonitorQMP;
>
> /*
> @@ -218,6 +225,8 @@ struct Monitor {
> /* Let's add monitor global variables to this struct. */
> static struct {
> IOThread *mon_iothread;
> + /* Bottom half to dispatch the requests received from IO thread */
> + QEMUBH *qmp_dispatcher_bh;
> } mon_global;
>
> /* QMP checker flags */
> @@ -600,11 +609,13 @@ static void monitor_data_init(Monitor *mon, bool skip_flush,
> {
> memset(mon, 0, sizeof(Monitor));
> qemu_mutex_init(&mon->out_lock);
> + qemu_mutex_init(&mon->qmp.qmp_queue_lock);
> mon->outbuf = qstring_new();
> /* Use *mon_cmds by default. */
> mon->cmd_table = mon_cmds;
> mon->skip_flush = skip_flush;
> mon->use_io_thr = use_io_thr;
> + mon->qmp.qmp_requests = g_queue_new();
> }
>
> static void monitor_data_destroy(Monitor *mon)
> @@ -617,6 +628,8 @@ static void monitor_data_destroy(Monitor *mon)
> readline_free(mon->rs);
> QDECREF(mon->outbuf);
> qemu_mutex_destroy(&mon->out_lock);
> + qemu_mutex_destroy(&mon->qmp.qmp_queue_lock);
> + g_queue_free(mon->qmp.qmp_requests);
> }
>
> char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
> @@ -1056,6 +1069,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)
> {
> @@ -3866,30 +3889,39 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp,
> qobject_decref(rsp);
> }
>
> -static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> +struct QMPRequest {
> + /* Owner of the request */
> + Monitor *mon;
> + /* "id" field of the request */
> + QObject *id;
> + /* Request object to be handled */
> + QObject *req;
> + /*
> + * Whether we need to resume the monitor afterward. This flag is
> + * used to emulate the old QMP server behavior that the current
> + * command must be completed before execution of the next one.
> + */
> + bool need_resume;
> +};
> +typedef struct QMPRequest QMPRequest;
> +
> +/*
> + * Dispatch one single QMP request. The function will free the req_obj
> + * and objects inside it before return.
> + */
> +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> {
> - QObject *req, *rsp = NULL, *id = NULL;
> + Monitor *mon, *old_mon;
> + QObject *req, *rsp = NULL, *id;
> QDict *qdict = NULL;
> - MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
> - Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
> -
> - Error *err = NULL;
> + bool need_resume;
>
> - req = json_parser_parse_err(tokens, NULL, &err);
> - if (!req && !err) {
> - /* json_parser_parse_err() sucks: can fail without setting @err */
> - error_setg(&err, QERR_JSON_PARSING);
> - }
> - if (err) {
> - goto err_out;
> - }
> + req = req_obj->req;
> + mon = req_obj->mon;
> + id = req_obj->id;
> + need_resume = req_obj->need_resume;
>
> - qdict = qobject_to_qdict(req);
> - if (qdict) {
> - id = qdict_get(qdict, "id");
> - qobject_incref(id);
> - qdict_del(qdict, "id");
> - } /* else will fail qmp_dispatch() */
> + g_free(req_obj);
>
> if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
> QString *req_json = qobject_to_json(req);
> @@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> old_mon = cur_mon;
> cur_mon = mon;
There is another issue with this series, since cur_mon is global (and
not protected), an oob command may change the cur_mon while another
command is running in the main thread with unexpected consequences. I
don't have a clear idea what is the best way to solve it. Making the
variable per-thread, or going all the way to get rid of cur_mon (my
preference, but much harder)
>
> - rsp = qmp_dispatch(cur_mon->qmp.commands, req);
> + rsp = qmp_dispatch(mon->qmp.commands, req);
>
> cur_mon = old_mon;
>
> @@ -3916,12 +3948,122 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> }
> }
>
> -err_out:
> - monitor_qmp_respond(mon, rsp, err, id);
> + /* Respond if necessary */
> + monitor_qmp_respond(mon, rsp, NULL, id);
> +
> + /* This pairs with the monitor_suspend() in handle_qmp_command(). */
> + if (need_resume) {
> + monitor_resume(mon);
> + }
>
> qobject_decref(req);
> }
>
> +/*
> + * Pop one QMP request from monitor queues, return NULL if not found.
> + * We are using round-robin fashion to pop the request, to avoid
> + * processing command only on a very busy monitor. To achieve that,
> + * when we processed one request on specific monitor, we put that
> + * monitor to the end of mon_list queue.
> + */
> +static QMPRequest *monitor_qmp_requests_pop_one(void)
> +{
> + QMPRequest *req_obj = NULL;
> + Monitor *mon;
> +
> + qemu_mutex_lock(&monitor_lock);
> +
> + QTAILQ_FOREACH(mon, &mon_list, entry) {
> + qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> + req_obj = g_queue_pop_head(mon->qmp.qmp_requests);
> + qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> + if (req_obj) {
> + break;
> + }
> + }
> +
> + if (req_obj) {
> + /*
> + * We found one request on the monitor. Degrade this monitor's
> + * priority to lowest by re-inserting it to end of queue.
> + */
> + QTAILQ_REMOVE(&mon_list, mon, entry);
> + QTAILQ_INSERT_TAIL(&mon_list, mon, entry);
> + }
> +
> + qemu_mutex_unlock(&monitor_lock);
> +
> + return req_obj;
> +}
> +
> +static void monitor_qmp_bh_dispatcher(void *data)
> +{
> + QMPRequest *req_obj = monitor_qmp_requests_pop_one();
> +
> + if (req_obj) {
> + monitor_qmp_dispatch_one(req_obj);
> + /* Reschedule instead of looping so the main loop stays responsive */
> + qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> + }
> +}
> +
> +static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> +{
> + QObject *req, *id = NULL;
> + QDict *qdict = NULL;
> + MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
> + Monitor *mon = container_of(mon_qmp, Monitor, qmp);
> + Error *err = NULL;
> + QMPRequest *req_obj;
> +
> + req = json_parser_parse_err(tokens, NULL, &err);
> + if (!req && !err) {
> + /* json_parser_parse_err() sucks: can fail without setting @err */
> + error_setg(&err, QERR_JSON_PARSING);
> + }
> + if (err) {
> + monitor_qmp_respond(mon, NULL, err, NULL);
> + qobject_decref(req);
> + return;
> + }
> +
> + qdict = qobject_to_qdict(req);
> + if (qdict) {
> + id = qdict_get(qdict, "id");
> + qobject_incref(id);
> + qdict_del(qdict, "id");
> + } /* else will fail qmp_dispatch() */
> +
> + req_obj = g_new0(QMPRequest, 1);
> + req_obj->mon = mon;
> + req_obj->id = id;
> + req_obj->req = req;
> + req_obj->need_resume = false;
> +
> + /*
> + * If OOB is not enabled on current monitor, we'll emulate the old
> + * behavior that we won't process current monitor any more until
> + * it is responded. This helps make sure that as long as OOB is
> + * not enabled, the server will never drop any command.
> + */
> + if (!qmp_oob_enabled(mon)) {
> + monitor_suspend(mon);
> + req_obj->need_resume = true;
> + }
> +
> + /*
> + * Put the request to the end of queue so that requests will be
> + * handled in time order. Ownership for req_obj, req, id,
> + * etc. will be delivered to the handler side.
> + */
> + qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> + g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
> + qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> +
> + /* Kick the dispatcher routine */
> + qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> +}
> +
> static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
> {
> Monitor *mon = opaque;
> @@ -4134,6 +4276,15 @@ static void monitor_iothread_init(void)
> {
> mon_global.mon_iothread = iothread_create("mon_iothread",
> &error_abort);
> +
> + /*
> + * This MUST be on main loop thread since we have commands that
> + * have assumption to be run on main loop thread. It would be
> + * nice that one day we can remove this assumption in the future.
> + */
> + mon_global.qmp_dispatcher_bh = aio_bh_new(qemu_get_aio_context(),
> + monitor_qmp_bh_dispatcher,
> + NULL);
> }
>
> void monitor_init_globals(void)
> @@ -4280,6 +4431,10 @@ void monitor_cleanup(void)
> }
> qemu_mutex_unlock(&monitor_lock);
>
> + /* QEMUBHs needs to be deleted before destroying the IOThread. */
> + qemu_bh_delete(mon_global.qmp_dispatcher_bh);
> + mon_global.qmp_dispatcher_bh = NULL;
> +
> iothread_destroy(mon_global.mon_iothread);
> mon_global.mon_iothread = NULL;
> }
> --
> 2.14.3
>
>
--
Marc-André Lureau
On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:
[...]
> > +/*
> > + * Dispatch one single QMP request. The function will free the req_obj
> > + * and objects inside it before return.
> > + */
> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> > {
> > - QObject *req, *rsp = NULL, *id = NULL;
> > + Monitor *mon, *old_mon;
> > + QObject *req, *rsp = NULL, *id;
> > QDict *qdict = NULL;
> > - MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
> > - Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
> > -
> > - Error *err = NULL;
> > + bool need_resume;
> >
> > - req = json_parser_parse_err(tokens, NULL, &err);
> > - if (!req && !err) {
> > - /* json_parser_parse_err() sucks: can fail without setting @err */
> > - error_setg(&err, QERR_JSON_PARSING);
> > - }
> > - if (err) {
> > - goto err_out;
> > - }
> > + req = req_obj->req;
> > + mon = req_obj->mon;
> > + id = req_obj->id;
> > + need_resume = req_obj->need_resume;
> >
> > - qdict = qobject_to_qdict(req);
> > - if (qdict) {
> > - id = qdict_get(qdict, "id");
> > - qobject_incref(id);
> > - qdict_del(qdict, "id");
> > - } /* else will fail qmp_dispatch() */
> > + g_free(req_obj);
> >
> > if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
> > QString *req_json = qobject_to_json(req);
> > @@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> > old_mon = cur_mon;
> > cur_mon = mon;
>
> There is another issue with this series, since cur_mon is global (and
> not protected), an oob command may change the cur_mon while another
> command is running in the main thread with unexpected consequences. I
> don't have a clear idea what is the best way to solve it. Making the
> variable per-thread, or going all the way to get rid of cur_mon (my
> preference, but much harder)
IMHO it is fine too.
Note that this cur_mon operation is in monitor_qmp_dispatch_one() now,
which is still running in main thread. So AFAICT all the cur_mon
references are in main thread, and monitor IOThread does not modify
that variable at all. Then we should probably be safe.
I would be far more than glad to see cur_mon go away one day.
Thanks,
--
Peter Xu
Hi
On Mon, Mar 26, 2018 at 10:07 AM, Peter Xu <peterx@redhat.com> wrote:
> On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:
>
> [...]
>
>> > +/*
>> > + * Dispatch one single QMP request. The function will free the req_obj
>> > + * and objects inside it before return.
>> > + */
>> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
>> > {
>> > - QObject *req, *rsp = NULL, *id = NULL;
>> > + Monitor *mon, *old_mon;
>> > + QObject *req, *rsp = NULL, *id;
>> > QDict *qdict = NULL;
>> > - MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
>> > - Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
>> > -
>> > - Error *err = NULL;
>> > + bool need_resume;
>> >
>> > - req = json_parser_parse_err(tokens, NULL, &err);
>> > - if (!req && !err) {
>> > - /* json_parser_parse_err() sucks: can fail without setting @err */
>> > - error_setg(&err, QERR_JSON_PARSING);
>> > - }
>> > - if (err) {
>> > - goto err_out;
>> > - }
>> > + req = req_obj->req;
>> > + mon = req_obj->mon;
>> > + id = req_obj->id;
>> > + need_resume = req_obj->need_resume;
>> >
>> > - qdict = qobject_to_qdict(req);
>> > - if (qdict) {
>> > - id = qdict_get(qdict, "id");
>> > - qobject_incref(id);
>> > - qdict_del(qdict, "id");
>> > - } /* else will fail qmp_dispatch() */
>> > + g_free(req_obj);
>> >
>> > if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
>> > QString *req_json = qobject_to_json(req);
>> > @@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>> > old_mon = cur_mon;
>> > cur_mon = mon;
>>
>> There is another issue with this series, since cur_mon is global (and
>> not protected), an oob command may change the cur_mon while another
>> command is running in the main thread with unexpected consequences. I
>> don't have a clear idea what is the best way to solve it. Making the
>> variable per-thread, or going all the way to get rid of cur_mon (my
>> preference, but much harder)
>
> IMHO it is fine too.
>
> Note that this cur_mon operation is in monitor_qmp_dispatch_one() now,
> which is still running in main thread. So AFAICT all the cur_mon
> references are in main thread, and monitor IOThread does not modify
> that variable at all. Then we should probably be safe.
But monitor_qmp_dispatch_one() is called from iothread if the command
is oob, so cur_mon may be updated while another command is running in
main thread, or am I wrong?
>
> I would be far more than glad to see cur_mon go away one day.
>
> Thanks,
>
> --
> Peter Xu
--
Marc-André Lureau
On Mon, Mar 26, 2018 at 10:33:27AM +0200, Marc-André Lureau wrote:
> Hi
>
> On Mon, Mar 26, 2018 at 10:07 AM, Peter Xu <peterx@redhat.com> wrote:
> > On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:
> >
> > [...]
> >
> >> > +/*
> >> > + * Dispatch one single QMP request. The function will free the req_obj
> >> > + * and objects inside it before return.
> >> > + */
> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> >> > {
> >> > - QObject *req, *rsp = NULL, *id = NULL;
> >> > + Monitor *mon, *old_mon;
> >> > + QObject *req, *rsp = NULL, *id;
> >> > QDict *qdict = NULL;
> >> > - MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
> >> > - Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
> >> > -
> >> > - Error *err = NULL;
> >> > + bool need_resume;
> >> >
> >> > - req = json_parser_parse_err(tokens, NULL, &err);
> >> > - if (!req && !err) {
> >> > - /* json_parser_parse_err() sucks: can fail without setting @err */
> >> > - error_setg(&err, QERR_JSON_PARSING);
> >> > - }
> >> > - if (err) {
> >> > - goto err_out;
> >> > - }
> >> > + req = req_obj->req;
> >> > + mon = req_obj->mon;
> >> > + id = req_obj->id;
> >> > + need_resume = req_obj->need_resume;
> >> >
> >> > - qdict = qobject_to_qdict(req);
> >> > - if (qdict) {
> >> > - id = qdict_get(qdict, "id");
> >> > - qobject_incref(id);
> >> > - qdict_del(qdict, "id");
> >> > - } /* else will fail qmp_dispatch() */
> >> > + g_free(req_obj);
> >> >
> >> > if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
> >> > QString *req_json = qobject_to_json(req);
> >> > @@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> >> > old_mon = cur_mon;
> >> > cur_mon = mon;
> >>
> >> There is another issue with this series, since cur_mon is global (and
> >> not protected), an oob command may change the cur_mon while another
> >> command is running in the main thread with unexpected consequences. I
> >> don't have a clear idea what is the best way to solve it. Making the
> >> variable per-thread, or going all the way to get rid of cur_mon (my
> >> preference, but much harder)
> >
> > IMHO it is fine too.
> >
> > Note that this cur_mon operation is in monitor_qmp_dispatch_one() now,
> > which is still running in main thread. So AFAICT all the cur_mon
> > references are in main thread, and monitor IOThread does not modify
> > that variable at all. Then we should probably be safe.
>
> But monitor_qmp_dispatch_one() is called from iothread if the command
> is oob, so cur_mon may be updated while another command is running in
> main thread, or am I wrong?
You are right. I missed that, sorry...
Would this be a simple workaround (but hopefully efficient) solution?
diff --git a/monitor.c b/monitor.c
index 77f4c41cfa..99641c0c6d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4023,7 +4023,7 @@ typedef struct QMPRequest QMPRequest;
* Dispatch one single QMP request. The function will free the req_obj
* and objects inside it before return.
*/
-static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
+static void monitor_qmp_dispatch_one(QMPRequest *req_obj, bool hack_curmon)
{
Monitor *mon, *old_mon;
QObject *req, *rsp = NULL, *id;
@@ -4043,12 +4043,16 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
QDECREF(req_json);
}
- old_mon = cur_mon;
- cur_mon = mon;
+ if (hack_curmon) {
+ old_mon = cur_mon;
+ cur_mon = mon;
+ }
rsp = qmp_dispatch(mon->qmp.commands, req);
- cur_mon = old_mon;
+ if (hack_curmon) {
+ cur_mon = old_mon;
+ }
if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error");
@@ -4116,7 +4120,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
if (req_obj) {
trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
- monitor_qmp_dispatch_one(req_obj);
+ monitor_qmp_dispatch_one(req_obj, true);
/* Reschedule instead of looping so the main loop stays responsive */
qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
}
@@ -4175,7 +4179,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
/* Out-Of-Band (OOB) requests are executed directly in parser. */
trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id)
?: "");
- monitor_qmp_dispatch_one(req_obj);
+ monitor_qmp_dispatch_one(req_obj, false);
return;
}
Then we forbit touching that evil cur_mon in OOB-capable command
handlers. Thanks,
--
Peter Xu
Hi
On Mon, Mar 26, 2018 at 11:08 AM, Peter Xu <peterx@redhat.com> wrote:
> On Mon, Mar 26, 2018 at 10:33:27AM +0200, Marc-André Lureau wrote:
>> Hi
>>
>> On Mon, Mar 26, 2018 at 10:07 AM, Peter Xu <peterx@redhat.com> wrote:
>> > On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:
>> >
>> > [...]
>> >
>> >> > +/*
>> >> > + * Dispatch one single QMP request. The function will free the req_obj
>> >> > + * and objects inside it before return.
>> >> > + */
>> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
>> >> > {
>> >> > - QObject *req, *rsp = NULL, *id = NULL;
>> >> > + Monitor *mon, *old_mon;
>> >> > + QObject *req, *rsp = NULL, *id;
>> >> > QDict *qdict = NULL;
>> >> > - MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
>> >> > - Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
>> >> > -
>> >> > - Error *err = NULL;
>> >> > + bool need_resume;
>> >> >
>> >> > - req = json_parser_parse_err(tokens, NULL, &err);
>> >> > - if (!req && !err) {
>> >> > - /* json_parser_parse_err() sucks: can fail without setting @err */
>> >> > - error_setg(&err, QERR_JSON_PARSING);
>> >> > - }
>> >> > - if (err) {
>> >> > - goto err_out;
>> >> > - }
>> >> > + req = req_obj->req;
>> >> > + mon = req_obj->mon;
>> >> > + id = req_obj->id;
>> >> > + need_resume = req_obj->need_resume;
>> >> >
>> >> > - qdict = qobject_to_qdict(req);
>> >> > - if (qdict) {
>> >> > - id = qdict_get(qdict, "id");
>> >> > - qobject_incref(id);
>> >> > - qdict_del(qdict, "id");
>> >> > - } /* else will fail qmp_dispatch() */
>> >> > + g_free(req_obj);
>> >> >
>> >> > if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
>> >> > QString *req_json = qobject_to_json(req);
>> >> > @@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>> >> > old_mon = cur_mon;
>> >> > cur_mon = mon;
>> >>
>> >> There is another issue with this series, since cur_mon is global (and
>> >> not protected), an oob command may change the cur_mon while another
>> >> command is running in the main thread with unexpected consequences. I
>> >> don't have a clear idea what is the best way to solve it. Making the
>> >> variable per-thread, or going all the way to get rid of cur_mon (my
>> >> preference, but much harder)
>> >
>> > IMHO it is fine too.
>> >
>> > Note that this cur_mon operation is in monitor_qmp_dispatch_one() now,
>> > which is still running in main thread. So AFAICT all the cur_mon
>> > references are in main thread, and monitor IOThread does not modify
>> > that variable at all. Then we should probably be safe.
>>
>> But monitor_qmp_dispatch_one() is called from iothread if the command
>> is oob, so cur_mon may be updated while another command is running in
>> main thread, or am I wrong?
>
> You are right. I missed that, sorry...
>
> Would this be a simple workaround (but hopefully efficient) solution?
>
> diff --git a/monitor.c b/monitor.c
> index 77f4c41cfa..99641c0c6d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4023,7 +4023,7 @@ typedef struct QMPRequest QMPRequest;
> * Dispatch one single QMP request. The function will free the req_obj
> * and objects inside it before return.
> */
> -static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> +static void monitor_qmp_dispatch_one(QMPRequest *req_obj, bool hack_curmon)
> {
> Monitor *mon, *old_mon;
> QObject *req, *rsp = NULL, *id;
> @@ -4043,12 +4043,16 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> QDECREF(req_json);
> }
>
> - old_mon = cur_mon;
> - cur_mon = mon;
> + if (hack_curmon) {
> + old_mon = cur_mon;
> + cur_mon = mon;
> + }
>
> rsp = qmp_dispatch(mon->qmp.commands, req);
>
> - cur_mon = old_mon;
> + if (hack_curmon) {
> + cur_mon = old_mon;
> + }
>
> if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
> qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error");
> @@ -4116,7 +4120,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
>
> if (req_obj) {
> trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
> - monitor_qmp_dispatch_one(req_obj);
> + monitor_qmp_dispatch_one(req_obj, true);
> /* Reschedule instead of looping so the main loop stays responsive */
> qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> }
> @@ -4175,7 +4179,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> /* Out-Of-Band (OOB) requests are executed directly in parser. */
> trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id)
> ?: "");
> - monitor_qmp_dispatch_one(req_obj);
> + monitor_qmp_dispatch_one(req_obj, false);
> return;
> }
>
> Then we forbit touching that evil cur_mon in OOB-capable command
> handlers. Thanks,
That's not easy to enforce though, afaict it is being used for:
- error reporting decision
- file & socket lookup (fd: & /dev/fdset etc)
- the current state of the monitor / list of commands, cpu_path, capabilities..
Wouldn't it be simpler to make it per-thread? I think it could also
use helpers to push/pop the current monitor.
--
Marc-André Lureau
On Mon, Mar 26, 2018 at 11:46:13AM +0200, Marc-André Lureau wrote:
> Hi
>
> On Mon, Mar 26, 2018 at 11:08 AM, Peter Xu <peterx@redhat.com> wrote:
> > On Mon, Mar 26, 2018 at 10:33:27AM +0200, Marc-André Lureau wrote:
> >> Hi
> >>
> >> On Mon, Mar 26, 2018 at 10:07 AM, Peter Xu <peterx@redhat.com> wrote:
> >> > On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:
> >> >
> >> > [...]
> >> >
> >> >> > +/*
> >> >> > + * Dispatch one single QMP request. The function will free the req_obj
> >> >> > + * and objects inside it before return.
> >> >> > + */
> >> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> >> >> > {
> >> >> > - QObject *req, *rsp = NULL, *id = NULL;
> >> >> > + Monitor *mon, *old_mon;
> >> >> > + QObject *req, *rsp = NULL, *id;
> >> >> > QDict *qdict = NULL;
> >> >> > - MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
> >> >> > - Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
> >> >> > -
> >> >> > - Error *err = NULL;
> >> >> > + bool need_resume;
> >> >> >
> >> >> > - req = json_parser_parse_err(tokens, NULL, &err);
> >> >> > - if (!req && !err) {
> >> >> > - /* json_parser_parse_err() sucks: can fail without setting @err */
> >> >> > - error_setg(&err, QERR_JSON_PARSING);
> >> >> > - }
> >> >> > - if (err) {
> >> >> > - goto err_out;
> >> >> > - }
> >> >> > + req = req_obj->req;
> >> >> > + mon = req_obj->mon;
> >> >> > + id = req_obj->id;
> >> >> > + need_resume = req_obj->need_resume;
> >> >> >
> >> >> > - qdict = qobject_to_qdict(req);
> >> >> > - if (qdict) {
> >> >> > - id = qdict_get(qdict, "id");
> >> >> > - qobject_incref(id);
> >> >> > - qdict_del(qdict, "id");
> >> >> > - } /* else will fail qmp_dispatch() */
> >> >> > + g_free(req_obj);
> >> >> >
> >> >> > if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
> >> >> > QString *req_json = qobject_to_json(req);
> >> >> > @@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> >> >> > old_mon = cur_mon;
> >> >> > cur_mon = mon;
> >> >>
> >> >> There is another issue with this series, since cur_mon is global (and
> >> >> not protected), an oob command may change the cur_mon while another
> >> >> command is running in the main thread with unexpected consequences. I
> >> >> don't have a clear idea what is the best way to solve it. Making the
> >> >> variable per-thread, or going all the way to get rid of cur_mon (my
> >> >> preference, but much harder)
> >> >
> >> > IMHO it is fine too.
> >> >
> >> > Note that this cur_mon operation is in monitor_qmp_dispatch_one() now,
> >> > which is still running in main thread. So AFAICT all the cur_mon
> >> > references are in main thread, and monitor IOThread does not modify
> >> > that variable at all. Then we should probably be safe.
> >>
> >> But monitor_qmp_dispatch_one() is called from iothread if the command
> >> is oob, so cur_mon may be updated while another command is running in
> >> main thread, or am I wrong?
> >
> > You are right. I missed that, sorry...
> >
> > Would this be a simple workaround (but hopefully efficient) solution?
> >
> > diff --git a/monitor.c b/monitor.c
> > index 77f4c41cfa..99641c0c6d 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4023,7 +4023,7 @@ typedef struct QMPRequest QMPRequest;
> > * Dispatch one single QMP request. The function will free the req_obj
> > * and objects inside it before return.
> > */
> > -static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj, bool hack_curmon)
> > {
> > Monitor *mon, *old_mon;
> > QObject *req, *rsp = NULL, *id;
> > @@ -4043,12 +4043,16 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> > QDECREF(req_json);
> > }
> >
> > - old_mon = cur_mon;
> > - cur_mon = mon;
> > + if (hack_curmon) {
> > + old_mon = cur_mon;
> > + cur_mon = mon;
> > + }
> >
> > rsp = qmp_dispatch(mon->qmp.commands, req);
> >
> > - cur_mon = old_mon;
> > + if (hack_curmon) {
> > + cur_mon = old_mon;
> > + }
> >
> > if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
> > qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error");
> > @@ -4116,7 +4120,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >
> > if (req_obj) {
> > trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
> > - monitor_qmp_dispatch_one(req_obj);
> > + monitor_qmp_dispatch_one(req_obj, true);
> > /* Reschedule instead of looping so the main loop stays responsive */
> > qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> > }
> > @@ -4175,7 +4179,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> > /* Out-Of-Band (OOB) requests are executed directly in parser. */
> > trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id)
> > ?: "");
> > - monitor_qmp_dispatch_one(req_obj);
> > + monitor_qmp_dispatch_one(req_obj, false);
> > return;
> > }
> >
> > Then we forbit touching that evil cur_mon in OOB-capable command
> > handlers. Thanks,
>
> That's not easy to enforce though, afaict it is being used for:
> - error reporting decision
IMO this should not be a problem, since any QMP handler (including
OOB-capable ones) will be with an Error** there, so logically speaking
people should never call things like error_report() in that.
> - file & socket lookup (fd: & /dev/fdset etc)
I suppose only very rare commands will use it? It'll be a big problem
to solve when we want to completely remove cur_mon though.
> - the current state of the monitor / list of commands, cpu_path, capabilities..
This is very rare to be used too? Most commands should not use them AFAIU.
>
> Wouldn't it be simpler to make it per-thread? I think it could also
> use helpers to push/pop the current monitor.
Anyway I think yes this is still a good option (though the cur_mon
logic will be a bit more complicated).
Do you plan to post some patch about this, or do you want me to do
this? I suppose we'll change the qemu_thread_create() a bit to pass
the cur_mon inside, and I suppose this might be better material after
2.12 release if OOB is off now.
--
Peter Xu
Hi Peter
On Wed, Mar 28, 2018 at 6:02 AM, Peter Xu <peterx@redhat.com> wrote:
> On Mon, Mar 26, 2018 at 11:46:13AM +0200, Marc-André Lureau wrote:
>> Hi
>>
>> On Mon, Mar 26, 2018 at 11:08 AM, Peter Xu <peterx@redhat.com> wrote:
>> > On Mon, Mar 26, 2018 at 10:33:27AM +0200, Marc-André Lureau wrote:
>> >> Hi
>> >>
>> >> On Mon, Mar 26, 2018 at 10:07 AM, Peter Xu <peterx@redhat.com> wrote:
>> >> > On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:
>> >> >
>> >> > [...]
>> >> >
>> >> >> > +/*
>> >> >> > + * Dispatch one single QMP request. The function will free the req_obj
>> >> >> > + * and objects inside it before return.
>> >> >> > + */
>> >> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
>> >> >> > {
>> >> >> > - QObject *req, *rsp = NULL, *id = NULL;
>> >> >> > + Monitor *mon, *old_mon;
>> >> >> > + QObject *req, *rsp = NULL, *id;
>> >> >> > QDict *qdict = NULL;
>> >> >> > - MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
>> >> >> > - Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
>> >> >> > -
>> >> >> > - Error *err = NULL;
>> >> >> > + bool need_resume;
>> >> >> >
>> >> >> > - req = json_parser_parse_err(tokens, NULL, &err);
>> >> >> > - if (!req && !err) {
>> >> >> > - /* json_parser_parse_err() sucks: can fail without setting @err */
>> >> >> > - error_setg(&err, QERR_JSON_PARSING);
>> >> >> > - }
>> >> >> > - if (err) {
>> >> >> > - goto err_out;
>> >> >> > - }
>> >> >> > + req = req_obj->req;
>> >> >> > + mon = req_obj->mon;
>> >> >> > + id = req_obj->id;
>> >> >> > + need_resume = req_obj->need_resume;
>> >> >> >
>> >> >> > - qdict = qobject_to_qdict(req);
>> >> >> > - if (qdict) {
>> >> >> > - id = qdict_get(qdict, "id");
>> >> >> > - qobject_incref(id);
>> >> >> > - qdict_del(qdict, "id");
>> >> >> > - } /* else will fail qmp_dispatch() */
>> >> >> > + g_free(req_obj);
>> >> >> >
>> >> >> > if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
>> >> >> > QString *req_json = qobject_to_json(req);
>> >> >> > @@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>> >> >> > old_mon = cur_mon;
>> >> >> > cur_mon = mon;
>> >> >>
>> >> >> There is another issue with this series, since cur_mon is global (and
>> >> >> not protected), an oob command may change the cur_mon while another
>> >> >> command is running in the main thread with unexpected consequences. I
>> >> >> don't have a clear idea what is the best way to solve it. Making the
>> >> >> variable per-thread, or going all the way to get rid of cur_mon (my
>> >> >> preference, but much harder)
>> >> >
>> >> > IMHO it is fine too.
>> >> >
>> >> > Note that this cur_mon operation is in monitor_qmp_dispatch_one() now,
>> >> > which is still running in main thread. So AFAICT all the cur_mon
>> >> > references are in main thread, and monitor IOThread does not modify
>> >> > that variable at all. Then we should probably be safe.
>> >>
>> >> But monitor_qmp_dispatch_one() is called from iothread if the command
>> >> is oob, so cur_mon may be updated while another command is running in
>> >> main thread, or am I wrong?
>> >
>> > You are right. I missed that, sorry...
>> >
>> > Would this be a simple workaround (but hopefully efficient) solution?
>> >
>> > diff --git a/monitor.c b/monitor.c
>> > index 77f4c41cfa..99641c0c6d 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -4023,7 +4023,7 @@ typedef struct QMPRequest QMPRequest;
>> > * Dispatch one single QMP request. The function will free the req_obj
>> > * and objects inside it before return.
>> > */
>> > -static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
>> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj, bool hack_curmon)
>> > {
>> > Monitor *mon, *old_mon;
>> > QObject *req, *rsp = NULL, *id;
>> > @@ -4043,12 +4043,16 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
>> > QDECREF(req_json);
>> > }
>> >
>> > - old_mon = cur_mon;
>> > - cur_mon = mon;
>> > + if (hack_curmon) {
>> > + old_mon = cur_mon;
>> > + cur_mon = mon;
>> > + }
>> >
>> > rsp = qmp_dispatch(mon->qmp.commands, req);
>> >
>> > - cur_mon = old_mon;
>> > + if (hack_curmon) {
>> > + cur_mon = old_mon;
>> > + }
>> >
>> > if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
>> > qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error");
>> > @@ -4116,7 +4120,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
>> >
>> > if (req_obj) {
>> > trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
>> > - monitor_qmp_dispatch_one(req_obj);
>> > + monitor_qmp_dispatch_one(req_obj, true);
>> > /* Reschedule instead of looping so the main loop stays responsive */
>> > qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
>> > }
>> > @@ -4175,7 +4179,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>> > /* Out-Of-Band (OOB) requests are executed directly in parser. */
>> > trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id)
>> > ?: "");
>> > - monitor_qmp_dispatch_one(req_obj);
>> > + monitor_qmp_dispatch_one(req_obj, false);
>> > return;
>> > }
>> >
>> > Then we forbit touching that evil cur_mon in OOB-capable command
>> > handlers. Thanks,
>>
>> That's not easy to enforce though, afaict it is being used for:
>> - error reporting decision
>
> IMO this should not be a problem, since any QMP handler (including
> OOB-capable ones) will be with an Error** there, so logically speaking
> people should never call things like error_report() in that.
>
>> - file & socket lookup (fd: & /dev/fdset etc)
>
> I suppose only very rare commands will use it? It'll be a big problem
> to solve when we want to completely remove cur_mon though.
>
>> - the current state of the monitor / list of commands, cpu_path, capabilities..
>
> This is very rare to be used too? Most commands should not use them AFAIU.
>
>>
>> Wouldn't it be simpler to make it per-thread? I think it could also
>> use helpers to push/pop the current monitor.
>
> Anyway I think yes this is still a good option (though the cur_mon
> logic will be a bit more complicated).
>
> Do you plan to post some patch about this, or do you want me to do
> this? I suppose we'll change the qemu_thread_create() a bit to pass
> the cur_mon inside, and I suppose this might be better material after
> 2.12 release if OOB is off now.
Have you looked at making cur_mon per-thread?
thanks
--
Marc-André Lureau
On Wed, Apr 04, 2018 at 03:58:56PM +0200, Marc-André Lureau wrote:
> Hi Peter
>
> On Wed, Mar 28, 2018 at 6:02 AM, Peter Xu <peterx@redhat.com> wrote:
> > On Mon, Mar 26, 2018 at 11:46:13AM +0200, Marc-André Lureau wrote:
> >> Hi
> >>
> >> On Mon, Mar 26, 2018 at 11:08 AM, Peter Xu <peterx@redhat.com> wrote:
> >> > On Mon, Mar 26, 2018 at 10:33:27AM +0200, Marc-André Lureau wrote:
> >> >> Hi
> >> >>
> >> >> On Mon, Mar 26, 2018 at 10:07 AM, Peter Xu <peterx@redhat.com> wrote:
> >> >> > On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:
> >> >> >
> >> >> > [...]
> >> >> >
> >> >> >> > +/*
> >> >> >> > + * Dispatch one single QMP request. The function will free the req_obj
> >> >> >> > + * and objects inside it before return.
> >> >> >> > + */
> >> >> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> >> >> >> > {
> >> >> >> > - QObject *req, *rsp = NULL, *id = NULL;
> >> >> >> > + Monitor *mon, *old_mon;
> >> >> >> > + QObject *req, *rsp = NULL, *id;
> >> >> >> > QDict *qdict = NULL;
> >> >> >> > - MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
> >> >> >> > - Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
> >> >> >> > -
> >> >> >> > - Error *err = NULL;
> >> >> >> > + bool need_resume;
> >> >> >> >
> >> >> >> > - req = json_parser_parse_err(tokens, NULL, &err);
> >> >> >> > - if (!req && !err) {
> >> >> >> > - /* json_parser_parse_err() sucks: can fail without setting @err */
> >> >> >> > - error_setg(&err, QERR_JSON_PARSING);
> >> >> >> > - }
> >> >> >> > - if (err) {
> >> >> >> > - goto err_out;
> >> >> >> > - }
> >> >> >> > + req = req_obj->req;
> >> >> >> > + mon = req_obj->mon;
> >> >> >> > + id = req_obj->id;
> >> >> >> > + need_resume = req_obj->need_resume;
> >> >> >> >
> >> >> >> > - qdict = qobject_to_qdict(req);
> >> >> >> > - if (qdict) {
> >> >> >> > - id = qdict_get(qdict, "id");
> >> >> >> > - qobject_incref(id);
> >> >> >> > - qdict_del(qdict, "id");
> >> >> >> > - } /* else will fail qmp_dispatch() */
> >> >> >> > + g_free(req_obj);
> >> >> >> >
> >> >> >> > if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
> >> >> >> > QString *req_json = qobject_to_json(req);
> >> >> >> > @@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> >> >> >> > old_mon = cur_mon;
> >> >> >> > cur_mon = mon;
> >> >> >>
> >> >> >> There is another issue with this series, since cur_mon is global (and
> >> >> >> not protected), an oob command may change the cur_mon while another
> >> >> >> command is running in the main thread with unexpected consequences. I
> >> >> >> don't have a clear idea what is the best way to solve it. Making the
> >> >> >> variable per-thread, or going all the way to get rid of cur_mon (my
> >> >> >> preference, but much harder)
> >> >> >
> >> >> > IMHO it is fine too.
> >> >> >
> >> >> > Note that this cur_mon operation is in monitor_qmp_dispatch_one() now,
> >> >> > which is still running in main thread. So AFAICT all the cur_mon
> >> >> > references are in main thread, and monitor IOThread does not modify
> >> >> > that variable at all. Then we should probably be safe.
> >> >>
> >> >> But monitor_qmp_dispatch_one() is called from iothread if the command
> >> >> is oob, so cur_mon may be updated while another command is running in
> >> >> main thread, or am I wrong?
> >> >
> >> > You are right. I missed that, sorry...
> >> >
> >> > Would this be a simple workaround (but hopefully efficient) solution?
> >> >
> >> > diff --git a/monitor.c b/monitor.c
> >> > index 77f4c41cfa..99641c0c6d 100644
> >> > --- a/monitor.c
> >> > +++ b/monitor.c
> >> > @@ -4023,7 +4023,7 @@ typedef struct QMPRequest QMPRequest;
> >> > * Dispatch one single QMP request. The function will free the req_obj
> >> > * and objects inside it before return.
> >> > */
> >> > -static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj, bool hack_curmon)
> >> > {
> >> > Monitor *mon, *old_mon;
> >> > QObject *req, *rsp = NULL, *id;
> >> > @@ -4043,12 +4043,16 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> >> > QDECREF(req_json);
> >> > }
> >> >
> >> > - old_mon = cur_mon;
> >> > - cur_mon = mon;
> >> > + if (hack_curmon) {
> >> > + old_mon = cur_mon;
> >> > + cur_mon = mon;
> >> > + }
> >> >
> >> > rsp = qmp_dispatch(mon->qmp.commands, req);
> >> >
> >> > - cur_mon = old_mon;
> >> > + if (hack_curmon) {
> >> > + cur_mon = old_mon;
> >> > + }
> >> >
> >> > if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
> >> > qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error");
> >> > @@ -4116,7 +4120,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >> >
> >> > if (req_obj) {
> >> > trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
> >> > - monitor_qmp_dispatch_one(req_obj);
> >> > + monitor_qmp_dispatch_one(req_obj, true);
> >> > /* Reschedule instead of looping so the main loop stays responsive */
> >> > qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> >> > }
> >> > @@ -4175,7 +4179,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> >> > /* Out-Of-Band (OOB) requests are executed directly in parser. */
> >> > trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id)
> >> > ?: "");
> >> > - monitor_qmp_dispatch_one(req_obj);
> >> > + monitor_qmp_dispatch_one(req_obj, false);
> >> > return;
> >> > }
> >> >
> >> > Then we forbit touching that evil cur_mon in OOB-capable command
> >> > handlers. Thanks,
> >>
> >> That's not easy to enforce though, afaict it is being used for:
> >> - error reporting decision
> >
> > IMO this should not be a problem, since any QMP handler (including
> > OOB-capable ones) will be with an Error** there, so logically speaking
> > people should never call things like error_report() in that.
> >
> >> - file & socket lookup (fd: & /dev/fdset etc)
> >
> > I suppose only very rare commands will use it? It'll be a big problem
> > to solve when we want to completely remove cur_mon though.
> >
> >> - the current state of the monitor / list of commands, cpu_path, capabilities..
> >
> > This is very rare to be used too? Most commands should not use them AFAIU.
> >
> >>
> >> Wouldn't it be simpler to make it per-thread? I think it could also
> >> use helpers to push/pop the current monitor.
> >
> > Anyway I think yes this is still a good option (though the cur_mon
> > logic will be a bit more complicated).
> >
> > Do you plan to post some patch about this, or do you want me to do
> > this? I suppose we'll change the qemu_thread_create() a bit to pass
> > the cur_mon inside, and I suppose this might be better material after
> > 2.12 release if OOB is off now.
>
> Have you looked at making cur_mon per-thread?
Above was my idea, nothing else has been done.
Please feel free to post a patch for this, or I'll do this after 2.12
release.
--
Peter Xu
Hi
On Sun, Apr 8, 2018 at 5:02 AM, Peter Xu <peterx@redhat.com> wrote:
> On Wed, Apr 04, 2018 at 03:58:56PM +0200, Marc-André Lureau wrote:
>> Hi Peter
>>
>> On Wed, Mar 28, 2018 at 6:02 AM, Peter Xu <peterx@redhat.com> wrote:
>> > On Mon, Mar 26, 2018 at 11:46:13AM +0200, Marc-André Lureau wrote:
>> >> Hi
>> >>
>> >> On Mon, Mar 26, 2018 at 11:08 AM, Peter Xu <peterx@redhat.com> wrote:
>> >> > On Mon, Mar 26, 2018 at 10:33:27AM +0200, Marc-André Lureau wrote:
>> >> >> Hi
>> >> >>
>> >> >> On Mon, Mar 26, 2018 at 10:07 AM, Peter Xu <peterx@redhat.com> wrote:
>> >> >> > On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:
>> >> >> >
>> >> >> > [...]
>> >> >> >
>> >> >> >> > +/*
>> >> >> >> > + * Dispatch one single QMP request. The function will free the req_obj
>> >> >> >> > + * and objects inside it before return.
>> >> >> >> > + */
>> >> >> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
>> >> >> >> > {
>> >> >> >> > - QObject *req, *rsp = NULL, *id = NULL;
>> >> >> >> > + Monitor *mon, *old_mon;
>> >> >> >> > + QObject *req, *rsp = NULL, *id;
>> >> >> >> > QDict *qdict = NULL;
>> >> >> >> > - MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
>> >> >> >> > - Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
>> >> >> >> > -
>> >> >> >> > - Error *err = NULL;
>> >> >> >> > + bool need_resume;
>> >> >> >> >
>> >> >> >> > - req = json_parser_parse_err(tokens, NULL, &err);
>> >> >> >> > - if (!req && !err) {
>> >> >> >> > - /* json_parser_parse_err() sucks: can fail without setting @err */
>> >> >> >> > - error_setg(&err, QERR_JSON_PARSING);
>> >> >> >> > - }
>> >> >> >> > - if (err) {
>> >> >> >> > - goto err_out;
>> >> >> >> > - }
>> >> >> >> > + req = req_obj->req;
>> >> >> >> > + mon = req_obj->mon;
>> >> >> >> > + id = req_obj->id;
>> >> >> >> > + need_resume = req_obj->need_resume;
>> >> >> >> >
>> >> >> >> > - qdict = qobject_to_qdict(req);
>> >> >> >> > - if (qdict) {
>> >> >> >> > - id = qdict_get(qdict, "id");
>> >> >> >> > - qobject_incref(id);
>> >> >> >> > - qdict_del(qdict, "id");
>> >> >> >> > - } /* else will fail qmp_dispatch() */
>> >> >> >> > + g_free(req_obj);
>> >> >> >> >
>> >> >> >> > if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
>> >> >> >> > QString *req_json = qobject_to_json(req);
>> >> >> >> > @@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>> >> >> >> > old_mon = cur_mon;
>> >> >> >> > cur_mon = mon;
>> >> >> >>
>> >> >> >> There is another issue with this series, since cur_mon is global (and
>> >> >> >> not protected), an oob command may change the cur_mon while another
>> >> >> >> command is running in the main thread with unexpected consequences. I
>> >> >> >> don't have a clear idea what is the best way to solve it. Making the
>> >> >> >> variable per-thread, or going all the way to get rid of cur_mon (my
>> >> >> >> preference, but much harder)
>> >> >> >
>> >> >> > IMHO it is fine too.
>> >> >> >
>> >> >> > Note that this cur_mon operation is in monitor_qmp_dispatch_one() now,
>> >> >> > which is still running in main thread. So AFAICT all the cur_mon
>> >> >> > references are in main thread, and monitor IOThread does not modify
>> >> >> > that variable at all. Then we should probably be safe.
>> >> >>
>> >> >> But monitor_qmp_dispatch_one() is called from iothread if the command
>> >> >> is oob, so cur_mon may be updated while another command is running in
>> >> >> main thread, or am I wrong?
>> >> >
>> >> > You are right. I missed that, sorry...
>> >> >
>> >> > Would this be a simple workaround (but hopefully efficient) solution?
>> >> >
>> >> > diff --git a/monitor.c b/monitor.c
>> >> > index 77f4c41cfa..99641c0c6d 100644
>> >> > --- a/monitor.c
>> >> > +++ b/monitor.c
>> >> > @@ -4023,7 +4023,7 @@ typedef struct QMPRequest QMPRequest;
>> >> > * Dispatch one single QMP request. The function will free the req_obj
>> >> > * and objects inside it before return.
>> >> > */
>> >> > -static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
>> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj, bool hack_curmon)
>> >> > {
>> >> > Monitor *mon, *old_mon;
>> >> > QObject *req, *rsp = NULL, *id;
>> >> > @@ -4043,12 +4043,16 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
>> >> > QDECREF(req_json);
>> >> > }
>> >> >
>> >> > - old_mon = cur_mon;
>> >> > - cur_mon = mon;
>> >> > + if (hack_curmon) {
>> >> > + old_mon = cur_mon;
>> >> > + cur_mon = mon;
>> >> > + }
>> >> >
>> >> > rsp = qmp_dispatch(mon->qmp.commands, req);
>> >> >
>> >> > - cur_mon = old_mon;
>> >> > + if (hack_curmon) {
>> >> > + cur_mon = old_mon;
>> >> > + }
>> >> >
>> >> > if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
>> >> > qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error");
>> >> > @@ -4116,7 +4120,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
>> >> >
>> >> > if (req_obj) {
>> >> > trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
>> >> > - monitor_qmp_dispatch_one(req_obj);
>> >> > + monitor_qmp_dispatch_one(req_obj, true);
>> >> > /* Reschedule instead of looping so the main loop stays responsive */
>> >> > qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
>> >> > }
>> >> > @@ -4175,7 +4179,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>> >> > /* Out-Of-Band (OOB) requests are executed directly in parser. */
>> >> > trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id)
>> >> > ?: "");
>> >> > - monitor_qmp_dispatch_one(req_obj);
>> >> > + monitor_qmp_dispatch_one(req_obj, false);
>> >> > return;
>> >> > }
>> >> >
>> >> > Then we forbit touching that evil cur_mon in OOB-capable command
>> >> > handlers. Thanks,
>> >>
>> >> That's not easy to enforce though, afaict it is being used for:
>> >> - error reporting decision
>> >
>> > IMO this should not be a problem, since any QMP handler (including
>> > OOB-capable ones) will be with an Error** there, so logically speaking
>> > people should never call things like error_report() in that.
>> >
>> >> - file & socket lookup (fd: & /dev/fdset etc)
>> >
>> > I suppose only very rare commands will use it? It'll be a big problem
>> > to solve when we want to completely remove cur_mon though.
>> >
>> >> - the current state of the monitor / list of commands, cpu_path, capabilities..
>> >
>> > This is very rare to be used too? Most commands should not use them AFAIU.
>> >
>> >>
>> >> Wouldn't it be simpler to make it per-thread? I think it could also
>> >> use helpers to push/pop the current monitor.
>> >
>> > Anyway I think yes this is still a good option (though the cur_mon
>> > logic will be a bit more complicated).
>> >
>> > Do you plan to post some patch about this, or do you want me to do
>> > this? I suppose we'll change the qemu_thread_create() a bit to pass
>> > the cur_mon inside, and I suppose this might be better material after
>> > 2.12 release if OOB is off now.
>>
>> Have you looked at making cur_mon per-thread?
>
> Above was my idea, nothing else has been done.
>
> Please feel free to post a patch for this, or I'll do this after 2.12
> release.
If it's fixed after 2.12, I think we should document the race as a known issue.
--
Marc-André Lureau
On Mon, Apr 09, 2018 at 11:19:43AM +0200, Marc-André Lureau wrote:
> Hi
>
> On Sun, Apr 8, 2018 at 5:02 AM, Peter Xu <peterx@redhat.com> wrote:
> > On Wed, Apr 04, 2018 at 03:58:56PM +0200, Marc-André Lureau wrote:
> >> Hi Peter
> >>
> >> On Wed, Mar 28, 2018 at 6:02 AM, Peter Xu <peterx@redhat.com> wrote:
> >> > On Mon, Mar 26, 2018 at 11:46:13AM +0200, Marc-André Lureau wrote:
> >> >> Hi
> >> >>
> >> >> On Mon, Mar 26, 2018 at 11:08 AM, Peter Xu <peterx@redhat.com> wrote:
> >> >> > On Mon, Mar 26, 2018 at 10:33:27AM +0200, Marc-André Lureau wrote:
> >> >> >> Hi
> >> >> >>
> >> >> >> On Mon, Mar 26, 2018 at 10:07 AM, Peter Xu <peterx@redhat.com> wrote:
> >> >> >> > On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:
> >> >> >> >
> >> >> >> > [...]
> >> >> >> >
> >> >> >> >> > +/*
> >> >> >> >> > + * Dispatch one single QMP request. The function will free the req_obj
> >> >> >> >> > + * and objects inside it before return.
> >> >> >> >> > + */
> >> >> >> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> >> >> >> >> > {
> >> >> >> >> > - QObject *req, *rsp = NULL, *id = NULL;
> >> >> >> >> > + Monitor *mon, *old_mon;
> >> >> >> >> > + QObject *req, *rsp = NULL, *id;
> >> >> >> >> > QDict *qdict = NULL;
> >> >> >> >> > - MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
> >> >> >> >> > - Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
> >> >> >> >> > -
> >> >> >> >> > - Error *err = NULL;
> >> >> >> >> > + bool need_resume;
> >> >> >> >> >
> >> >> >> >> > - req = json_parser_parse_err(tokens, NULL, &err);
> >> >> >> >> > - if (!req && !err) {
> >> >> >> >> > - /* json_parser_parse_err() sucks: can fail without setting @err */
> >> >> >> >> > - error_setg(&err, QERR_JSON_PARSING);
> >> >> >> >> > - }
> >> >> >> >> > - if (err) {
> >> >> >> >> > - goto err_out;
> >> >> >> >> > - }
> >> >> >> >> > + req = req_obj->req;
> >> >> >> >> > + mon = req_obj->mon;
> >> >> >> >> > + id = req_obj->id;
> >> >> >> >> > + need_resume = req_obj->need_resume;
> >> >> >> >> >
> >> >> >> >> > - qdict = qobject_to_qdict(req);
> >> >> >> >> > - if (qdict) {
> >> >> >> >> > - id = qdict_get(qdict, "id");
> >> >> >> >> > - qobject_incref(id);
> >> >> >> >> > - qdict_del(qdict, "id");
> >> >> >> >> > - } /* else will fail qmp_dispatch() */
> >> >> >> >> > + g_free(req_obj);
> >> >> >> >> >
> >> >> >> >> > if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
> >> >> >> >> > QString *req_json = qobject_to_json(req);
> >> >> >> >> > @@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> >> >> >> >> > old_mon = cur_mon;
> >> >> >> >> > cur_mon = mon;
> >> >> >> >>
> >> >> >> >> There is another issue with this series, since cur_mon is global (and
> >> >> >> >> not protected), an oob command may change the cur_mon while another
> >> >> >> >> command is running in the main thread with unexpected consequences. I
> >> >> >> >> don't have a clear idea what is the best way to solve it. Making the
> >> >> >> >> variable per-thread, or going all the way to get rid of cur_mon (my
> >> >> >> >> preference, but much harder)
> >> >> >> >
> >> >> >> > IMHO it is fine too.
> >> >> >> >
> >> >> >> > Note that this cur_mon operation is in monitor_qmp_dispatch_one() now,
> >> >> >> > which is still running in main thread. So AFAICT all the cur_mon
> >> >> >> > references are in main thread, and monitor IOThread does not modify
> >> >> >> > that variable at all. Then we should probably be safe.
> >> >> >>
> >> >> >> But monitor_qmp_dispatch_one() is called from iothread if the command
> >> >> >> is oob, so cur_mon may be updated while another command is running in
> >> >> >> main thread, or am I wrong?
> >> >> >
> >> >> > You are right. I missed that, sorry...
> >> >> >
> >> >> > Would this be a simple workaround (but hopefully efficient) solution?
> >> >> >
> >> >> > diff --git a/monitor.c b/monitor.c
> >> >> > index 77f4c41cfa..99641c0c6d 100644
> >> >> > --- a/monitor.c
> >> >> > +++ b/monitor.c
> >> >> > @@ -4023,7 +4023,7 @@ typedef struct QMPRequest QMPRequest;
> >> >> > * Dispatch one single QMP request. The function will free the req_obj
> >> >> > * and objects inside it before return.
> >> >> > */
> >> >> > -static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> >> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj, bool hack_curmon)
> >> >> > {
> >> >> > Monitor *mon, *old_mon;
> >> >> > QObject *req, *rsp = NULL, *id;
> >> >> > @@ -4043,12 +4043,16 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> >> >> > QDECREF(req_json);
> >> >> > }
> >> >> >
> >> >> > - old_mon = cur_mon;
> >> >> > - cur_mon = mon;
> >> >> > + if (hack_curmon) {
> >> >> > + old_mon = cur_mon;
> >> >> > + cur_mon = mon;
> >> >> > + }
> >> >> >
> >> >> > rsp = qmp_dispatch(mon->qmp.commands, req);
> >> >> >
> >> >> > - cur_mon = old_mon;
> >> >> > + if (hack_curmon) {
> >> >> > + cur_mon = old_mon;
> >> >> > + }
> >> >> >
> >> >> > if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
> >> >> > qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error");
> >> >> > @@ -4116,7 +4120,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >> >> >
> >> >> > if (req_obj) {
> >> >> > trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
> >> >> > - monitor_qmp_dispatch_one(req_obj);
> >> >> > + monitor_qmp_dispatch_one(req_obj, true);
> >> >> > /* Reschedule instead of looping so the main loop stays responsive */
> >> >> > qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> >> >> > }
> >> >> > @@ -4175,7 +4179,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> >> >> > /* Out-Of-Band (OOB) requests are executed directly in parser. */
> >> >> > trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id)
> >> >> > ?: "");
> >> >> > - monitor_qmp_dispatch_one(req_obj);
> >> >> > + monitor_qmp_dispatch_one(req_obj, false);
> >> >> > return;
> >> >> > }
> >> >> >
> >> >> > Then we forbit touching that evil cur_mon in OOB-capable command
> >> >> > handlers. Thanks,
> >> >>
> >> >> That's not easy to enforce though, afaict it is being used for:
> >> >> - error reporting decision
> >> >
> >> > IMO this should not be a problem, since any QMP handler (including
> >> > OOB-capable ones) will be with an Error** there, so logically speaking
> >> > people should never call things like error_report() in that.
> >> >
> >> >> - file & socket lookup (fd: & /dev/fdset etc)
> >> >
> >> > I suppose only very rare commands will use it? It'll be a big problem
> >> > to solve when we want to completely remove cur_mon though.
> >> >
> >> >> - the current state of the monitor / list of commands, cpu_path, capabilities..
> >> >
> >> > This is very rare to be used too? Most commands should not use them AFAIU.
> >> >
> >> >>
> >> >> Wouldn't it be simpler to make it per-thread? I think it could also
> >> >> use helpers to push/pop the current monitor.
> >> >
> >> > Anyway I think yes this is still a good option (though the cur_mon
> >> > logic will be a bit more complicated).
> >> >
> >> > Do you plan to post some patch about this, or do you want me to do
> >> > this? I suppose we'll change the qemu_thread_create() a bit to pass
> >> > the cur_mon inside, and I suppose this might be better material after
> >> > 2.12 release if OOB is off now.
> >>
> >> Have you looked at making cur_mon per-thread?
> >
> > Above was my idea, nothing else has been done.
> >
> > Please feel free to post a patch for this, or I'll do this after 2.12
> > release.
>
> If it's fixed after 2.12, I think we should document the race as a known issue.
Do you mean this page?
https://wiki.qemu.org/Planning/2.12
To be simpler, I'll see whether I can post the patches soon, and
whether that can be accepted as 2.12 material.
--
Peter Xu
On Tue, Apr 10, 2018 at 03:15:57PM +0800, Peter Xu wrote:
> On Mon, Apr 09, 2018 at 11:19:43AM +0200, Marc-André Lureau wrote:
> > Hi
> >
> > On Sun, Apr 8, 2018 at 5:02 AM, Peter Xu <peterx@redhat.com> wrote:
> > > On Wed, Apr 04, 2018 at 03:58:56PM +0200, Marc-André Lureau wrote:
> > >> Hi Peter
> > >>
> > >> On Wed, Mar 28, 2018 at 6:02 AM, Peter Xu <peterx@redhat.com> wrote:
> > >> > On Mon, Mar 26, 2018 at 11:46:13AM +0200, Marc-André Lureau wrote:
> > >> >> Hi
> > >> >>
> > >> >> On Mon, Mar 26, 2018 at 11:08 AM, Peter Xu <peterx@redhat.com> wrote:
> > >> >> > On Mon, Mar 26, 2018 at 10:33:27AM +0200, Marc-André Lureau wrote:
> > >> >> >> Hi
> > >> >> >>
> > >> >> >> On Mon, Mar 26, 2018 at 10:07 AM, Peter Xu <peterx@redhat.com> wrote:
> > >> >> >> > On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:
> > >> >> >> >
> > >> >> >> > [...]
> > >> >> >> >
> > >> >> >> >> > +/*
> > >> >> >> >> > + * Dispatch one single QMP request. The function will free the req_obj
> > >> >> >> >> > + * and objects inside it before return.
> > >> >> >> >> > + */
> > >> >> >> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> > >> >> >> >> > {
> > >> >> >> >> > - QObject *req, *rsp = NULL, *id = NULL;
> > >> >> >> >> > + Monitor *mon, *old_mon;
> > >> >> >> >> > + QObject *req, *rsp = NULL, *id;
> > >> >> >> >> > QDict *qdict = NULL;
> > >> >> >> >> > - MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, parser);
> > >> >> >> >> > - Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, qmp);
> > >> >> >> >> > -
> > >> >> >> >> > - Error *err = NULL;
> > >> >> >> >> > + bool need_resume;
> > >> >> >> >> >
> > >> >> >> >> > - req = json_parser_parse_err(tokens, NULL, &err);
> > >> >> >> >> > - if (!req && !err) {
> > >> >> >> >> > - /* json_parser_parse_err() sucks: can fail without setting @err */
> > >> >> >> >> > - error_setg(&err, QERR_JSON_PARSING);
> > >> >> >> >> > - }
> > >> >> >> >> > - if (err) {
> > >> >> >> >> > - goto err_out;
> > >> >> >> >> > - }
> > >> >> >> >> > + req = req_obj->req;
> > >> >> >> >> > + mon = req_obj->mon;
> > >> >> >> >> > + id = req_obj->id;
> > >> >> >> >> > + need_resume = req_obj->need_resume;
> > >> >> >> >> >
> > >> >> >> >> > - qdict = qobject_to_qdict(req);
> > >> >> >> >> > - if (qdict) {
> > >> >> >> >> > - id = qdict_get(qdict, "id");
> > >> >> >> >> > - qobject_incref(id);
> > >> >> >> >> > - qdict_del(qdict, "id");
> > >> >> >> >> > - } /* else will fail qmp_dispatch() */
> > >> >> >> >> > + g_free(req_obj);
> > >> >> >> >> >
> > >> >> >> >> > if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
> > >> >> >> >> > QString *req_json = qobject_to_json(req);
> > >> >> >> >> > @@ -3900,7 +3932,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> > >> >> >> >> > old_mon = cur_mon;
> > >> >> >> >> > cur_mon = mon;
> > >> >> >> >>
> > >> >> >> >> There is another issue with this series, since cur_mon is global (and
> > >> >> >> >> not protected), an oob command may change the cur_mon while another
> > >> >> >> >> command is running in the main thread with unexpected consequences. I
> > >> >> >> >> don't have a clear idea what is the best way to solve it. Making the
> > >> >> >> >> variable per-thread, or going all the way to get rid of cur_mon (my
> > >> >> >> >> preference, but much harder)
> > >> >> >> >
> > >> >> >> > IMHO it is fine too.
> > >> >> >> >
> > >> >> >> > Note that this cur_mon operation is in monitor_qmp_dispatch_one() now,
> > >> >> >> > which is still running in main thread. So AFAICT all the cur_mon
> > >> >> >> > references are in main thread, and monitor IOThread does not modify
> > >> >> >> > that variable at all. Then we should probably be safe.
> > >> >> >>
> > >> >> >> But monitor_qmp_dispatch_one() is called from iothread if the command
> > >> >> >> is oob, so cur_mon may be updated while another command is running in
> > >> >> >> main thread, or am I wrong?
> > >> >> >
> > >> >> > You are right. I missed that, sorry...
> > >> >> >
> > >> >> > Would this be a simple workaround (but hopefully efficient) solution?
> > >> >> >
> > >> >> > diff --git a/monitor.c b/monitor.c
> > >> >> > index 77f4c41cfa..99641c0c6d 100644
> > >> >> > --- a/monitor.c
> > >> >> > +++ b/monitor.c
> > >> >> > @@ -4023,7 +4023,7 @@ typedef struct QMPRequest QMPRequest;
> > >> >> > * Dispatch one single QMP request. The function will free the req_obj
> > >> >> > * and objects inside it before return.
> > >> >> > */
> > >> >> > -static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> > >> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj, bool hack_curmon)
> > >> >> > {
> > >> >> > Monitor *mon, *old_mon;
> > >> >> > QObject *req, *rsp = NULL, *id;
> > >> >> > @@ -4043,12 +4043,16 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> > >> >> > QDECREF(req_json);
> > >> >> > }
> > >> >> >
> > >> >> > - old_mon = cur_mon;
> > >> >> > - cur_mon = mon;
> > >> >> > + if (hack_curmon) {
> > >> >> > + old_mon = cur_mon;
> > >> >> > + cur_mon = mon;
> > >> >> > + }
> > >> >> >
> > >> >> > rsp = qmp_dispatch(mon->qmp.commands, req);
> > >> >> >
> > >> >> > - cur_mon = old_mon;
> > >> >> > + if (hack_curmon) {
> > >> >> > + cur_mon = old_mon;
> > >> >> > + }
> > >> >> >
> > >> >> > if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
> > >> >> > qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error");
> > >> >> > @@ -4116,7 +4120,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
> > >> >> >
> > >> >> > if (req_obj) {
> > >> >> > trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
> > >> >> > - monitor_qmp_dispatch_one(req_obj);
> > >> >> > + monitor_qmp_dispatch_one(req_obj, true);
> > >> >> > /* Reschedule instead of looping so the main loop stays responsive */
> > >> >> > qemu_bh_schedule(mon_global.qmp_dispatcher_bh);
> > >> >> > }
> > >> >> > @@ -4175,7 +4179,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> > >> >> > /* Out-Of-Band (OOB) requests are executed directly in parser. */
> > >> >> > trace_monitor_qmp_cmd_out_of_band(qobject_get_try_str(req_obj->id)
> > >> >> > ?: "");
> > >> >> > - monitor_qmp_dispatch_one(req_obj);
> > >> >> > + monitor_qmp_dispatch_one(req_obj, false);
> > >> >> > return;
> > >> >> > }
> > >> >> >
> > >> >> > Then we forbit touching that evil cur_mon in OOB-capable command
> > >> >> > handlers. Thanks,
> > >> >>
> > >> >> That's not easy to enforce though, afaict it is being used for:
> > >> >> - error reporting decision
> > >> >
> > >> > IMO this should not be a problem, since any QMP handler (including
> > >> > OOB-capable ones) will be with an Error** there, so logically speaking
> > >> > people should never call things like error_report() in that.
> > >> >
> > >> >> - file & socket lookup (fd: & /dev/fdset etc)
> > >> >
> > >> > I suppose only very rare commands will use it? It'll be a big problem
> > >> > to solve when we want to completely remove cur_mon though.
> > >> >
> > >> >> - the current state of the monitor / list of commands, cpu_path, capabilities..
> > >> >
> > >> > This is very rare to be used too? Most commands should not use them AFAIU.
> > >> >
> > >> >>
> > >> >> Wouldn't it be simpler to make it per-thread? I think it could also
> > >> >> use helpers to push/pop the current monitor.
> > >> >
> > >> > Anyway I think yes this is still a good option (though the cur_mon
> > >> > logic will be a bit more complicated).
> > >> >
> > >> > Do you plan to post some patch about this, or do you want me to do
> > >> > this? I suppose we'll change the qemu_thread_create() a bit to pass
> > >> > the cur_mon inside, and I suppose this might be better material after
> > >> > 2.12 release if OOB is off now.
> > >>
> > >> Have you looked at making cur_mon per-thread?
> > >
> > > Above was my idea, nothing else has been done.
> > >
> > > Please feel free to post a patch for this, or I'll do this after 2.12
> > > release.
> >
> > If it's fixed after 2.12, I think we should document the race as a known issue.
>
> Do you mean this page?
>
> https://wiki.qemu.org/Planning/2.12
>
> To be simpler, I'll see whether I can post the patches soon, and
> whether that can be accepted as 2.12 material.
Btw I think it's not a 2.12 "known issue" - IMHO now it's not an issue
at all. Because we don't have any real command support OOB (let's
ignore the x-oob-test command since it never touches cur_mon). So
cur_mon will still only be accessed by the main thread but never
anything else.
I'll post the patch as usual in case further OOB commands will touch
cur_mon, but I suppose that'll be for after the release.
--
Peter Xu
© 2016 - 2026 Red Hat, Inc.