205 lines
7.3 KiB
Diff
205 lines
7.3 KiB
Diff
From 4f0a31d66c2a6588495b8ae682f555584dafdf45 Mon Sep 17 00:00:00 2001
|
|
From: Claudio Saavedra <csaavedra@igalia.com>
|
|
Date: Wed, 21 Oct 2020 13:19:42 +0300
|
|
Subject: [PATCH] gmain: g_main_context_check() can skip updating polled FD
|
|
sources
|
|
|
|
If there is a file descriptor source that has a lower priority
|
|
than the one for sources that are going to be dispatched,
|
|
all subsequent file descriptor sources (internally sorted by
|
|
file descriptor identifier) do not get an update in their GPollRec
|
|
and later on wrong sources can be dispatched.
|
|
|
|
Fix this by first finding the first GPollRec that matches the current
|
|
GPollFD, instead of relying on it to be the current one. At
|
|
the same time, document the assumptions about the ordering of the
|
|
file descriptor records and array and make explicit in the documentation
|
|
that the array needs to be passed to g_main_context_check() as it was
|
|
received from g_main_context_query().
|
|
|
|
Added a new test that reproduces the bug by creating two file
|
|
descriptor sources and an idle one. Since the first
|
|
file descriptor created has a lower identifier and a low priority,
|
|
the second one is not dispatched even when it has the same, higher,
|
|
priority as the idle source. After fixing this bug, both
|
|
higher priority sources are dispatched as expected.
|
|
|
|
While this patch was written independently, a similar fix for this
|
|
bug was first submitted by Eugene M in GNOME/glib!562. Having a
|
|
second fix that basically does the same is a reassurance that we
|
|
are in the right here.
|
|
|
|
Fixes #1592
|
|
---
|
|
glib/gmain.c | 32 ++++++++++++++++++++++--
|
|
glib/tests/mainloop.c | 57 +++++++++++++++++++++++++++++++++++++++++++
|
|
2 files changed, 87 insertions(+), 2 deletions(-)
|
|
|
|
diff --git a/glib/gmain.c b/glib/gmain.c
|
|
index 95992253d..a59cd686c 100644
|
|
--- a/glib/gmain.c
|
|
+++ b/glib/gmain.c
|
|
@@ -3573,7 +3573,10 @@ g_main_context_prepare (GMainContext *context,
|
|
* store #GPollFD records that need to be polled.
|
|
* @n_fds: (in): length of @fds.
|
|
*
|
|
- * Determines information necessary to poll this main loop.
|
|
+ * Determines information necessary to poll this main loop. You should
|
|
+ * be careful to pass the resulting @fds array and its length @n_fds
|
|
+ * as is when calling g_main_context_check(), as this function relies
|
|
+ * on assumptions made when the array is filled.
|
|
*
|
|
* You must have successfully acquired the context with
|
|
* g_main_context_acquire() before you may call this function.
|
|
@@ -3597,6 +3600,10 @@ g_main_context_query (GMainContext *context,
|
|
|
|
TRACE (GLIB_MAIN_CONTEXT_BEFORE_QUERY (context, max_priority));
|
|
|
|
+ /* fds is filled sequentially from poll_records. Since poll_records
|
|
+ * are incrementally sorted by file descriptor identifier, fds will
|
|
+ * also be incrementally sorted.
|
|
+ */
|
|
n_poll = 0;
|
|
lastpollrec = NULL;
|
|
for (pollrec = context->poll_records; pollrec; pollrec = pollrec->next)
|
|
@@ -3611,6 +3618,10 @@ g_main_context_query (GMainContext *context,
|
|
*/
|
|
events = pollrec->fd->events & ~(G_IO_ERR|G_IO_HUP|G_IO_NVAL);
|
|
|
|
+ /* This optimization --using the same GPollFD to poll for more
|
|
+ * than one poll record-- relies on the poll records being
|
|
+ * incrementally sorted.
|
|
+ */
|
|
if (lastpollrec && pollrec->fd->fd == lastpollrec->fd->fd)
|
|
{
|
|
if (n_poll - 1 < n_fds)
|
|
@@ -3656,7 +3667,10 @@ g_main_context_query (GMainContext *context,
|
|
* the last call to g_main_context_query()
|
|
* @n_fds: return value of g_main_context_query()
|
|
*
|
|
- * Passes the results of polling back to the main loop.
|
|
+ * Passes the results of polling back to the main loop. You should be
|
|
+ * careful to pass @fds and its length @n_fds as received from
|
|
+ * g_main_context_query(), as this functions relies on assumptions
|
|
+ * on how @fds is filled.
|
|
*
|
|
* You must have successfully acquired the context with
|
|
* g_main_context_acquire() before you may call this function.
|
|
@@ -3711,10 +3725,22 @@ g_main_context_check (GMainContext *context,
|
|
return FALSE;
|
|
}
|
|
|
|
+ /* The linear iteration below relies on the assumption that both
|
|
+ * poll records and the fds array are incrementally sorted by file
|
|
+ * descriptor identifier.
|
|
+ */
|
|
pollrec = context->poll_records;
|
|
i = 0;
|
|
while (pollrec && i < n_fds)
|
|
{
|
|
+ /* Make sure that fds is sorted by file descriptor identifier. */
|
|
+ g_assert (i <= 0 || fds[i - 1].fd < fds[i].fd);
|
|
+
|
|
+ /* Skip until finding the first GPollRec matching the current GPollFD. */
|
|
+ while (pollrec && pollrec->fd->fd != fds[i].fd)
|
|
+ pollrec = pollrec->next;
|
|
+
|
|
+ /* Update all consecutive GPollRecs that match. */
|
|
while (pollrec && pollrec->fd->fd == fds[i].fd)
|
|
{
|
|
if (pollrec->priority <= max_priority)
|
|
@@ -3725,6 +3751,7 @@ g_main_context_check (GMainContext *context,
|
|
pollrec = pollrec->next;
|
|
}
|
|
|
|
+ /* Iterate to next GPollFD. */
|
|
i++;
|
|
}
|
|
|
|
@@ -4320,6 +4347,7 @@ g_main_context_add_poll_unlocked (GMainContext *context,
|
|
newrec->fd = fd;
|
|
newrec->priority = priority;
|
|
|
|
+ /* Poll records are incrementally sorted by file descriptor identifier. */
|
|
prevrec = NULL;
|
|
nextrec = context->poll_records;
|
|
while (nextrec)
|
|
diff --git a/glib/tests/mainloop.c b/glib/tests/mainloop.c
|
|
index f5d672a63..397921f2d 100644
|
|
--- a/glib/tests/mainloop.c
|
|
+++ b/glib/tests/mainloop.c
|
|
@@ -1511,6 +1511,62 @@ test_unix_file_poll (void)
|
|
close (fd);
|
|
}
|
|
|
|
+static void
|
|
+test_unix_fd_priority (void)
|
|
+{
|
|
+ gint fd1, fd2;
|
|
+ GMainLoop *loop;
|
|
+ GSource *source;
|
|
+
|
|
+ gint s1 = 0;
|
|
+ gboolean s2 = FALSE, s3 = FALSE;
|
|
+
|
|
+ g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/1592");
|
|
+
|
|
+ loop = g_main_loop_new (NULL, FALSE);
|
|
+
|
|
+ source = g_idle_source_new ();
|
|
+ g_source_set_callback (source, count_calls, &s1, NULL);
|
|
+ g_source_set_priority (source, 0);
|
|
+ g_source_attach (source, NULL);
|
|
+ g_source_unref (source);
|
|
+
|
|
+ fd1 = open ("/dev/random", O_RDONLY);
|
|
+ g_assert_cmpint (fd1, >=, 0);
|
|
+ source = g_unix_fd_source_new (fd1, G_IO_IN);
|
|
+ g_source_set_callback (source, (GSourceFunc) (void (*)(void)) (flag_bool), &s2, NULL);
|
|
+ g_source_set_priority (source, 10);
|
|
+ g_source_attach (source, NULL);
|
|
+ g_source_unref (source);
|
|
+
|
|
+ fd2 = open ("/dev/random", O_RDONLY);
|
|
+ g_assert_cmpint (fd2, >=, 0);
|
|
+ source = g_unix_fd_source_new (fd2, G_IO_IN);
|
|
+ g_source_set_callback (source, (GSourceFunc) (void (*)(void)) (flag_bool), &s3, NULL);
|
|
+ g_source_set_priority (source, 0);
|
|
+ g_source_attach (source, NULL);
|
|
+ g_source_unref (source);
|
|
+
|
|
+ /* This tests a bug that depends on the source with the lowest FD
|
|
+ identifier to have the lowest priority. Make sure that this is
|
|
+ the case. */
|
|
+ g_assert_cmpint (fd1, <, fd2);
|
|
+
|
|
+ g_assert_true (g_main_context_iteration (NULL, FALSE));
|
|
+
|
|
+ /* Idle source should have been dispatched. */
|
|
+ g_assert_cmpint (s1, ==, 1);
|
|
+ /* Low priority FD source shouldn't have been dispatched. */
|
|
+ g_assert_false (s2);
|
|
+ /* Default priority FD source should have been dispatched. */
|
|
+ g_assert_true (s3);
|
|
+
|
|
+ g_main_loop_unref (loop);
|
|
+
|
|
+ close (fd1);
|
|
+ close (fd2);
|
|
+}
|
|
+
|
|
#endif
|
|
|
|
static gboolean
|
|
@@ -1751,6 +1807,7 @@ main (int argc, char *argv[])
|
|
g_test_add_func ("/mainloop/source-unix-fd-api", test_source_unix_fd_api);
|
|
g_test_add_func ("/mainloop/wait", test_mainloop_wait);
|
|
g_test_add_func ("/mainloop/unix-file-poll", test_unix_file_poll);
|
|
+ g_test_add_func ("/mainloop/unix-fd-priority", test_unix_fd_priority);
|
|
#endif
|
|
g_test_add_func ("/mainloop/nfds", test_nfds);
|
|
|
|
--
|
|
2.31.1
|
|
|