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

[Daniel Wagner <wagi@monom.org>]

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")