1104664 - [gonk-l] Some netutils APIs are not available in newer libsystem

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Description

[Kai-Zhen Li [:seinlin][:kli]]

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

Comment 1

[Henry Chang [:hchang]]

Looks like we need this:

http://androidxref.com/5.0.0_r2/xref/system/netd/server/CommandListener.cpp#1433

Comment 2

[Henry Chang [:hchang]]

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

Comment 3

[Henry Chang [:hchang]]

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

Comment 4

[Henry Chang [:hchang]]

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

[Vincent Chang[:vchang][changyihsin]]

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

Comment 6

[Henry Chang [:hchang]]

Attachment: WIP.diff

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

Comment 7

[Henry Chang [:hchang]]

Attachment: WIP2.diff

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

Comment 8

[Henry Chang [:hchang]]

Attachment: WIP3.diff

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

Comment 9

[Henry Chang [:hchang]]

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

Comment 10

[Henry Chang [:hchang]]

(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

Comment 12

[Henry Chang [:hchang]]

Attachment: WIP4.patch

Comment 13

[Vincent Chang[:vchang][changyihsin]]

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

Comment 14

[Steven Yang [:styang]]

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

Comment 15

[Henry Chang [:hchang]]

Attachment: WIP5.diff

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.

Comment 16

[Vincent Chang[:vchang][changyihsin]]

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

Comment 17

[Henry Chang [:hchang]]

Attachment: WIP6.diff

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

Comment 18

[Henry Chang [:hchang]]

Attachment: WIP7.diff

Comment 19

[Henry Chang [:hchang]]

Attachment: Bug1104664.patch (on top of Bug 1116458)

Comment 20

[Henry Chang [:hchang]]

Attachment: Bug1104664.patch (on top of Bug 1116458)

Comment 21

[Edgar Chen [:edgar]]

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 22

[Henry Chang [:hchang]]

Attachment: Bug1104664_V2.patch

Comment 23

[Henry Chang [:hchang]]

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!

Comment 24

[Henry Chang [:hchang]]

(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

[Edgar Chen [:edgar]]

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 26

[Edgar Chen [:edgar]]

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

[Jessica Jong [:jessica] (inactive)]

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

[Edgar Chen [:edgar]]

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

Comment 29

[Henry Chang [:hchang]]

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!

Comment 31

[Henry Chang [:hchang]]

Attachment: Bug1104664_V3.patch

Comment 32

[Jessica Jong [:jessica] (inactive)]

(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

[Edgar Chen [:edgar]]

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

Comment 34

[Henry Chang [:hchang]]

Attachment: Bug1104664_V4.patch

Comment 35

[Henry Chang [:hchang]]

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 36

[Edgar Chen [:edgar]]

Comment on attachment 8549342 [details] [diff] [review]
Bug1104664_V4.patch

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

Perfect!! Thank you, Henry.

Comment 37

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9b329ca923c

Comment 38

[Henry Chang [:hchang]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/703edef9eb7e

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/703edef9eb7e

Comment 41

[Henry Chang [:hchang]]

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

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/af6dc88123da