[PATCH v2] iwd: Always disconnect connection completely

[PATCH v2] iwd: Always disconnect connection completely

[VAUTRIN Emmanuel (Canal Plus Prestataire)]

[-- Attachment #1: Type: text/plain, Size: 6852 bytes --]

Before being able to connect to a new network, finish disconnecting
the old connection. The network can change while the
cm_network_disconnect_cb is scheduled.

Commit 024309a9e04a ("wifi: Reset disconnecting status of any network")
Commit 795883e98eba ("wifi: Check valid network in disconnect callback")
Commit dd86f09107e8 ("wifi: Always disconnect connection completely")
---
 plugins/iwd.c | 115 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 103 insertions(+), 12 deletions(-)

diff --git a/plugins/iwd.c b/plugins/iwd.c
index ac3d1e1787ad..f65148b691e7 100644
--- a/plugins/iwd.c
+++ b/plugins/iwd.c
@@ -82,6 +82,10 @@ struct iwd_device {
         char *mode;
 
         struct connman_device *device;
+
+       char *network;
+       char *pending_network;
+       bool disconnecting;
 };
 
 struct iwd_network {
@@ -219,12 +223,6 @@ static void update_network_connected(struct iwd_network *iwdn)
         connman_network_set_connected(iwdn->network, true);
 }
 
-static void update_network_disconnected(struct iwd_network *iwdn)
-{
-       DBG("interface name %s", iwdn->name);
-       connman_network_set_connected(iwdn->network, false);
-}
-
 static void cm_network_connect_cb(DBusMessage *message, void *user_data)
 {
         const char *path = user_data;
@@ -253,21 +251,94 @@ static void cm_network_connect_cb(DBusMessage *message, void *user_data)
         update_network_connected(iwdn);
 }
 
+static void abort_pending_network(struct iwd_device *iwdd,
+               enum connman_network_error error)
+{
+       struct iwd_network *iwdn;
+
+       if (!iwdd->pending_network)
+               return;
+
+       iwdn = g_hash_table_lookup(networks, iwdd->pending_network);
+       if (iwdn)
+               connman_network_set_error(iwdn->network, error);
+
+       g_free(iwdd->pending_network);
+       iwdd->pending_network = NULL;
+}
+
 static int cm_network_connect(struct connman_network *network)
 {
         struct iwd_network *iwdn = connman_network_get_data(network);
+       struct iwd_device *iwdd;
+       int err;
 
-       if (!iwdn)
+       if (!iwdn || !iwdn->iwdd)
                 return -EINVAL;
 
+       iwdd = iwdn->iwdd;
+       if (iwdd->disconnecting) {
+               if (g_strcmp0(iwdn->path, iwdd->pending_network)) {
+                       abort_pending_network(iwdd,
+                                       CONNMAN_NETWORK_ERROR_CONNECT_FAIL);
+                       iwdd->pending_network = g_strdup(iwdn->path);
+               }
+               return -EINPROGRESS;
+       }
+
         if (!g_dbus_proxy_method_call(iwdn->proxy, "Connect",
                         NULL, cm_network_connect_cb,
-                       g_strdup(iwdn->path), g_free))
-               return -EIO;
+                       g_strdup(iwdn->path), g_free)) {
+               err = -EIO;
+               goto out;
+       }
 
         connman_network_set_associating(iwdn->network, true);
 
-       return -EINPROGRESS;
+       g_free(iwdd->network);
+       iwdd->network = g_strdup(iwdn->path);
+
+       err = -EINPROGRESS;
+
+out:
+       abort_pending_network(iwdd, CONNMAN_NETWORK_ERROR_UNKNOWN);
+       return err;
+}
+
+static void update_network_disconnected(struct iwd_network *iwdn)
+{
+       struct iwd_device *iwdd;
+
+       if (!iwdn || !iwdn->iwdd)
+               return;
+
+       iwdd = iwdn->iwdd;
+
+       DBG("interface name %s", iwdn->name);
+       connman_network_set_connected(iwdn->network, false);
+
+       iwdd->disconnecting = false;
+
+       if (g_strcmp0(iwdn->path, iwdd->network)) {
+               if (!g_strcmp0(iwdn->path, iwdd->pending_network)) {
+                       abort_pending_network(iwdd,
+                                       CONNMAN_NETWORK_ERROR_CONNECT_FAIL);
+               }
+               DBG("current wifi network has changed since disconnection");
+               return;
+       }
+
+       g_free(iwdd->network);
+       iwdd->network = NULL;
+
+       if (iwdd->pending_network) {
+               struct iwd_network *iwdn_pending =
+                       g_hash_table_lookup(networks, iwdd->pending_network);
+               if (!iwdn_pending)
+                       return;
+
+               cm_network_connect(iwdn_pending->network);
+       }
 }
 
 static void cm_network_disconnect_cb(DBusMessage *message, void *user_data)
