From b31d2ea02a156b11d3530de57673f20a34a1173f Mon Sep 17 00:00:00 2001 From: Andrew Zaborowski Date: Sun, 16 Sep 2018 17:19:52 +0200 Subject: [PATCH] wifi/iwd: actually wait for Connect() call return state_changed (called when IWD signalled device state change) was supposed to not change NM device state on connect success or failure and instead wait for the DBus Connect() method callback but it would actually still call nm_device_emit_recheck_auto_activate on failure so refactor state_changed and network_connect_cb to make sure the state change and nm_device_emit_recheck_auto_activate are only called from network_connect_cb. This fixes a race where during a switch from one network to another NM would immediately start a new activation after state_changed and network_connect_cb would then handle the Connect failure and mark the new activation as failed. (cherry picked from commit 44af62eb1f868d8649fe438d476d17e5ea5bbf32) --- src/devices/wifi/nm-device-iwd.c | 88 +++++++++++++++++--------------- 1 file changed, 47 insertions(+), 41 deletions(-) diff --git a/src/devices/wifi/nm-device-iwd.c b/src/devices/wifi/nm-device-iwd.c index 31f8cbe0c7..ef5ad020b6 100644 --- a/src/devices/wifi/nm-device-iwd.c +++ b/src/devices/wifi/nm-device-iwd.c @@ -1285,11 +1285,14 @@ network_connect_cb (GObject *source, GAsyncResult *res, gpointer user_data) { NMDeviceIwd *self = user_data; NMDevice *device = NM_DEVICE (self); + NMDeviceIwdPrivate *priv = NM_DEVICE_IWD_GET_PRIVATE (self); gs_free_error GError *error = NULL; NMConnection *connection; NMSettingWireless *s_wifi; GBytes *ssid; gs_free char *ssid_utf8 = NULL; + NMDeviceStateReason reason = NM_DEVICE_STATE_REASON_SUPPLICANT_FAILED; + GVariant *value; if (!_nm_dbus_proxy_call_finish (G_DBUS_PROXY (source), res, G_VARIANT_TYPE ("()"), @@ -1304,6 +1307,12 @@ network_connect_cb (GObject *source, GAsyncResult *res, gpointer user_data) "Activation: (wifi) Network.Connect failed: %s", error->message); + if (nm_utils_error_is_cancelled (error, TRUE)) + return; + + if (!NM_IN_SET (nm_device_get_state (device), NM_DEVICE_STATE_CONFIG, NM_DEVICE_STATE_NEED_AUTH)) + return; + connection = nm_device_get_applied_connection (device); if (!connection) goto failed; @@ -1311,20 +1320,17 @@ network_connect_cb (GObject *source, GAsyncResult *res, gpointer user_data) if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_DBUS_ERROR)) dbus_error = g_dbus_error_get_remote_error (error); - /* If secrets were wrong, we'd be getting a net.connman.iwd.Failed */ if (nm_streq0 (dbus_error, "net.connman.iwd.Failed")) { nm_connection_clear_secrets (connection); - nm_device_state_changed (device, NM_DEVICE_STATE_FAILED, - NM_DEVICE_STATE_REASON_NO_SECRETS); - } else if ( !nm_utils_error_is_cancelled (error, TRUE) - && nm_device_is_activating (device)) - goto failed; + /* If secrets were wrong, we'd be getting a net.connman.iwd.Failed */ + reason = NM_DEVICE_STATE_REASON_NO_SECRETS; + } else if (nm_streq0 (dbus_error, "net.connman.iwd.Aborted")) { + /* If agent call was cancelled we'd be getting a net.connman.iwd.Aborted */ + reason = NM_DEVICE_STATE_REASON_NO_SECRETS; + } - /* Call Disconnect to make sure IWD's autoconnect is disabled */ - cleanup_association_attempt (self, TRUE); - - return; + goto failed; } nm_assert (nm_device_get_state (device) == NM_DEVICE_STATE_CONFIG); @@ -1351,9 +1357,17 @@ network_connect_cb (GObject *source, GAsyncResult *res, gpointer user_data) return; failed: - cleanup_association_attempt (self, FALSE); - nm_device_queue_state (device, NM_DEVICE_STATE_FAILED, - NM_DEVICE_STATE_REASON_SUPPLICANT_FAILED); + /* Call Disconnect to make sure IWD's autoconnect is disabled */ + cleanup_association_attempt (self, TRUE); + + nm_device_queue_state (device, NM_DEVICE_STATE_FAILED, reason); + + value = g_dbus_proxy_get_cached_property (priv->dbus_station_proxy, "State"); + if (!priv->can_connect && nm_streq0 (get_variant_state (value), "disconnected")) { + priv->can_connect = true; + nm_device_emit_recheck_auto_activate (device); + } + g_variant_unref (value); } static void @@ -1737,13 +1751,6 @@ state_changed (NMDeviceIwd *self, const char *new_state) /* Don't allow scanning while connecting, disconnecting or roaming */ priv->can_scan = NM_IN_STRSET (new_state, "connected", "disconnected"); - /* Don't allow new connection until iwd exits disconnecting */ - can_connect = NM_IN_STRSET (new_state, "disconnected"); - if (can_connect != priv->can_connect) { - priv->can_connect = can_connect; - nm_device_emit_recheck_auto_activate (device); - } - if (NM_IN_STRSET (new_state, "connecting", "connected", "roaming")) { /* If we were connecting, do nothing, the confirmation of * a connection success is handled in the Device.Connect @@ -1757,40 +1764,39 @@ state_changed (NMDeviceIwd *self, const char *new_state) _LOGW (LOGD_DEVICE | LOGD_WIFI, "Unsolicited connection success, asking IWD to disconnect"); send_disconnect (self); - - return; } else if (NM_IN_STRSET (new_state, "disconnecting", "disconnected")) { - if (!iwd_connection) - return; - /* Call Disconnect on the IWD device object to make sure it * disables its own autoconnect. - * - * Note we could instead call net.connman.iwd.KnownNetworks.ForgetNetwork - * and leave the device in autoconnect. This way if NetworkManager - * changes any settings for this connection, they'd be taken into - * account on the next connection attempt. But both methods are - * a hack, we'll perhaps need an IWD API to "connect once" without - * storing anything. */ send_disconnect (self); /* - * If IWD is still handling the Connect call, let our callback - * for the dbus method handle the failure. + * If IWD is still handling the Connect call, let our Connect + * callback for the dbus method handle the failure. The main + * reason we can't handle the failure here is because the method + * callback will have more information on the specific failure + * reason. */ if (dev_state == NM_DEVICE_STATE_CONFIG) return; - nm_device_state_changed (device, - NM_DEVICE_STATE_FAILED, - NM_DEVICE_STATE_REASON_SUPPLICANT_DISCONNECT); - - return; - } else if (nm_streq (new_state, "unknown")) + if (iwd_connection) + nm_device_state_changed (device, + NM_DEVICE_STATE_FAILED, + NM_DEVICE_STATE_REASON_SUPPLICANT_DISCONNECT); + } else if (!nm_streq (new_state, "unknown")) { + _LOGE (LOGD_WIFI, "State %s unknown", new_state); return; + } - _LOGE (LOGD_WIFI, "State %s unknown", new_state); + /* Don't allow new connection until iwd exits disconnecting and no + * Connect callback is pending. + */ + can_connect = NM_IN_STRSET (new_state, "disconnected"); + if (can_connect != priv->can_connect) { + priv->can_connect = can_connect; + nm_device_emit_recheck_auto_activate (device); + } } static void