ConnMan network manager
 help / color / 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; 6+ 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	[flat|nested] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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)
  0 siblings, 0 replies; 6+ 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] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ 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)

ConnMan network manager

Archives are clonable:
	git clone --mirror https://lore.kernel.org/connman/0 connman/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 connman connman/ https://lore.kernel.org/connman \
		connman@lists.linux.dev
	public-inbox-index connman

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/dev.linux.lists.connman


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git