[B2G] Support resolveHostForInterface in nsINetworkService

RESOLVED INVALID

Status

Firefox OS
General
RESOLVED INVALID
4 years ago
3 years ago

People

(Reporter: hchang, Assigned: hchang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: p=3)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 992772
(Assignee)

Updated

4 years ago
Assignee: nobody → hchang
(Assignee)

Updated

4 years ago
Whiteboard: p=3
(Assignee)

Updated

4 years ago
Attachment #8534185 - Attachment is obsolete: true
(Assignee)

Comment 3

4 years ago
Created attachment 8534223 [details] [diff] [review]
Bug1109452.patch
Attachment #8534221 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
Created attachment 8534761 [details] [diff] [review]
Bug1109452.patch
Attachment #8534223 - Attachment is obsolete: true
(Assignee)

Comment 5

4 years ago
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!
Attachment #8534223 - Flags: review?(echen)
(Assignee)

Updated

4 years ago
Blocks: 1053650
(Assignee)

Updated

4 years ago
Attachment #8534223 - Flags: review?(echen)
(Assignee)

Updated

4 years ago
Attachment #8534761 - Flags: review?(echen)
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

4 years ago
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.
Attachment #8534761 - Flags: review?(echen)
(Assignee)

Comment 8

4 years ago
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!
Flags: needinfo?(echen)
(Assignee)

Comment 9

4 years ago
(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.
Flags: needinfo?(echen) → needinfo?(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.
Flags: needinfo?(mcmanus)

Comment 11

4 years ago
(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!
(Assignee)

Comment 12

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

Comment 13

3 years ago
According to comment 12, change the state.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.