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 2/3] wifi: p2p: Fix conditional for p2p driver
unregister. (John Ernberg)
2. Re: [PATCH 3/3] wifi: p2p: Remove peers when technology is
disabled. (John Ernberg)
3. Re: [PATCH 2/3] wifi: p2p: Fix conditional for p2p driver
unregister. (Tomasz Bursztyka)
4. Re: [PATCH 2/3] wifi: p2p: Fix conditional for p2p driver
unregister. (John Ernberg)
5. Re: Equivalent to wpa_supplicant's freq_list? (Patrik Flykt)
6. Re: [PATCH 3/3] wifi: p2p: Remove peers when technology is
disabled. (Tomasz Bursztyka)
----------------------------------------------------------------------
Message: 1
Date: Wed, 16 Dec 2015 12:50:17 +0000
From: John Ernberg <[email protected]>
To: Tomasz Bursztyka <[email protected]>,
"[email protected]" <[email protected]>
Subject: Re: [PATCH 2/3] wifi: p2p: Fix conditional for p2p driver
unregister.
Message-ID: <[email protected]>
Content-Type: text/plain; charset="Windows-1252"
Hi Tomasz
On 12/16/2015 01:27 PM, Tomasz Bursztyka wrote:
> Hi John,
>
>> From: John Ernberg <[email protected]>
>>
>> ---
>> plugins/wifi.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/plugins/wifi.c b/plugins/wifi.c
>> index 7270ee9..dc8a303 100644
>> --- a/plugins/wifi.c
>> +++ b/plugins/wifi.c
>> @@ -859,7 +859,7 @@ static void check_p2p_technology(void)
>> p2p_exists = true;
>> }
>> - if (!p2p_exists) {
>> + if (p2p_exists) {
>> connman_technology_driver_unregister(&p2p_tech_driver);
>> connman_peer_driver_unregister(&peer_driver);
>
> So if the wifi device supports P2P you remove its exposure into ConnMan?
>
>
> What are you trying to fix here?
This is checked during the disable / removal of the Wi-Fi technology,
thus the current logic to me looks like doesn't unregister the p2p
technology driver for p2p interfaces, which is the only type of
interface that registers one.
If I understood this logic wrong, please correct me.
>
>
> Tomasz
> _______________________________________________
> connman mailing list
> [email protected]
> https://lists.01.org/mailman/listinfo/connman
Best regards // John Ernberg
------------------------------
Message: 2
Date: Wed, 16 Dec 2015 13:01:32 +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 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.
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.
>>
>> 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.
>
> Don't follow the same logic as networks. It's much different.
>
>> ---
>> plugins/wifi.c | 55
>> ++++++++++++++++++++++++++++++++++++++++---------------
>> 1 file changed, 40 insertions(+), 15 deletions(-)
>>
>> 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.
>
>> + 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.
>
>> 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.
>
>> + 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.
> 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.
>
>
> Tomasz
> _______________________________________________
> connman mailing list
> [email protected]
> https://lists.01.org/mailman/listinfo/connman
If the issue this patch is addressing shall be solved differently, then
I am all ears.
Best regards // John Ernberg
------------------------------
Message: 3
Date: Wed, 16 Dec 2015 14:12:52 +0100
From: Tomasz Bursztyka <[email protected]>
To: John Ernberg <[email protected]>, "[email protected]"
<[email protected]>
Subject: Re: [PATCH 2/3] wifi: p2p: Fix conditional for p2p driver
unregister.
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
Le 16/12/2015 13:50, John Ernberg a ?crit :
> Hi Tomasz
>
> On 12/16/2015 01:27 PM, Tomasz Bursztyka wrote:
>> Hi John,
>>
>>> From: John Ernberg <[email protected]>
>>>
>>> ---
>>> plugins/wifi.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/plugins/wifi.c b/plugins/wifi.c
>>> index 7270ee9..dc8a303 100644
>>> --- a/plugins/wifi.c
>>> +++ b/plugins/wifi.c
>>> @@ -859,7 +859,7 @@ static void check_p2p_technology(void)
>>> p2p_exists = true;
>>> }
>>> - if (!p2p_exists) {
>>> + if (p2p_exists) {
>>> connman_technology_driver_unregister(&p2p_tech_driver);
>>> connman_peer_driver_unregister(&peer_driver);
>> So if the wifi device supports P2P you remove its exposure into ConnMan?
>>
>>
>> What are you trying to fix here?
> This is checked during the disable / removal of the Wi-Fi technology,
> thus the current logic to me looks like doesn't unregister the p2p
> technology driver for p2p interfaces
"looks like"?
Tests the stuff without your patch. Afaik, it works. (ok, used to like 1
year ago... Haven't played with p2p since)
It's not because the wifi technology is being disabled that the p2p
technology _driver_ should be removed.
And if the wifi_remove() removes the last device of wifi technology,
then the current logic is right: there would be
no device that could serve p2p thus unregistering p2p technology.
The state of the technology is handled differently.
Tomasz
------------------------------
Message: 4
Date: Wed, 16 Dec 2015 13:14:42 +0000
From: John Ernberg <[email protected]>
To: Tomasz Bursztyka <[email protected]>,
"[email protected]" <[email protected]>
Subject: Re: [PATCH 2/3] wifi: p2p: Fix conditional for p2p driver
unregister.
Message-ID: <[email protected]>
Content-Type: text/plain; charset="Windows-1252"
On 12/16/2015 02:12 PM, Tomasz Bursztyka wrote:
> Le 16/12/2015 13:50, John Ernberg a ?crit :
>> Hi Tomasz
>>
>> On 12/16/2015 01:27 PM, Tomasz Bursztyka wrote:
>>> Hi John,
>>>
>>>> From: John Ernberg <[email protected]>
>>>>
>>>> ---
>>>> plugins/wifi.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/plugins/wifi.c b/plugins/wifi.c
>>>> index 7270ee9..dc8a303 100644
>>>> --- a/plugins/wifi.c
>>>> +++ b/plugins/wifi.c
>>>> @@ -859,7 +859,7 @@ static void check_p2p_technology(void)
>>>> p2p_exists = true;
>>>> }
>>>> - if (!p2p_exists) {
>>>> + if (p2p_exists) {
>>>> connman_technology_driver_unregister(&p2p_tech_driver);
>>>> connman_peer_driver_unregister(&peer_driver);
>>> So if the wifi device supports P2P you remove its exposure into
>>> ConnMan?
>>>
>>>
>>> What are you trying to fix here?
>> This is checked during the disable / removal of the Wi-Fi technology,
>> thus the current logic to me looks like doesn't unregister the p2p
>> technology driver for p2p interfaces
>
> "looks like"?
>
> Tests the stuff without your patch. Afaik, it works. (ok, used to like
> 1 year ago... Haven't played with p2p since)
> It's not because the wifi technology is being disabled that the p2p
> technology _driver_ should be removed.
>
> And if the wifi_remove() removes the last device of wifi technology,
> then the current logic is right: there would be
> no device that could serve p2p thus unregistering p2p technology.
>
> The state of the technology is handled differently.
>
> Tomasz
Ok, then I did not trace right when I tried to figure out how that
worked which led me to the wrong conclusion, apologies.
The patch can be dropped.
Best regards // John Ernberg
------------------------------
Message: 5
Date: Wed, 16 Dec 2015 15:19:37 +0200
From: Patrik Flykt <[email protected]>
To: Mike Purvis <[email protected]>
Cc: [email protected]
Subject: Re: Equivalent to wpa_supplicant's freq_list?
Message-ID: <[email protected]>
Content-Type: text/plain; charset="UTF-8"
Hi,
On Mon, 2015-12-14 at 10:39 -0500, Mike Purvis wrote:
> How are the frequencies set for tethering? Is this possible using
> connmanctl, or do I need to speak dbus to access this functionality?
ConnMan is using channel 1 on the 2.4 GHz band when tethering. One
really cannot use any 5 GHz channel without prior knowledge as most of
the 5 GHz channels need to do radar avoidance. And of course the
channels are different around the world. And there still are 2.4 GHz
only devices around.
In a perfect world wpa_supplicant should know which channel to use as it
is the only component with possibly all the data about the radio
conditions and channels to avoid. In a not so perfect world ConnMan
guesses safely and always uses channel 1. The "crowded channel"
-syndrome, if there happens to be any in the location tethering is
enabled, is of course also present on channel 1. Thus ConnMan still
waits for logic to happen on wpa_supplicant side and wpa_supplicant to
be able to pick a channel...
> I'm assuming that there's no way to specify frequencies/channels when
> in client mode?
I think it is possible, but if the AP is not on the channel or roaming
needs to take place, it won't connect or roam. So not use there.
Cheers,
Patrik
------------------------------
Message: 6
Date: Wed, 16 Dec 2015 14:26:53 +0100
From: Tomasz Bursztyka <[email protected]>
To: John Ernberg <[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; format=flowed
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.
> 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.
>>> + 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...)
>>> 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?
As I said, we might have a bug elsewhere or worse: wpa_s might have a
bug as well.
>> 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
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 2, Issue 17
**************************************
