From nobody Tue Nov 4 18:50:21 2025 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) client-ip=208.118.235.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Authentication-Results: mx.zohomail.com; spf=pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=fail(p=none dis=none) header.from=redhat.com Return-Path: Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) by mx.zohomail.com with SMTPS id 1530376540634701.1319886375198; Sat, 30 Jun 2018 09:35:40 -0700 (PDT) Received: from localhost ([::1]:47414 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fZIqZ-0005jr-V2 for importer@patchew.org; Sat, 30 Jun 2018 12:35:39 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55934) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fZIiz-0008To-PM for qemu-devel@nongnu.org; Sat, 30 Jun 2018 12:27:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fZIiy-0007aA-A4 for qemu-devel@nongnu.org; Sat, 30 Jun 2018 12:27:49 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:39146 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fZIiy-0007ZW-4T for qemu-devel@nongnu.org; Sat, 30 Jun 2018 12:27:48 -0400 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BFFA17B4A7 for ; Sat, 30 Jun 2018 16:27:47 +0000 (UTC) Received: from blackfin.pond.sub.org (ovpn-116-16.ams2.redhat.com [10.36.116.16]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D71A71C599; Sat, 30 Jun 2018 16:27:45 +0000 (UTC) Received: by blackfin.pond.sub.org (Postfix, from userid 1000) id F16971132DA0; Sat, 30 Jun 2018 18:27:44 +0200 (CEST) From: Markus Armbruster To: qemu-devel@nongnu.org Date: Sat, 30 Jun 2018 18:27:42 +0200 Message-Id: <20180630162744.15920-4-armbru@redhat.com> In-Reply-To: <20180630162744.15920-1-armbru@redhat.com> References: <20180630162744.15920-1-armbru@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Sat, 30 Jun 2018 16:27:47 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Sat, 30 Jun 2018 16:27:47 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'armbru@redhat.com' RCPT:'' X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 66.187.233.73 Subject: [Qemu-devel] [PULL 3/5] monitor: flush qmp responses when CLOSED X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Peter Xu Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: "Qemu-devel" X-ZohoMail: RSF_0 Z_629925259 SPT_0 Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" From: Peter Xu Previously we clean up the queues when we got CLOSED event. It was used to make sure we won't send leftover replies/events of a old client to a new client which makes perfect sense. However this will also drop the replies/events even if the output port of the previous chardev backend is still open, which can lead to missing of the last replies/events. Now this patch does an extra operation to flush the response queue before cleaning up. In most cases, a QMP session will be based on a bidirectional channel (a TCP port, for example, we read/write to the same socket handle), so in port and out port of the backend chardev are fundamentally the same port. In these cases, it does not really matter much on whether we'll flush the response queue since flushing will fail anyway. However there can be cases where in & out ports of the QMP monitor's backend chardev are separated. Here is an example: cat $QMP_COMMANDS | qemu -qmp stdio ... | filter_commands In this case, the backend is fd-typed, and it is connected to stdio where in port is stdin and out port is stdout. Now if we drop all the events on the response queue then filter_command process might miss some events that it might expect. The thing is that, when stdin closes, stdout might still be there alive! In practice, I encountered SHUTDOWN event missing when running test with iotest 087 with Out-Of-Band enabled. Here is one of the ways that this can happen (after "quit" command is executed and QEMU quits the main loop): 1. [main thread] QEMU queues a SHUTDOWN event into response queue. 2. "cat" terminates (to distinguish it from the animal, I quote it). 3. [monitor iothread] QEMU's monitor iothread reads EOF from stdin. 4. [monitor iothread] QEMU's monitor iothread calls the CLOSED event hook for the monitor, which will destroy the response queue of the monitor, then the SHUTDOWN event is dropped. 5. [main thread] QEMU's main thread cleans up the monitors in monitor_cleanup(). When trying to flush pending responses, it sees nothing. SHUTDOWN is lost forever. Note that before the monitor iothread was introduced, step [4]/[5] could never happen since the main loop was the only place to detect the EOF event of stdin and run the CLOSED event hooks. Now things can happen in parallel in the iothread. Without this patch, iotest 087 will have ~10% chance to miss the SHUTDOWN event and fail when with Out-Of-Band enabled: --- /home/peterx/git/qemu/tests/qemu-iotests/087.out +++ /home/peterx/git/qemu/bin/tests/qemu-iotests/087.out.bad @@ -8,7 +8,6 @@ {"return": {}} {"error": {"class": "GenericError", "desc": "'node-name' must be specified for the root node"}} {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event"= : "SHUTDOWN", "data": {"guest": false}} =3D=3D=3D Duplicate ID =3D=3D=3D @@ -53,7 +52,6 @@ {"return": {}} {"return": {}} {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event"= : "SHUTDOWN", "data": {"guest": false}} This patch fixes the problem. Fixes: 6d2d563f8c ("qmp: cleanup qmp queues properly", 2018-03-27) Suggested-by: Markus Armbruster Signed-off-by: Peter Xu Message-Id: <20180620073223.31964-4-peterx@redhat.com> Reviewed-by: Markus Armbruster [Commit message and a comment touched up] Signed-off-by: Markus Armbruster --- monitor.c | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/monitor.c b/monitor.c index 2881345758..567668a0e7 100644 --- a/monitor.c +++ b/monitor.c @@ -541,6 +541,27 @@ struct QMPResponse { }; typedef struct QMPResponse QMPResponse; =20 +static QObject *monitor_qmp_response_pop_one(Monitor *mon) +{ + QObject *data; + + qemu_mutex_lock(&mon->qmp.qmp_queue_lock); + data =3D g_queue_pop_head(mon->qmp.qmp_responses); + qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); + + return data; +} + +static void monitor_qmp_response_flush(Monitor *mon) +{ + QObject *data; + + while ((data =3D monitor_qmp_response_pop_one(mon))) { + monitor_json_emitter_raw(mon, data); + qobject_unref(data); + } +} + /* * Pop a QMPResponse from any monitor's response queue into @response. * Return false if all the queues are empty; else true. @@ -552,9 +573,7 @@ static bool monitor_qmp_response_pop_any(QMPResponse *r= esponse) =20 qemu_mutex_lock(&monitor_lock); QTAILQ_FOREACH(mon, &mon_list, entry) { - qemu_mutex_lock(&mon->qmp.qmp_queue_lock); - data =3D g_queue_pop_head(mon->qmp.qmp_responses); - qemu_mutex_unlock(&mon->qmp.qmp_queue_lock); + data =3D monitor_qmp_response_pop_one(mon); if (data) { response->mon =3D mon; response->data =3D data; @@ -4456,6 +4475,13 @@ static void monitor_qmp_event(void *opaque, int even= t) mon_refcount++; break; case CHR_EVENT_CLOSED: + /* + * Note: this is only useful when the output of the chardev + * backend is still open. For example, when the backend is + * stdio, it's possible that stdout is still open when stdin + * is closed. + */ + monitor_qmp_response_flush(mon); monitor_qmp_cleanup_queues(mon); json_message_parser_destroy(&mon->qmp.parser); json_message_parser_init(&mon->qmp.parser, handle_qmp_command); --=20 2.17.1