From 755d4e55c2253dc342a232aff725843fcfa727df Mon Sep 17 00:00:00 2001 From: Andrew Zaborowski Date: Mon, 22 Jan 2018 15:55:08 +0100 Subject: [PATCH 1/6] iwd: simple periodic scanning Add very simple periodic scanning because IWD itself only does periodic scanning when it is in charge of autoconnecting (by policy). Since we keep IWD out of the autoconnect state in order to use NM's autoconnect logic, we need to request the scanning. The policy in this patch is to use a simple 10s period between the end of one scan the requesting of another while not connected, and 20s when connected. This is so that users can expect similar results from both wifi backends but without duplicating the more elaborate code in the wpa_supplicant backend which can potentially be moved to a common superclass. --- src/devices/wifi/nm-device-iwd.c | 98 ++++++++++++++++++++++++++++++-- 1 file changed, 93 insertions(+), 5 deletions(-) diff --git a/src/devices/wifi/nm-device-iwd.c b/src/devices/wifi/nm-device-iwd.c index eaf504cc4..6c5412039 100644 --- a/src/devices/wifi/nm-device-iwd.c +++ b/src/devices/wifi/nm-device-iwd.c @@ -75,10 +75,12 @@ typedef struct { GCancellable * cancellable; NMDeviceWifiCapabilities capabilities; NMActRequestGetSecretsCallId *wifi_secrets_id; + guint periodic_scan_id; bool enabled:1; bool can_scan:1; bool can_connect:1; bool scanning:1; + bool scan_requested:1; } NMDeviceIwdPrivate; struct _NMDeviceIwd { @@ -101,6 +103,9 @@ G_DEFINE_TYPE (NMDeviceIwd, nm_device_iwd, NM_TYPE_DEVICE) /*****************************************************************************/ +static void schedule_periodic_scan (NMDeviceIwd *self, + NMDeviceState current_state); + static void _ap_dump (NMDeviceIwd *self, NMLogLevel log_level, @@ -863,6 +868,33 @@ check_scanning_prohibited (NMDeviceIwd *self, gboolean periodic) return prohibited; } +static void +scan_cb (GObject *source, GAsyncResult *res, gpointer user_data) +{ + NMDeviceIwd *self = user_data; + NMDeviceIwdPrivate *priv; + gs_free_error GError *error = NULL; + + if ( !_nm_dbus_proxy_call_finish (G_DBUS_PROXY (source), res, + G_VARIANT_TYPE ("()"), &error) + && g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + return; + + priv = NM_DEVICE_IWD_GET_PRIVATE (self); + priv->scan_requested = FALSE; + + /* On success, priv->scanning becomes true right before or right + * after this callback, so the next automatic scan will be + * scheduled when priv->scanning goes back to false. On error, + * schedule a retry now. + */ + if (error && !priv->scanning) { + NMDeviceState state = nm_device_get_state (NM_DEVICE (self)); + + schedule_periodic_scan (self, state); + } +} + static void dbus_request_scan_cb (NMDevice *device, GDBusMethodInvocation *context, @@ -912,8 +944,14 @@ dbus_request_scan_cb (NMDevice *device, } } - g_dbus_proxy_call (priv->dbus_proxy, "Scan", g_variant_new ("()"), - G_DBUS_CALL_FLAGS_NONE, -1, NULL, NULL, NULL); + if (!priv->scanning && !priv->scan_requested) { + g_dbus_proxy_call (priv->dbus_proxy, "Scan", + g_variant_new ("()"), + G_DBUS_CALL_FLAGS_NONE, -1, + priv->cancellable, scan_cb, self); + priv->scan_requested = TRUE; + } + g_dbus_method_invocation_return_value (context, NULL); } @@ -1461,6 +1499,45 @@ get_configured_mtu (NMDevice *device, gboolean *out_is_user_config) return mtu; } +static gboolean +periodic_scan_timeout_cb (gpointer user_data) +{ + NMDeviceIwd *self = user_data; + NMDeviceIwdPrivate *priv = NM_DEVICE_IWD_GET_PRIVATE (self); + + priv->periodic_scan_id = 0; + + if (priv->scanning || priv->scan_requested) + return FALSE; + + g_dbus_proxy_call (priv->dbus_proxy, "Scan", g_variant_new ("()"), + G_DBUS_CALL_FLAGS_NONE, -1, + priv->cancellable, scan_cb, self); + priv->scan_requested = TRUE; + + return FALSE; +} + +static void +schedule_periodic_scan (NMDeviceIwd *self, NMDeviceState current_state) +{ + NMDeviceIwdPrivate *priv = NM_DEVICE_IWD_GET_PRIVATE (self); + guint interval; + + if (current_state <= NM_DEVICE_STATE_UNAVAILABLE) + return; + + if (current_state == NM_DEVICE_STATE_DISCONNECTED) + interval = 10; + else + interval = 20; + + nm_clear_g_source (&priv->periodic_scan_id); + priv->periodic_scan_id = g_timeout_add_seconds (interval, + periodic_scan_timeout_cb, + self); +} + static void device_state_changed (NMDevice *device, NMDeviceState new_state, @@ -1470,10 +1547,13 @@ device_state_changed (NMDevice *device, NMDeviceIwd *self = NM_DEVICE_IWD (device); NMDeviceIwdPrivate *priv = NM_DEVICE_IWD_GET_PRIVATE (self); - if (new_state <= NM_DEVICE_STATE_UNAVAILABLE) + if (new_state <= NM_DEVICE_STATE_UNAVAILABLE) { remove_all_aps (self); - else if (old_state <= NM_DEVICE_STATE_UNAVAILABLE) + nm_clear_g_source (&priv->periodic_scan_id); + } else if (old_state <= NM_DEVICE_STATE_UNAVAILABLE) { update_aps (self); + schedule_periodic_scan (self, new_state); + } switch (new_state) { case NM_DEVICE_STATE_UNMANAGED: @@ -1710,6 +1790,7 @@ static void scanning_changed (NMDeviceIwd *self, gboolean new_scanning) { NMDeviceIwdPrivate *priv = NM_DEVICE_IWD_GET_PRIVATE (self); + NMDeviceState state = nm_device_get_state (NM_DEVICE (self)); if (new_scanning == priv->scanning) return; @@ -1718,8 +1799,12 @@ scanning_changed (NMDeviceIwd *self, gboolean new_scanning) _notify (self, PROP_SCANNING); - if (!priv->scanning) + if (!priv->scanning) { update_aps (self); + + if (!priv->scan_requested) + schedule_periodic_scan (self, state); + } } static void @@ -1779,6 +1864,7 @@ nm_device_iwd_set_dbus_object (NMDeviceIwd *self, GDBusObject *object) value = g_dbus_proxy_get_cached_property (priv->dbus_proxy, "Scanning"); priv->scanning = g_variant_get_boolean (value); + priv->scan_requested = FALSE; g_signal_connect (priv->dbus_proxy, "g-properties-changed", G_CALLBACK (properties_changed), self); @@ -1861,6 +1947,8 @@ dispose (GObject *object) nm_clear_g_cancellable (&priv->cancellable); + nm_clear_g_source (&priv->periodic_scan_id); + wifi_secrets_cancel (self); cleanup_association_attempt (self, TRUE); From eea06b8a8cf144fd6436879d78d6c50e0e4b8849 Mon Sep 17 00:00:00 2001 From: Andrew Zaborowski Date: Mon, 22 Jan 2018 19:19:32 +0100 Subject: [PATCH 2/6] iwd: initialize priv->can_connect when DBus interface appears Call state_changed with the initial Device.State property value to make sure can_connect and can_scan are up to date. --- src/devices/wifi/nm-device-iwd.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/devices/wifi/nm-device-iwd.c b/src/devices/wifi/nm-device-iwd.c index 6c5412039..022c37cbb 100644 --- a/src/devices/wifi/nm-device-iwd.c +++ b/src/devices/wifi/nm-device-iwd.c @@ -1835,7 +1835,7 @@ nm_device_iwd_set_dbus_object (NMDeviceIwd *self, GDBusObject *object) { NMDeviceIwdPrivate *priv = NM_DEVICE_IWD_GET_PRIVATE (self); GDBusInterface *interface; - gs_unref_variant GVariant *value = NULL; + GVariant *value; if (!nm_g_object_ref_set ((GObject **) &priv->dbus_obj, (GObject *) object)) return; @@ -1864,8 +1864,13 @@ nm_device_iwd_set_dbus_object (NMDeviceIwd *self, GDBusObject *object) value = g_dbus_proxy_get_cached_property (priv->dbus_proxy, "Scanning"); priv->scanning = g_variant_get_boolean (value); + g_variant_unref (value); priv->scan_requested = FALSE; + value = g_dbus_proxy_get_cached_property (priv->dbus_proxy, "State"); + state_changed (self, g_variant_get_string (value, NULL)); + g_variant_unref (value); + g_signal_connect (priv->dbus_proxy, "g-properties-changed", G_CALLBACK (properties_changed), self); From d32987fdd19b14b16409f6f8578e66e26bb3793f Mon Sep 17 00:00:00 2001 From: Andrew Zaborowski Date: Mon, 22 Jan 2018 19:38:29 +0100 Subject: [PATCH 3/6] iwd: keep reference to NMManager, disconnect signals Disconnect from NMManager signals in our cleanup, make sure the NMManager singleton is not destroyed before we are by keeping a reference until we've disconnected from its signals. --- src/devices/wifi/nm-iwd-manager.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/src/devices/wifi/nm-iwd-manager.c b/src/devices/wifi/nm-iwd-manager.c index 4eda5ebac..26553fd6b 100644 --- a/src/devices/wifi/nm-iwd-manager.c +++ b/src/devices/wifi/nm-iwd-manager.c @@ -39,6 +39,7 @@ typedef struct { } KnownNetworkData; typedef struct { + NMManager *nm_manager; GCancellable *cancellable; gboolean running; GDBusObjectManager *object_manager; @@ -97,7 +98,6 @@ psk_agent_dbus_method_cb (GDBusConnection *connection, gs_unref_object GDBusInterface *network = NULL, *device_obj = NULL; gs_unref_variant GVariant *value = NULL; gint ifindex; - NMManager *manager; NMDevice *device; const gchar *psk; @@ -139,9 +139,7 @@ psk_agent_dbus_method_cb (GDBusConnection *connection, goto return_error; } - manager = nm_manager_get (); - - device = nm_manager_get_device_by_ifindex (manager, ifindex); + device = nm_manager_get_device_by_ifindex (priv->nm_manager, ifindex); if (!NM_IS_DEVICE_IWD (device)) { _LOGE ("IWD device named %s is not a Wifi device in IWD Agent request", ifname); @@ -271,7 +269,6 @@ set_device_dbus_object (NMIwdManager *self, GDBusInterface *interface, const char *ifname; gint ifindex; NMDevice *device; - NMManager *manager; if (!priv->running) return; @@ -301,9 +298,7 @@ set_device_dbus_object (NMIwdManager *self, GDBusInterface *interface, return; } - manager = nm_manager_get (); - - device = nm_manager_get_device_by_ifindex (manager, ifindex); + device = nm_manager_get_device_by_ifindex (priv->nm_manager, ifindex); if (!NM_IS_DEVICE_IWD (device)) { _LOGE ("IWD device named %s is not a Wifi device", ifname); return; @@ -481,12 +476,11 @@ name_owner_changed (GObject *object, GParamSpec *pspec, gpointer user_data) update_known_networks (self); } else { - NMManager *manager = nm_manager_get (); const GSList *devices, *iter; priv->running = false; - devices = nm_manager_get_devices (manager); + devices = nm_manager_get_devices (priv->nm_manager); for (iter = devices; iter; iter = iter->next) { NMDevice *device = NM_DEVICE (iter->data); @@ -554,7 +548,6 @@ got_object_manager (GObject *object, GAsyncResult *result, gpointer user_data) GError *error = NULL; GDBusObjectManager *object_manager; GDBusConnection *connection; - NMManager *manager = nm_manager_get (); g_clear_object (&priv->cancellable); @@ -588,9 +581,6 @@ got_object_manager (GObject *object, GAsyncResult *result, gpointer user_data) } name_owner_changed (G_OBJECT (object_manager), NULL, self); - - g_signal_connect (manager, "device-added", - G_CALLBACK (device_added), self); } static void @@ -649,6 +639,10 @@ nm_iwd_manager_init (NMIwdManager *self) { NMIwdManagerPrivate *priv = NM_IWD_MANAGER_GET_PRIVATE (self); + priv->nm_manager = g_object_ref (nm_manager_get ()); + g_signal_connect (priv->nm_manager, "device-added", + G_CALLBACK (device_added), self); + priv->cancellable = g_cancellable_new (); prepare_object_manager (self); } @@ -684,6 +678,9 @@ dispose (GObject *object) g_slist_free_full (priv->known_networks, (GDestroyNotify) known_network_free); priv->known_networks = NULL; + g_signal_handlers_disconnect_by_data (priv->nm_manager, self); + g_clear_object (&priv->nm_manager); + G_OBJECT_CLASS (nm_iwd_manager_parent_class)->dispose (object); } From 86dd4000494de4d0050129ac7632a22840a103cb Mon Sep 17 00:00:00 2001 From: Andrew Zaborowski Date: Mon, 22 Jan 2018 19:22:55 +0100 Subject: [PATCH 4/6] iwd: recreate GDbusObjectManagerClient on reconnect Reuse the apparent workaround from libnm/nm-client.c in which the GDbusObjectManagerClient is recreated every time the name owner pops up, instead of creating it once and using that object forever. Resubscribe to all the signals on the new object. The initial GDbusObjectManager we create is only used to listed for the name-owner changes. There's nothing in gdbus docs that justifies doing that but there doesn't seem to be any way to reliably receive all the signals from the dbus service the normal way. The signals do appear on dbus-monitor and the gdbus apparently subscribes to those signals with AddMatch() correctly but they sometimes won't be received by the client code, unless this workaround is applied. While making changes to got_object_manager, don't destroy the cancellable there as it is supposed to be used throughout the NMIwdManager life. --- src/devices/wifi/nm-iwd-manager.c | 48 +++++++++++++++++-------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/src/devices/wifi/nm-iwd-manager.c b/src/devices/wifi/nm-iwd-manager.c index 26553fd6b..68d50a587 100644 --- a/src/devices/wifi/nm-iwd-manager.c +++ b/src/devices/wifi/nm-iwd-manager.c @@ -451,6 +451,8 @@ update_known_networks (NMIwdManager *self) g_object_unref (known_networks_if); } +static void prepare_object_manager (NMIwdManager *self); + static void name_owner_changed (GObject *object, GParamSpec *pspec, gpointer user_data) { @@ -461,20 +463,9 @@ name_owner_changed (GObject *object, GParamSpec *pspec, gpointer user_data) nm_assert (object_manager == priv->object_manager); if (_om_has_name_owner (object_manager)) { - GList *objects, *iter; - - priv->running = true; - - objects = g_dbus_object_manager_get_objects (object_manager); - for (iter = objects; iter; iter = iter->next) - object_added (self, G_DBUS_OBJECT (iter->data)); - - g_list_free_full (objects, g_object_unref); - - if (priv->agent_id) - register_agent (self); - - update_known_networks (self); + g_signal_handlers_disconnect_by_data (object_manager, self); + g_clear_object (&priv->object_manager); + prepare_object_manager (self); } else { const GSList *devices, *iter; @@ -549,8 +540,6 @@ got_object_manager (GObject *object, GAsyncResult *result, gpointer user_data) GDBusObjectManager *object_manager; GDBusConnection *connection; - g_clear_object (&priv->cancellable); - object_manager = g_dbus_object_manager_client_new_for_bus_finish (result, &error); if (object_manager == NULL) { _LOGE ("failed to acquire IWD Object Manager: Wi-Fi will not be available (%s)", @@ -561,10 +550,6 @@ got_object_manager (GObject *object, GAsyncResult *result, gpointer user_data) priv->object_manager = object_manager; - g_signal_connect (priv->object_manager, "interface-added", - G_CALLBACK (interface_added), self); - g_signal_connect (priv->object_manager, "interface-removed", - G_CALLBACK (interface_removed), self); g_signal_connect (priv->object_manager, "notify::name-owner", G_CALLBACK (name_owner_changed), self); @@ -580,7 +565,27 @@ got_object_manager (GObject *object, GAsyncResult *result, gpointer user_data) g_clear_error (&error); } - name_owner_changed (G_OBJECT (object_manager), NULL, self); + if (_om_has_name_owner (object_manager)) { + GList *objects, *iter; + + priv->running = true; + + g_signal_connect (priv->object_manager, "interface-added", + G_CALLBACK (interface_added), self); + g_signal_connect (priv->object_manager, "interface-removed", + G_CALLBACK (interface_removed), self); + + objects = g_dbus_object_manager_get_objects (object_manager); + for (iter = objects; iter; iter = iter->next) + object_added (self, G_DBUS_OBJECT (iter->data)); + + g_list_free_full (objects, g_object_unref); + + if (priv->agent_id) + register_agent (self); + + update_known_networks (self); + } } static void @@ -644,6 +649,7 @@ nm_iwd_manager_init (NMIwdManager *self) G_CALLBACK (device_added), self); priv->cancellable = g_cancellable_new (); + prepare_object_manager (self); } From 3a30ea9fc6363dc018c658456b6795284a63cad6 Mon Sep 17 00:00:00 2001 From: Andrew Zaborowski Date: Mon, 22 Jan 2018 19:34:18 +0100 Subject: [PATCH 5/6] iwd: avoid duplicate nm_device_iwd_set_dbus_object call Avoid calling nm_device_iwd_set_dbus_object (device, NULL) if the dbus_object was NULL already. Apparently gdbus guarantees that a name-owner notification either has a NULL old owner or a NULL new owner but can also have both old and new owner NULL. --- src/devices/wifi/nm-iwd-manager.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/devices/wifi/nm-iwd-manager.c b/src/devices/wifi/nm-iwd-manager.c index 68d50a587..2010021b1 100644 --- a/src/devices/wifi/nm-iwd-manager.c +++ b/src/devices/wifi/nm-iwd-manager.c @@ -469,6 +469,9 @@ name_owner_changed (GObject *object, GParamSpec *pspec, gpointer user_data) } else { const GSList *devices, *iter; + if (!priv->running) + return; + priv->running = false; devices = nm_manager_get_devices (priv->nm_manager); From 6473b0868cc4e880bdf81afca68457847168d9d1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 9 Feb 2018 21:34:20 +0100 Subject: [PATCH 6/6] wifi/iwd: make NMIwdManager:dispose() reentrant Theoretically, dispose() could be called multiple times. --- src/devices/wifi/nm-iwd-manager.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/devices/wifi/nm-iwd-manager.c b/src/devices/wifi/nm-iwd-manager.c index 2010021b1..21f0a7555 100644 --- a/src/devices/wifi/nm-iwd-manager.c +++ b/src/devices/wifi/nm-iwd-manager.c @@ -687,8 +687,10 @@ dispose (GObject *object) g_slist_free_full (priv->known_networks, (GDestroyNotify) known_network_free); priv->known_networks = NULL; - g_signal_handlers_disconnect_by_data (priv->nm_manager, self); - g_clear_object (&priv->nm_manager); + if (priv->nm_manager) { + g_signal_handlers_disconnect_by_data (priv->nm_manager, self); + g_clear_object (&priv->nm_manager); + } G_OBJECT_CLASS (nm_iwd_manager_parent_class)->dispose (object); }