From a2b9620b1b69bc2e3e4feed2337ad92867738d04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Correa=20G=C3=B3mez?= Date: Mon, 12 Dec 2022 17:44:51 +0100 Subject: [PATCH] network: keep track of radio buttons in connection editor with action Hooking to all the toggled signals from all the buttons for executing the same action is inneficient, and can potenticall end up in a segmentation fault due to some race in the signal emmission, where the active button gets deactivated before the clicked button is activated Looking at the GTK4 code, in a radio group: - The button which was previously active gets de-activated, emitting its corresponding toggled signal. - The active property for the clicked button gets set. - The clicked button emits its toggled signal. Therefore, if the first toggle signal gets processed before the active property is set, there can be a race condition. We are seeing this downstream at pmOS: https://gitlab.com/postmarketOS/pmaports/-/issues/1816 Instead of this racy behavior, follow upstream recommendation and keep track of the state through a stateful signal. --- .../network/connection-editor/ce-page-ip4.c | 102 ++++++++-------- .../network/connection-editor/ce-page-ip6.c | 113 ++++++++---------- panels/network/connection-editor/ip4-page.ui | 14 ++- panels/network/connection-editor/ip6-page.ui | 17 ++- 4 files changed, 120 insertions(+), 126 deletions(-) diff --git a/panels/network/connection-editor/ce-page-ip4.c b/panels/network/connection-editor/ce-page-ip4.c index 7025fe524..a58d6480b 100644 --- a/panels/network/connection-editor/ce-page-ip4.c +++ b/panels/network/connection-editor/ce-page-ip4.c @@ -47,13 +47,10 @@ struct _CEPageIP4 GtkSizeGroup *address_sizegroup; GtkSwitch *auto_dns_switch; GtkSwitch *auto_routes_switch; - GtkCheckButton *automatic_radio; GtkBox *content_box; GtkCheckButton *disabled_radio; GtkEntry *dns_entry; - GtkCheckButton *local_radio; GtkGrid *main_box; - GtkCheckButton *manual_radio; GtkCheckButton *never_default_check; GtkLabel *routes_address_label; GtkBox *routes_box; @@ -68,6 +65,8 @@ struct _CEPageIP4 GtkWidget *address_list; GtkWidget *routes_list; + + GActionGroup *method_group; }; static void ce_page_iface_init (CEPageInterface *); @@ -80,30 +79,27 @@ enum { METHOD_COL_METHOD }; -enum { - IP4_METHOD_AUTO, - IP4_METHOD_MANUAL, - IP4_METHOD_LINK_LOCAL, - IP4_METHOD_SHARED, - IP4_METHOD_DISABLED -}; - static void method_changed (CEPageIP4 *self) { gboolean addr_enabled; gboolean dns_enabled; gboolean routes_enabled; + g_autoptr(GVariant) method_variant = NULL; + const gchar *method; - if (gtk_check_button_get_active (GTK_CHECK_BUTTON (self->disabled_radio)) || - gtk_check_button_get_active (GTK_CHECK_BUTTON (self->shared_radio))) { + method_variant = g_action_group_get_action_state (self->method_group, "ip4method"); + method = g_variant_get_string (method_variant, NULL); + + if (g_str_equal (method, "disabled") || + g_str_equal (method, "shared")) { addr_enabled = FALSE; dns_enabled = FALSE; routes_enabled = FALSE; } else { - addr_enabled = gtk_check_button_get_active (GTK_CHECK_BUTTON (self->manual_radio)); - routes_enabled = !gtk_check_button_get_active (GTK_CHECK_BUTTON (self->local_radio)); - if (gtk_check_button_get_active (GTK_CHECK_BUTTON (self->local_radio))) + addr_enabled = g_str_equal (method, "manual"); + routes_enabled = !g_str_equal (method, "local"); + if (g_str_equal (method, "local")) dns_enabled = FALSE; else dns_enabled = !gtk_switch_get_active (self->auto_dns_switch); @@ -483,7 +479,7 @@ static void connect_ip4_page (CEPageIP4 *self) { const gchar *str_method; - guint method; + gchar *method; add_address_box (self); add_dns_section (self); @@ -500,45 +496,22 @@ connect_ip4_page (CEPageIP4 *self) self->content_box, "sensitive", G_BINDING_SYNC_CREATE | G_BINDING_INVERT_BOOLEAN); - method = IP4_METHOD_AUTO; + method = "automatic"; if (g_strcmp0 (str_method, NM_SETTING_IP4_CONFIG_METHOD_LINK_LOCAL) == 0) { - method = IP4_METHOD_LINK_LOCAL; + method = "local"; } else if (g_strcmp0 (str_method, NM_SETTING_IP4_CONFIG_METHOD_MANUAL) == 0) { - method = IP4_METHOD_MANUAL; + method = "manual"; } else if (g_strcmp0 (str_method, NM_SETTING_IP4_CONFIG_METHOD_SHARED) == 0) { - method = IP4_METHOD_SHARED; + method = "shared"; } else if (g_strcmp0 (str_method, NM_SETTING_IP4_CONFIG_METHOD_DISABLED) == 0) { - method = IP4_METHOD_DISABLED; + method = "disabled"; } gtk_check_button_set_active (GTK_CHECK_BUTTON (self->never_default_check), nm_setting_ip_config_get_never_default (self->setting)); g_signal_connect_object (self->never_default_check, "toggled", G_CALLBACK (ce_page_changed), self, G_CONNECT_SWAPPED); - g_signal_connect_object (self->automatic_radio, "toggled", G_CALLBACK (method_changed), self, G_CONNECT_SWAPPED); - g_signal_connect_object (self->local_radio, "toggled", G_CALLBACK (method_changed), self, G_CONNECT_SWAPPED); - g_signal_connect_object (self->manual_radio, "toggled", G_CALLBACK (method_changed), self, G_CONNECT_SWAPPED); - g_signal_connect_object (self->disabled_radio, "toggled", G_CALLBACK (method_changed), self, G_CONNECT_SWAPPED); - - switch (method) { - case IP4_METHOD_AUTO: - gtk_check_button_set_active (GTK_CHECK_BUTTON (self->automatic_radio), TRUE); - break; - case IP4_METHOD_LINK_LOCAL: - gtk_check_button_set_active (GTK_CHECK_BUTTON (self->local_radio), TRUE); - break; - case IP4_METHOD_MANUAL: - gtk_check_button_set_active (GTK_CHECK_BUTTON (self->manual_radio), TRUE); - break; - case IP4_METHOD_SHARED: - gtk_check_button_set_active (GTK_CHECK_BUTTON (self->shared_radio), TRUE); - break; - case IP4_METHOD_DISABLED: - gtk_check_button_set_active (GTK_CHECK_BUTTON (self->disabled_radio), TRUE); - break; - default: - break; - } + g_action_group_change_action_state (self->method_group, "ip4method", g_variant_new_string (method)); method_changed (self); } @@ -547,6 +520,7 @@ static gboolean ui_to_setting (CEPageIP4 *self) { const gchar *method; + g_autoptr(GVariant) method_variant = NULL; gboolean ignore_auto_dns; gboolean ignore_auto_routes; gboolean never_default; @@ -562,16 +536,20 @@ ui_to_setting (CEPageIP4 *self) gchar *dns_text = NULL; guint i; - if (gtk_check_button_get_active (GTK_CHECK_BUTTON (self->disabled_radio))) + method_variant = g_action_group_get_action_state (self->method_group, "ip4method"); + method = g_variant_get_string (method_variant, NULL); + if (g_str_equal (method, "disabled")) method = NM_SETTING_IP4_CONFIG_METHOD_DISABLED; - else if (gtk_check_button_get_active (GTK_CHECK_BUTTON (self->automatic_radio))) + else if (g_str_equal (method, "automatic")) method = NM_SETTING_IP4_CONFIG_METHOD_AUTO; - else if (gtk_check_button_get_active (GTK_CHECK_BUTTON (self->local_radio))) + else if (g_str_equal (method, "local")) method = NM_SETTING_IP4_CONFIG_METHOD_LINK_LOCAL; - else if (gtk_check_button_get_active (GTK_CHECK_BUTTON (self->manual_radio))) + else if (g_str_equal (method, "manual")) method = NM_SETTING_IP4_CONFIG_METHOD_MANUAL; - else if (gtk_check_button_get_active (GTK_CHECK_BUTTON (self->shared_radio))) + else if (g_str_equal (method, "shared")) method = NM_SETTING_IP4_CONFIG_METHOD_SHARED; + else + g_assert_not_reached (); addresses = g_ptr_array_new_with_free_func ((GDestroyNotify) nm_ip_address_unref); add_addresses = g_str_equal (method, NM_SETTING_IP4_CONFIG_METHOD_MANUAL); @@ -768,6 +746,17 @@ out: return ret; } +static void +on_ip4_method_activated_cb (GSimpleAction* action, + GVariant* parameter, + gpointer user_data) +{ + CEPageIP4 *self = CE_PAGE_IP4 (user_data); + g_simple_action_set_state (action, parameter); + + method_changed (self); +} + static const gchar * ce_page_ip4_get_title (CEPage *page) { @@ -788,6 +777,14 @@ ce_page_ip4_validate (CEPage *self, static void ce_page_ip4_init (CEPageIP4 *self) { + const GActionEntry ip4_entries[] = { + { "ip4method", on_ip4_method_activated_cb, "s", "'automatic'", NULL}, + }; + self->method_group = G_ACTION_GROUP (g_simple_action_group_new ()); + + g_action_map_add_action_entries (G_ACTION_MAP (self->method_group), ip4_entries, G_N_ELEMENTS (ip4_entries), self); + gtk_widget_insert_action_group (GTK_WIDGET (self), "ip4page", G_ACTION_GROUP (self->method_group)); + gtk_widget_init_template (GTK_WIDGET (self)); } @@ -802,13 +799,10 @@ ce_page_ip4_class_init (CEPageIP4Class *klass) gtk_widget_class_bind_template_child (widget_class, CEPageIP4, address_sizegroup); gtk_widget_class_bind_template_child (widget_class, CEPageIP4, auto_dns_switch); gtk_widget_class_bind_template_child (widget_class, CEPageIP4, auto_routes_switch); - gtk_widget_class_bind_template_child (widget_class, CEPageIP4, automatic_radio); gtk_widget_class_bind_template_child (widget_class, CEPageIP4, content_box); gtk_widget_class_bind_template_child (widget_class, CEPageIP4, disabled_radio); gtk_widget_class_bind_template_child (widget_class, CEPageIP4, dns_entry); - gtk_widget_class_bind_template_child (widget_class, CEPageIP4, local_radio); gtk_widget_class_bind_template_child (widget_class, CEPageIP4, main_box); - gtk_widget_class_bind_template_child (widget_class, CEPageIP4, manual_radio); gtk_widget_class_bind_template_child (widget_class, CEPageIP4, never_default_check); gtk_widget_class_bind_template_child (widget_class, CEPageIP4, address_address_label); gtk_widget_class_bind_template_child (widget_class, CEPageIP4, address_netmask_label); diff --git a/panels/network/connection-editor/ce-page-ip6.c b/panels/network/connection-editor/ce-page-ip6.c index ef6ce8b11..2711b4c3e 100644 --- a/panels/network/connection-editor/ce-page-ip6.c +++ b/panels/network/connection-editor/ce-page-ip6.c @@ -47,14 +47,10 @@ struct _CEPageIP6 GtkSizeGroup *address_sizegroup; GtkSwitch *auto_dns_switch; GtkSwitch *auto_routes_switch; - GtkCheckButton *automatic_radio; GtkBox *content_box; - GtkCheckButton *dhcp_radio; GtkCheckButton *disabled_radio; GtkEntry *dns_entry; - GtkCheckButton *local_radio; GtkGrid *main_box; - GtkCheckButton *manual_radio; GtkCheckButton *never_default_check; GtkBox *routes_box; GtkLabel *routes_address_label; @@ -69,6 +65,8 @@ struct _CEPageIP6 GtkWidget *address_list; GtkWidget *routes_list; + + GActionGroup *method_group; }; static void ce_page_iface_init (CEPageInterface *); @@ -81,31 +79,27 @@ enum { METHOD_COL_METHOD }; -enum { - IP6_METHOD_AUTO, - IP6_METHOD_DHCP, - IP6_METHOD_MANUAL, - IP6_METHOD_LINK_LOCAL, - IP6_METHOD_SHARED, - IP6_METHOD_DISABLED -}; - static void method_changed (CEPageIP6 *self) { gboolean addr_enabled; gboolean dns_enabled; gboolean routes_enabled; + g_autoptr(GVariant) method_variant = NULL; + const gchar *method; - if (gtk_check_button_get_active (self->disabled_radio) || - gtk_check_button_get_active (self->shared_radio)) { + method_variant = g_action_group_get_action_state (self->method_group, "ip6method"); + method = g_variant_get_string (method_variant, NULL); + + if (g_str_equal (method, "disabled") || + g_str_equal (method, "shared")) { addr_enabled = FALSE; dns_enabled = FALSE; routes_enabled = FALSE; } else { - addr_enabled = gtk_check_button_get_active (self->manual_radio); - routes_enabled = !gtk_check_button_get_active (self->local_radio); - if (gtk_check_button_get_active (GTK_CHECK_BUTTON (self->local_radio))) + addr_enabled = g_str_equal (method, "manual"); + routes_enabled = !g_str_equal (method, "local"); + if (g_str_equal (method, "local")) dns_enabled = FALSE; else dns_enabled = !gtk_switch_get_active (self->auto_dns_switch); @@ -453,7 +447,7 @@ static void connect_ip6_page (CEPageIP6 *self) { const gchar *str_method; - guint method; + gchar *method; add_address_box (self); add_dns_section (self); @@ -470,52 +464,25 @@ connect_ip6_page (CEPageIP6 *self) self->content_box, "sensitive", G_BINDING_SYNC_CREATE | G_BINDING_INVERT_BOOLEAN); - method = IP6_METHOD_AUTO; + method = "automatic"; if (g_strcmp0 (str_method, NM_SETTING_IP6_CONFIG_METHOD_DHCP) == 0) { - method = IP6_METHOD_DHCP; + method = "dhcp"; } else if (g_strcmp0 (str_method, NM_SETTING_IP6_CONFIG_METHOD_LINK_LOCAL) == 0) { - method = IP6_METHOD_LINK_LOCAL; + method = "local"; } else if (g_strcmp0 (str_method, NM_SETTING_IP6_CONFIG_METHOD_MANUAL) == 0) { - method = IP6_METHOD_MANUAL; + method = "manual"; } else if (g_strcmp0 (str_method, NM_SETTING_IP6_CONFIG_METHOD_SHARED) == 0) { - method = IP6_METHOD_SHARED; + method = "shared"; } else if (g_strcmp0 (str_method, NM_SETTING_IP6_CONFIG_METHOD_DISABLED) == 0 || g_strcmp0 (str_method, NM_SETTING_IP6_CONFIG_METHOD_IGNORE) == 0) { - method = IP6_METHOD_DISABLED; + method = "disabled"; } gtk_check_button_set_active (GTK_CHECK_BUTTON (self->never_default_check), nm_setting_ip_config_get_never_default (self->setting)); g_signal_connect_object (self->never_default_check, "toggled", G_CALLBACK (ce_page_changed), self, G_CONNECT_SWAPPED); - g_signal_connect_object (self->automatic_radio, "toggled", G_CALLBACK (method_changed), self, G_CONNECT_SWAPPED); - g_signal_connect_object (self->dhcp_radio, "toggled", G_CALLBACK (method_changed), self, G_CONNECT_SWAPPED); - g_signal_connect_object (self->local_radio, "toggled", G_CALLBACK (method_changed), self, G_CONNECT_SWAPPED); - g_signal_connect_object (self->manual_radio, "toggled", G_CALLBACK (method_changed), self, G_CONNECT_SWAPPED); - g_signal_connect_object (self->disabled_radio, "toggled", G_CALLBACK (method_changed), self, G_CONNECT_SWAPPED); - - switch (method) { - case IP6_METHOD_AUTO: - gtk_check_button_set_active (GTK_CHECK_BUTTON (self->automatic_radio), TRUE); - break; - case IP6_METHOD_DHCP: - gtk_check_button_set_active (GTK_CHECK_BUTTON (self->dhcp_radio), TRUE); - break; - case IP6_METHOD_LINK_LOCAL: - gtk_check_button_set_active (GTK_CHECK_BUTTON (self->local_radio), TRUE); - break; - case IP6_METHOD_MANUAL: - gtk_check_button_set_active (GTK_CHECK_BUTTON (self->manual_radio), TRUE); - break; - case IP6_METHOD_SHARED: - gtk_check_button_set_active (GTK_CHECK_BUTTON (self->shared_radio), TRUE); - break; - case IP6_METHOD_DISABLED: - gtk_check_button_set_active (GTK_CHECK_BUTTON (self->disabled_radio), TRUE); - break; - default: - break; - } + g_action_group_change_action_state (self->method_group, "ip6method", g_variant_new_string (method)); method_changed (self); } @@ -524,6 +491,7 @@ static gboolean ui_to_setting (CEPageIP6 *self) { GtkWidget *child; + g_autoptr(GVariant) method_variant = NULL; const gchar *method; gboolean ignore_auto_dns; gboolean ignore_auto_routes; @@ -535,18 +503,22 @@ ui_to_setting (CEPageIP6 *self) gchar *dns_text = NULL; guint i; - if (gtk_check_button_get_active (self->disabled_radio)) + method_variant = g_action_group_get_action_state (self->method_group, "ip6method"); + method = g_variant_get_string (method_variant, NULL); + if (g_str_equal (method, "disabled")) method = NM_SETTING_IP6_CONFIG_METHOD_DISABLED; - else if (gtk_check_button_get_active (self->manual_radio)) + else if (g_str_equal (method, "manual")) method = NM_SETTING_IP6_CONFIG_METHOD_MANUAL; - else if (gtk_check_button_get_active (self->local_radio)) + else if (g_str_equal (method, "local")) method = NM_SETTING_IP6_CONFIG_METHOD_LINK_LOCAL; - else if (gtk_check_button_get_active (self->dhcp_radio)) + else if (g_str_equal (method, "dhcp")) method = NM_SETTING_IP6_CONFIG_METHOD_DHCP; - else if (gtk_check_button_get_active (self->automatic_radio)) + else if (g_str_equal (method, "automatic")) method = NM_SETTING_IP6_CONFIG_METHOD_AUTO; - else if (gtk_check_button_get_active (self->shared_radio)) + else if (g_str_equal (method, "shared")) method = NM_SETTING_IP6_CONFIG_METHOD_SHARED; + else + g_assert_not_reached (); nm_setting_ip_config_clear_addresses (self->setting); if (g_str_equal (method, NM_SETTING_IP6_CONFIG_METHOD_MANUAL)) { @@ -738,6 +710,17 @@ out: return ret; } +static void +on_ip6_method_activated_cb (GSimpleAction* action, + GVariant* parameter, + gpointer user_data) +{ + CEPageIP6 *self = CE_PAGE_IP6 (user_data); + g_simple_action_set_state (action, parameter); + + method_changed (self); +} + static const gchar * ce_page_ip6_get_title (CEPage *page) { @@ -758,6 +741,14 @@ ce_page_ip6_validate (CEPage *self, static void ce_page_ip6_init (CEPageIP6 *self) { + const GActionEntry ip6_entries[] = { + { "ip6method", on_ip6_method_activated_cb, "s", "'automatic'", NULL}, + }; + self->method_group = G_ACTION_GROUP (g_simple_action_group_new ()); + + g_action_map_add_action_entries (G_ACTION_MAP (self->method_group), ip6_entries, G_N_ELEMENTS (ip6_entries), self); + gtk_widget_insert_action_group (GTK_WIDGET (self), "ip6page", G_ACTION_GROUP (self->method_group)); + gtk_widget_init_template (GTK_WIDGET (self)); } @@ -775,14 +766,10 @@ ce_page_ip6_class_init (CEPageIP6Class *klass) gtk_widget_class_bind_template_child (widget_class, CEPageIP6, address_sizegroup); gtk_widget_class_bind_template_child (widget_class, CEPageIP6, auto_dns_switch); gtk_widget_class_bind_template_child (widget_class, CEPageIP6, auto_routes_switch); - gtk_widget_class_bind_template_child (widget_class, CEPageIP6, automatic_radio); gtk_widget_class_bind_template_child (widget_class, CEPageIP6, content_box); - gtk_widget_class_bind_template_child (widget_class, CEPageIP6, dhcp_radio); gtk_widget_class_bind_template_child (widget_class, CEPageIP6, disabled_radio); gtk_widget_class_bind_template_child (widget_class, CEPageIP6, dns_entry); - gtk_widget_class_bind_template_child (widget_class, CEPageIP6, local_radio); gtk_widget_class_bind_template_child (widget_class, CEPageIP6, main_box); - gtk_widget_class_bind_template_child (widget_class, CEPageIP6, manual_radio); gtk_widget_class_bind_template_child (widget_class, CEPageIP6, never_default_check); gtk_widget_class_bind_template_child (widget_class, CEPageIP6, routes_box); gtk_widget_class_bind_template_child (widget_class, CEPageIP6, routes_address_label); diff --git a/panels/network/connection-editor/ip4-page.ui b/panels/network/connection-editor/ip4-page.ui index a2210ea8a..c03b644d7 100644 --- a/panels/network/connection-editor/ip4-page.ui +++ b/panels/network/connection-editor/ip4-page.ui @@ -34,6 +34,8 @@ Automatic (DHCP) + ip4page.ip4method + 'automatic' 0 1 @@ -43,7 +45,8 @@ Link-Local Only - automatic_radio + ip4page.ip4method + 'local' 0 2 @@ -53,7 +56,8 @@ Manual - automatic_radio + ip4page.ip4method + 'manual' 1 1 @@ -63,7 +67,8 @@ Disable - automatic_radio + ip4page.ip4method + 'disabled' 1 2 @@ -73,7 +78,8 @@ Shared to other computers - automatic_radio + ip4page.ip4method + 'shared' 2 1 diff --git a/panels/network/connection-editor/ip6-page.ui b/panels/network/connection-editor/ip6-page.ui index c22fb0f9e..c4817ceac 100644 --- a/panels/network/connection-editor/ip6-page.ui +++ b/panels/network/connection-editor/ip6-page.ui @@ -34,6 +34,8 @@ Automatic + ip6page.ip6method + 'automatic' 0 1 @@ -43,7 +45,8 @@ Automatic, DHCP only - automatic_radio + ip6page.ip6method + 'dhcp' 0 2 @@ -53,7 +56,8 @@ Link-Local Only - automatic_radio + ip6page.ip6method + 'local' 1 1 @@ -63,7 +67,8 @@ Manual - automatic_radio + ip6page.ip6method + 'manual' 1 2 @@ -73,7 +78,8 @@ Disable - automatic_radio + ip6page.ip6method + 'disabled' 2 1 @@ -83,7 +89,8 @@ Shared to other computers - automatic_radio + ip6page.ip6method + 'shared' 2 2