978709 - To appropriately configure a interface having both IPv6 and IPv4 addresses in network manager.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Ken Chang[:kenkai]]

Network manager may receive a interface having both IPv6 and IPv4 addresses from WIFI or RIL. This kind of interfaces need to be appropriately configured in network manager.

Comment 1

[Ken Chang[:kenkai]]

Jessica, we need to have a draft patch for this bug before next week's workshop. Can you please help this?
Edger, it seems that you can provide some information for Jessica. Thanks.

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: nsINetworkInterface changes.

Comment 3

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #2)
> Created attachment 8385215 [details] [diff] [review]
> Part 1: nsINetworkInterface changes.

This interface is what I have in mind. However, I found that NetworkManager is not the only one using nsINetworkInterface, see gonk_addrs [1], so I am not sure if jsval is the best type for 'addresses'.
I will work on NetworkManager, NetworkService and NetworkUtils changes first, and Edgar will help on jsval handling in gonk_addrs.

Any suggestion on the interface is welcome! Thanks.

[1] http://mxr.mozilla.org/mozilla-central/source/media/mtransport/gonk_addrs.cpp#67

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 2: NetworkService changes.

Comment 5

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 3: NetworkUtils changes.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Note that the I commented out the gonk_addrs part first for development.

TODOs:
1. Verify wifi part.
2. Handle secondary routes for IPv4v6.

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8386001 [details] [diff] [review]
(WIP) Part 2: NetworkService changes.

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

::: dom/system/gonk/NetworkService.js
@@ +277,5 @@
> +        ifname: network.name,
> +        ip: address.ip,
> +        prefixLength: address.prefixLength
> +      };
> +      this.controlMessage(options, function() {});

nit: please remove that empty function.

Comment 8

[Edgar Chen [:edgar]]

Comment on attachment 8386001 [details] [diff] [review]
(WIP) Part 2: NetworkService changes.

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

Thank you, please see my below comment.

