Closed Bug 787406 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: swu, Assigned: swu)

Details

(Whiteboard: [LOE:S])

Attachments

(1 file, 3 obsolete files)

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.
Attached patch WIP V1 (obsolete) — Splinter Review
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
Attachment #657299 - Flags: review?(philipp)
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.
Attachment #657299 - Flags: review?(philipp) → review+
(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!
Attached file Patch V2 (obsolete) —
Addressed previous comments.  r=philikon.
Assignee: nobody → swu
Attachment #657299 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch Patch V2 (obsolete) — Splinter Review
Attachment #658367 - Attachment is obsolete: true
Attachment #658368 - Flags: review+
Attachment #658368 - Attachment is obsolete: true
Whiteboard: [LOE:S]
https://hg.mozilla.org/mozilla-central/rev/85d7349e7086
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.