767375 - B2G network manager: expose IP Information of network interfaces

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Thinker Li [:sinker]]

Network manager should expose IP information of network interfaces; like IP Address, netmask, broadcasting address, DNS, default gateway, ...  These information are required for network services running on devices to keep the implementations portable.

Comment 1

[Philipp von Weitershausen [:philikon]]

What exactly do you mean by "portable"? And are you talking about a Gecko-internal API (e.g. on one of the nsINetwork* interfaces) or a DOM API?

Comment 2

[Thinker Li [:sinker]]

I am talking about nsINetwork* interfaces.  The portable means the code would not locked into APIs of Linux or windows to retrieve IP information of NICs.  I think network manager is good place to provide this kind of information.

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to Thinker Li [:sinker] from comment #2)
> I am talking about nsINetwork* interfaces.  The portable means the code
> would not locked into APIs of Linux or windows to retrieve IP information of
> NICs.  I think network manager is good place to provide this kind of
> information.

Agreed. Might be a good idea to expose this stuff on nsINetworkInterface.

Comment 4

[Shian-Yow Wu [:swu]]

Attachment: Part 1: IDL

This is the proposed IP information to expose.
Please advice if these are enough, or anything should be changed.

Comment 5

[Shian-Yow Wu [:swu]]

For bug 762426, we may also need to expose MMS proxy address on nsINetworkInterface.  For consistency (HTTP proxy was just added to nsINetworkInterface by bug 740640) and convenience, we can do the same thing for MMS proxy.
But MMS proxy is only valid for data call, anyone think we shouldn't expose it in nsINetworkInterface?  If not, we'll consider to add it in next version of IDL.

Comment 6

[Shian-Yow Wu [:swu]]

After discussing with Vicamo, because MMS proxy is only referenced internally by MMS Service, he suggested to retrieve MMS proxy from settings API instead.  So current IDL proposal will be kept same as comment 4.

Comment 7

[Shian-Yow Wu [:swu]]

Attachment: Part 2: RIL implementation (WIP v1)

We'll need to set IP information when RIL/Wifi registering to network manager.

This is the implementation on RIL part.

We need to use net helper functions in systemlib.js, but when importing systemlib.js from RadioInterfaceLayer.js, there will be the following error:
E/GeckoConsole(  105): [JavaScript Error: "ReferenceError: ctypes is not defined" {file: "resource://gre/modules/systemlibs.js" line: 120}]
Importing ctypes.jsm in RadioInterfaceLayer.js doesn't help.
Maybe it can be solved by doing something like Bug 735589?  Not sure if it worths for this case.

Also, the original net helpers in systemlibs.js didn't generate expected results in some conditions.  For example, the ipToString(makeMask(1)) supposed to return "128.0.0.0", but it actually returns "1.0.0.0".  I created another netlib.js and re-wrote the net helpers to generate correct results.

Comment 8

[Philipp von Weitershausen [:philikon]]

