1109452 - [B2G] Support resolveHostForInterface in nsINetworkService

Status: RESOLVED INVALID

(Firefox OS Graveyard :: General, defect)

Description

[Henry Chang [:hchang]]

We need this function to co-work with setDNS as the foundation of Bug 992772.
One should call setDNS to bind the interface to a DNS name server and call
resolveHostForInterface for per-interface DNS lookup.

Comment 1

[Henry Chang [:hchang]]

Attachment: Bug1109452.patch

Comment 2

[Henry Chang [:hchang]]

Attachment: Bug1109452.patch

Comment 3

[Henry Chang [:hchang]]

Attachment: Bug1109452.patch

Comment 4

[Henry Chang [:hchang]]

Attachment: Bug1109452.patch

Comment 5

[Henry Chang [:hchang]]

Comment on attachment 8534223 [details] [diff] [review]
Bug1109452.patch

Hi Edgar,

Not sure if you are available to review this patch. It is the most
basic building block to support per-interface DNS query. It's essentially
a wrapper call to |android_getaddrinfoforiface| and should work with
|setDNS|.

Thanks!

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 8534761 [details] [diff] [review]
Bug1109452.patch

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

I'd like the idl changes here to be in general platform idls, not gonk specific ones

thanks

Comment 7

[Edgar Chen [:edgar]]

Comment on attachment 8534761 [details] [diff] [review]
Bug1109452.patch

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

Please see my comments below, thank you.

::: dom/system/gonk/NetworkService.js
@@ +578,5 @@
> +      debug('resolveHostForInterface: ' + hostname + ' @ ' + interfaceName);
> +    }
> +
> +    this.controlMessage(params, function(data) {
> +      callback.resolveHostForInterfaceResult(data.ret, data.ips, data.ips.length);

I have one question. If |data.ret| is `false`, is |data.ips| still an array type?
Or should we give a default value for `ips` in webidl?

::: dom/system/gonk/nsINetworkService.idl
@@ +162,5 @@
> +   * Callback function used to report the result of updating upstream.
> +   *
> +   * @param success
> +   *        Boolean to indicate the operation is successful or not.
> +   * @param externalIfname

I guess you put a wrong comments here.

::: dom/webidl/NetworkOptions.webidl
@@ +53,5 @@
>    long ipaddr;                        // for "ifc_configure".
>    long mask;                          // for "ifc_configure".
>    long gateway_long;                  // for "ifc_configure".
>    long dns1_long;                     // for "ifc_configure".
> +  long dns2_long;

Please help add `// for "ifc_configure".` comment back.

Comment 8

[Henry Chang [:hchang]]

Thanks for the review!

(In reply to Edgar Chen [:edgar][:echen] from comment #7)
> Comment on attachment 8534761 [details] [diff] [review]
> Bug1109452.patch
> 
> Review of attachment 8534761 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my comments below, thank you.
> 
> ::: dom/system/gonk/NetworkService.js
> @@ +578,5 @@
> > +      debug('resolveHostForInterface: ' + hostname + ' @ ' + interfaceName);
> > +    }
> > +
> > +    this.controlMessage(params, function(data) {
> > +      callback.resolveHostForInterfaceResult(data.ret, data.ips, data.ips.length);
> 
> I have one question. If |data.ret| is `false`, is |data.ips| still an array
> type?
> Or should we give a default value for `ips` in webidl?

According to the implementation (NetworkUtils::resolveHostForInterface),
if |data.ret| is false, |data.ips| will never be constructed. 
I have no idea what type would it be in JS binding. Since |data.ret|
suggests the failure of the operation, the caller shouldn't do any 
assumption to other outputs. That being said, it is a little implicit.
I can 
1) leave it undefined but add the description to the comment or
2) give a default value to ips in webidl or
3) construct ips in the implementation no matter what

What do you think?

Thanks!

Comment 9

[Henry Chang [:hchang]]

(In reply to Patrick McManus [:mcmanus] from comment #6)
> Comment on attachment 8534761 [details] [diff] [review]
> Bug1109452.patch
> 
> Review of attachment 8534761 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like the idl changes here to be in general platform idls, not gonk
> specific ones
> 
> thanks

Hi Patrick,

Since nsINetworkSerivce.idl is currently a gonk specific idl,
what we can do is

1) Make it a general idl and it would only have an implementation on gonk since
   all provided services are gonk specific.

2) Don't use nsINetworkSerivce.idl but extend nsIDNSService.idl and its implementation.
   In this case, we will still need a ifdef for gonk in the implementation.

There must be a place where the gonk specific code existing. It's in SocketTransport.
For 2) it would be in nsHostResolver.cpp. We can even move it to PR layer.

Comment 10

[Patrick McManus [:mcmanus]]

please see the other bug - I think Dragana thinks it can better be done inside the existing dns service implementation.. there would be an ifdef, though I think it can be broadened t android instead of just gonk.

Comment 11

[Edgar Chen [:edgar]]

(In reply to Henry Chang [:henry] from comment #8)
> Thanks for the review!
> 
> (In reply to Edgar Chen [:edgar][:echen] from comment #7)
> > Comment on attachment 8534761 [details] [diff] [review]
> > Bug1109452.patch
> > 
> > Review of attachment 8534761 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Please see my comments below, thank you.
> > 
> > ::: dom/system/gonk/NetworkService.js
> > @@ +578,5 @@
> > > +      debug('resolveHostForInterface: ' + hostname + ' @ ' + interfaceName);
> > > +    }
> > > +
> > > +    this.controlMessage(params, function(data) {
> > > +      callback.resolveHostForInterfaceResult(data.ret, data.ips, data.ips.length);
> > 
> > I have one question. If |data.ret| is `false`, is |data.ips| still an array
> > type?
> > Or should we give a default value for `ips` in webidl?
> 
> According to the implementation (NetworkUtils::resolveHostForInterface),
> if |data.ret| is false, |data.ips| will never be constructed. 
> I have no idea what type would it be in JS binding. Since |data.ret|
If the attribute didn't be assigned a default value in webIDL and didn't be constructed, it will be `undefined` after ToJSValue.

> suggests the failure of the operation, the caller shouldn't do any 
> assumption to other outputs. That being said, it is a little implicit.
> I can 
> 1) leave it undefined but add the description to the comment or
But the `ip` in idl is defined as an array type, I don't know what will happen if you pass a |undefined| value into it.

> 2) give a default value to ips in webidl or
I prefer this.

> 3) construct ips in the implementation no matter what
2) and 3) are the same thing I think.
If ips will always be constructed, we could just give a default value in webidl and let webidl do construct for us.

> 
> What do you think?
> 
> Thanks!

Comment 12

[Henry Chang [:hchang]]

This bug is going to obsolete since bug 1108957 would take over the role.

Comment 13

[Ken Chang[:kenkai]]

According to comment 12, change the state.