::: dom/system/gonk/NetworkService.js
@@ +264,5 @@
>      });
>    },
>  
>    resetRoutingTable: function(network) {
> +    for (let i = 0; i < network.addresses; i++) {

I guess it should be |network.addresses.length|.

Comment 9

[Edgar Chen [:edgar]]

Comment on attachment 8386003 [details] [diff] [review]
(WIP) Part 3: NetworkUtils changes.

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

Please see my below comment. :)
Thank you.

::: dom/system/gonk/NetworkUtils.cpp
@@ +339,5 @@
> +    if ((getIpType(autoGateway.get()) == AF_INET &&
> +         getIpType(ip) == AF_INET) ||
> +        (getIpType(autoGateway.get()) == AF_INET6 &&
> +         getIpType(ip) == AF_INET6)) {
> +      strcpy(bestGateway, autoGateway.get());

Hmm ... do a strcpy() here, It looks strange to me. Could we return gateways[i] or index directly?

@@ +1166,5 @@
> +    for (uint32_t i = 0; i < length; i++) {
> +      NS_ConvertUTF16toUTF8 autoDns(aOptions.mDnses[i]);
> +
> +      char dns_prop_key[PROPERTY_VALUE_MAX];
> +      sprintf(dns_prop_key, "net.dns%d", i);

dns properties starts with "net.dns1". So I think it should be:

sprintf(dns_prop_key, "net.dns%d", i+1);

@@ +1217,5 @@
> +      }
> +
> +      if (type == AF_INET6) {
> +        if (!aOptions.mOldIfname.IsEmpty()) {
> +          mNetUtils->do_ifc_remove_route(GET_CHAR(mOldIfname), "::", 0, NULL);

I think we only need to execute removing route one time, so move it out of for loop.

@@ +1223,5 @@
> +
> +        mNetUtils->do_ifc_add_route(autoIfname.get(), "::", 0, gateway);
> +      } else { /* type == AF_INET */
> +        if (!aOptions.mOldIfname.IsEmpty()) {
> +          mNetUtils->do_ifc_remove_default_route(GET_CHAR(mOldIfname));

ditto

@@ +1242,5 @@
>      }
>  
> +    if (type == AF_INET6) {
> +      if (!aOptions.mOldIfname.IsEmpty()) {
> +        mNetUtils->do_ifc_remove_route(GET_CHAR(mOldIfname), "::", 0, NULL);

The "if (length > 0)" and "else" parts will both do removing route, so could we combine them into one?

@@ +1248,5 @@
>  
> +      mNetUtils->do_ifc_add_route(autoIfname.get(), "::", 0, gateway);
> +    } else { /* type == AF_INET */
> +      if (!aOptions.mOldIfname.IsEmpty()) {
> +        mNetUtils->do_ifc_remove_default_route(GET_CHAR(mOldIfname));

ditto

@@ +1294,5 @@
>    uint32_t length = aOptions.mHostnames.Length();
>    for (uint32_t i = 0; i < length; i++) {
>      NS_ConvertUTF16toUTF8 autoHostname(aOptions.mHostnames[i]);
> +    gateway = {0};
> +    selectGateway(aOptions.mGateways, autoHostname.get(), gateway);

Please see above comment in selectGateway().

@@ +1321,5 @@
>    uint32_t length = aOptions.mHostnames.Length();
>    for (uint32_t i = 0; i < length; i++) {
>      NS_ConvertUTF16toUTF8 autoHostname(aOptions.mHostnames[i]);
> +    gateway = {0};
> +    selectGateway(aOptions.mGateways, autoHostname.get(), gateway);

Please see above comment in selectGateway().

::: dom/webidl/NetworkOptions.webidl
@@ +17,4 @@
>    unsigned long prefixLength;         // for "removeNetworkRoute".
>    DOMString domain;                   // for "setDNS"
>    DOMString dns1_str;                 // for "setDNS", "setDefaultRouteAndDNS".
>    DOMString dns2_str;                 // for "setDNS", "setDefaultRouteAndDNS".

"setDefaultRouteAndDNS" won't use these two parameter any more, please help to correct the comment.

@@ +18,5 @@
>    DOMString domain;                   // for "setDNS"
>    DOMString dns1_str;                 // for "setDNS", "setDefaultRouteAndDNS".
>    DOMString dns2_str;                 // for "setDNS", "setDefaultRouteAndDNS".
>    DOMString oldIfname;                // for "setDefaultRouteAndDNS".
>    DOMString gateway_str;              // for "setDefaultRouteAndDNS".

Could we remove |gateway_str|? Is there any other command will use it?
If only "setDefaultRouteAndDns" will use this parameter, I think we can remove it given that you already replaced it by using |gateways| in Part2.
((And we also need to remove |mGateway_str| in NetworkUtils.h))

@@ +20,5 @@
>    DOMString dns2_str;                 // for "setDNS", "setDefaultRouteAndDNS".
>    DOMString oldIfname;                // for "setDefaultRouteAndDNS".
>    DOMString gateway_str;              // for "setDefaultRouteAndDNS".
>    DOMString gateway;                  // for "addHostRoute", "removeHostRoute",
>                                        //     "removeDefaultRoute".

Same as above. Could we remove this parameter?

@@ +44,5 @@
>    DOMString usbStartIp;               // for "setWifiTethering".
>    DOMString usbEndIp;                 // for "setWifiTethering".
>    DOMString dns1;                     // for "setWifiTethering".
>    DOMString dns2;                     // for "setWifiTethering".
> +  sequence<DOMString> dnses;          // for "addHostRoute", "removeHostRoute"

Please correct the comment.
// for "setDNS", "setDefaultRouteAndDNS".

Comment 10

[Jessica Jong [:jessica] (inactive)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #7)
> Comment on attachment 8386001 [details] [diff] [review]
> (WIP) Part 2: NetworkService changes.
> 
> Review of attachment 8386001 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkService.js
> @@ +277,5 @@
> > +        ifname: network.name,
> > +        ip: address.ip,
> > +        prefixLength: address.prefixLength
> > +      };
> > +      this.controlMessage(options, function() {});
> 
> nit: please remove that empty function.

Will do. Should we remove existing empty functions as well?

Comment 11

[Jessica Jong [:jessica] (inactive)]

(In reply to Edgar Chen [:edgar][:echen] from comment #8)
> Comment on attachment 8386001 [details] [diff] [review]
> (WIP) Part 2: NetworkService changes.
> 
> Review of attachment 8386001 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you, please see my below comment.
> 
> ::: dom/system/gonk/NetworkService.js
> @@ +264,5 @@
> >      });
> >    },
> >  
> >    resetRoutingTable: function(network) {
> > +    for (let i = 0; i < network.addresses; i++) {
> 
> I guess it should be |network.addresses.length|.

Yes, thanks. :) Haven't verified this part with wifi yet...

Comment 12

[Edgar Chen [:edgar]]

Comment on attachment 8385215 [details] [diff] [review]
Part 1: nsINetworkInterface changes.

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

::: dom/system/gonk/nsINetworkManager.idl
@@ +79,5 @@
>    readonly attribute long httpProxyPort;
>  
>  };
>  
> +dictionary NetworkAddress

Bug 978611 plans to move all dictionary to Webidl. Please help to move it to Webidl. Thank you ;)

Comment 13

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes, v2.

Address review comment 12:
- move dictionary to webidl.

Comment 14

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: NetworkService changes, v2.

Address review comment 7 and comment 8:
- remove empty function
- correct |network.addresses.length|.

Comment 15

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: NetworkService changes, v2.

Remove debug log.

Comment 16

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3.1: NetworkUtils changes, v2.

Address review comment 9:
- change selectGateway(...) to return index.
- set dns property "net.dnsX" starting from index 1.
- per offline discussion, remove default route once for IPv4 and once for IPv6.
- parameters fix will be on the next patch.

Comment 17

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3.2: remove unused parameters in NetworkOptions, v1.

Comment 18

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: secondary routing changes, v1.

Comment 19

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes, v3.

fix typo in dictionary.

Comment 20

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 5: wifi and ril changes, v1.

The is for testing, we should discuss how to land this.

Comment 21

[Edgar Chen [:edgar]]

Attachment: (WIP) Changes for gonk_addrs.cpp

Some code in webrtc will use NetworkInterface to get ip address. So we need to modify it as well.

This patch is WIP and works well with the patch that Jessica provided to me yesterday.

You can use following step to test webrtc in b2g:
1). Open browser.
2). Goto: http://mozilla.github.io/webrtc-landing/data_test.html
3). Press "Start!!" button.
4). If test success, you will see some Hello messages with blue color in text box. 

Thank you

Comment 22

[Edgar Chen [:edgar]]

Comment on attachment 8386669 [details] [diff] [review]
Part 1: interface changes, v3.

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

Looks good. Thank you.

::: dom/system/gonk/nsINetworkManager.idl
@@ +52,2 @@
>     */
> +  readonly attribute jsval addresses;

nit: .... // NetworkAddress[]

Comment 23

[Edgar Chen [:edgar]]

Comment on attachment 8386640 [details] [diff] [review]
Part 2: NetworkService changes, v2.

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

Looks good to me. Thank you.

Comment 24

[Edgar Chen [:edgar]]

Comment on attachment 8386654 [details] [diff] [review]
Part 3.2: remove unused parameters in NetworkOptions, v1.

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

Thank you.

Comment 25

[pgravel]

Comment on attachment 8386669 [details] [diff] [review]
Part 1: interface changes, v3.

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

::: dom/system/gonk/nsINetworkManager.idl
@@ +52,2 @@
>     */
> +  readonly attribute jsval addresses;

From the commercial ril perspective, we would want this interface to not use "jsval" to pass arrays.
Instead of using an attribute, how about using a function which allows passing typed arrays such as:

  void getAddresses([array, size_is(aLength)] in DOMString aAddresses, in unsigned long aLength);

Comment 26

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8386669 [details] [diff] [review]
Part 1: interface changes, v3.

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

Please help have a minor revise to be more friendly to native implementations. All addresses/gateways/dnses.  Thank you :)

Comment 27

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8386640 [details] [diff] [review]
Part 2: NetworkService changes, v2.

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

::: dom/system/gonk/NetworkService.js
@@ +269,5 @@
> +      let address = network.addresses[i];
> +      if (!address.ip || !address.prefixLength) {
> +        if(DEBUG) debug("Either ip or prefixLength is null. Cannot reset routing table.");
> +        return;
> +      }

I think we no longer need this check.  Instead, we should make sure whenever an entry is pushed into |network.addresses|, its ip and prefixLength attributes must be available.

Comment 28

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8386646 [details] [diff] [review]
Part 3.1: NetworkUtils changes, v2.

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

::: dom/system/gonk/NetworkUtils.cpp
@@ +328,5 @@
> +/**
> + * Helper function to find the best match gateway. Return the index in
> + * gateways if found, return invalid index otherwise.
> + */
> +static uint32_t selectGateway(nsTArray<nsString>& gateways, const char* ip)

When |selectGateway| gets called, we always have |getIpType(ip)| called as well in a few lines later.  So why not pass AF of ip instead?

@@ +337,5 @@
> +    NS_ConvertUTF16toUTF8 autoGateway(gateways[i]);
> +    if ((getIpType(autoGateway.get()) == AF_INET &&
> +         getIpType(ip) == AF_INET) ||
> +        (getIpType(autoGateway.get()) == AF_INET6 &&
> +         getIpType(ip) == AF_INET6)) {

We can surely cache |getIpType(ip)| here, can't we?

@@ +864,5 @@
> +  for (uint32_t i = 0; i < length; i++) {
> +    NS_ConvertUTF16toUTF8 autoDns(dnses[i]);
> +
> +    strcat(command, " ");
> +    strcat(command, autoDns.get());

|snprintf| returns the number of chars would be/have been written to the output buffer.  Please use it to calculate the start address of the next write.

@@ +1187,1 @@
>      property_set("net.dns1", interfaceProperties.dns1);

Why do we still have dns1/dns2 here?

@@ +1269,4 @@
>  
> +    int type = getIpType(autoGateway.get());
> +    if (type != AF_INET && type != AF_INET6) {
> +      return false;

continue;

::: dom/webidl/NetworkOptions.webidl
@@ +22,5 @@
>    DOMString oldIfname;                // for "setDefaultRouteAndDNS".
>    DOMString gateway_str;              // for "setDefaultRouteAndDNS".
> +  DOMString gateway;                  // for "addSecondaryRoute", "removeSecondaryRoute".
> +  sequence<DOMString> gateways;       // for "setDefaultRouteAndDNS", "removeDefaultRoute",
> +                                      //     "addHostRoute", "removeHostRoute".

I believe there are more to do.  For example, remove dns1_str and dns2_str.

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8386654 [details] [diff] [review]
Part 3.2: remove unused parameters in NetworkOptions, v1.

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

Ok, it's here.

Comment 30

[Jessica Jong [:jessica] (inactive)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #26)
> Comment on attachment 8386669 [details] [diff] [review]
> Part 1: interface changes, v3.
> 
> Review of attachment 8386669 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please help have a minor revise to be more friendly to native
> implementations. All addresses/gateways/dnses.  Thank you :)

Hi Vicamo,

Do you mean we should change as pgravel suggested?
void getAddresses([array, size_is(aLength)] in DOMString aAddresses, in unsigned long aLength);

I think it should be 'out' instead of 'in', right? And should we use 'DOMString' for simplicity or use a newly created interface like nsINeworkAddress for addresses?

Thank you.

Comment 31

[Edgar Chen [:edgar]]

Comment on attachment 8386646 [details] [diff] [review]
Part 3.1: NetworkUtils changes, v2.

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

Thank you.

::: dom/system/gonk/NetworkUtils.cpp
@@ +1175,5 @@
> +    for (uint32_t i = 0; i < length; i++) {
> +      NS_ConvertUTF16toUTF8 autoDns(aOptions.mDnses[i]);
> +
> +      char dns_prop_key[PROPERTY_VALUE_MAX];
> +      sprintf(dns_prop_key, "net.dns%d", i+1);

snprintf(...)

Comment 32

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Jessica Jong [:jjong] [:jessica] from comment #30)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #26)
> > Comment on attachment 8386669 [details] [diff] [review]
> > Part 1: interface changes, v3.
> > 
> > Review of attachment 8386669 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Please help have a minor revise to be more friendly to native
> > implementations. All addresses/gateways/dnses.  Thank you :)
> 
> Hi Vicamo,
> 
> Do you mean we should change as pgravel suggested?
> void getAddresses([array, size_is(aLength)] in DOMString aAddresses, in
> unsigned long aLength);

Yes, please.

> I think it should be 'out' instead of 'in', right? And should we use
> 'DOMString' for simplicity or use a newly created interface like
> nsINeworkAddress for addresses?

Both aAddresses and aLength are output parameters.

Comment 33

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8386657 [details] [diff] [review]
Part 4: secondary routing changes, v1.

Per offline discussion, we will use a simpler way to check if address is IPv6 or not. IP validity check will be done in NetworkUtils.

Comment 34

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: interface changes, v4.

Change interface from attributes to method calls.

For dnses and gateways, the array is returned for simplicity; for addresses, we can only return the length cause there are two arrays.

Comment 35

[Edgar Chen [:edgar]]

Attachment: (WIP) Changes for gonk_addrs.cpp, v2

WIP v2 for new interface. I have not verify it yet.

Comment 36

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: NetworkService changes, v3.

change to use method calls instead of attributes.

Comment 37

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: NetworkUtils changes, v3.

address review comment 28 and squash part 3.2 into this commit.

Comment 38

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: secondary routing changes, v2.

use a simpler way to check if IP is IPv6 or not.

Comment 39

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 5: wifi and ril changes, v2.

implement nsINetworkInterface new methods.

Comment 40

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 5: wifi and ril changes, v3.

Modify WifiP2pManager implementation as well.

Comment 41

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 1/N: remove nsINetworkInterface::broadcast

Comment 42

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 2.a/N: remove nsIRILDataCallback::receiveDataCallList

nsIRILDataCallback::receiveDataCallList has been obsoleted for a long long time and all the existing classes that implement nsIRILDataCallback leave the function body empty.  Since we're going to change the object layout of a |datacall| JS object, removing related but obsoleted functions saves us some time.

Comment 43

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 2.b/N: don't use nsIRILDataCallback in GonkGPSGeolocationProvider

nsIRILDataCallback has been abused in RadioInterfaceLayer.js and is becoming an RIL internal utility.  Besides, nsIRILDataCallback has also racing problem as described in bug 976897.  We should really use NetworkManager observer events to replace the notification mechanism here.

Comment 44

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 2.c/N: completely remove nsIRILDataCallback

Comment 45

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 2.d/N: don't throw in _deliverDataCallCallback loop

Comment 46

[Vicamo Yang [:vicamo][:vyang]]

Depending on bug 976897 to remove nsIRILDataCallback in GonkGPSGeolocationProvider.

Comment 47

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 3.a/N: RIL_Data_Call_Response_v5 has also DNS server list

Comment 48

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 3.b/N: support multiple addresses in ril_worker

So far verified with all existing data connection test cases and the currently disabled one in bug 957917 -- IPv6 test cases.

Comment 49

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 4.a/6: idl changes to support IPv4v6

This is to replace attachment 8389021 [details] [diff] [review] -- part 1, v4.

Comment 50

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 4.b/6: NetworkService changes to support IPV4V6

To replace attachment 8389606 [details] [diff] [review] -- part 2, v3.

Comment 51

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 4.c/6: NetworkUtils changes

To replace attachment 8389607 [details] [diff] [review] -- part 3, v3.

Comment 52

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 4.d/4: secondary routes changes

To replace attachment 8389639 [details] [diff] [review] -- part 4, v2.

Comment 53

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 4.e/6: nsIRILNetworkInterface changes

To replace attachment 8389825 [details] [diff] [review] -- part 5, v3.

Comment 54

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 4.f/6: P2pNetworkInterface changes

To replace attachment 8389825 [details] [diff] [review] -- part 5, v3.

Comment 55

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 4.g/6: WifiNetworkInterface changes

To replace attachment 8389825 [details] [diff] [review] -- part 5, v3.

Comment 56

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 4.h/6: NetworkInterfaceListService changes

To replace attachment 8389219 [details] [diff] [review] -- changes for gonk_addrs.cpp, v2.

Comment 57

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 5/6: really add an IPV4V6 option

Comment 58

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 4.b/6: NetworkService changes to support IPV4V6 : v2

fix "'network' undefined" in test_data_connection.js.

Comment 59

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 4.h/6: NetworkInterfaceListService changes : v2

fix NetworkInterfaceListService.

Comment 60

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 4.e/6: nsIRILNetworkInterface changes : v2

1) fix an exception thrown when |getAddresses| is called.
2) we always have ips/prefixLengths/dnses/gateways assigned to an array in nsIRILNetworkInterface.

Comment 61

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 6/6: test cases

This entity is currently disabled on tryserver due to bug 871475, but it passes in my local setup.

Try: https://tbpl.mozilla.org/?tree=Try&rev=37c2bd2c6cc2

Comment 62

[Edgar Chen [:edgar]]

Attachment: (WIP) Changes for NetworkInterfaceListService, v1

Comment 63

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 4.e/6: nsIRILNetworkInterface changes : v3

s/this.addresses/this.ips/

Comment 66

[Jessica Jong [:jessica] (inactive)]

Attachment: [clean-up] part 4.c/6: NetworkUtils changes : v2

Address review comment 31:
- use snprintf(...) instead of sprintf(...)

Comment 67

[Jessica Jong [:jessica] (inactive)]

Attachment: [clean-up] part 4.f/6: P2pNetworkInterface changes : v2

Initialize ips/prefixLenghts/gateways/dnses to empty array (to be consistent with WifiNetworkInterface).

Comment 68

[Jessica Jong [:jessica] (inactive)]

Attachment: [clean-up] part 4.h/6: NetworkInterfaceListService changes : v3

- Integrated Edgar's work in attachment 8390329 [details] [diff] [review], but preserved Vicamo's change in NetworkInterfaceListService, cause we need to expose the attributes state, type, name, etc.
- Modified dom/media/tests/mochitest/pc.js to avoid media mochitest failures.

Comment 69

[Jessica Jong [:jessica] (inactive)]

Test full try: https://tbpl.mozilla.org/?tree=Try&rev=9715e0b1f02a
Not breaking the media mochitests! (the Gu and KK failures were known issues at the time I sent to try)

Comment 70

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 1/6: remove nsINetworkInterface::broadcast : v2

update commit summary only.

Comment 71

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 2.a/6: remove nsIRILDataCallback::receiveDataCallList : v2

update commit summary only

Comment 72

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 2.b/6: don't use nsIRILDataCallback in GonkGPSGeolocationProvider : v2

Rebase after bug 977725.

Comment 73

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 2.c/6: completely remove nsIRILDataCallback : v2

update commit summary only.

Comment 74

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 2.d/6: don't throw in _deliverDataCallCallback loop : v2

update commit summary only.

Comment 75

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 3.a/6: RIL_Data_Call_Response_v5 has also DNS server list : v2

update commit summary only.

Comment 76

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 3.b/6: support multiple addresses in ril_worker : v2

update commit summary only.

Comment 77

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8389021 [details] [diff] [review]
Part 1: interface changes, v4.

Replaced by attachment 8390245 [details] [diff] [review].

Comment 78

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8389606 [details] [diff] [review]
Part 2: NetworkService changes, v3.

Replaced by attachment 8390308 [details] [diff] [review].

Comment 79

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8389607 [details] [diff] [review]
Part 3: NetworkUtils changes, v3.

Replaced by attachment 8393931 [details] [diff] [review].

Comment 80

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8390329 [details] [diff] [review]
(WIP) Changes for NetworkInterfaceListService, v1

Replaced by attachment 8393934 [details] [diff] [review].

Comment 81

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 5/6: really add an IPV4V6 option : v2

update commit summary only.

Comment 82

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 6/6: test cases : v2

update commit summary only.

Comment 83

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8389639 [details] [diff] [review]
Part 4: secondary routing changes, v2.

Replaced by attachment 8390249 [details] [diff] [review].

Comment 84

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8396234 [details] [diff] [review]
[clean-up] part 2.b/6: don't use nsIRILDataCallback in GonkGPSGeolocationProvider : v2

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ -783,5 @@
>  
> -  // We call Setting Service before we get the state of supl data connection
> -  // since it is possible that state of supl data connection haven't been
> -  // updated and will be updated after we finished this function (code that
> -  // updates the state is in another dataCallStateChanged callback).

Please keep this comment so it's clear why we need this indirection. Just remove the reference of dataCallStateChanged.

Comment 85

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8390308 [details] [diff] [review]
[clean-up] part 4.b/6: NetworkService changes to support IPV4V6 : v2

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

::: dom/system/gonk/NetworkService.js
@@ +284,5 @@
>    },
>  
>    setDNS: function(networkInterface) {
>      if(DEBUG) debug("Going DNS to " + networkInterface.name);
> +    let dnses = networkInterface.getDnses({});

nit: the first parameter of |nsINetworkInterface.getDnses()| is an optional output u_long integer.  You can simply omit them.  Same applies to |nsINetworkInterface.getGateways()|

Comment 86

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8393931 [details] [diff] [review]
[clean-up] part 4.c/6: NetworkUtils changes : v2

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

::: dom/system/gonk/NetworkUtils.cpp
@@ +854,5 @@
>                                     NetworkResultOptions& aResult)
>  {
>    char command[MAX_COMMAND_SIZE];
> +  int written = snprintf(command, MAX_COMMAND_SIZE - 1, "resolver setifdns %s %s",
> +                         GET_CHAR(mIfname), GET_CHAR(mDomain));

nit: `snprintf` writes at most (n - 1) characters, so the 2nd parameter here should be MAX_COMMAND_SIZE, or better, |sizeof command|.

@@ +868,5 @@
> +    }
> +    strcat(command, " ");
> +    strcat(command, autoDns.get());
> +
> +    written += strlen(autoDns.get()) + 1;

int ret = snprintf(command + written, sizeof(command) - written, " %s", autoDns.get());
if (ret <= 1) {
  command[written] = '\0';
  continue;
}

if ((ret + written) >= sizeof(command)) {
  command[written] = '\0';
  break;
}

@@ +1179,5 @@
> +    for (uint32_t i = 0; i < length; i++) {
> +      NS_ConvertUTF16toUTF8 autoDns(aOptions.mDnses[i]);
> +
> +      char dns_prop_key[PROPERTY_VALUE_MAX];
> +      snprintf(dns_prop_key, PROPERTY_VALUE_MAX - 1, "net.dns%d", i+1);

nit: `snprintf` writes at most (n - 1) characters, so the 2nd parameter here should be PROPERTY_VALUE_MAX, or better, |sizeof dns_prop_key|.

@@ +1305,5 @@
> +    uint32_t index = selectGateway(aOptions.mGateways, type);
> +    if (index < aOptions.mGateways.Length()) {
> +      strncpy(gateway, NS_ConvertUTF16toUTF8(aOptions.mGateways[index]).get(),
> +              sizeof(gateway) - 1);
> +    }

uint32_t index = selectGateway(aOptions.mGateways, type);
if (index >= aOptions.mGateways.Length()) {
  continue;
}

NS_ConvertUTF16toUTF8 autoGateway(aOptions.mGateways[index]);
prefix = type == AF_INET ? 32 : 128;
mNetUtils->do_ifc_add_route(autoIfname.get(), autoHostname.get(), prefix, autoGateway.get());

@@ +1333,5 @@
>      }
>  
> +    char gateway[128] = {0};
> +    uint32_t index = selectGateway(aOptions.mGateways, type);
> +    if (index < aOptions.mGateways.Length()) {

uint32_t index = selectGateway(aOptions.mGateways, type);
if (index >= aOptions.mGateways.Length()) {
  continue;
}

NS_ConvertUTF16toUTF8 autoGateway(aOptions.mGateways[index]);
prefix = type == AF_INET ? 32 : 128;
mNetUtils->do_ifc_remove_route(autoIfname.get(), autoHostname.get(), prefix, autoGateway.get());

Comment 87

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8390249 [details] [diff] [review]
[clean-up] part 4.d/4: secondary routes changes

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

::: dom/system/gonk/NetworkManager.js
@@ +503,5 @@
>      }
>    },
>  
>    setSecondaryDefaultRoute: function(network) {
> +    let gateways = network.getGateways({});

nit: omit {}

Comment 88

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8393933 [details] [diff] [review]
[clean-up] part 4.f/6: P2pNetworkInterface changes : v2

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

::: dom/wifi/WifiP2pManager.jsm
@@ +505,5 @@
> +      if (!this.ips || !this.prefixLengths ||
> +          (this.ips.length != this.prefixLengths.length)) {
> +        ips.value = prefixLengths.value = [];
> +        return 0;
> +      }

nit: don't need such check because we have always a valid array-typed |this.ips|.

Comment 89

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8390255 [details] [diff] [review]
[clean-up] part 4.g/6: WifiNetworkInterface changes

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

::: dom/wifi/WifiWorker.js
@@ +1533,5 @@
> +    if (!this.ips || !this.prefixLengths ||
> +        (this.ips.length != this.prefixLengths.length)) {
> +      ips.value = prefixLengths.value = [];
> +      return 0;
> +    }

nit: don't need such check because we have always a valid array-typed |this.ips|.

Comment 90

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8393934 [details] [diff] [review]
[clean-up] part 4.h/6: NetworkInterfaceListService changes : v3

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

::: dom/media/tests/mochitest/pc.js
@@ +347,5 @@
> +
> +      for (var j = 0; j < length; j++) {
> +        var ip = ips.value[j];
> +        // skip IPv6 address
> +        if (ip && ip.indexOf(":") < 0) {

nit: ip should be always valid here, or it's a bug.

::: media/mtransport/gonk_addrs.cpp
@@ +68,5 @@
>      NetworkInterface interface;
>      memset(&(interface.addr), 0, sizeof(interface.addr));
>      interface.addr.sin_family = AF_INET;
> +
> +    if (NS_FAILED(iface->GetAddresses(&ips, &prefixs, &count))) {

We have to free memory allocated by GetAddresses().

Comment 91

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8389825 [details] [diff] [review]
Part 5: wifi and ril changes, v3.

Replaced by attachment 8390338 [details] [diff] [review](4.e), attachment 8393933 [details] [diff] [review](4.f), and attachment 8390225 [details] [diff] [review](4.g).

Comment 92

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8389219 [details] [diff] [review]
(WIP) Changes for gonk_addrs.cpp, v2

Replaced by attachment 8393934 [details] [diff] [review](4.h).

Comment 93

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8396230 [details] [diff] [review]
[clean-up] part 2.a/6: remove nsIRILDataCallback::receiveDataCallList : v2

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

Nice clean-up!

Comment 94

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8396230 [details] [diff] [review]
[clean-up] part 2.a/6: remove nsIRILDataCallback::receiveDataCallList : v2

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

Oh, just caught a miss.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ -1558,5 @@
> -   */
> -  handleDataCallList: function(message) {
> -    this._deliverDataCallCallback("receiveDataCallList",
> -                                  [message.datacalls, message.datacalls.length]);
> -  },

If we remove this, then how about [1]? We will get a Javascript referenced error. 

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js?from=RadioInterfaceLayer.js&case=true#2175

Comment 95

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8396237 [details] [diff] [review]
[clean-up] part 2.d/6: don't throw in _deliverDataCallCallback loop : v2

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

Looks okay but I don't get a reason to enforce this. Could you share your thoughts?

Comment 96

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #95)
> Comment on attachment 8396237 [details] [diff] [review]
> [clean-up] part 2.d/6: don't throw in _deliverDataCallCallback loop : v2
> -----------------------------------------------------------------
> Looks okay but I don't get a reason to enforce this. Could you share your
> thoughts?

When we register multiple callbacks to an event target, we expect these callback being called if a certain condition meets.  However, if one of the callbacks throws or the dispatching function throws in the dispatching loop, the callbacks standing in line after that executed/to be executed one starve.  That's why we have a try-block in the same function.

Comment 97

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8396280 [details] [diff] [review]
[clean-up] part 3.b/6: support multiple addresses in ril_worker : v2

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2153,5 @@
>          connHandler.handleDataCallError(message);
>          break;
>        case "datacallstatechange":
> +        let addresses = [];
> +        for (let entry of message.addresses) {

[1] shows performance issue of for-of is still valid. Let's consider trying our best to avoid that.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=969231#c9

@@ +4366,5 @@
>           datacall.state == RIL.GECKO_NETWORK_STATE_CONNECTED)) {
>        this.connecting = false;
>        this.cid = datacall.cid;
>        this.name = datacall.ifname;
> +      this.ip = datacall.addresses[0].address;

Assume you are going to support multiple addresses in the following patches.

::: dom/system/gonk/ril_worker.js
@@ +3883,5 @@
> +      return "deactivate";
> +    }
> +
> +    // If any existing address is missing, report as "deactivate".
> +    for (let address of currentDataCall.addresses) {

[1] shows performance issue of for-of is still valid. Let's consider trying our best to avoid that.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=969231#c9

@@ +3896,5 @@
> +      // reported.
> +      return "changed";
> +    }
> +
> +    for (let field of ["gateways", "dnses"]) {

ditto.

Comment 98

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8396322 [details] [diff] [review]
[clean-up] part 5/6: really add an IPV4V6 option : v2

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

::: dom/system/gonk/ril_consts.js
@@ +2366,5 @@
>  this.GECKO_DATACALL_PDP_TYPE_IPV6 = "IPV6";
>  this.GECKO_DATACALL_PDP_TYPE_DEFAULT = GECKO_DATACALL_PDP_TYPE_IP;
>  this.RIL_DATACALL_PDP_TYPES = [
>    GECKO_DATACALL_PDP_TYPE_IP,
> +  GECKO_DATACALL_PDP_TYPE_IPV4V6,

This part looks good but kindly me point me where the multiple addresses are handled. I didn't see that part in the pieces I reviewed.

Comment 99

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #96)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #95)
> > Comment on attachment 8396237 [details] [diff] [review]
> > [clean-up] part 2.d/6: don't throw in _deliverDataCallCallback loop : v2
> > -----------------------------------------------------------------
> > Looks okay but I don't get a reason to enforce this. Could you share your
> > thoughts?
> 
> When we register multiple callbacks to an event target, we expect these
> callback being called if a certain condition meets.  However, if one of the
> callbacks throws or the dispatching function throws in the dispatching loop,
> the callbacks standing in line after that executed/to be executed one
> starve.  That's why we have a try-block in the same function.

Oh, got it! Thanks. :)

Comment 100

[Jessica Jong [:jessica] (inactive)]

Attachment: [clean-up] part 4.b/6: NetworkService changes to support IPV4V6 : v3

Address review comment 85:
- omit the parameter '{}' in |nsINetworkInterface.getDnses()| and |nsINetworkInterface.getGateways()|

Comment 101

[Jessica Jong [:jessica] (inactive)]

Attachment: [clean-up] part 4.c/6: NetworkUtils changes : v3

Address review comment 86:
- use a more appropriate parameter for snprintf
- refine |setInterfaceDns()| part
- refine the handling of the result of |selectGateway()|

Comment 102

[Jessica Jong [:jessica] (inactive)]

Attachment: [clean-up] part 4.d/4: secondary routes changes : v2

Address review comment 87:
- Omit parameter {} in |network.getGateways()|

Comment 103

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #94)
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ -1558,5 @@
> > -   */
> > -  handleDataCallList: function(message) {
> > -    this._deliverDataCallCallback("receiveDataCallList",
> > -                                  [message.datacalls, message.datacalls.length]);
> > -  },
> 
> If we remove this, then how about [1]? We will get a Javascript referenced
> error. 

Oops! But we still won't get an error because nobody ever calls ril_worker:enumerateDataCalls, so "datacalllist" is never dispatched, and this function is never called.  Will add another patch to remove them completely.

Comment 104

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8396323 [details] [diff] [review]
[clean-up] part 6/6: test cases : v2

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

Thank you!!!

Comment 105

[Jessica Jong [:jessica] (inactive)]

Attachment: [clean-up] part 4.f/6: P2pNetworkInterface changes : v3

Address review comment 88:
- remove extra checks

Comment 106

[Jessica Jong [:jessica] (inactive)]

Attachment: [clean-up] part 4.g/6: WifiNetworkInterface changes : v2

Address review comment 89:
- remove extra checks

Comment 107

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #97)
> Comment on attachment 8396280 [details] [diff] [review]
> [clean-up] part 3.b/6: support multiple addresses in ril_worker : v2
> -----------------------------------------------------------------
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +4366,5 @@
> >           datacall.state == RIL.GECKO_NETWORK_STATE_CONNECTED)) {
> >        this.connecting = false;
> >        this.cid = datacall.cid;
> >        this.name = datacall.ifname;
> > +      this.ip = datacall.addresses[0].address;
> 
> Assume you are going to support multiple addresses in the following patches.

They are attachment 8390338 [details] [diff] [review] (4.e, RIL), attachment 8393933 [details] [diff] [review] (4.f, P2P), and attachment 8390225 [details] [diff] [review] (4.g, WiFi).

Comment 108

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8396323 [details] [diff] [review]
[clean-up] part 6/6: test cases : v2

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

I should also update "manifest.ini" with "disabled = bug 979137".

Comment 109

[Jessica Jong [:jessica] (inactive)]

Attachment: [clean-up] part 4.h/6: NetworkInterfaceListService changes : v4

Address review comment 90:
- remove extra check for |ip|
- free memory allocated by |GetAddresses()|

Comment 110

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 2.a/6: remove nsIRILDataCallback::receiveDataCallList : v3

Address review comment in comment 94.

Comment 111

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 2.b/6: don't use nsIRILDataCallback in GonkGPSGeolocationProvider : v3

Rebase & r=kanru.

Comment 112

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 2.e/6: remove unused data call enumeration stuff

Comment 113

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 3.b/6: support multiple addresses in ril_worker : v3

Comment 114

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [clean-up] part 6/6: test cases : v3

Comment 115

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8397719 [details] [diff] [review]
[clean-up] part 4.c/6: NetworkUtils changes : v3

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

::: dom/system/gonk/NetworkUtils.cpp
@@ +1305,5 @@
>      if (type != AF_INET && type != AF_INET6) {
>        continue;
>      }
>  
> +    char gateway[128] = {0};

nit: unused variable.

@@ +1336,5 @@
>      if (type != AF_INET && type != AF_INET6) {
>        continue;
>      }
>  
> +    char gateway[128] = {0};

nit: unused variable.

Comment 116

[Vicamo Yang [:vicamo][:vyang]]

Full try: https://tbpl.mozilla.org/?tree=Try&rev=7dd549bb5a4e
My WIP branch: https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/978711/clean-up

Comment 117

[Jessica Jong [:jessica] (inactive)]

Attachment: [clean-up] part 4.c/6: NetworkUtils changes : v4

Address nits in comment 115:
- removed unused variables

Comment 118

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8397767 [details] [diff] [review]
[clean-up] part 3.b/6: support multiple addresses in ril_worker : v3

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

Thank you~

Comment 119

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/357915343e5c
https://hg.mozilla.org/integration/b2g-inbound/rev/2bbef391c9de
https://hg.mozilla.org/integration/b2g-inbound/rev/3c5bf7672c15
https://hg.mozilla.org/integration/b2g-inbound/rev/0bd14c1f55cc
https://hg.mozilla.org/integration/b2g-inbound/rev/c09704cbfa80
https://hg.mozilla.org/integration/b2g-inbound/rev/8a66c3d82619
https://hg.mozilla.org/integration/b2g-inbound/rev/bb0a18fca85c
https://hg.mozilla.org/integration/b2g-inbound/rev/b0ad09fbb31b
https://hg.mozilla.org/integration/b2g-inbound/rev/95491050118f
https://hg.mozilla.org/integration/b2g-inbound/rev/d58b069a7e89
https://hg.mozilla.org/integration/b2g-inbound/rev/a77ba205d922
https://hg.mozilla.org/integration/b2g-inbound/rev/af837a37ec6d
https://hg.mozilla.org/integration/b2g-inbound/rev/4e4bf4bed57a
https://hg.mozilla.org/integration/b2g-inbound/rev/ea0d1b6abcb8
https://hg.mozilla.org/integration/b2g-inbound/rev/886854c3fee2
https://hg.mozilla.org/integration/b2g-inbound/rev/71d166d7aece
https://hg.mozilla.org/integration/b2g-inbound/rev/5b801a301e1d
https://hg.mozilla.org/integration/b2g-inbound/rev/f3f89bd31a49

Comment 120

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/357915343e5c
https://hg.mozilla.org/mozilla-central/rev/2bbef391c9de
https://hg.mozilla.org/mozilla-central/rev/3c5bf7672c15
https://hg.mozilla.org/mozilla-central/rev/0bd14c1f55cc
https://hg.mozilla.org/mozilla-central/rev/c09704cbfa80
https://hg.mozilla.org/mozilla-central/rev/8a66c3d82619
https://hg.mozilla.org/mozilla-central/rev/bb0a18fca85c
https://hg.mozilla.org/mozilla-central/rev/b0ad09fbb31b
https://hg.mozilla.org/mozilla-central/rev/95491050118f
https://hg.mozilla.org/mozilla-central/rev/d58b069a7e89
https://hg.mozilla.org/mozilla-central/rev/a77ba205d922
https://hg.mozilla.org/mozilla-central/rev/af837a37ec6d
https://hg.mozilla.org/mozilla-central/rev/4e4bf4bed57a
https://hg.mozilla.org/mozilla-central/rev/ea0d1b6abcb8
https://hg.mozilla.org/mozilla-central/rev/886854c3fee2
https://hg.mozilla.org/mozilla-central/rev/71d166d7aece
https://hg.mozilla.org/mozilla-central/rev/5b801a301e1d
https://hg.mozilla.org/mozilla-central/rev/f3f89bd31a49

Comment 121

[Jessica Jong [:jessica] (inactive)]

Hi Anshul,

The patches are already in trunk. Could someone from you help to test if this bug work as expected? as we don't have IPv4/v6 environment in Taiwan.

Thanks a lot.

Comment 122

[Anshul]

Sure Jessica. Phil will give this a try once we are done adapting to this new interface. Please expect a few days of delay from our side.

Comment 123

[pgravel]

Initial sanity tests look good. Verified both IPv4 and IPv6 routes are useable while in IPv4v6 mode.

Comment 124

[Jessica Jong [:jessica] (inactive)]

(In reply to pgravel from comment #123)
> Initial sanity tests look good. Verified both IPv4 and IPv6 routes are
> useable while in IPv4v6 mode.

Thanks pgravel and Anshul, that's good to hear!
Please let us know if you have any other findings.

Comment 125

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #111)
> Created attachment 8397763 [details] [diff] [review]
> [clean-up] part 2.b/6: don't use nsIRILDataCallback in
> GonkGPSGeolocationProvider : v3
> 
> Rebase & r=kanru.

This part fails Flatfish build.  Will file a follow-up and fix it as soon as possible.

Comment 126

[Anshul]

Vicamo, shouldn't this patch be uplifted to 1.4 as well?

Comment 127

[Anshul]

Preeti, Madai is asking us about this feature and wondering if you could get this uplifted to 1.4.

Comment 128

[Wayne Chang [:wchang]]

Target network had confirmed this is not needed during our last meet up, and discussed again in 3 way meeting yesterday with madai partners.

Adding partners, who were going to confirm again on this requirement.

Comment 129

[Jason Smith [:jsmith]]

Per comment 128, not blocking.

Comment 130

[Wayne Chang [:wchang]]

Confirmed with partner that IPv4v6 over one interface is not required.

Comment 131

[Chrys Choi]

This feature has implemented on v2.0.