[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.12 2/8] qmp: cleanup qmp queues properly
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH for-2.12 2/8] qmp: cleanup qmp queues properly |
Date: |
Mon, 26 Mar 2018 14:42:23 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 |
On 03/26/2018 01:38 AM, Peter Xu wrote:
Marc-André Lureau reported that we can have this happen:
1. client1 connects, send command C1
2. client1 disconnects before getting response for C1
3. client2 connects, who might receive response of C1
However client2 should not receive remaining responses for client1.
Basically, we should clean up the request/response queue elements when:
- before a session established
Why here? [1]
- after a session is closed
- before destroying the queues
Some helpers are introduced to achieve that. We need to make sure we're
with the lock when operating on those queues.
It would also be helpful to mention that the patch includes code motion
to declare struct QMPRequest earlier in the file.
Reported-by: Marc-André Lureau <address@hidden>
Signed-off-by: Peter Xu <address@hidden>
---
monitor.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 59 insertions(+), 20 deletions(-)
+static void qmp_request_free(QMPRequest *req)
+{
+ qobject_decref(req->id);
+ qobject_decref(req->req);
+ g_free(req);
+}
+
+static void qmp_response_free(QObject *obj)
+{
+ qobject_decref(obj);
+}
Why do we need this function? Unless you plan to add to it in later
patches, I'd rather just inline things and directly call
qobject_decref() at the callsites...
+
+/* Must with the mon->qmp.qmp_queue_lock held */
+static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon)
+{
+ while (!g_queue_is_empty(mon->qmp.qmp_requests)) {
+ qmp_request_free(g_queue_pop_head(mon->qmp.qmp_requests));
+ }
+}
+
+/* Must with the mon->qmp.qmp_queue_lock held */
+static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon)
+{
+ while (!g_queue_is_empty(mon->qmp.qmp_responses)) {
+ qmp_response_free(g_queue_pop_head(mon->qmp.qmp_responses));
...here,
+ }
+}
+
+static void monitor_qmp_cleanup_queues(Monitor *mon)
+{
+ qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+ monitor_qmp_cleanup_req_queue_locked(mon);
+ monitor_qmp_cleanup_resp_queue_locked(mon);
+ qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+}
+
+
static void monitor_flush_locked(Monitor *mon);
static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
@@ -497,7 +550,7 @@ static void monitor_qmp_bh_responder(void *opaque)
break;
}
monitor_json_emitter_raw(response.mon, response.data);
- qobject_decref(response.data);
+ qmp_response_free(response.data);
and here.
@@ -4327,6 +4364,7 @@ static void monitor_qmp_event(void *opaque, int event)
switch (event) {
case CHR_EVENT_OPENED:
+ monitor_qmp_cleanup_queues(mon);
[1] How would something be queued to need cleanup at this point, if we
already start with a clean queue before the first monitor, and if all
monitor close actions clean the queue?
mon->qmp.commands = &qmp_cap_negotiation_commands;
monitor_qmp_caps_reset(mon);
data = get_qmp_greeting(mon);
@@ -4335,6 +4373,7 @@ static void monitor_qmp_event(void *opaque, int event)
mon_refcount++;
break;
case CHR_EVENT_CLOSED:
+ monitor_qmp_cleanup_queues(mon);
json_message_parser_destroy(&mon->qmp.parser);
json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
mon_refcount--;
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
- [Qemu-devel] [PATCH for-2.12 0/8] Monitor: some oob related patches (fixes, new param, tests), Peter Xu, 2018/03/26
- [Qemu-devel] [PATCH for-2.12 1/8] qmp: fix qmp_capabilities error regression, Peter Xu, 2018/03/26
- [Qemu-devel] [PATCH for-2.12 2/8] qmp: cleanup qmp queues properly, Peter Xu, 2018/03/26
- [Qemu-devel] [PATCH for-2.12 3/8] monitor: new parameter "x-oob", Peter Xu, 2018/03/26
- [Qemu-devel] [PATCH for-2.12 4/8] qapi: restrict allow-oob value to be "true", Peter Xu, 2018/03/26
- [Qemu-devel] [PATCH for-2.12 5/8] tests: let qapi-schema tests detect oob, Peter Xu, 2018/03/26