From 81bb7c9138fb45661b49cd2d3d5f8d7bd2a308dc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 29 Sep 2017 15:11:33 +0200 Subject: [PATCH 1/6] manager: don't use platform singleton but keep a private pointer We should reduce uses of singletons in general. Instead, the platform instance should be passed around and kept for as long as it's needed. Especially, as we subscribe platform_link_cb() signal. Currently, we never unsubscribe it (wrongly). Subscribing signals is a strong indication that the target object should keep the source object alive until the signal is unsubscribed. --- src/nm-manager.c | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 2e47a97591..73dc307e52 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -110,6 +110,8 @@ typedef struct { } RadioState; typedef struct { + NMPlatform *platform; + GArray *capabilities; GSList *active_connections; @@ -1780,14 +1782,14 @@ get_existing_connection (NMManager *self, nm_device_capture_initial_config (device); if (ifindex) { - int master_ifindex = nm_platform_link_get_master (NM_PLATFORM_GET, ifindex); + int master_ifindex = nm_platform_link_get_master (priv->platform, ifindex); if (master_ifindex) { master = nm_manager_get_device_by_ifindex (self, master_ifindex); if (!master) { _LOG2D (LOGD_DEVICE, device, "assume: don't assume because " "cannot generate connection for slave before its master (%s/%d)", - nm_platform_link_get_name (NM_PLATFORM_GET, master_ifindex), master_ifindex); + nm_platform_link_get_name (priv->platform, master_ifindex), master_ifindex); return NULL; } if (!nm_device_get_act_request (master)) { @@ -2444,14 +2446,17 @@ static gboolean _platform_link_cb_idle (PlatformLinkCbData *data) { NMManager *self = data->self; + NMManagerPrivate *priv; const NMPlatformLink *l; if (!self) goto out; + priv = NM_MANAGER_GET_PRIVATE (self); + g_object_remove_weak_pointer (G_OBJECT (self), (gpointer *) &data->self); - l = nm_platform_link_get (NM_PLATFORM_GET, data->ifindex); + l = nm_platform_link_get (priv->platform, data->ifindex); if (l) { NMPlatformLink pllink; @@ -2511,6 +2516,7 @@ platform_link_cb (NMPlatform *platform, static void platform_query_devices (NMManager *self) { + NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); gs_unref_ptrarray GPtrArray *links = NULL; int i; gboolean guess_assume; @@ -2521,7 +2527,7 @@ platform_query_devices (NMManager *self) NM_CONFIG_KEYFILE_GROUP_MAIN, NM_CONFIG_KEYFILE_KEY_MAIN_SLAVES_ORDER, NM_CONFIG_GET_VALUE_STRIP); - links = nm_platform_link_get_all (NM_PLATFORM_GET, !nm_streq0 (order, "index")); + links = nm_platform_link_get_all (priv->platform, !nm_streq0 (order, "index")); if (!links) return; for (i = 0; i < links->len; i++) { @@ -4169,7 +4175,7 @@ impl_manager_add_and_activate_connection (NMManager *self, goto error; } - nm_utils_complete_generic (NM_PLATFORM_GET, + nm_utils_complete_generic (priv->platform, connection, NM_SETTING_VPN_SETTING_NAME, all_connections, @@ -4386,9 +4392,9 @@ done: } static gboolean -device_is_wake_on_lan (NMDevice *device) +device_is_wake_on_lan (NMPlatform *platform, NMDevice *device) { - return nm_platform_link_get_wake_on_lan (NM_PLATFORM_GET, nm_device_get_ip_ifindex (device)); + return nm_platform_link_get_wake_on_lan (platform, nm_device_get_ip_ifindex (device)); } static gboolean @@ -4506,7 +4512,8 @@ do_sleep_wake (NMManager *self, gboolean sleeping_changed) if (nm_device_is_software (device)) continue; /* Wake-on-LAN devices will be taken down post-suspend rather than pre- */ - if (suspending && device_is_wake_on_lan (device)) { + if ( suspending + && device_is_wake_on_lan (priv->platform, device)) { _LOGD (LOGD_SUSPEND, "sleep: device %s has wake-on-lan, skipping", nm_device_get_ip_iface (device)); continue; @@ -4537,7 +4544,7 @@ do_sleep_wake (NMManager *self, gboolean sleeping_changed) /* Belatedly take down Wake-on-LAN devices; ideally we wouldn't have to do this * but for now it's the only way to make sure we re-check their connectivity. */ - if (device_is_wake_on_lan (device)) + if (device_is_wake_on_lan (priv->platform, device)) nm_device_set_unmanaged_by_flags (device, NM_UNMANAGED_SLEEPING, TRUE, NM_DEVICE_STATE_REASON_SLEEPING); /* Check if the device is unmanaged but the state transition is still pending. @@ -5111,7 +5118,7 @@ nm_manager_write_device_state (NMManager *self) continue; } - if (!nm_platform_link_get (NM_PLATFORM_GET, ifindex)) + if (!nm_platform_link_get (priv->platform, ifindex)) continue; managed = nm_device_get_managed (device, FALSE); @@ -5196,9 +5203,9 @@ nm_manager_start (NMManager *self, GError **error) nm_device_factory_manager_load_factories (_register_device_factory, self); nm_device_factory_manager_for_each_factory (start_factory, NULL); - nm_platform_process_events (NM_PLATFORM_GET); + nm_platform_process_events (priv->platform); - g_signal_connect (NM_PLATFORM_GET, + g_signal_connect (priv->platform, NM_PLATFORM_SIGNAL_LINK_CHANGED, G_CALLBACK (platform_link_cb), self); @@ -6149,6 +6156,8 @@ nm_manager_init (NMManager *self) guint i; GFile *file; + priv->platform = g_object_ref (NM_PLATFORM_GET); + priv->capabilities = g_array_new (FALSE, FALSE, sizeof (guint32)); /* Initialize rfkill structures and states */ @@ -6480,6 +6489,8 @@ finalize (GObject *object) g_array_free (priv->capabilities, TRUE); G_OBJECT_CLASS (nm_manager_parent_class)->finalize (object); + + g_object_unref (priv->platform); } static void From cfe3d8bdd0039b312f03e66f16371fad474132ab Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 29 Sep 2017 15:18:48 +0200 Subject: [PATCH 2/6] manager/trivial: rename self variable in NMManager:dispose() --- src/nm-manager.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 73dc307e52..3377b604d1 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -6389,8 +6389,8 @@ _deinit_device_factory (NMDeviceFactory *factory, gpointer user_data) static void dispose (GObject *object) { - NMManager *manager = NM_MANAGER (object); - NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (manager); + NMManager *self = NM_MANAGER (object); + NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); g_slist_free_full (priv->auth_chains, (GDestroyNotify) nm_auth_chain_unref); priv->auth_chains = NULL; @@ -6405,7 +6405,7 @@ dispose (GObject *object) if (priv->auth_mgr) { g_signal_handlers_disconnect_by_func (priv->auth_mgr, G_CALLBACK (auth_mgr_changed), - manager); + self); g_clear_object (&priv->auth_mgr); } @@ -6414,32 +6414,32 @@ dispose (GObject *object) nm_clear_g_source (&priv->ac_cleanup_id); while (priv->active_connections) - active_connection_remove (manager, NM_ACTIVE_CONNECTION (priv->active_connections->data)); + active_connection_remove (self, NM_ACTIVE_CONNECTION (priv->active_connections->data)); g_clear_pointer (&priv->active_connections, g_slist_free); g_clear_object (&priv->primary_connection); g_clear_object (&priv->activating_connection); if (priv->config) { - g_signal_handlers_disconnect_by_func (priv->config, _config_changed_cb, manager); + g_signal_handlers_disconnect_by_func (priv->config, _config_changed_cb, self); g_clear_object (&priv->config); } if (priv->policy) { - g_signal_handlers_disconnect_by_func (priv->policy, policy_default_device_changed, manager); - g_signal_handlers_disconnect_by_func (priv->policy, policy_activating_device_changed, manager); + g_signal_handlers_disconnect_by_func (priv->policy, policy_default_device_changed, self); + g_signal_handlers_disconnect_by_func (priv->policy, policy_activating_device_changed, self); g_clear_object (&priv->policy); } if (priv->settings) { - g_signal_handlers_disconnect_by_func (priv->settings, settings_startup_complete_changed, manager); - g_signal_handlers_disconnect_by_func (priv->settings, system_unmanaged_devices_changed_cb, manager); - g_signal_handlers_disconnect_by_func (priv->settings, connection_added_cb, manager); - g_signal_handlers_disconnect_by_func (priv->settings, connection_updated_cb, manager); + g_signal_handlers_disconnect_by_func (priv->settings, settings_startup_complete_changed, self); + g_signal_handlers_disconnect_by_func (priv->settings, system_unmanaged_devices_changed_cb, self); + g_signal_handlers_disconnect_by_func (priv->settings, connection_added_cb, self); + g_signal_handlers_disconnect_by_func (priv->settings, connection_updated_cb, self); g_clear_object (&priv->settings); } if (priv->hostname_manager) { - g_signal_handlers_disconnect_by_func (priv->hostname_manager, hostname_changed_cb, manager); + g_signal_handlers_disconnect_by_func (priv->hostname_manager, hostname_changed_cb, self); g_clear_object (&priv->hostname_manager); } @@ -6447,21 +6447,21 @@ dispose (GObject *object) /* Unregister property filter */ if (priv->dbus_mgr) { - g_signal_handlers_disconnect_by_func (priv->dbus_mgr, dbus_connection_changed_cb, manager); + g_signal_handlers_disconnect_by_func (priv->dbus_mgr, dbus_connection_changed_cb, self); g_clear_object (&priv->dbus_mgr); } - _set_prop_filter (manager, NULL); + _set_prop_filter (self, NULL); - sleep_devices_clear (manager); + sleep_devices_clear (self); g_clear_pointer (&priv->sleep_devices, g_hash_table_unref); if (priv->sleep_monitor) { - g_signal_handlers_disconnect_by_func (priv->sleep_monitor, sleeping_cb, manager); + g_signal_handlers_disconnect_by_func (priv->sleep_monitor, sleeping_cb, self); g_clear_object (&priv->sleep_monitor); } if (priv->fw_monitor) { - g_signal_handlers_disconnect_by_func (priv->fw_monitor, firmware_dir_changed, manager); + g_signal_handlers_disconnect_by_func (priv->fw_monitor, firmware_dir_changed, self); nm_clear_g_source (&priv->fw_changed_id); @@ -6470,11 +6470,11 @@ dispose (GObject *object) } if (priv->rfkill_mgr) { - g_signal_handlers_disconnect_by_func (priv->rfkill_mgr, rfkill_manager_rfkill_changed_cb, manager); + g_signal_handlers_disconnect_by_func (priv->rfkill_mgr, rfkill_manager_rfkill_changed_cb, self); g_clear_object (&priv->rfkill_mgr); } - nm_device_factory_manager_for_each_factory (_deinit_device_factory, manager); + nm_device_factory_manager_for_each_factory (_deinit_device_factory, self); nm_clear_g_source (&priv->timestamp_update_id); From 3b3c5843cd8ff239c34a78ae4b699e93c2e0132c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 29 Sep 2017 15:19:19 +0200 Subject: [PATCH 3/6] manager: disconnect platform_link_cb() from NMManager in dispose() --- src/nm-manager.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/nm-manager.c b/src/nm-manager.c index 3377b604d1..5ef99586e7 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -6392,6 +6392,10 @@ dispose (GObject *object) NMManager *self = NM_MANAGER (object); NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); + g_signal_handlers_disconnect_by_func (priv->platform, + G_CALLBACK (platform_link_cb), + self); + g_slist_free_full (priv->auth_chains, (GDestroyNotify) nm_auth_chain_unref); priv->auth_chains = NULL; From 4db253b059f22c3e8d549dd85b112e60dc3e3bea Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 29 Sep 2017 15:04:53 +0200 Subject: [PATCH 4/6] manager: refactor lifetime handling for idle callback _platform_link_cb_idle() We call _platform_link_cb_idle() on idle, so we must take care of the lifetime of NMManager. We don't want to take a reference, so that the manager is not kept alive by platform events. Refactor the previous implementation with weak pointers to use a linked list instead. Let's not have any pending idle actions after the manager instance is destroyed. Instead, properly track and cancel the events. --- src/nm-manager.c | 44 +++++++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 5ef99586e7..86b68a0bc7 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -137,6 +137,8 @@ typedef struct { } prop_filter; NMRfkillManager *rfkill_mgr; + CList link_cb_lst; + NMCheckpointManager *checkpoint_mgr; NMSettings *settings; @@ -2438,35 +2440,34 @@ platform_link_added (NMManager *self, } typedef struct { + CList lst; NMManager *self; int ifindex; + guint idle_id; } PlatformLinkCbData; static gboolean _platform_link_cb_idle (PlatformLinkCbData *data) { + int ifindex = data->ifindex; NMManager *self = data->self; - NMManagerPrivate *priv; + NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); const NMPlatformLink *l; - if (!self) - goto out; + c_list_unlink (&data->lst); + g_slice_free (PlatformLinkCbData, data); - priv = NM_MANAGER_GET_PRIVATE (self); - - g_object_remove_weak_pointer (G_OBJECT (self), (gpointer *) &data->self); - - l = nm_platform_link_get (priv->platform, data->ifindex); + l = nm_platform_link_get (priv->platform, ifindex); if (l) { NMPlatformLink pllink; pllink = *l; /* make a copy of the link instance */ - platform_link_added (self, data->ifindex, &pllink, FALSE, NULL); + platform_link_added (self, ifindex, &pllink, FALSE, NULL); } else { NMDevice *device; GError *error = NULL; - device = nm_manager_get_device_by_ifindex (self, data->ifindex); + device = nm_manager_get_device_by_ifindex (self, ifindex); if (device) { if (nm_device_is_software (device)) { nm_device_sys_iface_state_set (device, NM_DEVICE_SYS_IFACE_STATE_REMOVED); @@ -2483,8 +2484,6 @@ _platform_link_cb_idle (PlatformLinkCbData *data) } } -out: - g_slice_free (PlatformLinkCbData, data); return G_SOURCE_REMOVE; } @@ -2496,17 +2495,22 @@ platform_link_cb (NMPlatform *platform, int change_type_i, gpointer user_data) { + NMManager *self; + NMManagerPrivate *priv; const NMPlatformSignalChangeType change_type = change_type_i; PlatformLinkCbData *data; switch (change_type) { case NM_PLATFORM_SIGNAL_ADDED: case NM_PLATFORM_SIGNAL_REMOVED: + self = NM_MANAGER (user_data); + priv = NM_MANAGER_GET_PRIVATE (self); + data = g_slice_new (PlatformLinkCbData); - data->self = NM_MANAGER (user_data); + data->self = self; data->ifindex = ifindex; - g_object_add_weak_pointer (G_OBJECT (data->self), (gpointer *) &data->self); - g_idle_add ((GSourceFunc) _platform_link_cb_idle, data); + c_list_link_tail (&priv->link_cb_lst, &data->lst); + data->idle_id = g_idle_add ((GSourceFunc) _platform_link_cb_idle, data); break; default: break; @@ -6156,6 +6160,8 @@ nm_manager_init (NMManager *self) guint i; GFile *file; + c_list_init (&priv->link_cb_lst); + priv->platform = g_object_ref (NM_PLATFORM_GET); priv->capabilities = g_array_new (FALSE, FALSE, sizeof (guint32)); @@ -6391,10 +6397,18 @@ dispose (GObject *object) { NMManager *self = NM_MANAGER (object); NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); + CList *iter, *iter_safe; g_signal_handlers_disconnect_by_func (priv->platform, G_CALLBACK (platform_link_cb), self); + c_list_for_each_safe (iter, iter_safe, &priv->link_cb_lst) { + PlatformLinkCbData *data = c_list_entry (iter, PlatformLinkCbData, lst); + + g_source_remove (data->idle_id); + c_list_unlink (iter); + g_slice_free (PlatformLinkCbData, data); + } g_slist_free_full (priv->auth_chains, (GDestroyNotify) nm_auth_chain_unref); priv->auth_chains = NULL; From ba8f81581e00f8809212c6be1243bd83ed907f3b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 29 Sep 2017 16:58:24 +0200 Subject: [PATCH 5/6] core: keep platform link object alive and don't copy it Sometimes, when we have a platform object, we need to keep it alive, because any subsequent platform operation might invalidate the object. Previously, we achieved that by copying the NMPlatformLink data. For a while now, all platform object are immuable and recounted. We should not copy the instance to a NMPlatformLink object, because then the instance is no longer a full NMPObject. Instead, just take an additional reference. Since the object must be immutable, that is just as safe. But now callees down the stack get a proper NMPObject instance, and might reference it too. --- src/devices/nm-device.c | 26 ++++++++++++++++---------- src/nm-manager.c | 11 +++++------ 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 5e0940c451..e05d9649d3 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2983,7 +2983,9 @@ nm_device_realize_start (NMDevice *self, gboolean *out_compatible, GError **error) { - NMPlatformLink plink_copy; + nm_auto_nmpobj const NMPObject *plink_keep_alive = NULL; + + nm_assert (!plink || NMP_OBJECT_GET_TYPE (NMP_OBJECT_UP_CAST (plink)) == NMP_OBJECT_TYPE_LINK); NM_SET_OUT (out_compatible, TRUE); @@ -2997,13 +2999,12 @@ nm_device_realize_start (NMDevice *self, if (!link_type_compatible (self, plink->type, out_compatible, error)) return FALSE; + + plink_keep_alive = nmp_object_ref (NMP_OBJECT_UP_CAST (plink)); } - if (plink) { - plink_copy = *plink; - plink = &plink_copy; - } - realize_start_setup (self, plink, + realize_start_setup (self, + plink, assume_state_guess_assume, assume_state_connection_uuid, set_nm_owned, @@ -3029,8 +3030,8 @@ nm_device_create_and_realize (NMDevice *self, NMDevice *parent, GError **error) { + nm_auto_nmpobj const NMPObject *plink_keep_alive = NULL; NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - NMPlatformLink plink_copy; const NMPlatformLink *plink = NULL; /* Must be set before device is realized */ @@ -3043,12 +3044,13 @@ nm_device_create_and_realize (NMDevice *self, if (!NM_DEVICE_GET_CLASS (self)->create_and_realize (self, connection, parent, &plink, error)) return FALSE; if (plink) { - plink_copy = *plink; - plink = &plink_copy; + nm_assert (NMP_OBJECT_GET_TYPE (NMP_OBJECT_UP_CAST (plink)) == NMP_OBJECT_TYPE_LINK); + plink_keep_alive = nmp_object_ref (NMP_OBJECT_UP_CAST (plink)); } } - realize_start_setup (self, plink, + realize_start_setup (self, + plink, FALSE, /* assume_state_guess_assume */ NULL, /* assume_state_connection_uuid */ FALSE, NM_UNMAN_FLAG_OP_FORGET); @@ -3175,6 +3177,10 @@ realize_start_setup (NMDevice *self, guint real_rate; guint32 mtu; + /* plink is a NMPlatformLink type, however, we require it to come from the platform + * cache (where else would it come from?). */ + nm_assert (!plink || NMP_OBJECT_GET_TYPE (NMP_OBJECT_UP_CAST (plink)) == NMP_OBJECT_TYPE_LINK); + g_return_if_fail (NM_IS_DEVICE (self)); priv = NM_DEVICE_GET_PRIVATE (self); diff --git a/src/nm-manager.c b/src/nm-manager.c index 86b68a0bc7..075623fbbd 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -2452,17 +2452,16 @@ _platform_link_cb_idle (PlatformLinkCbData *data) int ifindex = data->ifindex; NMManager *self = data->self; NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); - const NMPlatformLink *l; + const NMPlatformLink *plink; c_list_unlink (&data->lst); g_slice_free (PlatformLinkCbData, data); - l = nm_platform_link_get (priv->platform, ifindex); - if (l) { - NMPlatformLink pllink; + plink = nm_platform_link_get (priv->platform, ifindex); + if (plink) { + nm_auto_nmpobj const NMPObject *plink_keep_alive = nmp_object_ref (NMP_OBJECT_UP_CAST (plink)); - pllink = *l; /* make a copy of the link instance */ - platform_link_added (self, ifindex, &pllink, FALSE, NULL); + platform_link_added (self, ifindex, plink, FALSE, NULL); } else { NMDevice *device; GError *error = NULL; From 9ad8010fe0c42291580e4a801ed85947ae660d38 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 29 Sep 2017 17:08:55 +0200 Subject: [PATCH 6/6] device: fix delay startup complete for unrealized devices Since commit 6845b9b80a9fcec9d2c9e7b56a37329f38089f2e ("device: delay startup complete until device is initialized in platform", we also wait for devices that are still initializing platform/UDEV. Obviously, that only applies to realized devices. Otherwise, an unrealized device is going to block startup complete. Fixes: 6845b9b80a9fcec9d2c9e7b56a37329f38089f2e --- src/devices/nm-device.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index e05d9649d3..0ecd8a2b61 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -12213,7 +12213,8 @@ nm_device_has_pending_action (NMDevice *self) if (priv->pending_actions) return TRUE; - if (nm_device_get_unmanaged_flags (self, NM_UNMANAGED_PLATFORM_INIT)) { + if ( nm_device_is_real (self) + && nm_device_get_unmanaged_flags (self, NM_UNMANAGED_PLATFORM_INIT)) { /* as long as the platform link is not yet initialized, we have a pending * action. */ return TRUE;