@@ -279,6 +350,9 @@ static void cm_network_disconnect_cb(DBusMessage *message, void *user_data)
         if (!iwdn)
                 return;
 
+       if(iwdn->iwdd)
+               iwdn->iwdd->disconnecting = false;
+
         if (dbus_message_get_type(message) == DBUS_MESSAGE_TYPE_ERROR) {
                 const char *dbus_error = dbus_message_get_error_name(message);
 
@@ -303,11 +377,16 @@ static int cm_network_disconnect(struct connman_network *network)
 {
         struct iwd_network *iwdn = connman_network_get_data(network);
         struct iwd_station *iwds;
+       struct iwd_device *iwdd;
 
-       if (!iwdn && !iwdn->iwdd)
+       if (!iwdn || !iwdn->iwdd)
                 return -EINVAL;
 
-       iwds = g_hash_table_lookup(stations, iwdn->iwdd->path);
+       iwdd = iwdn->iwdd;
+       if (iwdd->disconnecting)
+               return -EALREADY;
+
+       iwds = g_hash_table_lookup(stations, iwdd->path);
         if (!iwds)
                 return -EIO;
 
@@ -317,6 +396,8 @@ static int cm_network_disconnect(struct connman_network *network)
                         NULL, cm_network_disconnect_cb, g_strdup(iwdn->path), g_free))
                 return -EIO;
 
+       iwdd->disconnecting = true;
+
         return 0;
 }
 
@@ -515,6 +596,12 @@ static void device_powered_cb(const DBusError *error, void *user_data)
         }
 
         connman_device_set_powered(iwdd->device, cbd->powered);
+
+       if(!cbd->powered) {
+               abort_pending_network(iwdd,
+                                       CONNMAN_NETWORK_ERROR_CONNECT_FAIL);
+               iwdd->disconnecting = false;
+       }
 out:
         g_free(cbd->path);
         g_free(cbd);
@@ -1309,6 +1396,10 @@ static void device_free(gpointer data)
         g_free(iwdd->adapter);
         g_free(iwdd->name);
         g_free(iwdd->address);
+
+       g_free(iwdd->network);
+       g_free(iwdd->pending_network);
+
         g_free(iwdd);
 }
 
-- 
2.25.1
Please find the well formatted path as attachment.

> I don't have any fallouts in there. This could be just because I use a
> different compiler with different settings. Please report those
> failures. It's not good to paper over them in the long run.
Probably, by my side, I am building via Yocto.
I will send you the related patch.

Emmanuel

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v2-0001-iwd-Always-disconnect-connection-completely.patch --]
[-- Type: text/x-patch; name="v2-0001-iwd-Always-disconnect-connection-completely.patch", Size: 5435 bytes --]

From 2526604133f8bf47da3234650f06ff27fa01ecb1 Mon Sep 17 00:00:00 2001
From: Emmanuel VAUTRIN <Emmanuel.VAUTRIN@cpexterne.org>
Date: Wed, 19 Jan 2022 10:38:07 +0100
Subject: [PATCH v2] iwd: Always disconnect connection completely

Before being able to connect to a new network, finish disconnecting
the old connection. The network can change while the
cm_network_disconnect_cb is scheduled.

Commit 024309a9e04a ("wifi: Reset disconnecting status of any network")
Commit 795883e98eba ("wifi: Check valid network in disconnect callback")
Commit dd86f09107e8 ("wifi: Always disconnect connection completely")
---
 plugins/iwd.c | 115 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 103 insertions(+), 12 deletions(-)

