[PATCH] service: Fix preferred service reordering on ready state

[PATCH] service: Fix preferred service reordering on ready state

[VAUTRIN Emmanuel (Canal Plus Prestataire)]

When a service reaches the ready state, the service list shall always
be reordered according to the PreferredTechnologies config options.

This issue can be reproduced with PreferredTechnologies=ethernet,wifi,
by following the steps above:
1. Starting, Ethernet (E) plugged, with a known Wifi (W) network.
E idle -> online, W idle -> ready
2. Unplug Ethernet.
E online -> idle, W ready -> online
3. Plug Ethernet.
E idle -> ready, W online
Even if Ethernet has the highest priority, it will never be online,
except if Wifi connection is lost.

Fixes: e600366f6035 ("service: Start online check on IP address update")
---
 src/service.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/src/service.c b/src/service.c
index 20917a8923a4..cadd9fc76881 100644
--- a/src/service.c
+++ b/src/service.c
@@ -4711,15 +4711,11 @@ static void apply_relevant_default_downgrade(struct connman_service *service)
 	struct connman_service *def_service;
 
 	def_service = connman_service_get_default();
-	if (!def_service)
+	if (!def_service || def_service != service ||
+		def_service->state != CONNMAN_SERVICE_STATE_ONLINE)
 		return;
 
