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.

1 files 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;
-
-		/* Call Disconnect to make sure IWD's autoconnect is disabled */
-		cleanup_association_attempt (self, TRUE);
+			/* 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;
+		}
 
-		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