connman.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH] service: Fix default service update on ready state
@ 2021-06-23 12:18 VAUTRIN Emmanuel (Canal Plus Prestataire)
  2021-06-24  8:07 ` Daniel Wagner
  0 siblings, 1 reply; 12+ messages in thread
From: VAUTRIN Emmanuel (Canal Plus Prestataire) @ 2021-06-23 12:18 UTC (permalink / raw)
  To: connman

On a ready state transition, the default service shall be re-evaluated
after the preferred service reordering, before being processed.
---
 src/service.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/service.c b/src/service.c
index eb74a85d6e32..cb0103fd7221 100644
--- a/src/service.c
+++ b/src/service.c
@@ -6051,12 +6051,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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] service: Fix default service update on ready state
  2021-06-23 12:18 [PATCH] service: Fix default service update on ready state VAUTRIN Emmanuel (Canal Plus Prestataire)
@ 2021-06-24  8:07 ` Daniel Wagner
  2021-06-24  9:01   ` VAUTRIN Emmanuel (Canal Plus Prestataire)
  0 siblings, 1 reply; 12+ messages in thread
From: Daniel Wagner @ 2021-06-24  8:07 UTC (permalink / raw)
  To: VAUTRIN Emmanuel (Canal Plus Prestataire); +Cc: connman

On Wed, Jun 23, 2021 at 12:18:13PM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> On a ready state transition, the default service shall be re-evaluated
> after the preferred service reordering, before being processed.

This is needed on top of my reorder patch?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] service: Fix default service update on ready state
  2021-06-24  8:07 ` Daniel Wagner
@ 2021-06-24  9:01   ` VAUTRIN Emmanuel (Canal Plus Prestataire)
  2021-07-02  7:27     ` Daniel Wagner
  0 siblings, 1 reply; 12+ messages in thread
From: VAUTRIN Emmanuel (Canal Plus Prestataire) @ 2021-06-24  9:01 UTC (permalink / raw)
  To: Daniel Wagner; +Cc: connman

Hello Daniel,

> This is needed on top of my reorder patch?
Indeed.


B.R.

Emmanuel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] service: Fix default service update on ready state
  2021-06-24  9:01   ` VAUTRIN Emmanuel (Canal Plus Prestataire)
@ 2021-07-02  7:27     ` Daniel Wagner
  2021-07-02  7:41       ` Daniel Wagner
  0 siblings, 1 reply; 12+ messages in thread
From: Daniel Wagner @ 2021-07-02  7:27 UTC (permalink / raw)
  To: VAUTRIN Emmanuel (Canal Plus Prestataire); +Cc: connman

Hi Emmanual,

On Thu, Jun 24, 2021 at 09:01:36AM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> Hello Daniel,
> 
> > This is needed on top of my reorder patch?
> Indeed.

Okay, this one is easy to understand. Patch applied.

Thanks!
Daniel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] service: Fix default service update on ready state
  2021-07-02  7:27     ` Daniel Wagner
@ 2021-07-02  7:41       ` Daniel Wagner
  2021-07-02 15:43         ` VAUTRIN Emmanuel (Canal Plus Prestataire)
  0 siblings, 1 reply; 12+ messages in thread
From: Daniel Wagner @ 2021-07-02  7:41 UTC (permalink / raw)
  To: VAUTRIN Emmanuel (Canal Plus Prestataire); +Cc: connman

Hi Emmanuel,

On Fri, Jul 02, 2021 at 09:27:03AM +0200, Daniel Wagner wrote:
> > > This is needed on top of my reorder patch?
> > Indeed.
> 
> Okay, this one is easy to understand. Patch applied.

After looking at your other patches I dropped the patch.

By moving default_changed() after

	def_service = connman_service_get_default();
	service_update_preferred_order(def_service, service, new_state);

the result is the code doesn't make sense anymore. Why calling
service_update_preferred_order *before* we change the default?

