On Mon, Mar 18, 2024 at 10:25 PM Daniel P. Berrangé <berra...@redhat.com> wrote: > > 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. > > Signed-off-by: Daniel P. Berrangé <berra...@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com> > --- > chardev/char-socket.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c > index 8a0406cc1e..2c4dffc0e6 100644 > --- a/chardev/char-socket.c > +++ b/chardev/char-socket.c > @@ -601,6 +601,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.43.0 > > -- Marc-André Lureau