diff --git a/plugins/iwd.c b/plugins/iwd.c
index ac3d1e1787ad..f65148b691e7 100644
--- a/plugins/iwd.c
+++ b/plugins/iwd.c
@@ -82,6 +82,10 @@ struct iwd_device {
 	char *mode;
 
 	struct connman_device *device;
+
+	char *network;
+	char *pending_network;
+	bool disconnecting;
 };
 
 struct iwd_network {
@@ -219,12 +223,6 @@ static void update_network_connected(struct iwd_network *iwdn)
 	connman_network_set_connected(iwdn->network, true);
 }
 
-static void update_network_disconnected(struct iwd_network *iwdn)
-{
-	DBG("interface name %s", iwdn->name);
-	connman_network_set_connected(iwdn->network, false);
-}
-
 static void cm_network_connect_cb(DBusMessage *message, void *user_data)
 {
 	const char *path = user_data;
@@ -253,21 +251,94 @@ static void cm_network_connect_cb(DBusMessage *message, void *user_data)
 	update_network_connected(iwdn);
 }
 
+static void abort_pending_network(struct iwd_device *iwdd,
+		enum connman_network_error error)
+{
+	struct iwd_network *iwdn;
+
+	if (!iwdd->pending_network)
+		return;
+
+	iwdn = g_hash_table_lookup(networks, iwdd->pending_network);
+	if (iwdn)
+		connman_network_set_error(iwdn->network, error);
+
+	g_free(iwdd->pending_network);
+	iwdd->pending_network = NULL;
+}
+
 static int cm_network_connect(struct connman_network *network)
 {
 	struct iwd_network *iwdn = connman_network_get_data(network);
+	struct iwd_device *iwdd;
+	int err;
 
-	if (!iwdn)
+	if (!iwdn || !iwdn->iwdd)
 		return -EINVAL;
 
+	iwdd = iwdn->iwdd;
+	if (iwdd->disconnecting) {
+		if (g_strcmp0(iwdn->path, iwdd->pending_network)) {
+			abort_pending_network(iwdd,
+					CONNMAN_NETWORK_ERROR_CONNECT_FAIL);
+			iwdd->pending_network = g_strdup(iwdn->path);
+		}
+		return -EINPROGRESS;
+	}
+
 	if (!g_dbus_proxy_method_call(iwdn->proxy, "Connect",
 			NULL, cm_network_connect_cb,
-			g_strdup(iwdn->path), g_free))
-		return -EIO;
+			g_strdup(iwdn->path), g_free)) {
+		err = -EIO;
+		goto out;
+	}
 
 	connman_network_set_associating(iwdn->network, true);
 
-	return -EINPROGRESS;
+	g_free(iwdd->network);
+	iwdd->network = g_strdup(iwdn->path);
+
+	err = -EINPROGRESS;
+
+out:
+	abort_pending_network(iwdd, CONNMAN_NETWORK_ERROR_UNKNOWN);
+	return err;
+}
+
+static void update_network_disconnected(struct iwd_network *iwdn)
+{
+	struct iwd_device *iwdd;
+
+	if (!iwdn || !iwdn->iwdd)
+		return;
+
+	iwdd = iwdn->iwdd;
+
+	DBG("interface name %s", iwdn->name);
+	connman_network_set_connected(iwdn->network, false);
+
+	iwdd->disconnecting = false;
+
+	if (g_strcmp0(iwdn->path, iwdd->network)) {
+		if (!g_strcmp0(iwdn->path, iwdd->pending_network)) {
+			abort_pending_network(iwdd,
+					CONNMAN_NETWORK_ERROR_CONNECT_FAIL);
+		}
+		DBG("current wifi network has changed since disconnection");
+		return;
+	}
+
+	g_free(iwdd->network);
+	iwdd->network = NULL;
+
+	if (iwdd->pending_network) {
+		struct iwd_network *iwdn_pending =
+			g_hash_table_lookup(networks, iwdd->pending_network);
+		if (!iwdn_pending)
+			return;
+
+		cm_network_connect(iwdn_pending->network);
+	}
 }
 
 static void cm_network_disconnect_cb(DBusMessage *message, void *user_data)
