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. Re: [PATCH 3/3] wifi: p2p: Remove peers when technology is
disabled. (John Ernberg)
2. [PATCH v2 0/2] wifi: p2p: Improvements on p2p and peer
handling (John Ernberg)
3. [PATCH v2 1/2] wifi: p2p: Fix several unref of NULL pointer.
(John Ernberg)
4. [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. (Tomasz Bursztyka)
----------------------------------------------------------------------
Message: 1
Date: Wed, 16 Dec 2015 13:38:00 +0000
From: John Ernberg <[email protected]>
To: Tomasz Bursztyka <[email protected]>,
"[email protected]" <[email protected]>
Subject: Re: [PATCH 3/3] 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 02:26 PM, Tomasz Bursztyka wrote:
> Le 16/12/2015 14:01, John Ernberg a ?crit :
>> Hi Tomasz
>>
>> On 12/16/2015 01:52 PM, Tomasz Bursztyka wrote:
>>> Hi John,
>>>
>>> NACK.
>>>
>>>> From: John Ernberg <[email protected]>
>>>>
>>>> If Wi-Fi was disabled, either by a removal of a Wi-Fi dongle, or
>>>> through the
>>>> DBus API, the discovered Peers would sometimes remain as the callback
>>>> from
>>>> gsupplicant would be called after the disable / destruction of the
>>>> Wi-Fi
>>>> technology. This resulted in the list of Peers never being cleared.
>>> Gsupplicant follows wpa_supplicant. So if wpa_s does not clear up its
>>> peer list,
>>> gsupplicant (and wifi plugin so) won't either. Then the bug is in wpa_s
>>>
>>> That said, if wpa_s is actually clearing up its list but not
>>> gsupplicant then this last
>>> one has a bug.
>>>
>>> But I don't see this in your patch, you are doing all in wifi plugin,
>>> thus interpreting
>>> stuff there. Plus you are adding code which does not match any of your
>>> commit message.
>>>
>> What happens is that the Wi-Fi technology gets cleaned up before wpa_s
>> (and GSupplicant) has sent the peer-unregister signals.
>
> Ok that's the bug you want to fix. Much clearer that way.
>
> Be very specific in you commit message, you can even paste some logs
> there, it's not a problem.
I apologize, I do sometimes have problems expressing myself the first
time around, I will update the commit message.
>
>> Without patch 1, this caused Connman to crash with a SIGSEGV due to
>> unref of a NULL pointer. When that crash was fixed, this issue became
>> apparent.
>> Calling GetPeers() with the Wi-Fi disabled would still return a list of
>> peers.
>
> Then cleaning up the peer list from wifi.c makes sense indeed.
>
>> diff --git a/plugins/wifi.c b/plugins/wifi.c
>> index dc8a303..21f6c27 100644
>> --- a/plugins/wifi.c
>> +++ b/plugins/wifi.c
>> @@ -139,8 +139,8 @@ struct wifi_data {
>> GSupplicantScanParams *scan_params;
>> unsigned int p2p_find_timeout;
>> unsigned int p2p_connection_timeout;
>> - struct connman_peer *pending_peer;
>> - GSupplicantPeer *peer;
>> + struct connman_peer *peer;
>>> No renaming please.
>> A pending_peer is no longer pending once connected, so keeping that name
>> would make the code confusing.
>
> And pending_peer is used only when connecting. So we don't care once
> it's connected.
> Keep it as pending_peer.
Ok. Will revert the name changes.
>
>>>> + GSList *peers;
>>> Ok, let's see the point below.
>>>
>>>> bool p2p_connecting;
>>>> bool p2p_device;
>>>> int servicing;
>>>> @@ -244,12 +244,10 @@ static void peer_cancel_timeout(struct
>>>> wifi_data *wifi)
>>>> wifi->p2p_connection_timeout = 0;
>>>> wifi->p2p_connecting = false;
>>>> - if (wifi->pending_peer) {
>>>> - connman_peer_unref(wifi->pending_peer);
>>>> - wifi->pending_peer = NULL;
>>>> + if (wifi->peer) {
>>>> + connman_peer_unref(wifi->peer);
>>>> + wifi->peer = NULL;
>>>> }
>>>> -
>>>> - wifi->peer = NULL;
>>> So no renaming here as well.
>>>
>>>> }
>>>> static gboolean peer_connect_timeout(gpointer data)
>>>> @@ -258,13 +256,16 @@ static gboolean peer_connect_timeout(gpointer
>>>> data)
>>>> DBG("");
>>>> - if (wifi->p2p_connecting) {
>>>> + if (wifi && wifi->p2p_connecting) {
>>> Checking the validity of wifi should go in your patch 2, not here.
>> Then this patch has a dependency on the other patch, which I have gotten
>> the impression of through other patch-sets is not allowed.
>
> My mistake, it should go in your patch 1. (my mail client ordered them
> like 2, 1, 3...)
My mistake, I missed a parameter in my configuration so the patches got
sent out-of-order.
Will move the guards to patch 1.
>
>>>> enum connman_peer_state state = CONNMAN_PEER_STATE_FAILURE;
>>>> + GSupplicantPeer *gs_peer =
>>>> + g_supplicant_interface_peer_lookup(wifi->interface,
>>>> + connman_peer_get_identifier(wifi->peer));
>>> ok, but a real fix would be to properly track peers from gsupplicant,
>>> then through wifi.c and finaly to peer.c
>>> If it happens that a peer is still in peer.c but not in gsupplicant
>>> anymore then we have a bug. And the logic
>>> here is just a workaround I'd prefer not to see.
>>>
>>>> - if (g_supplicant_peer_has_requested_connection(wifi->peer))
>>>> + if (g_supplicant_peer_has_requested_connection(gs_peer))
>>>> state = CONNMAN_PEER_STATE_IDLE;
>>>> - connman_peer_set_state(wifi->pending_peer, state);
>>>> + connman_peer_set_state(wifi->peer, state);
>>>> }
>>>> peer_cancel_timeout(wifi);
>>>> @@ -276,7 +277,11 @@ static void peer_connect_callback(int result,
>>>> GSupplicantInterface *interface,
>>>> void *user_data)
>>>> {
>>>> struct wifi_data *wifi = user_data;
>>>> - struct connman_peer *peer = wifi->pending_peer;
>>>> +
>>>> + if (!wifi)
>>>> + return;
>>>> +
>>> Into your patch 2.
>
> Patch 1 then.
>
>>>
>>>> + struct connman_peer *peer = wifi->peer;
>>> No renaming.
>>>
>>>> DBG("peer %p - %d", peer, result);
>>>> @@ -357,8 +362,7 @@ static int peer_connect(struct connman_peer
>>>> *peer,
>>>> ret = g_supplicant_interface_p2p_connect(wifi->interface,
>>>> peer_params,
>>>> peer_connect_callback, wifi);
>>>> if (ret == -EINPROGRESS) {
>>>> - wifi->pending_peer = connman_peer_ref(peer);
>>>> - wifi->peer = gs_peer;
>>>> + wifi->peer = connman_peer_ref(peer);
>>>> wifi->p2p_connecting = true;
>>>> } else if (ret < 0) {
>>>> g_free(peer_params->path);
>>>> @@ -811,6 +815,21 @@ static void remove_networks(struct
>>>> connman_device *device,
>>>> wifi->networks = NULL;
>>>> }
>>>> +static void remove_peers(struct wifi_data *wifi)
>>>> +{
>>>> + GSList *list;
>>>> +
>>>> + for (list = wifi->peers; list; list = list->next) {
>>>> + struct connman_peer *peer = list->data;
>>>> +
>>>> + connman_peer_unregister(peer);
>>>> + connman_peer_unref(peer);
>>>> + }
>>>> +
>>>> + g_slist_free(wifi->peers);
>>>> + wifi->peers = NULL;
>>>> +}
>>>> +
>>>> static void reset_autoscan(struct connman_device *device)
>>>> {
>>>> struct wifi_data *wifi = connman_device_get_data(device);
>>>> @@ -894,6 +913,7 @@ static void wifi_remove(struct connman_device
>>>> *device)
>>>> g_source_remove(wifi->p2p_connection_timeout);
>>>> remove_networks(device, wifi);
>>>> + remove_peers(wifi);
>>>
>>> We follow the peers from wpa_supplicant -> gsupplicant -> wifi.c ->
>>> peer.c
>>> So that chain should "always" work. Plus the logic of peers is much
>>> different with wifi networks and services.
>>> as there is no in-between objects like networks.
>>>
>>> So no need of that.
>> See the first comment.
>>>
>>>> connman_device_set_powered(device, false);
>>>> connman_device_set_data(device, NULL);
>>>> @@ -1541,6 +1561,7 @@ static int wifi_disable(struct connman_device
>>>> *device)
>>>> }
>>>> remove_networks(device, wifi);
>>>> + remove_peers(wifi);
>>> See, here also no need of that. Unless wpa_s does something wrong ...
>>> which would need to be fixed there.
>>>
>>> Finally in this patch, I don't see the point of your internal list of
>>> peers then.
>>>
>>>> ret = g_supplicant_interface_remove(wifi->interface, NULL,
>>>> NULL);
>>>> if (ret < 0)
>>>> @@ -2799,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);
>>>> }
>>>> static void peer_lost(GSupplicantPeer *peer)
>>>> @@ -2818,12 +2841,14 @@ static void peer_lost(GSupplicantPeer *peer)
>>>> connman_peer = connman_peer_get(wifi->device, identifier);
>>>> if (connman_peer) {
>>>> if (wifi->p2p_connecting &&
>>>> - wifi->pending_peer == connman_peer) {
>>>> + wifi->peer == connman_peer) {
>>> No renaming.
>>>
>>>> peer_connect_timeout(wifi);
>>>> }
>>>> connman_peer_unregister(connman_peer);
>>>> connman_peer_unref(connman_peer);
>>>> }
>>>> +
>>>> + wifi->peers = g_slist_remove(wifi->peers, connman_peer);
>>>> }
>>>> static void peer_changed(GSupplicantPeer *peer,
>>>> GSupplicantPeerState state)
>>>> @@ -2879,7 +2904,7 @@ static void peer_changed(GSupplicantPeer *peer,
>>>> GSupplicantPeerState state)
>>>> if (p_state == CONNMAN_PEER_STATE_CONFIGURATION ||
>>>> p_state == CONNMAN_PEER_STATE_FAILURE) {
>>>> if (wifi->p2p_connecting
>>>> - && connman_peer == wifi->pending_peer)
>>>> + && connman_peer == wifi->peer)
>>> No renaming.
>>>
>>>> peer_cancel_timeout(wifi);
>>>> else
>>>> p_state = CONNMAN_PEER_STATE_UNKNOWN;
>>>
>>> To sum up, have you experienced a wpa_s peer list being empty, but not
>>> in connman?
>> I have experienced the peer-list being populated with the wifi
>> technology set to Powered=False
>> Which is just an accident waiting to happen in my opinion.
>
> In the same context as you are talking about above or?
Yes, same context.
>
> As I said, we might have a bug elsewhere or worse: wpa_s might have a
> bug as well.
I have not dug into how Connman reacts when for example a Wi-Fi
interface disappears from the kernel compared to wpa_s, might be an
issue there, I don't know, it's beyond my abilities to research.
>
>>> If so, then the bug is in gsupplicant or wifi.c or peer.c (a
>>> reference count maybe etc...)
>>>
>>> But here your patch is just adding a logic we don't need. When you
>>> disable wifi, wpa_s will
>>> destroy its objects, thus guspplicant will do the same etc... Your
>>> code is unnecessary overhead.
>>> It's a shortcut for something that will happen anyway.
>> Except when it doesn't happen as mentioned in the first comment.
>
> Yes, which bug you should have explained in the first place.
> It's a chicken and egg issue.
>
>
> Tomasz
Thank you for the review. Updated patch-set incoming.
Best regards // John Ernberg
------------------------------
Message: 2
Date: Wed, 16 Dec 2015 15:04:04 +0000
From: John Ernberg <[email protected]>
To: "[email protected]" <[email protected]>
Subject: [PATCH v2 0/2] wifi: p2p: Improvements on p2p and peer
handling
Message-ID: <[email protected]>
Content-Type: text/plain; charset="iso-8859-1"
From: John Ernberg <[email protected]>
Here is a new patch-set addressing the issues mentioned in the review.
This patch-set improves stability of Connman in p2p context.
John Ernberg (2):
wifi: p2p: Fix several unref of NULL pointer.
wifi: p2p: Remove peers when technology is disabled.
plugins/wifi.c | 94 +++++++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 73 insertions(+), 21 deletions(-)
--
1.9.1
------------------------------
Message: 3
Date: Wed, 16 Dec 2015 15:04:04 +0000
From: John Ernberg <[email protected]>
To: "[email protected]" <[email protected]>
Subject: [PATCH v2 1/2] wifi: p2p: Fix several unref of NULL pointer.
Message-ID: <[email protected]>
Content-Type: text/plain; charset="iso-8859-1"
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;
+
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) {
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;
+
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;
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;
+
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;
+
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;
+
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;
+
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);
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;
+ }
error:
p2p_find_stop(device);
}
@@ -2512,6 +2537,9 @@ static void p2p_support(GSupplicantInterface *interface)
DBG("");
+ if (!interface)
+ return;
+
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;
+
connman_peer = connman_peer_get(wifi->device, identifier);
if (!connman_peer)
return;
--
1.9.1
------------------------------
Message: 4
Date: Wed, 16 Dec 2015 15:04:04 +0000
From: John Ernberg <[email protected]>
To: "[email protected]" <[email protected]>
Subject: [PATCH v2 2/2] wifi: p2p: Remove peers when technology is
disabled.
Message-ID: <[email protected]>
Content-Type: text/plain; charset="iso-8859-1"
From: John Ernberg <[email protected]>
If Wi-Fi was disabled, either by removal of a Wi-Fi dongle, or through the
DBus API, the Wi-Fi technology would sometimes be cleaned up before
gsupplicant would be signalled by wpa_s that the Peers were removed.
This means that the discovered peers are not cleaned up properly as there is
no wifi instance that can handle the callback of the signal.
Thus the GetPeers() method call could return a list of Peers while Wi-Fi was
disabled or removed.
Rewrite the peer-handling to be done more like the networks handling so that
when the technology is disabled it's possible to loop over the peers and
free them.
---
plugins/wifi.c | 31 ++++++++++++++++++++++++++-----
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/plugins/wifi.c b/plugins/wifi.c
index 6320b5b..e03d275 100644
--- a/plugins/wifi.c
+++ b/plugins/wifi.c
@@ -140,7 +140,7 @@ struct wifi_data {
unsigned int p2p_find_timeout;
unsigned int p2p_connection_timeout;
struct connman_peer *pending_peer;
- GSupplicantPeer *peer;
+ GSList *peers;
bool p2p_connecting;
bool p2p_device;
int servicing;
@@ -248,8 +248,6 @@ static void peer_cancel_timeout(struct wifi_data *wifi)
connman_peer_unref(wifi->pending_peer);
wifi->pending_peer = NULL;
}
-
- wifi->peer = NULL;
}
static gboolean peer_connect_timeout(gpointer data)
@@ -260,8 +258,11 @@ static gboolean peer_connect_timeout(gpointer data)
if (wifi && wifi->p2p_connecting) {
enum connman_peer_state state = CONNMAN_PEER_STATE_FAILURE;
+ GSupplicantPeer *gs_peer =
+ g_supplicant_interface_peer_lookup(wifi->interface,
+ connman_peer_get_identifier(wifi->peer));
- if (g_supplicant_peer_has_requested_connection(wifi->peer))
+ if (g_supplicant_peer_has_requested_connection(gs_peer))
state = CONNMAN_PEER_STATE_IDLE;
connman_peer_set_state(wifi->pending_peer, state);
@@ -362,7 +363,6 @@ static int peer_connect(struct connman_peer *peer,
peer_connect_callback, wifi);
if (ret == -EINPROGRESS) {
wifi->pending_peer = connman_peer_ref(peer);
- wifi->peer = gs_peer;
wifi->p2p_connecting = true;
} else if (ret < 0) {
g_free(peer_params->path);
@@ -815,6 +815,21 @@ static void remove_networks(struct connman_device *device,
wifi->networks = NULL;
}
+static void remove_peers(struct wifi_data *wifi)
+{
+ GSList *list;
+
+ for (list = wifi->peers; list; list = list->next) {
+ struct connman_peer *peer = list->data;
+
+ connman_peer_unregister(peer);
+ connman_peer_unref(peer);
+ }
+
+ g_slist_free(wifi->peers);
+ wifi->peers = NULL;
+}
+
static void reset_autoscan(struct connman_device *device)
{
struct wifi_data *wifi = connman_device_get_data(device);
@@ -898,6 +913,7 @@ static void wifi_remove(struct connman_device *device)
g_source_remove(wifi->p2p_connection_timeout);
remove_networks(device, wifi);
+ remove_peers(wifi);
connman_device_set_powered(device, false);
connman_device_set_data(device, NULL);
@@ -1545,6 +1561,7 @@ static int wifi_disable(struct connman_device *device)
}
remove_networks(device, wifi);
+ remove_peers(wifi);
ret = g_supplicant_interface_remove(wifi->interface, NULL, NULL);
if (ret < 0)
@@ -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);
}
static void peer_lost(GSupplicantPeer *peer)
@@ -2828,6 +2847,8 @@ static void peer_lost(GSupplicantPeer *peer)
connman_peer_unregister(connman_peer);
connman_peer_unref(connman_peer);
}
+
+ wifi->peers = g_slist_remove(wifi->peers, connman_peer);
}
static void peer_changed(GSupplicantPeer *peer, GSupplicantPeerState state)
--
1.9.1
------------------------------
Message: 5
Date: Wed, 16 Dec 2015 17:22:41 +0100
From: Tomasz Bursztyka <[email protected]>
To: [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,
> 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.
> 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.
> 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.
> 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...
> 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.
> 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.
> 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.
> 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.
> 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
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 2, Issue 18
**************************************
