Support start/stop DHCP server in Network Manager

RESOLVED FIXED in mozilla27

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: chucklee, Assigned: hchang)

Tracking

unspecified
mozilla27
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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

Comment 1

4 years ago
Created attachment 790662 [details] [diff] [review]
NetworkManager_backend_support_for_WifiDirect.patch
(Assignee)

Comment 2

4 years ago
One of the patches for 811635 solved this bug
(Assignee)

Updated

4 years ago
Assignee: nobody → hchang
(Assignee)

Comment 3

4 years ago
Created attachment 802775 [details] [diff] [review]
Bug-900847.patch
Attachment #790662 - Attachment is obsolete: true
(Assignee)

Comment 4

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

Updated

4 years ago
Attachment #802775 - Flags: review?(chulee)
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.
Attachment #802775 - Flags: review?(chulee) → review?(vchang)
(Assignee)

Comment 6

4 years ago
(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 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 ?
Attachment #802775 - Flags: review?(vchang) → review-
(Assignee)

Comment 8

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

Comment 9

4 years ago
Created attachment 810995 [details] [diff] [review]
Bug 900847 - Add and implement nsINetworkManager.idl::setDhcpServer v2
(Assignee)

Updated

4 years ago
Attachment #810995 - Attachment description: Bug 900847 - Add and implement nsINetworkManager.idl::setDhcpServer → Bug 900847 - Add and implement nsINetworkManager.idl::setDhcpServer v2
Attachment #810995 - Flags: review?(vchang)
(Assignee)

Updated

4 years ago
Attachment #802775 - Attachment is obsolete: true
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);
Attachment #810995 - Flags: review?(vchang)
(Assignee)

Comment 11

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

Comment 12

4 years ago
Try server result: https://tbpl.mozilla.org/?tree=Try&rev=66c4a5d54f80
(Assignee)

Comment 13

4 years ago
Created attachment 812432 [details] [diff] [review]
Bug900847 v3.patch

Removed startUsbIp/endUsbIp and added trailing periods.
(Assignee)

Updated

4 years ago
Attachment #812432 - Flags: review?(vchang)
Comment on attachment 812432 [details] [diff] [review]
Bug900847 v3.patch

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

Looks good. Thank you.
Attachment #812432 - Flags: review?(vchang) → review+
(Assignee)

Comment 15

4 years ago
Thanks Vincent :)
(Assignee)

Updated

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

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/6512d20b7d6f
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6512d20b7d6f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.