900847 - Support start/stop DHCP server in Network Manager

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

Starting DHCP Server is required in Wifi Direct, but starting DHCP Server is currently only allowed in Tethering mode.
Wifi Direct uses different mechanism as Tethering, so we need another way to control only DHCP server.

Comment 1

[Henry Chang [:hchang]]

Attachment: NetworkManager_backend_support_for_WifiDirect.patch

Comment 2

[Henry Chang [:hchang]]

One of the patches for 811635 solved this bug

Comment 3

[Henry Chang [:hchang]]

Attachment: Bug-900847.patch

Comment 4

[Henry Chang [:hchang]]

https://tbpl.mozilla.org/?tree=Try&rev=490327c356e3(In reply to Henry Chang [:henry] from comment #3)
> Created attachment 802775 [details] [diff] [review]
> Bug-900847.patch
https://tbpl.mozilla.org/?tree=Try&rev=490327c356e3

Comment 5

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

Comment on attachment 802775 [details] [diff] [review]
Bug-900847.patch

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

Mostly are fine to me, just some nit and little issues about setDhcpServer() in NetworkManager.js.
Please double check those issues.

Also I am not a reviewer, forward to vchang.

::: dom/system/gonk/NetworkManager.js
@@ +907,5 @@
> +      isAsync: true,
> +      enabled: enabled,
> +      startIp: (enabled ? range.startIp : null),
> +      endIp:   (enabled ? range.endIp   : null)
> +    };

What type |range| is do we expect when setDhcpServer() is called while |enabled| === false?
If |range| should always be an object, but can be empty on |enabled| === false, then we don't have the |?:| operation here, just use |range.startIp| and |range.endIp|.

