All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] iwd: Always disconnect connection completely
       [not found] <20220119133927.1245607-1-Emmanuel.VAUTRIN@cpexterne.org>
@ 2022-01-19 13:43 ` VAUTRIN Emmanuel (Canal Plus Prestataire)
  2022-01-21  7:47   ` Daniel Wagner
  0 siblings, 1 reply; 6+ messages in thread
From: VAUTRIN Emmanuel (Canal Plus Prestataire) @ 2022-01-19 13:43 UTC (permalink / raw)
  To: connman

[-- Attachment #1: Type: text/plain, Size: 6584 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 | 114 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 102 insertions(+), 12 deletions(-)

diff --git a/plugins/iwd.c b/plugins/iwd.c
index b0e17a4d6396..48f1f7136e24 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,93 @@ static void cm_network_connect_cb(DBusMessage *message, void *user_data)
         update_network_connected(iwdn);
 }
 
+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 +349,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 +376,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 +395,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 +595,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);
@@ -1307,6 +1393,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

Hi,

Please find the right formatted patch in attachment.


Emmanuel

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

From c3d36262db113abb9967f2998f7ba0f7f9d46851 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] 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 | 114 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 102 insertions(+), 12 deletions(-)

diff --git a/plugins/iwd.c b/plugins/iwd.c
index b0e17a4d6396..48f1f7136e24 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,93 @@ static void cm_network_connect_cb(DBusMessage *message, void *user_data)
 	update_network_connected(iwdn);
 }
 
+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 +349,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 +376,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 +395,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 +595,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);
@@ -1307,6 +1393,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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] iwd: Always disconnect connection completely
  2022-01-19 13:43 ` [PATCH] iwd: Always disconnect connection completely VAUTRIN Emmanuel (Canal Plus Prestataire)
@ 2022-01-21  7:47   ` Daniel Wagner
  2022-01-21 10:50     ` VAUTRIN Emmanuel (Canal Plus Prestataire)
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel Wagner @ 2022-01-21  7:47 UTC (permalink / raw)
  To: VAUTRIN Emmanuel (Canal Plus Prestataire); +Cc: connman

On Wed, Jan 19, 2022 at 01:43:34PM +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")

plugins/iwd.c:254:6: error: no previous declaration for ‘abort_pending_network’ [-Werror=missing-declarations]
  254 | void abort_pending_network(struct iwd_device *iwdd,
      |      ^~~~~~~~~~~~~~~~~~~~~

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] iwd: Always disconnect connection completely
  2022-01-21  7:47   ` Daniel Wagner
@ 2022-01-21 10:50     ` VAUTRIN Emmanuel (Canal Plus Prestataire)
  2022-01-23 13:18       ` Daniel Wagner
  0 siblings, 1 reply; 6+ messages in thread
From: VAUTRIN Emmanuel (Canal Plus Prestataire) @ 2022-01-21 10:50 UTC (permalink / raw)
  To: connman