@@ -279,6 +350,9 @@ static void cm_network_disconnect_cb(DBusMessage *message, void *user_data)
 	if (!iwdn)
 		return;
 
+	if(iwdn->iwdd)
+		iwdn->iwdd->disconnecting = false;
+
 	if (dbus_message_get_type(message) == DBUS_MESSAGE_TYPE_ERROR) {
 		const char *dbus_error = dbus_message_get_error_name(message);
 
@@ -303,11 +377,16 @@ static int cm_network_disconnect(struct connman_network *network)
 {
 	struct iwd_network *iwdn = connman_network_get_data(network);
 	struct iwd_station *iwds;
+	struct iwd_device *iwdd;
 
-	if (!iwdn && !iwdn->iwdd)
+	if (!iwdn || !iwdn->iwdd)
 		return -EINVAL;
 
-	iwds = g_hash_table_lookup(stations, iwdn->iwdd->path);
+	iwdd = iwdn->iwdd;
+	if (iwdd->disconnecting)
+		return -EALREADY;
+
+	iwds = g_hash_table_lookup(stations, iwdd->path);
 	if (!iwds)
 		return -EIO;
 
@@ -317,6 +396,8 @@ static int cm_network_disconnect(struct connman_network *network)
 			NULL, cm_network_disconnect_cb, g_strdup(iwdn->path), g_free))
 		return -EIO;
 
+	iwdd->disconnecting = true;
+
 	return 0;
 }
 
@@ -515,6 +596,12 @@ static void device_powered_cb(const DBusError *error, void *user_data)
 	}
 
 	connman_device_set_powered(iwdd->device, cbd->powered);
+
+	if(!cbd->powered) {
+		abort_pending_network(iwdd,
+					CONNMAN_NETWORK_ERROR_CONNECT_FAIL);
+		iwdd->disconnecting = false;
+	}
 out:
 	g_free(cbd->path);
 	g_free(cbd);
@@ -1309,6 +1396,10 @@ static void device_free(gpointer data)
 	g_free(iwdd->adapter);
 	g_free(iwdd->name);
 	g_free(iwdd->address);
+
+	g_free(iwdd->network);
+	g_free(iwdd->pending_network);
+
 	g_free(iwdd);
 }
 
-- 
2.25.1

Re: [PATCH v2] iwd: Always disconnect connection completely

[Daniel Wagner]

Hi Emmanuel,

On Mon, Jan 24, 2022 at 09:06:27AM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> Before being able to connect to a new network, finish disconnecting
> the old connection. The network can change while the
> cm_network_disconnect_cb is scheduled.
> 
> Commit 024309a9e04a ("wifi: Reset disconnecting status of any network")
> Commit 795883e98eba ("wifi: Check valid network in disconnect callback")
> Commit dd86f09107e8 ("wifi: Always disconnect connection completely")

I haven't made my mind up yet on this one. The problem I see with this
is that we start add glue logic which is hard to follow and I am not
sure if this is the right place for it. If you have some more info
what's happening I would love to hear.

Daniel

RE: [PATCH v2] iwd: Always disconnect connection completely

[VAUTRIN Emmanuel (Canal Plus Prestataire)]

> I haven't made my mind up yet on this one. The problem I see with this
> is that we start add glue logic which is hard to follow and I am not
> sure if this is the right place for it. If you have some more info
> what's happening I would love to hear.
Indeed, Daniel it is a tricky subject.

The similar patch has already been applied for wpa_supplicant
(cf commits referenced in the commit message), after a long discussion
if I remember well.
It is still necessary to manage the transitions between 3 known Wifi networks:
 - formerly connected,
 - currently connected,
 - to be connected.
In this case, they are 2 disconnections to take into account:
 - of the currently connected
 - of the formerly connected,  auto-connecting at the disconnection of
the currently connected.

In fact, I was waiting for the integration of this patch to provide another
major one, managing the networks removal on iwd.
Do you prefer I provide this one first?

Best Regards

Emmanuel

Re: [PATCH v2] iwd: Always disconnect connection completely

[Daniel Wagner]