@@ +909,5 @@
> +      startIp: (enabled ? range.startIp : null),
> +      endIp:   (enabled ? range.endIp   : null)
> +    };
> +
> +    this.controlMessage(config, function setDhcpServerResult(success) {

The callback is called with object as argument in handleWorkerMessage()[1]
The callback paramater is better named |response| than |success|, so the callback function declaration might become |setDhcpServerResult(response)|.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/NetworkManager.js#479

@@ +910,5 @@
> +      endIp:   (enabled ? range.endIp   : null)
> +    };
> +
> +    this.controlMessage(config, function setDhcpServerResult(success) {
> +      if (!success) {

if (!response.success)

This fits the callback parameter usage, or this judgement might always be false.

::: dom/system/gonk/nsINetworkManager.idl
@@ +220,5 @@
>    /**
> +   * Enable or disable DHCP server
> +   *
> +   * @param enabled
> +   *   Boolean that indicates enabling or disabling DHCP server

nit: lack of tailing period, '.'.
Parameter description should be aligned to the parameter name.
You can refer to other comments in same file.

@@ +224,5 @@
> +   *   Boolean that indicates enabling or disabling DHCP server
> +   *
> +   * @param range An object to specify the IP range to lease when |enabled| === |true|
> +   *   .startIp
> +   *   .endIp

Same as above, parameter description should be in new line and aligned to parameter name.

Comment 6

[Henry Chang [:hchang]]

(In reply to Chuck Lee [:chucklee] from comment #5)
> Comment on attachment 802775 [details] [diff] [review]
> Bug-900847.patch
> 
> Review of attachment 802775 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly are fine to me, just some nit and little issues about setDhcpServer()
> in NetworkManager.js.
> Please double check those issues.
> 
> Also I am not a reviewer, forward to vchang.
> 

Thanks :)

> ::: dom/system/gonk/NetworkManager.js
> @@ +907,5 @@
> > +      isAsync: true,
> > +      enabled: enabled,
> > +      startIp: (enabled ? range.startIp : null),
> > +      endIp:   (enabled ? range.endIp   : null)
> > +    };
> 
> What type |range| is do we expect when setDhcpServer() is called while
> |enabled| === false?
> If |range| should always be an object, but can be empty on |enabled| ===
> false, then we don't have the |?:| operation here, just use |range.startIp|
> and |range.endIp|.
> 

|range| is expected to be null when |enabled| is false.
I will add this to the IDL description.

> @@ +909,5 @@
> > +      startIp: (enabled ? range.startIp : null),
> > +      endIp:   (enabled ? range.endIp   : null)
> > +    };
> > +
> > +    this.controlMessage(config, function setDhcpServerResult(success) {
> 
> The callback is called with object as argument in handleWorkerMessage()[1]
> The callback paramater is better named |response| than |success|, so the
> callback function declaration might become |setDhcpServerResult(response)|.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/NetworkManager.
> js#479
> 
> @@ +910,5 @@
> > +      endIp:   (enabled ? range.endIp   : null)
> > +    };
> > +
> > +    this.controlMessage(config, function setDhcpServerResult(success) {
> > +      if (!success) {
> 
> if (!response.success)
> 
> This fits the callback parameter usage, or this judgement might always be
> false.
> 

Oops sorry for this mis-use :p

> ::: dom/system/gonk/nsINetworkManager.idl
> @@ +220,5 @@
> >    /**
> > +   * Enable or disable DHCP server
> > +   *
> > +   * @param enabled
> > +   *   Boolean that indicates enabling or disabling DHCP server
> 
> nit: lack of tailing period, '.'.
> Parameter description should be aligned to the parameter name.
> You can refer to other comments in same file.
> 
> @@ +224,5 @@
> > +   *   Boolean that indicates enabling or disabling DHCP server
> > +   *
> > +   * @param range An object to specify the IP range to lease when |enabled| === |true|
> > +   *   .startIp
> > +   *   .endIp
> 
> Same as above, parameter description should be in new line and aligned to
> parameter name.

You will see them is the next patch!

Thanks!

Comment 7

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 802775 [details] [diff] [review]
Bug-900847.patch

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

::: dom/system/gonk/net_worker.js
@@ +225,5 @@
> +    command = "tether stop";
> +  }
> +  doCommand(command, function(error, result) {
> +    postMessage({ id: config.id, success: !error });
> +  });

We already have tether start/stop functions in this file. It is used for wifi/usb hotspot. What will happen if you stop dhcp server while wifi or usb hotspot is running ?

Comment 8

[Henry Chang [:hchang]]

(In reply to Vincent Chang[:vchang] from comment #7)
> Comment on attachment 802775 [details] [diff] [review]
> Bug-900847.patch
> 
> Review of attachment 802775 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/net_worker.js
> @@ +225,5 @@
> > +    command = "tether stop";
> > +  }
> > +  doCommand(command, function(error, result) {
> > +    postMessage({ id: config.id, success: !error });
> > +  });
> 
> We already have tether start/stop functions in this file. It is used for
> wifi/usb hotspot. What will happen if you stop dhcp server while wifi or usb
> hotspot is running ?

Since netd has no specific command to start/stop DHCP server, we
have to use the relevant command "tether start/stop", which happens
to starting/stopping DHCP server only. I intentionally don't use
|startTethering| in net_worker.js to emphasize this point. Maybe
I should add some comment here to explain it.

Thanks :)

Comment 9

[Henry Chang [:hchang]]

Attachment: Bug 900847 - Add and implement nsINetworkManager.idl::setDhcpServer v2

Comment 10

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 810995 [details] [diff] [review]
Bug 900847 - Add and implement nsINetworkManager.idl::setDhcpServer v2

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

::: dom/system/gonk/NetworkManager.js
@@ +911,5 @@
>        callback.wifiTetheringEnabledChange(msg);
>      }
>    },
>  
> +  // Enable/Disable DHCP server

Period at end of sentence.

::: dom/system/gonk/net_worker.js
@@ +245,5 @@
> +
> +    chain(params, startDhcpServerChain, onError);
> +  } else {
> +    chain({}, stopDhcpServerChain, onError);
> +  }

Early return

if (!config.enabled) {
  chain({}, stopDhcpServerChain, onError);
  return;
}

let params = { wifiStartIp: config.startIp,
               wifiEndIp: config.endIp,
               usbStartIp: "",
               usbEndIp: "",
               ip: config.serverIp,
               prefix: config.maskLength,
               ifname: config.ifname,
               link: "up" };
	 
chain(params, startDhcpServerChain, onError);

Comment 11

[Henry Chang [:hchang]]

(In reply to Vincent Chang[:vchang] from comment #10)
> Comment on attachment 810995 [details] [diff] [review]
> Bug 900847 - Add and implement nsINetworkManager.idl::setDhcpServer v2
> 
> Review of attachment 810995 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Early return
> 
> if (!config.enabled) {
>   chain({}, stopDhcpServerChain, onError);
>   return;
> }
> 
> let params = { wifiStartIp: config.startIp,
>                wifiEndIp: config.endIp,
>                usbStartIp: "",
>                usbEndIp: "",
>                ip: config.serverIp,
>                prefix: config.maskLength,
>                ifname: config.ifname,
>                link: "up" };
> 	 
> chain(params, startDhcpServerChain, onError);

Is this case really suitable for early return?

Early return do increase the readability when the sanity check fails 
or meet any exception. But in this case, neither |!config.enabled| has
any thing wrong nor each statement is long. 

Btw, I am gonna remove useStartIp/endEndIp since it's totally fine
to pass an object without them.

Thanks :)

Comment 12

[Henry Chang [:hchang]]

Try server result: https://tbpl.mozilla.org/?tree=Try&rev=66c4a5d54f80

Comment 13

[Henry Chang [:hchang]]

Attachment: Bug900847 v3.patch

Removed startUsbIp/endUsbIp and added trailing periods.

Comment 14

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 812432 [details] [diff] [review]
Bug900847 v3.patch

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

Looks good. Thank you.

Comment 15

[Henry Chang [:hchang]]

Thanks Vincent :)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6512d20b7d6f

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6512d20b7d6f