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: Abort tethering enabling if bridge
creation/configuration fails (Daniel Wagner)
2. Re: [PATCH 3/3][v2] wifi: Abort tethering enabling if bridge
creation/configuration fails (Daniel Wagner)
3. Re: [PATCH] TODO: Add task for tethering (Daniel Wagner)
4. Re: [PATCH 3/3][v2] wifi: Abort tethering enabling if bridge
creation/configuration fails (Jose Blanquicet)
5. Re: [PATCH 3/3][v2] wifi: Abort tethering enabling if bridge
creation/configuration fails (Daniel Wagner)
----------------------------------------------------------------------
Message: 1
Date: Wed, 26 Oct 2016 09:09:57 +0200
From: Daniel Wagner <[email protected]>
To: Jose Blanquicet <[email protected]>
Cc: [email protected], "MANIEZZO Marco (MM)"
<[email protected]>
Subject: Re: [PATCH 3/3] wifi: Abort tethering enabling if bridge
creation/configuration fails
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
>> Nah, I didn't read it correctly when giving a quick glance. I suggest
> to move the whole functionality into a function and just call it from
> the loop. If it returns success leave the loop otherwise continue
> looping. This is just a bit too easy to get it wrong as it stands now.
>
> You are right. It has become a complex code even before this patch. To
> do what you say will make it more clear/readable but unfortunately I am
> currently quite busy to modify and retest all again.
If it is not easy to read, it is usually and pretty good sign that it
might be too complex. I completely understand that everyone is quite
busy but I still hope you can convince your manager that it is worth
getting this right upstream.
> On the other hand, I found that it is required to initialize the
> variable berr (previously bridge_error) because it could be evaluated
> without being assigned in case of one of these two actions fail:
>
> info->ssid = ssid_ap_init(identifier, passphrase);
> if (!info->ssid)
> goto failed;
>
> or ...
>
> wifi->tethering_param->ssid = ssid_ap_init(identifier, passphrase);
> if (!wifi->tethering_param->ssid)
> goto failed;
I see. Yeah, let's fix this later. I have a question which I ask on your
patch v2.
cheers,
daniel
------------------------------
Message: 2
Date: Wed, 26 Oct 2016 09:22:39 +0200
From: Daniel Wagner <[email protected]>
To: Jose Blanquicet <[email protected]>,
[email protected], [email protected]
Subject: Re: [PATCH 3/3][v2] wifi: Abort tethering enabling if bridge
creation/configuration fails
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
On 10/25/2016 04:50 PM, Jose Blanquicet wrote:
> In case there is an error while setting up the bridge for WiFi tethering, the
> selected interface for tethering will remain removed and could not be use
> anymore. This patch aims to avoid such a situation by using
> connman_technology_tethering_notify()'s return value to abort tethering
> enabling thus do not unnecessary remove the interface in case of something was
> wrong with bridge.
>
> ---
> plugins/wifi.c | 24 +++++++++++++++++++-----
> 1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/plugins/wifi.c b/plugins/wifi.c
> index 8bde9c0..68b231d 100644
> --- a/plugins/wifi.c
> +++ b/plugins/wifi.c
> @@ -3138,13 +3138,14 @@ static void sta_remove_callback(int result,
> DBG("ifname %s result %d ", info->ifname, result);
>
> if ((result < 0) || (info->wifi->ap_supported != WIFI_AP_SUPPORTED)) {
> - info->wifi->tethering = true;
> + info->wifi->tethering = false;
> + connman_technology_tethering_notify(info->technology, false);
Isn't this is a bug fix, at least the 'tethering = false' even without
your change? I would prefer to have it in a separate patch.
>
> g_free(info->ifname);
> g_free(info->ssid);
> g_free(info);
>
> - if (info->wifi->ap_supported == WIFI_AP_SUPPORTED){
> + if (info->wifi->ap_supported == WIFI_AP_SUPPORTED) {
Not really necessary to do whitespace fixes in this patch.
> g_free(info->wifi->tethering_param->ssid);
> g_free(info->wifi->tethering_param);
> info->wifi->tethering_param = NULL;
> @@ -3154,8 +3155,6 @@ static void sta_remove_callback(int result,
>
> info->wifi->interface = NULL;
>
> - connman_technology_tethering_notify(info->technology, true);
> -
> g_supplicant_interface_create(info->ifname, driver, info->wifi->bridge,
> ap_create_callback,
> info);
> @@ -3171,7 +3170,7 @@ static int enable_wifi_tethering(struct
> connman_technology *technology,
> struct wifi_tethering_info *info;
> const char *ifname;
> unsigned int mode;
> - int err;
> + int err, berr = 0;
>
> for (list = iface_list; list; list = list->next) {
> wifi = list->data;
> @@ -3230,6 +3229,10 @@ static int enable_wifi_tethering(struct
> connman_technology *technology,
> info->wifi->tethering = true;
> info->wifi->ap_supported = WIFI_AP_SUPPORTED;
>
> + berr = connman_technology_tethering_notify(technology, true);
> + if (berr < 0)
> + goto failed;
> +
> err = g_supplicant_interface_remove(interface,
> sta_remove_callback,
> info);
> @@ -3244,6 +3247,17 @@ static int enable_wifi_tethering(struct
> connman_technology *technology,
> g_free(info);
> g_free(wifi->tethering_param);
> wifi->tethering_param = NULL;
> +
> + /*
> + * Remove bridge if it was correctly created but remove
> + * operation failed. Instead, if bridge creation failed then
> + * break out and do not try again on another interface,
> + * bridge set-up does not depend on it.
> + */
> + if (berr == 0)
> + connman_technology_tethering_notify(technology, false);
> + else
> + break;
> }
>
> return -EOPNOTSUPP;
Okay, that looks much better. Still I am little bothered with complexity
overall but that was before your patch already there. So if my question
above it correct then I hope you can spin a v3 for me. Thanks!
cheers,
daniel
------------------------------
Message: 3
Date: Wed, 26 Oct 2016 09:24:43 +0200
From: Daniel Wagner <[email protected]>
To: Jose Blanquicet <[email protected]>,
[email protected], [email protected]
Subject: Re: [PATCH] TODO: Add task for tethering
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
On 10/25/2016 04:51 PM, Jose Blanquicet wrote:
> ---
> TODO | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/TODO b/TODO
> index 00d9676..c1694e3 100644
> --- a/TODO
> +++ b/TODO
> @@ -99,6 +99,20 @@ Core
> ObjectManager common to Linux desktops can be implemented.
Applied.
Thanks,
Daniel
------------------------------
Message: 4
Date: Wed, 26 Oct 2016 09:56:48 +0200
From: Jose Blanquicet <[email protected]>
To: Daniel Wagner <[email protected]>
Cc: "MANIEZZO Marco (MM)" <[email protected]>,
[email protected]
Subject: Re: [PATCH 3/3][v2] wifi: Abort tethering enabling if bridge
creation/configuration fails
Message-ID:
<cafc8ijjg1+o1z5kte6-dq7zo5vywc7dzcqvfg34gt_twbvr...@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
Hi Daniel,
On Wed, Oct 26, 2016 at 9:22 AM, Daniel Wagner <[email protected]> wrote:
> On 10/25/2016 04:50 PM, Jose Blanquicet wrote:
>>
>> In case there is an error while setting up the bridge for WiFi tethering,
>> the
>> selected interface for tethering will remain removed and could not be use
>> anymore. This patch aims to avoid such a situation by using
>> connman_technology_tethering_notify()'s return value to abort tethering
>> enabling thus do not unnecessary remove the interface in case of
something
>> was
>> wrong with bridge.
>>
>> ---
>> plugins/wifi.c | 24 +++++++++++++++++++-----
>> 1 file changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/plugins/wifi.c b/plugins/wifi.c
>> index 8bde9c0..68b231d 100644
>> --- a/plugins/wifi.c
>> +++ b/plugins/wifi.c
>> @@ -3138,13 +3138,14 @@ static void sta_remove_callback(int result,
>> DBG("ifname %s result %d ", info->ifname, result);
>>
>> if ((result < 0) || (info->wifi->ap_supported !=
>> WIFI_AP_SUPPORTED)) {
>> - info->wifi->tethering = true;
>> + info->wifi->tethering = false;
>> + connman_technology_tethering_notify(info->technology,
>> false);
>
>
> Isn't this is a bug fix, at least the 'tethering = false' even without
your
> change? I would prefer to have it in a separate patch.
Yes, this is actually a bug-fix thus I will split this into two separated
patches.
>>
>> g_free(info->ifname);
>> g_free(info->ssid);
>> g_free(info);
>>
>> - if (info->wifi->ap_supported == WIFI_AP_SUPPORTED){
>> + if (info->wifi->ap_supported == WIFI_AP_SUPPORTED) {
>
>
> Not really necessary to do whitespace fixes in this patch.
Actually, Tomasz suggested me to always run linux's checkpatch on my
patches before sending them, doing so that space is marked as error:
ERROR: space required before the open brace '{'
#3147: FILE: plugins/wifi.c:3147:
+ if (info->wifi->ap_supported == WIFI_AP_SUPPORTED){
Therefore, I just took advantage of this patch to fix it.
>> g_free(info->wifi->tethering_param->ssid);
>> g_free(info->wifi->tethering_param);
>> info->wifi->tethering_param = NULL;
>> @@ -3154,8 +3155,6 @@ static void sta_remove_callback(int result,
>>
>> info->wifi->interface = NULL;
>>
>> - connman_technology_tethering_notify(info->technology, true);
>> -
>> g_supplicant_interface_create(info->ifname, driver,
>> info->wifi->bridge,
>> ap_create_callback,
>> info);
>> @@ -3171,7 +3170,7 @@ static int enable_wifi_tethering(struct
>> connman_technology *technology,
>> struct wifi_tethering_info *info;
>> const char *ifname;
>> unsigned int mode;
>> - int err;
>> + int err, berr = 0;
>>
>> for (list = iface_list; list; list = list->next) {
>> wifi = list->data;
>> @@ -3230,6 +3229,10 @@ static int enable_wifi_tethering(struct
>> connman_technology *technology,
>> info->wifi->tethering = true;
>> info->wifi->ap_supported = WIFI_AP_SUPPORTED;
>>
>> + berr = connman_technology_tethering_notify(technology,
>> true);
>> + if (berr < 0)
>> + goto failed;
>> +
>> err = g_supplicant_interface_remove(interface,
>> sta_remove_callback,
>> info);
>> @@ -3244,6 +3247,17 @@ static int enable_wifi_tethering(struct
>> connman_technology *technology,
>> g_free(info);
>> g_free(wifi->tethering_param);
>> wifi->tethering_param = NULL;
>> +
>> + /*
>> + * Remove bridge if it was correctly created but remove
>> + * operation failed. Instead, if bridge creation failed
>> then
>> + * break out and do not try again on another interface,
>> + * bridge set-up does not depend on it.
>> + */
>> + if (berr == 0)
>> + connman_technology_tethering_notify(technology,
>> false);
>> + else
>> + break;
>> }
>>
>> return -EOPNOTSUPP;
>
>
> Okay, that looks much better. Still I am little bothered with complexity
> overall but that was before your patch already there. So if my question
> above it correct then I hope you can spin a v3 for me. Thanks!
Regards,
Jose Blanquicet
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.01.org/pipermail/connman/attachments/20161026/fcb00610/attachment-0001.html>
------------------------------
Message: 5
Date: Wed, 26 Oct 2016 09:59:43 +0200
From: Daniel Wagner <[email protected]>
To: Jose Blanquicet <[email protected]>
Cc: "MANIEZZO Marco (MM)" <[email protected]>,
[email protected]
Subject: Re: [PATCH 3/3][v2] wifi: Abort tethering enabling if bridge
creation/configuration fails
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
s result %d ", info->ifname, result);
>>>
>>> if ((result < 0) || (info->wifi->ap_supported !=
>>> WIFI_AP_SUPPORTED)) {
>>> - info->wifi->tethering = true;
>>> + info->wifi->tethering = false;
>>> + connman_technology_tethering_notify(info->technology,
>>> false);
>>
>>
>> Isn't this is a bug fix, at least the 'tethering = false' even without
> your
>> change? I would prefer to have it in a separate patch.
>
> Yes, this is actually a bug-fix thus I will split this into two
> separated patches.
Excellent.
>
>>>
>>> g_free(info->ifname);
>>> g_free(info->ssid);
>>> g_free(info);
>>>
>>> - if (info->wifi->ap_supported == WIFI_AP_SUPPORTED){
>>> + if (info->wifi->ap_supported == WIFI_AP_SUPPORTED) {
>>
>>
>> Not really necessary to do whitespace fixes in this patch.
>
> Actually, Tomasz suggested me to always run linux's checkpatch on my
> patches before sending them, doing so that space is marked as error:
>
> ERROR: space required before the open brace '{'
> #3147: FILE: plugins/wifi.c:3147:
> + if (info->wifi->ap_supported == WIFI_AP_SUPPORTED){
>
> Therefore, I just took advantage of this patch to fix it.
Ah, I didn't remember Tomasz suggestion. Okay, in this case let's take
the opportunity and fix it up :)
thanks,
daniel
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 12, Issue 28
***************************************