On Tue, Jan 25, 2022 at 10:43:22AM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> > I haven't made my mind up yet on this one. The problem I see with this
> > is that we start add glue logic which is hard to follow and I am not
> > sure if this is the right place for it. If you have some more info
> > what's happening I would love to hear.
> Indeed, Daniel it is a tricky subject.
> 
> The similar patch has already been applied for wpa_supplicant
> (cf commits referenced in the commit message), after a long discussion
> if I remember well.
> It is still necessary to manage the transitions between 3 known Wifi networks:
>  - formerly connected,
>  - currently connected,
>  - to be connected.
> In this case, they are 2 disconnections to take into account:
>  - of the currently connected
>  - of the formerly connected,  auto-connecting at the disconnection of
> the currently connected.

Let me add the iwd mailing list here. I'd like to know what we are
supposed to handle on the plugin layer.

For the iwd developers: Currently ConnMan's iwd plugin doesn't track any
networks state. It just forwards the connect/disconnect requests from
the core. Do we need to filter/serialize those?

Daniel

Re: [PATCH v2] iwd: Always disconnect connection completely

[Denis Kenzior]

Hi Daniel,

> For the iwd developers: Currently ConnMan's iwd plugin doesn't track any
> networks state. It just forwards the connect/disconnect requests from
> the core. Do we need to filter/serialize those?

So in theory iwd should gracefully handle such scenarios.  For example:
- Connect comes while already connected (network switch)
- Disconnect comes when connecting (aborts the connect attempt)
- Connect while connecting (aborts original attempt, starts new one)

There may be situations where the timing between requests is so small that we 
simply have to return an -EBUSY (net.connman.iwd.Busy).

Is there a particular scenario which isn't working?

Regards,
-Denis

RE: [PATCH v2] iwd: Always disconnect connection completely

[VAUTRIN Emmanuel (Canal Plus Prestataire)]

