Closed Bug 900847 Opened 11 years ago Closed 11 years ago

Support start/stop DHCP server in Network Manager

Categories

(Core :: DOM: Device Interfaces, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: chucklee, Assigned: hchang)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
One of the patches for 811635 solved this bug
Assignee: nobody → hchang
Attached patch Bug-900847.patch (obsolete) — Splinter Review
Attachment #790662 - Attachment is obsolete: true
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)
(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-
(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 :)
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)
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)
(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 :)
Removed startUsbIp/endUsbIp and added trailing periods.
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+
Thanks Vincent :)
Attachment #810995 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6512d20b7d6f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: