Closed
Bug 1104664
Opened 10 years ago
Closed 10 years ago
[gonk-l] Some netutils APIs are not available in newer libsystem
Categories
(Firefox OS Graveyard :: GonkIntegration, defect)
Tracking
(feature-b2g:2.2+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)
People
(Reporter: seinlin, Assigned: hchang)
References
Details
(Keywords: crash, Whiteboard: [caf priority: p2][b2g-crash][caf-crash 468][CR 781047])
Attachments
(1 file, 11 obsolete files)
(deleted),
patch
|
edgar
:
review+
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
These functions are deprecated.
extern int ifc_add_host_route(const char *name, in_addr_t addr);
extern int ifc_remove_host_routes(const char *name);
extern int ifc_get_default_route(const char *ifname);
extern int ifc_set_default_route(const char *ifname, in_addr_t gateway);
extern int ifc_add_route(const char *name, const char *addr, int prefix_length, const char *gw);
extern int ifc_remove_route(const char *ifname, const char *dst, int prefix_length, const char *gw);
Assignee | ||
Comment 1•10 years ago
|
||
Looks like we need this:
http://androidxref.com/5.0.0_r2/xref/system/netd/server/CommandListener.cpp#1433
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → hchang
Assignee | ||
Comment 2•10 years ago
|
||
How android works:
1) Generate netid [1] (say 100)
2) Create physical network [2] (network create 100)
3) Add interface to network [3] (network interface add 100)
4) Add route [4] (network route add 100 wlan0 10.10.0.0/26 10.10.10.1)
[1] http://androidxref.com/5.0.0_r2/xref/frameworks/base/services/core/java/com/android/server/ConnectivityService.java#3539
[2] http://androidxref.com/5.0.0_r2/xref/frameworks/base/services/core/java/com/android/server/NetworkManagementService.java#1965
[3] http://androidxref.com/5.0.0_r2/xref/frameworks/base/services/core/java/com/android/server/NetworkManagementService.java#1999
[4] http://androidxref.com/5.0.0_r2/xref/frameworks/base/services/core/java/com/android/server/NetworkManagementService.java#952
Assignee | ||
Updated•10 years ago
|
Blocks: gonk-L-Wifi
Updated•10 years ago
|
Blocks: gonk-L-RIL
Assignee | ||
Comment 3•10 years ago
|
||
Following is what would AOSP do:
Boot
(wifi disabled)
E/CommandListener( 183): interface list
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth enable ...
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
V/CommandListener( 183): bwctrlcmd: argc=3 bandwidth setglobalalert ...
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
V/CommandListener( 183): bwctrlcmd: argc=3 bandwidth setglobalalert ...
E/CommandListener( 183): network permission user set android.permission.CONNECTIVITY_INTERNAL 10068 10033 10059 10002 10027 10053 10040 10001 10003 10052
E/CommandListener( 183): network permission user set android.permission.CONNECTIVITY_INTERNAL 10022 10008 10013 10067 10073 10076 10066 10081 10004 10024
E/CommandListener( 183): network permission user set android.permission.CONNECTIVITY_INTERNAL 10054 10045 10036 10020 10079 10034 10029 10063 10074 10057
E/CommandListener( 183): network permission user set android.permission.CONNECTIVITY_INTERNAL 10005 10048 10072 10028 10071 10078 10016 1001 1027 10058
E/CommandListener( 183): network permission user set android.permission.CONNECTIVITY_INTERNAL 10070 1000 10015 10077 10056 10025 10047 10060 10032 10014
E/CommandListener( 183): network permission user set android.permission.CONNECTIVITY_INTERNAL 10017 10035 10043 10011 10009 10061 10019 10080 10031 10018
E/CommandListener( 183): network permission user set android.permission.CONNECTIVITY_INTERNAL 10075 10082 10044 10042 1002 10055 10050 10023 10046 10064
E/CommandListener( 183): network permission user set android.permission.CONNECTIVITY_INTERNAL 10007 10030 10041 2000 10038 10039 10069 10010 10049 10065
E/CommandListener( 183): network permission user set android.permission.CONNECTIVITY_INTERNAL 10021 10012 10037 10051 10000 10006 10062 10026
enable wifi
E/CommandListener( 183): interface getcfg wlan0
E/CommandListener( 183): interface setcfg wlan0 0.0.0.0 0 multicast broadcast down
D/CommandListener( 183): Setting iface cfg
D/CommandListener( 183): Trying to bring down wlan0
E/CommandListener( 183): interface clearaddrs wlan0
D/CommandListener( 183): Clearing all IP addresses on wlan0
E/CommandListener( 183): interface ipv6privacyextensions wlan0 enable
E/CommandListener( 183): interface ipv6 wlan0 disable
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
E/CommandListener( 183): interface getcfg p2p0
E/CommandListener( 183): interface setcfg p2p0 0.0.0.0 0 up multicast broadcast
D/CommandListener( 183): Setting iface cfg
D/CommandListener( 183): Trying to bring up p2p0
associate
E/CommandListener( 183): interface ipv6 wlan0 enable
E/CommandListener( 183): interface setcfg wlan0 0.0.0.0 0
D/CommandListener( 183): Setting iface cfg
E/CommandListener( 183): network create 100
E/CommandListener( 183): network interface add 100 wlan0
E/CommandListener( 183): network route add 100 wlan0 fe80::/64
E/CommandListener( 183): network route add 100 wlan0 10.247.32.0/21
E/CommandListener( 183): network route add 100 wlan0 0.0.0.0/0 10.247.32.1
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
E/CommandListener( 183): network default set 100
V/CommandListener( 183): bwctrlcmd: argc=3 bandwidth setglobalalert ...
forget
/CommandListener( 183): interface clearaddrs wlan0
D/CommandListener( 183): Clearing all IP addresses on wlan0
E/CommandListener( 183): interface ipv6 wlan0 disable
E/CommandListener( 183): network destroy 100
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
V/CommandListener( 183): bwctrlcmd: argc=3 bandwidth setglobalalert ...
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
V/CommandListener( 183): bwctrlcmd: argc=3 bandwidth setglobalalert ...
disable wifi
D/CommandListener( 183): Clearing all IP addresses on wlan0
E/CommandListener( 183): interface ipv6 wlan0 disable
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
enable wifi
E/CommandListener( 183): interface getcfg wlan0
E/CommandListener( 183): interface setcfg wlan0 0.0.0.0 0 multicast broadcast down
D/CommandListener( 183): Setting iface cfg
D/CommandListener( 183): Trying to bring down wlan0
E/CommandListener( 183): interface clearaddrs wlan0
D/CommandListener( 183): Clearing all IP addresses on wlan0
E/CommandListener( 183): interface ipv6privacyextensions wlan0 enable
E/CommandListener( 183): interface ipv6 wlan0 disable
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
E/CommandListener( 183): interface getcfg p2p0
E/CommandListener( 183): interface setcfg p2p0 0.0.0.0 0 up multicast broadcast
D/CommandListener( 183): Setting iface cfg
D/CommandListener( 183): Trying to bring up p2p0
associate
E/CommandListener( 183): interface ipv6 wlan0 enable
E/CommandListener( 183): interface setcfg wlan0 0.0.0.0 0
D/CommandListener( 183): Setting iface cfg
E/CommandListener( 183): network create 101
E/CommandListener( 183): network interface add 101 wlan0
E/CommandListener( 183): network route add 101 wlan0 fe80::/64
E/CommandListener( 183): network route add 101 wlan0 10.247.32.0/21
E/CommandListener( 183): network route add 101 wlan0 0.0.0.0/0 10.247.32.1
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
E/CommandListener( 183): network default set 101
V/CommandListener( 183): bwctrlcmd: argc=3 bandwidth setglobalalert ...
turn off wifi
E/CommandListener( 183): interface clearaddrs wlan0
D/CommandListener( 183): Clearing all IP addresses on wlan0
E/CommandListener( 183): interface ipv6 wlan0 disable
E/CommandListener( 183): network destroy 101
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
E/CommandListener( 183): interface clearaddrs wlan0
D/CommandListener( 183): Clearing all IP addresses on wlan0
E/CommandListener( 183): interface ipv6 wlan0 disable
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
V/CommandListener( 183): bwctrlcmd: argc=3 bandwidth setglobalalert ...
V/CommandListener( 183): bwctrlcmd: argc=3 bandwidth setglobalalert ...
V/CommandListener( 183): bwctrlcmd: argc=3 bandwidth setglobalalert ...
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
E/CommandListener( 183): interface getcfg wlan0
E/CommandListener( 183): interface setcfg wlan0 0.0.0.0 0 multicast broadcast down
D/CommandListener( 183): Setting iface cfg
D/CommandListener( 183): Trying to bring down wlan0
E/CommandListener( 183): interface clearaddrs wlan0
D/CommandListener( 183): Clearing all IP addresses on wlan0
E/CommandListener( 183): interface ipv6privacyextensions wlan0 enable
E/CommandListener( 183): interface ipv6 wlan0 disable
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
E/CommandListener( 183): interface getcfg p2p0
E/CommandListener( 183): interface setcfg p2p0 0.0.0.0 0 up multicast broadcast
D/CommandListener( 183): Setting iface cfg
D/CommandListener( 183): Trying to bring up p2p0
E/CommandListener( 183): interface ipv6 wlan0 enable
E/CommandListener( 183): interface setcfg wlan0 0.0.0.0 0
D/CommandListener( 183): Setting iface cfg
E/CommandListener( 183): network create 102
E/CommandListener( 183): network interface add 102 wlan0
E/CommandListener( 183): network route add 102 wlan0 fe80::/64
E/CommandListener( 183): network route add 102 wlan0 10.247.32.0/21
E/CommandListener( 183): network route add 102 wlan0 0.0.0.0/0 10.247.32.1
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
E/CommandListener( 183): network default set 102
V/CommandListener( 183): bwctrlcmd: argc=3 bandwidth setglobalalert ...
E/CommandListener( 183): interface clearaddrs wlan0
D/CommandListener( 183): Clearing all IP addresses on wlan0
E/CommandListener( 183): interface ipv6 wlan0 disable
E/CommandListener( 183): network destroy 102
E/CommandListener( 183): interface clearaddrs wlan0
D/CommandListener( 183): Clearing all IP addresses on wlan0
E/CommandListener( 183): interface ipv6 wlan0 disable
V/CommandListener( 183): bwctrlcmd: argc=3 bandwidth setglobalalert ...
V/CommandListener( 183): bwctrlcmd: argc=3 bandwidth setglobalalert ...
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
V/CommandListener( 183): bwctrlcmd: argc=3 bandwidth setglobalalert ...
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
E/CommandListener( 183): network create 103
E/CommandListener( 183): network interface add 103 rmnet0
E/CommandListener( 183): interface setmtu rmnet0 1500
E/CommandListener( 183): network route add 103 rmnet0 0.0.0.0/0 42.77.166.188
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
V/CommandListener( 183): idletimerctrlcmd: argc=5 idletimer add ...
V/CommandListener( 183): bwctrlcmd: argc=4 bandwidth setiquota ...
E/CommandListener( 183): network default set 103
V/CommandListener( 183): bwctrlcmd: argc=3 bandwidth setglobalalert ...
turn on wifi
E/CommandListener( 183): interface getcfg wlan0
E/CommandListener( 183): interface setcfg wlan0 0.0.0.0 0 multicast broadcast down
D/CommandListener( 183): Setting iface cfg
D/CommandListener( 183): Trying to bring down wlan0
E/CommandListener( 183): interface clearaddrs wlan0
D/CommandListener( 183): Clearing all IP addresses on wlan0
E/CommandListener( 183): interface ipv6privacyextensions wlan0 enable
E/CommandListener( 183): interface ipv6 wlan0 disable
E/CommandListener( 183): interface getcfg p2p0
E/CommandListener( 183): interface setcfg p2p0 0.0.0.0 0 up multicast broadcast
D/CommandListener( 183): Setting iface cfg
D/CommandListener( 183): Trying to bring up p2p0
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
E/CommandListener( 183): interface ipv6 wlan0 enable
E/CommandListener( 183): interface setcfg wlan0 0.0.0.0 0
D/CommandListener( 183): Setting iface cfg
E/CommandListener( 183): network create 104
E/CommandListener( 183): network interface add 104 wlan0
E/CommandListener( 183): network route add 104 wlan0 fe80::/64
E/CommandListener( 183): network route add 104 wlan0 10.247.32.0/21
E/CommandListener( 183): network route add 104 wlan0 0.0.0.0/0 10.247.32.1
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
V/CommandListener( 183): bwctrlcmd: argc=3 bandwidth removeiquota ...
E/CommandListener( 183): network default set 104
V/CommandListener( 183): bwctrlcmd: argc=2 bandwidth gettetherstats ...
V/CommandListener( 183): bwctrlcmd: argc=3 bandwidth setglobalalert ...
V/CommandListener( 183): bwctrlcmd: argc=3 bandwidth setglobalalert ...
Assignee | ||
Comment 4•10 years ago
|
||
The minimum netd commands to add default route are the following:
$ ndc network create 100
$ ndc network interface add 100 wlan0
$ ndc network route add 100 wlan0 0.0.0.0/0 10.247.32.1
$ ndc network default set 100
However, I have no idea where does "10.247.32.1" come from ...
Comment 5•10 years ago
|
||
(In reply to Henry Chang [:henry] from comment #4)
> The minimum netd commands to add default route are the following:
>
> $ ndc network create 100
> $ ndc network interface add 100 wlan0
> $ ndc network route add 100 wlan0 0.0.0.0/0 10.247.32.1
> $ ndc network default set 100
>
> However, I have no idea where does "10.247.32.1" come from ...
I guess it should be gateway address obtained from dhcp server.
Assignee | ||
Comment 6•10 years ago
|
||
WIP. It works sometimes. Sometimes there is netd racing issue. Some commands must be sent after certain broadcast command is received.
Assignee | ||
Comment 7•10 years ago
|
||
1) Add untested netd commands to addHostRoute/removeHostRoute/removeHostRoutes
2) Add ipv6 enable/disable
3) Change nsINetworkService.resetRoutingTable flow
4) Still doesn't fix the racing issue
Attachment #8540046 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
Use correct netd commands for addHostRoute/removeHostRoute
but not removeHostRouts. Considering to get rid of removeHostRoutes.
Attachment #8541478 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
To check routing table:
1)
$ ip rule list
0: from all lookup local
10000: from all fwmark 0xc0000/0xd0000 lookup legacy_system
13000: from all fwmark 0x10063/0x1ffff lookup local_network
13000: from all fwmark 0x10064/0x1ffff lookup wlan0
14000: from all oif wlan0 lookup wlan0
15000: from all fwmark 0x0/0x10000 lookup legacy_system
16000: from all fwmark 0x0/0x10000 lookup legacy_network
17000: from all fwmark 0x0/0x10000 lookup local_network
19000: from all fwmark 0x64/0x1ffff lookup wlan0
22000: from all fwmark 0x0/0xffff lookup wlan0
23000: from all fwmark 0x0/0xffff uidrange 0-0 lookup main
32000: from all unreachable
$ ip route list table wlan0
default via 10.247.32.1 dev wlan0 proto static
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Henry Chang [:henry] from comment #9)
> To check routing table:
>
> 1)
> $ ip rule list
> 0: from all lookup local
> 10000: from all fwmark 0xc0000/0xd0000 lookup legacy_system
> 13000: from all fwmark 0x10063/0x1ffff lookup local_network
> 13000: from all fwmark 0x10064/0x1ffff lookup wlan0
> 14000: from all oif wlan0 lookup wlan0
> 15000: from all fwmark 0x0/0x10000 lookup legacy_system
> 16000: from all fwmark 0x0/0x10000 lookup legacy_network
> 17000: from all fwmark 0x0/0x10000 lookup local_network
> 19000: from all fwmark 0x64/0x1ffff lookup wlan0
> 22000: from all fwmark 0x0/0xffff lookup wlan0
> 23000: from all fwmark 0x0/0xffff uidrange 0-0 lookup main
> 32000: from all unreachable
>
> $ ip route list table wlan0
> default via 10.247.32.1 dev wlan0 proto static
It should be like the following:
default via 10.247.32.1 dev wlan0 proto static
10.247.32.0/21 dev wlan0 proto static scope link
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8541482 -
Attachment is obsolete: true
Comment 13•10 years ago
|
||
This should be 2.2+ because it is mandatory for Android L porting.
feature-b2g: --- → 2.2+
Target Milestone: --- → 2.2 S4 (23jan)
Comment 14•10 years ago
|
||
Hi Vincent,
Suggest make it ready before branching (1/12), please help to evaluate could it be done in Sprint 3 (~1/9)? Thanks.
Flags: needinfo?(vchang)
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 15•10 years ago
|
||
This patch should make wifi/data/mms work properly. Besides, it is easy to support tethering and DUN since table wlan0 and rmnet0 is actually created.
Attachment #8541660 -
Attachment is obsolete: true
Comment 16•10 years ago
|
||
Per offline talk. We'll try to finish this earlier. But we would like to keep current milestone.
Flags: needinfo?(vchang)
Assignee | ||
Comment 17•10 years ago
|
||
1) Add callback wrapper per command
2) Fix command chain interleaving issue
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8542527 -
Attachment is obsolete: true
Attachment #8543798 -
Attachment is obsolete: true
Assignee | ||
Comment 19•10 years ago
|
||
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8543837 -
Attachment is obsolete: true
Attachment #8543857 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8543921 -
Flags: review?(echen)
Comment 21•10 years ago
|
||
Comment on attachment 8543921 [details] [diff] [review]
Bug1104664.patch (on top of Bug 1116458)
Review of attachment 8543921 [details] [diff] [review]:
-----------------------------------------------------------------
Please see my comments below. Thank you.
::: dom/system/gonk/NetIdManager.cpp
@@ +1,1 @@
> +/* Copyright 2012 Mozilla Foundation and Mozilla contributors
Please use Mozilla Public License for this file.
e.g.
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
::: dom/system/gonk/NetworkManager.js
@@ +350,2 @@
>
> // Probing the public network accessibility after routing table is ready
Should we move this into createNetwork's callback, too?
::: dom/system/gonk/NetworkUtils.cpp
@@ +1010,5 @@
> +{
> + char command[MAX_COMMAND_SIZE];
> +
> + if (isDefaultInterface(GET_FIELD(mIftype))) {
> + snprintf(command, MAX_COMMAND_SIZE - 1, "network route add %d %s %s/32 %s",
Per offline discussion, adding host rout in legacy table and adding scope link route in interface table seems okay, let's try it.
@@ +1042,5 @@
> +{
> + char command[MAX_COMMAND_SIZE];
> +
> + if (isDefaultInterface(GET_FIELD(mIftype))) {
> + snprintf(command, MAX_COMMAND_SIZE - 1, "network route remove %d %s %s/32 %s",
Ditto
@@ +1105,5 @@
> + CommandCallback aCallback,
> + NetworkResultOptions& aResult)
> +{
> + char command[MAX_COMMAND_SIZE];
> + snprintf(command, MAX_COMMAND_SIZE - 1, "interface ipv6 %s enable", GET_CHAR(mIfname));
Enabling ipv6 may fail if kernel doesn't enable ipv6 function, we should use wrapped callback to cache the error here, too.
@@ +1115,5 @@
> + CommandCallback aCallback,
> + NetworkResultOptions& aResult)
> +{
> + char command[MAX_COMMAND_SIZE];
> + snprintf(command, MAX_COMMAND_SIZE - 1, "interface ipv6 %s disable", GET_CHAR(mIfname));
Ditto
@@ +1171,5 @@
> + // Notify the main thread.
> + postMessage(aOptions, aResult);
> +
> + static CommandFunc COMMAND_CHAIN[] = {
> + destroyNetwork,
If we have to do `destroyNetwork` when fail to set default route, let's bubble this error up to NetworkManager, and let NetworkManager do destroy.
@@ +1657,5 @@
> + }
> +
> + static CommandFunc COMMAND_CHAIN[] = {
> + enableIpv6,
> + addInterfaceToNetwork,
Per off-line discussion, let's do `enableIpv6` and `addInterfaceToNetwork` in `createNetwork`.
::: dom/system/gonk/nsINetworkService.idl
@@ +325,5 @@
> *
> * @return A deferred promise that resolves on success or rejects with a
> * specified reason otherwise.
> */
> + jsval addHostRoute(in DOMString interfaceName,
nit: trailing ws
@@ +326,5 @@
> * @return A deferred promise that resolves on success or rejects with a
> * specified reason otherwise.
> */
> + jsval addHostRoute(in DOMString interfaceName,
> + in long interfaceType,
With new adding host route policy (see comments in NetUtils.cpp), it seems we won't have to pass |iftype| to net_worker anymore.
@@ +344,5 @@
> * @return A deferred promise that resolves on success or rejects with a
> * specified reason otherwise.
> */
> + jsval removeHostRoute(in DOMString interfaceName,
> + in long interfaceType,
Ditto
::: dom/webidl/NetworkOptions.webidl
@@ +13,5 @@
> // "setDefaultRouteAndDNS", "removeDefaultRoute"
> // "addHostRoute", "removeHostRoute"
> // "removeHostRoutes".
> +
> + long iftype; // for "removeNetworkRoute", "setDNS",
With new adding host policy (see comments in NetUtils.cpp), it seems we won't have to pass |iftype| to net_worker anymore.
Attachment #8543921 -
Flags: review?(echen)
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8543921 -
Attachment is obsolete: true
Assignee | ||
Comment 23•10 years ago
|
||
Comment on attachment 8546434 [details] [diff] [review]
Bug1104664_V2.patch
Hi Edgar,
Could you please review it again? All comments from the last review
has been addressed plus
1) Add routes for subnet explicitly in NetworkManager.js::_setDefaultRouteAndDNS
2) Extract common part from NetworkUtils.cpp::enableIpv6/disableIpv4 and addRouteToInterface/removeRouteFromInterface
3) Add prefixLength to nsINetworkService.addHostRoute/removeHostRoute
Thanks!
Attachment #8546434 -
Flags: review?(echen)
Assignee | ||
Comment 24•10 years ago
|
||
(In reply to Henry Chang [:henry] from comment #23)
> Comment on attachment 8546434 [details] [diff] [review]
> Bug1104664_V2.patch
>
> Hi Edgar,
>
> Could you please review it again? All comments from the last review
> has been addressed plus
> 1) Add routes for subnet explicitly in
> NetworkManager.js::_setDefaultRouteAndDNS
> 2) Extract common part from NetworkUtils.cpp::enableIpv6/disableIpv4 and
> addRouteToInterface/removeRouteFromInterface
> 3) Add prefixLength to nsINetworkService.addHostRoute/removeHostRoute
>
> Thanks!
The patch is also working well on flame v188!
Comment 25•10 years ago
|
||
Comment on attachment 8546434 [details] [diff] [review]
Bug1104664_V2.patch
Review of attachment 8546434 [details] [diff] [review]:
-----------------------------------------------------------------
We are almost there, please see my comments below.
::: dom/system/gonk/NetIdManager.h
@@ +8,5 @@
> +#include "nsString.h"
> +#include "nsDataHashtable.h"
> +
> +// NetId is a logical network identifier defined by netd.
> +// A network is typically a physical one (i.e. PhysicalNetwork.cpp)
Nit: Trailing ws
@@ +9,5 @@
> +#include "nsDataHashtable.h"
> +
> +// NetId is a logical network identifier defined by netd.
> +// A network is typically a physical one (i.e. PhysicalNetwork.cpp)
> +// for netd but it could be a virtual network as well.
Ditto
::: dom/system/gonk/NetworkManager.js
@@ +1290,5 @@
> + let prefixLengths = {};
> + let length = network.getAddresses(ips, prefixLengths);
> + for (let i = 0; i < length; i++) {
> + debug('Adding subnet routes: ' + ips.value[i] + '/' + prefixLengths.value[i]);
> + gNetworkService.addHostRoute(network.name, network.type,
|NetworkService.addHostRoute| doesn't take network type any more.
@@ +1300,5 @@
> + if (!success) {
> + gNetworkService.destroyNetwork(network, function() {});
> + return;
> + }
> + addSubnetRoutes(network);
Should we always add scope link for a network no matter it is default network or not?
::: dom/system/gonk/NetworkUtils.cpp
@@ +1537,5 @@
> // DNS needs to be set through netd since JellyBean (4.3).
> + if (SDK_VERSION >= 20) {
> + // Lollipop.
> + static CommandFunc COMMAND_CHAIN[] = {
> + setInterfaceDns
I guess we should append |defaultAsyncSuccessHandler| in this command chain, no?
@@ +1551,1 @@
> if (SDK_VERSION >= 18) {
I guess we should use `else if` here.
} else if (SDK_VERSION >= 18) {
@@ +1551,5 @@
> if (SDK_VERSION >= 18) {
> + // JB, KK.
> + static CommandFunc COMMAND_CHAIN[] = {
> + setDefaultInterface,
> + setInterfaceDns
Ditto, append |defaultAsyncSuccessHandler|?
@@ +1795,5 @@
> + return addHostRouteLegacy(aOptions);
> + }
> +
> + static CommandFunc COMMAND_CHAIN[] = {
> + addInterfaceToNetwork,
Since we already do |addInterfaceToNetwork| when creating a network, it seems we won't have to do it again here.
@@ +1899,5 @@
> + if (SDK_VERSION < 20) {
> + return removeHostRoutesLegacy(aOptions);
> + }
> +
> + NU_DBG("Don't know how to remove host routes on a interface");
It seems we won't need |removeHostRoutes| any more after having reference counting mechanism in bug 973543, so I am fine with this now. Let's see if we could remove |removeHostRoutes| in bug 973543.
@@ +1964,5 @@
> +
> + return nsCString(subnetStr);
> + }
> +
> + if (AF_INET == type) {
nit: `else if (type == AF_INET) {` maybe
::: dom/system/gonk/nsINetworkService.idl
@@ +327,5 @@
> * specified reason otherwise.
> */
> + jsval addHostRoute(in DOMString interfaceName,
> + in DOMString host,
> + in long prefixLength,
Nit: Please help to update above comment as well.
@@ +345,5 @@
> * specified reason otherwise.
> */
> + jsval removeHostRoute(in DOMString interfaceName,
> + in DOMString host,
> + in long prefixLength,
Ditto.
Attachment #8546434 -
Flags: review?(echen)
Comment 26•10 years ago
|
||
Comment on attachment 8546434 [details] [diff] [review]
Bug1104664_V2.patch
Review of attachment 8546434 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/system/gonk/NetworkUtils.cpp
@@ +916,5 @@
> + CommandCallback aCallback,
> + NetworkResultOptions& aResult)
> +{
> + char command[MAX_COMMAND_SIZE];
> + snprintf(command, MAX_COMMAND_SIZE - 1, "network route remove %d %s 0.0.0.0/0",
I think we should specify `gateway` when removing default route, otherwise we will get 'No such process' error.
Comment 27•10 years ago
|
||
Sorry to jump in. I see that we have added a "prefixLength" param to nsINetworkService.addHostRoute/removeHostRoute. This is kind of confusing, since a 'host route' always has prefix length 32 for IPv4 or 128 for IPv6. Maybe we should consider changing the function name or just hide the prefix length details in NetworkUtils. What do you think?
Comment 28•10 years ago
|
||
(In reply to Jessica Jong [:jjong] [:jessica] from comment #27)
> Sorry to jump in. I see that we have added a "prefixLength" param to
> nsINetworkService.addHostRoute/removeHostRoute. This is kind of confusing,
> since a 'host route' always has prefix length 32 for IPv4 or 128 for IPv6.
> Maybe we should consider changing the function name or just hide the prefix
> length details in NetworkUtils. What do you think?
This is because we extend addHostRoute/removeHostRoute to support adding/removing scope link route, and scope link route needs specifying `prefixLength`, but it doesn't require `gateway`, so we mark gateway as optional. Another way to achieve this is adding new functions for scope link. Since this is an internal service, I haven't strong opinion on this. What do you think, Jessica and Henry?
Assignee | ||
Comment 29•10 years ago
|
||
Thanks for the review!
(In reply to Edgar Chen [:edgar][:echen] from comment #25)
> Comment on attachment 8546434 [details] [diff] [review]
>
> ::: dom/system/gonk/NetworkUtils.cpp
> @@ +1537,5 @@
> > // DNS needs to be set through netd since JellyBean (4.3).
> > + if (SDK_VERSION >= 20) {
> > + // Lollipop.
> > + static CommandFunc COMMAND_CHAIN[] = {
> > + setInterfaceDns
>
> I guess we should append |defaultAsyncSuccessHandler| in this command chain,
> no?
>
Yes! We need to append defaultAsyncSuccessHandler to this command chain
as well as another setDNS command chain.
> @@ +1551,1 @@
> > if (SDK_VERSION >= 18) {
>
> I guess we should use `else if` here.
>
> } else if (SDK_VERSION >= 18) {
>
Since the statement of (SDK_VERSION >= 20) always returns,
we don't need a 'else' here.
> @@ +1551,5 @@
> > if (SDK_VERSION >= 18) {
> > + // JB, KK.
> > + static CommandFunc COMMAND_CHAIN[] = {
> > + setDefaultInterface,
> > + setInterfaceDns
>
> Ditto, append |defaultAsyncSuccessHandler|?
> @@ +1899,5 @@
> > + if (SDK_VERSION < 20) {
> > + return removeHostRoutesLegacy(aOptions);
> > + }
> > +
> > + NU_DBG("Don't know how to remove host routes on a interface");
>
> It seems we won't need |removeHostRoutes| any more after having reference
> counting mechanism in bug 973543, so I am fine with this now. Let's see if
> we could remove |removeHostRoutes| in bug 973543.
>
> @@ +1964,5 @@
> > +
> > + return nsCString(subnetStr);
> > + }
> > +
> > + if (AF_INET == type) {
>
> nit: `else if (type == AF_INET) {` maybe
>
Since I don't see any internal documentation to refrain us
from doing this, I would like to put the constant to rhs.
> @@ +1300,5 @@
> > + if (!success) {
> > + gNetworkService.destroyNetwork(network, function() {});
> > + return;
> > + }
> > + addSubnetRoutes(network);
>
> Should we always add scope link for a network no matter it is default
> network or not?
I have no idea, actually :p Discuss with you offline. Thanks!
Assignee | ||
Comment 31•10 years ago
|
||
Attachment #8546434 -
Attachment is obsolete: true
Updated•10 years ago
|
Whiteboard: [CR 781047]
Updated•10 years ago
|
Whiteboard: [CR 781047] → [b2g-crash][caf-crash 468][CR 781047]
Assignee | ||
Updated•10 years ago
|
Attachment #8548696 -
Flags: review?(echen)
Comment 32•10 years ago
|
||
(In reply to Edgar Chen [:edgar][:echen] from comment #28)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #27)
> > Sorry to jump in. I see that we have added a "prefixLength" param to
> > nsINetworkService.addHostRoute/removeHostRoute. This is kind of confusing,
> > since a 'host route' always has prefix length 32 for IPv4 or 128 for IPv6.
> > Maybe we should consider changing the function name or just hide the prefix
> > length details in NetworkUtils. What do you think?
>
> This is because we extend addHostRoute/removeHostRoute to support
> adding/removing scope link route, and scope link route needs specifying
> `prefixLength`, but it doesn't require `gateway`, so we mark gateway as
> optional. Another way to achieve this is adding new functions for scope
> link. Since this is an internal service, I haven't strong opinion on this.
> What do you think, Jessica and Henry?
Per offline discussion with Henry, we decided to use a more general function name, as you can see in the latest patch. :)
Comment 33•10 years ago
|
||
Comment on attachment 8548696 [details] [diff] [review]
Bug1104664_V3.patch
Review of attachment 8548696 [details] [diff] [review]:
-----------------------------------------------------------------
Looks really good, I will give r+ once I finish the basic tests on nexus-5-l and flame-kk.
Thank you, Henry.
::: dom/system/gonk/NetworkService.js
@@ +305,5 @@
> + modifyRoute: function(action, interfaceName, host, prefixLength, gateway) {
> + let command;
> +
> + switch (action) {
> + case Ci.nsINetworkService.MODIFY_ROUTE_ADD:
nit: coding style
switch (...) {
case 1:
...
break;
case 2:
...
break;
default:
...
break;
}
::: dom/system/gonk/NetworkUtils.cpp
@@ +1069,5 @@
> + NetworkResultOptions& aResult)
> +{
> + char command[MAX_COMMAND_SIZE];
> + snprintf(command, MAX_COMMAND_SIZE - 1, "network route add %d %s 0.0.0.0/0 %s",
> + GET_FIELD(mNetId), GET_CHAR(mIfname), GET_CHAR(mGateways[0]));
Just realize we only handle the first gateway for now and we will get some trouble in dual stack device (support both IPv4 and IPv6) or IPv6 only network. But we could enhance this in a follow-up bug.
@@ +1941,5 @@
> +nsCString NetworkUtils::getSubnetIp(const nsCString& aIp, int aPrefixLength)
> +{
> + int type = getIpType(aIp.get());
> +
> + if (type == AF_INET6) {
Super nit: Having consistent style for AF_INET6 and AF_INET.
@@ +1965,5 @@
> +
> + return nsCString(subnetStr);
> + }
> +
> + if (AF_INET == type) {
Ditto, super nit: Having consistent style for AF_INET6 and AF_INET.
::: dom/system/gonk/nsINetworkService.idl
@@ +331,1 @@
> * @param gateway
nit: @param [optional] gateway
Updated•10 years ago
|
blocking-b2g: --- → 2.2+
feature-b2g: 2.2+ → ---
Updated•10 years ago
|
blocking-b2g: 2.2+ → ---
feature-b2g: --- → 2.2+
Updated•10 years ago
|
Blocks: CAF-v2.2-metabug
Assignee | ||
Comment 34•10 years ago
|
||
Assignee | ||
Comment 35•10 years ago
|
||
V4 addresses comments from Comment 33 and puts
Services.obs.notifyObservers(network, TOPIC_CONNECTION_STATE_CHANGED,
this.convertConnectionType(network));
to the callback of createNetwork/destroyNetwork to avoid MMS regression.
Assignee | ||
Updated•10 years ago
|
Attachment #8548696 -
Flags: review?(echen)
Assignee | ||
Updated•10 years ago
|
Attachment #8549342 -
Flags: review?(echen)
Updated•10 years ago
|
Attachment #8548696 -
Attachment is obsolete: true
Comment 36•10 years ago
|
||
Comment on attachment 8549342 [details] [diff] [review]
Bug1104664_V4.patch
Review of attachment 8549342 [details] [diff] [review]:
-----------------------------------------------------------------
Perfect!! Thank you, Henry.
Attachment #8549342 -
Flags: review?(echen) → review+
Assignee | ||
Comment 37•10 years ago
|
||
Assignee | ||
Comment 38•10 years ago
|
||
(In reply to Edgar Chen [:edgar][:echen] from comment #36)
> Comment on attachment 8549342 [details] [diff] [review]
> Bug1104664_V4.patch
>
> Review of attachment 8549342 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Perfect!! Thank you, Henry.
Thanks man!
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 39•10 years ago
|
||
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 41•10 years ago
|
||
Comment on attachment 8549342 [details] [diff] [review]
Bug1104664_V4.patch
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Wifi
User impact if declined: Data and wifi will not work
Testing completed: Yes
Risk to taking this patch (and alternatives if risky): No
String or UUID changes made by this patch: nsINetorkService
Attachment #8549342 -
Flags: approval-mozilla-b2g37?
Updated•10 years ago
|
Attachment #8549342 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Comment 42•10 years ago
|
||
status-b2g-v2.2:
--- → fixed
status-b2g-master:
--- → fixed
status-firefox36:
--- → wontfix
status-firefox37:
--- → wontfix
status-firefox38:
--- → fixed
Updated•10 years ago
|
Whiteboard: [b2g-crash][caf-crash 468][CR 781047] → [caf priority: p2][b2g-crash][caf-crash 468][CR 781047]
Blocks: CAF-v3.0-FL-metabug
No longer blocks: CAF-v3.0-FL-metabug
You need to log in
before you can comment on or make changes to this bug.
Description
•