[-- Attachment #1: Type: text/plain, Size: 1526 bytes --]

Thank you Denis for your answer.

> So in theory iwd should gracefully handle such scenarios.  For example:
> - Connect comes while already connected (network switch)
> - Disconnect comes when connecting (aborts the connect attempt)
> - Connect while connecting (aborts original attempt, starts new one)

> There may be situations where the timing between requests is so small that we 
> simply have to return an -EBUSY (net.connman.iwd.Busy).

> Is there a particular scenario which isn't working?
In my opinion, the fix shall be done at ConnMan level and will always be
necessary, as the disconnection is asynchronous, contrary to the connection.

One year ago, a similar patch was integrated for wpa_supplicant.
(cf https://lists.01.org/hyperkitty/list/connman@lists.01.org/thread/NSWBVTHYPKUDON6K66YPL2OWFY6UYOV2/)

It is easy to reproduce the issue, by switching quickly between two known
Wifi networks (W1 & W2), via the Service api. By quickly, I mean: one call shall
immediately follow the reply of the previous call.
In this case, the following steps happen:
1. Disconnection of current network (W1).
2. Connection to the new one (W2) failing with "net.connman.iwd.Failed"
dbus error in cm_network_connect_cb, probably due to the fact that W1
disconnection was not already completed, aborting connection to W2.


Daniel, I have attached the v3 of the patch to be more compliant with the coding rules.
Please tell me both if you need additional information.


Best Regards,

Emmanuel

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v3-0001-iwd-Always-disconnect-connection-completely.patch --]
[-- Type: text/x-patch; name="v3-0001-iwd-Always-disconnect-connection-completely.patch", Size: 5439 bytes --]

From 7b7da16f4e495c269e1ca02177b8030a2189cefe Mon Sep 17 00:00:00 2001
From: Emmanuel VAUTRIN <Emmanuel.VAUTRIN@cpexterne.org>
Date: Mon, 7 Feb 2022 15:47:10 +0100
Subject: [PATCH v3] iwd: Always disconnect connection completely

Before being able to connect to a new network, finish disconnecting
the old connection. The network can change while the
cm_network_disconnect_cb is scheduled.

Commit 024309a9e04a ("wifi: Reset disconnecting status of any network")
Commit 795883e98eba ("wifi: Check valid network in disconnect callback")
Commit dd86f09107e8 ("wifi: Always disconnect connection completely")
---
 plugins/iwd.c | 115 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 103 insertions(+), 12 deletions(-)

diff --git a/plugins/iwd.c b/plugins/iwd.c
index ac3d1e1787ad..d76e0818bff2 100644
--- a/plugins/iwd.c
+++ b/plugins/iwd.c
@@ -82,6 +82,10 @@ struct iwd_device {
 	char *mode;
 
 	struct connman_device *device;
+
+	char *network;
+	char *pending_network;
+	bool disconnecting;
 };
 
 struct iwd_network {
@@ -219,12 +223,6 @@ static void update_network_connected(struct iwd_network *iwdn)
 	connman_network_set_connected(iwdn->network, true);
 }
 
-static void update_network_disconnected(struct iwd_network *iwdn)
-{
-	DBG("interface name %s", iwdn->name);
-	connman_network_set_connected(iwdn->network, false);
-}
-
 static void cm_network_connect_cb(DBusMessage *message, void *user_data)
 {
 	const char *path = user_data;
@@ -253,21 +251,94 @@ static void cm_network_connect_cb(DBusMessage *message, void *user_data)
 	update_network_connected(iwdn);
 }
 
+static void abort_pending_network(struct iwd_device *iwdd,
+					enum connman_network_error error)
+{
+	struct iwd_network *iwdn;
+
+	if (!iwdd->pending_network)
+		return;
+
+	iwdn = g_hash_table_lookup(networks, iwdd->pending_network);
+	if (iwdn)
+		connman_network_set_error(iwdn->network, error);
+
+	g_free(iwdd->pending_network);
+	iwdd->pending_network = NULL;
+}
+
 static int cm_network_connect(struct connman_network *network)
 {
 	struct iwd_network *iwdn = connman_network_get_data(network);
+	struct iwd_device *iwdd;
+	int err;
 
-	if (!iwdn)
+	if (!iwdn || !iwdn->iwdd)
 		return -EINVAL;
 
+	iwdd = iwdn->iwdd;
+	if (iwdd->disconnecting) {
+		if (g_strcmp0(iwdn->path, iwdd->pending_network)) {
+			abort_pending_network(iwdd,
+					CONNMAN_NETWORK_ERROR_CONNECT_FAIL);
+			iwdd->pending_network = g_strdup(iwdn->path);
+		}
+		return -EINPROGRESS;
+	}
+
 	if (!g_dbus_proxy_method_call(iwdn->proxy, "Connect",
 			NULL, cm_network_connect_cb,
-			g_strdup(iwdn->path), g_free))
-		return -EIO;
+			g_strdup(iwdn->path), g_free)) {
+		err = -EIO;
+		goto out;
+	}
 
 	connman_network_set_associating(iwdn->network, true);
 
-	return -EINPROGRESS;
+	g_free(iwdd->network);
+	iwdd->network = g_strdup(iwdn->path);
+
+	err = -EINPROGRESS;
+
+out:
+	abort_pending_network(iwdd, CONNMAN_NETWORK_ERROR_UNKNOWN);
+	return err;
+}
+
+static void update_network_disconnected(struct iwd_network *iwdn)
+{
+	struct iwd_device *iwdd;
+
+	if (!iwdn || !iwdn->iwdd)
+		return;
+
+	iwdd = iwdn->iwdd;
+
+	DBG("interface name %s", iwdn->name);
+	connman_network_set_connected(iwdn->network, false);
+
+	iwdd->disconnecting = false;
+
+	if (g_strcmp0(iwdn->path, iwdd->network)) {
+		if (!g_strcmp0(iwdn->path, iwdd->pending_network)) {
+			abort_pending_network(iwdd,
+					CONNMAN_NETWORK_ERROR_CONNECT_FAIL);
+		}
+		DBG("current wifi network has changed since disconnection");
+		return;
+	}
+
+	g_free(iwdd->network);
+	iwdd->network = NULL;
+
+	if (iwdd->pending_network) {
+		struct iwd_network *iwdn_pending =
+			g_hash_table_lookup(networks, iwdd->pending_network);
+		if (!iwdn_pending)
+			return;
+
+		cm_network_connect(iwdn_pending->network);
+	}
 }
 
 static void cm_network_disconnect_cb(DBusMessage *message, void *user_data)
