[PATCH] service: Fix minor memory leak on exit
services_notify->add and services_notify->remove are always created
and have to be always destroyed.
---
src/service.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/service.c b/src/service.c
index 196f6b5..02a6844 100644
--- a/src/service.c
+++ b/src/service.c
@@ -7118,9 +7118,10 @@ void __connman_service_cleanup(void)
if (services_notify->id != 0) {
g_source_remove(services_notify->id);
service_send_changed(NULL);
- g_hash_table_destroy(services_notify->remove);
- g_hash_table_destroy(services_notify->add);
}
+
+ g_hash_table_destroy(services_notify->remove);
+ g_hash_table_destroy(services_notify->add);
g_free(services_notify);
dbus_connection_unref(connection);
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH 1/5] service: Add function to remove empty strings
Hi Jaakko,
On ke, 2015-07-15 at 11:35 +0300, Jaakko Hannikainen wrote:
This helper function takes in a heap-allocated buffer of heap-allocated
strings. If no strings should be removed, return the buffer. Else, free
all empty strings, place nonempty strings to a new container and return
it, freeing the old container.
---
src/service.c | 32
1 file changed, 32 insertions(+)
diff --git a/src/service.c b/src/service.c
index 2d8245e..1723586 100644
--- a/src/service.c
+++ b/src/service.c
@@ -2926,6 +2926,38 @@ static DBusMessage *get_properties(DBusConnection
*conn,
return reply;
}
+static char **remove_empty_strings(char **strv)
+{
+int amount, length, index;
+char **iter, **out;
+
+amount = 0;
+length = g_strv_length(strv);
We could remove the call to g_strv_length() and calculate the max length
in the while loop below.
+iter = strv;
+
+while (*iter)
+if (strlen(*iter++))
+amount++;
And it's unnecessary to calculate the length of each string. Twice.
It would suffice to just check the first byte.
Regards,
-Slava
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
RFC: D-Bus access to all configured services
Does upstream have any interest in providing D-Bus access to all configured services, including those that have no network associated with it? Before I start writing any code, I would like to know whether it's going to be accepted upstream or will remain part of our fork. Basically the idea is to add available (or whatever you want to call it) service property which would be true if it's associated with the network, or false otherwise. Which types of services would provide access to all services and which only to those that are available, would be configurable via the conf file. All D-Bus calls (except for Connect obviously) would work for unavailable service just as they do for available services. The main use case is wifi. The user has to be able to see the list of configured wifi networks, edit their properties, remove them etc. For other types of services it probably doesn't make sense but who knows. I found a relevant 3+ years old discussion in the archives with some patches, the reaction back then wasn't a definite no and yet those patches were never applied. What's the current attitude towards that? Regards, -Slava ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: RFC: D-Bus access to all configured services
On 10/07/15 14:51, Marcel Holtmann wrote: Hi Slava, Does upstream have any interest in providing D-Bus access to all configured services, including those that have no network associated with it? Before I start writing any code, I would like to know whether it's going to be accepted upstream or will remain part of our fork. Basically the idea is to add available (or whatever you want to call it) service property which would be true if it's associated with the network, or false otherwise. Which types of services would provide access to all services and which only to those that are available, would be configurable via the conf file. All D-Bus calls (except for Connect obviously) would work for unavailable service just as they do for available services. The main use case is wifi. The user has to be able to see the list of configured wifi networks, edit their properties, remove them etc. For other types of services it probably doesn't make sense but who knows. I found a relevant 3+ years old discussion in the archives with some patches, the reaction back then wasn't a definite no and yet those patches were never applied. What's the current attitude towards that? it was a clear no against an Available property. That would be the wrong direction to take this. It would also not be backwards compatible and all clients would need to understand what that property means to fix their UI. Backward compatibility could be provided by having this functionality off by default. And yes, if you turn it on, you would probably have to fix your UI. And I have seen such an UI in Android. A total disaster and I have no intention to put anybody through this. My question was about providing D-Bus access to saved services, not the UI. But since you have touched this topic, forget Android, how would you implement iPhone-style UI with the current D-Bus interface? Anyway, I've got the answer I was looking for. Thanks. -Slava ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: RFC: D-Bus access to all configured services
On 10/07/15 14:56, Tomasz Bursztyka wrote: Hi Slava, All D-Bus calls (except for Connect obviously) would work for unavailable service just as they do for available services. The main use case is wifi. The user has to be able to see the list of configured wifi networks, edit their properties, remove them etc. For other types of services it probably doesn't make sense but who knows. Isn't it the same behavior as Jolla's phone has? Not quite. The user gets to see the list of saved networks but their properties are not editable (and not even available in the current UI but that's purely a UI issue). That's why I asked this question. I still don't get the usefulness of that feature. Why annoying the user with service he cannot connect to, at a time T? (why would he feel the need to tweak the parameters if the network is not even present?!) Two scenarios off the top of my head: 1. To see the password (if you have forgotten it) or proxy configuration (to copy/paste it into the new configuration) 2. To set Favourite to false so that it doesn't get automatically connected when it becomes available And generally, if these properties are there and changing them doesn't break anything, why not to provide access to them, especially if there's mechanism already in place for it. connman is a piece of middleware, its job is to provide the functionality and leave it to UI designers to decide what to show, what not to show and how to present it to the user. Regards, -Slava Tomasz ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: RFC: D-Bus access to all configured services
On 10/07/15 16:07, Tomasz Bursztyka wrote: Hi Slava, connman is a piece of middleware, its job is to provide the functionality and leave it to UI designers to decide what to show, what not to show and how to present it to the user. That's a debate on its own. Actually, I don't think UI designers should be the only responsible for what to expose on the UI. It then creates a top-to-bottom dependency, without a proper understanding of the lower layers, which tend to generate more problem with time (from maintaining the API to properly architecture the service etc...). Like many other things, it's a trade-off. Allowing UI designers to blindly produce functional requirements may be a pain from the middleware developer's prospective. On the other hand, letting developers define their own UI would most likely result in something chaotic and visually disturbing like your beloved Android UI (which I'm not actually familiar with but I trust you that it's horrible). Both parties have to talk to each other but someone has to take care of the general consistency across the entire UI platform and it's clearly the job of UI designers. Regarding connman, it's positioned as portable middleware component for Linux-based systems. As such, it can't possibly make any assumptions about the UI. The platform may have no UI at all. Regards, -Slava Tomasz ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] service: Reset state to idle when disassociating service from the network.
On 09/07/15 11:14, Patrik Flykt wrote: On Wed, 2015-07-08 at 17:05 +0300, Slava Monich wrote: Otherwise, service may get stuck in the ASSOCIATION state forever and update_from_network() won't do anything because is_connecting() keeps returning true, making recovery impossible. This is not the place to handle a stuck association. Calling __connman_service_remove_from_network() is always due to a group removal, causes no change to connectivity status except that the gateway is removed. It's the set_disconnected() earlier in src/network.c, network_remove() that is supposed to do the job. It only does the job for the networks that have been connected: if (network-connected) set_disconnected(network); This patch prevents services that are BEING connected from getting stuck in the that state forever. -Slava Cheers, Patrik --- src/service.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/service.c b/src/service.c index 2d8245e..bd150aa 100644 --- a/src/service.c +++ b/src/service.c @@ -6858,6 +6858,12 @@ void __connman_service_remove_from_network(struct connman_network *network) __connman_connection_gateway_remove(service, CONNMAN_IPCONFIG_TYPE_ALL); +__connman_service_ipconfig_indicate_state(service, +CONNMAN_SERVICE_STATE_IDLE, +CONNMAN_IPCONFIG_TYPE_IPV4); +__connman_service_ipconfig_indicate_state(service, +CONNMAN_SERVICE_STATE_IDLE, +CONNMAN_IPCONFIG_TYPE_IPV6); connman_service_unref(service); } ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] service: Reset state to idle when disassociating service from the network.
On 09/07/15 13:03, Tomasz Bursztyka wrote: Hi Slava, It only does the job for the networks that have been connected: if (network-connected) set_disconnected(network); Why not adding there a test on network-associating/network-connecting as well? Because __connman_service_remove_from_network() is invoked from two places. Besides, most of what set_disconnected() is doing is irrelevant in case if the network hasn't been connected. Regards, -Slava Tomasz ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH] service: Add __connman_service_ipconfig_indicate_states function
Calling __connman_service_ipconfig_indicate_state for both IPv4 and IPv6 is a fairly common pattern on connman. It makes sense to combine these two calls into one function. --- src/connman.h | 2 ++ src/network.c | 17 - src/service.c | 34 +++--- 3 files changed, 21 insertions(+), 32 deletions(-) diff --git a/src/connman.h b/src/connman.h index aac6a0b..c5a94c9 100644 --- a/src/connman.h +++ b/src/connman.h @@ -703,6 +703,8 @@ int __connman_service_online_check_failed(struct connman_service *service, int __connman_service_ipconfig_indicate_state(struct connman_service *service, enum connman_service_state new_state, enum connman_ipconfig_type type); +void __connman_service_ipconfig_indicate_states(struct connman_service *service, + enum connman_service_state new_state); enum connman_service_state __connman_service_ipconfig_get_state( struct connman_service *service, enum connman_ipconfig_type type); diff --git a/src/network.c b/src/network.c index badb770..cd66c4e 100644 --- a/src/network.c +++ b/src/network.c @@ -697,13 +697,8 @@ static void set_disconnected(struct connman_network *network) } } - __connman_service_ipconfig_indicate_state(service, - CONNMAN_SERVICE_STATE_IDLE, - CONNMAN_IPCONFIG_TYPE_IPV4); - - __connman_service_ipconfig_indicate_state(service, - CONNMAN_SERVICE_STATE_IDLE, - CONNMAN_IPCONFIG_TYPE_IPV6); + __connman_service_ipconfig_indicate_states(service, + CONNMAN_SERVICE_STATE_IDLE); network-connecting = false; network-connected = false; @@ -1214,12 +1209,8 @@ int connman_network_set_associating(struct connman_network *network, struct connman_service *service; service = connman_service_lookup_from_network(network); - __connman_service_ipconfig_indicate_state(service, - CONNMAN_SERVICE_STATE_ASSOCIATION, - CONNMAN_IPCONFIG_TYPE_IPV4); - __connman_service_ipconfig_indicate_state(service, - CONNMAN_SERVICE_STATE_ASSOCIATION, - CONNMAN_IPCONFIG_TYPE_IPV6); + __connman_service_ipconfig_indicate_states(service, + CONNMAN_SERVICE_STATE_ASSOCIATION); } return 0; diff --git a/src/service.c b/src/service.c index 2d8245e..00385ac 100644 --- a/src/service.c +++ b/src/service.c @@ -3927,12 +3927,8 @@ static gboolean connect_timeout(gpointer user_data) } else autoconnect = true; - __connman_service_ipconfig_indicate_state(service, - CONNMAN_SERVICE_STATE_FAILURE, - CONNMAN_IPCONFIG_TYPE_IPV4); - __connman_service_ipconfig_indicate_state(service, - CONNMAN_SERVICE_STATE_FAILURE, - CONNMAN_IPCONFIG_TYPE_IPV6); + __connman_service_ipconfig_indicate_states(service, + CONNMAN_SERVICE_STATE_FAILURE); if (autoconnect service-connect_reason != @@ -5452,12 +5448,8 @@ int __connman_service_indicate_error(struct connman_service *service, set_error(service, error); - __connman_service_ipconfig_indicate_state(service, - CONNMAN_SERVICE_STATE_FAILURE, - CONNMAN_IPCONFIG_TYPE_IPV4); - __connman_service_ipconfig_indicate_state(service, - CONNMAN_SERVICE_STATE_FAILURE, - CONNMAN_IPCONFIG_TYPE_IPV6); + __connman_service_ipconfig_indicate_states(service, + CONNMAN_SERVICE_STATE_FAILURE); return 0; } @@ -5478,13 +5470,8 @@ int __connman_service_clear_error(struct connman_service *service) provider_pending = service-provider_pending; service-provider_pending = NULL; - __connman_service_ipconfig_indicate_state(service, - CONNMAN_SERVICE_STATE_IDLE, - CONNMAN_IPCONFIG_TYPE_IPV6); - - __connman_service_ipconfig_indicate_state(service, - CONNMAN_SERVICE_STATE_IDLE, -
[PATCH] service: Reset state to idle when disassociating service from the network.
Otherwise, service may get stuck in the ASSOCIATION state forever and update_from_network() won't do anything because is_connecting() keeps returning true, making recovery impossible. --- src/service.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/service.c b/src/service.c index 2d8245e..bd150aa 100644 --- a/src/service.c +++ b/src/service.c @@ -6858,6 +6858,12 @@ void __connman_service_remove_from_network(struct connman_network *network) __connman_connection_gateway_remove(service, CONNMAN_IPCONFIG_TYPE_ALL); + __connman_service_ipconfig_indicate_state(service, + CONNMAN_SERVICE_STATE_IDLE, + CONNMAN_IPCONFIG_TYPE_IPV4); + __connman_service_ipconfig_indicate_state(service, + CONNMAN_SERVICE_STATE_IDLE, + CONNMAN_IPCONFIG_TYPE_IPV6); connman_service_unref(service); } -- 1.8.3.2 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH] gsupplicant: Add support for SignalPoll
Unlike Scan, SignalPoll is not supposed to disturb active connections
or affect throughput in any way. The primary use case is querying the
signal strength of the AP we are currently connected to.
---
gsupplicant/gsupplicant.h | 14 ++
gsupplicant/supplicant.c | 121 ++
2 files changed, 135 insertions(+)
mode change 100755 = 100644 gsupplicant/supplicant.c
diff --git a/gsupplicant/gsupplicant.h b/gsupplicant/gsupplicant.h
index 2a87f2f..161c8a2 100644
--- a/gsupplicant/gsupplicant.h
+++ b/gsupplicant/gsupplicant.h
@@ -168,6 +168,13 @@ struct _GSupplicantScanParams {
uint16_t *freqs;
};
+typedef struct _GSupplicantSignalPoll {
+ int rssi;
+ int linkspeed;
+ int noise;
+ unsigned int frequency;
+} GSupplicantSignalPoll;
+
typedef struct _GSupplicantScanParams GSupplicantScanParams;
struct _GSupplicantPeerParams {
@@ -211,6 +218,10 @@ typedef void (*GSupplicantInterfaceCallback) (int result,
GSupplicantInterface *interface,
void *user_data);
+typedef void (*GSupplicantSignalPollCallback) (int result,
+ const GSupplicantSignalPoll
*data,
+ void *user_data);
+
void g_supplicant_interface_cancel(GSupplicantInterface *interface);
int g_supplicant_interface_create(const char *ifname, const char *driver,
@@ -287,6 +298,9 @@ int g_supplicant_interface_set_country(GSupplicantInterface
*interface,
GSupplicantCountryCallback callback,
const char *alpha2,
void *user_data);
+int g_supplicant_interface_signal_poll(GSupplicantInterface *interface,
+ GSupplicantSignalPollCallback callback,
+ void *user_data);
bool g_supplicant_interface_has_p2p(GSupplicantInterface *interface);
int g_supplicant_interface_set_p2p_device_config(GSupplicantInterface
*interface,
const char *device_name,
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
old mode 100755
new mode 100644
index fb62a97..2e9b640
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -3322,6 +3322,127 @@ int
g_supplicant_interface_set_country(GSupplicantInterface *interface,
return ret;
}
+static bool signal_poll_get_data(DBusMessageIter *iter,
+ GSupplicantSignalPoll *data)
+{
+ DBusMessageIter array, dict;
+
+ memset(data, 0, sizeof(*data));
+ if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_VARIANT)
+ return false;
+
+ dbus_message_iter_recurse(iter, array);
+ if (dbus_message_iter_get_arg_type(array) != DBUS_TYPE_ARRAY)
+ return false;
+
+ dbus_message_iter_recurse(array, dict);
+ while (dbus_message_iter_get_arg_type(dict) == DBUS_TYPE_DICT_ENTRY) {
+ DBusMessageIter entry, value;
+ DBusBasicValue basic;
+ const char *key = NULL;
+
+ dbus_message_iter_recurse(dict, entry);
+if (dbus_message_iter_get_arg_type(entry) != DBUS_TYPE_STRING)
+return false;
+
+ dbus_message_iter_get_basic(entry, key);
+ dbus_message_iter_next(entry);
+if (dbus_message_iter_get_arg_type(entry) !=
DBUS_TYPE_VARIANT)
+return false;
+
+ dbus_message_iter_recurse(entry, value);
+ if (g_str_equal(key, rssi)) {
+ if (dbus_message_iter_get_arg_type(value) !=
+ DBUS_TYPE_INT32)
+ return false;
+
+ dbus_message_iter_get_basic(value, basic);
+ data-rssi = basic.i32;
+ } else if (g_str_equal(key, linkspeed)) {
+ if (dbus_message_iter_get_arg_type(value) !=
+ DBUS_TYPE_INT32)
+ return false;
+
+ dbus_message_iter_get_basic(value, basic);
+ data-linkspeed = basic.i32;
+ } else if (g_str_equal(key, noise)) {
+ if (dbus_message_iter_get_arg_type(value) !=
+ DBUS_TYPE_INT32)
+ return false;
+
+ dbus_message_iter_get_basic(value, basic);
+ data-noise = basic.i32;
+ } else if (g_str_equal(key, frequency)) {
+ if (dbus_message_iter_get_arg_type(value) !=
+
[PATCH] ofono: Fix crash in modem_update_interfaces
modem_update_interfaces could crash if org.ofono.ConnectionManager
interface is removed right after it has been added, before GetContext
call completes (or if it fails):
connmand[5141]: plugins/ofono.c:modem_changed() /ril_0 Interfaces 0x05
connmand[5141]: plugins/ofono.c:modem_update_interfaces() /ril_0
connmand[5141]: plugins/ofono.c:api_added() cm added
connmand[5141]: plugins/ofono.c:get_properties() /ril_0 path /ril_0
org.ofono.ConnectionManager
connmand[5141]: plugins/ofono.c:cm_get_contexts() /ril_0
connmand[5141]: plugins/ofono.c:cm_update_attached() /ril_0 Attached 1
connmand[5141]: plugins/ofono.c:modem_changed() /ril_0 Interfaces 0x01
connmand[5141]: plugins/ofono.c:modem_update_interfaces() /ril_0
connmand[5141]: plugins/ofono.c:api_removed() cm removed
==5141== Invalid read of size 4
==5141==at 0x31FB4: modem_update_interfaces (ofono.c:2147)
==5141==by 0x326F3: modem_changed (ofono.c:2214)
==5141==by 0x82C0B: signal_filter (watch.c:407)
==5141==by 0x82A4F: message_filter (watch.c:557)
==5141==by 0x497AF4F: dbus_connection_dispatch (in
/usr/lib/libdbus-1.so.3.7.12)
==5141==by 0x8197F: message_dispatch (mainloop.c:72)
==5141==by 0x48ABA8B: g_idle_dispatch (gmain.c:5251)
==5141==by 0x48AFB1F: g_main_dispatch (gmain.c:3066)
==5141==by 0x48AFB1F: g_main_context_dispatch (gmain.c:3642)
==5141==by 0x48AFE23: g_main_context_iterate.part.19 (gmain.c:3713)
==5141==by 0x48B048B: g_main_loop_run (gmain.c:3906)
==5141==by 0x149D3: main (main.c:779)
==5141== Address 0x0 is not stack'd, malloc'd or (recently) free'd
==5141==
==5141==
==5141== Process terminating with default action of signal 11 (SIGSEGV):
dumping core
==5141== Access not within mapped region at address 0x0
==5141==at 0x31FB4: modem_update_interfaces (ofono.c:2147)
==5141==by 0x326F3: modem_changed (ofono.c:2214)
==5141==by 0x82C0B: signal_filter (watch.c:407)
==5141==by 0x82A4F: message_filter (watch.c:557)
==5141==by 0x497AF4F: dbus_connection_dispatch (in
/usr/lib/libdbus-1.so.3.7.12)
==5141==by 0x8197F: message_dispatch (mainloop.c:72)
==5141==by 0x48ABA8B: g_idle_dispatch (gmain.c:5251)
==5141==by 0x48AFB1F: g_main_dispatch (gmain.c:3066)
==5141==by 0x48AFB1F: g_main_context_dispatch (gmain.c:3642)
==5141==by 0x48AFE23: g_main_context_iterate.part.19 (gmain.c:3713)
==5141==by 0x48B048B: g_main_loop_run (gmain.c:3906)
==5141==by 0x149D3: main (main.c:779)
---
plugins/ofono.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/plugins/ofono.c b/plugins/ofono.c
index 5cd8302..267b3bd 100644
--- a/plugins/ofono.c
+++ b/plugins/ofono.c
@@ -2142,8 +2142,18 @@ static void modem_update_interfaces(struct modem_data
*modem,
if (api_added(old_ifaces, new_ifaces, OFONO_API_CDMA_NETREG))
cdma_netreg_get_properties(modem);
- if (api_removed(old_ifaces, new_ifaces, OFONO_API_CM))
- remove_cm_context(modem, modem-context-path);
+ if (api_removed(old_ifaces, new_ifaces, OFONO_API_CM)) {
+ if (modem-call_get_contexts) {
+ DBG(cancelling pending GetContexts call);
+ dbus_pending_call_cancel(modem-call_get_contexts);
+ dbus_pending_call_unref(modem-call_get_contexts);
+ modem-call_get_contexts = NULL;
+ }
+ if (modem-context) {
+ DBG(removing context %s, modem-context-path);
+ remove_cm_context(modem, modem-context-path);
+ }
+ }
if (api_removed(old_ifaces, new_ifaces, OFONO_API_CDMA_CM))
remove_cm_context(modem, modem-context-path);
--
1.8.3.2
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] ofono: Unref pending dbus calls after cancelling them
dbus_pending_call_cancel() drops the dbus library's internal reference
to DBusPendingCall but we also need to release the reference we got from
dbus_connection_send_with_reply()
---
plugins/ofono.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/plugins/ofono.c b/plugins/ofono.c
index 5cd8302..eda6ad8 100644
--- a/plugins/ofono.c
+++ b/plugins/ofono.c
@@ -401,8 +401,8 @@ static int set_property(struct modem_data *modem,
if (modem-call_set_property) {
DBG(Cancel pending SetProperty);
-
dbus_pending_call_cancel(modem-call_set_property);
+ dbus_pending_call_unref(modem-call_set_property);
modem-call_set_property = NULL;
}
@@ -2340,14 +2340,20 @@ static void remove_modem(gpointer data)
DBG(%s, modem-path);
- if (modem-call_set_property)
+ if (modem-call_set_property) {
dbus_pending_call_cancel(modem-call_set_property);
+ dbus_pending_call_unref(modem-call_set_property);
+ }
- if (modem-call_get_properties)
+ if (modem-call_get_properties) {
dbus_pending_call_cancel(modem-call_get_properties);
+ dbus_pending_call_unref(modem-call_get_properties);
+ }
- if (modem-call_get_contexts)
+ if (modem-call_get_contexts) {
dbus_pending_call_cancel(modem-call_get_contexts);
+ dbus_pending_call_unref(modem-call_get_contexts);
+ }
if (modem-device)
destroy_device(modem);
--
1.8.3.2
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] device: Add configurable DontBringDownAtStartup list
On 27/04/15 08:54, Patrik Flykt wrote: On Fri, 2015-04-24 at 16:53 +0300, Slava Monich wrote: connman brings down managed interfaces at startup. Sometimes it's unnecessary or even harmful. DontBringDownAtStartup list in main.conf allows to make exceptions for some interfaces. Taking the interfaces down was the easiest way to clean up any left-over routing and IP addressing information from the time before ConnMan starts. The real issue here is what to do with existing interface IP and routing configuration present before ConnMan starts. Should those be left in place or removed and re-installed with the information ConnMan is configured with? In the latter case, we most likely break NFS, etc. anyway if we touch IP addressing and routing information? There is always the NetworkInterfaceBlacklist for interfaces that need to be left alone. This in general, any comments on removing pre-existing interface information? Particular for this patch, which issue was being fixed here? This resolves the conflict between developer mode (handled by usb-moded) and USB tethering (handled by connman) on Jolla phone. When phone boots up with USB connected, usb-moded handles it and turns on the developer mode. connman interferes with that by bringing interface down. If that happens, USB cable has to be physically reconnected to re-enable developer mode, which breaks automated testing and is generally inconvenient. We need connman to leave rndis0 alone until it's asked to turn USB tethering on. Regards, -Slava Cheers, Patrik ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH] device: Add configurable DontBringDownAtStartup list
connman brings down managed interfaces at startup. Sometimes it's unnecessary
or even harmful. DontBringDownAtStartup list in main.conf allows to make
exceptions for some interfaces.
---
src/device.c | 21 +
src/main.c | 17 +
2 files changed, 38 insertions(+)
diff --git a/src/device.c b/src/device.c
index c0683ab..03eac39 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1367,6 +1367,24 @@ list:
return false;
}
+static bool can_bring_down(const char *devname)
+{
+ char **pattern =
+ connman_setting_get_string_list(DontBringDownAtStartup);
+
+ if (pattern) {
+ while (*pattern) {
+ if (g_str_has_prefix(devname, *pattern++)) {
+ DBG(%s no, devname);
+ return false;
+ }
+ }
+ }
+
+ DBG(%s yes, devname);
+ return true;
+}
+
static void cleanup_devices(void)
{
/*
@@ -1399,6 +1417,9 @@ static void cleanup_devices(void)
if (filtered)
continue;
+ if (!can_bring_down(interfaces[i]))
+ continue;
+
index = connman_inet_ifindex(interfaces[i]);
if (index 0)
continue;
diff --git a/src/main.c b/src/main.c
index 1c17991..52209f4 100644
--- a/src/main.c
+++ b/src/main.c
@@ -73,6 +73,7 @@ static struct {
bool single_tech;
char **tethering_technologies;
bool persistent_tethering_mode;
+ char **dont_bring_down_at_startup;
} connman_settings = {
.bg_scan = true,
.pref_timeservers = NULL,
@@ -86,6 +87,7 @@ static struct {
.single_tech = false,
.tethering_technologies = NULL,
.persistent_tethering_mode = false,
+ .dont_bring_down_at_startup = NULL,
};
#define CONF_BG_SCANBackgroundScanning
@@ -100,6 +102,7 @@ static struct {
#define CONF_SINGLE_TECHSingleConnectedTechnology
#define CONF_TETHERING_TECHNOLOGIES TetheringTechnologies
#define CONF_PERSISTENT_TETHERING_MODE PersistentTetheringMode
+#define CONF_DONT_BRING_DOWN_AT_STARTUP DontBringDownAtStartup
static const char *supported_options[] = {
CONF_BG_SCAN,
@@ -114,6 +117,7 @@ static const char *supported_options[] = {
CONF_SINGLE_TECH,
CONF_TETHERING_TECHNOLOGIES,
CONF_PERSISTENT_TETHERING_MODE,
+ CONF_DONT_BRING_DOWN_AT_STARTUP,
NULL
};
@@ -236,6 +240,7 @@ static void parse_config(GKeyFile *config)
char **interfaces;
char **str_list;
char **tethering;
+ char **dontbringdown;
gsize len;
int timeout;
@@ -347,6 +352,14 @@ static void parse_config(GKeyFile *config)
g_clear_error(error);
+ dontbringdown = __connman_config_get_string_list(config, General,
+ CONF_DONT_BRING_DOWN_AT_STARTUP, len, error);
+
+ if (!error)
+ connman_settings.dont_bring_down_at_startup = dontbringdown;
+
+ g_clear_error(error);
+
boolean = __connman_config_get_bool(config, General,
CONF_PERSISTENT_TETHERING_MODE,
error);
@@ -545,6 +558,9 @@ char **connman_setting_get_string_list(const char *key)
if (g_str_equal(key, CONF_TETHERING_TECHNOLOGIES))
return connman_settings.tethering_technologies;
+ if (g_str_equal(key, CONF_DONT_BRING_DOWN_AT_STARTUP))
+ return connman_settings.dont_bring_down_at_startup;
+
return NULL;
}
@@ -747,6 +763,7 @@ int main(int argc, char *argv[])
g_strfreev(connman_settings.fallback_nameservers);
g_strfreev(connman_settings.blacklisted_interfaces);
g_strfreev(connman_settings.tethering_technologies);
+ g_strfreev(connman_settings.dont_bring_down_at_startup);
g_free(option_debug);
g_free(option_wifi);
--
1.8.3.2
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
supplicant.c memory leak etc.
The memory leak shows up in valgrind like this: ==13796== 5,544 (1,848 direct, 3,696 indirect) bytes in 22 blocks are definitely lost in loss record 219 of 222 ==13796==at 0x483933C: malloc (vg_replace_malloc.c:296) ==13796==by 0x48B778F: g_try_malloc0 (gmem.c:280) ==13796==by 0x25C0B: interface_network_added (supplicant.c:1157) ==13796==by 0x260EF: interface_property (supplicant.c:1906) ==13796==by 0x286FF: supplicant_dbus_property_foreach (dbus.c:145) ==13796==by 0x2245F: g_supplicant_filter (supplicant.c:2636) ==13796==by 0x497AF4F: dbus_connection_dispatch (in /usr/lib/libdbus-1.so.3.7.12) ==13796==by 0x8164F: message_dispatch (mainloop.c:72) ==13796==by 0x48ABA8B: g_idle_dispatch (gmain.c:5251) ==13796==by 0x48AFB1F: g_main_dispatch (gmain.c:3066) ==13796==by 0x48AFB1F: g_main_context_dispatch (gmain.c:3642) ==13796==by 0x48AFE23: g_main_context_iterate.part.19 (gmain.c:3713) ==13796==by 0x48B048B: g_main_loop_run (gmain.c:3906) ==13796==by 0x149D3: main (main.c:761) I can make this leak go away by deleting a few hundred lines of code from supplicant.c, specifically the following functions: 1. merge_network 2. network_property 3. interface_network_added 4. interface_network_removed 5. signal_network_added 6. signal_network_removed No functionality seem to be affected. Additionally, it appears that GSupplicantInterface::net_mapping table is quite useless. I couldn't figure out how anything could possibly get into that table. Am I missing something? Or should I submit a patch removing the above mentioned stuff? Regards, -Slava ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] dnsproxy: request_data in request_list need the request data
On 07/04/15 12:14, Patrik Flykt wrote: Hi, On Thu, 2015-04-02 at 21:36 +0300, Slava Monich wrote: 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); To me it seems only TCP is using req-name, so not all code paths need to set it. What issues did you notice before this patch? |==7180== Syscall param socketcall.sendto(msg) points to unaddressable byte(s) ==7180==at 0x4B9CE34: sendto (in /lib/libc-2.15.so) ==7180==by 0x79283: ns_resolv (dnsproxy.c:1644) ==7180==by 0x7977F: resolv (dnsproxy.c:2648) ==7180==by 0x7C80F: __connman_dnsproxy_flush (dnsproxy.c:2770) ==7180==by 0x47C37: update_nameservers (service.c:1158) ==7180==by 0x47F27: __connman_service_nameserver_remove (service.c:1275) ==7180==by 0x5D87B: dhcp_invalidate (dhcp.c:133) ==7180==by 0x5E677: __connman_dhcp_stop (dhcp.c:641) ==7180==by 0x3F9CB: set_disconnected (network.c:746) ==7180==by 0x4079B: connman_network_set_connected (network.c:1465) ==7180==by 0x21563: interface_state (wifi.c:1824) ==7180==by 0x2648B: callback_interface_state (supplicant.c:377) ==7180==by 0x2648B: interface_property (supplicant.c:1854) ==7180==by 0x28DBF: supplicant_dbus_property_foreach (dbus.c:145) ==7180==by 0x22B7F: g_supplicant_filter (supplicant.c:2636) ==7180==by 0x497AF4F: dbus_connection_dispatch (in /usr/lib/libdbus-1.so.3.7.12) ==7180==by 0x81C57: message_dispatch (mainloop.c:72) ==7180==by 0x48ABA8B: g_idle_dispatch (gmain.c:5251) ==7180==by 0x48AFB1F: g_main_dispatch (gmain.c:3066) ==7180==by 0x48AFB1F: g_main_context_dispatch (gmain.c:3642) ==7180==by 0x48AFE23: g_main_context_iterate.part.19 (gmain.c:3713) ==7180==by 0x48B048B: g_main_loop_run (gmain.c:3906) ==7180==by 0x149D3: main (main.c:761) ==7180== Address 0x0 is not stack'd, malloc'd or (recently) free'd| ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] service: remove_from_network should drop reference to the network
On 07/04/15 12:42, Patrik Flykt wrote:
@@ -6863,6 +6868,11 @@ void __connman_service_remove_from_network(struct
connman_network *network)
__connman_connection_gateway_remove(service,
CONNMAN_IPCONFIG_TYPE_ALL);
+if (service-network) {
+connman_network_unref(service-network);
+service-network = NULL;
+}
+
connman_service_unref(service);
}
No network, no service. The network unref is done with
g_hash_table_remove() in connman_service_unref().
Again Jolla's additional patches break this assumption as you may have
one more reference for the service not to loose a network-less one from
the list, right?
The fewer assumptions the better.
The code which calls unref() can't make the assumption that it's the
last reference and the object gets deallocated. Otherwise why would you
use refcount in the first place if you know exactly when to deallocate
the object, right?
I'm reasonably sure that in Jolla code the additional reference is held
by the wispr context. Otherwise it would crash there.
-Slava
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] dnsproxy: request_data in request_list need the request data
On 07/04/15 13:40, Jukka Rissanen wrote: On ti, 2015-04-07 at 12:58 +0300, Slava Monich wrote: On 07/04/15 12:14, Patrik Flykt wrote: Hi, On Thu, 2015-04-02 at 21:36 +0300, Slava Monich wrote: 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); To me it seems only TCP is using req-name, so not all code paths need to set it. What issues did you notice before this patch? |==7180== Syscall param socketcall.sendto(msg) points to unaddressable byte(s) ==7180==at 0x4B9CE34: sendto (in /lib/libc-2.15.so) ==7180==by 0x79283: ns_resolv (dnsproxy.c:1644) ==7180==by 0x7977F: resolv (dnsproxy.c:2648) ==7180==by 0x7C80F: __connman_dnsproxy_flush (dnsproxy.c:2770) ==7180==by 0x47C37: update_nameservers (service.c:1158) ==7180==by 0x47F27: __connman_service_nameserver_remove (service.c:1275) ==7180==by 0x5D87B: dhcp_invalidate (dhcp.c:133) ==7180==by 0x5E677: __connman_dhcp_stop (dhcp.c:641) ==7180==by 0x3F9CB: set_disconnected (network.c:746) ==7180==by 0x4079B: connman_network_set_connected (network.c:1465) ==7180==by 0x21563: interface_state (wifi.c:1824) ==7180==by 0x2648B: callback_interface_state (supplicant.c:377) ==7180==by 0x2648B: interface_property (supplicant.c:1854) ==7180==by 0x28DBF: supplicant_dbus_property_foreach (dbus.c:145) ==7180==by 0x22B7F: g_supplicant_filter (supplicant.c:2636) ==7180==by 0x497AF4F: dbus_connection_dispatch (in /usr/lib/libdbus-1.so.3.7.12) ==7180==by 0x81C57: message_dispatch (mainloop.c:72) ==7180==by 0x48ABA8B: g_idle_dispatch (gmain.c:5251) ==7180==by 0x48AFB1F: g_main_dispatch (gmain.c:3066) ==7180==by 0x48AFB1F: g_main_context_dispatch (gmain.c:3642) ==7180==by 0x48AFE23: g_main_context_iterate.part.19 (gmain.c:3713) ==7180==by 0x48B048B: g_main_loop_run (gmain.c:3906) ==7180==by 0x149D3: main (main.c:761) ==7180== Address 0x0 is not stack'd, malloc'd or (recently) free'd| FYI, something like this trace would be really nice to see in the commit message. OK, will do next time around. I have valgrind/gdb backtraces for most patches I send to the list, I just wasn't sure if it would be appropriate here. Here is one for the agent.c patch I sent a few days ago: https://github.com/mer-packages/connman/pull/210 Regards, -Slava ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH] ipv6pd: Check timer_hash for NULL prior to destroying it
GLib doesn't like NULL pointers.
cleanup() may be invoked more than once after successful setup,
first from dhcpv6_callback() and then from __connman_ipv6pd_cleanup()
---
src/ipv6pd.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/ipv6pd.c b/src/ipv6pd.c
index 5ecda38..0d221f0 100644
--- a/src/ipv6pd.c
+++ b/src/ipv6pd.c
@@ -165,8 +165,10 @@ static void cleanup(void)
timer_uplink = 0;
}
- g_hash_table_destroy(timer_hash);
- timer_hash = NULL;
+ if (timer_hash) {
+ g_hash_table_destroy(timer_hash);
+ timer_hash = NULL;
+ }
if (prefixes) {
g_slist_free_full(prefixes, g_free);
--
1.8.3.2
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] service: remove_from_network should drop reference to the network
Otherwise the service may remain associated with a dead network.
In addition to that, update_from_network() now checks that session
is associated with the correct network. Without this check, association
with a dead network in the connecting state was unrecoverable.
---
src/service.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/src/service.c b/src/service.c
index 7538bdd..6d3414d 100644
--- a/src/service.c
+++ b/src/service.c
@@ -6616,6 +6616,11 @@ static void update_from_network(struct connman_service
*service,
if (is_connected(service))
return;
+ if (service-network service-network != network) {
+ connman_network_unref(service-network);
+ service-network = NULL;
+ }
+
if (is_connecting(service))
return;
@@ -6863,6 +6868,11 @@ void __connman_service_remove_from_network(struct
connman_network *network)
__connman_connection_gateway_remove(service,
CONNMAN_IPCONFIG_TYPE_ALL);
+ if (service-network) {
+ connman_network_unref(service-network);
+ service-network = NULL;
+ }
+
connman_service_unref(service);
}
--
1.8.3.2
___
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] 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
[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
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
[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
[PATCH] network: fixed memory leak in check_dhcpv6
---
src/network.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/network.c b/src/network.c
index f61f698..badb770 100644
--- a/src/network.c
+++ b/src/network.c
@@ -471,10 +471,12 @@ static void check_dhcpv6(struct nd_router_advert *reply,
*/
if (reply-nd_ra_flags_reserved ND_RA_FLAG_MANAGED) {
__connman_dhcpv6_start(network, prefixes, dhcpv6_callback);
- } else if (reply-nd_ra_flags_reserved ND_RA_FLAG_OTHER) {
- __connman_dhcpv6_start_info(network, dhcpv6_info_callback);
- network-connecting = false;
} else {
+ if (reply-nd_ra_flags_reserved ND_RA_FLAG_OTHER)
+ __connman_dhcpv6_start_info(network,
+ dhcpv6_info_callback);
+
+ g_slist_free_full(prefixes, g_free);
network-connecting = false;
}
--
1.8.3.2
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] inet: fixed the check of inet_pton return value
---
src/inet.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/inet.c b/src/inet.c
index cd220ff..fae36a0 100644
--- a/src/inet.c
+++ b/src/inet.c
@@ -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);
- }
rt.rtmsg_metric = 1;
rt.rtmsg_ifindex = index;
@@ -686,7 +684,7 @@ int connman_inet_clear_ipv6_gateway_address(int index,
const char *gateway)
memset(rt, 0, sizeof(rt));
- if (inet_pton(AF_INET6, gateway, rt.rtmsg_gateway) 0) {
+ if (inet_pton(AF_INET6, gateway, rt.rtmsg_gateway) 1) {
err = -errno;
goto out;
}
--
1.8.3.2
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] dhcpv6: fixed the check of inet_pton return value
---
src/dhcpv6.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/dhcpv6.c b/src/dhcpv6.c
index bdb3b98..dcf91f6 100644
--- a/src/dhcpv6.c
+++ b/src/dhcpv6.c
@@ -950,7 +950,7 @@ static void do_dad(GDHCPClient *dhcp_client, struct
connman_dhcpv6 *dhcp)
ref_own_address(user_data);
- if (inet_pton(AF_INET6, address, addr) 0) {
+ if (inet_pton(AF_INET6, address, addr) 1) {
DBG(Invalid IPv6 address %s %d/%s, address,
-errno, strerror(errno));
goto fail;
--
1.8.3.2
___
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
[PATCH] dnsproxy: Don't try to destroy NULL hashtable on exit
glib doesn't like it.
---
src/dnsproxy.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 9d7ba61..9787b68 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -3872,8 +3872,10 @@ void __connman_dnsproxy_cleanup(void)
cache_timer = 0;
}
- g_hash_table_destroy(cache);
- cache = NULL;
+ if (cache) {
+ g_hash_table_destroy(cache);
+ cache = NULL;
+ }
connman_notifier_unregister(dnsproxy_notifier);
--
1.8.3.2
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] Hold a reference to the service while disconnecting
There are places where __connman_service_disconnect is called by the code which isn't holding its own reference to connman_service. Here is an example (line numbers may not exactly match upstream): ==5339== Invalid write of size 4 ==5339== at 0x70C9C: __connman_service_ipconfig_indicate_state (service.c:6131) ==5339== by 0x5BC0B: set_disconnected (network.c:791) ==5339== by 0x5D64B: __connman_network_disconnect (network.c:1616) ==5339== by 0x7191F: __connman_service_disconnect (service.c:6480) ==5339== by 0x57DAB: __connman_device_disable (device.c:247) ... ==5339== Address 0x4e25264 is 212 bytes inside a block of size 240 free'd ==5339== at 0x483752C: free (vg_replace_malloc.c:446) ==5339== by 0x48B56AB: g_free (gmem.c:197) ==5339== by 0x6E273: service_destroy (service.c:4894) ==5339== by 0x6E34B: service_free (service.c:4921) ==5339== by 0x48971E7: g_hash_table_remove_node (ghash.c:448) ==5339== by 0x48979D3: g_hash_table_remove_internal (ghash.c:1276) ==5339== by 0x6E77B: connman_service_unref_debug (service.c:5040) ==5339== by 0x605BF: remove_gateway (connection.c:707) ==5339== by 0x48971E7: g_hash_table_remove_node (ghash.c:448) ==5339== by 0x48979D3: g_hash_table_remove_internal (ghash.c:1276) ==5339== by 0x61197: __connman_connection_gateway_remove (connection.c:1001) ==5339== by 0x5BBCF: set_disconnected (network.c:773) ==5339== by 0x5D64B: __connman_network_disconnect (network.c:1616) ==5339== by 0x7191F: __connman_service_disconnect (service.c:6480) ==5339== by 0x57DAB: __connman_device_disable (device.c:247) ... Slava Monich (1): service: Hold a reference to the service while disconnecting src/service.c | 16 ++-- 1 file changed, 14 insertions(+), 2 deletions(-) -- 1.8.3.2 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH] service: Hold a reference to the service while disconnecting
Otherwise it could be deallocated by __connman_network_disconnect
meaning that all subsequent references to it will access freed memory
---
src/service.c | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/src/service.c b/src/service.c
index 9bba227..7e6cd7a 100644
--- a/src/service.c
+++ b/src/service.c
@@ -127,6 +127,7 @@ struct connman_service {
};
static bool allow_property_changed(struct connman_service *service);
+static int service_disconnect(struct connman_service *service);
static struct connman_ipconfig *create_ip4config(struct connman_service
*service,
int index, enum connman_ipconfig_method method);
@@ -4616,7 +4617,7 @@ void connman_service_unref_debug(struct connman_service
*service,
service_list = g_list_remove(service_list, service);
- __connman_service_disconnect(service);
+ service_disconnect(service);
g_hash_table_remove(service_hash, service-identifier);
}
@@ -6020,7 +6021,7 @@ int __connman_service_connect(struct connman_service
*service,
return err;
}
-int __connman_service_disconnect(struct connman_service *service)
+static int service_disconnect(struct connman_service *service)
{
int err;
@@ -6067,6 +6068,17 @@ int __connman_service_disconnect(struct connman_service
*service)
return err;
}
+int __connman_service_disconnect(struct connman_service *service)
+{
+ int err;
+
+ connman_service_ref(service);
+ err = service_disconnect(service);
+ connman_service_unref(service);
+
+ return err;
+}
+
int __connman_service_disconnect_all(void)
{
struct connman_service *service;
--
1.8.3.2
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH 1/2] plugins: Add logcontrol plugin
This plugin implements net.connman.DebugLog D-Bus interface which allows changing debug log settings at runtime without restarting connman. --- Makefile.plugins | 5 ++ configure.ac | 5 ++ plugins/logcontrol.c | 198 +++ 3 files changed, 208 insertions(+) create mode 100644 plugins/logcontrol.c It was just noticed that --enable-logcontrol description is wrong, enable jolla GPS support needs to be replaced with something that makes more sense. I can resubmit the patch if necessary. Regards, -Slava ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH 1/2] plugins: Add logcontrol plugin
This plugin implements net.connman.DebugLog D-Bus interface which
allows changing debug log settings at runtime without restarting
connman.
---
Makefile.plugins | 5 ++
configure.ac | 5 ++
plugins/logcontrol.c | 198 +++
3 files changed, 208 insertions(+)
create mode 100644 plugins/logcontrol.c
diff --git a/Makefile.plugins b/Makefile.plugins
index e90ad19..a1f6f95 100644
--- a/Makefile.plugins
+++ b/Makefile.plugins
@@ -55,6 +55,11 @@ builtin_modules += dundee
builtin_sources += plugins/dundee.c
endif
+if LOGCONTROL
+builtin_modules += logcontrol
+builtin_sources += plugins/logcontrol.c
+endif
+
if VPN
builtin_modules += vpn
builtin_sources += plugins/vpn.c
diff --git a/configure.ac b/configure.ac
index e3326ad..bc2b47d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -326,6 +326,11 @@ AC_ARG_ENABLE(wispr, AC_HELP_STRING([--disable-wispr],
[enable_wispr=${enableval}])
AM_CONDITIONAL(WISPR, test ${enable_wispr} != no)
+AC_ARG_ENABLE(logcontrol,
+ AC_HELP_STRING([--enable-logcontrol], [enable jolla GPS support]),
+ [enable_logcontrol=${enableval}],
[enable_logcontrol=no])
+AM_CONDITIONAL(LOGCONTROL, test ${enable_logcontrol} != no)
+
AC_ARG_ENABLE(tools, AC_HELP_STRING([--disable-tools],
[disable testing tools]),
[enable_tools=${enableval}])
diff --git a/plugins/logcontrol.c b/plugins/logcontrol.c
new file mode 100644
index 000..203fe7b
--- /dev/null
+++ b/plugins/logcontrol.c
@@ -0,0 +1,198 @@
+/*
+ *
+ * Connection Manager
+ *
+ * Copyright (C) 2015 Jolla Ltd. All rights reserved.
+ * Contact: Slava Monich slava.mon...@jolla.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include config.h
+#endif
+
+#include gdbus.h
+
+#define CONNMAN_API_SUBJECT_TO_CHANGE
+#include connman/plugin.h
+#include connman/log.h
+#include connman.h
+
+#define LOG_INTERFACE CONNMAN_SERVICE .DebugLog
+#define LOG_PATH /
+
+static DBusConnection *connection = NULL;
+
+extern struct connman_debug_desc __start___debug[];
+extern struct connman_debug_desc __stop___debug[];
+
+static void logcontrol_update(const char* pattern, unsigned int set_flags,
+ unsigned int clear_flags)
+{
+ struct connman_debug_desc *start = __start___debug;
+ struct connman_debug_desc *stop = __stop___debug;
+ struct connman_debug_desc *desc;
+ const char *alias = NULL, *file = NULL;
+
+ if (!start || !stop)
+ return;
+
+ for (desc = start; desc stop; desc++) {
+ const char* name;
+
+ if (desc-flags CONNMAN_DEBUG_FLAG_ALIAS) {
+ file = desc-file;
+ alias = desc-name;
+ continue;
+ }
+
+ if (file g_strcmp0(desc-file, file)) {
+ file = NULL;
+ alias = NULL;
+ }
+
+ name = desc-name ? desc-name : alias;
+ if ((name g_pattern_match_simple(pattern, name)) ||
+ (desc-file g_pattern_match_simple(pattern,
+ desc-file))) {
+ desc-flags |= set_flags;
+ desc-flags = ~clear_flags;
+ }
+ }
+}
+
+static DBusMessage *logcontrol_dbusmsg(DBusConnection *conn, DBusMessage *msg,
+ unsigned int set_flags, unsigned int clear_flags)
+{
+ const char *pattern;
+
+ if (dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, pattern,
+ DBUS_TYPE_INVALID)) {
+ logcontrol_update(pattern, set_flags, clear_flags);
+ return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
+ } else {
+ return __connman_error_invalid_arguments(msg);
+ }
+}
+
+static DBusMessage *logcontrol_enable(DBusConnection *conn,
+ DBusMessage *msg, void *data)
+{
+ return logcontrol_dbusmsg(conn, msg, CONNMAN_DEBUG_FLAG_PRINT, 0);
+}
+
+static DBusMessage *logcontrol_disable(DBusConnection *conn
[PATCH 2/2] doc: Add net.connman.DebugLog API documentation
---
doc/logcontrol-api.txt | 22 ++
1 file changed, 22 insertions(+)
create mode 100644 doc/logcontrol-api.txt
diff --git a/doc/logcontrol-api.txt b/doc/logcontrol-api.txt
new file mode 100644
index 000..c9fdae3
--- /dev/null
+++ b/doc/logcontrol-api.txt
@@ -0,0 +1,22 @@
+Debug log control
+=
+
+Servicenet.connman
+Interface net.connman.DebugLog
+Object path/
+
+Methodsvoid Enable(string pattern)
+
+ Enables all logs that match the pattern.
+
+ void Disable(string pattern)
+
+ Disables all logs that match the pattern.
+
+ array{string} List()
+
+ Returns all available log names and aliases.
+
+ In order for Enable or Disable call to have any
+ effect, the pattern must match one or more of
+ these strings.
--
1.8.3.2
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH 0/2] Debug log control API
These patches introduce net.connman.DebugLog D-Bus interface
which provides the following methods:
void Enable(string pattern)
void Disable(string pattern)
array{string} List()
This interface allows changing debug log settings at runtime
without restarting connman. It's useful in the situations when
connman breaks in the production environment, where logs are
not enabled at startup.
By default this plugin is not compiled in. It needs to be
explicitely enabled with --enable-logcontrol configure switch.
Slava Monich (2):
plugins: Add logcontrol plugin
doc: Add net.connman.DebugLog API documentation
Makefile.plugins | 5 ++
configure.ac | 5 ++
doc/logcontrol-api.txt | 22 ++
plugins/logcontrol.c | 198 +
4 files changed, 230 insertions(+)
create mode 100644 doc/logcontrol-api.txt
create mode 100644 plugins/logcontrol.c
--
1.8.3.2
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] ofono: Fix memory leak
Free connman_ipaddress prior to allocating the new one. --- plugins/ofono.c | 8 1 file changed, 8 insertions(+) diff --git a/plugins/ofono.c b/plugins/ofono.c index 7af551b..7a8442b 100644 --- a/plugins/ofono.c +++ b/plugins/ofono.c @@ -755,6 +755,10 @@ static void extract_ipv4_settings(DBusMessageIter *array, const char *interface = NULL; int index = -1; + connman_ipaddress_free(context-ipv4_address); + context-ipv4_address = NULL; + context-index = -1; + if (dbus_message_iter_get_arg_type(array) != DBUS_TYPE_ARRAY) return; @@ -854,6 +858,10 @@ static void extract_ipv6_settings(DBusMessageIter *array, const char *interface = NULL; int index = -1; + connman_ipaddress_free(context-ipv6_address); + context-ipv6_address = NULL; + context-index = -1; + if (dbus_message_iter_get_arg_type(array) != DBUS_TYPE_ARRAY) return; -- 1.8.3.2 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH] gweb: Don't close socket descriptor handed over to GIOChannel
Otherwise the channel will close it again when being deallocated.
---
gweb/gweb.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/gweb/gweb.c b/gweb/gweb.c
index f6828cf..dcb0549 100644
--- a/gweb/gweb.c
+++ b/gweb/gweb.c
@@ -1075,7 +1075,8 @@ static int connect_session_transport(struct web_session
*session)
session-addr-ai_addrlen) 0) {
if (errno != EINPROGRESS) {
debug(session-web, connect() %s, strerror(errno));
- close(sk);
+ g_io_channel_unref(session-transport_channel);
+ session-transport_channel = NULL;
return -EIO;
}
}
--
1.8.3.2
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] client: Detach threads since they are never joined
On 01/09/14 13:16, Patrik Flykt wrote: On Sat, 2014-08-30 at 15:15 +0300, Slava Monich wrote: This is for pacrunner. More motivation in here why this patch is good, please. Cheers, Patrik A thread may either be/joinable/ or/detached/. If a thread is joinable, then another thread can callpthread_join(3) http://man7.org/linux/man-pages/man3/pthread_join.3.html to wait for the thread to terminate and fetch its exit status. Only when a terminated joinable thread has been joined are the last of its resources released back to the system. When a detached thread terminates, its resources are automatically released back to the system: it is not possible to join with the thread in order to obtain its exit status. Making a thread detached is useful for some types of daemon threads whose exit status the application does not need to care about. By default, a new thread is created in a joinable state, unless/attr/ was set to create the thread in a detached state (using pthread_attr_setdetachstate(3) http://man7.org/linux/man-pages/man3/pthread_attr_setdetachstate.3.html). And here is a piece of evidence from valgrind: ==14392== HEAP SUMMARY: ==14392== in use at exit: 99,659 bytes in 3,563 blocks ==14392== total heap usage: 11,478 allocs, 7,915 frees, 8,096,821 bytes allocated ==14392== ==14392== 31,104 bytes in 216 blocks are possibly lost in loss record 709 of 709 ==14392== at 0x483643C: calloc (vg_replace_malloc.c:593) ==14392== by 0x4013B37: _dl_allocate_tls (dl-tls.c:297) ==14392== by 0x49C0947: pthread_create@@GLIBC_2.4 (allocatestack.c:571) ==14392== by 0x1418F: find_proxy_for_url (client.c:97) ==14392== by 0x107CB: process_message (object.c:259) ==14392== by 0x49F58A3: _dbus_object_tree_dispatch_and_unlock (dbus-object-tree.c:862) ==14392== by 0x49E5F9B: dbus_connection_dispatch (dbus-connection.c:4672) ==14392== by 0xD66B: message_dispatch (mainloop.c:72) ==14392== by 0x48F7A8B: g_idle_dispatch (gmain.c:5251) ==14392== by 0x48FBB1F: g_main_context_dispatch (gmain.c:3066) ==14392== by 0x48FBE23: g_main_context_iterate.part.19 (gmain.c:3713) ==14392== by 0x48FC48B: g_main_loop_run (gmain.c:3906) ==14392== ==14392== LEAK SUMMARY: ==14392== definitely lost: 0 bytes in 0 blocks ==14392== indirectly lost: 0 bytes in 0 blocks ==14392== possibly lost: 31,104 bytes in 216 blocks ==14392== still reachable: 68,555 bytes in 3,347 blocks ==14392== suppressed: 0 bytes in 0 blocks And I'm pretty sure it was leaking some kernel resources as well. Regards, -Slava ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH] client: Detach threads since they are never joined
This is for pacrunner.
---
src/client.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/client.c b/src/client.c
index d354c2b..8d1e22b 100644
--- a/src/client.c
+++ b/src/client.c
@@ -84,6 +84,7 @@ static DBusMessage *find_proxy_for_url(DBusConnection *conn,
DBusMessage *msg, void *user_data)
{
struct jsrun_data *jsrun;
+ pthread_attr_t attrs;
jsrun = g_try_new0(struct jsrun_data, 1);
if (!jsrun)
@@ -94,7 +95,9 @@ static DBusMessage *find_proxy_for_url(DBusConnection *conn,
jsrun-conn = dbus_connection_ref(conn);
jsrun-msg = dbus_message_ref(msg);
- if (pthread_create(jsrun-thread, NULL, jsrun_thread, jsrun) != 0) {
+ pthread_attr_init(attrs);
+ pthread_attr_setdetachstate(attrs, PTHREAD_CREATE_DETACHED);
+ if (pthread_create(jsrun-thread, attrs, jsrun_thread, jsrun) != 0) {
jsrun_free(jsrun);
return g_dbus_create_error(msg,
PACRUNNER_ERROR_INTERFACE .Failed,
--
1.8.3.2
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] service: cancel agent requests prior to deleting wispr context
On 26/06/14 10:13, Patrik Flykt wrote: On Wed, 2014-06-25 at 18:56 +0300, Slava Monich wrote: Calling __connman_wispr_stop() without connman_agent_cancel() allows pending wispr requests to complete later which results in a read/write access to the freed memory and a subsequent crash. Which freed memory is accessed here? connmand[3388]: src/agent.c:agent_receive_message() agent 0x51129e0 req 0x5018d50 connmand[3388]: src/wispr.c:wispr_portal_browser_reply_cb() connmand[3388]: src/wispr.c:wispr_portal_error() Failed to proceed wispr/portal web request ==3388== Invalid write of size 4 ==3388==at 0xAB2C4: wispr_portal_error (wispr.c:422) ==3388==by 0xAB88B: wispr_portal_browser_reply_cb (wispr.c:562) ==3388==by 0x74F63: request_browser_reply (agent-connman.c:587) ==3388==by 0x756EB: agent_finalize_pending (agent.c:121) ==3388==by 0x75A33: agent_receive_message (agent.c:203) ==3388==by 0x4965773: _dbus_pending_call_complete (dbus-pending-call.c:223) ==3388==by 0x4951607: complete_pending_call_and_unlock (dbus-connection.c:2314) ==3388==by 0x4954CA3: dbus_connection_dispatch (dbus-connection.c:4580) ==3388==by 0xC3837: message_dispatch (mainloop.c:72) ==3388==by 0x489EEEF: g_idle_dispatch (gmain.c:5251) ==3388==by 0x48A1EAB: g_main_context_dispatch (gmain.c:3066) ==3388==by 0x48A2067: g_main_context_iterate.part.8 (gmain.c:3713) ==3388==by 0x48A2683: g_main_loop_run (gmain.c:3680) ==3388==by 0x52313: main (main.c:739) ==3388== Address 0x4ef7ff0 is 344 bytes inside a block of size 376 free'd ==3388==at 0x4837698: free (vg_replace_malloc.c:468) ==3388==by 0x4AE2D4F: fclose (in /lib/libc-2.15.so) ==3388== ==3388== Invalid read of size 4 ==3388==at 0xAA964: free_wispr_routes (wispr.c:127) ==3388==by 0xAB893: wispr_portal_browser_reply_cb (wispr.c:563) ==3388==by 0x74F63: request_browser_reply (agent-connman.c:587) ==3388==by 0x756EB: agent_finalize_pending (agent.c:121) ==3388==by 0x75A33: agent_receive_message (agent.c:203) ==3388==by 0x4965773: _dbus_pending_call_complete (dbus-pending-call.c:223) ==3388==by 0x4951607: complete_pending_call_and_unlock (dbus-connection.c:2314) ==3388==by 0x4954CA3: dbus_connection_dispatch (dbus-connection.c:4580) ==3388==by 0xC3837: message_dispatch (mainloop.c:72) ==3388==by 0x489EEEF: g_idle_dispatch (gmain.c:5251) ==3388==by 0x48A1EAB: g_main_context_dispatch (gmain.c:3066) ==3388==by 0x48A2067: g_main_context_iterate.part.8 (gmain.c:3713) ==3388==by 0x48A2683: g_main_loop_run (gmain.c:3680) ==3388==by 0x52313: main (main.c:739) ==3388== Address 0x4ef7ff4 is 348 bytes inside a block of size 376 free'd ==3388==at 0x4837698: free (vg_replace_malloc.c:468) ==3388==by 0x4AE2D4F: fclose (in /lib/libc-2.15.so) and so on. Eventually ==3388== Process terminating with default action of signal 11 (SIGSEGV): dumping core ==3388== Access not within mapped region at address 0x2E726567 ==3388==at 0xAA858: free_wispr_routes (wispr.c:128) ==3388==by 0xAB893: wispr_portal_browser_reply_cb (wispr.c:563) ==3388==by 0x74F63: request_browser_reply (agent-connman.c:587) ==3388==by 0x756EB: agent_finalize_pending (agent.c:121) ==3388==by 0x75A33: agent_receive_message (agent.c:203) ==3388==by 0x4965773: _dbus_pending_call_complete (dbus-pending-call.c:223) ==3388==by 0x4951607: complete_pending_call_and_unlock (dbus-connection.c:2314) ==3388==by 0x4954CA3: dbus_connection_dispatch (dbus-connection.c:4580) ==3388==by 0xC3837: message_dispatch (mainloop.c:72) ==3388==by 0x489EEEF: g_idle_dispatch (gmain.c:5251) ==3388==by 0x48A1EAB: g_main_context_dispatch (gmain.c:3066) ==3388==by 0x48A2067: g_main_context_iterate.part.8 (gmain.c:3713) ==3388==by 0x48A2683: g_main_loop_run (gmain.c:3680) ==3388==by 0x52313: main (main.c:739) ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] service: cancel agent requests prior to deleting wispr context
On 26/06/14 10:30, Tomasz Bursztyka wrote: Hi, -connman_agent_cancel(service); +__connman_wispr_stop(service); Just adding __connman_wispr_stop() should do the trick here. It could. However, if connman_agent_cancel() must be invoked every time __connman_wispr_stop() is called then it should be impossible to call __connman_wispr_stop() without calling connman_agent_cancel(). Otherwise crashes likes this will keep happening. This one cost me at least a day of work. I don't want to do it again. ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] service: cancel agent requests prior to deleting wispr context
On 26/06/14 11:12, Tomasz Bursztyka wrote: Hi, - connman_agent_cancel(service); +__connman_wispr_stop(service); Just adding __connman_wispr_stop() should do the trick here. It could. However, if connman_agent_cancel() must be invoked every time __connman_wispr_stop() is called then it should be impossible to call __connman_wispr_stop() without calling connman_agent_cancel(). Otherwise crashes likes this will keep happening. This one cost me at least a day of work. I don't want to do it again. You are assuming that agent and wispr are tight together, which is not exactly true. Link is indirect. If the logic is broken, it will be in service.c: that one starts wispr (no matter whatever agent request is running or not), wispr then might start an agent request (browser stuff): still the request is tighten to service. Thus it's always up to service to cancel any agent request. There is not much place where __connman_wispr_stop() is called in service.c And actually, what your patch does is calling it earlier than in service_indicate_state() with state CONNMAN_SERVICE_STATE_DISCONNECT as it is done currently. Which I think you are right here. So a proper patch would be to remove this call in service_indicate_state() and put the one you want in __connman_service_disconnect() Also, it is called in service_free() but not connman_agent_cancel(), which in this case might be another side of the bug. Adding connman_agent_cancel() here would be relevant I guess. Not sure if that can be easily tested though. Verify that but I think it looks like the proper way to do it. Nice catch anyway Tomasz You haven't convinced me. If service doesn't start those requests then it's not its responsibility to cancel them. By doing so it would make an unnecessary assumption about the inner workings of the wispr module. If the wispr implementation changes, this assumption may become invalid. The requests that need to be canceled when wispr context is destroyed, are those started from the wispr code. And therefore it's the wispr code which has to cancel them. The fewer assumptions and inter-dependencies the better. I'm not 100% happy with my solution either. Ideally I would like to be able to cancel individual connection agent requests rather than everything related to the same service. Otherwise destroying wispr context may affect something totally unrelated (like passphrase input). But there doesn't seem to be any API for that. -Slava ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH] service: cancel agent requests prior to deleting wispr context
Calling __connman_wispr_stop() without connman_agent_cancel() allows pending wispr requests to complete later which results in a read/write access to the freed memory and a subsequent crash. Calling connman_agent_cancel() without __connman_wispr_stop() stops the wispr sequence which renders the wispr context useless. That makes no sense either. --- src/service.c | 2 +- src/wispr.c | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/service.c b/src/service.c index cbca669..1df9b57 100644 --- a/src/service.c +++ b/src/service.c @@ -6061,7 +6061,7 @@ int __connman_service_disconnect(struct connman_service *service) service-connect_reason = CONNMAN_SERVICE_CONNECT_REASON_NONE; service-proxy = CONNMAN_SERVICE_PROXY_METHOD_UNKNOWN; - connman_agent_cancel(service); + __connman_wispr_stop(service); reply_pending(service, ECONNABORTED); diff --git a/src/wispr.c b/src/wispr.c index dcce93c..e5a7ddc 100644 --- a/src/wispr.c +++ b/src/wispr.c @@ -29,6 +29,7 @@ #include gweb/gweb.h #include connman.h +#include agent.h #define STATUS_URL_IPV4 http://ipv4.connman.net/online/status.html; #define STATUS_URL_IPV6 http://ipv6.connman.net/online/status.html; @@ -972,6 +973,7 @@ void __connman_wispr_stop(struct connman_service *service) if (index 0) return; + connman_agent_cancel(service); g_hash_table_remove(wispr_portal_list, GINT_TO_POINTER(index)); } -- 1.8.3.2 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH] service: check service-pending for NULL
connect_service() calls __connman_service_connect() which may in turn call
reply_pending() which unrefs service-pending and sets it to NULL. Therefore,
connect_service() needs to check service-pending for NULL prior to calling
dbus_message_unref() on it.
---
src/service.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/service.c b/src/service.c
index cbca669..b0a4eeb 100644
--- a/src/service.c
+++ b/src/service.c
@@ -4022,8 +4022,10 @@ static DBusMessage *connect_service(DBusConnection *conn,
if (err == -EINPROGRESS)
return NULL;
- dbus_message_unref(service-pending);
- service-pending = NULL;
+ if (service-pending) {
+ dbus_message_unref(service-pending);
+ service-pending = NULL;
+ }
if (err 0)
return __connman_error_failed(msg, -err);
--
1.8.3.2
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] service: check service-pending for NULL
Hi,
+++ b/src/service.c
@@ -4022,8 +4022,10 @@ static DBusMessage
*connect_service(DBusConnection *conn,
if (err == -EINPROGRESS)
return NULL;
-dbus_message_unref(service-pending);
-service-pending = NULL;
+if (service-pending) {
+dbus_message_unref(service-pending);
+service-pending = NULL;
+}
if (err 0)
return __connman_error_failed(msg, -err);
What is this fixing?
dbus_message_unref() handles NULL pointers relevantly, so there is no
bug here.
see
http://dbus.freedesktop.org/doc/api/html/dbus-message_8c_source.html#l01689
Tomasz
#0 __GI_raise (sig=6) at raise.c:67
#1 __GI_abort () at abort.c:91
#2 _dbus_abort () at dbus-sysdeps.c:94
#3 _dbus_warn_check_failed (format=0x403492c4 arguments to %s() were
incorrect, assertion \%s\ failed in file %s line %d.\nThis is normally
a bug in some application using the D-Bus library.\n) at
dbus-internals.c:290
#4 dbus_message_unref (message=optimized out) at dbus-message.c:1616
#5 dbus_message_unref (message=0x0) at dbus-message.c:1612
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] log: allow disabling certain logs
Log patterns prefixed with ! disable the logging. That makes it
possible to enable logs everywhere except for the specific files
which don't interest the person doing the debugging.
---
src/log.c | 20 +++-
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/src/log.c b/src/log.c
index a693bd0..18eb363 100644
--- a/src/log.c
+++ b/src/log.c
@@ -246,20 +246,30 @@ static gchar **enabled = NULL;
static bool is_enabled(struct connman_debug_desc *desc)
{
int i;
+ gboolean ret = false;
if (!enabled)
return false;
for (i = 0; enabled[i]; i++) {
- if (desc-name g_pattern_match_simple(enabled[i],
+ const char* pattern = enabled[i];
+ gboolean value = true;
+
+ if (pattern[0] == '!') {
+ pattern++;
+ value = false;
+ }
+
+ if (desc-name g_pattern_match_simple(pattern,
desc-name))
- return true;
- if (desc-file g_pattern_match_simple(enabled[i],
+ ret = value;
+ if (desc-file g_pattern_match_simple(pattern,
desc-file))
- return true;
+ ret = value;
}
- return false;
+ /* Return the last seen value */
+ return ret;
}
void __connman_log_enable(struct connman_debug_desc *start,
--
1.8.3.2
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
