[PATCH] agent: corrected usage of g_list_delete_link
To avoid memory leaks. --- src/agent.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/agent.c b/src/agent.c index a340026..361a5e1 100644 --- a/src/agent.c +++ b/src/agent.c @@ -521,8 +521,8 @@ void connman_agent_cancel(void *user_context) agent_request_free(request); - agent->queue = list->next; - list = g_list_delete_link(list, list); + agent->queue = g_list_delete_link(agent->queue, + list); } else list = list->next; } -- 1.8.3.2 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH] dnsproxy: request_data in request_list need the request data
The ones received over UDP didn't have it. --- src/dnsproxy.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/dnsproxy.c b/src/dnsproxy.c index 9787b68..0698387 100644 --- a/src/dnsproxy.c +++ b/src/dnsproxy.c @@ -3467,6 +3467,9 @@ static bool udp_listener_event(GIOChannel *channel, GIOCondition condition, return true; } + req->name = g_strdup(query); + req->request = g_malloc(len); + memcpy(req->request, buf, len); req->timeout = g_timeout_add_seconds(5, request_timeout, req); request_list = g_slist_append(request_list, req); -- 1.8.3.2 ___ 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
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
Re: [PATCH] gsupplicant: Remove unused networks
On 02/04/15 13:46, 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? > > 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 #8 0x4031b1e8 in g_hash_table_remove_node () at ghash.c:448 #9 0x4031b9d4 in g_hash_table_remove_internal () at ghash.c:1276 #10 0x00025e04 in interface_bss_removed () at gsupplicant/supplicant.c:1809 #11 signal_bss_removed () at gsupplicant/supplicant.c:2252 #12 0x0002332c in g_supplicant_filter () at gsupplicant/supplicant.c:2636 #13 0x40481f50 in dbus_connection_dispatch () from /usr/lib/libdbus-1.so.3 #14 0x000833a0 in message_dispatch () at gdbus/mainloop.c:72 #15 0x4032da8c in g_idle_dispatch () at gmain.c:5251 #16 0x40331b20 in g_main_dispatch () at gmain.c:3066 #17 g_main_context_dispatch () at gmain.c:3642 #18 0x40331e24 in g_main_context_iterate () at gmain.c:3713 #19 0x4033248c in g_main_loop_run () at gmain.c:3906 #20 0x00014b9c in main () at src/main.c:761 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. 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". -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. 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
[PATCH] configure: Add option to set path of the pptp binary
User can set the path to client binary using --with-pptp option.
---
configure.ac | 3 +++
1 file changed, 3 insertions(+)
diff --git a/configure.ac b/configure.ac
index 85c01b9..5eb5a34 100644
--- a/configure.ac
+++ b/configure.ac
@@ -146,6 +146,9 @@ fi
AM_CONDITIONAL(L2TP, test "${enable_l2tp}" != "no")
AM_CONDITIONAL(L2TP_BUILTIN, test "${enable_l2tp}" = "builtin")
+AC_ARG_WITH(pptp, AC_HELP_STRING([--with-pptp=PROGRAM],
+[specify location of pptp binary]), [path_pptp=${withval}])
+
AC_ARG_ENABLE(pptp,
AC_HELP_STRING([--enable-pptp], [enable pptp support]),
[enable_pptp=${enableval}], [enable_pptp="no"])
--
2.1.0
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] configure: Add option to set path of the l2tp binary
User can set the path to client binary using --with-l2tp option.
---
configure.ac | 3 +++
1 file changed, 3 insertions(+)
diff --git a/configure.ac b/configure.ac
index cec10bf..85c01b9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -121,6 +121,9 @@ fi
AM_CONDITIONAL(VPNC, test "${enable_vpnc}" != "no")
AM_CONDITIONAL(VPNC_BUILTIN, test "${enable_vpnc}" = "builtin")
+AC_ARG_WITH(l2tp, AC_HELP_STRING([--with-l2tp=PROGRAM],
+[specify location of l2tp binary]), [path_l2tp=${withval}])
+
AC_ARG_ENABLE(l2tp,
AC_HELP_STRING([--enable-l2tp], [enable l2tp support]),
[enable_l2tp=${enableval}], [enable_l2tp="no"])
--
2.1.0
___
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] inet: fixed the check of inet_pton return value
Hi Slava,
On to, 2015-04-02 at 12:20 +0300, Slava Monich wrote:
> On 02/04/15 09:14, Patrik Flykt wrote:
> > @@ -634,17 +634,15 @@ int connman_inet_add_ipv6_network_route(int index,
> > const char *host,
> >
> > rt.rtmsg_dst_len = prefix_len;
> >
> > - if (inet_pton(AF_INET6, host, &rt.rtmsg_dst) < 0) {
> > + if (inet_pton(AF_INET6, host, &rt.rtmsg_dst) < 1) {
> > err = -errno;
> > goto out;
> > }
> >
> > rt.rtmsg_flags = RTF_UP | RTF_HOST;
> >
> > - if (gateway) {
> > + if (gateway && inet_pton(AF_INET6, gateway, &rt.rtmsg_gateway) > 0)
> > rt.rtmsg_flags |= RTF_GATEWAY;
> > - inet_pton(AF_INET6, gateway, &rt.rtmsg_gateway);
> > - }
> > At this point if inet_pton fails, why are we continuing with the
> > rt.rtmsg? Apparently there was a gateway with an unusable IPv6 address
> > so shouldn't we return error instead?
>
>
> In cases where it does happen in reality, even if we return an error,
> the caller (add_nameserver_route) would call
> connman_inet_add_ipv6_network_route again with NULL gateway. This code
> is doing it in one shot but it does make an assumption about what the
> caller actually means by providing an invalid gateway address.
>
> In any case it's better than using bogus gateway address.
As Patrik mentioned, why not just do
if (gateway) {
if (inet_pton(AF_INET6, gateway, &rt.rtmsg_gateway) < 1)
return -EINVAL;
rt.rtmsg_flags |= RTF_GATEWAY;
}
and let caller decide what to do next if gateway address was bogus.
Cheers,
Jukka
___
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] inet: fixed the check of inet_pton return value
On 02/04/15 09:14, Patrik Flykt wrote:
> @@ -634,17 +634,15 @@ int connman_inet_add_ipv6_network_route(int index,
> const char *host,
>
> rt.rtmsg_dst_len = prefix_len;
>
> - if (inet_pton(AF_INET6, host, &rt.rtmsg_dst) < 0) {
> + if (inet_pton(AF_INET6, host, &rt.rtmsg_dst) < 1) {
> err = -errno;
> goto out;
> }
>
> rt.rtmsg_flags = RTF_UP | RTF_HOST;
>
> - if (gateway) {
> + if (gateway && inet_pton(AF_INET6, gateway, &rt.rtmsg_gateway) > 0)
> rt.rtmsg_flags |= RTF_GATEWAY;
> - inet_pton(AF_INET6, gateway, &rt.rtmsg_gateway);
> - }
> At this point if inet_pton fails, why are we continuing with the
> rt.rtmsg? Apparently there was a gateway with an unusable IPv6 address
> so shouldn't we return error instead?
In cases where it does happen in reality, even if we return an error,
the caller (add_nameserver_route) would call
connman_inet_add_ipv6_network_route again with NULL gateway. This code
is doing it in one shot but it does make an assumption about what the
caller actually means by providing an invalid gateway address.
In any case it's better than using bogus gateway address.
Regards,
-Slava
___
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
