Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
https://lists.01.org/mailman/listinfo/connman
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. [PATCH] service: state transition to configuration (Naveen Singh)
2. Re: DHCP renewal failure (Naveen Singh)
3. Re: [PATCH v2 1/2] wifi: p2p: Fix several unref of NULL
pointer. (John Ernberg)
4. Re: [PATCH v2 2/2] wifi: p2p: Remove peers when technology is
disabled. (John Ernberg)
5. Re: [PATCH v2 1/2] wifi: p2p: Fix several unref of NULL
pointer. (Hannikainen, Jaakko)
6. Re: [PATCH v2 1/2] wifi: p2p: Fix several unref of NULL
pointer. (John Ernberg)
7. Re: [PATCH v2 1/2] wifi: p2p: Fix several unref of NULL
pointer. (Tomasz Bursztyka)
----------------------------------------------------------------------
Message: 1
Date: Wed, 16 Dec 2015 21:02:17 -0800
From: Naveen Singh <[email protected]>
To: [email protected]
Subject: [PATCH] service: state transition to configuration
Message-ID:
<[email protected]>
From: nasingh <[email protected]>
If DHCP renewal fails and eventually lease expiry happens the
service state machine was still left to online. This change changes
ipv4 service state to configuration when ipv4 address is released.
---
src/service.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/service.c b/src/service.c
index abf0899..52fd6e8 100644
--- a/src/service.c
+++ b/src/service.c
@@ -6377,6 +6377,12 @@ static void service_ip_release(struct connman_ipconfig
*ipconfig,
CONNMAN_SERVICE_STATE_DISCONNECT,
CONNMAN_IPCONFIG_TYPE_IPV4);
+ if (type == CONNMAN_IPCONFIG_TYPE_IPV4 &&
+ method == CONNMAN_IPCONFIG_METHOD_DHCP)
+ __connman_service_ipconfig_indicate_state(service,
+ CONNMAN_SERVICE_STATE_CONFIGURATION,
+ CONNMAN_IPCONFIG_TYPE_IPV4);
+
settings_changed(service, ipconfig);
}
--
2.6.0.rc2.230.g3dd15c0
------------------------------
Message: 2
Date: Wed, 16 Dec 2015 21:12:40 -0800
From: Naveen Singh <[email protected]>
To: Patrik Flykt <[email protected]>
Cc: [email protected]
Subject: Re: DHCP renewal failure
Message-ID:
<CAGTDzKmCfrHCof2pnjEZvQamEEJu+o51rra=tute-0oubc4...@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
On Thu, Dec 10, 2015 at 1:12 AM, Patrik Flykt <[email protected]>
wrote:
>
> Hi,
>
> On Fri, 2015-12-04 at 07:28 -0800, Naveen Singh wrote:
> > Hi All
> > In case of DHCP server not being available and eventually device
> > failing to renew its IP address, I am observing following:
> >
> > 1. Connman service state is left to online.
> >
> > 2. Although the connman_ipconfig structure entries for ipv4 address is
> > cleared out, connman never generated service property changed event.
> > This probably is because DHCP renewal process never feeds back to
> > service state machine from where we generate these events. At this
> > point if we run connmanctl, for ipv4 configuration it will still show
> > the old IP address.
>
> This looks like a bug needing fixing. The state should be disconnected
> or configuration and eventually ready if IPv4LL is an option for this
> service. Provided that IPv6 is not already online for this service.
>
I have a patch out for this. I used service_ip_release which gets called
when the
address is deleted to do the appropriate state transition.
In my local testing i found another issue which may require fix. This is
the scenario:
1. Device fails to renew its IP address
2. After my patch IPV4 state transitions to configuration.
3. Service gets an IPV4 LL address.
4. IPv4 state is changed to ready.
5. Device is still sending broadcast DHCP discover request
6. As soon as device finds a DHCP server and it gets an IP address, IPV4 LL
address is removed and state changes to configuration again.
7. But the service state is still left to configuration.
Why do not we use service_ops (connman_ipconfig_ops) various functions to
do service transitions? DHCP state machine adds various addresses and
routes once DHCP ACK is received and that would call rtnl handlers which
would then call these service handlers. Are not they perfect place to do
service state transitions.
>
> > 3. I am not sure how IPV4LL would be handled.
> >
> > Not sure if we can do anything for 1 or not as connman service state
> > is an OR of ipv4 service state and ipv6 service state. But for sure 2
> > looks like an issue. Currently there is no API to indicate clearing of
> > IP address. I am working on a patch to have an API which would
> > indicate generating of a Service property changed API when IP address
> > is set to NULL. But before that I wanted to get the feedback on the
> > current behavior and find out if this is an intended behavior or a
> > bug?
>
> I don't think there is a need for an new API here. When service state is
> updated, a new State property is sent. It seems it all depends on the
> non-information that DHCP lost its address, which when fixed will
> resolve all three points above.
>
> Cheers,
>
> Patrik
>
>
> _______________________________________________
> connman mailing list
> [email protected]
> https://lists.01.org/mailman/listinfo/connman
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.01.org/pipermail/connman/attachments/20151216/798c28e9/attachment-0001.html>
------------------------------
Message: 3
Date: Thu, 17 Dec 2015 07:31:15 +0000
From: John Ernberg <[email protected]>
To: Tomasz Bursztyka <[email protected]>,
"[email protected]" <[email protected]>
Subject: Re: [PATCH v2 1/2] wifi: p2p: Fix several unref of NULL
pointer.
Message-ID: <[email protected]>
Content-Type: text/plain; charset="Windows-1252"
Hi Tomasz
On 12/16/2015 05:22 PM, Tomasz Bursztyka wrote:
> Hi John,
>
>> From: John Ernberg <[email protected]>
>>
>> When a Wi-Fi dongle would be removed from the system or if
>> wpa_supplicant
>> would send a system_killed signal it's possible that the wifi plugin is
>> destroyed before gsupplicant has reached a state where it will no longer
>> call any callbacks.
>> This would result in various SIGSEGV abortions by unref of NULL pointer.
>>
>> The changes to the p2p scanning were copied from wifi scanning.
>> ---
>> plugins/wifi.c | 63
>> +++++++++++++++++++++++++++++++++++++++++++---------------
>> 1 file changed, 47 insertions(+), 16 deletions(-)
>>
>> diff --git a/plugins/wifi.c b/plugins/wifi.c
>> index 9e9ad64..6320b5b 100644
>> --- a/plugins/wifi.c
>> +++ b/plugins/wifi.c
>> @@ -235,6 +235,9 @@ static void remove_pending_wifi_device(struct
>> wifi_data *wifi)
>> static void peer_cancel_timeout(struct wifi_data *wifi)
>> {
>> + if (!wifi)
>> + return;
>> +
>
> No need of that one. Look where peer_cancel_timeout is called: wifi is
> always a valid pointer.
Ok.
>
>> if (wifi->p2p_connection_timeout > 0)
>> g_source_remove(wifi->p2p_connection_timeout);
>> @@ -255,7 +258,7 @@ static gboolean peer_connect_timeout(gpointer
>> data)
>> DBG("");
>> - if (wifi->p2p_connecting) {
>> + if (wifi && wifi->p2p_connecting) {
>
> Same here.
Ok.
>
>> enum connman_peer_state state = CONNMAN_PEER_STATE_FAILURE;
>> if (g_supplicant_peer_has_requested_connection(wifi->peer))
>> @@ -273,6 +276,10 @@ static void peer_connect_callback(int result,
>> GSupplicantInterface *interface,
>> void *user_data)
>> {
>> struct wifi_data *wifi = user_data;
>> +
>> + if (!wifi)
>> + return;
>> +
>
> Same here.
Had a SIGSEGV here, can happen if you remove the Wi-Fi dongle from the
computer in the middle of a p2p connect process.
>
>> struct connman_peer *peer = wifi->pending_peer;
>> DBG("peer %p - %d", peer, result);
>> @@ -309,7 +316,7 @@ static int peer_connect(struct connman_peer *peer,
>> return -ENODEV;
>> wifi = connman_device_get_data(device);
>> - if (!wifi)
>> + if (!wifi && !wifi->interface)
>> return -ENODEV;
>
> OK.
>
>> if (wifi->p2p_connecting)
>> @@ -417,6 +424,9 @@ static void apply_p2p_listen_on_iface(gpointer
>> data, gpointer user_data)
>> {
>> struct wifi_data *wifi = data;
>> + if (!wifi)
>> + return;
>> +
>
> I don't think it's possible to get a NULL here, when it comes to
> travers iface_list.
> Or then this means we would have inserted a NULL pointer as a valid
> iface...
What happens if wifi is cleaned up before the callback calling this
function is called?
I added this one based on the other case where I had a SIGSEGV.
>
>> if (!wifi->interface ||
>> !g_supplicant_interface_has_p2p(wifi->interface))
>> return;
>> @@ -578,6 +588,10 @@ static int peer_register_service(const unsigned
>> char *specification,
>> for (list = iface_list; list; list = list->next) {
>> struct wifi_data *wifi = list->data;
>> +
>> + if (!wifi)
>> + continue;
>> +
>
> Same here.
Ok
>
>> GSupplicantInterface *iface = wifi->interface;
>> if (!g_supplicant_interface_has_p2p(iface))
>> @@ -629,6 +643,9 @@ static int peer_unregister_wfd_service(void)
>> for (list = iface_list; list; list = list->next) {
>> struct wifi_data *wifi = list->data;
>> + if (!wifi)
>> + continue;
>> +
>
> Same here.
Ok
>
>> if (!g_supplicant_interface_has_p2p(wifi->interface))
>> continue;
>> @@ -663,6 +680,10 @@ static int peer_unregister_service(const
>> unsigned char *specification,
>> for (list = iface_list; list; list = list->next) {
>> struct wifi_data *wifi = list->data;
>> +
>> + if (!wifi)
>> + continue;
>> +
>
> Same here.
Ok
>
>> GSupplicantInterface *iface = wifi->interface;
>> if (wfd)
>> @@ -1678,11 +1699,13 @@ static gboolean p2p_find_stop(gpointer data)
>> DBG("");
>> - wifi->p2p_find_timeout = 0;
>> + if (wifi) {
>> + wifi->p2p_find_timeout = 0;
>> - connman_device_set_scanning(device, CONNMAN_SERVICE_TYPE_P2P,
>> false);
>> + g_supplicant_interface_p2p_stop_find(wifi->interface);
>> + }
>> - g_supplicant_interface_p2p_stop_find(wifi->interface);
>> + connman_device_set_scanning(device, CONNMAN_SERVICE_TYPE_P2P,
>> false);
>
> OK.
>
>> connman_device_unref(device);
>> reset_autoscan(device);
>> @@ -1698,20 +1721,22 @@ static void p2p_find_callback(int result,
>> GSupplicantInterface *interface,
>> DBG("result %d wifi %p", result, wifi);
>> - if (wifi->p2p_find_timeout) {
>> - g_source_remove(wifi->p2p_find_timeout);
>> - wifi->p2p_find_timeout = 0;
>> - }
>> + if (wifi) {
>> + if (wifi->p2p_find_timeout) {
>> + g_source_remove(wifi->p2p_find_timeout);
>> + wifi->p2p_find_timeout = 0;
>> + }
>> - if (result)
>> - goto error;
>> + if (result)
>> + goto error;
>> - wifi->p2p_find_timeout = g_timeout_add_seconds(P2P_FIND_TIMEOUT,
>> - p2p_find_stop, device);
>> - if (!wifi->p2p_find_timeout)
>> - goto error;
>> + wifi->p2p_find_timeout =
>> g_timeout_add_seconds(P2P_FIND_TIMEOUT,
>> + p2p_find_stop, device);
>> + if (!wifi->p2p_find_timeout)
>> + goto error;
>> - return;
>> + return;
>> + }
>
> A bit too complicated.
> Just add at the beginning a :
> if (!wifi)
> goto error;
>
> And that's it.
Ok
>
>> error:
>> p2p_find_stop(device);
>> }
>> @@ -2512,6 +2537,9 @@ static void p2p_support(GSupplicantInterface
>> *interface)
>> DBG("");
>> + if (!interface)
>> + return;
>> +
>
> OK.
>
>> if (!g_supplicant_interface_has_p2p(interface))
>> return;
>> @@ -2814,6 +2842,9 @@ static void peer_changed(GSupplicantPeer
>> *peer, GSupplicantPeerState state)
>> DBG("ident: %s", identifier);
>> + if (!wifi)
>> + return;
>> +
>
> OK.
>
>> connman_peer = connman_peer_get(wifi->device, identifier);
>> if (!connman_peer)
>> return;
>
> Tomasz
> _______________________________________________
> connman mailing list
> [email protected]
> https://lists.01.org/mailman/listinfo/connman
Best regards // John Ernberg
------------------------------
Message: 4
Date: Thu, 17 Dec 2015 07:34:32 +0000
From: John Ernberg <[email protected]>
To: Tomasz Bursztyka <[email protected]>,
"[email protected]" <[email protected]>
Subject: Re: [PATCH v2 2/2] wifi: p2p: Remove peers when technology is
disabled.
Message-ID: <[email protected]>
Content-Type: text/plain; charset="Windows-1252"
Hi Tomasz
On 12/16/2015 05:26 PM, Tomasz Bursztyka wrote:
> Hi John,
>> @@ -2803,6 +2820,8 @@ static void peer_found(GSupplicantPeer *peer)
>> ret = connman_peer_register(connman_peer);
>> if (ret < 0 && ret != -EALREADY)
>> connman_peer_unref(connman_peer);
>> +
>> + wifi->peers = g_slist_prepend(wifi->peers, connman_peer);
>
> Add this line in a else here to the previous if () statement.
> You cannot track a connman_peer if has not successfully registered.
That's a really bad 'woops' on my part, thank you for catching it.
>
>
> Tomasz
> _______________________________________________
> connman mailing list
> [email protected]
> https://lists.01.org/mailman/listinfo/connman
Best regards // John Ernberg
------------------------------
Message: 5
Date: Thu, 17 Dec 2015 07:37:33 +0000
From: "Hannikainen, Jaakko" <[email protected]>
To: "[email protected]" <[email protected]>
Subject: Re: [PATCH v2 1/2] wifi: p2p: Fix several unref of NULL
pointer.
Message-ID: <[email protected]>
Content-Type: text/plain; charset="utf-8"
Hi,
On Wed, 2015-12-16 at 15:04 +0000, John Ernberg wrote:
> @@ -309,7 +316,7 @@ static int peer_connect(struct connman_peer
> *peer,
> ? return -ENODEV;
> ?
> ? wifi = connman_device_get_data(device);
> - if (!wifi)
> + if (!wifi && !wifi->interface)
> ? return -ENODEV;
> ?
> ? if (wifi->p2p_connecting)
Crashes with wifi = NULL, you probably want to replace && with ||.
Jaakko
------------------------------
Message: 6
Date: Thu, 17 Dec 2015 07:40:40 +0000
From: John Ernberg <[email protected]>
To: "Hannikainen, Jaakko" <[email protected]>,
"[email protected]" <[email protected]>
Subject: Re: [PATCH v2 1/2] wifi: p2p: Fix several unref of NULL
pointer.
Message-ID: <[email protected]>
Content-Type: text/plain; charset="utf-8"
Hi Jaakko
On 12/17/2015 08:37 AM, Hannikainen, Jaakko wrote:
> Hi,
>
> On Wed, 2015-12-16 at 15:04 +0000, John Ernberg wrote:
>> @@ -309,7 +316,7 @@ static int peer_connect(struct connman_peer
>> *peer,
>> return -ENODEV;
>>
>> wifi = connman_device_get_data(device);
>> - if (!wifi)
>> + if (!wifi && !wifi->interface)
>> return -ENODEV;
>>
>> if (wifi->p2p_connecting)
> Crashes with wifi = NULL, you probably want to replace && with ||.
Indeed. That was terrible of me, thank you for noticing.
>
> Jaakko
> _______________________________________________
> connman mailing list
> [email protected]
> https://lists.01.org/mailman/listinfo/connman
Best regards // John Ernberg
------------------------------
Message: 7
Date: Thu, 17 Dec 2015 09:25:21 +0100
From: Tomasz Bursztyka <[email protected]>
To: John Ernberg <[email protected]>, "[email protected]"
<[email protected]>
Subject: Re: [PATCH v2 1/2] wifi: p2p: Fix several unref of NULL
pointer.
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
Hi John,
>>> enum connman_peer_state state = CONNMAN_PEER_STATE_FAILURE;
>>> if (g_supplicant_peer_has_requested_connection(wifi->peer))
>>> @@ -273,6 +276,10 @@ static void peer_connect_callback(int result,
>>> GSupplicantInterface *interface,
>>> void *user_data)
>>> {
>>> struct wifi_data *wifi = user_data;
>>> +
>>> + if (!wifi)
>>> + return;
>>> +
>> Same here.
> Had a SIGSEGV here, can happen if you remove the Wi-Fi dongle from the
> computer in the middle of a p2p connect process.
Sure but not because wifi will be NULL but because it won't be a valid
pointer anymore at this point.
So to fix it, it would require to cancel the p2p connection properly so
Gsupplicant will forget about the connection.
>>> struct connman_peer *peer = wifi->pending_peer;
>>> DBG("peer %p - %d", peer, result);
>>> @@ -309,7 +316,7 @@ static int peer_connect(struct connman_peer *peer,
>>> return -ENODEV;
>>> wifi = connman_device_get_data(device);
>>> - if (!wifi)
>>> + if (!wifi && !wifi->interface)
>>> return -ENODEV;
>> OK.
>>
>>> if (wifi->p2p_connecting)
>>> @@ -417,6 +424,9 @@ static void apply_p2p_listen_on_iface(gpointer
>>> data, gpointer user_data)
>>> {
>>> struct wifi_data *wifi = data;
>>> + if (!wifi)
>>> + return;
>>> +
>> I don't think it's possible to get a NULL here, when it comes to
>> travers iface_list.
>> Or then this means we would have inserted a NULL pointer as a valid
>> iface...
> What happens if wifi is cleaned up before the callback calling this
> function is called?
> I added this one based on the other case where I had a SIGSEGV.
Same issue as above.
Tomasz
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 2, Issue 20
**************************************