@@ -279,6 +350,9 @@ static void cm_network_disconnect_cb(DBusMessage *message, void *user_data)
 	if (!iwdn)
 		return;
 
+	if (iwdn->iwdd)
+		iwdn->iwdd->disconnecting = false;
+
 	if (dbus_message_get_type(message) == DBUS_MESSAGE_TYPE_ERROR) {
 		const char *dbus_error = dbus_message_get_error_name(message);
 
@@ -303,11 +377,16 @@ static int cm_network_disconnect(struct connman_network *network)
 {
 	struct iwd_network *iwdn = connman_network_get_data(network);
 	struct iwd_station *iwds;
+	struct iwd_device *iwdd;
 
-	if (!iwdn && !iwdn->iwdd)
+	if (!iwdn || !iwdn->iwdd)
 		return -EINVAL;
 
-	iwds = g_hash_table_lookup(stations, iwdn->iwdd->path);
+	iwdd = iwdn->iwdd;
+	if (iwdd->disconnecting)
+		return -EALREADY;
+
+	iwds = g_hash_table_lookup(stations, iwdd->path);
 	if (!iwds)
 		return -EIO;
 
@@ -317,6 +396,8 @@ static int cm_network_disconnect(struct connman_network *network)
 			NULL, cm_network_disconnect_cb, g_strdup(iwdn->path), g_free))
 		return -EIO;
 
+	iwdd->disconnecting = true;
+
 	return 0;
 }
 
@@ -515,6 +596,12 @@ static void device_powered_cb(const DBusError *error, void *user_data)
 	}
 
 	connman_device_set_powered(iwdd->device, cbd->powered);
+
+	if (!cbd->powered) {
+		abort_pending_network(iwdd,
+					CONNMAN_NETWORK_ERROR_CONNECT_FAIL);
+		iwdd->disconnecting = false;
+	}
 out:
 	g_free(cbd->path);
 	g_free(cbd);
@@ -1309,6 +1396,10 @@ static void device_free(gpointer data)
 	g_free(iwdd->adapter);
 	g_free(iwdd->name);
 	g_free(iwdd->address);
+
+	g_free(iwdd->network);
+	g_free(iwdd->pending_network);
+
 	g_free(iwdd);
 }
 
-- 
2.25.1

RE: [PATCH v2] iwd: Always disconnect connection completely

[VAUTRIN Emmanuel (Canal Plus Prestataire)]

Hi Daniel,

Do you need more information about this subject?
I have an important patch on iwd networks removal,
based on this patch. Do you prefer I propose this one first?

Best regards,

Emmanuel

Re: [PATCH v2] iwd: Always disconnect connection completely

[Daniel Wagner]

Hi Emmanuel,

On Mon, Feb 21, 2022 at 09:10:09AM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> Do you need more information about this subject?

Yes, Dennis asked you:

  Is there a particular scenario which isn't working?

As I said previously, I don't like the patch as it add complex logic to
the plugin. I would prefer we really understand what is not working as
expected. Maybe this is something which can be easily addressed in iwd
directly?

Daniel

RE: [PATCH v2] iwd: Always disconnect connection completely

[VAUTRIN Emmanuel (Canal Plus Prestataire)]

> Yes, Dennis asked you:
>
>  Is there a particular scenario which isn't working?
>
> As I said previously, I don't like the patch as it add complex logic to
> the plugin. I would prefer we really understand what is not working as
> expected. Maybe this is something which can be easily addressed in iwd
> directly?
Hello Daniel,

In fact, I have already proposed a specific scenario on last mail:
It is easy to reproduce the issue, by switching quickly between two known
Wifi networks (W1 & W2), via the Service api. By quickly, I mean: one call shall
immediately follow the reply of the previous call.
In this case, the following steps happen:
1. Disconnection of current network (W1).
2. Connection to the new one (W2) failing with "net.connman.iwd.Failed"
dbus error in cm_network_connect_cb, probably due to the fact that W1
disconnection was not already completed, aborting connection to W2.

Best Regards,

Emmanuel

Re: [PATCH v2] iwd: Always disconnect connection completely

[Daniel Wagner]

Hi Emmanuel,

