Re: [PATCH] gsupplicant: Remove unused networks
On Sun, 2015-04-05 at 14:35 +0300, Slava Monich wrote: I think I finally caught one of the scenarios like that. The trick was to receive DISCONNECTED event from wpa_supplicant followed by ASSOCIATING while network-connecting is set to true. 1. DISCONNECTED sets device-scanning to true 2. ASSOCIATING sets device-scanning to false, which invokes remove_unavailable_network(), sets network-driver to NULL and eventually calls __connman_service_remove_from_network() for the network in question. Now the service is holding reference to the dead network, update_from_network() doesn't work because network-connecting is true, __connman_network_disconnect() doesn't work because network-driver is NULL. That's it. I just sent a patch that should prevent that from happening in two places along this course of events - by dropping reference to the dead service in __connman_service_remove_from_network() and in update_from_network(). Under this particular scenario the latter seems to be unnecessary but I'm pretty sure there are more bugs floating around so I suggest to keep both checks. This is way too late/too high in the stack. Please fix this latest in plugins/wifi.c if gsupplicant cannot handle this. Cheers, Patrik ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] gsupplicant: Remove unused networks
On 02/04/15 09:25, Patrik Flykt wrote:
@@ -4041,6 +4046,15 @@ static void interface_add_network_result(const char
*error,
SUPPLICANT_DBG(PATH: %s, path);
+ if (interface-network_path strcmp(interface-network_path, path)) {
+ /* Prevent unused wpa_supplicant networks from piling up */
+ SUPPLICANT_DBG(Removing network %s, interface-network_path);
+ supplicant_dbus_method_call(interface-path,
+ SUPPLICANT_INTERFACE .Interface, RemoveNetwork,
+ objpath_param, NULL, interface-network_path,
+ interface);
+ }
+
Why do we care about how wpa_supplicant internally handles its networks
at this point, especially for the wpa_cli tool? AFAIK wpa_supplicant
will clear out networks after ~2 mins if they haven't been seen in a
later scan.
Strictly speaking it's not much of a problem for connman since it's
wpa_supplicant who is going to run out of memory.
It appears that the client is responsible for removing network
configurations it has created, which makes sense to me. In an
environment with unstable wifi reception, connman may end up creating
hundreds of those in just one day of normal usage.
What is the logic to remove something that apparently isn't matching the
provided user data? How is this even possible?
I don't quite understand what you mean by provided user data but
data-interface-network_path points to the path of the previously
created network configuration and that's all the data one needs to
remove it when it's not longer needed.
Regards,
-Slava
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] gsupplicant: Remove unused networks
Hi Slava, It appears that the client is responsible for removing network configurations it has created, which makes sense to me. In an environment with unstable wifi reception, connman may end up creating hundreds of those in just one day of normal usage. Well this is true if it always fail connecting to hundreds different networks yes (and only at SelectNetwork). That's the only possibility. So indeed there is a bug, but unlikely to happen. Your patch is fine but it is not removing the network at the right place then. You should do it in interface_select_network_result() if that one returns an error. So the network is removed as soon as possible. I don't think there is any other case matching this situation: when we disconnect, we remove the network. If AddNetwork fails, no need to remove anything. I think it should be fine then. Also, try reusing existing code: network_remove() does what you want already. Tomasz ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] gsupplicant: Remove unused networks
Hi Slava, It appears that the client is responsible for removing network configurations it has created, which makes sense to me. In an environment with unstable wifi reception, connman may end up creating hundreds of those in just one day of normal usage. Well this is true if it always fail connecting to hundreds different networks yes (and only at SelectNetwork). That's the only possibility. So indeed there is a bug, but unlikely to happen. Try this: connect to wifi, walk out of the wifi range, wait for connman to realize that, return back into the range, wait for connman to re-connect and compare 'wpa_cli list_net' before and after the exercise. What do you get? Ah, That's another bug then! If we detect that we got disconnected, the wifi plugins will need to cleanup. Actually, it cleanups if the user disconnects, but not this way round. Check interface_state() in wifi_plugins. This will need a separate patch. Your patch is fine but it is not removing the network at the right place then. You should do it in interface_select_network_result() if that one returns an error. So the network is removed as soon as possible. As soon as possible is where I put it. By the time it gets to interface_select_network_result() the previous path is lost. In the context I was thinking, the SelectNetwork that fails, this is the place to go. If you handle this non-user disconnection properly, there will be no need to run anything in interface_add_network_result(). Actually, gsupplicant should not run AddNetwork if interface-network_path is a valid pointer. If this pointer is already set, it means we are doing something wrong elsewhere, which we are obviously. I don't think there is any other case matching this situation: when we disconnect, we remove the network. If AddNetwork fails, no need to remove anything. I think it should be fine then. Also, try reusing existing code: network_remove() does what you want already. network_remove() requires allocation of another interface_data which in this case was unnecessary and would require writing more code than I wanted to. That's true. Now that might be an occasion to factorize the code: make network_remove() generic so it would allocate or not some data (for instance if there is a callback/user data, then allocate. If not: NULL). As you want, but that could avoid duplicate routines. Just looked at the code, network_remove(GSupplicantInterface *interface, struct interface_data *data); could be the solution. Use the interface pointer (not the one that might be in data). If data is valid, use that one to add the path parameter, if not use your function objpath_params() which you could rename as network_remove_append_objpath() for instance. Tomasz ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] gsupplicant: Remove unused networks
Hi Slava, Ah, That's another bug then! If we detect that we got disconnected, the wifi plugins will need to cleanup. Actually, it cleanups if the user disconnects, but not this way round. Check interface_state() in wifi_plugins. This will need a separate patch. Well, it actually kind of works. When wifi signal is lost, wifi plugin does some sort of cleanup like this: #1 0x0003d8cc in free_network () at src/device.c:378 #2 0x4031b1e8 in g_hash_table_remove_node () at ghash.c:448 #3 0x4031b9d4 in g_hash_table_remove_internal () at ghash.c:1276 #4 0x0003ea20 in connman_device_remove_network () at src/device.c:905 #5 0x000201c4 in network_removed () at plugins/wifi.c:2064 #6 0x0002388c in callback_network_removed () at gsupplicant/supplicant.c:445 #7 remove_network () at gsupplicant/supplicant.c:523 Ok, yes is removes the gsupplicant's network entity, but does not remove the wpa_supplicant's side of it, if it has been added there. And that happens only when the ssid cannot be found from scan result. I'll check, but I am sure we are missing a cleanup at the point where we get disconnected in interface_state(). (actually, and that's a kind of design mistake, gsupplicant's network entity does not know anything about itself being added or not in wpa_supplicant... only interface knows) There are some corner cases that I have been unable to catch so far, which result in connman_service holding a reference to connman_network in connecting/associating state (albeit with driver pointer being NULL) but those are quite rare. That sort of things happens once in a new weeks of normal usage (which involves periodic switching between mobile data and various wifi networks) but once it does happen, connman is unable to recover and needs to be restarted. That's quite annoying for the end-users of the device. Yup, looks like a critical bug. If by chance you get the connman logs of such issue, it will be welcomed. But normally things get cleaned up all right, except that wpa_supplicant configuration doesn't get removed, which is the problem that I addressed in this patch. Since I'm not the one who designed this thing, I prefer small fixes that don't affect the overall architecture which at times is pretty hard to grasp. If you are not happy with this approach, you are quite welcome to fix it the right way. gsupplicant/wifi plugins is indeed probably the most complicated part, and for a good reason: have a look at wpa_supplicant DBus API ;( plus some design issues (maybe the gsupplicant's network part could have been avoided at least. The thing is: you patch at this stage is not going to make it upstream. Up to you. Tomasz ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] gsupplicant: Remove unused networks
On 02/04/15 12:35, Tomasz Bursztyka wrote: Hi Slava, It appears that the client is responsible for removing network configurations it has created, which makes sense to me. In an environment with unstable wifi reception, connman may end up creating hundreds of those in just one day of normal usage. Well this is true if it always fail connecting to hundreds different networks yes (and only at SelectNetwork). That's the only possibility. So indeed there is a bug, but unlikely to happen. Try this: connect to wifi, walk out of the wifi range, wait for connman to realize that, return back into the range, wait for connman to re-connect and compare 'wpa_cli list_net' before and after the exercise. What do you get? I'm consistently getting from 1 to 3 new network configurations per each trip outside of the wifi range. Your patch is fine but it is not removing the network at the right place then. You should do it in interface_select_network_result() if that one returns an error. So the network is removed as soon as possible. As soon as possible is where I put it. By the time it gets to interface_select_network_result() the previous path is lost. I don't think there is any other case matching this situation: when we disconnect, we remove the network. If AddNetwork fails, no need to remove anything. I think it should be fine then. Also, try reusing existing code: network_remove() does what you want already. network_remove() requires allocation of another interface_data which in this case was unnecessary and would require writing more code than I wanted to. -Slava ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] gsupplicant: Remove unused networks
Hi, On Thu, 2015-04-02 at 14:32 +0300, Slava Monich wrote: Since I'm not the one who designed this thing, I prefer small fixes that don't affect the overall architecture which at times is pretty hard to grasp. If you are not happy with this approach, you are quite welcome to fix it the right way. The patch you sent received comments that there are more than one bug floating around and that a more appropriate and logical fix exists if done elsewhere in the code. Please update your patch according to comments so we get rid of all related bugs. Cheers, Patrik ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH] gsupplicant: Remove unused networks
Remove previously created network after creating a new one.
This prevents wpa_supplicant network objects (wpa_cli list_net)
from piling up.
---
gsupplicant/supplicant.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index cd91f95..749eb20 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -4024,6 +4024,11 @@ static void
interface_select_network_params(DBusMessageIter *iter,
interface-network_path);
}
+static void objpath_param(DBusMessageIter *iter, void *path)
+{
+ dbus_message_iter_append_basic(iter, DBUS_TYPE_OBJECT_PATH, path);
+}
+
static void interface_add_network_result(const char *error,
DBusMessageIter *iter, void *user_data)
{
@@ -4041,6 +4046,15 @@ static void interface_add_network_result(const char
*error,
SUPPLICANT_DBG(PATH: %s, path);
+ if (interface-network_path strcmp(interface-network_path, path)) {
+ /* Prevent unused wpa_supplicant networks from piling up */
+ SUPPLICANT_DBG(Removing network %s, interface-network_path);
+ supplicant_dbus_method_call(interface-path,
+ SUPPLICANT_INTERFACE .Interface, RemoveNetwork,
+ objpath_param, NULL, interface-network_path,
+ interface);
+ }
+
g_free(interface-network_path);
interface-network_path = g_strdup(path);
--
1.8.3.2
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