(In reply to Shian-Yow Wu from comment #7)
> Also, the original net helpers in systemlibs.js didn't generate expected
> results in some conditions.  For example, the ipToString(makeMask(1))
> supposed to return "128.0.0.0", but it actually returns "1.0.0.0".  I
> created another netlib.js and re-wrote the net helpers to generate correct
> results.

Please use `hg copy` to when splitting a file. That way both files carry the history (hg blame). But I don't think we need to split `netHelpers` out of systemlibs.js. We could (and IMHO should) just pre-compute all values in ril_worker.js and send them to the main thread.

Comment 9

[Philipp von Weitershausen [:philikon]]

Comment on attachment 648686 [details] [diff] [review]
Part 2: RIL implementation (WIP v1)

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1766,5 @@
>  
> +  ip: null,
> +
> +  netmask: null,
> + 

Nit: trailing whitespace.

@@ +1794,5 @@
> +      let ip_value = netHelpers.stringToIP(this.ip);
> +      let prefix_len = datacall.ipaddr.split("/")[1];
> +      let mask_value = netHelpers.makeMask(prefix_len);
> +      this.netmask = netHelpers.ipToString(mask_value);
> +      this.broadcast = netHelpers.ipToString((ip_value & mask_value) + ~mask_value);

Precompute these in ril_worker.js. In fact, making a helper that precomputes these values would be very useful because we can probably make use of it for Wifi as well.

Comment 10

[Shian-Yow Wu [:swu]]

(In reply to Philipp von Weitershausen [:philikon] from comment #8)
> Please use `hg copy` to when splitting a file. That way both files carry the
> history (hg blame). But I don't think we need to split `netHelpers` out of
> systemlibs.js. We could (and IMHO should) just pre-compute all values in
> ril_worker.js and send them to the main thread.

Thanks for your comments.  Yes, if we do it in ril_worker.js, then we don't need to split those helpers.

(In reply to Philipp von Weitershausen [:philikon] from comment #9)
> Precompute these in ril_worker.js. In fact, making a helper that precomputes
> these values would be very useful because we can probably make use of it for
> Wifi as well.

Do you mean making helpers in systemlib.js for ril_worker.js and wifi_worker.js to precompute these values?  Please correct if I misunderstand your point.

Comment 11

[Philipp von Weitershausen [:philikon]]

(In reply to Shian-Yow Wu from comment #10)
> (In reply to Philipp von Weitershausen [:philikon] from comment #9)
> > Precompute these in ril_worker.js. In fact, making a helper that precomputes
> > these values would be very useful because we can probably make use of it for
> > Wifi as well.
> 
> Do you mean making helpers in systemlib.js for ril_worker.js and
> wifi_worker.js to precompute these values?  Please correct if I
> misunderstand your point.

Yes. Put it/them in systemlibs.js. I think both ril_worker.js and wifi_worker.js import it already anyway.

Comment 12

[Shian-Yow Wu [:swu]]

Attachment: Part 2: RIL/Wifi implementation (WIP v2)

This includes implementation on RIL and Wifi, please help to review, thank you!

Comment 13

[Philipp von Weitershausen [:philikon]]

Comment on attachment 649626 [details] [diff] [review]
Part 2: RIL/Wifi implementation (WIP v2)

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

::: dom/system/gonk/net_worker.js
@@ +61,5 @@
>    if (!options.gateway || !options.dns1_str) {
>      options = getIFProperties(options.ifname);
>    }
>  
> +  libnetutils.ifc_set_default_route(options.ifname, netHelpers.htonl(options.gateway));

`options.gateway` is computed as `netHelpers.stringToIP(gateway_str)`. As far as I can tell, you've changed stringToIP and ipToString to work in big-endian (network byte order). So `options.gateway` should be in network byte order.

So why would you still need to convert from host to network byte order here?

Perhaps `libnetutils.ifc_set_default_route` takes host byte order and you meant network to host (ntohl)?

I'm confused, sorry! :)

::: dom/system/gonk/ril_worker.js
@@ +3470,5 @@
>    }
> +  options.ip = null;
> +  options.netmask = null;
> +  options.broadcast = null;
> +  if(options.ipaddr) {

Nit: space between `if` and `(`

::: dom/system/gonk/systemlibs.js
@@ +219,3 @@
>        };
>        obj.ipaddr = netHelpers.stringToIP(obj.ipaddr_str);
> +      obj.mask_str = netHelpers.ipToString(obj.mask),

s/,/;/

@@ +293,5 @@
> +   * Convert host byte order to network byte order
> +   * Note: Assume that the system is little endian
> +   */
> +  htonl: function htonl(n) {
> +    return (((n >> 24) & 0xFF) <<  0) | 

Nit: trailing space

Comment 14

[Shian-Yow Wu [:swu]]

(In reply to Philipp von Weitershausen [:philikon] from comment #13)
> `options.gateway` is computed as `netHelpers.stringToIP(gateway_str)`. As
> far as I can tell, you've changed stringToIP and ipToString to work in
> big-endian (network byte order). So `options.gateway` should be in network
> byte order.
> 
> So why would you still need to convert from host to network byte order here?
> 
> Perhaps `libnetutils.ifc_set_default_route` takes host byte order and you
> meant network to host (ntohl)?
> 
> I'm confused, sorry! :)
> 

Thank you for this question.  Actually this is something I'm a little confused too.  The 'libnetutils.ifc_set_default_route' expects type 'in_addr_t'(which means network byte order) on gateway parameter.  But according to real testing, seems it takes host byte order.

Maybe another way is to keep stringToIP and ipToString in little-endian, and do a ntohl() when computing mask in makeMask().  I'll check it further.

> ::: dom/system/gonk/ril_worker.js
> @@ +3470,5 @@
> >    }
> > +  options.ip = null;
> > +  options.netmask = null;
> > +  options.broadcast = null;
> > +  if(options.ipaddr) {
> 
> Nit: space between `if` and `(`
> 

Ok, thanks.

> ::: dom/system/gonk/systemlibs.js
> @@ +219,3 @@
> >        };
> >        obj.ipaddr = netHelpers.stringToIP(obj.ipaddr_str);
> > +      obj.mask_str = netHelpers.ipToString(obj.mask),
> 
> s/,/;/
> 

Oops, thanks for point out this error!

> @@ +293,5 @@
> > +   * Convert host byte order to network byte order
> > +   * Note: Assume that the system is little endian
> > +   */
> > +  htonl: function htonl(n) {
> > +    return (((n >> 24) & 0xFF) <<  0) | 
> 
> Nit: trailing space

Ok, thanks.  After submitting patch, I can see this error in splinter review of bugzilla, but I cannot see it in the diff.  Is there any good way to prevent this kind of error before submitting the patch?

Comment 15

[Shian-Yow Wu [:swu]]

(In reply to Philipp von Weitershausen [:philikon] from comment #13)
> Comment on attachment 649626 [details] [diff] [review]
> Part 2: RIL/Wifi implementation (WIP v2)
> 
> Review of attachment 649626 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/net_worker.js
> @@ +61,5 @@
> >    if (!options.gateway || !options.dns1_str) {
> >      options = getIFProperties(options.ifname);
> >    }
> >  
> > +  libnetutils.ifc_set_default_route(options.ifname, netHelpers.htonl(options.gateway));
> 
> `options.gateway` is computed as `netHelpers.stringToIP(gateway_str)`. As
> far as I can tell, you've changed stringToIP and ipToString to work in
> big-endian (network byte order). So `options.gateway` should be in network
> byte order.
> 
> So why would you still need to convert from host to network byte order here?
> 
> Perhaps `libnetutils.ifc_set_default_route` takes host byte order and you
> meant network to host (ntohl)?
> 
> I'm confused, sorry! :)

After some study, below's my understanding.

For IP address 10.1.2.3, we should compute the value to be 0x0A010203.  Depending on little or big endian system,
the value is stored in memory with different orders, we all call it host byte order.  When calling a function that expects network byte order as input value, we then do a htonl() to convert the value from host to network byte order.
The htonl() function either reverses the byte orders on little endian system or does nothing on big endian system.

The original version of stringToIP and ipToString assume the system to be little endian, so the value is computed in reversed byte order in advance, that's why it doesn't need convert `options.gateway` by htonl() before calling `libnetutils.ifc_set_default_route`.

Either way can generate correct result.  IMHO, the changed version of stringToIP and ipToString without converting it to little endian in advance may be easier to understand.

How do you think?

Comment 16

[Philipp von Weitershausen [:philikon]]

(In reply to Shian-Yow Wu from comment #15)
> For IP address 10.1.2.3, we should compute the value to be 0x0A010203. 
> Depending on little or big endian system,
> the value is stored in memory with different orders, we all call it host
> byte order.  When calling a function that expects network byte order as
> input value, we then do a htonl() to convert the value from host to network
> byte order.
> The htonl() function either reverses the byte orders on little endian system
> or does nothing on big endian system.
> 
> The original version of stringToIP and ipToString assume the system to be
> little endian, so the value is computed in reversed byte order in advance,
> that's why it doesn't need convert `options.gateway` by htonl() before
> calling `libnetutils.ifc_set_default_route`.

I'm not sure that makes sense. libnetutils.ifc_set_default_route() seems to take little endian (host byte order), so htonl() would be the wrong function to use, no? I know htonl() and ntohl() are symmetric, just trying to be correct here.

> Either way can generate correct result.  IMHO, the changed version of
> stringToIP and ipToString without converting it to little endian in advance
> may be easier to understand.

I don't know. If we explicitly mention the endianness of stringToIP and ipToString in a comment, I think it would be fine.

Comment 17

[Shian-Yow Wu [:swu]]

(In reply to Philipp von Weitershausen [:philikon] from comment #16)
> I'm not sure that makes sense. libnetutils.ifc_set_default_route() seems to
> take little endian (host byte order), so htonl() would be the wrong function
> to use, no? I know htonl() and ntohl() are symmetric, just trying to be
> correct here.

My understanding is libnetutils.ifc_set_default_route() expects network byte order.  So, what we need is htonl(), not ntohl().  The original version of stringToIP generates the ip value in reversed order,  on a little endian system it equals to a network byte order output.

Take the library function inet_aton() [1] for example (this function converts IP string to binary format in network byte order).  It first generates IP address in a normal way (like the changed version stringToIP) and does a htonl() before return.

[1] http://source-android.frandroid.com/bionic/libc/inet/inet_aton.c

> > Either way can generate correct result.  IMHO, the changed version of
> > stringToIP and ipToString without converting it to little endian in advance
> > may be easier to understand.
> 
> I don't know. If we explicitly mention the endianness of stringToIP and
> ipToString in a comment, I think it would be fine.

OK, I will use keep original version of stringToIP and ipToString.

Comment 18

[Shian-Yow Wu [:swu]]

Attachment: Part 2: RIL/Wifi implementation (WIP v3)

Comment 19

[Shian-Yow Wu [:swu]]

Attachment: Part 2: RIL/Wifi implementation (WIP v3)

Addressed previous comments.

Comment 20

[Shian-Yow Wu [:swu]]

Attachment: Part 1: IDL

Rebase with hg.

Comment 21

[Shian-Yow Wu [:swu]]

Attachment: Part 2: RIL/Wifi implementation (WIP v3)

Rebase with hg.

Comment 22

[Philipp von Weitershausen [:philikon]]

Comment on attachment 651259 [details] [diff] [review]
Part 2: RIL/Wifi implementation (WIP v3)

Not checked in yet.

Comment 23

[Philipp von Weitershausen [:philikon]]

Comment on attachment 651258 [details] [diff] [review]
Part 1: IDL

Not checked in yet.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Pushing without Try results since this is b2g-only.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fafcc046e839
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce89d8c06783

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fafcc046e839
https://hg.mozilla.org/mozilla-central/rev/ce89d8c06783

Comment 26

[Philipp von Weitershausen [:philikon]]

(In reply to Ryan VanderMeulen from comment #24)
> Pushing without Try results since this is b2g-only.

Thanks, Ryan!