This is the very reason I am pushing back against these changes. They
look innocent and might even work for some cases, but anyone later
looking at this code will scratch his head and will say "this doesn't
make sense". Also The commit message saying "the default shall be
re-evaluated" doesn't help either. You need to explain why you need to
change the order and why service_update_preferred_order() should operate
on the 'outdated' default route.

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] service: Fix default service update on ready state
  2021-07-02  7:41       ` Daniel Wagner
@ 2021-07-02 15:43         ` VAUTRIN Emmanuel (Canal Plus Prestataire)
  2021-08-25 12:52           ` VAUTRIN Emmanuel (Canal Plus Prestataire)
  0 siblings, 1 reply; 12+ messages in thread
From: VAUTRIN Emmanuel (Canal Plus Prestataire) @ 2021-07-02 15:43 UTC (permalink / raw)
  To: Daniel Wagner; +Cc: connman

> By moving default_changed() after
>        def_service = connman_service_get_default();
>        service_update_preferred_order(def_service, service, new_state);
>
> the result is the code doesn't make sense anymore. Why calling
> service_update_preferred_order *before* we change the default?
>
> This is the very reason I am pushing back against these changes. They
> look innocent and might even work for some cases, but anyone later
> looking at this code will scratch his head and will say "this doesn't
> make sense". Also The commit message saying "the default shall be
> re-evaluated" doesn't help either. You need to explain why you need to
> change the order and why service_update_preferred_order() should operate
> on the 'outdated' default route.
Daniel,

I shall admit this part is really complex, with separated updates of the 
default service and the service list, so maybe it is not the best choice.
I propose to run my full test campaign again and check for a better solution
(or at least to describe the issues I am facing), after the completion of the topic
"service: Ignore state information in service reordering".


Best Regards,

Emmanuel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] service: Fix default service update on ready state
  2021-07-02 15:43         ` VAUTRIN Emmanuel (Canal Plus Prestataire)
@ 2021-08-25 12:52           ` VAUTRIN Emmanuel (Canal Plus Prestataire)
  2021-08-29 19:07             ` Daniel Wagner
  0 siblings, 1 reply; 12+ messages in thread
From: VAUTRIN Emmanuel (Canal Plus Prestataire) @ 2021-08-25 12:52 UTC (permalink / raw)
  To: Daniel Wagner; +Cc: connman

Hi Daniel,

> I propose to run my full test campaign again and check for a better solution
> (or at least to describe the issues I am facing), after the completion of the topic
> "service: Ignore state information in service reordering".
I confirm this patch is still necessary, and still makes sense.

Best Regards,

Emmanuel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] service: Fix default service update on ready state
  2021-08-25 12:52           ` VAUTRIN Emmanuel (Canal Plus Prestataire)
@ 2021-08-29 19:07             ` Daniel Wagner
  2021-08-30  7:31               ` VAUTRIN Emmanuel (Canal Plus Prestataire)
  0 siblings, 1 reply; 12+ messages in thread
From: Daniel Wagner @ 2021-08-29 19:07 UTC (permalink / raw)
  To: VAUTRIN Emmanuel (Canal Plus Prestataire); +Cc: connman

Hi Emmanuel,

On Wed, Aug 25, 2021 at 12:52:24PM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> > I propose to run my full test campaign again and check for a better solution
> > (or at least to describe the issues I am facing), after the completion of the topic
> > "service: Ignore state information in service reordering".
> I confirm this patch is still necessary, and still makes sense.

Right, after being absent for a while (sorry) and rereading my argument
in my last email I checked the code base again more closely. And I see
where my thinko is. We have to sort against the old default for the
preferred settings. Because we don't know yet the order according the
user's preference. After we have done that we update the default route.
Very tricky stuff.

Thanks for your patience.
Daniel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] service: Fix default service update on ready state
  2021-08-29 19:07             ` Daniel Wagner
@ 2021-08-30  7:31               ` VAUTRIN Emmanuel (Canal Plus Prestataire)
  2021-08-30  8:13                 ` Daniel Wagner
  0 siblings, 1 reply; 12+ messages in thread
From: VAUTRIN Emmanuel (Canal Plus Prestataire) @ 2021-08-30  7:31 UTC (permalink / raw)
  To: Daniel Wagner; +Cc: connman

> Right, after being absent for a while (sorry) and rereading my argument
> in my last email I checked the code base again more closely. And I see
> where my thinko is. We have to sort against the old default for the
> preferred settings. Because we don't know yet the order according the
> user's preference. After we have done that we update the default route.
> Very tricky stuff.

> Thanks for your patience.
> Daniel

Great news Daniel, that we could finally converge, thank you for your time.

