976959 - [B2G] Change the netmask to prefix length for supporting IPv6

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Ken Chang[:kenkai]]

When data call is established, ril will get the prefix from data call information,and then this prefix value is going to be changed into netmask value. 

https://github.com/mozilla/gecko-dev/blob/master/dom/system/gonk/RadioInterfaceLayer.js#L2169
...
          let prefix_len = message.ipaddr.split("/")[1];
          let mask_value = netHelpers.makeMask(prefix_len);
...

However, this netmask value isn't adapt by other components. Moreover, to use netmask makes implementing IPv6 hard. We should use prefix in ril.

Comment 1

[Ken Chang[:kenkai]]

Hi Edgar, please help this bug.

Comment 2

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: WIP, Patch, v1

Just a quick patch, not verify yet.

Comment 3

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Patch, v2

Comment 4

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Comment on attachment 8382125 [details] [diff] [review]
Patch, v2

Hi Hsinyi, Vincent:

There is no netmask in IPv6, it uses "prefix" to calculate subnet instead.
In this patch, I replace netmask by using prefix directly, so that we could support both IPv4 and IPv6.
The modification crosses different modules, like WIFI/RIL/NetworkManager/..., so I send r? to you both.

I had tested below scenario in unagi:
1). Connect to 3G data.
2). Connect to Wifi.
3). Send MMS + 3G.
4). Send MMS + Wifi.
5). 3G + USB Tethering.
6). 3G + Wifi Hotspot.

Thank you.

Comment 5

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8382125 [details] [diff] [review]
Patch, v2

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

::: dom/webidl/NetworkOptions.webidl
@@ +13,5 @@
>                                        //     "setDefaultRouteAndDNS", "removeDefaultRoute"
>                                        //     "addHostRoute", "removeHostRoute"
>                                        //     "removeHostRoutes".
>    DOMString ip;                       // for "removeNetworkRoute", "setWifiTethering".
> +  unsigned long prefixLength;         // for "removeNetworkRoute".

There's already a prefix, why not use the same parameter?

Comment 6

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8382125 [details] [diff] [review]
Patch, v2

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

Looks good for me. Thank you.

Comment 7

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

(In reply to Chuck Lee [:chucklee] from comment #5)
> Comment on attachment 8382125 [details] [diff] [review]
> Patch, v2
> 
> Review of attachment 8382125 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/NetworkOptions.webidl
> @@ +13,5 @@
> >                                        //     "setDefaultRouteAndDNS", "removeDefaultRoute"
> >                                        //     "addHostRoute", "removeHostRoute"
> >                                        //     "removeHostRoutes".
> >    DOMString ip;                       // for "removeNetworkRoute", "setWifiTethering".
> > +  unsigned long prefixLength;         // for "removeNetworkRoute".
> 
> There's already a prefix, why not use the same parameter?

prefix is defined as "string", if we want to reuse "prefix", I have to review all code again to make sure we won't encounter type issue. And I found another similar parameter, 'maskLength', I think we could combine them all. However, due to the schedule pressure, I propose doing it in a follow-up.

Comment 8

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

Comment on attachment 8382125 [details] [diff] [review]
Patch, v2

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

Generally good but I have concerns on webidl. r=me with comment addressed or answered. Thank you.

::: dom/system/gonk/NetworkManager.js
@@ +500,5 @@
>          defaultDataNetwork = network;
>        }
>  #endif
>        this.active = network;
> +      this._activeInfo = {name:network.name, ip:network.ip, prefixLength:network.prefixLength};

Seems this._activeInfo is never referenced, should we remove it?

::: dom/webidl/NetworkOptions.webidl
@@ +13,5 @@
>                                        //     "setDefaultRouteAndDNS", "removeDefaultRoute"
>                                        //     "addHostRoute", "removeHostRoute"
>                                        //     "removeHostRoutes".
>    DOMString ip;                       // for "removeNetworkRoute", "setWifiTethering".
> +  unsigned long prefixLength;         // for "removeNetworkRoute".

I might miss the conext when we introduced 'DOMString prefix'. But I am wondering why the type of existing prefix is DOMString. It looks that "unsigned long" is the right type instead.

Agree with Chuck that it's not good to have two attributes standing for the same thing. However, IMHO 'prefixLength' is clearer and better represents the meaning of this attribute. So the change here seems good, and I'd like to see 'DOMString prefix' being replaced. The only concern is as I am not sure if API user is currently using 'DOMString prefix' it's acceptable for me to remove 'DOMString prefix' afterwards.

Please let me know if I miss anything.

Comment 9

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #8)
> Comment on attachment 8382125 [details] [diff] [review]
> Patch, v2
> 
> Review of attachment 8382125 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Generally good but I have concerns on webidl. r=me with comment addressed or
> answered. Thank you.
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +500,5 @@
> >          defaultDataNetwork = network;
> >        }
> >  #endif
> >        this.active = network;
> > +      this._activeInfo = {name:network.name, ip:network.ip, prefixLength:network.prefixLength};
> 
> Seems this._activeInfo is never referenced, should we remove it?

Okay, I will remove it in next version.