On Mon, Feb 21, 2022 at 04:53:05PM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> In fact, I have already proposed a specific scenario on last mail:
> It is easy to reproduce the issue, by switching quickly between two known
> Wifi networks (W1 & W2), via the Service api. By quickly, I mean: one call shall
> immediately follow the reply of the previous call.
> In this case, the following steps happen:
> 1. Disconnection of current network (W1).
> 2. Connection to the new one (W2) failing with "net.connman.iwd.Failed"
> dbus error in cm_network_connect_cb, probably due to the fact that W1
> disconnection was not already completed, aborting connection to W2.

If I understood it correctly, you are saying ConnMan receives a
Disconnect call via D-Bus, it is forwarded to iwd and wait for iwd to
respond that the network is disconnected. Then ConnMan replies the
initial posted Disconnect call to the user back. The user posts then
Connect() which is too fast?

Or does the application post a Disconnect call via D-Bus and immediately
post a Connect() call?

Daniel

RE: [PATCH v2] iwd: Always disconnect connection completely

[VAUTRIN Emmanuel (Canal Plus Prestataire)]

Hi Daniel,
> If I understood it correctly, you are saying ConnMan receives a
> Disconnect call via D-Bus, it is forwarded to iwd and wait for iwd to
> respond that the network is disconnected. Then ConnMan replies the
> initial posted Disconnect call to the user back. The user posts then
> Connect() which is too fast?
I switch between 2 known secure networks.
I have retried this morning with the following sequence:
gdbus call --system -d net.connman -o $PATH1 -m net.connman.Service.Disconnect
()
gdbus call --system -d net.connman -o $PATH2 -m  net.connman.Service.Connect
()
gdbus call --system -d net.connman -o $PATH1 -m net.connman.Service.Connect
Error: GDBus.Error:net.connman.Error.Failed: Input/output error

I think that the D-Bus reply of the Disconnect is immediate and does not wait for
the network to be really disconnected or on error, contrary to the Connect, which
really waits for the connection to be effective before replying.
I thought it was on purpose.

Best Regards,

Emmanuel

Re: [PATCH v2] iwd: Always disconnect connection completely

[Daniel Wagner]

On Mon, Feb 28, 2022 at 11:21:44AM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> > If I understood it correctly, you are saying ConnMan receives a
> > Disconnect call via D-Bus, it is forwarded to iwd and wait for iwd to
> > respond that the network is disconnected. Then ConnMan replies the
> > initial posted Disconnect call to the user back. The user posts then
> > Connect() which is too fast?
> I switch between 2 known secure networks.
> I have retried this morning with the following sequence:
> gdbus call --system -d net.connman -o $PATH1 -m net.connman.Service.Disconnect
> ()
> gdbus call --system -d net.connman -o $PATH2 -m  net.connman.Service.Connect
> ()
> gdbus call --system -d net.connman -o $PATH1 -m net.connman.Service.Connect
> Error: GDBus.Error:net.connman.Error.Failed: Input/output error

Thanks for this example. It really helps understanding what you want to
fix. Is the PATH2 Connect necessary for triggering the error?

> I think that the D-Bus reply of the Disconnect is immediate and does not wait for
> the network to be really disconnected or on error, contrary to the Connect, which
> really waits for the connection to be effective before replying.
> I thought it was on purpose.

IIRC, Disconnect was designed to return immediately. This might be the
source of the problem here.

RE: [PATCH v2] iwd: Always disconnect connection completely

[VAUTRIN Emmanuel (Canal Plus Prestataire)]

> Thanks for this example. It really helps understanding what you want to
> fix. Is the PATH2 Connect necessary for triggering the error?
Hello Daniel, exactly, PATH2 Connect seems to be necessary to reproduce
the issue.

> IIRC, Disconnect was designed to return immediately. This might be the
> source of the problem here.
I think so. And probably the same issue when removing a network, which is
also designed to return immediately. By my side, I have updated this patch
accordingly, to be applied after [PATCH] iwd: Forget network on service removal.

I still need to create a patch on iwd repository itself (related to the hidden networks),
but after that, I hope passing my full test campaign completely successfully, as before
with wpa-supplicant.

Best Regards,

Emmanuel