-	if (def_service == service &&
-			def_service->state == CONNMAN_SERVICE_STATE_ONLINE) {
-		def_service->state = CONNMAN_SERVICE_STATE_READY;
-		__connman_notifier_leave_online(def_service->type);
-		state_changed(def_service);
-	}
+	downgrade_state(def_service);
 }
 
 static void switch_default_service(struct connman_service *default_service,
@@ -5898,8 +5894,7 @@ static int service_update_preferred_order(struct connman_service *default_servic
 	unsigned int *tech_array;
 	int i;
 
-	if (!default_service || default_service == new_service ||
-			default_service->state != new_state)
+	if (!default_service || default_service == new_service)
 		return 0;
 
 	tech_array = connman_setting_get_uint_list("PreferredTechnologies");
@@ -6051,12 +6046,12 @@ static int service_indicate_state(struct connman_service *service)
 
 		service->new_service = false;
 
-		default_changed();
-
 		def_service = connman_service_get_default();
 
 		service_update_preferred_order(def_service, service, new_state);
 
+		default_changed();
+
 		__connman_service_set_favorite(service, true);
 
 		reply_pending(service, 0);
-- 
2.25.1

Re: [PATCH] service: Fix preferred service reordering on ready state

[Jussi Laakkonen]

Hi Emmanuel,

Wouldn't this be more simpler to be done service_compare() utilizing the 
PreferredTechnologies setting as I assume this is the issue you are 
trying to resolve:
https://git.kernel.org/pub/scm/network/connman/connman.git/tree/src/service.c#n5353 
?

Cheers,
  Jussi


On 6/14/21 12:41 PM, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> When a service reaches the ready state, the service list shall always
> be reordered according to the PreferredTechnologies config options.
> 
> This issue can be reproduced with PreferredTechnologies=ethernet,wifi,
> by following the steps above:
> 1. Starting, Ethernet (E) plugged, with a known Wifi (W) network.
> E idle -> online, W idle -> ready
> 2. Unplug Ethernet.
> E online -> idle, W ready -> online
> 3. Plug Ethernet.
> E idle -> ready, W online
> Even if Ethernet has the highest priority, it will never be online,
> except if Wifi connection is lost.
> 
> Fixes: e600366f6035 ("service: Start online check on IP address update")
> ---
>   src/service.c | 17 ++++++-----------
>   1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/src/service.c b/src/service.c
> index 20917a8923a4..cadd9fc76881 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -4711,15 +4711,11 @@ static void apply_relevant_default_downgrade(struct connman_service *service)
>   	struct connman_service *def_service;
>   
>   	def_service = connman_service_get_default();
> -	if (!def_service)
> +	if (!def_service || def_service != service ||
> +		def_service->state != CONNMAN_SERVICE_STATE_ONLINE)
>   		return;
>   
> -	if (def_service == service &&
> -			def_service->state == CONNMAN_SERVICE_STATE_ONLINE) {
> -		def_service->state = CONNMAN_SERVICE_STATE_READY;
> -		__connman_notifier_leave_online(def_service->type);
> -		state_changed(def_service);
> -	}
> +	downgrade_state(def_service);
>   }
>   
>   static void switch_default_service(struct connman_service *default_service,
> @@ -5898,8 +5894,7 @@ static int service_update_preferred_order(struct connman_service *default_servic
>   	unsigned int *tech_array;
>   	int i;
>   
> -	if (!default_service || default_service == new_service ||
> -			default_service->state != new_state)
> +	if (!default_service || default_service == new_service)
>   		return 0;
>   
>   	tech_array = connman_setting_get_uint_list("PreferredTechnologies");
> @@ -6051,12 +6046,12 @@ static int service_indicate_state(struct connman_service *service)
>   
>   		service->new_service = false;
>   
> -		default_changed();
> -
>   		def_service = connman_service_get_default();
>   
>   		service_update_preferred_order(def_service, service, new_state);
>   
> +		default_changed();
> +
>   		__connman_service_set_favorite(service, true);
>   
>   		reply_pending(service, 0);
> 

RE: [PATCH] service: Fix preferred service reordering on ready state

[VAUTRIN Emmanuel (Canal Plus Prestataire)]

Hi Jussi,

> Wouldn't this be more simpler to be done service_compare() utilizing the 
> PreferredTechnologies setting as I assume this is the issue you are 
> trying to resolve:
As I said in the "[PATCH 0/1] start online check even if service is not
the default service" former thread,  I have also worked on this issue,
and I presume the reason is not related to the reordering itself, but is
more a state update matter.
I let Daniel and Christophe Ronco test and check my changes carefully, 
but I am pretty convinced that fixing the service_compare is not enough.


Best Regards,

Emmanuel
 

Re: [PATCH] service: Fix preferred service reordering on ready state

[Jussi Laakkonen]

Hi Emmanuel,

On 6/15/21 3:35 PM, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> Hi Jussi,
> 
>> Wouldn't this be more simpler to be done service_compare() utilizing the
>> PreferredTechnologies setting as I assume this is the issue you are
>> trying to resolve:
> As I said in the "[PATCH 0/1] start online check even if service is not
> the default service" former thread,  I have also worked on this issue,
> and I presume the reason is not related to the reordering itself, but is
> more a state update matter.
> I let Daniel and Christophe Ronco test and check my changes carefully,
> but I am pretty convinced that fixing the service_compare is not enough.
> 

The thing is that I've been doing similar work on our fork and the end 
result turned out to be fixing the sorting of the services. It does not 
anyways seem to be quite feasible to first let the sorting re-arrange 
services, then with these changes make exceptions based on a specific 
state, which then will change after the services are again sorted?

I haven't submitted our changes to upstream just mainly because of 
historic reasons there is a bit of a different approach in use with 
service.c. This hopefully changes when there is time to check all things 
and get the fork closer to upstream. Our for does involve a slightly 
stricter approach which you can see here 
https://github.com/sailfishos/connman/blob/master/connman/src/service.c#L6722 
and the unit tests (that are not yet 100% complete) 
https://github.com/sailfishos/connman/blob/master/connman/unit/test-service.c 


But since this is about allowing to follow the preferred technologies 
list that is being set why not do it in the place where the list is 
already utilized for sorting? I mean that as there is a clear reference 
to prefer online over ready state so the preferred technologies list 
should be as an exception in this regard.

Your approach works but I'm not sure if there are some corner cases 
induced by this approach. These are my notes on this matter, I hope you 
consider this proposition.

Cheers,
  Jussi

RE: [PATCH] service: Fix preferred service reordering on ready state

[VAUTRIN Emmanuel (Canal Plus Prestataire)]

Hi Jussi,

> The thing is that I've been doing similar work on our fork and the end 
> result turned out to be fixing the sorting of the services.
Great.

> It does not anyways seem to be quite feasible to first let the sorting re-arrange 
> services, then with these changes make exceptions based on a specific 
> state, which then will change after the services are again sorted?
> I haven't submitted our changes to upstream just mainly because of 
> historic reasons there is a bit of a different approach in use with 
> service.c. This hopefully changes when there is time to check all things 
> and get the fork closer to upstream. Our for does involve a slightly 
> stricter approach [...]
> and the unit tests (that are not yet 100% complete) [...]
I have no opinion about that, my knowledge of this part is to light,
and I have just a limited time to spend on it. I let Daniel check this part.

> But since this is about allowing to follow the preferred technologies 
> list that is being set why not do it in the place where the list is 
> already utilized for sorting? I mean that as there is a clear reference 
> to prefer online over ready state so the preferred technologies list 
> should be as an exception in this regard.
> Your approach works but I'm not sure if there are some corner cases 
> induced by this approach. These are my notes on this matter, I hope you 
> consider this proposition.
As first step, I have just proposed a direct fix based on current implementation
(also fixing the service state update notification), in order to limit
regressions and quickly be integrated.
In my opinion, the second step will consist in a more consequent work,
based directly on the sorting itself, what Daniel has probably also started
by his side, but of course, I let him decide the best way to follow. If required,
I can try spending some time to test the final solution based on your both works.

Best Regards,

Emmanuel

Re: [PATCH] service: Fix preferred service reordering on ready state

[Daniel Wagner]

Hi,

On Thu, Jun 17, 2021 at 10:22:18AM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> > The thing is that I've been doing similar work on our fork and the end 
> > result turned out to be fixing the sorting of the services.
> Great.
> 
> > It does not anyways seem to be quite feasible to first let the sorting re-arrange 
> > services, then with these changes make exceptions based on a specific 
> > state, which then will change after the services are again sorted?
> > I haven't submitted our changes to upstream just mainly because of 
> > historic reasons there is a bit of a different approach in use with 
> > service.c. This hopefully changes when there is time to check all things 
> > and get the fork closer to upstream. Our for does involve a slightly 
> > stricter approach [...]
> > and the unit tests (that are not yet 100% complete) [...]
> I have no opinion about that, my knowledge of this part is to light,
> and I have just a limited time to spend on it. I let Daniel check this
> part.

The whole sorting logic is a overloaded with corner cases. So I am very
careful with any change in this area.

> > But since this is about allowing to follow the preferred technologies 
> > list that is being set why not do it in the place where the list is 
> > already utilized for sorting? I mean that as there is a clear reference 
> > to prefer online over ready state so the preferred technologies list 
> > should be as an exception in this regard.
> > Your approach works but I'm not sure if there are some corner cases 
> > induced by this approach. These are my notes on this matter, I hope you 
> > consider this proposition.
> As first step, I have just proposed a direct fix based on current implementation
> (also fixing the service state update notification), in order to limit
> regressions and quickly be integrated.
> In my opinion, the second step will consist in a more consequent work,
> based directly on the sorting itself, what Daniel has probably also started
> by his side, but of course, I let him decide the best way to follow. If required,
> I can try spending some time to test the final solution based on your both works.

There were already not trivial fixes for this section of the code
[1]. It clearly shows that there a lot of corner cases to be
considered. While Emmanuel's change looks innocent, I can't really say
it doesn't have any unwanted side effects. As Jussi points out, the
initial problem is clearly in service_compare() not utilizing the
PreferredTechnologies setting. My suggestion is to use fix first the
sorting as this seems to be something we can agree on it.

Can we agree on this one:

   https://lore.kernel.org/connman/20210621072036.302-1-wagi@monom.org/

? And fix the remaining problems in tiny steps?

Thanks,
Daniel

[1]
5bd396590fdf ("service: Send state changed signal after downgrading state")
82c0faebfe4d ("service: Notify leaving online state when downgrading it to ready")

RE: [PATCH] service: Fix preferred service reordering on ready state

[VAUTRIN Emmanuel (Canal Plus Prestataire)]

Hi Daniel,

Great to see you back.

> The whole sorting logic is a overloaded with corner cases. So I am very
> careful with any change in this area.
Of course.

> There were already not trivial fixes for this section of the code
> [1]. It clearly shows that there a lot of corner cases to be
> considered. While Emmanuel's change looks innocent, I can't really say
> it doesn't have any unwanted side effects. As Jussi points out, the
> initial problem is clearly in service_compare() not utilizing the
> PreferredTechnologies setting. My suggestion is to use fix first the
> sorting as this seems to be something we can agree on it.
>
> Can we agree on this one:
> https://lore.kernel.org/connman/20210621072036.302-1-wagi@monom.org/
> 
> ? And fix the remaining problems in tiny steps?
>
> [1]
> 5bd396590fdf ("service: Send state changed signal after downgrading state")
> 82c0faebfe4d ("service: Notify leaving online state when downgrading it to ready")
Allright, I will (re-)check this patch, and propose, if necessary, the relevant fixes
of my previous patch.

Best Regards,

Emmanuel

RE: [PATCH] service: Fix preferred service reordering on ready state

[VAUTRIN Emmanuel (Canal Plus Prestataire)]

Hello Daniel,

> My suggestion is to use fix first the
> sorting as this seems to be something we can agree on it.
I have tested your patch integrated on a 1.40, and I still reproduce
the problem quickly, only by unplugging / plugging the Ethernet repeatedly,
with variable delays (less than 5s).
In this case, when I try to connect manually to a second Wifi network,
the Ethernet gets back online, as expected.
That reinforces my belief in a  state update issue.

The good news is that I have not faced any regression with
this version (1.40 + patch, before I was on 1.39).


Best Regards,

Emmanuel