> 
> ::: dom/webidl/NetworkOptions.webidl
> @@ +13,5 @@
> >                                        //     "setDefaultRouteAndDNS", "removeDefaultRoute"
> >                                        //     "addHostRoute", "removeHostRoute"
> >                                        //     "removeHostRoutes".
> >    DOMString ip;                       // for "removeNetworkRoute", "setWifiTethering".
> > +  unsigned long prefixLength;         // for "removeNetworkRoute".
> 
> I might miss the conext when we introduced 'DOMString prefix'. But I am
> wondering why the type of existing prefix is DOMString. It looks that
> "unsigned long" is the right type instead.
> 
> Agree with Chuck that it's not good to have two attributes standing for the
> same thing. However, IMHO 'prefixLength' is clearer and better represents
> the meaning of this attribute. So the change here seems good, and I'd like
> to see 'DOMString prefix' being replaced. The only concern is as I am not
> sure if API user is currently using 'DOMString prefix' it's acceptable for
> me to remove 'DOMString prefix' afterwards.

NetworkOptions is an internal interface and only been used between NetworkService.js and NetworkUtil.cpp. So I think it is okay to combine them. BTW, there is another similar parameter, 'DOMString maskLength', we should consider as well. Let's do it in a follow-up, bug 977474.

Thank you.

> 
> Please let me know if I miss anything.

Comment 10

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

(In reply to Edgar Chen [:edgar][:echen] from comment #9)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #8)
> > Comment on attachment 8382125 [details] [diff] [review]
> > Patch, v2
> > 
> > Review of attachment 8382125 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Generally good but I have concerns on webidl. r=me with comment addressed or
> > answered. Thank you.
> > 
> > ::: dom/system/gonk/NetworkManager.js
> > @@ +500,5 @@
> > >          defaultDataNetwork = network;
> > >        }
> > >  #endif
> > >        this.active = network;
> > > +      this._activeInfo = {name:network.name, ip:network.ip, prefixLength:network.prefixLength};
> > 
> > Seems this._activeInfo is never referenced, should we remove it?
> 
> Okay, I will remove it in next version.
> 
> > 
> > ::: dom/webidl/NetworkOptions.webidl
> > @@ +13,5 @@
> > >                                        //     "setDefaultRouteAndDNS", "removeDefaultRoute"
> > >                                        //     "addHostRoute", "removeHostRoute"
> > >                                        //     "removeHostRoutes".
> > >    DOMString ip;                       // for "removeNetworkRoute", "setWifiTethering".
> > > +  unsigned long prefixLength;         // for "removeNetworkRoute".
> > 
> > I might miss the conext when we introduced 'DOMString prefix'. But I am
> > wondering why the type of existing prefix is DOMString. It looks that
> > "unsigned long" is the right type instead.
> > 
> > Agree with Chuck that it's not good to have two attributes standing for the
> > same thing. However, IMHO 'prefixLength' is clearer and better represents
> > the meaning of this attribute. So the change here seems good, and I'd like
> > to see 'DOMString prefix' being replaced. The only concern is as I am not
> > sure if API user is currently using 'DOMString prefix' it's acceptable for
> > me to remove 'DOMString prefix' afterwards.
> 
> NetworkOptions is an internal interface and only been used between
> NetworkService.js and NetworkUtil.cpp. So I think it is okay to combine
> them. BTW, there is another similar parameter, 'DOMString maskLength', we
> should consider as well. Let's do it in a follow-up, bug 977474.
> 

Sounds good!

> Thank you.
> 
> > 
> > Please let me know if I miss anything.

Comment 11

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Patch, v3, r=vchang,hsinyi

1). Rebase
2). Address review comment #8. 
    - Remove "this._activeInfo".

Comment 12

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

Comment on attachment 8382783 [details] [diff] [review]
Patch, v3, r=vchang,hsinyi

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

::: dom/system/gonk/NetworkUtils.cpp
@@ +217,4 @@
>  {
> +  uint32_t mask = 0;
> +  for (uint32_t i = 0; i < prefixLength; ++i) {
> +    mask |= (0x80000000 >> i);

uint32_t mask = ~0x0 << (sizeof uint32_t * 8 - prefixLength);
return ntohl(mask);

Comment 13

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

I am waiting the result of try server: https://tbpl.mozilla.org/?tree=Try&rev=60f52dcf90fd

Comment 14

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #12)
> Comment on attachment 8382783 [details] [diff] [review]
> Patch, v3, r=vchang,hsinyi
> 
> Review of attachment 8382783 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkUtils.cpp
> @@ +217,4 @@
> >  {
> > +  uint32_t mask = 0;
> > +  for (uint32_t i = 0; i < prefixLength; ++i) {
> > +    mask |= (0x80000000 >> i);
> 
> uint32_t mask = ~0x0 << (sizeof uint32_t * 8 - prefixLength);
> return ntohl(mask);

Will address this in bug 977474. Thank you.

Comment 15

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

(In reply to Edgar Chen [:edgar][:echen] from comment #13)
> I am waiting the result of try server:
> https://tbpl.mozilla.org/?tree=Try&rev=60f52dcf90fd

All green \o/

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f68dd0e6a6ee