787406 - B2G Network Manager: Allow only one default route at the same time

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Shian-Yow Wu [:swu]]

This is a follow up bug as mentioned in bug 762426 comment 17.
Network manager adds default route only for preferred network interface and removes it for old interface.  But it's possible that other Android native processes (rild or wpa_supplicant) add default route for a newly activated interface, which is unwanted by network manager.

Comment 1

[Shian-Yow Wu [:swu]]

Attachment: WIP V1

To be sure only one default route existing, remove any pre-created default route which may or may not be created by Network Manager.  Then let NetworkManager.setAndConfigure() to set default route only on preferred network.

I tested it with one Wifi and one data call.  When wifi is preferred network, only one default route wlan0 existing.

root@android:/ # ip route                                                      
default via 192.168.1.1 dev wlan0 
187.88.243.160/30 dev rmnet0  proto kernel  scope link  src 187.88.243.162 
192.168.1.0/24 dev wlan0  proto kernel  scope link  src 192.168.1.18  metric 313

Comment 2

[Philipp von Weitershausen [:philikon]]

Comment on attachment 657299 [details] [diff] [review]
WIP V1

Review of attachment 657299 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/NetworkManager.js
@@ +173,5 @@
>                this.removeHostRoute(network);
>              }
> +            // Remove pre-created default route and let this.setAndConfigure decide
> +            // whether we need to add it or not.
> +            this.removeDefaultRoute(network.name);

Do we really need this for DISCONNECTED? I think we probably only need it for CONNECTED and REGISTERED, no?

But then again, if the call isn't harmful, then I suppose we can leave it here.

Comment 3

[Shian-Yow Wu [:swu]]

(In reply to Philipp von Weitershausen [:philikon] from comment #2)
> Do we really need this for DISCONNECTED? I think we probably only need it
> for CONNECTED and REGISTERED, no?
> 
> But then again, if the call isn't harmful, then I suppose we can leave it
> here.

It's there only for code consistency and safety reason, as it doesn't harm to call again.  But after more thinking, I think it's better add it only for CONNECTED and REGISTERED to make the code simpler.  Thanks for your comments!

Comment 4

[Shian-Yow Wu [:swu]]

Attachment: Patch V2

Addressed previous comments.  r=philikon.

Comment 5

[Shian-Yow Wu [:swu]]

Attachment: Patch V2

Comment 6

[Shian-Yow Wu [:swu]]

Attachment: Patch V2, r=philikon

Comment 7

[Shian-Yow Wu [:swu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85d7349e7086

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85d7349e7086