79 lines
3.4 KiB
Diff
79 lines
3.4 KiB
Diff
From 4d4102f6e2f9afd6182888787ae8b570347df87d Mon Sep 17 00:00:00 2001
|
|
From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= <berrange@redhat.com>
|
|
Date: Mon, 18 Mar 2024 18:06:59 +0000
|
|
Subject: [PATCH 1/3] chardev: lower priority of the HUP GSource in socket
|
|
chardev
|
|
MIME-Version: 1.0
|
|
Content-Type: text/plain; charset=UTF-8
|
|
Content-Transfer-Encoding: 8bit
|
|
|
|
RH-Author: Daniel P. Berrangé <berrange@redhat.com>
|
|
RH-MergeRequest: 233: Fix handling of TLS sessions in chardevs
|
|
RH-Jira: RHEL-24614
|
|
RH-Acked-by: Thomas Huth <thuth@redhat.com>
|
|
RH-Acked-by: Marc-André Lureau <marcandre.lureau@redhat.com>
|
|
RH-Commit: [1/3] 842f54349191b0206e68f35a7a80155f5a584942 (berrange/centos-src-qemu)
|
|
|
|
The socket chardev often has 2 GSource object registered against the
|
|
same FD. One is registered all the time and is just intended to handle
|
|
POLLHUP events, while the other gets registered & unregistered on the
|
|
fly as the frontend is ready to receive more data or not.
|
|
|
|
It is very common for poll() to signal a POLLHUP event at the same time
|
|
as there is pending incoming data from the disconnected client. It is
|
|
therefore essential to process incoming data prior to processing HUP.
|
|
The problem with having 2 GSource on the same FD is that there is no
|
|
guaranteed ordering of execution between them, so the chardev code may
|
|
process HUP first and thus discard data.
|
|
|
|
This failure scenario is non-deterministic but can be seen fairly
|
|
reliably by reverting a7077b8e354d90fec26c2921aa2dea85b90dff90, and
|
|
then running 'tests/unit/test-char', which will sometimes fail with
|
|
missing data.
|
|
|
|
Ideally QEMU would only have 1 GSource, but that's a complex code
|
|
refactoring job. The next best solution is to try to ensure ordering
|
|
between the 2 GSource objects. This can be achieved by lowering the
|
|
priority of the HUP GSource, so that it is never dispatched if the
|
|
main GSource is also ready to dispatch. Counter-intuitively, lowering
|
|
the priority of a GSource is done by raising its priority number.
|
|
|
|
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
|
|
Reviewed-by: Thomas Huth <thuth@redhat.com>
|
|
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
|
|
(cherry picked from commit 8bd8b04adc9f18904f323dff085f8b4ec77915c6)
|
|
---
|
|
chardev/char-socket.c | 16 ++++++++++++++++
|
|
1 file changed, 16 insertions(+)
|
|
|
|
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
|
|
index 034840593d..f48d341ebc 100644
|
|
--- a/chardev/char-socket.c
|
|
+++ b/chardev/char-socket.c
|
|
@@ -597,6 +597,22 @@ static void update_ioc_handlers(SocketChardev *s)
|
|
|
|
remove_hup_source(s);
|
|
s->hup_source = qio_channel_create_watch(s->ioc, G_IO_HUP);
|
|
+ /*
|
|
+ * poll() is liable to return POLLHUP even when there is
|
|
+ * still incoming data available to read on the FD. If
|
|
+ * we have the hup_source at the same priority as the
|
|
+ * main io_add_watch_poll GSource, then we might end up
|
|
+ * processing the POLLHUP event first, closing the FD,
|
|
+ * and as a result silently discard data we should have
|
|
+ * read.
|
|
+ *
|
|
+ * By setting the hup_source to G_PRIORITY_DEFAULT + 1,
|
|
+ * we ensure that io_add_watch_poll GSource will always
|
|
+ * be dispatched first, thus guaranteeing we will be
|
|
+ * able to process all incoming data before closing the
|
|
+ * FD
|
|
+ */
|
|
+ g_source_set_priority(s->hup_source, G_PRIORITY_DEFAULT + 1);
|
|
g_source_set_callback(s->hup_source, (GSourceFunc)tcp_chr_hup,
|
|
chr, NULL);
|
|
g_source_attach(s->hup_source, chr->gcontext);
|
|
--
|
|
2.39.3
|
|
|