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);

Looks like we need this: http://androidxref.com/5.0.0_r2/xref/system/netd/server/CommandListener.cpp#1433

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

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 ...

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 ...

(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.

WIP. It works sometimes. Sometimes there is netd racing issue. Some commands must be sent after certain broadcast command is received.

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

Use correct netd commands for addHostRoute/removeHostRoute but not removeHostRouts. Considering to get rid of removeHostRoutes.

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

(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

This should be 2.2+ because it is mandatory for Android L porting.

Hi Vincent, Suggest make it ready before branching (1/12), please help to evaluate could it be done in Sprint 3 (~1/9)? Thanks.

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.

Per offline talk. We'll try to finish this earlier. But we would like to keep current milestone.

1) Add callback wrapper per command 2) Fix command chain interleaving issue

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.

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!

(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 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.

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.

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?

(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?

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!

(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 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

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.

Comment on attachment 8549342 [details] [diff] [review] Bug1104664_V4.patch Review of attachment 8549342 [details] [diff] [review]: ----------------------------------------------------------------- Perfect!! Thank you, Henry.

(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!

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