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

RESOLVED FIXED in mozilla18

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: swu, Assigned: swu)

Tracking

Trunk
mozilla18
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [LOE:S])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 657299 [details] [diff] [review]
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
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+
(Assignee)

Comment 3

6 years ago
(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!
(Assignee)

Comment 4

6 years ago
Created attachment 658367 [details]
Patch V2

Addressed previous comments.  r=philikon.
Assignee: nobody → swu
Attachment #657299 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 5

6 years ago
Created attachment 658368 [details] [diff] [review]
Patch V2
Attachment #658367 - Attachment is obsolete: true
Attachment #658368 - Flags: review+
(Assignee)

Comment 6

6 years ago
Created attachment 658370 [details] [diff] [review]
Patch V2, r=philikon
Attachment #658368 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Whiteboard: [LOE:S]
https://hg.mozilla.org/mozilla-central/rev/85d7349e7086
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.