> plugins/iwd.c:254:6: error: no previous declaration for ‘abort_pending_network’ [-Werror=missing-declarations]
>  254 | void abort_pending_network(struct iwd_device *iwdd,
>      |      ^~~~~~~~~~~~~~~~~~~~~

Hi Daniel.

This is pretty strange, as line 254 corresponds to the declaration
itself, and as I am using  -Wall -Werror by my side.
I have tried to apply this patch again on the current master
and it applies well.
Can you check again with the well formated path that was
in attachment?

Best Regards,

Emmanuel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] iwd: Always disconnect connection completely
  2022-01-21 10:50     ` VAUTRIN Emmanuel (Canal Plus Prestataire)
@ 2022-01-23 13:18       ` Daniel Wagner
  2022-01-23 17:40         ` VAUTRIN Emmanuel (Canal Plus Prestataire)
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel Wagner @ 2022-01-23 13:18 UTC (permalink / raw)
  To: VAUTRIN Emmanuel (Canal Plus Prestataire); +Cc: connman

You forgot a static. BTW, please check your formatting:

$ git am ~/0001-iwd-Always-disconnect-connection-completely.patch
Applying: iwd: Always disconnect connection completely
 *** .git/hooks/pre-applypatch: Testing for mode changes...
 *** .git/hooks/pre-applypatch: Testing that this is buildable...
  GEN      src/builtin.h
make --no-print-directory all-am
  CC       plugins/src_connmand-iwd.o
  CC       src/connmand-plugin.o
plugins/iwd.c:254:6: error: no previous declaration for ‘abort_pending_network’ [-Werror=missing-declarations]
  254 | void abort_pending_network(struct iwd_device *iwdd,
      |      ^~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:3405: plugins/src_connmand-iwd.o] Error 1
make: *** [Makefile:1871: all] Error 2
 *** .git/hooks/pre-applypatch: Didn't pass buildable test


--- a/plugins/iwd.c
+++ b/plugins/iwd.c
@@ -251,8 +251,9 @@ static void cm_network_connect_cb(DBusMessage *message, void *user_data)
        update_network_connected(iwdn);
 }
 
-void abort_pending_network(struct iwd_device *iwdd,
-                               enum connman_network_error error) {
+static void abort_pending_network(struct iwd_device *iwdd,
+                                       enum connman_network_error error)
+{
        struct iwd_network *iwdn;
 
        if (!iwdd->pending_network)
╭─wagi@beryllium ~/cm/connman-upstream ‹master*› 
╰─$ make
  GEN      src/builtin.h
make --no-print-directory all-am
  CC       plugins/src_connmand-iwd.o
  CC       src/connmand-plugin.o
  CCLD     src/connmand

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] iwd: Always disconnect connection completely
  2022-01-23 13:18       ` Daniel Wagner
@ 2022-01-23 17:40         ` VAUTRIN Emmanuel (Canal Plus Prestataire)
  2022-01-24  8:21           ` Daniel Wagner
  0 siblings, 1 reply; 6+ messages in thread
From: VAUTRIN Emmanuel (Canal Plus Prestataire) @ 2022-01-23 17:40 UTC (permalink / raw)
  To: connman

> You forgot a static.
Yes you are right, I have activated  -Wmissing-declarations,
which was not activated by -Wall.
For your information, it seems that some other code does
not compile with this option (xt_match_parse, xt_target_parse in 
unit/test-iptables.c & reset_xtables with no void argument in src/iptables.c).

> BTW, please check your formatting
What do you mean by that? Do you have some link with associated
guidelines to follow?

Best regards

Emmanuel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] iwd: Always disconnect connection completely
  2022-01-23 17:40         ` VAUTRIN Emmanuel (Canal Plus Prestataire)
@ 2022-01-24  8:21           ` Daniel Wagner
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Wagner @ 2022-01-24  8:21 UTC (permalink / raw)
  To: VAUTRIN Emmanuel (Canal Plus Prestataire); +Cc: connman

On Sun, Jan 23, 2022 at 05:40:45PM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> For your information, it seems that some other code does
> not compile with this option (xt_match_parse, xt_target_parse in 
> unit/test-iptables.c & reset_xtables with no void argument in src/iptables.c).

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.

> > BTW, please check your formatting
> What do you mean by that? Do you have some link with associated
> guidelines to follow?

There was the curly bracket on the same line as the function name. It
should be covered in doc/conding-style.txt

Daniel

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-01-24  8:21 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20220119133927.1245607-1-Emmanuel.VAUTRIN@cpexterne.org>
2022-01-19 13:43 ` [PATCH] iwd: Always disconnect connection completely VAUTRIN Emmanuel (Canal Plus Prestataire)
2022-01-21  7:47   ` Daniel Wagner
2022-01-21 10:50     ` VAUTRIN Emmanuel (Canal Plus Prestataire)
2022-01-23 13:18       ` Daniel Wagner
2022-01-23 17:40         ` VAUTRIN Emmanuel (Canal Plus Prestataire)
2022-01-24  8:21           ` Daniel Wagner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.