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 1/3] tethering: Propagate error value when
enabling tethering (Daniel Wagner)
2. Re: [PATCH 2/3] technology: Handle errors when enabling
tethering (Daniel Wagner)
3. Re: [PATCH][v2] tethering: Add verification to bridge
creation and configuration (Daniel Wagner)
4. Re: [PATCH 3/3] wifi: Abort tethering enabling if bridge
creation/configuration fails (Jose Blanquicet)
5. Re: [PATCH 3/3] wifi: Abort tethering enabling if bridge
creation/configuration fails (Daniel Wagner)
6. Re: [PATCH 3/3] wifi: Abort tethering enabling if bridge
creation/configuration fails (Jose Blanquicet)
7. Re: [PATCH 3/3] wifi: Abort tethering enabling if bridge
creation/configuration fails (Daniel Wagner)
----------------------------------------------------------------------
Message: 1
Date: Tue, 25 Oct 2016 10:25:50 +0200
From: Daniel Wagner <[email protected]>
To: [email protected]
Subject: Re: [PATCH 1/3] tethering: Propagate error value when
enabling tethering
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
On 10/24/2016 04:21 PM, Jose Blanquicet wrote:
> Add return value to __connman_tethering_set_enabled() in order to propagate
> errors and allow connman_technology_tethering_notify() to not notify tethering
> as enabled when actually it does not because bridge creation fails.
Applied.
Thanks,
Daniel
------------------------------
Message: 2
Date: Tue, 25 Oct 2016 10:26:04 +0200
From: Daniel Wagner <[email protected]>
To: Jose Blanquicet <[email protected]>, [email protected],
[email protected]
Subject: Re: [PATCH 2/3] technology: Handle errors when enabling
tethering
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
On 10/24/2016 04:21 PM, Jose Blanquicet wrote:
> Use __connman_tethering_set_enabled()'s return value to verify if there was
> an error while creating/configuring the bridge in order to not wrongly notify
> that tethering was enabled and allow plugins to abort tethering enabling in
> case of something was wrong.
Applied.
Thanks,
Daniel
------------------------------
Message: 3
Date: Tue, 25 Oct 2016 10:26:57 +0200
From: Daniel Wagner <[email protected]>
To: Jose Blanquicet <[email protected]>, [email protected],
[email protected]
Subject: Re: [PATCH][v2] tethering: Add verification to bridge
creation and configuration
Message-ID: <[email protected]>
Content-Type: text/plain; charset=windows-1252; format=flowed
On 10/24/2016 04:21 PM, Jose Blanquicet wrote:
> Version 2:
> - Return 0 (No error) in case __sync_fetch_and_add(&tethering_enabled, 1) !=
> 0.
> - Split up PATCH v1 into multiple patches as requested.
>
> As I told before, use this verification in others tethering technologies
> should
> be evaluated and implemented in case it is advantageous. I will let this in
> the
> hands of someone with more expertise/knowledge on those other technologies. I
> only focused on WiFi. Should I add also the TODO comments into the other
> plugins
> as I had done for v1?
Please send a patch which adds an TODO entry to our TODO file, so we
don't forget it.
Thanks,
Daniel
------------------------------
Message: 4
Date: Tue, 25 Oct 2016 10:03:01 +0000
From: Jose Blanquicet <[email protected]>
To: Daniel Wagner <[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:
<CAFC8iJKvZOi+nqLMkUd1EzPHpJsM9ojm4_ZDac3_8J=ynkw...@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Hi Daniel,
>> @@ -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, bridge_result = 0;
>
>
> I don't think you need to initialize bridge_result. And it is quite a long
> name for a variable. Since the rest of the file is using short names, please
> try stick to the style, maybe berr or so.
You are right, initialization is not required here. On the other hand,
berr is also fine for me, I will take this into account for future
patches. I will send this again with the variable name.
>>
>> for (list = iface_list; list; list = list->next) {
>> wifi = list->data;
>> @@ -3230,6 +3229,11 @@ static int enable_wifi_tethering(struct
>> connman_technology *technology,
>> info->wifi->tethering = true;
>> info->wifi->ap_supported = WIFI_AP_SUPPORTED;
>>
>> + bridge_result =
>> + connman_technology_tethering_notify(technology,
>> true);
>> + if (bridge_result < 0)
>> + goto failed;
>> +
>> err = g_supplicant_interface_remove(interface,
>> sta_remove_callback,
>> info);
>> @@ -3244,6 +3248,15 @@ static int enable_wifi_tethering(struct
>> connman_technology *technology,
>> g_free(info);
>> g_free(wifi->tethering_param);
>> wifi->tethering_param = NULL;
>> +
>> + if (bridge_result < 0) {
>> + /*
>> + * If it was an error while setting up the bridge
>> then
>> + * do not try again on another interface, bridge
>> set-up
>> + * does not depend on it.
>> + */
>> + break;
>> + }
>
>
> No I am wondering why we should call connman_technology_tethering_notify()
> several times? Wouldn't it make sense to do it only once?
This function further than notify Tethering property has changed, it
also creates/configures the bridge which we need to be ready in
sta_remove_callback() because in this function we recreate the same
wpa_s interface but bridged. That is the reason this function is
called from the very beginning and needs to be called each time we
found there was an error to remove the bridge. I think what you mean
is why we just enable Tethering property once at the end of the whole
process. If so, you are right but then we would need to split this
function into two where one of them just enable Tethering property and
the other handle the bridge. However, the function that handle the
bridge should anyway be called each time we found there was an error.
Regards,
Jose Blanquicet
------------------------------
Message: 5
Date: Tue, 25 Oct 2016 14:00:33 +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
>>> @@ -3244,6 +3248,15 @@ static int enable_wifi_tethering(struct
>>> connman_technology *technology,
>>> g_free(info);
>>> g_free(wifi->tethering_param);
>>> wifi->tethering_param = NULL;
>>> +
>>> + if (bridge_result < 0) {
>>> + /*
>>> + * If it was an error while setting up the bridge
>>> then
>>> + * do not try again on another interface, bridge
>>> set-up
>>> + * does not depend on it.
>>> + */
>>> + break;
>>> + }
>>
>>
>> No I am wondering why we should call connman_technology_tethering_notify()
>> several times? Wouldn't it make sense to do it only once?
>
> This function further than notify Tethering property has changed, it
> also creates/configures the bridge which we need to be ready in
> sta_remove_callback() because in this function we recreate the same
> wpa_s interface but bridged. That is the reason this function is
> called from the very beginning and needs to be called each time we
> found there was an error to remove the bridge. I think what you mean
> is why we just enable Tethering property once at the end of the whole
> process. If so, you are right but then we would need to split this
> function into two where one of them just enable Tethering property and
> the other handle the bridge. However, the function that handle the
> bridge should anyway be called each time we found there was an error.
Let me show you why calling connnman_technology_tethering_notify()
in the loop is not useful:
int connman_technology_tethering_notify(struct connman_technology
*technology,
bool enabled)
{
int err;
DBG("technology %p enabled %u", technology, enabled);
if (technology->tethering == enabled)
return -EALREADY;
if (enabled) {
err = __connman_tethering_set_enabled();
if (err < 0)
return err;
} else
__connman_tethering_set_disabled();
technology->tethering = enabled;
tethering_changed(technology);
return 0;
}
After the first successful call with enabled=true it is a no op (return
-EALREDAY). Hmm, even worse, since you will break out with
if (bridge_result < 0)
break
you will only enable the first interface. I suggest you call
connman_technology_tethering_notify() before the while loop
or after. Depending what is simpler to do (error handling).
cheers,
daniel
------------------------------
Message: 6
Date: Tue, 25 Oct 2016 13:01:10 +0000
From: Jose Blanquicet <[email protected]>
To: Daniel Wagner <[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:
<CAFC8iJ+oXRKLhmzLnBKogP1e16-n21bv3Cr=vMrF2S_=ugi...@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
Hi Daniel,
>>> No I am wondering why we should call connman_technology_tethering_notify()
several times? Wouldn't it make sense to do it only once?
>>
>>
>> This function further than notify Tethering property has changed, it
>> also creates/configures the bridge which we need to be ready in
>> sta_remove_callback() because in this function we recreate the same
>> wpa_s interface but bridged. That is the reason this function is
>> called from the very beginning and needs to be called each time we
>> found there was an error to remove the bridge. I think what you mean
>> is why we just enable Tethering property once at the end of the whole
>> process. If so, you are right but then we would need to split this
>> function into two where one of them just enable Tethering property and
>> the other handle the bridge. However, the function that handle the
>> bridge should anyway be called each time we found there was an error.
>
>
> Let me show you why calling connnman_technology_tethering_notify()
> in the loop is not useful:
>
> int connman_technology_tethering_notify(struct connman_technology
*technology,
> bool enabled)
> {
> int err;
>
> DBG("technology %p enabled %u", technology, enabled);
>
> if (technology->tethering == enabled)
> return -EALREADY;
>
> if (enabled) {
> err = __connman_tethering_set_enabled();
> if (err < 0)
> return err;
> } else
> __connman_tethering_set_disabled();
>
> technology->tethering = enabled;
> tethering_changed(technology);
>
> return 0;
> }
>
> After the first successful call with enabled=true it is a no op (return
-EALREDAY). Hmm, even worse, since you will break out with
>
> if (bridge_result < 0)
> break
>
> you will only enable the first interface. I suggest you call
> connman_technology_tethering_notify() before the while loop
> or after. Depending what is simpler to do (error handling).
Now I got your point. However, I do not know why you continue iterating in
the loop after the first successful call, we only want to enable tethering
on one interface (The first one that supports tethering). That's the reason
loop return when everything went well:
err = g_supplicant_interface_remove(interface,
sta_remove_callback,
info);
if (err >= 0) {
DBG("tethering wifi %p ifname %s", wifi, ifname);
return 0;
}
Therefore, connman_technology_tethering_notify() will get called only once.
However, what you made me notice is that I have to remove the bridge in
case g_supplicant_interface_remove() fails.
Is there something I am missing?
Regards,
Jose Blanquicet
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.01.org/pipermail/connman/attachments/20161025/dc3c69a8/attachment-0001.html>
------------------------------
Message: 7
Date: Tue, 25 Oct 2016 15:54:50 +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
On 10/25/2016 03:01 PM, Jose Blanquicet wrote:
>> After the first successful call with enabled=true it is a no op
> (return -EALREDAY). Hmm, even worse, since you will break out with
>>
>> if (bridge_result < 0)
>> break
>>
>> you will only enable the first interface. I suggest you call
>> connman_technology_tethering_notify() before the while loop
>> or after. Depending what is simpler to do (error handling).
>
> Now I got your point. However, I do not know why you continue iterating
> in the loop after the first successful call, we only want to enable
> tethering on one interface (The first one that supports tethering).
> That's the reason loop return when everything went well:
>
> err = g_supplicant_interface_remove(interface,
> sta_remove_callback,
> info);
> if (err >= 0) {
> DBG("tethering wifi %p ifname %s", wifi, ifname);
> return 0;
> }
>
> Therefore, connman_technology_tethering_notify() will get called only
> once. However, what you made me notice is that I have to remove the
> bridge in case g_supplicant_interface_remove() fails.
>
> Is there something I am missing?
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.
cheers,
daniel
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list
[email protected]
https://lists.01.org/mailman/listinfo/connman
------------------------------
End of connman Digest, Vol 12, Issue 26
***************************************
