From 15e88379452e231d1821c0d7f8e4df89ccd86e8b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 8 Mar 2022 20:53:38 +0100 Subject: [PATCH 1/3] glib-aux: fix priority for nm_g_idle_add_source() nm_g_idle_add_source() is supposed to work like g_idle_add(). Use the correct priority. I think this causes little actual problems, because usually we don't carefully tune the priorities and would be mostly fine with either. Fixes: 6b18fc252d1e ('shared: add nm_g_{idle,timeout}_add_source() helpers') --- src/libnm-glib-aux/nm-shared-utils.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 46434033ed..6070bb0e0c 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -1789,7 +1789,7 @@ nm_g_idle_add_source(GSourceFunc func, gpointer user_data) /* A convenience function to attach a new timeout source to the default GMainContext. * In that sense it's very similar to g_idle_add() except that it returns a * reference to the new source. */ - return nm_g_source_attach(nm_g_idle_source_new(G_PRIORITY_DEFAULT, func, user_data, NULL), + return nm_g_source_attach(nm_g_idle_source_new(G_PRIORITY_DEFAULT_IDLE, func, user_data, NULL), NULL); } From 9b030a398895e554e0848f562dcd0fc307263288 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 8 Mar 2022 14:29:51 +0100 Subject: [PATCH 2/3] all: change scheduling priority for idle actions to G_PRIORITY_DEFAULT_IDLE g_idle_add() uses G_PRIORITY_DEFAULT_IDLE priority. Most of the time we don't care much about the priority. But at the places that this patch changes, I think that using G_PRIORITY_DEFAULT_IDLE (and following g_idle_add()) is more correct. The reason for this is not very strong, except that it's probably the better choice. And the old choice was made because I didn't realize that g_idle_add() uses another default priority. Hence, the old choice was not for good reasons either. --- src/core/nm-l3-ipv4ll.c | 6 ++++-- src/libnm-client-impl/nm-client.c | 5 ++++- src/libnm-glib-aux/nm-shared-utils.c | 12 +++++++++--- src/nm-cloud-setup/nm-cloud-setup-utils.c | 4 ++-- 4 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/core/nm-l3-ipv4ll.c b/src/core/nm-l3-ipv4ll.c index 68cb17fb09..503039843e 100644 --- a/src/core/nm-l3-ipv4ll.c +++ b/src/core/nm-l3-ipv4ll.c @@ -921,8 +921,10 @@ _ipv4ll_state_change_on_idle(NML3IPv4LL *self) nm_assert(NM_IS_L3_IPV4LL(self)); if (!self->state_change_on_idle_source) { - self->state_change_on_idle_source = - nm_g_idle_source_new(G_PRIORITY_DEFAULT, _ipv4ll_state_change_on_idle_cb, self, NULL); + self->state_change_on_idle_source = nm_g_idle_source_new(G_PRIORITY_DEFAULT_IDLE, + _ipv4ll_state_change_on_idle_cb, + self, + NULL); g_source_attach(self->state_change_on_idle_source, NULL); } } diff --git a/src/libnm-client-impl/nm-client.c b/src/libnm-client-impl/nm-client.c index cd2aa73055..f794ff9e93 100644 --- a/src/libnm-client-impl/nm-client.c +++ b/src/libnm-client-impl/nm-client.c @@ -7349,7 +7349,10 @@ _init_start_with_bus(NMClient *self) NULL); if (id == 0) { priv->init_data->cancel_on_idle_source = - nm_g_idle_source_new(G_PRIORITY_DEFAULT, _init_start_cancel_on_idle_cb, self, NULL); + nm_g_idle_source_new(G_PRIORITY_DEFAULT_IDLE, + _init_start_cancel_on_idle_cb, + self, + NULL); g_source_attach(priv->init_data->cancel_on_idle_source, priv->main_context); return; } diff --git a/src/libnm-glib-aux/nm-shared-utils.c b/src/libnm-glib-aux/nm-shared-utils.c index 847e85bb67..f059741e23 100644 --- a/src/libnm-glib-aux/nm-shared-utils.c +++ b/src/libnm-glib-aux/nm-shared-utils.c @@ -4691,14 +4691,20 @@ _nm_utils_invoke_on_idle_start(gboolean use_timeout, } if (use_timeout) { + /* We use G_PRIORITY_DEFAULT_IDLE both for the with/without timeout + * case. The reason is not strong, but it seems right that the caller + * requests a lower priority than G_PRIORITY_DEFAULT. That is unlike + * what g_timeout_add() would do. */ source = nm_g_timeout_source_new(timeout_msec, - G_PRIORITY_DEFAULT, + G_PRIORITY_DEFAULT_IDLE, _nm_utils_invoke_on_idle_cb_idle, data, NULL); } else { - source = - nm_g_idle_source_new(G_PRIORITY_DEFAULT, _nm_utils_invoke_on_idle_cb_idle, data, NULL); + source = nm_g_idle_source_new(G_PRIORITY_DEFAULT_IDLE, + _nm_utils_invoke_on_idle_cb_idle, + data, + NULL); } /* use the current thread default context. */ diff --git a/src/nm-cloud-setup/nm-cloud-setup-utils.c b/src/nm-cloud-setup/nm-cloud-setup-utils.c index bf705549f3..e505f8bd03 100644 --- a/src/nm-cloud-setup/nm-cloud-setup-utils.c +++ b/src/nm-cloud-setup/nm-cloud-setup-utils.c @@ -146,7 +146,7 @@ nmcs_wait_for_objects_iterate_until_done(GMainContext *context, int timeout_msec nm_auto_destroy_and_unref_gsource GSource *idle_source = NULL; idle_source = - nm_g_source_attach(nm_g_idle_source_new(G_PRIORITY_DEFAULT, + nm_g_source_attach(nm_g_idle_source_new(G_PRIORITY_DEFAULT_IDLE, _wait_for_objects_iterate_until_done_idle_cb, &data, NULL), @@ -364,7 +364,7 @@ nmcs_utils_poll(int poll_timeout_ms, } poll_task_data->source_next_poll = nm_g_source_attach( - nm_g_idle_source_new(G_PRIORITY_DEFAULT, _poll_start_cb, poll_task_data, NULL), + nm_g_idle_source_new(G_PRIORITY_DEFAULT_IDLE, _poll_start_cb, poll_task_data, NULL), poll_task_data->context); if (cancellable) { From 216c46c8816ebc5df2a063f7b22a869ebf07fd56 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 8 Mar 2022 14:31:53 +0100 Subject: [PATCH 3/3] all: prefer nm wrappers to automatically attach GSource to default context We often create the source with default priority, no destroy function and attach it to the default context (g_main_context_default()). For that case, we have wrapper functions like nm_g_timeout_add_source() and nm_g_idle_add_source(). Use those. There should be no change in behavior. --- src/core/nm-l3-ipv4ll.c | 15 ++++----------- src/core/nm-l3cfg.c | 4 +--- src/core/nm-session-monitor.c | 7 ++----- src/core/platform/tests/test-common.c | 8 +------- src/libnm-platform/nm-linux-platform.c | 7 ++----- src/nm-cloud-setup/main.c | 7 +------ src/nm-cloud-setup/nm-http-client.c | 15 +++++++-------- src/nmcli/common.c | 8 +------- src/nmcli/connections.c | 9 ++++++--- 9 files changed, 25 insertions(+), 55 deletions(-) diff --git a/src/core/nm-l3-ipv4ll.c b/src/core/nm-l3-ipv4ll.c index 503039843e..24f33c6771 100644 --- a/src/core/nm-l3-ipv4ll.c +++ b/src/core/nm-l3-ipv4ll.c @@ -748,12 +748,8 @@ _ipv4ll_set_timed_out_update(NML3IPv4LL *self, TimedOutState new_state) if (self->timed_out_expiry_msec == 0 || self->timed_out_expiry_msec < expiry_msec) { self->timed_out_expiry_msec = expiry_msec; nm_clear_g_source_inst(&self->timed_out_source); - self->timed_out_source = nm_g_timeout_source_new(timeout_msec, - G_PRIORITY_DEFAULT, - _ipv4ll_set_timed_out_timeout_cb, - self, - NULL); - g_source_attach(self->timed_out_source, NULL); + self->timed_out_source = + nm_g_timeout_add_source(timeout_msec, _ipv4ll_set_timed_out_timeout_cb, self); } break; } @@ -921,11 +917,8 @@ _ipv4ll_state_change_on_idle(NML3IPv4LL *self) nm_assert(NM_IS_L3_IPV4LL(self)); if (!self->state_change_on_idle_source) { - self->state_change_on_idle_source = nm_g_idle_source_new(G_PRIORITY_DEFAULT_IDLE, - _ipv4ll_state_change_on_idle_cb, - self, - NULL); - g_source_attach(self->state_change_on_idle_source, NULL); + self->state_change_on_idle_source = + nm_g_idle_add_source(_ipv4ll_state_change_on_idle_cb, self); } } diff --git a/src/core/nm-l3cfg.c b/src/core/nm-l3cfg.c index eeb041d042..e178b10907 100644 --- a/src/core/nm-l3cfg.c +++ b/src/core/nm-l3cfg.c @@ -1743,9 +1743,7 @@ again: n_acd_get_fd(self->priv.p->nacd, &fd); - self->priv.p->nacd_source = - nm_g_unix_fd_source_new(fd, G_IO_IN, G_PRIORITY_DEFAULT, _l3_acd_nacd_event, self, NULL); - nm_g_source_attach(self->priv.p->nacd_source, NULL); + self->priv.p->nacd_source = nm_g_unix_fd_add_source(fd, G_IO_IN, _l3_acd_nacd_event, self); NM_SET_OUT(out_acd_not_supported, FALSE); return self->priv.p->nacd; diff --git a/src/core/nm-session-monitor.c b/src/core/nm-session-monitor.c index 08bfedca6f..a2e9373c88 100644 --- a/src/core/nm-session-monitor.c +++ b/src/core/nm-session-monitor.c @@ -114,13 +114,10 @@ st_sd_init(NMSessionMonitor *monitor) return; } - monitor->sd.watch = nm_g_unix_fd_source_new(sd_login_monitor_get_fd(monitor->sd.monitor), + monitor->sd.watch = nm_g_unix_fd_add_source(sd_login_monitor_get_fd(monitor->sd.monitor), G_IO_IN, - G_PRIORITY_DEFAULT, st_sd_changed, - monitor, - NULL); - g_source_attach(monitor->sd.watch, NULL); + monitor); } static void diff --git a/src/core/platform/tests/test-common.c b/src/core/platform/tests/test-common.c index 864c97dd10..2137b8781c 100644 --- a/src/core/platform/tests/test-common.c +++ b/src/core/platform/tests/test-common.c @@ -2778,13 +2778,7 @@ nmtstp_acd_defender_new(int ifindex, in_addr_t ip_addr, const NMEtherAddr *mac_a n_acd_get_fd(defender->nacd, &fd); g_assert_cmpint(fd, >=, 0); - defender->source = nm_g_source_attach(nm_g_unix_fd_source_new(fd, - G_IO_IN, - G_PRIORITY_DEFAULT, - _l3_acd_nacd_event, - defender, - NULL), - NULL); + defender->source = nm_g_unix_fd_add_source(fd, G_IO_IN, _l3_acd_nacd_event, defender); return defender; } diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index e184ed4a2a..b0fc16d780 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -9759,13 +9759,10 @@ constructed(GObject *_object) fd); priv->event_source = - nm_g_unix_fd_source_new(fd, + nm_g_unix_fd_add_source(fd, G_IO_IN | G_IO_NVAL | G_IO_PRI | G_IO_ERR | G_IO_HUP, - G_PRIORITY_DEFAULT, event_handler, - platform, - NULL); - g_source_attach(priv->event_source, NULL); + platform); /* complete construction of the GObject instance before populating the cache. */ G_OBJECT_CLASS(nm_linux_platform_parent_class)->constructed(_object); diff --git a/src/nm-cloud-setup/main.c b/src/nm-cloud-setup/main.c index f6acadca33..0c452acfe3 100644 --- a/src/nm-cloud-setup/main.c +++ b/src/nm-cloud-setup/main.c @@ -625,12 +625,7 @@ main(int argc, const char *const *argv) sigterm_cancellable = g_cancellable_new(); - sigterm_source = nm_g_source_attach(nm_g_unix_signal_source_new(SIGTERM, - G_PRIORITY_DEFAULT, - sigterm_handler, - sigterm_cancellable, - NULL), - NULL); + sigterm_source = nm_g_unix_signal_add_source(SIGTERM, sigterm_handler, sigterm_cancellable); provider = _provider_detect(sigterm_cancellable); if (!provider) diff --git a/src/nm-cloud-setup/nm-http-client.c b/src/nm-cloud-setup/nm-http-client.c index 7ef9f38d34..a0053d472d 100644 --- a/src/nm-cloud-setup/nm-http-client.c +++ b/src/nm-cloud-setup/nm-http-client.c @@ -680,14 +680,13 @@ _mhandle_socketfunction_cb(CURL *e_handle, condition = 0; if (condition) { - source_socket = nm_g_unix_fd_source_new(fd, - condition, - G_PRIORITY_DEFAULT, - _mhandle_socket_cb, - self, - NULL); - g_source_attach(source_socket, priv->context); - + source_socket = _source_attach(self, + nm_g_unix_fd_source_new(fd, + condition, + G_PRIORITY_DEFAULT, + _mhandle_socket_cb, + self, + NULL)); g_hash_table_insert(priv->source_sockets_hashtable, GINT_TO_POINTER(fd), source_socket); } } diff --git a/src/nmcli/common.c b/src/nmcli/common.c index 26398c8429..f42f76e4c2 100644 --- a/src/nmcli/common.c +++ b/src/nmcli/common.c @@ -889,13 +889,7 @@ nmc_readline_helper(const NmcConfig *nmc_config, const char *prompt) nmc_set_in_readline(TRUE); - io_source = nm_g_unix_fd_source_new(STDIN_FILENO, - G_IO_IN, - G_PRIORITY_DEFAULT, - stdin_ready_cb, - NULL, - NULL); - g_source_attach(io_source, NULL); + io_source = nm_g_unix_fd_add_source(STDIN_FILENO, G_IO_IN, stdin_ready_cb, NULL); read_again: rl_string = NULL; diff --git a/src/nmcli/connections.c b/src/nmcli/connections.c index 025f96a179..a045fff0bc 100644 --- a/src/nmcli/connections.c +++ b/src/nmcli/connections.c @@ -8280,9 +8280,12 @@ editor_menu_main(NmCli *nmc, NMConnection *connection, const char *connection_ty connection_changed = FALSE; } - source = g_timeout_source_new(10 * NM_UTILS_MSEC_PER_SEC); - g_source_set_callback(source, editor_save_timeout, &timeout, NULL); - g_source_attach(source, g_main_loop_get_context(loop)); + source = nm_g_source_attach(nm_g_timeout_source_new(10 * NM_UTILS_MSEC_PER_SEC, + G_PRIORITY_DEFAULT, + editor_save_timeout, + &timeout, + NULL), + g_main_loop_get_context(loop)); while (!nmc_editor_cb_called && !timeout) g_main_context_iteration(NULL, TRUE);