Emmanuel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] service: Fix default service update on ready state
  2021-08-30  7:31               ` VAUTRIN Emmanuel (Canal Plus Prestataire)
@ 2021-08-30  8:13                 ` Daniel Wagner
  2021-08-30  8:28                   ` VAUTRIN Emmanuel (Canal Plus Prestataire)
  0 siblings, 1 reply; 12+ messages in thread
From: Daniel Wagner @ 2021-08-30  8:13 UTC (permalink / raw)
  To: VAUTRIN Emmanuel (Canal Plus Prestataire); +Cc: connman

Hi Emmanuel,

On Mon, Aug 30, 2021 at 07:31:12AM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> > Right, after being absent for a while (sorry) and rereading my argument
> > in my last email I checked the code base again more closely. And I see
> > where my thinko is. We have to sort against the old default for the
> > preferred settings. Because we don't know yet the order according the
> > user's preference. After we have done that we update the default route.
> > Very tricky stuff.
> 
> > Thanks for your patience.
> > Daniel
> 
> Great news Daniel, that we could finally converge, thank you for your time.

No problem. I know I am bottleneck with upstream work. Sorry about
that. Now I just hope we didn't introduce new regressions :)

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH] service: Fix default service update on ready state
  2021-08-30  8:13                 ` Daniel Wagner
@ 2021-08-30  8:28                   ` VAUTRIN Emmanuel (Canal Plus Prestataire)
  2021-08-30  8:44                     ` Daniel Wagner
  0 siblings, 1 reply; 12+ messages in thread
From: VAUTRIN Emmanuel (Canal Plus Prestataire) @ 2021-08-30  8:28 UTC (permalink / raw)
  To: Daniel Wagner; +Cc: connman

> No problem. I know I am bottleneck with upstream work. Sorry about
> that. Now I just hope we didn't introduce new regressions :)

I can imagine such a big time-consumer it is!
The great news are that I have no more pending patches.
And it is right on time, because I don't know how much time
I will keep working on a project integrating ConnMan.
If I can, I will try to integrate iwd and run my full test campaign with it,
 to check the regressions, if any.

Best Regards,

Emmanuel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] service: Fix default service update on ready state
  2021-08-30  8:28                   ` VAUTRIN Emmanuel (Canal Plus Prestataire)
@ 2021-08-30  8:44                     ` Daniel Wagner
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel Wagner @ 2021-08-30  8:44 UTC (permalink / raw)
  To: VAUTRIN Emmanuel (Canal Plus Prestataire); +Cc: connman

On Mon, Aug 30, 2021 at 08:28:29AM +0000, VAUTRIN Emmanuel (Canal Plus Prestataire) wrote:
> > No problem. I know I am bottleneck with upstream work. Sorry about
> > that. Now I just hope we didn't introduce new regressions :)
> 
> I can imagine such a big time-consumer it is!

Well, the main issue is lacking main power on this project. We are
clearly in the maintenance phase. Sometimes there is a new feature but
overall the development has pretty much stalled. I try to keep an eye on
the incoming patches but that's about it what I am able and willing to
spend on the project. I wouldn't mind if someone else wants to
maintain/develop this project but so far no one has showed reasonable
interest.

> The great news are that I have no more pending patches.

:)

> And it is right on time, because I don't know how much time
> I will keep working on a project integrating ConnMan.
> If I can, I will try to integrate iwd and run my full test campaign with it,
>  to check the regressions, if any.

Great to hear!

Thanks for your contributions,
Daniel

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-08-30  8:44 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-23 12:18 [PATCH] service: Fix default service update on ready state VAUTRIN Emmanuel (Canal Plus Prestataire)
2021-06-24  8:07 ` Daniel Wagner
2021-06-24  9:01   ` VAUTRIN Emmanuel (Canal Plus Prestataire)
2021-07-02  7:27     ` Daniel Wagner
2021-07-02  7:41       ` Daniel Wagner
2021-07-02 15:43         ` VAUTRIN Emmanuel (Canal Plus Prestataire)
2021-08-25 12:52           ` VAUTRIN Emmanuel (Canal Plus Prestataire)
2021-08-29 19:07             ` Daniel Wagner
2021-08-30  7:31               ` VAUTRIN Emmanuel (Canal Plus Prestataire)
2021-08-30  8:13                 ` Daniel Wagner
2021-08-30  8:28                   ` VAUTRIN Emmanuel (Canal Plus Prestataire)
2021-08-30  8:44                     ` Daniel Wagner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).