Closed Bug 976959 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S2 (28feb)

People

(Reporter: kchang, Assigned: edgar)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Hi Edgar, please help this bug.
Assignee: nobody → echen
Blocks: 957917
Attached patch WIP, Patch, v1 (obsolete) — Splinter Review
Just a quick patch, not verify yet.
Component: RIL → General
Summary: [Gecko] To change the netmask to prefix for data call. → [B2G] Change the netmask to prefix length for IPv6 supporting
Attached patch Patch, v2 (obsolete) — Splinter Review
Attachment #8382054 - Attachment is obsolete: true
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.
Attachment #8382125 - Flags: review?(vchang)
Attachment #8382125 - Flags: review?(htsai)
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 on attachment 8382125 [details] [diff] [review]
Patch, v2

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

Looks good for me. Thank you.
Attachment #8382125 - Flags: review?(vchang) → review+
(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 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.
Blocks: 977474
(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.
(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.
Attachment #8382125 - Flags: review?(htsai) → review+
1). Rebase
2). Address review comment #8. 
    - Remove "this._activeInfo".
Attachment #8382125 - Attachment is obsolete: true
Attachment #8382783 - Flags: review+
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);
I am waiting the result of try server: https://tbpl.mozilla.org/?tree=Try&rev=60f52dcf90fd
(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.
Summary: [B2G] Change the netmask to prefix length for IPv6 supporting → [B2G] Change the netmask to prefix length for supporting IPv6
(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
https://hg.mozilla.org/mozilla-central/rev/f68dd0e6a6ee
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S2 (28feb)
You need to log in before you can comment on or make changes to this bug.