James Ho Reporter
	Description • 13 years ago

Adding usb tethering support to b2g backend. Also requires a setting api for at least on/off.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 1 • 12 years ago

We can use shell commands to support usb Tethering in B2G-ICS, just following below steps
Please also add dnsmasq and iptables support in manifest.xml. 

# set mac address of rndis interface before enable it 
echo "00:50:ba:48:88:99" > /sys/class/android_usb/f_rndis/ethaddr 

# enable rndis interface by modify usb config property, android will help to set necessary   commands according to the configuration in init.smdk4210.usb.rc.  
setprop persist.sys.usb.config rndis,adb 

# enable nat, you can modify -o option to rmn0 or wlan0 if you want to set wan to 3G or wifi
iptables -t nat -A POSTROUTING -o wlan0 -j MASQUERADE

# accept ip forwarding 
iptables -P FORWARD ACCEPT

echo 1 > /proc/sys/net/ipv4/ip_forward

# set lan interface's ip address 
ifconfig rndis0 192.168.0.1

# enable dnsmasq, please modify the dhcp-range according to your lan interface's ip address
/system/bin/dnsmasq --no-daemon --no-resolv --no-poll --dhcp-range=192.168.0.2,192.168.0.10 &

We need to implement a component to deal with these commands in gecko.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 2 • 12 years ago

Netd is android's native daemon which can be used to configure network sub system of linux kernel. We enhance network manager interface by adding functions to control network interface, enables/disables IP forward and NAT... These functions then send commands to netd via unix domain socket in net_worker.
We also need to generalize the hacks we use for RIL. So that net_worker can use new interface to communicate with netd.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 5 • 12 years ago

I tried to copy the codes in SystemWorkerManager.cpp and defined the postNetdMessage JS function. This function is used to connect Network Manager worker to Netd. However, it shows undefined function when I try to call postNetdMessage in Network Manager worker. The ConnectWorkerToNetd::RunTask task dispatched by WorkerCrossThreadDispatcher can be called without problem. And postNetdMessage is called after this task.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 6 • 12 years ago

You're reusing the RIL worker? Wouldn't it make sense for this stuff to use its own dedicated worker?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 7 • 12 years ago

I *hope* the problem is that you're calling do_CreateInstance on the network manager, not do_GetService. Obviously a bug, but there may be other issues here. We need to have a well-defined startup order here so that the network manager doesn't try to call netd functions before your netd function has been added to net_worker.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 8 • 12 years ago

Hi Ben, 

I modified do_CreateInstance to do_GetService on the SystemWorkerManager.cpp. I can use Defined JS functions to send/receive messages to/from android's netd process now. So I can continue my work about tethering. Thank you. 
I will study startup order a little bit more. 


Regards
Vincent

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 9 • 12 years ago

added set usb function

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 11 • 12 years ago

I attached the wrong file, please ignore previous post. 
doNetdCommand JS function is used to create unix domain socket, sending command and waitting for response from netd. The network manager worker uses this function to communicate with netd.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 12 • 12 years ago

Use doNetdCommand JS fucntion to do IPC with netd in network manager worker.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 13 • 12 years ago

added wifi tethering support

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 14 • 12 years ago

added wifi tethering implementation

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 15 • 12 years ago

Updated IPC interface implementation. 
Don't close unix domain socket for each IPC commands

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 16 • 12 years ago

Tethering settting path definition 
 
[key, value] = [tethering.usb.enabled, true/false]
[key, value] = [tethering.wifi.enabled, true/false]
[key, value] = [tethering.wifi.ssid, characters]
[key, value] = [tethering.wifi.security.type, open/wpa-psk/wpa2-psk] 
[key, value] = [tethering.wifi.security.password, none/8~63 characters/8~63 characters]  

The application developer can modify the settings for both usb and wifi to enable/disable tethering. The wifi security settings are also defined.

	Philipp von Weitershausen [:philikon]
	Comment 17 • 12 years ago

Comment on attachment 632151 [details] [diff] [review]
Updated interface definition V2

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

I'm confused why any of these methods need to be exposed via XPCOM. As far as I can tell, we only call them from inside NetworkManager, based on settings changes. I'm also not sure we need the granularity of those methods on the main thread. I'll elaborate in the review for the other patch.

	Philipp von Weitershausen [:philikon]
	Comment 18 • 12 years ago

Comment on attachment 632152 [details] [diff] [review]
Updated interface implementation V2

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

::: dom/system/gonk/NetworkManager.js
@@ +74,5 @@
>      event.preventDefault();
> +  }.bind(this);
> +
> +  // Callbacks to invoke when a reply arrives from the controlWorker.
> +  this.controlCallbacks = Object.create(null);                                                                              

Superfluous newline.

@@ +82,5 @@
> +  gSettingsService.getLock().get("tethering.usb.enabled", this);
> +  gSettingsService.getLock().get("tethering.wifi.enabled", this);
> +  gSettingsService.getLock().get("tethering.wifi.ssid", this);
> +  gSettingsService.getLock().get("tethering.wifi.security.type", this);
> +  gSettingsService.getLock().get("tethering.wifi.security.password", this);

Shouldn't you be able to get the lock just once?

@@ +93,5 @@
>    classID:   NETWORKMANAGER_CID,
>    classInfo: XPCOMUtils.generateCI({classID: NETWORKMANAGER_CID,
>                                      contractID: NETWORKMANAGER_CONTRACTID,
>                                      classDescription: "Network Manager",
> +                                    interfaces: [Ci.nsIWorkerHolder,Ci.nsINetworkManager]}),

nit: space after comma

But why do we need to add nsIWorkerHolder to the classInfo?

@@ +100,2 @@
>                                           Ci.nsISupportsWeakReference,
> +                                         Ci.nsIObserver, Ci.nsISettingsServiceCallback]),

I would prefer to keep it at one interface per line.

@@ +112,5 @@
> +  _tetheringWifiSSID   : DEFAULT_HOTSPOT_SSID,
> +  _tetheringWifiSecType: DEFAULT_HOTSPOT_SECURITY_TYPE,
> +  _tetheringWifiSecPWD : DEFAULT_HOTSPOT_SECURITY_PWD,
> +
> +  handle: function handle(aName, aResult) {

Would be nice to add a 

  // nsISettingsServiceCallback

comment above this method to indicate where it's coming from. For extra credit, add a JavaDoc comment a la

  /**
   * Handle settings changes.
   */

@@ +114,5 @@
> +  _tetheringWifiSecPWD : DEFAULT_HOTSPOT_SECURITY_PWD,
> +
> +  handle: function handle(aName, aResult) {
> +
> +    switch (aName) {

Nit: Get rid of extraneous whitespace line.

@@ +157,5 @@
> +   */
> +  handleMozSettingsChanged: function handleMozSettingsChanged(setting) {
> +    let SSID   = Services.prefs.getCharPref("tethering.wifi.ssid");
> +    let SecType= Services.prefs.getCharPref("tethering.wifi.security.type");
> +    let SecPWD = Services.prefs.getCharPref("tethering.wifi.security.password");

If there are no default preferences for these (e.g. in b2g/app/b2g.js), these calls can throw.

Also, coding style nit: spaces around all operators, variable names should start with lower case.

@@ +175,5 @@
> +          this.setIpForwardingEnabled("disable");
> +          this.stopTethering();
> +          this.untetherInterface(DEFAULT_USB_INTERFACE_NAME);
> +          this.setInterfaceDown(DEFAULT_USB_INTERFACE_NAME);
> +          this.setUSBFunction("adb");

I count 6 calls here that all end up going to `net_worker.js` in the end. There's a couple of issues I have with that:

* It's bloaty. We have to write a lot of code on both threads, but we never actually need any of the individual methods.
* It's inefficient to have a lot of back and forth between the threads.
* Right now you don't catch errors from a previous step. You schedule all 6 events on the worker thread, which means they'll all run, no matter what.

I think we should just implement a few helpers in `net_worker.js`, e.g.:

* enableUSBTethering(options)
* disableUSBTethering()
* enableWifiTethering(options)
* disableWifiTethering

These functions will do all the work, they can do error checking, etc. and they can totally be synchronous, too. As a result, `NetworkManager` will be much smaller, too.

@@ +304,5 @@
>      this.setAndConfigureActive();
>    },
> +  // Helpers
> +  controlMessage: function controlMessage(options, callback) {
> +    var id = this.idgen++;

nit: s/var/let/

(I'm not going to call out each instance of that here, but please fix this everywhere.)

@@ +307,5 @@
> +  controlMessage: function controlMessage(options, callback) {
> +    var id = this.idgen++;
> +    options.id = id;
> +    if (callback)
> +      this.controlCallbacks[id] = callback;

coding style nit: always use braces.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 19 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #17)
> Comment on attachment 632151 [details] [diff] [review]
> Updated interface definition V2
> 
> Review of attachment 632151 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm confused why any of these methods need to be exposed via XPCOM. As far
> as I can tell, we only call them from inside NetworkManager, based on
> settings changes. I'm also not sure we need the granularity of those methods
> on the main thread. I'll elaborate in the review for the other patch.

We can implement tethering outside networkmanager if we expose these methods via XPCOM.
But it is straight forward to implement tethering in networkmanager directly. Could I have your comments about this ?

	Philipp von Weitershausen [:philikon]
	Comment 20 • 12 years ago

(In reply to Vincent Chang from comment #19)
> > I'm confused why any of these methods need to be exposed via XPCOM. As far
> > as I can tell, we only call them from inside NetworkManager, based on
> > settings changes. I'm also not sure we need the granularity of those methods
> > on the main thread. I'll elaborate in the review for the other patch.
> 
> We can implement tethering outside networkmanager if we expose these methods
> via XPCOM.

Sure, but why? Also, with the current interface there's no way to do proper error handling (e.g. to make the switch to tethering transactional/atomic).

> But it is straight forward to implement tethering in networkmanager
> directly. Could I have your comments about this ?

I thought comment 18 was pretty clear: Most of the logic can be done in net_worker.js. Since it's on a separate thread, all calls are synchronous, error handling is easy to do, and NetworkManager stays pretty simple.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 21 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #18)
> Comment on attachment 632152 [details] [diff] [review]
> Updated interface implementation V2
> 
> Review of attachment 632152 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +74,5 @@
> >      event.preventDefault();
> > +  }.bind(this);
> > +
> > +  // Callbacks to invoke when a reply arrives from the controlWorker.
> > +  this.controlCallbacks = Object.create(null);                                                                              
> 
> Superfluous newline.
> 


> @@ +82,5 @@
> > +  gSettingsService.getLock().get("tethering.usb.enabled", this);
> > +  gSettingsService.getLock().get("tethering.wifi.enabled", this);
> > +  gSettingsService.getLock().get("tethering.wifi.ssid", this);
> > +  gSettingsService.getLock().get("tethering.wifi.security.type", this);
> > +  gSettingsService.getLock().get("tethering.wifi.security.password", this);
> 
> Shouldn't you be able to get the lock just once?
> 
  ok, I will modify it to 
  let settingsLock = gSettingsService.getLock();
  settingsLock.get("tethering.usb.enabled", this); 
  settingsLock.get("tethering.wifi.ssid", this);
> @@ +93,5 @@
> >    classID:   NETWORKMANAGER_CID,
> >    classInfo: XPCOMUtils.generateCI({classID: NETWORKMANAGER_CID,
> >                                      contractID: NETWORKMANAGER_CONTRACTID,
> >                                      classDescription: "Network Manager",
> > +                                    interfaces: [Ci.nsIWorkerHolder,Ci.nsINetworkManager]}),
> 
> nit: space after comma
> 
> But why do we need to add nsIWorkerHolder to the classInfo?
> 
  It is not necessary. I will remove it. 
> @@ +100,2 @@
> >                                           Ci.nsISupportsWeakReference,
> > +                                         Ci.nsIObserver, Ci.nsISettingsServiceCallback]),
> 
> I would prefer to keep it at one interface per line.
> 
  ok, I got it.

> @@ +112,5 @@
> > +  _tetheringWifiSSID   : DEFAULT_HOTSPOT_SSID,
> > +  _tetheringWifiSecType: DEFAULT_HOTSPOT_SECURITY_TYPE,
> > +  _tetheringWifiSecPWD : DEFAULT_HOTSPOT_SECURITY_PWD,
> > +
> > +  handle: function handle(aName, aResult) {
> 
> Would be nice to add a 
> 
>   // nsISettingsServiceCallback
> 
> comment above this method to indicate where it's coming from. For extra
> credit, add a JavaDoc comment a la
> 
  ok, I will add the comment.

>   /**
>    * Handle settings changes.
>    */
> 
> @@ +114,5 @@
> > +  _tetheringWifiSecPWD : DEFAULT_HOTSPOT_SECURITY_PWD,
> > +
> > +  handle: function handle(aName, aResult) {
> > +
> > +    switch (aName) {
> 
> Nit: Get rid of extraneous whitespace line.
> 
  ok, I got it. 
> @@ +157,5 @@
> > +   */
> > +  handleMozSettingsChanged: function handleMozSettingsChanged(setting) {
> > +    let SSID   = Services.prefs.getCharPref("tethering.wifi.ssid");
> > +    let SecType= Services.prefs.getCharPref("tethering.wifi.security.type");
> > +    let SecPWD = Services.prefs.getCharPref("tethering.wifi.security.password");
> 
> If there are no default preferences for these (e.g. in b2g/app/b2g.js),
> these calls can throw.

  I added function NetworkManagerSettingsToPrefs() in b2g/chrome/content/settings.js

> Also, coding style nit: spaces around all operators, variable names should
> start with lower case.
  ok, I will modify it. 
> 
> @@ +175,5 @@
> > +          this.setIpForwardingEnabled("disable");
> > +          this.stopTethering();
> > +          this.untetherInterface(DEFAULT_USB_INTERFACE_NAME);
> > +          this.setInterfaceDown(DEFAULT_USB_INTERFACE_NAME);
> > +          this.setUSBFunction("adb");
> 
> I count 6 calls here that all end up going to `net_worker.js` in the end.
> There's a couple of issues I have with that:
> 
> * It's bloaty. We have to write a lot of code on both threads, but we never
> actually need any of the individual methods.
> * It's inefficient to have a lot of back and forth between the threads.
> * Right now you don't catch errors from a previous step. You schedule all 6
> events on the worker thread, which means they'll all run, no matter what.
> 
> I think we should just implement a few helpers in `net_worker.js`, e.g.:
> 
> * enableUSBTethering(options)
> * disableUSBTethering()
> * enableWifiTethering(options)
> * disableWifiTethering
> 
> These functions will do all the work, they can do error checking, etc. and
> they can totally be synchronous, too. As a result, `NetworkManager` will be
> much smaller, too.

  ok, thank you for your suggestion. I will modify it.   

> 
> @@ +304,5 @@
> >      this.setAndConfigureActive();
> >    },
> > +  // Helpers
> > +  controlMessage: function controlMessage(options, callback) {
> > +    var id = this.idgen++;
> 
> nit: s/var/let/

  I have a question about this. I read the mdn and find the "let" is block scoping and "var" is global and functional scoping. I am wondering if there is any difference for GC if we use "let" to declare id in this case.   

> (I'm not going to call out each instance of that here, but please fix this
> everywhere.)
> 
> @@ +307,5 @@
> > +  controlMessage: function controlMessage(options, callback) {
> > +    var id = this.idgen++;
> > +    options.id = id;
> > +    if (callback)
> > +      this.controlCallbacks[id] = callback;
> 
> coding style nit: always use braces.
  ok, I will modify it.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 22 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #20)
> (In reply to Vincent Chang from comment #19)
> > > I'm confused why any of these methods need to be exposed via XPCOM. As far
> > > as I can tell, we only call them from inside NetworkManager, based on
> > > settings changes. I'm also not sure we need the granularity of those methods
> > > on the main thread. I'll elaborate in the review for the other patch.
> > 
> > We can implement tethering outside networkmanager if we expose these methods
> > via XPCOM.
> 
> Sure, but why? Also, with the current interface there's no way to do proper
> error handling (e.g. to make the switch to tethering transactional/atomic).
> 
> > But it is straight forward to implement tethering in networkmanager
> > directly. Could I have your comments about this ?
> 
> I thought comment 18 was pretty clear: Most of the logic can be done in
> net_worker.js. Since it's on a separate thread, all calls are synchronous,
> error handling is easy to do, and NetworkManager stays pretty simple.

I will move most of the logic to net_worker.js and submit a new patch soon.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 23 • 12 years ago

Comment on attachment 632153 [details] [diff] [review]
Updated IPC interface implementation

Sounds like I should wait for the new patch? Please re-request review if not.

	Philipp von Weitershausen [:philikon]
	Comment 24 • 12 years ago

(In reply to Vincent Chang from comment #21)
> > @@ +157,5 @@
> > > +   */
> > > +  handleMozSettingsChanged: function handleMozSettingsChanged(setting) {
> > > +    let SSID   = Services.prefs.getCharPref("tethering.wifi.ssid");
> > > +    let SecType= Services.prefs.getCharPref("tethering.wifi.security.type");
> > > +    let SecPWD = Services.prefs.getCharPref("tethering.wifi.security.password");
> > 
> > If there are no default preferences for these (e.g. in b2g/app/b2g.js),
> > these calls can throw.
> 
>   I added function NetworkManagerSettingsToPrefs() in
> b2g/chrome/content/settings.js

I saw that. It's still good form to

(a) create defaults prefs in b2g/app/b2g.js
(b) guard these calls against exceptions

Belts and suspenders FTW :)

> > @@ +304,5 @@
> > >      this.setAndConfigureActive();
> > >    },
> > > +  // Helpers
> > > +  controlMessage: function controlMessage(options, callback) {
> > > +    var id = this.idgen++;
> > 
> > nit: s/var/let/
> 
>   I have a question about this. I read the mdn and find the "let" is block
> scoping and "var" is global and functional scoping. I am wondering if there
> is any difference for GC if we use "let" to declare id in this case.

At function scope, `var` and `let` are equivalent. Which means there's no reason to use `var` anywhere in chrome code. Using `let` everywhere means consistent variable declarations which helps, for instance, when you're looking for where a variable was declared.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 25 • 12 years ago

(In reply to ben turner [:bent] from comment #23)
> Comment on attachment 632153 [details] [diff] [review]
> Updated IPC interface implementation
> 
> Sounds like I should wait for the new patch? Please re-request review if not.

It would be very helpful if I could have your feedback about this patch.
I am going to modify IPC mechanism from synchronous to asynchronous one. Let IO thread to listen to netd socket using select. It seems that netd process may send link status change event actively to his client. If we don't handle it properly, the netd may crash sometimes.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 26 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #24)
> (In reply to Vincent Chang from comment #21)
> > > @@ +157,5 @@
> > > > +   */
> > > > +  handleMozSettingsChanged: function handleMozSettingsChanged(setting) {
> > > > +    let SSID   = Services.prefs.getCharPref("tethering.wifi.ssid");
> > > > +    let SecType= Services.prefs.getCharPref("tethering.wifi.security.type");
> > > > +    let SecPWD = Services.prefs.getCharPref("tethering.wifi.security.password");
> > > 
> > > If there are no default preferences for these (e.g. in b2g/app/b2g.js),
> > > these calls can throw.
> > 
> >   I added function NetworkManagerSettingsToPrefs() in
> > b2g/chrome/content/settings.js
> 
> I saw that. It's still good form to
> 
> (a) create defaults prefs in b2g/app/b2g.js
> (b) guard these calls against exceptions
> 
> Belts and suspenders FTW :)
  
  ok, I got it. So I should use try ... catch ... to check the exception and 
  get it a default value when exception happened, right ?  
 
> > > @@ +304,5 @@
> > > >      this.setAndConfigureActive();
> > > >    },
> > > > +  // Helpers
> > > > +  controlMessage: function controlMessage(options, callback) {
> > > > +    var id = this.idgen++;
> > > 
> > > nit: s/var/let/
> > 
> >   I have a question about this. I read the mdn and find the "let" is block
> > scoping and "var" is global and functional scoping. I am wondering if there
> > is any difference for GC if we use "let" to declare id in this case.
> 
> At function scope, `var` and `let` are equivalent. Which means there's no
> reason to use `var` anywhere in chrome code. Using `let` everywhere means
> consistent variable declarations which helps, for instance, when you're
> looking for where a variable was declared.

  ok, I got it. Thank you for your explanation.

	Dave Hylands [:dhylands]
	Comment 27 • 12 years ago

(In reply to Vincent Chang from comment #25)
> (In reply to ben turner [:bent] from comment #23)
> > Comment on attachment 632153 [details] [diff] [review]
> > Updated IPC interface implementation
> > 
> > Sounds like I should wait for the new patch? Please re-request review if not.
> 
> It would be very helpful if I could have your feedback about this patch.
> I am going to modify IPC mechanism from synchronous to asynchronous one. Let
> IO thread to listen to netd socket using select. It seems that netd process
> may send link status change event actively to his client. If we don't handle
> it properly, the netd may crash sometimes.

Hey Vincent,

You may want to look at gecko/dom/system/gonk/VolumeManager.cpp. In particular the WriteCommandData, OnFileCanReadWithoutBlocking and OnFileCanWriteWithoutBlocking functions. That's what I use for talking to vold over a socket interface. It's fully async.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 28 • 12 years ago

Comment on attachment 632153 [details] [diff] [review]
Updated IPC interface implementation

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

So, in general, all of this netd code should live in its own cpp file. Then you should have one header file that just exposes a single "NetdCommand" function. Then ConnectWorkerToNetd::RunTask would just use that in the call to JS_DefineFunction. That way we keep everything isolated nicely.

::: dom/system/gonk/SystemWorkerManager.cpp
@@ +199,5 @@
>                               argv, argv);
>  }
>  
> +
> +static int mSocket;

We usually prefix variables with 'm' to indicate a member of an object. We use 'g' for global objects, and 's' for static members. Since this is global please use a 'g'.

@@ +207,5 @@
> +  virtual bool RunTask(JSContext *aCx);
> +};
> +
> +static int 
> +nsWrite(int fd, const void *buffer, int len) {

The 'ns' prefix is usually only used on classes, and it's definitely not what you want here. You should remove it here and on the other functions below.

Once this is in its own file you could just call them "Write", "SendCommand", "DoCommand", etc.

@@ +211,5 @@
> +nsWrite(int fd, const void *buffer, int len) {
> +  int write_offset = 0;
> +  const unsigned char *to_write;
> +
> +  to_write = (const unsigned char *)buffer;

In general please use C++ casts (static_cast, const_cast) whenever possible.

@@ +269,5 @@
> +}
> +
> +
> +JSBool
> +nsNetdCommand(JSContext *cx, unsigned argc, jsval *vp)

Most of this seems to be copied from existing code, so we should factor out the common stuff to a helper function.

@@ +548,5 @@
>    return NS_OK;
>  }
>  
> +static void 
> +initRndisAddress()

Functions should begin with a capital letter.

@@ +550,5 @@
>  
> +static void 
> +initRndisAddress()
> +{
> +  int fd = open(ICS_SYS_USB_RNDIS_MAC, O_WRONLY);

Since this is doing IO you should first assert that this function is not being called on the main thread:

  NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!");

@@ +558,5 @@
> +    LOG(("Unable to open file %s.", ICS_SYS_USB_RNDIS_MAC));
> +    return;
> +  }
> +
> +  sprintf(mac, "%02x:%02x:%02x:%02x:%02x:%02x", 0x00, 0x50, 0xba, 0x48, 0x88, 0x88);

I'm unclear on where this mac address is coming from, but I'm assuming that we can't hardcode it. In any case, if you know all the values you don't need to call sprintf :)

@@ +560,5 @@
> +  }
> +
> +  sprintf(mac, "%02x:%02x:%02x:%02x:%02x:%02x", 0x00, 0x50, 0xba, 0x48, 0x88, 0x88);
> +
> +  if (write(fd, mac, strlen(mac) + 1) < 0) {

You probably want to check that it wrote the correct number of bytes rather than just checking if it returned -1.

@@ +566,5 @@
> +    close(fd);
> +    return;
> +  }
> +
> +  close(fd);

Having to write "close(fd)" twice can and should be avoided.

@@ +573,5 @@
> +nsresult
> +SystemWorkerManager::InitNetd(JSContext *cx)
> +{
> +  // todo init rndis interface's mac address. Should be placed to usbdevice manager someday later.  
> +  initRndisAddress();

This will block the main thread so we need to figure out a better way to do this. Can't we put this into the ConnectWorkerToNetd runnable?

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 29 • 12 years ago

(In reply to Dave Hylands [:dhylands] from comment #27)
> (In reply to Vincent Chang from comment #25)
> > (In reply to ben turner [:bent] from comment #23)
> > > Comment on attachment 632153 [details] [diff] [review]
> > > Updated IPC interface implementation
> > > 
> > > Sounds like I should wait for the new patch? Please re-request review if not.
> > 
> > It would be very helpful if I could have your feedback about this patch.
> > I am going to modify IPC mechanism from synchronous to asynchronous one. Let
> > IO thread to listen to netd socket using select. It seems that netd process
> > may send link status change event actively to his client. If we don't handle
> > it properly, the netd may crash sometimes.
> 
> Hey Vincent,
> 
> You may want to look at gecko/dom/system/gonk/VolumeManager.cpp. In
> particular the WriteCommandData, OnFileCanReadWithoutBlocking and
> OnFileCanWriteWithoutBlocking functions. That's what I use for talking to
> vold over a socket interface. It's fully async.

Thank you, I will check it.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 30 • 12 years ago

1.Using asynchronous I/O to communicate with netd. 
2.Move InitRndisAddress to I/O thread, the mac address comes from serialno property. 
3.Filtered out unsolicited broadcasts message

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 32 • 12 years ago

1. moved netd command to net_worker.js when enable/disable tethering
2. added state machine to handle asynchronous I/O between netd and net_worker
3. use gjslint and fixjsstyle utilites to fix coding style
4. Before doing the wifi/usb tethering, sending tethering request to wifi/usb manager to grant the access right. The wifi/usb manager can do some actions before switch to tethering state.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 33 • 12 years ago

For test purpose, I added gaia code in settings. It allows users to enable/disable usb/wifi tethering. You can checkout the code located in github.  
https://github.com/changyihsin/gaia/commit/7675a6b53908efdc74d228ed9bb366ef7b8e1d35

	Philipp von Weitershausen [:philikon]
	Comment 34 • 12 years ago

Comment on attachment 637383 [details] [diff] [review]
Updated tethering implementation V3 and removed interface definition

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

This patch contains a lot of unrelated changes that violate the coding style. In particular, changing double quotes to single quotes. Strings should be in double quotes everywhere (but if you find existing code that's wrong, please fight the temptation to clean it up.). Anyway, the unrelated changes create a lot of noise that make it very hard for me to see the actual changes you've created. Please remove them all.

::: b2g/chrome/content/settings.js
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +'use strict;';

The correct string would be

  "use strict";

with double quotes (Mozilla coding style) and the semicolon outside of the string.

::: dom/system/gonk/NetworkManager.js
@@ +42,5 @@
> +const DEFAULT_USB_DHCP_STOP_IP = '192.168.1.6';
> +
> +const DEFAULT_HOTSPOT_SSID = 'B2G_HOTSPOT';
> +const DEFAULT_HOTSPOT_SECURITY_TYPE = 'open';
> +const DEFAULT_HOTSPOT_SECURITY_PWD = '1234567890';

Surely these should be configurable. I'm ok with const'ing these for now to get the patch out of the door, but please file a follow-up for making these settings, and mention the bug number in a // TODO comment above this stanza.

@@ +46,5 @@
> +const DEFAULT_HOTSPOT_SECURITY_PWD = '1234567890';
> +
> +const DEFAULT_USB_INTERFACE_NAME = 'rndis0';
> +const DEFAULT_3G_INTERFACE_NAME = 'rmnet0';
> +const DEFAULT_WIFI_INTERFACE_NAME = 'wlan0';

Shouldn't we detect these? I know that at least the Wifi interface name differs across devices.

@@ +50,5 @@
> +const DEFAULT_WIFI_INTERFACE_NAME = 'wlan0';
> +
> +const TYPE_WIFI = 'WiFi';
> +const TYPE_USB = 'USB';
> +const TYPE_BLUETOOTH = 'bluetooth';

Please be more specific. TETHERING_TYPE_WIFI would be more appropriate, for instance.

@@ +52,5 @@
> +const TYPE_WIFI = 'WiFi';
> +const TYPE_USB = 'USB';
> +const TYPE_BLUETOOTH = 'bluetooth';
> +
> +const DEBUG = true;

Ahem.

@@ +129,2 @@
>        return;
>      }

At this point we don't really need the `if` block because we have the `switch` statement below. Please remove it.

@@ +523,5 @@
> +      dump('fatal error !!! unsupported type');
> +    }
> +  },
> +
> +  loadWifiDriver: function loadWifiDriver() {

Why is this needed?

@@ +533,5 @@
> +      debug('callback loadWifiDriver' + data.response);
> +    });
> +  },
> +
> +  unloadWifiDriver: function unloadWifiDriver() {

Same question.

::: dom/system/gonk/net_worker.js
@@ +20,5 @@
> +const DEFAULT_USB_DHCP_STOP_IP = '192.168.1.6';
> +
> +const DEFAULT_USB_INTERFACE_NAME = 'rndis0';
> +const DEFAULT_3G_INTERFACE_NAME = 'rmnet0';
> +const DEFAULT_WIFI_INTERFACE_NAME = 'wlan0';

Please don't duplicate this information. Make the IP info and interface names parameters to the cross-thread messages and pass them from the main thread. We will have to do anyway when we make these configurable.

@@ +25,5 @@
> +
> +
> +importScripts('libhardware_legacy.js', 'systemlibs.js');
> +
> +var sm;

Nit: use `let`, not `var`. Also, I would highly encourage using more descriptive variable names. `gStateMachine` would be appropriate. For extra credit, add a comment explaining what the variable is used for.

@@ +28,5 @@
> +
> +var sm;
> +function wifiTetheringFail() {
> +  dump('##################################### wifi Fail');
> +}

What's purpose of these callbacks? Please either use `debug()`, propagate an error event to the main thread, or remove these.

@@ +42,5 @@
> +function usbTetheringSuccess() {
> +  dump('##################################### usb Success');
> +}
> +
> +var gEnableWiFiStates = [

The variable name should reflect that these states are about wifi *tethering*, not wifi itself. So e.g. `gEnableWifiTetheringStates`.

Also, please use `let`.

@@ +127,5 @@
> +      }
> +  }
> +];
> +
> +var gdisableWiFiStates = [

Should be `gDisableWifiStates` (correct use of camel case)

@@ +279,5 @@
> +  }
> +];
> +
> +/**
> + * simple statemechine to handle asynchronous tethering command sequence.

I think you're overthinking the problem a little bit. In the end it just looks like you want to chain a bunch of asynchronous functions. AFAICT the error handling is pretty simple because it doesn't matter which step fails, the entire process is one big transaction. (This is also evident from the fact that you're using `usbTetheringFail` everywhere for the failure callback).

I think if you adapted something like Async.chain() [1] for your purpose, you can easily get rid of the "state machine" object and this complicated data structure of callbacks. Heck, it's not even that many states, you could easily just nest the callbacks (not pretty, but effective):

  setIpForwardingEnabled(function () {
    stopTethering(function () {
      untetherInterface(function() {
        usbTetheringSuccess();
      });
    });
  });

(Error handling not included, but trivial to add.)

[1] https://mxr.mozilla.org/mozilla-central/source/services/common/async.js#23


I ended up glancing over the code of the state machine anyway and added some comments on it below. But that doesn't mean I expect you to fix this code; I'd rather see something much simpler.

@@ +284,5 @@
> + *
> + * @param name
> + *        name of this state machine.
> + * @param state
> + *        state table.

If the parameter contains a state table, it should be called `stateTable` or `states` or something, but not singular. I don't think "table" is quite correct anyway, since it's an array, not a table.

@@ +288,5 @@
> + *        state table.
> + * @param param
> + *        callback function's parameters.
> + */
> +function statemachine(name, state, param) {

Nit: object constructors should be named like `StateMachine`.

@@ +292,5 @@
> +function statemachine(name, state, param) {
> +  let initialstate = false;
> +
> +  if (this.gStates == null) {
> +    this.gStates = {};

Why is this object member called `gStates`? It's not a global.

@@ +306,5 @@
> +  this.gStates[name].curState = null;
> +  this.params = param;
> +
> +  for (let i = 0; i < this.gStates[name].length; i++) {
> +    if (this.gStates[name][i].initial == true) {

Coding style nit: don't compare to `true`.

@@ +361,3 @@
>      gateway_str: gateway_str,
> +    dns1_str: libcutils.property_get('net.' + ifname + '.dns1'),
> +    dns2_str: libcutils.property_get('net.' + ifname + '.dns2')

Please get rid of these unrelated formatting changes.

::: dom/wifi/WifiWorker.js
@@ +1170,5 @@
>  
>    this._mm = Cc["@mozilla.org/parentprocessmessagemanager;1"].getService(Ci.nsIFrameMessageManager);
>    const messages = ["WifiManager:setEnabled", "WifiManager:getNetworks",
>                      "WifiManager:associate", "WifiManager:forget",
> +                    "WifiManager:getState", "WifiManager:enableTethering", "WifiManager:disableTethering"];

What is the purpose of these IPC messages? I don't see where they're being sent.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 35 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #34)
> Comment on attachment 637383 [details] [diff] [review]
> Updated tethering implementation V3 and removed interface definition
> 
> Review of attachment 637383 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch contains a lot of unrelated changes that violate the coding
> style. In particular, changing double quotes to single quotes. Strings
> should be in double quotes everywhere (but if you find existing code that's
> wrong, please fight the temptation to clean it up.). Anyway, the unrelated
> changes create a lot of noise that make it very hard for me to see the
> actual changes you've created. Please remove them all.

  I use gjslint to detect coding style error and use fixjsstyle to fix it. It treats 
  double quotes as an error and replaces the double quotes to single automatically.
  There seems many discussion talking about this. 
  http://stackoverflow.com/questions/242813/when-to-use-double-or-single-quotes-in-javascript
   
  I am not sure the detail. So I will follow your comments to modify it.  
     
 
> ::: b2g/chrome/content/settings.js
> @@ +3,5 @@
> >  /* This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> >   * You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +'use strict;';
> 
> The correct string would be
> 
>   "use strict";
> 
> with double quotes (Mozilla coding style) and the semicolon outside of the
> string.
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +42,5 @@
> > +const DEFAULT_USB_DHCP_STOP_IP = '192.168.1.6';
> > +
> > +const DEFAULT_HOTSPOT_SSID = 'B2G_HOTSPOT';
> > +const DEFAULT_HOTSPOT_SECURITY_TYPE = 'open';
> > +const DEFAULT_HOTSPOT_SECURITY_PWD = '1234567890';
> 
> Surely these should be configurable. I'm ok with const'ing these for now to
> get the patch out of the door, but please file a follow-up for making these
> settings, and mention the bug number in a // TODO comment above this stanza.

  The ssid, security type and security password will be obtained from preferences 
  in setWiFiTethering function
  
    let ssid = Services.prefs.getCharPref("tethering.wifi.ssid");
    let secType = Services.prefs.getCharPref("tethering.wifi.security.type");
    let secPwd = Services.prefs.getCharPref("tethering.wifi.security.password");

   and preferences will be updated in b2g/chrome/content/settings.js when we change settings for these path. 
> @@ +46,5 @@
> > +const DEFAULT_HOTSPOT_SECURITY_PWD = '1234567890';
> > +
> > +const DEFAULT_USB_INTERFACE_NAME = 'rndis0';
> > +const DEFAULT_3G_INTERFACE_NAME = 'rmnet0';
> > +const DEFAULT_WIFI_INTERFACE_NAME = 'wlan0';
> 
> Shouldn't we detect these? I know that at least the Wifi interface name
> differs across devices.

  We can have 3G and wifi interface name from networkInterfaces when they are registered 
  However, if 3G or wifi interface is not enabled, we can't have the interface name and
  should generate some notification to user. Any comments about this is welcome. 

  As for the usb interface, I don't have better idea to get it so far. Do we have platform 
  dependent configuration file to help to distinguish from different hardwares or android 
  versions ? 

> 
> @@ +50,5 @@
> > +const DEFAULT_WIFI_INTERFACE_NAME = 'wlan0';
> > +
> > +const TYPE_WIFI = 'WiFi';
> > +const TYPE_USB = 'USB';
> > +const TYPE_BLUETOOTH = 'bluetooth';
> 
> Please be more specific. TETHERING_TYPE_WIFI would be more appropriate, for
> instance.
> 
> @@ +52,5 @@
> > +const TYPE_WIFI = 'WiFi';
> > +const TYPE_USB = 'USB';
> > +const TYPE_BLUETOOTH = 'bluetooth';
> > +
> > +const DEBUG = true;
> 
> Ahem.
> 
> @@ +129,2 @@
> >        return;
> >      }
> 
> At this point we don't really need the `if` block because we have the
> `switch` statement below. Please remove it.
> 
> @@ +523,5 @@
> > +      dump('fatal error !!! unsupported type');
> > +    }
> > +  },
> > +
> > +  loadWifiDriver: function loadWifiDriver() {
> 
> Why is this needed?
> 
> @@ +533,5 @@
> > +      debug('callback loadWifiDriver' + data.response);
> > +    });
> > +  },
> > +
> > +  unloadWifiDriver: function unloadWifiDriver() {
> 
> Same question.

  I am keeping these just in case we need to load wifi driver by NetworkManager, rather than 
  by WiFiManager. 

> ::: dom/system/gonk/net_worker.js
> @@ +20,5 @@
> > +const DEFAULT_USB_DHCP_STOP_IP = '192.168.1.6';
> > +
> > +const DEFAULT_USB_INTERFACE_NAME = 'rndis0';
> > +const DEFAULT_3G_INTERFACE_NAME = 'rmnet0';
> > +const DEFAULT_WIFI_INTERFACE_NAME = 'wlan0';
> 
> Please don't duplicate this information. Make the IP info and interface
> names parameters to the cross-thread messages and pass them from the main
> thread. We will have to do anyway when we make these configurable.

  Good idea, I will pass these parameters from main thread.   

> @@ +25,5 @@
> > +
> > +
> > +importScripts('libhardware_legacy.js', 'systemlibs.js');
> > +
> > +var sm;
> 
> Nit: use `let`, not `var`. Also, I would highly encourage using more
> descriptive variable names. `gStateMachine` would be appropriate. For extra
> credit, add a comment explaining what the variable is used for.
> 
> @@ +28,5 @@
> > +
> > +var sm;
> > +function wifiTetheringFail() {
> > +  dump('##################################### wifi Fail');
> > +}
> 
> What's purpose of these callbacks? Please either use `debug()`, propagate an
> error event to the main thread, or remove these.

  I will propagate an error event to the main thread and indicate the error state. 

> @@ +42,5 @@
> > +function usbTetheringSuccess() {
> > +  dump('##################################### usb Success');
> > +}
> > +
> > +var gEnableWiFiStates = [
> 
> The variable name should reflect that these states are about wifi
> *tethering*, not wifi itself. So e.g. `gEnableWifiTetheringStates`.
> 
> Also, please use `let`.
> 
> @@ +127,5 @@
> > +      }
> > +  }
> > +];
> > +
> > +var gdisableWiFiStates = [
> 
> Should be `gDisableWifiStates` (correct use of camel case)
> 
> @@ +279,5 @@
> > +  }
> > +];
> > +
> > +/**
> > + * simple statemechine to handle asynchronous tethering command sequence.
> 
> I think you're overthinking the problem a little bit. In the end it just
> looks like you want to chain a bunch of asynchronous functions. AFAICT the
> error handling is pretty simple because it doesn't matter which step fails,
> the entire process is one big transaction. (This is also evident from the
> fact that you're using `usbTetheringFail` everywhere for the failure
> callback).
> 
> I think if you adapted something like Async.chain() [1] for your purpose,
> you can easily get rid of the "state machine" object and this complicated
> data structure of callbacks. Heck, it's not even that many states, you could
> easily just nest the callbacks (not pretty, but effective):
> 
>   setIpForwardingEnabled(function () {
>     stopTethering(function () {
>       untetherInterface(function() {
>         usbTetheringSuccess();
>       });
>     });
>   });
> 
> (Error handling not included, but trivial to add.)
> 
> [1]
> https://mxr.mozilla.org/mozilla-central/source/services/common/async.js#23
> 
> 
> I ended up glancing over the code of the state machine anyway and added some
> comments on it below. But that doesn't mean I expect you to fix this code;
> I'd rather see something much simpler.

  I will study a little bit more about async.js. Thank you for your information. 
  But before that, can I keep these state machine code ? 


> @@ +284,5 @@
> > + *
> > + * @param name
> > + *        name of this state machine.
> > + * @param state
> > + *        state table.
> 
> If the parameter contains a state table, it should be called `stateTable` or
> `states` or something, but not singular. I don't think "table" is quite
> correct anyway, since it's an array, not a table.
> 
> @@ +288,5 @@
> > + *        state table.
> > + * @param param
> > + *        callback function's parameters.
> > + */
> > +function statemachine(name, state, param) {
> 
> Nit: object constructors should be named like `StateMachine`.
> 
> @@ +292,5 @@
> > +function statemachine(name, state, param) {
> > +  let initialstate = false;
> > +
> > +  if (this.gStates == null) {
> > +    this.gStates = {};
> 
> Why is this object member called `gStates`? It's not a global.
> 
> @@ +306,5 @@
> > +  this.gStates[name].curState = null;
> > +  this.params = param;
> > +
> > +  for (let i = 0; i < this.gStates[name].length; i++) {
> > +    if (this.gStates[name][i].initial == true) {
> 
> Coding style nit: don't compare to `true`.
> 
> @@ +361,3 @@
> >      gateway_str: gateway_str,
> > +    dns1_str: libcutils.property_get('net.' + ifname + '.dns1'),
> > +    dns2_str: libcutils.property_get('net.' + ifname + '.dns2')
> 
> Please get rid of these unrelated formatting changes.
> 
> ::: dom/wifi/WifiWorker.js
> @@ +1170,5 @@
> >  
> >    this._mm = Cc["@mozilla.org/parentprocessmessagemanager;1"].getService(Ci.nsIFrameMessageManager);
> >    const messages = ["WifiManager:setEnabled", "WifiManager:getNetworks",
> >                      "WifiManager:associate", "WifiManager:forget",
> > +                    "WifiManager:getState", "WifiManager:enableTethering", "WifiManager:disableTethering"];
> 
> What is the purpose of these IPC messages? I don't see where they're being
> sent.

The WifiManager should do something before we can do the wifi tethering. But I am not quite sure what actions should WiFiManager do. Maybe some applications are using WiFi interface to 
send data ? Or maybe it should terminate wpa_supplicate process. Any comments are very welcome.  


Thank you for spending time to review my patch. I will try to update the patch soon.

	Dietrich Ayala (:dietrich)
	Comment 37 • 12 years ago

The IPC patch is unreviewed for over a week. Is Bent out of the office? If so, is there anyone else who can review?

	Philipp von Weitershausen [:philikon]
	Comment 38 • 12 years ago

Comment on attachment 639594 [details] [diff] [review]
Updated tethering implementation V4 based on comment 34

(Please don't forget to set the patch flag when you upload patches so that splinter review mode works on the attachment.)

	Philipp von Weitershausen [:philikon]
	Comment 39 • 12 years ago

(In reply to Vincent Chang from comment #35)
> > This patch contains a lot of unrelated changes that violate the coding
> > style. In particular, changing double quotes to single quotes. Strings
> > should be in double quotes everywhere (but if you find existing code that's
> > wrong, please fight the temptation to clean it up.). Anyway, the unrelated
> > changes create a lot of noise that make it very hard for me to see the
> > actual changes you've created. Please remove them all.
> 
>   I use gjslint to detect coding style error and use fixjsstyle to fix it.
>   It treats double quotes as an error and replaces the double quotes to single
>   automatically. There seems many discussion talking about this. 

Sure, people love bikeshedding about coding style. That's why we have *one* project-wide one. What's sad is that people still manage to deviate from it and defend it with random tools. Let's not get bogged down with that, let alone reformat existing code.

> > ::: dom/system/gonk/NetworkManager.js
> > @@ +42,5 @@
> > > +const DEFAULT_USB_DHCP_STOP_IP = '192.168.1.6';
> > > +
> > > +const DEFAULT_HOTSPOT_SSID = 'B2G_HOTSPOT';
> > > +const DEFAULT_HOTSPOT_SECURITY_TYPE = 'open';
> > > +const DEFAULT_HOTSPOT_SECURITY_PWD = '1234567890';
> > 
> > Surely these should be configurable. I'm ok with const'ing these for now to
> > get the patch out of the door, but please file a follow-up for making these
> > settings, and mention the bug number in a // TODO comment above this stanza.
> 
> The ssid, security type and security password will be obtained from
> preferences in setWiFiTethering function
>   
>     let ssid = Services.prefs.getCharPref("tethering.wifi.ssid");
>     let secType = Services.prefs.getCharPref("tethering.wifi.security.type");
>     let secPwd =
> Services.prefs.getCharPref("tethering.wifi.security.password");
> 
> and preferences will be updated in b2g/chrome/content/settings.js when we
> change settings for these path. 

Right. My point is, we should *always* use those values. And we should make sure that they *always* exist. So if there are default values, let's put them there. I wouldn't want to be in the position where you can turn on tethering but the user can't actually see any of the settings in the settings app because we're using some default consts in the code.

> > @@ +46,5 @@
> > > +const DEFAULT_HOTSPOT_SECURITY_PWD = '1234567890';
> > > +
> > > +const DEFAULT_USB_INTERFACE_NAME = 'rndis0';
> > > +const DEFAULT_3G_INTERFACE_NAME = 'rmnet0';
> > > +const DEFAULT_WIFI_INTERFACE_NAME = 'wlan0';
> > 
> > Shouldn't we detect these? I know that at least the Wifi interface name
> > differs across devices.
> 
> We can have 3G and wifi interface name from networkInterfaces when they
> are registered
> However, if 3G or wifi interface is not enabled, we can't have the interface name
> and should generate some notification to user. Any comments about this is welcome. 

Yep, we need to make sure both interfaces were enabled before. Perhaps we can even trigger that programmatically. We can punt on this for now, but let's make sure we file follow-up bugs.

> As for the usb interface, I don't have better idea to get it so far. Do we
> have platform dependent configuration file to help to distinguish from different
> hardwares or android versions ? 

I'm afraid I don't know. Let's do this in a follow-up and involve our hardware partners in the discussion.

> > @@ +523,5 @@
> > > +      dump('fatal error !!! unsupported type');
> > > +    }
> > > +  },
> > > +
> > > +  loadWifiDriver: function loadWifiDriver() {
> > 
> > Why is this needed?
> > 
> > @@ +533,5 @@
> > > +      debug('callback loadWifiDriver' + data.response);
> > > +    });
> > > +  },
> > > +
> > > +  unloadWifiDriver: function unloadWifiDriver() {
> > 
> > Same question.
> 
> I am keeping these just in case we need to load wifi driver by
> NetworkManager, rather than by WiFiManager. 

But it isn't called anywhere, as far as I can tell. Dead code will bitrot. Do not want.

> >   setIpForwardingEnabled(function () {
> >     stopTethering(function () {
> >       untetherInterface(function() {
> >         usbTetheringSuccess();
> >       });
> >     });
> >   });
> > 
> > (Error handling not included, but trivial to add.)
> > 
> > [1]
> > https://mxr.mozilla.org/mozilla-central/source/services/common/async.js#23
> > 
> > 
> > I ended up glancing over the code of the state machine anyway and added some
> > comments on it below. But that doesn't mean I expect you to fix this code;
> > I'd rather see something much simpler.
> 
> I will study a little bit more about async.js. Thank you for your information. 
> But before that, can I keep these state machine code ? 

I would prefer not to. In lieu of something like Async.chain(), just use nested callbacks like above. It's not like there's *that* much nesting, and it's much clearer than the state machine code. The code sample I wrote above can be read and understood in a few seconds. It took me many minutes to understand your state machine.

> > ::: dom/wifi/WifiWorker.js
> > @@ +1170,5 @@
> > >  
> > >    this._mm = Cc["@mozilla.org/parentprocessmessagemanager;1"].getService(Ci.nsIFrameMessageManager);
> > >    const messages = ["WifiManager:setEnabled", "WifiManager:getNetworks",
> > >                      "WifiManager:associate", "WifiManager:forget",
> > > +                    "WifiManager:getState", "WifiManager:enableTethering", "WifiManager:disableTethering"];
> > 
> > What is the purpose of these IPC messages? I don't see where they're being
> > sent.
> 
> The WifiManager should do something before we can do the wifi tethering. But
> I am not quite sure what actions should WiFiManager do. Maybe some
> applications are using WiFi interface to 
> send data ? Or maybe it should terminate wpa_supplicate process. Any
> comments are very welcome.  

So what you're saying is that either your patch is incomplete and needs to be amended still, or we need to address this issue in a follow-up. Either one is fine by me, but again, let's not commit dead code.

	Philipp von Weitershausen [:philikon]
	Comment 40 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #39)
> > > ::: dom/wifi/WifiWorker.js
> > > @@ +1170,5 @@
> > > >  
> > > >    this._mm = Cc["@mozilla.org/parentprocessmessagemanager;1"].getService(Ci.nsIFrameMessageManager);
> > > >    const messages = ["WifiManager:setEnabled", "WifiManager:getNetworks",
> > > >                      "WifiManager:associate", "WifiManager:forget",
> > > > +                    "WifiManager:getState", "WifiManager:enableTethering", "WifiManager:disableTethering"];
> > > 
> > > What is the purpose of these IPC messages? I don't see where they're being
> > > sent.
> > 
> > The WifiManager should do something before we can do the wifi tethering. But
> > I am not quite sure what actions should WiFiManager do. Maybe some
> > applications are using WiFi interface to 
> > send data ? Or maybe it should terminate wpa_supplicate process. Any
> > comments are very welcome.  
> 
> So what you're saying is that either your patch is incomplete and needs to
> be amended still, or we need to address this issue in a follow-up. Either
> one is fine by me, but again, let's not commit dead code.

Ahem, never mind, I now see the messages being used in NetworkManager.js. I must've been blind before. Sorry about that.

I'm a bit perplexed why you're using frame messages. AFAICT, NetworkManager and WifiWorker are all on the main thread of the chrome process... could just make these API calls! But I'll read your patch again more closely to figure out what's going on.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 41 • 12 years ago

(In reply to Dietrich Ayala (:dietrich) from comment #37)
> The IPC patch is unreviewed for over a week. Is Bent out of the office?

Yep, I was out. Back now!

	Philipp von Weitershausen [:philikon]
	Comment 42 • 12 years ago

This is a P2 item in the M4 list, so presumably this should block something. Nomming.

	Philipp von Weitershausen [:philikon]
	Comment 43 • 12 years ago

Comment on attachment 639594 [details] [diff] [review]
Updated tethering implementation V4 based on comment 34

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

Ok, I've given this a bit of a closer look, on top of what I've already written in comment 39.

::: dom/system/gonk/NetworkManager.js
@@ +59,5 @@
>   * This component watches for network interfaces changing state and then
>   * adjusts routes etc. accordingly.
>   */
>  function NetworkManager() {
> +  let settingsLock;

Inline below when you assign to `settingsLock`.

@@ +76,5 @@
>      debug("Received error from worker: " + event.filename +
>            ":" + event.lineno + ": " + event.message + "\n");
>      // Prevent the event from bubbling any further.
>      event.preventDefault();
> +  }.bind(this);

You're not using `this` in the function, so why this change?

@@ +84,5 @@
> +  // used to receive tethering result from worker.
> +  this.controlCallbacks ["tetheringResultReport"] = this.tetheringResultReport;
> +  this.idgen = 0;
> +
> +  //get the default value from settings API in handle callback function

Nit: Space between `//` and the first letter, capitalize the letter, period at the end of the sentence (English grammar plz!)

@@ +94,5 @@
> +  this._cpmm = Cc["@mozilla.org/childprocessmessagemanager;1"].
> +               getService(Ci.nsIFrameMessageManager);
> +
> +  const messages = ["WifiManager:enableTethering:OK", "WifiManager:enableTethering:NO",
> +               "WifiManager:disableTethering:OK", "WifiManager:disableTethering:NO"];

This is lacking proper clean up. You would want to move this to the global scope and make sure you call this._cpmm.removeMessageListener() for each of them during XPCOM shutdown, and then null this._cpmm.

However, I don't understand why you're using IPC messages to talk to another XPCOM component in the same process on the same thread. As far as I can tell, you're merely interested in the Wifi network interface name. I see several possibilities:

(a) access it directly from nsIWifi (WifiWorker) via

  let wifi = Cc["@mozilla.org/telephony/system-worker-manager;1"]
               .getService(Ci.nsIInterfaceRequestor)
               .getInterface(Ci.nsIWifi);
  let interfaceName = wifi.ifname;

(b) the Wifi code registers itself with the nsINetworkManager via an nsINetworkInterface; this interface already contains the interface name in the `name` attribute. You also added a `getNetworkInterface()` method to NetworkManager, so you could just query for nsINetworkInterface::NETWORK_TYPE_WIFI and use `interface.name`.

Whichever option we chose we have to make sure that Wifi was initialized enough so that its ifname is known.

@@ +98,5 @@
> +               "WifiManager:disableTethering:OK", "WifiManager:disableTethering:NO"];
> +
> +  messages.forEach((function(msgName) {
> +    this._cpmm.addMessageListener(msgName, this);
> +  }).bind(this));

Instead of `array.forEach(func.bind(this))`, you can do `array.forEach(func, this)`

@@ +283,5 @@
> +      if (network.type == type) {
> +        return network;
> +      }   
> +    }
> +    return "NotFound";   

Eeek, magic values! If we wanted something like this, then this would have to be const'ed. But I think we should just return `null` here.

@@ +332,5 @@
> +          else {
> +            dump("we have not run constructor yet !!!");
> +            this.tetheringResultReport(null, {response: "wifiTetheringFail", currentState: "initialstate"});
> +            return;
> +          }

Easier to read if you turn this around and do


  if (this._tetheringInterface == null) {
    ...
    return;
  }
  this.disableTethering(TETHERING_TYPE_USB, this._tetheringInterface);

@@ +430,5 @@
> +
> +    let options = {
> +      cmd: "setWiFiTethering",
> +      params : {
> +                 ifname: internalinterface,

Coding style nit: either do

  params: {ifname: internalinterface,

or

  params: {
    ifname: internalinterface,

@@ +589,5 @@
> +
> +  tetheringResultReport: function tetheringResultReport(obj, data) {
> +    let response = data.response;
> +    let currentState = data.currentState; 
> +    // todo, report enable/disable tethering result to gaia 

How are you planning on doing this? What sort of API are you proposing on the DOM side? Please either complete this part of the bug (ideally in a separate patch because this one is already getting quite large) or file a follow-up bug and mention it here.

Also, Gaia is just the name for *our* UI. In general, we're just sending messages to the DOM.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 44 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #43)
> Comment on attachment 639594 [details] [diff] [review]
> Updated tethering implementation V4 based on comment 34
> 
> Review of attachment 639594 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, I've given this a bit of a closer look, on top of what I've already
> written in comment 39.
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +59,5 @@
> >   * This component watches for network interfaces changing state and then
> >   * adjusts routes etc. accordingly.
> >   */
> >  function NetworkManager() {
> > +  let settingsLock;
> 
> Inline below when you assign to `settingsLock`.

  ok, I got it. 
> 
> @@ +76,5 @@
> >      debug("Received error from worker: " + event.filename +
> >            ":" + event.lineno + ": " + event.message + "\n");
> >      // Prevent the event from bubbling any further.
> >      event.preventDefault();
> > +  }.bind(this);
> 
> You're not using `this` in the function, so why this change?

  Ok, I will change it back.  

> @@ +84,5 @@
> > +  // used to receive tethering result from worker.
> > +  this.controlCallbacks ["tetheringResultReport"] = this.tetheringResultReport;
> > +  this.idgen = 0;
> > +
> > +  //get the default value from settings API in handle callback function
> 
> Nit: Space between `//` and the first letter, capitalize the letter, period
> at the end of the sentence (English grammar plz!)
 
  Ok, I will modify it. 
  
> @@ +94,5 @@
> > +  this._cpmm = Cc["@mozilla.org/childprocessmessagemanager;1"].
> > +               getService(Ci.nsIFrameMessageManager);
> > +
> > +  const messages = ["WifiManager:enableTethering:OK", "WifiManager:enableTethering:NO",
> > +               "WifiManager:disableTethering:OK", "WifiManager:disableTethering:NO"];
> 
> This is lacking proper clean up. You would want to move this to the global
> scope and make sure you call this._cpmm.removeMessageListener() for each of
> them during XPCOM shutdown, and then null this._cpmm.
> 
> However, I don't understand why you're using IPC messages to talk to another
> XPCOM component in the same process on the same thread. As far as I can
> tell, you're merely interested in the Wifi network interface name. I see
> several possibilities:
> 
> (a) access it directly from nsIWifi (WifiWorker) via
> 
>   let wifi = Cc["@mozilla.org/telephony/system-worker-manager;1"]
>                .getService(Ci.nsIInterfaceRequestor)
>                .getInterface(Ci.nsIWifi);
>   let interfaceName = wifi.ifname;
> 
> (b) the Wifi code registers itself with the nsINetworkManager via an
> nsINetworkInterface; this interface already contains the interface name in
> the `name` attribute. You also added a `getNetworkInterface()` method to
> NetworkManager, so you could just query for
> nsINetworkInterface::NETWORK_TYPE_WIFI and use `interface.name`.
> 
> Whichever option we chose we have to make sure that Wifi was initialized
> enough so that its ifname is known.

  I will chose option (a). But a little bit different here. I would like to create the new function in nsIWifi interface. When we enable wifi tethering, we call reqWifiTethering() function and do the following things.  

interface nsIWifi : nsISupports
{
    /** 
     * Shutdown the wifi system.
     */
    void shutdown();
    void reqWifiTethering(in boolean enabled);
};

1. check if wifi connection is enabled, disabled it if it is enabled. So the wifi driver and wpa_supplicant will be removed. 

2. After disabling wifi connection, call setWifiApEnable() function to reload wifi driver. Some wifi driver may load different driver when it behaves as AP. But it should be same in galaxys2 case. So just reload the wifi driver and set wifi interface up.

3. add two new functions in nsINetworkManager interface. When loading wifi driver successfully, call reqWifiTehteringSuccess(), otherwise, call reqWifiTetheringFail() function.       

  void reqWifiTetheringSuccess(in nsINetworkInterface network);
  void reqWifiTetheringFail(in nsINetworkInterface network);

4. In reqWifiTetheringSuccess() function, we can have wifi interface name. Then, we can start to talk to netd to true on/off wifi tethering. 

> 
> @@ +98,5 @@
> > +               "WifiManager:disableTethering:OK", "WifiManager:disableTethering:NO"];
> > +
> > +  messages.forEach((function(msgName) {
> > +    this._cpmm.addMessageListener(msgName, this);
> > +  }).bind(this));
> 
> Instead of `array.forEach(func.bind(this))`, you can do `array.forEach(func,
> this)`
> 
> @@ +283,5 @@
> > +      if (network.type == type) {
> > +        return network;
> > +      }   
> > +    }
> > +    return "NotFound";   
> 
> Eeek, magic values! If we wanted something like this, then this would have
> to be const'ed. But I think we should just return `null` here.

  OK, I will modify it. 
> 
> @@ +332,5 @@
> > +          else {
> > +            dump("we have not run constructor yet !!!");
> > +            this.tetheringResultReport(null, {response: "wifiTetheringFail", currentState: "initialstate"});
> > +            return;
> > +          }
> 
> Easier to read if you turn this around and do
> 
> 
>   if (this._tetheringInterface == null) {
>     ...
>     return;
>   }
>   this.disableTethering(TETHERING_TYPE_USB, this._tetheringInterface);
> 
> @@ +430,5 @@
> > +
> > +    let options = {
> > +      cmd: "setWiFiTethering",
> > +      params : {
> > +                 ifname: internalinterface,
> 
> Coding style nit: either do
> 
>   params: {ifname: internalinterface,
> 
> or
> 
>   params: {
>     ifname: internalinterface,

  Ok, I will correct it. 
> 
> @@ +589,5 @@
> > +
> > +  tetheringResultReport: function tetheringResultReport(obj, data) {
> > +    let response = data.response;
> > +    let currentState = data.currentState; 
> > +    // todo, report enable/disable tethering result to gaia 
> 
> How are you planning on doing this? What sort of API are you proposing on
> the DOM side? Please either complete this part of the bug (ideally in a
> separate patch because this one is already getting quite large) or file a
> follow-up bug and mention it here.
> 
> Also, Gaia is just the name for *our* UI. In general, we're just sending
> messages to the DOM.

I will file a follow-up bug for this. Using read-only settings to notify gaia 
is fine for me as you mentioned in dev-b2g mail-list. Could I have your comments ? 

BTW, we also need to provide a API to enable/disable mobile connection. I will file the other bug for this, too.

	Philipp von Weitershausen [:philikon]
	Comment 45 • 12 years ago

(In reply to Vincent Chang from comment #44)
>   I will chose option (a). But a little bit different here. I would like to
> create the new function in nsIWifi interface. When we enable wifi tethering,
> we call reqWifiTethering() function and do the following things.

What does the "req" stand for? require? request? Please write words fully out, we can afford the extra characters. No need to be cryptic.

Anyway, from what I can tell from your proposed interface, it seems `setWifiTethering(enabled)` would be a good option, too.

> interface nsIWifi : nsISupports
> {
>     /** 
>      * Shutdown the wifi system.
>      */
>     void shutdown();
>     void reqWifiTethering(in boolean enabled);
> };
> 
> 1. check if wifi connection is enabled, disabled it if it is enabled. So the
> wifi driver and wpa_supplicant will be removed. 
> 
> 2. After disabling wifi connection, call setWifiApEnable() function to
> reload wifi driver. Some wifi driver may load different driver when it
> behaves as AP. But it should be same in galaxys2 case. So just reload the
> wifi driver and set wifi interface up.

Those two sound good to me. However, mrbkap should be in the loop for these changes. He may have some valuable input. If you can, please split the Wifi-tethering bits to a separate patch that mrbkap can review.

> 3. add two new functions in nsINetworkManager interface. When loading wifi
> driver successfully, call reqWifiTehteringSuccess(), otherwise, call
> reqWifiTetheringFail() function.       
> 
>   void reqWifiTetheringSuccess(in nsINetworkInterface network);
>   void reqWifiTetheringFail(in nsINetworkInterface network);

That's pretty ugly and very hard to follow. Why not add a `callback` argument to setWifiTethering()?

> BTW, we also need to provide a API to enable/disable mobile connection. I
> will file the other bug for this, too.

Sounds good. (By API you mean XPCOM API, right?)

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 46 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #45)
> (In reply to Vincent Chang from comment #44)
> >   I will chose option (a). But a little bit different here. I would like to
> > create the new function in nsIWifi interface. When we enable wifi tethering,
> > we call reqWifiTethering() function and do the following things.
> 
> What does the "req" stand for? require? request? Please write words fully
> out, we can afford the extra characters. No need to be cryptic.
> 
> Anyway, from what I can tell from your proposed interface, it seems
> `setWifiTethering(enabled)` would be a good option, too.
> 
> > interface nsIWifi : nsISupports
> > {
> >     /** 
> >      * Shutdown the wifi system.
> >      */
> >     void shutdown();
> >     void reqWifiTethering(in boolean enabled);
> > };
> > 
> > 1. check if wifi connection is enabled, disabled it if it is enabled. So the
> > wifi driver and wpa_supplicant will be removed. 
> > 
> > 2. After disabling wifi connection, call setWifiApEnable() function to
> > reload wifi driver. Some wifi driver may load different driver when it
> > behaves as AP. But it should be same in galaxys2 case. So just reload the
> > wifi driver and set wifi interface up.
> 
> Those two sound good to me. However, mrbkap should be in the loop for these
> changes. He may have some valuable input. If you can, please split the
> Wifi-tethering bits to a separate patch that mrbkap can review.
> 
> > 3. add two new functions in nsINetworkManager interface. When loading wifi
> > driver successfully, call reqWifiTehteringSuccess(), otherwise, call
> > reqWifiTetheringFail() function.       
> > 
> >   void reqWifiTetheringSuccess(in nsINetworkInterface network);
> >   void reqWifiTetheringFail(in nsINetworkInterface network);
> 
> That's pretty ugly and very hard to follow. Why not add a `callback`
> argument to setWifiTethering()?

How about the interfaces definition below ?  

[scriptable, uuid(e232c530-cc9f-11e1-9b23-0800200c9a66)]
interface nsISetWifiTetheringCallback : nsISupports
{
    /** 
     * Report the setWifiTetheringEnabled result and pass nsINetworkInterface to caller.
     */
    void setWifiTetheringEnabledResult(in jsval network, in short result);
    /** 
     * Report the setWifiTetheringDisabled result and pass nsINetworkInterface to caller.
     */
    void setWifiTetheringDisabledResult(in short result);
};

[scriptable, uuid(abb936bc-ba81-4c23-8dfa-3e5d96557044)]
interface nsIWifi : nsISupports
{
    /**
     * Shutdown the wifi system.
     */
    void shutdown();
    /**
     * Request to enable/disable Wifi Tethering.
     */
    void setWifiTethering(in boolean enabled, in nsISetWifiTetheringCallback callback);
};


 
> > BTW, we also need to provide a API to enable/disable mobile connection. I
> > will file the other bug for this, too.
> 
> Sounds good. (By API you mean XPCOM API, right?)

Yes, I would like to create XPCOM API to control mobile data connection. BTW, it should be 
in the same thread with NetworkManager right ?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 47 • 12 years ago

Comment on attachment 637381 [details] [diff] [review]
Updated IPC interface implementation.

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

Hm. The mixture of mfbt, stdlib, and chromium is a bit dizzying here. Not your fault!

Lots of cleanup needed here but also some more serious stuff.

::: dom/system/gonk/SystemWorkerManager.cpp
@@ +51,5 @@
>  #ifdef MOZ_B2G_BT
>  using namespace mozilla::dom::bluetooth;
>  #endif
>  
> +

Nit: Please remove this extra line.

@@ +65,5 @@
>  NS_DEFINE_CID(kRadioInterfaceLayerCID, NS_RADIOINTERFACELAYER_CID);
>  NS_DEFINE_CID(kWifiWorkerCID, NS_WIFIWORKER_CID);
> +NS_DEFINE_CID(kNetworkManagerCID, NS_NETWORKMANAGER_CID);
> +
> +

Nit: please remove these two extra lines.

@@ +195,5 @@
>                               argv, argv);
>  }
>  
> +JSBool
> +doNetdCommand(JSContext *cx, unsigned argc, jsval *vp)

Nit: Functions should start with a capital letter.

@@ +206,5 @@
> +  }
> +
> +  jsval v = JS_ARGV(cx, vp)[0];
> +
> +  nsAutoPtr<NetdCommand> rm(new NetdCommand());

What is "rm"? Please give this a better name.

@@ +247,5 @@
> +    return false;
> +  }
> +
> +  rm->mSize = size + 1;
> +  memcpy(rm->mData, data, size);

This is weird. Were you intending to null-terminate the size? If so, you're not doing it. But I don't think you should anyway. Just lose the +1 there.

@@ +248,5 @@
> +  }
> +
> +  rm->mSize = size + 1;
> +  memcpy(rm->mData, data, size);
> +  LOG("size = %d", size);

This isn't a very useful log message. Is it just for your debugging? Either improve or remove.

@@ +250,5 @@
> +  rm->mSize = size + 1;
> +  memcpy(rm->mData, data, size);
> +  LOG("size = %d", size);
> +  NetdCommand *tosend = rm.forget();
> +  JS_ALWAYS_TRUE(SendNetdCommand(&tosend));

In general it's a bad idea to put function calls in macros (you don't know how many times the function will be called). Also, I proposed this later, but I think this will be nicer without doing the separate stack pointer and passing the address of it to SendNetdCommand.

@@ +294,5 @@
> +  { }
> +
> +  virtual void MessageReceived(NetdCommand *aMessage) {
> +    nsRefPtr<DispatchNetdEvent> dre(new DispatchNetdEvent(aMessage));
> +    mDispatcher->PostTask(dre);

This can fail, you should at least warn if it does.

@@ +525,5 @@
> +SystemWorkerManager::InitNetd(JSContext *cx)
> +{
> +  nsCOMPtr<nsIWorkerHolder> worker = do_GetService(NETWORKMANAGER_CONTRACTID);
> +  NS_ENSURE_TRUE(worker, NS_ERROR_FAILURE);
> +  jsval workerval;

Nit: Newline above here.

@@ +527,5 @@
> +  nsCOMPtr<nsIWorkerHolder> worker = do_GetService(NETWORKMANAGER_CONTRACTID);
> +  NS_ENSURE_TRUE(worker, NS_ERROR_FAILURE);
> +  jsval workerval;
> +  nsresult rv = worker->GetWorker(&workerval);
> +

Nit: remove this line.

@@ +534,5 @@
> +
> +  JSAutoRequest ar(cx);
> +  JSAutoEnterCompartment ac;
> +  if (!ac.enter(cx, JSVAL_TO_OBJECT(workerval))) {
> +    return NS_ERROR_OUT_OF_MEMORY;

Nit: Add |NS_WARNING("Failed to enter compartment!");| here

@@ +536,5 @@
> +  JSAutoEnterCompartment ac;
> +  if (!ac.enter(cx, JSVAL_TO_OBJECT(workerval))) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +  WorkerCrossThreadDispatcher *wctd =

Nit: newline above here.

@@ +539,5 @@
> +  }
> +  WorkerCrossThreadDispatcher *wctd =
> +    GetWorkerCrossThreadDispatcher(cx, workerval);
> +  if (!wctd) {
> +    return NS_ERROR_FAILURE;

Need a warning here too.

@@ +544,5 @@
> +  }
> +
> +  nsRefPtr<ConnectWorkerToNetd> connection = new ConnectWorkerToNetd();
> +  if (!wctd->PostTask(connection)) {
> +    return NS_ERROR_UNEXPECTED;

And here.

::: ipc/netd/Netd.cpp
@@ +12,5 @@
> +#include "nsXULAppAPI.h"
> +
> +#include "cutils/properties.h"
> +#include "android/log.h"
> +#include "Netd.h"

Please make Netd.h the very first include in this file. That way you can make sure that the header compiles on its own.

@@ +21,5 @@
> +namespace mozilla {
> +namespace system {
> +
> +static RefPtr<NetdClient> sNetdClient;
> +static RefPtr<NetdConsumer> sNetdConsumer;

Why are you declaring these inside the mozilla::system namespace? They should just be globals in this file I believe (just make an anonymous namespace here).

@@ +22,5 @@
> +namespace system {
> +
> +static RefPtr<NetdClient> sNetdClient;
> +static RefPtr<NetdConsumer> sNetdConsumer;
> +/***************************************************************************/

Nit: This doesn't seem to be very helpful.

@@ +23,5 @@
> +
> +static RefPtr<NetdClient> sNetdClient;
> +static RefPtr<NetdConsumer> sNetdConsumer;
> +/***************************************************************************/
> +NetdClient::NetdClient()

You're not initializing mBlockedOnWrite or mCurrentWriteOffset, please do.

Also, please add the MOZ_COUNT_CTOR macro here.

@@ +24,5 @@
> +static RefPtr<NetdClient> sNetdClient;
> +static RefPtr<NetdConsumer> sNetdConsumer;
> +/***************************************************************************/
> +NetdClient::NetdClient()
> +  : mSocket(-1)

Is there some INVALID_SOCKET define or something you could use here? If not please create one. "Magic" numeric constants are hard to read.

@@ +27,5 @@
> +NetdClient::NetdClient()
> +  : mSocket(-1)
> +  , mIOLoop(MessageLoopForIO::current())
> +  , mMutex("NetdClient.mMutex")
> +  , mCurrentNetdCommand(NULL)

This is not needed, nsAutoPtr initializes to null automatically.

@@ +32,5 @@
> +  , mRcvIdx(0)
> +{
> +}
> +
> +NetdClient::~NetdClient()

Please add the MOZ_COUNT_DTOR macro here.

@@ +41,5 @@
> +NetdClient::OpenSocket()
> +{
> +  if ((mSocket.rwget() = socket_local_client("netd",
> +                                             ANDROID_SOCKET_NAMESPACE_RESERVED,
> +                                             SOCK_STREAM)) < 0) {

Let's split this into two statements:

  mSocket.rwget() = ...;
  if (mSocket.get() < 0) {

@@ +45,5 @@
> +                                             SOCK_STREAM)) < 0) {
> +    LOG("Error connecting to : netd (%s) - will retry", strerror(errno));
> +    return false;
> +  }
> +  // add FD_CLOEXEC flag

Nit: Please make your comments real sentences (i.e. begin with capital letter, end with period). Here and everywhere.

@@ +47,5 @@
> +    return false;
> +  }
> +  // add FD_CLOEXEC flag
> +  int flags = fcntl(mSocket.get(), F_GETFD);
> +  if (flags == -1) {

Hm, the docs I see say that this will always return current flags, so checking for -1 seems unnecessary.

@@ +52,5 @@
> +    return false;
> +  }
> +  flags |= FD_CLOEXEC;
> +  if (fcntl(mSocket.get(), F_SETFD, flags) == -1) {
> +    return false;

LOG failure here and in the other places where you exit early.

@@ +80,5 @@
> +  char rcvBuf[MAX_COMMAND_SIZE];  
> +
> +  MOZ_ASSERT(fd == mSocket.get());
> +  while (true) {
> +    length = read(fd, &rcvBuf[mRcvIdx], MAX_COMMAND_SIZE - mRcvIdx);

Wait, what happens if you fill up your buffer? Looks like you'll just exit.

Also, you should at least assert that length <= MAX_COMMAND_SIZE - mRcvIdx.

@@ +86,5 @@
> +      if (length == -1) {
> +        if (errno == EINTR) {
> +          continue; // retry system call when interrupted
> +        }   
> +        else if (errno == EAGAIN || errno == EWOULDBLOCK) {

Nit: No need for 'else' here

@@ +96,5 @@
> +      // At this point, assume that we can't actually access
> +      // the socket anymore, and start a reconnect loop.
> +      mReadWatcher.StopWatchingFileDescriptor();
> +      mWriteWatcher.StopWatchingFileDescriptor();
> +      close(mSocket.get());

Should you null out mSocket now? Then it would automatically call close on mSocket.

@@ +99,5 @@
> +      mWriteWatcher.StopWatchingFileDescriptor();
> +      close(mSocket.get());
> +      return;
> +    } 
> +    pos = length; 

Nit: newline above here.

@@ +101,5 @@
> +      return;
> +    } 
> +    pos = length; 
> +    while (pos > 0) {
> +      pos--;

Nit: This could just be:

  while (pos--)

@@ +107,5 @@
> +        // We found a line terminator. Each line is formatted as an
> +        // integer response code followed by the rest of the line.
> +        // Fish out the response code.
> +        char *endPtr;
> +        int responseCode = strtol(rcvBuf, &endPtr, 10);

You're not checking for errors here.

@@ +109,5 @@
> +        // Fish out the response code.
> +        char *endPtr;
> +        int responseCode = strtol(rcvBuf, &endPtr, 10);
> +        if (*endPtr == ' ') {
> +          endPtr++;

It seems possible that this could bump you to the null terminator which might change what you should do below.

@@ +119,5 @@
> +          // These are unsolicited broadcasts. We intercept these and process
> +          // them ourselves
> +          // todo process InterfaceChange(600) and BandwidthControl(601)
> +          // skip this message  
> +          ;

Nit: Rather than do this, put the comment outside the if block, then just check responseCode < 600 and lose the else block.

@@ +121,5 @@
> +          // todo process InterfaceChange(600) and BandwidthControl(601)
> +          // skip this message  
> +          ;
> +        } else {
> +          mIncoming = new NetdCommand;

This member is a little useless. You only create it here, then immediately forget it below. How about just having this be a stack variable?

@@ +123,5 @@
> +          ;
> +        } else {
> +          mIncoming = new NetdCommand;
> +          // Everything else is considered to be part of the command response.
> +          mIncoming->mSize = length;

This isn't really right. The size is just up until the null terminator, and you're not guaranteed (as far as I can tell) that |rcvBuf + length| is always right. You want mRcvIdx I think.

@@ +124,5 @@
> +        } else {
> +          mIncoming = new NetdCommand;
> +          // Everything else is considered to be part of the command response.
> +          mIncoming->mSize = length;
> +          memcpy(mIncoming->mData, rcvBuf, length); 

Yeah, you may be copying part of the next message here.

@@ +132,5 @@
> +          // There is data in the receive buffer beyond the current line.
> +          // Shift it down to the beginning.
> +          memmove(&rcvBuf[0], &rcvBuf[mRcvIdx + 1], pos);
> +        }
> +        mRcvIdx = 0;

Wait, this isn't right if you just did a memmove. You need to set mRcvIndex to pos here.

@@ +143,5 @@
> +
> +void
> +NetdClient::OnFileCanWriteWithoutBlocking(int fd)
> +{
> +  MOZ_ASSERT(aFd == mSocket.get());

Um, this won't compile. Have you tried a debug build yet?

@@ +145,5 @@
> +NetdClient::OnFileCanWriteWithoutBlocking(int fd)
> +{
> +  MOZ_ASSERT(aFd == mSocket.get());
> +
> +  while (!mOutgoingQ.empty() || mCurrentNetdCommand != NULL) {

mCurrentNetdCommand is also kind of a useless member (it's only used in this function, and you both set and unset it here). Just make this a stack variable.

@@ +151,5 @@
> +      MutexAutoLock lock(mMutex);
> +
> +      mCurrentNetdCommand = mOutgoingQ.front();
> +      mOutgoingQ.pop();
> +      mCurrentWriteOffset = 0;

mCurrentWriteOffset is only used here too. Stack var please.

@@ +161,5 @@
> +
> +    while (mCurrentWriteOffset < mCurrentNetdCommand->mSize) {
> +      ssize_t write_amount = mCurrentNetdCommand->mSize - mCurrentWriteOffset;
> +      ssize_t written;
> +      written = write (fd, toWrite + mCurrentWriteOffset, write_amount);

Nit: ssize_t written = write(...)

@@ +166,5 @@
> +      if (written > 0) {
> +        mCurrentWriteOffset += written;
> +      }
> +      if (written != write_amount) {
> +        break;

I think it would make more sense to do this:

  if (written != write_amount) {
    NS_WARNING(...);
    break;
  }
  mCurrentWriteOffset += written;

@@ +177,5 @@
> +                  false,
> +                  MessageLoopForIO::WATCH_WRITE,
> +                  &mWriteWatcher,
> +                  this);
> +      return;

Hm, do you really want to always queue another write task? What about some catastrophic error (think something like disk full). Seems like we could be a little smarter here.

@@ +184,5 @@
> +  }
> +}
> +
> +void NetdWriteTask::Run() {
> +    sNetdClient->OnFileCanWriteWithoutBlocking(sNetdClient->mSocket.rwget());

You have a null check below for sNetdClient... Do you need one here? In general I like dealing with assertable invariants rather than guesswork, so I'd prefer if we could do without the null checks.

Nit: return type on its own line, and then { on its own line, and then 2-space indent instead of 4.

@@ +187,5 @@
> +void NetdWriteTask::Run() {
> +    sNetdClient->OnFileCanWriteWithoutBlocking(sNetdClient->mSocket.rwget());
> +}
> +
> +//static

Nit: space after //

@@ +194,5 @@
> +{
> +  MOZ_ASSERT(MessageLoop::current() == XRE_GetIOMessageLoop());
> +
> +  if (!sNetdClient) {
> +    return;

You want some kind of warning here right? Maybe even an assertion.

@@ +197,5 @@
> +  if (!sNetdClient) {
> +    return;
> +  }
> +  if (!sNetdClient->OpenSocket()) {
> +    // Socket open failed, try again in a second.

Hm, and what happens if this fails a second time, or a third? We need some way to report an error out of here somehow if we can't get this socket open after a while, and we need to not retry forever.

@@ +211,5 @@
> +InitRndisAddress()
> +{
> +  char mac[20];
> +  char serialno[] = "1234567890ABCDEF";
> +  int ETH_ALEN = 6;

static const int, and then please name it with lowercase letters (all caps are for macros). Also, no need to be so terse, let's call it "kEthernetAddressLength" or something?

@@ +225,5 @@
> +  }
> +  property_get("ro.serialno", serialno, "1234567890ABCDEF");
> +  // first byte is 0x02 to signify a locally administered address
> +  address[0] = 0x02;
> +  for (i = 0; i < strlen(serialno); i++) {

You're calling strlen in a loop here. Just call it once above.

@@ +233,5 @@
> +    address[0], address[1], address[2], address[3], address[4], address[5]);
> +  length = strlen(mac); 
> +  ret = write(fd, mac, length); 
> +  if (ret != length)
> +    LOG("Fail to write file %s.", ICS_SYS_USB_RNDIS_MAC);

Nit: Please brace all single-line if statements

@@ +244,5 @@
> +  MOZ_ASSERT(MessageLoop::current() == XRE_GetIOMessageLoop());
> +  MOZ_ASSERT(!sNetdClient);
> +
> +  InitRndisAddress();
> +  sNetdClient = new NetdClient();

Hm, is there any reason to do this if InitRndisAddress failed? Of course, since it's a void function, you can't tell if it failed anyway. I think you should make all of these functions that can possibly fail have a bool return value so that you can propagate and check error conditions better.

@@ +257,5 @@
> +  sNetdClient = NULL;
> +}
> +
> +//-----------------------------------------------------------------------------
> +// This code runs on any thread.

This is almost always bad. We should design this and then be able to ensure that each function happens only on one thread.

@@ +281,5 @@
> +
> +bool
> +SendNetdCommand(NetdCommand** aMessage)
> +{
> +  NetdCommand *msg = *aMessage;

You should assert that aMessage and *aMessage are both non-null.

Actually, why is this a NetdCommand** ? You're just trying to convey the transfer of ownership here? In-out params are awkward to use imo, I think a straight pointer is better.

@@ +283,5 @@
> +SendNetdCommand(NetdCommand** aMessage)
> +{
> +  NetdCommand *msg = *aMessage;
> +
> +  if (!sNetdClient) {

This isn't safe. You're accessing this variable on some random thread but it is set and unset on the IO thread. Is there any higher-level logic that prevents a race here?

@@ +288,5 @@
> +    return false;
> +  }
> +  *aMessage = nsnull;
> +  {
> +    MutexAutoLock lock(sNetdClient->mMutex);

So if you block here waiting on the mutex what prevents the other thread from destroying sNetdClient while you're waiting?

@@ +289,5 @@
> +  }
> +  *aMessage = nsnull;
> +  {
> +    MutexAutoLock lock(sNetdClient->mMutex);
> +    sNetdClient->mOutgoingQ.push(msg);

In general locking and member manipulation should be contained in a method. Rather than digging out the mutex and queue here you should just make a method on NetdClient like Push(NetdCommand*) that does all this internally.

::: ipc/netd/Netd.h
@@ +13,5 @@
> +
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +#include <sys/select.h>
> +#include <sys/types.h>

Wait, does this build on windows? I understand that we don't care about windows here, but I don't see anything in your makefile logic that would exclude these files on windows builds. Please see what the try server has to say?

@@ +17,5 @@
> +#include <sys/types.h>
> +
> +#include "base/message_loop.h"
> +#include "mozilla/FileUtils.h"
> +#include "mozilla/Observer.h"

Is this needed? Please try to only put add includes to header files that are absolutely necessary (usually only base classes and class members). Everything else should go to the cpp file.

@@ +28,5 @@
> +namespace mozilla {
> +namespace system {
> +
> +
> +#define MAX_COMMAND_SIZE  512

Nit, please only have a single newline above and then add a single newline below. Also, please move above the namespace stuff.

@@ +43,5 @@
> +
> +class NetdConsumer : public RefCounted<NetdConsumer>
> +{
> +public:
> +    virtual ~NetdConsumer() { }

Nit: here and below you've got some 4-space indents. Let's make it 2.

@@ +44,5 @@
> +class NetdConsumer : public RefCounted<NetdConsumer>
> +{
> +public:
> +    virtual ~NetdConsumer() { }
> +    virtual void MessageReceived(NetdCommand* aMessage) { }

Shouldn't this be pure virtual?

@@ +53,5 @@
> +    virtual void Run();
> +};
> +
> +class NetdClient : public MessageLoopForIO::Watcher,
> +                      public RefCounted<NetdClient>

Nit: please line up the 'public' statements

@@ +56,5 @@
> +class NetdClient : public MessageLoopForIO::Watcher,
> +                      public RefCounted<NetdClient>
> +{
> +public:
> +  typedef std::queue<NetdCommand*> NetdCommandQueue;

Nit: Newline below here.

@@ +60,5 @@
> +  typedef std::queue<NetdCommand*> NetdCommandQueue;
> +  NetdClient();
> +  virtual ~NetdClient();
> +  virtual void OnFileCanReadWithoutBlocking(int aFd);
> +  virtual void OnFileCanWriteWithoutBlocking(int aFd);

I'm a little unclear about when these are called. I see you calling the write function manually in the cpp file (is that safe?) but are they actually called from somewhere else?

@@ +64,5 @@
> +  virtual void OnFileCanWriteWithoutBlocking(int aFd);
> +  bool OpenSocket();
> +  void WriteCommandData();
> +  static void Start();
> +  ScopedClose mSocket;

All of these members should be private I think.

@@ +74,5 @@
> +  NetdCommandQueue mOutgoingQ;
> +  bool mBlockedOnWrite;
> +  nsAutoPtr<NetdCommand> mCurrentNetdCommand;
> +  size_t mCurrentWriteOffset;
> +  size_t mRcvIdx;

Nit: No need to abbreviate really, how about "mReceivedIndex"?

@@ +79,5 @@
> +};
> +
> +void StartNetd(NetdConsumer *);
> +void StopNetd();
> +bool SendNetdCommand(NetdCommand **);

Nit: newline after here.

@@ +81,5 @@
> +void StartNetd(NetdConsumer *);
> +void StopNetd();
> +bool SendNetdCommand(NetdCommand **);
> +} // system
> +} // mozilla

Nit: Please make this 'namespace mozilla' (same for 'system')

	Philipp von Weitershausen [:philikon]
	Comment 48 • 12 years ago

(In reply to Vincent Chang from comment #46)
> [scriptable, uuid(e232c530-cc9f-11e1-9b23-0800200c9a66)]
> interface nsISetWifiTetheringCallback : nsISupports
> {
>     /** 
>      * Report the setWifiTetheringEnabled result and pass
> nsINetworkInterface to caller.
>      */
>     void setWifiTetheringEnabledResult(in jsval network, in short result);
>     /** 
>      * Report the setWifiTetheringDisabled result and pass
> nsINetworkInterface to caller.
>      */
>     void setWifiTetheringDisabledResult(in short result);
> };

If you're passing nsINetworkInterface, you might as well declare it as such, instead of `jsval`. Also, I would just declare one method on the callback interface. Then we can declare the interface a [function] interface and just pass a normal JS function.

  [scriptable, function, uuid(...)]
  interface nsIWifiTetheringCallback : nsISupports
  {
      void wifiTetheringEnabledChange(in jsval error,
                                      in jsval nsINetworkInterface networkInterface);
  };

`error` is normally `null`, otherwise it could be a JavaScript `Error` object or something like that. `networkInterface` can be null as well when it's not of interest.

You can then use it like this:

  setWifiTethering(true, function onEnableChange(error, networkInterface) {
    ...
  });

> Yes, I would like to create XPCOM API to control mobile data connection.
> BTW, it should be in the same thread with NetworkManager right ?

Yes. All of XPCOM JS has to live on the main thread.

	Dave Hylands [:dhylands]
	Comment 49 • 12 years ago

(In reply to ben turner [:bent] from comment #47)
> Comment on attachment 637381 [details] [diff] [review]

I disagree with at least one of bent's comments, so I've put a few comments in.

And I think that some of this is similar to some of my code (in dom/system/gonk/VolumeManager.cpp),
so I'll make a few changes in VolumeManager.cpp (so thanks bent).

> @@ +24,5 @@
> > +static RefPtr<NetdClient> sNetdClient;
> > +static RefPtr<NetdConsumer> sNetdConsumer;
> > +/***************************************************************************/
> > +NetdClient::NetdClient()
> > +  : mSocket(-1)
> 
> Is there some INVALID_SOCKET define or something you could use here? If not
> please create one. "Magic" numeric constants are hard to read.

We could use ScopedClose::empty() which returns -1. The Scoped constructor actually
already initializes itself, so you could also just drop the mSocket initialization
altogether.

> @@ +47,5 @@
> > +    return false;
> > +  }
> > +  // add FD_CLOEXEC flag
> > +  int flags = fcntl(mSocket.get(), F_GETFD);
> > +  if (flags == -1) {
> 
> Hm, the docs I see say that this will always return current flags, so
> checking for -1 seems unnecessary.

The docs I read say that fcntl returns -1 on error (for all F_XXX). I'm using
http://linux.die.net/man/3/fcntl

Perusal of the bionic runtime library sources also confirms that fcntl returns
-1 for any return that sets errno.

> @@ +80,5 @@
> > +  char rcvBuf[MAX_COMMAND_SIZE];  
> > +
> > +  MOZ_ASSERT(fd == mSocket.get());
> > +  while (true) {
> > +    length = read(fd, &rcvBuf[mRcvIdx], MAX_COMMAND_SIZE - mRcvIdx);
> 
> Wait, what happens if you fill up your buffer? Looks like you'll just exit.

If you fill up the buffer then length will equal MAX_COMMAND_SIZE - mRcvIdx
and it will process the data which is received.

> @@ +96,5 @@
> > +      // At this point, assume that we can't actually access
> > +      // the socket anymore, and start a reconnect loop.
> > +      mReadWatcher.StopWatchingFileDescriptor();
> > +      mWriteWatcher.StopWatchingFileDescriptor();
> > +      close(mSocket.get());
> 
> Should you null out mSocket now? Then it would automatically call close on
> mSocket.

It should probably just call mSocket.dispose() which will do the close and
set the internal fd to -1

> @@ +107,5 @@
> > +        // We found a line terminator. Each line is formatted as an
> > +        // integer response code followed by the rest of the line.
> > +        // Fish out the response code.
> > +        char *endPtr;
> > +        int responseCode = strtol(rcvBuf, &endPtr, 10);
> 
> You're not checking for errors here.

To check for errors properly on strtol you need to set errno to 0 before 
calling strtol and check errno for being non-zero afterwards.


> @@ +109,5 @@
> > +        // Fish out the response code.
> > +        char *endPtr;
> > +        int responseCode = strtol(rcvBuf, &endPtr, 10);
> > +        if (*endPtr == ' ') {
> > +          endPtr++;
> 
> It seems possible that this could bump you to the null terminator which
> might change what you should do below.

In this case that's fine. The strings below will be empty. You definitely
don't want to advance endPtr beyond the null terminator.

> @@ +132,5 @@
> > +          // There is data in the receive buffer beyond the current line.
> > +          // Shift it down to the beginning.
> > +          memmove(&rcvBuf[0], &rcvBuf[mRcvIdx + 1], pos);
> > +        }
> > +        mRcvIdx = 0;
> 
> Wait, this isn't right if you just did a memmove. You need to set mRcvIndex
> to pos here.

Setting mRcvIdx to 0 is the right thing to do. mRcvIdx points to unparsed data.
The data from  &rcvBuf[mRcvIdx + 1] for pos bytes is unparsed data and it's
being moved to the beginning of the buffer (&rcvBuf[0]) so mRcvIdx needs to
be set to 0 so that the:

if (rcvBuf[mRcvIdx] == '\0') {

at the beginning of the while loop continues to scan the unparsed data.

> @@ +166,5 @@
> > +      if (written > 0) {
> > +        mCurrentWriteOffset += written;
> > +      }
> > +      if (written != write_amount) {
> > +        break;
> 
> I think it would make more sense to do this:
> 
>   if (written != write_amount) {
>     NS_WARNING(...);
>     break;
>   }
>   mCurrentWriteOffset += written;

write not writing the full amount of data should be considered as a normal
thing and isn't worth a warning. It just means that the pipe is full and 
you need to try again later.

> @@ +177,5 @@
> > +                  false,
> > +                  MessageLoopForIO::WATCH_WRITE,
> > +                  &mWriteWatcher,
> > +                  this);
> > +      return;
> 
> Hm, do you really want to always queue another write task? What about some
> catastrophic error (think something like disk full). Seems like we could be
> a little smarter here.

It should check for write returning < 0 and in that case not do the Watch.

> @@ +197,5 @@
> > +  if (!sNetdClient) {
> > +    return;
> > +  }
> > +  if (!sNetdClient->OpenSocket()) {
> > +    // Socket open failed, try again in a second.
> 
> Hm, and what happens if this fails a second time, or a third? We need some
> way to report an error out of here somehow if we can't get this socket open
> after a while, and we need to not retry forever.

OpenSocket should print the error. I would argue that it should retry forever,
as long as there is a delay between the retries. If netd died and doesn't
restart, you're going to need to reboot your phone anyways.

> @@ +60,5 @@
> > +  typedef std::queue<NetdCommand*> NetdCommandQueue;
> > +  NetdClient();
> > +  virtual ~NetdClient();
> > +  virtual void OnFileCanReadWithoutBlocking(int aFd);
> > +  virtual void OnFileCanWriteWithoutBlocking(int aFd);
> 
> I'm a little unclear about when these are called. I see you calling the
> write function manually in the cpp file (is that safe?) but are they
> actually called from somewhere else?

These are called as a result of calling WatchFileDescriptor when its ok to read or write
without blocking (as part of the MessageLoopForIO::Watcher class)

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 50 • 12 years ago

(In reply to Dave Hylands [:dhylands] from comment #49)

Thanks for your comments. I still have some concerns though:

> The docs I read say that fcntl returns -1 on error (for all F_XXX). I'm using
> http://linux.die.net/man/3/fcntl

Yeah, I was looking at the same thing. "The return value shall not be negative" was what I saw and it made me think that F_GETFD was somehow special in that it would always just return the right flags. I see how it also puts that under the success cases though, so I guess I'm wrong?

> If you fill up the buffer then length will equal MAX_COMMAND_SIZE - mRcvIdx
> and it will process the data which is received.

No, I was talking about what happens if you fill the buffer previously without hitting a null terminator. Then you'd loop back around and have this:

  length = read(fd, &rcvBuf[mRcvIdx], MAX_COMMAND_SIZE - mRcvIdx);

|MAX_COMMAND_SIZE - mRcvIdx| would be 0, so length should return 0, right? In which case you'd exit the loop I believe.

> Setting mRcvIdx to 0 is the right thing to do. mRcvIdx points to unparsed
> data.

Yes, but it is also the place where you start filling the buffer from your read call:

  length = read(fd, &rcvBuf[mRcvIdx], MAX_COMMAND_SIZE - mRcvIdx);

So I think you'll overwrite unparsed data on your next loop iteration?

> write not writing the full amount of data should be considered as a normal
> thing and isn't worth a warning. It just means that the pipe is full and 
> you need to try again later.

Ok, but I still think you should break first before incrementing mCurrentWriteOffset.

> OpenSocket should print the error. I would argue that it should retry
> forever,
> as long as there is a delay between the retries. If netd died and doesn't
> restart, you're going to need to reboot your phone anyways.

This would drain the battery, right? Maybe an adaptively increasing timer? Dunno, see if cjones cares?

	Dave Hylands [:dhylands]
	Comment 51 • 12 years ago

(In reply to ben turner [:bent] from comment #50)
> (In reply to Dave Hylands [:dhylands] from comment #49)
> 
> Thanks for your comments. I still have some concerns though:
> 
> > The docs I read say that fcntl returns -1 on error (for all F_XXX). I'm using
> > http://linux.die.net/man/3/fcntl
> 
> Yeah, I was looking at the same thing. "The return value shall not be
> negative" was what I saw and it made me think that F_GETFD was somehow
> special in that it would always just return the right flags. I see how it
> also puts that under the success cases though, so I guess I'm wrong?

So here's the code in the kernel that does the heavy lifting:
http://lxr.linux.no/linux+v3.4.4/fs/fcntl.c#L343

Any negative return value, gets passed up to userspace (the sys_fcntl call gets a
-ve return), and this ultimately goes through the __set_errno routine in bionic,
and the return value from that function (which is -1) is what gets returned by the
syscall (so fcntl in this example).

> > If you fill up the buffer then length will equal MAX_COMMAND_SIZE - mRcvIdx
> > and it will process the data which is received.
> 
> No, I was talking about what happens if you fill the buffer previously
> without hitting a null terminator. Then you'd loop back around and have this:
> 
>   length = read(fd, &rcvBuf[mRcvIdx], MAX_COMMAND_SIZE - mRcvIdx);
> 
> |MAX_COMMAND_SIZE - mRcvIdx| would be 0, so length should return 0, right?
> In which case you'd exit the loop I believe.

Yeah - I think this logic breaks if you your line is too long to fit into the buffer.
I picked a 1K buffer. You could use some dynamic resizing thing, but it seems like
quite a bit more complexity to deal with a situation which will never happen.

> > Setting mRcvIdx to 0 is the right thing to do. mRcvIdx points to unparsed
> > data.
> 
> Yes, but it is also the place where you start filling the buffer from your
> read call:
> 
>   length = read(fd, &rcvBuf[mRcvIdx], MAX_COMMAND_SIZE - mRcvIdx);
> 
> So I think you'll overwrite unparsed data on your next loop iteration?

No, you wind up advancing mRcvIdx to the end of the read data before issuing
another read.

there is a while (true) loop with the read, and after the read is a while (pos > 0) loop
which you could read as while there is unparsed data (so a while inside the while)

> > write not writing the full amount of data should be considered as a normal
> > thing and isn't worth a warning. It just means that the pipe is full and 
> > you need to try again later.
> 
> Ok, but I still think you should break first before incrementing
> mCurrentWriteOffset.

Yeah - I'd go further and say it should return if written < 0.

> > OpenSocket should print the error. I would argue that it should retry
> > forever,
> > as long as there is a delay between the retries. If netd died and doesn't
> > restart, you're going to need to reboot your phone anyways.
> 
> This would drain the battery, right? Maybe an adaptively increasing timer?
> Dunno, see if cjones cares?

I think that if the reconnect doesn't happen within a second or two, your phone is hooped anyways. We need a roll-over-and-play-dead command :)

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 52 • 12 years ago

(In reply to Dave Hylands [:dhylands] from comment #51)
> Yeah - I think this logic breaks if you your line is too long to fit into
> the buffer.
> I picked a 1K buffer. You could use some dynamic resizing thing, but it
> seems like
> quite a bit more complexity to deal with a situation which will never happen.

This particular buffer is 512 bytes. I don't know enough about netd to say for sure, but is it really not possible for that to be filled with the data from a single command? If it isn't then I'm happy just adding an assertion. If it can happen (or if we can't say for sure) then we need to handle this case.

> there is a while (true) loop with the read, and after the read is a while
> (pos > 0) loop
> which you could read as while there is unparsed data (so a while inside the
> while)

Hm... Foiled by nested loops. I see how this will work now though, thanks for explaining.

> Yeah - I'd go further and say it should return if written < 0.

Sounds good.

> I think that if the reconnect doesn't happen within a second or two, your
> phone is hooped anyways. We need a roll-over-and-play-dead command :)

cjones wants either a) exponential backoff, or b) failure after about 10 tries. I don't care which, so whatever is easier to implement :)

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 54 • 12 years ago

Added setWifiTethering function to support wifi tethering. 
The wifi manager should disable wifi before switch from station mode to ap mode. 
Also, if wifi driver is not loaded, wifi manager should help to load wifi driver before we can enable wifi tethering. 
We pass setWifiTetheringEnabledResult function and setWifiTetheringDisabledResult callback function defined in NetworkManager.js(tethering.implement.patch.V5) to help to support wifi tethering.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 55 • 12 years ago

(In reply to ben turner [:bent] from comment #47)
> Comment on attachment 637381 [details] [diff] [review]
> Updated IPC interface implementation.
> 
> Review of attachment 637381 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hm. The mixture of mfbt, stdlib, and chromium is a bit dizzying here. Not
> your fault!
> 
> Lots of cleanup needed here but also some more serious stuff.
> 
> ::: dom/system/gonk/SystemWorkerManager.cpp
> @@ +51,5 @@
> >  #ifdef MOZ_B2G_BT
> >  using namespace mozilla::dom::bluetooth;
> >  #endif
> >  
> > +
> 
> Nit: Please remove this extra line.
> 
> @@ +65,5 @@
> >  NS_DEFINE_CID(kRadioInterfaceLayerCID, NS_RADIOINTERFACELAYER_CID);
> >  NS_DEFINE_CID(kWifiWorkerCID, NS_WIFIWORKER_CID);
> > +NS_DEFINE_CID(kNetworkManagerCID, NS_NETWORKMANAGER_CID);
> > +
> > +
> 
> Nit: please remove these two extra lines.
> 
> @@ +195,5 @@
> >                               argv, argv);
> >  }
> >  
> > +JSBool
> > +doNetdCommand(JSContext *cx, unsigned argc, jsval *vp)
> 
> Nit: Functions should start with a capital letter.
> 
> @@ +206,5 @@
> > +  }
> > +
> > +  jsval v = JS_ARGV(cx, vp)[0];
> > +
> > +  nsAutoPtr<NetdCommand> rm(new NetdCommand());
> 
> What is "rm"? Please give this a better name.
> 
> @@ +247,5 @@
> > +    return false;
> > +  }
> > +
> > +  rm->mSize = size + 1;
> > +  memcpy(rm->mData, data, size);
> 
> This is weird. Were you intending to null-terminate the size? If so, you're
> not doing it. But I don't think you should anyway. Just lose the +1 there.
>
  Yes, it is used to include null-terminate. I would like to modify codes as below. 

  // Include the null terminate.
  if (size > MAX_COMMAND_SIZE - 1) {
    JS_ReportError(cx, "Passed-in data is too large");
    return false;
  }

  memcpy(command->mData, data, size);
  // Include the null terminate to the command to make netd happy. 
  command->mData[size] = '\0';
  command->mSize = size + 1;

 
> @@ +248,5 @@
> > +  }
> > +
> > +  rm->mSize = size + 1;
> > +  memcpy(rm->mData, data, size);
> > +  LOG("size = %d", size);
> 
> This isn't a very useful log message. Is it just for your debugging? Either
> improve or remove.
> 
> @@ +250,5 @@
> > +  rm->mSize = size + 1;
> > +  memcpy(rm->mData, data, size);
> > +  LOG("size = %d", size);
> > +  NetdCommand *tosend = rm.forget();
> > +  JS_ALWAYS_TRUE(SendNetdCommand(&tosend));
> 
> In general it's a bad idea to put function calls in macros (you don't know
> how many times the function will be called). Also, I proposed this later,
> but I think this will be nicer without doing the separate stack pointer and
> passing the address of it to SendNetdCommand.
   
  Sorry, I don't quite understand here. Is it like this ? 

  result = SendNetdCommand(&(command.forget()));
  if (result == false) {
    NS_WARNING("Failed to send command to netd!");
  }
  return result;
   
> 
> @@ +294,5 @@
> > +  { }
> > +
> > +  virtual void MessageReceived(NetdCommand *aMessage) {
> > +    nsRefPtr<DispatchNetdEvent> dre(new DispatchNetdEvent(aMessage));
> > +    mDispatcher->PostTask(dre);
> 
> This can fail, you should at least warn if it does.
> 
> @@ +525,5 @@
> > +SystemWorkerManager::InitNetd(JSContext *cx)
> > +{
> > +  nsCOMPtr<nsIWorkerHolder> worker = do_GetService(NETWORKMANAGER_CONTRACTID);
> > +  NS_ENSURE_TRUE(worker, NS_ERROR_FAILURE);
> > +  jsval workerval;
> 
> Nit: Newline above here.
> 
> @@ +527,5 @@
> > +  nsCOMPtr<nsIWorkerHolder> worker = do_GetService(NETWORKMANAGER_CONTRACTID);
> > +  NS_ENSURE_TRUE(worker, NS_ERROR_FAILURE);
> > +  jsval workerval;
> > +  nsresult rv = worker->GetWorker(&workerval);
> > +
> 
> Nit: remove this line.
> 
> @@ +534,5 @@
> > +
> > +  JSAutoRequest ar(cx);
> > +  JSAutoEnterCompartment ac;
> > +  if (!ac.enter(cx, JSVAL_TO_OBJECT(workerval))) {
> > +    return NS_ERROR_OUT_OF_MEMORY;
> 
> Nit: Add |NS_WARNING("Failed to enter compartment!");| here
> 
> @@ +536,5 @@
> > +  JSAutoEnterCompartment ac;
> > +  if (!ac.enter(cx, JSVAL_TO_OBJECT(workerval))) {
> > +    return NS_ERROR_OUT_OF_MEMORY;
> > +  }
> > +  WorkerCrossThreadDispatcher *wctd =
> 
> Nit: newline above here.
> 
> @@ +539,5 @@
> > +  }
> > +  WorkerCrossThreadDispatcher *wctd =
> > +    GetWorkerCrossThreadDispatcher(cx, workerval);
> > +  if (!wctd) {
> > +    return NS_ERROR_FAILURE;
> 
> Need a warning here too.
> 
> @@ +544,5 @@
> > +  }
> > +
> > +  nsRefPtr<ConnectWorkerToNetd> connection = new ConnectWorkerToNetd();
> > +  if (!wctd->PostTask(connection)) {
> > +    return NS_ERROR_UNEXPECTED;
> 
> And here.
> 
> ::: ipc/netd/Netd.cpp
> @@ +12,5 @@
> > +#include "nsXULAppAPI.h"
> > +
> > +#include "cutils/properties.h"
> > +#include "android/log.h"
> > +#include "Netd.h"
> 
> Please make Netd.h the very first include in this file. That way you can
> make sure that the header compiles on its own.
> 
> @@ +21,5 @@
> > +namespace mozilla {
> > +namespace system {
> > +
> > +static RefPtr<NetdClient> sNetdClient;
> > +static RefPtr<NetdConsumer> sNetdConsumer;
> 
> Why are you declaring these inside the mozilla::system namespace? They
> should just be globals in this file I believe (just make an anonymous
> namespace here).

  Or could I use namespace ipc { .... ? Because this code is located in ipc/netd...
  and remove namespace led to compiler error in SystemWorkerManager.cpp. 

> 
> @@ +22,5 @@
> > +namespace system {
> > +
> > +static RefPtr<NetdClient> sNetdClient;
> > +static RefPtr<NetdConsumer> sNetdConsumer;
> > +/***************************************************************************/
> 
> Nit: This doesn't seem to be very helpful.
> 
> @@ +23,5 @@
> > +
> > +static RefPtr<NetdClient> sNetdClient;
> > +static RefPtr<NetdConsumer> sNetdConsumer;
> > +/***************************************************************************/
> > +NetdClient::NetdClient()
> 
> You're not initializing mBlockedOnWrite or mCurrentWriteOffset, please do.
> 
> Also, please add the MOZ_COUNT_CTOR macro here.
> 
> @@ +24,5 @@
> > +static RefPtr<NetdClient> sNetdClient;
> > +static RefPtr<NetdConsumer> sNetdConsumer;
> > +/***************************************************************************/
> > +NetdClient::NetdClient()
> > +  : mSocket(-1)
> 
> Is there some INVALID_SOCKET define or something you could use here? If not
> please create one. "Magic" numeric constants are hard to read.
> 
> @@ +27,5 @@
> > +NetdClient::NetdClient()
> > +  : mSocket(-1)
> > +  , mIOLoop(MessageLoopForIO::current())
> > +  , mMutex("NetdClient.mMutex")
> > +  , mCurrentNetdCommand(NULL)
> 
> This is not needed, nsAutoPtr initializes to null automatically.

  I will add this. 
  #define INVALID_SOCKET -1
  NetdClient::NetdClient()
    : mSocket(INVALID_SOCKET)

> 
> @@ +32,5 @@
> > +  , mRcvIdx(0)
> > +{
> > +}
> > +
> > +NetdClient::~NetdClient()
> 
> Please add the MOZ_COUNT_DTOR macro here.
> 
> @@ +41,5 @@
> > +NetdClient::OpenSocket()
> > +{
> > +  if ((mSocket.rwget() = socket_local_client("netd",
> > +                                             ANDROID_SOCKET_NAMESPACE_RESERVED,
> > +                                             SOCK_STREAM)) < 0) {
> 
> Let's split this into two statements:
> 
>   mSocket.rwget() = ...;
>   if (mSocket.get() < 0) {
> 
> @@ +45,5 @@
> > +                                             SOCK_STREAM)) < 0) {
> > +    LOG("Error connecting to : netd (%s) - will retry", strerror(errno));
> > +    return false;
> > +  }
> > +  // add FD_CLOEXEC flag
> 
> Nit: Please make your comments real sentences (i.e. begin with capital
> letter, end with period). Here and everywhere.
> 
> @@ +47,5 @@
> > +    return false;
> > +  }
> > +  // add FD_CLOEXEC flag
> > +  int flags = fcntl(mSocket.get(), F_GETFD);
> > +  if (flags == -1) {
> 
> Hm, the docs I see say that this will always return current flags, so
> checking for -1 seems unnecessary.
  
  Could I keep this code based on Dave Hylands and your comments in Comment 49 ~ 52 ?
  
> 
> @@ +52,5 @@
> > +    return false;
> > +  }
> > +  flags |= FD_CLOEXEC;
> > +  if (fcntl(mSocket.get(), F_SETFD, flags) == -1) {
> > +    return false;
> 
> LOG failure here and in the other places where you exit early.
> 
> @@ +80,5 @@
> > +  char rcvBuf[MAX_COMMAND_SIZE];  
> > +
> > +  MOZ_ASSERT(fd == mSocket.get());
> > +  while (true) {
> > +    length = read(fd, &rcvBuf[mRcvIdx], MAX_COMMAND_SIZE - mRcvIdx);
> 
> Wait, what happens if you fill up your buffer? Looks like you'll just exit.
> 
> Also, you should at least assert that length <= MAX_COMMAND_SIZE - mRcvIdx.

  Basically, netd's response is less than 100 bytes for tethering commands. I will  check if others command may have longer response length or not. And of course assert if length <= MAX_COMMAND_SIZE.  
> 
> @@ +86,5 @@
> > +      if (length == -1) {
> > +        if (errno == EINTR) {
> > +          continue; // retry system call when interrupted
> > +        }   
> > +        else if (errno == EAGAIN || errno == EWOULDBLOCK) {
> 
> Nit: No need for 'else' here
> 
> @@ +96,5 @@
> > +      // At this point, assume that we can't actually access
> > +      // the socket anymore, and start a reconnect loop.
> > +      mReadWatcher.StopWatchingFileDescriptor();
> > +      mWriteWatcher.StopWatchingFileDescriptor();
> > +      close(mSocket.get());
> 
> Should you null out mSocket now? Then it would automatically call close on
> mSocket.

  I will use mSocket.dispose() instead, and try to re-establish a connection to netd here. 
> 
> @@ +99,5 @@
> > +      mWriteWatcher.StopWatchingFileDescriptor();
> > +      close(mSocket.get());
> > +      return;
> > +    } 
> > +    pos = length; 
> 
> Nit: newline above here.
> 
> @@ +101,5 @@
> > +      return;
> > +    } 
> > +    pos = length; 
> > +    while (pos > 0) {
> > +      pos--;
> 
> Nit: This could just be:
> 
>   while (pos--)
> 
> @@ +107,5 @@
> > +        // We found a line terminator. Each line is formatted as an
> > +        // integer response code followed by the rest of the line.
> > +        // Fish out the response code.
> > +        char *endPtr;
> > +        int responseCode = strtol(rcvBuf, &endPtr, 10);
> 
> You're not checking for errors here.
  
  I can modify it like 
  #include <errno.h>  

  errno = 0; 
  int responseCode = strtol(rcvBuf, &endPtr, 10);
  if (errno < 0 ) {
    do something ...
  }
> 
> @@ +109,5 @@
> > +        // Fish out the response code.
> > +        char *endPtr;
> > +        int responseCode = strtol(rcvBuf, &endPtr, 10);
> > +        if (*endPtr == ' ') {
> > +          endPtr++;
> 
> It seems possible that this could bump you to the null terminator which
> might change what you should do below.

  The format of netd's response is "200 tethering operation successfully"  
> 
> @@ +119,5 @@
> > +          // These are unsolicited broadcasts. We intercept these and process
> > +          // them ourselves
> > +          // todo process InterfaceChange(600) and BandwidthControl(601)
> > +          // skip this message  
> > +          ;
> 
> Nit: Rather than do this, put the comment outside the if block, then just
> check responseCode < 600 and lose the else block.
> 
> @@ +121,5 @@
> > +          // todo process InterfaceChange(600) and BandwidthControl(601)
> > +          // skip this message  
> > +          ;
> > +        } else {
> > +          mIncoming = new NetdCommand;
> 
> This member is a little useless. You only create it here, then immediately
> forget it below. How about just having this be a stack variable?
   
  Can we use stack variable here ? The mIncoming will be passed to net worker thread. 
> 
> @@ +123,5 @@
> > +          ;
> > +        } else {
> > +          mIncoming = new NetdCommand;
> > +          // Everything else is considered to be part of the command response.
> > +          mIncoming->mSize = length;
> 
> This isn't really right. The size is just up until the null terminator, and
> you're not guaranteed (as far as I can tell) that |rcvBuf + length| is
> always right. You want mRcvIdx I think.
> 
> @@ +124,5 @@
> > +        } else {
> > +          mIncoming = new NetdCommand;
> > +          // Everything else is considered to be part of the command response.
> > +          mIncoming->mSize = length;
> > +          memcpy(mIncoming->mData, rcvBuf, length); 
> 
> Yeah, you may be copying part of the next message here.
> 
> @@ +132,5 @@
> > +          // There is data in the receive buffer beyond the current line.
> > +          // Shift it down to the beginning.
> > +          memmove(&rcvBuf[0], &rcvBuf[mRcvIdx + 1], pos);
> > +        }
> > +        mRcvIdx = 0;
> 
> Wait, this isn't right if you just did a memmove. You need to set mRcvIndex
> to pos here.

Could I keep this read logic based on Commend 49 ~ 52 ? 
 
> @@ +143,5 @@
> > +
> > +void
> > +NetdClient::OnFileCanWriteWithoutBlocking(int fd)
> > +{
> > +  MOZ_ASSERT(aFd == mSocket.get());
> 
> Um, this won't compile. Have you tried a debug build yet?

  Sorry, it's typo. I will correct it. 
 
> @@ +145,5 @@
> > +NetdClient::OnFileCanWriteWithoutBlocking(int fd)
> > +{
> > +  MOZ_ASSERT(aFd == mSocket.get());
> > +
> > +  while (!mOutgoingQ.empty() || mCurrentNetdCommand != NULL) {
> 
> mCurrentNetdCommand is also kind of a useless member (it's only used in this
> function, and you both set and unset it here). Just make this a stack
> variable.

> 
> @@ +151,5 @@
> > +      MutexAutoLock lock(mMutex);
> > +
> > +      mCurrentNetdCommand = mOutgoingQ.front();
> > +      mOutgoingQ.pop();
> > +      mCurrentWriteOffset = 0;
> 
> mCurrentWriteOffset is only used here too. Stack var please.
> 

  Could we use stack variable here, the mCurrentNetdCommand will be passed to I/O thread ? 

> @@ +161,5 @@
> > +
> > +    while (mCurrentWriteOffset < mCurrentNetdCommand->mSize) {
> > +      ssize_t write_amount = mCurrentNetdCommand->mSize - mCurrentWriteOffset;
> > +      ssize_t written;
> > +      written = write (fd, toWrite + mCurrentWriteOffset, write_amount);
> 
> Nit: ssize_t written = write(...)
> 
> @@ +166,5 @@
> > +      if (written > 0) {
> > +        mCurrentWriteOffset += written;
> > +      }
> > +      if (written != write_amount) {
> > +        break;
> 
> I think it would make more sense to do this:
> 
>   if (written != write_amount) {
>     NS_WARNING(...);
>     break;
>   }
>   mCurrentWriteOffset += written;
> 
> @@ +177,5 @@
> > +                  false,
> > +                  MessageLoopForIO::WATCH_WRITE,
> > +                  &mWriteWatcher,
> > +                  this);
> > +      return;
> 
> Hm, do you really want to always queue another write task? What about some
> catastrophic error (think something like disk full). Seems like we could be
> a little smarter here.

  I will check the case when write return < 0. And also when write return EINTR
 
> 
> @@ +184,5 @@
> > +  }
> > +}
> > +
> > +void NetdWriteTask::Run() {
> > +    sNetdClient->OnFileCanWriteWithoutBlocking(sNetdClient->mSocket.rwget());
> 
> You have a null check below for sNetdClient... Do you need one here? In
> general I like dealing with assertable invariants rather than guesswork, so
> I'd prefer if we could do without the null checks.
> 
> Nit: return type on its own line, and then { on its own line, and then
> 2-space indent instead of 4.
> 
> @@ +187,5 @@
> > +void NetdWriteTask::Run() {
> > +    sNetdClient->OnFileCanWriteWithoutBlocking(sNetdClient->mSocket.rwget());
> > +}
> > +
> > +//static
> 
> Nit: space after //
> 
> @@ +194,5 @@
> > +{
> > +  MOZ_ASSERT(MessageLoop::current() == XRE_GetIOMessageLoop());
> > +
> > +  if (!sNetdClient) {
> > +    return;
> 
> You want some kind of warning here right? Maybe even an assertion.
> 
> @@ +197,5 @@
> > +  if (!sNetdClient) {
> > +    return;
> > +  }
> > +  if (!sNetdClient->OpenSocket()) {
> > +    // Socket open failed, try again in a second.
> 
> Hm, and what happens if this fails a second time, or a third? We need some
> way to report an error out of here somehow if we can't get this socket open
> after a while, and we need to not retry forever.

  I will add "retry 10 times than give up" here. 
 
> @@ +211,5 @@
> > +InitRndisAddress()
> > +{
> > +  char mac[20];
> > +  char serialno[] = "1234567890ABCDEF";
> > +  int ETH_ALEN = 6;
> 
> static const int, and then please name it with lowercase letters (all caps
> are for macros). Also, no need to be so terse, let's call it
> "kEthernetAddressLength" or something?
> 
> @@ +225,5 @@
> > +  }
> > +  property_get("ro.serialno", serialno, "1234567890ABCDEF");
> > +  // first byte is 0x02 to signify a locally administered address
> > +  address[0] = 0x02;
> > +  for (i = 0; i < strlen(serialno); i++) {
> 
> You're calling strlen in a loop here. Just call it once above.

  ok 

> 
> @@ +233,5 @@
> > +    address[0], address[1], address[2], address[3], address[4], address[5]);
> > +  length = strlen(mac); 
> > +  ret = write(fd, mac, length); 
> > +  if (ret != length)
> > +    LOG("Fail to write file %s.", ICS_SYS_USB_RNDIS_MAC);
> 
> Nit: Please brace all single-line if statements
> 
> @@ +244,5 @@
> > +  MOZ_ASSERT(MessageLoop::current() == XRE_GetIOMessageLoop());
> > +  MOZ_ASSERT(!sNetdClient);
> > +
> > +  InitRndisAddress();
> > +  sNetdClient = new NetdClient();
> 
> Hm, is there any reason to do this if InitRndisAddress failed? Of course,
> since it's a void function, you can't tell if it failed anyway. I think you
> should make all of these functions that can possibly fail have a bool return
> value so that you can propagate and check error conditions better.

  Ok, I will modify it. 
> 
> @@ +257,5 @@
> > +  sNetdClient = NULL;
> > +}
> > +
> > +//-----------------------------------------------------------------------------
> > +// This code runs on any thread.
> 
> This is almost always bad. We should design this and then be able to ensure
> that each function happens only on one thread.

  Actually, these codes run on main thread, should I add NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); here ? 

> 
> @@ +281,5 @@
> > +
> > +bool
> > +SendNetdCommand(NetdCommand** aMessage)
> > +{
> > +  NetdCommand *msg = *aMessage;
> 
> You should assert that aMessage and *aMessage are both non-null.
> 
> Actually, why is this a NetdCommand** ? You're just trying to convey the
> transfer of ownership here? In-out params are awkward to use imo, I think a
> straight pointer is better.
  
  ok, I will check it. 
> 
> @@ +283,5 @@
> > +SendNetdCommand(NetdCommand** aMessage)
> > +{
> > +  NetdCommand *msg = *aMessage;
> > +
> > +  if (!sNetdClient) {
> 
> This isn't safe. You're accessing this variable on some random thread but it
> is set and unset on the IO thread. Is there any higher-level logic that
> prevents a race here?

  StartNetd should be called in very early stage before SendNetdCommand could be called
  by net_worker. But let me think if any logic can help to prevent a race condtion.   
> 
> @@ +288,5 @@
> > +    return false;
> > +  }
> > +  *aMessage = nsnull;
> > +  {
> > +    MutexAutoLock lock(sNetdClient->mMutex);
> 
> So if you block here waiting on the mutex what prevents the other thread
> from destroying sNetdClient while you're waiting?
> 
> @@ +289,5 @@
> > +  }
> > +  *aMessage = nsnull;
> > +  {
> > +    MutexAutoLock lock(sNetdClient->mMutex);
> > +    sNetdClient->mOutgoingQ.push(msg);
> 
> In general locking and member manipulation should be contained in a method.
> Rather than digging out the mutex and queue here you should just make a
> method on NetdClient like Push(NetdCommand*) that does all this internally.

  Ok, I will try to do it. 
> 
> ::: ipc/netd/Netd.h
> @@ +13,5 @@
> > +
> > +#include <sys/socket.h>
> > +#include <sys/un.h>
> > +#include <sys/select.h>
> > +#include <sys/types.h>
> 
> Wait, does this build on windows? I understand that we don't care about
> windows here, but I don't see anything in your makefile logic that would
> exclude these files on windows builds. Please see what the try server has to
> say?

  I will add MOZ_B2G_NETD in Makefile. 
> 
> @@ +17,5 @@
> > +#include <sys/types.h>
> > +
> > +#include "base/message_loop.h"
> > +#include "mozilla/FileUtils.h"
> > +#include "mozilla/Observer.h"
> 
> Is this needed? Please try to only put add includes to header files that are
> absolutely necessary (usually only base classes and class members).
> Everything else should go to the cpp file.

  Ok, I will check it. 
> 
> @@ +28,5 @@
> > +namespace mozilla {
> > +namespace system {
> > +
> > +
> > +#define MAX_COMMAND_SIZE  512
> 
> Nit, please only have a single newline above and then add a single newline
> below. Also, please move above the namespace stuff.
> 
> @@ +43,5 @@
> > +
> > +class NetdConsumer : public RefCounted<NetdConsumer>
> > +{
> > +public:
> > +    virtual ~NetdConsumer() { }
> 
> Nit: here and below you've got some 4-space indents. Let's make it 2.
> 
> @@ +44,5 @@
> > +class NetdConsumer : public RefCounted<NetdConsumer>
> > +{
> > +public:
> > +    virtual ~NetdConsumer() { }
> > +    virtual void MessageReceived(NetdCommand* aMessage) { }
> 
> Shouldn't this be pure virtual?
> 
> @@ +53,5 @@
> > +    virtual void Run();
> > +};
> > +
> > +class NetdClient : public MessageLoopForIO::Watcher,
> > +                      public RefCounted<NetdClient>
> 
> Nit: please line up the 'public' statements
> 
> @@ +56,5 @@
> > +class NetdClient : public MessageLoopForIO::Watcher,
> > +                      public RefCounted<NetdClient>
> > +{
> > +public:
> > +  typedef std::queue<NetdCommand*> NetdCommandQueue;
> 
> Nit: Newline below here.
> 
> @@ +60,5 @@
> > +  typedef std::queue<NetdCommand*> NetdCommandQueue;
> > +  NetdClient();
> > +  virtual ~NetdClient();
> > +  virtual void OnFileCanReadWithoutBlocking(int aFd);
> > +  virtual void OnFileCanWriteWithoutBlocking(int aFd);
> 
> I'm a little unclear about when these are called. I see you calling the
> write function manually in the cpp file (is that safe?) but are they
> actually called from somewhere else?
> 
> @@ +64,5 @@
> > +  virtual void OnFileCanWriteWithoutBlocking(int aFd);
> > +  bool OpenSocket();
> > +  void WriteCommandData();
> > +  static void Start();
> > +  ScopedClose mSocket;
> 
> All of these members should be private I think.
> 
> @@ +74,5 @@
> > +  NetdCommandQueue mOutgoingQ;
> > +  bool mBlockedOnWrite;
> > +  nsAutoPtr<NetdCommand> mCurrentNetdCommand;
> > +  size_t mCurrentWriteOffset;
> > +  size_t mRcvIdx;
> 
> Nit: No need to abbreviate really, how about "mReceivedIndex"?
> 
> @@ +79,5 @@
> > +};
> > +
> > +void StartNetd(NetdConsumer *);
> > +void StopNetd();
> > +bool SendNetdCommand(NetdCommand **);
> 
> Nit: newline after here.
> 
> @@ +81,5 @@
> > +void StartNetd(NetdConsumer *);
> > +void StopNetd();
> > +bool SendNetdCommand(NetdCommand **);
> > +} // system
> > +} // mozilla
> 
> Nit: Please make this 'namespace mozilla' (same for 'system')

	Philipp von Weitershausen [:philikon]
	Comment 56 • 12 years ago

Vincent, please don't quote review comments in their entirety if you're only going to respond to a few passages. Please get rid of the parts you're not replying to, it makes everything much more readable.

	Philipp von Weitershausen [:philikon]
	Comment 57 • 12 years ago

Comment on attachment 642239 [details] [diff] [review]
Updated tethering implementation V5

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

Vincent, you still haven't addressed some points I've mentioned several times before. I'm going to cancel review on this patch, please address them first.

::: dom/system/gonk/NetworkManager.js
@@ +29,5 @@
> +const TOPIC_MOZSETTINGS_CHANGED = "mozsettings-changed";
> +const TOPIC_XPCOM_SHUTDOWN = "xpcom-shutdown";
> +
> +const DEFAULT_DNS_FORWORDER1_IP = "8.8.8.8";
> +const DEFAULT_DNS_FORWORDER2_IP = "8.8.4.4";

s/FORWORDER/FORWARDER/, though I'm not sure "forwarder" is a word. I think you can just get rid of that word and call it DEFAULT_DNS1.

All that said, I still disagree with these default constants, as I have expressed several times in this bug already. All this should live in settings.

::: dom/system/gonk/net_worker.js
@@ +8,4 @@
>  
>  importScripts("systemlibs.js");
>  
> +let gCurStateMachine;

You still haven't gotten rid of the statemachine stuff. I'll take a close look at the patch once you have. For now I've just glanced at it.

@@ +371,5 @@
>    let dhcp = libnetutils.dhcp_do_request(options.ifname);
>    dhcp.ifname = options.ifname;
>    dhcp.oldIfname = options.oldIfname;
>  
> +  // TODO this could be race-y... by the time we've finished the DHCP request

Unrelated change. Please avoid.

@@ +424,5 @@
> +function setInterfaceUp(options) {
> +  let response;
> +  let command = "interface setcfg";
> +  let space = " ";
> +  let e;

`response` and `e` are unused in most (all?) of these functions. Please remove all dead code.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 58 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #56)
> Vincent, please don't quote review comments in their entirety if you're only
> going to respond to a few passages. Please get rid of the parts you're not
> replying to, it makes everything much more readable.

Thank you for your reminding. I will pay more attention to it next time.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 59 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #57)
> Comment on attachment 642239 [details] [diff] [review]
> Updated tethering implementation V5
> 
> Review of attachment 642239 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Vincent, you still haven't addressed some points I've mentioned several
> times before. I'm going to cancel review on this patch, please address them
> first.
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +29,5 @@
> > +const TOPIC_MOZSETTINGS_CHANGED = "mozsettings-changed";
> > +const TOPIC_XPCOM_SHUTDOWN = "xpcom-shutdown";
> > +
> > +const DEFAULT_DNS_FORWORDER1_IP = "8.8.8.8";
> > +const DEFAULT_DNS_FORWORDER2_IP = "8.8.4.4";
> 
> s/FORWORDER/FORWARDER/, though I'm not sure "forwarder" is a word. I think
> you can just get rid of that word and call it DEFAULT_DNS1.
> 
> All that said, I still disagree with these default constants, as I have
> expressed several times in this bug already. All this should live in
> settings.

  We use the dnsmasq process to help to forward dns query to dns server. So I added the keyword forwarder. I would like to use getIFProperties(ifname) function to get dns server's ip address automatically and remove the default value. For dhcp server's start ip address and end ip address, do we need to allow user to set it ? if the answer is yes, than I can add the settings below.  

"tethering.wifi.dhcpserver.startip"      
"tethering.wifi.dhcpserver.endip"      
"tethering.usb.dhcpserver.startip"      
"tethering.usb.dhcpserver.endip"
        
> 
> ::: dom/system/gonk/net_worker.js
> @@ +8,4 @@
> >  
> >  importScripts("systemlibs.js");
> >  
> > +let gCurStateMachine;
> 
> You still haven't gotten rid of the statemachine stuff. I'll take a close
> look at the patch once you have. For now I've just glanced at it.

  I implement the state machine stuff for two or three days and it looks cool. That's why I still keep them at this patch. But it seems make the codes more difficult to read. So I will modify it as below in next patch.   
    setIpForwardingEnabled(function () {
      stopTethering(function () {
        untetherInterface(function() {
          usbTetheringSuccess();
        });
      });
    });

	Blake Kaplan (:mrbkap) (inactive)
	Comment 60 • 12 years ago

Comment on attachment 642242 [details] [diff] [review]
Added setWifiTethering function in nsIWifi interface V1

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

::: dom/wifi/WifiWorker.js
@@ +925,5 @@
>  
> +  // Reload driver and terminate wpa_supplicant before enabling softAP.
> +  manager.setWifiApEnabled = function(enable, callback) {
> +    if ((enable && manager.state !== "UNINITIALIZED") ||
> +        (!enable && manager.state === "UNINITIALIZED")) {

This is based off of an old version of this function, you're going to have to rebase... That being said...

@@ +930,5 @@
> +      callback(-1, null);
> +      return;
> +    }
> +
> +    if (enable) {

Please factor the common parts of this and setWifiEnabled into a common function, that way we avoid duplicating what is likely the ugliest function in the file.

@@ +1880,5 @@
> +
> +  setWifiTethering: function(enabled, callback) {
> +    debug("Requesting Wifi Tethering from NetworkManager " + enabled);
> +    if (enabled) {
> +      if (WifiManager.state != "UNINITIALIZED") {

This can now use .enabled.

@@ +1884,5 @@
> +      if (WifiManager.state != "UNINITIALIZED") {
> +        // Remove wifi driver and wpa_supplicant before enabling tethering.
> +        WifiManager.setWifiEnabled(false, (function (status) {
> +          if (status === 0) {
> +            this.setWifiApEnabled(enabled, callback.wifiTetheringEnabledChange);

A couple of questions to answer here:
  - Can we avoid doing some of the work (unloading and loading the driver, in particular) here?
  - The callback here can fire before wpa_supplicant is done shutting down, is that OK? If not, we'll probably need to add something to _stateRequests and wait for that to fire. If not, this might be fine.

@@ +1891,5 @@
> +      } else {
> +        this.setWifiApEnabled(enabled, callback.wifiTetheringEnabledChange);
> +      }
> +    } else {
> +      this.setWifiApEnabled(enabled, callback.wifiTetheringEnabledChange);

Is it possible to avoid duplicating this line?

::: dom/wifi/nsIWifi.idl
@@ +25,5 @@
>      void shutdown();
> +    /**
> +     * Request to enable/disable Wifi Tethering.
> +     */
> +    void setWifiTethering(in boolean enabled, in nsIWifiTetheringCallback callback);

Are you sure you want to add this to nsIWifi? It seems unlikely. Who is supposed to call this function? Also, no matter where this ends up, make sure to change the uuid of the interface that you're adding the function to.

	Philipp von Weitershausen [:philikon]
	Comment 61 • 12 years ago

(In reply to Blake Kaplan (:mrbkap) from comment #60)
> > +  // Reload driver and terminate wpa_supplicant before enabling softAP.
> > +  manager.setWifiApEnabled = function(enable, callback) {
> > +    if ((enable && manager.state !== "UNINITIALIZED") ||
> > +        (!enable && manager.state === "UNINITIALIZED")) {
> 
> This is based off of an old version of this function, you're going to have
> to rebase...

Vincent, please make sure you work against mozilla-central tip (Mercurial)!

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 62 • 12 years ago

(In reply to Blake Kaplan (:mrbkap) from comment #60)
> Comment on attachment 642242 [details] [diff] [review]
> Added setWifiTethering function in nsIWifi interface V1
> 
> Review of attachment 642242 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/wifi/WifiWorker.js
> @@ +925,5 @@
> >  
> > +  // Reload driver and terminate wpa_supplicant before enabling softAP.
> > +  manager.setWifiApEnabled = function(enable, callback) {
> > +    if ((enable && manager.state !== "UNINITIALIZED") ||
> > +        (!enable && manager.state === "UNINITIALIZED")) {
> 
> This is based off of an old version of this function, you're going to have
> to rebase... That being said...
>
  Sorry, my version is too old, I will prevent it on next patch.  

> @@ +930,5 @@
> > +      callback(-1, null);
> > +      return;
> > +    }
> > +
> > +    if (enable) {
> 
> Please factor the common parts of this and setWifiEnabled into a common
> function, that way we avoid duplicating what is likely the ugliest function
> in the file.
  1. Should we add one more argument "mode" to indicate AP or Station mode. 
  2. If we use the name setWifiEnabled to enable AP mode, is it confusing ?    
  3. Is it possible that we need to load different driver for AP or STATION mode. As I know, 
most of driver should support both AP or Station mode at the same time.
> 
> @@ +1880,5 @@
> > +
> > +  setWifiTethering: function(enabled, callback) {
> > +    debug("Requesting Wifi Tethering from NetworkManager " + enabled);
> > +    if (enabled) {
> > +      if (WifiManager.state != "UNINITIALIZED") {
> 
> This can now use .enabled.

Cool~~~

> 
> @@ +1884,5 @@
> > +      if (WifiManager.state != "UNINITIALIZED") {
> > +        // Remove wifi driver and wpa_supplicant before enabling tethering.
> > +        WifiManager.setWifiEnabled(false, (function (status) {
> > +          if (status === 0) {
> > +            this.setWifiApEnabled(enabled, callback.wifiTetheringEnabledChange);
> 
> A couple of questions to answer here:
>   - Can we avoid doing some of the work (unloading and loading the driver,
> in particular) here?

If we don't unload the driver, I found that beacon is still sent by our hotspot. Therefore, stations still can detect our hotspot. But I am not sure if we could avoid it in others way. Have you decided not to unload driver when disable wifi or it's just for otoro ?   

>   - The callback here can fire before wpa_supplicant is done shutting down,
> is that OK? If not, we'll probably need to add something to _stateRequests
> and wait for that to fire. If not, this might be fine.

  It should be fine. Tethering doesn't depend on wpa_supplicant. But wpa_supplicant may generate some wrong messsage when switch to AP mode.  
> 
> @@ +1891,5 @@
> > +      } else {
> > +        this.setWifiApEnabled(enabled, callback.wifiTetheringEnabledChange);
> > +      }
> > +    } else {
> > +      this.setWifiApEnabled(enabled, callback.wifiTetheringEnabledChange);
> 
> Is it possible to avoid duplicating this line?

  I will modify it. But I have a question here. We can only support AP mode or Station mode at one time. So we may need to turn off wifi tethering when enable wifi connection. Could I have your comment about how to do it gracefully ?  

> ::: dom/wifi/nsIWifi.idl
> @@ +25,5 @@
> >      void shutdown();
> > +    /**
> > +     * Request to enable/disable Wifi Tethering.
> > +     */
> > +    void setWifiTethering(in boolean enabled, in nsIWifiTetheringCallback callback);
> 
> Are you sure you want to add this to nsIWifi? It seems unlikely. Who is
> supposed to call this function? Also, no matter where this ends up, make
> sure to change the uuid of the interface that you're adding the function to.

Currently, when detect settings change in NetworkManager, NetworkManager will try to enable/disable wifi tethering by sending commands to netd. Before that, we may need to switch to AP mode and do some clean up in WifiWorker. And WifiWorker can use nsIWifiTetheringCallback to notify NetworkManager and move to next step.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 63 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #61)
> (In reply to Blake Kaplan (:mrbkap) from comment #60)
> > > +  // Reload driver and terminate wpa_supplicant before enabling softAP.
> > > +  manager.setWifiApEnabled = function(enable, callback) {
> > > +    if ((enable && manager.state !== "UNINITIALIZED") ||
> > > +        (!enable && manager.state === "UNINITIALIZED")) {
> > 
> > This is based off of an old version of this function, you're going to have
> > to rebase...
> 
> Vincent, please make sure you work against mozilla-central tip (Mercurial)!

Sorry, I will take care of it on next time.

	Philipp von Weitershausen [:philikon]
	Comment 65 • 12 years ago

Comment on attachment 644706 [details] [diff] [review]
Updated tethering implementation V6

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

::: dom/system/gonk/NetworkManager.js
@@ +581,5 @@
> +    }
> +
> +    // Disable tethering settings when fail to enable it.     
> +    if (result == TETHERING_COMMAND_FAIL) {
> +      let object = (obj === null ? this:obj);

I don't understand what the point of `object` / `obj` is.

Also, some nits:

* space around all operators (in this case `:`)
* you can test for just `obj`, no need to test for `=== null`.

@@ +582,5 @@
> +
> +    // Disable tethering settings when fail to enable it.     
> +    if (result == TETHERING_COMMAND_FAIL) {
> +      let object = (obj === null ? this:obj);
> +      object._wifitetheringState = false; 

Nit: please do camelCasing correctly: _wifiTetheringState

::: dom/system/gonk/net_worker.js
@@ +140,5 @@
>    let dhcp = libnetutils.dhcp_do_request(options.ifname);
>    dhcp.ifname = options.ifname;
>    dhcp.oldIfname = options.oldIfname;
>  
> +  // TODO this could be race-y... by the time we've finished the DHCP request

Unrelated change. Please remove.

@@ +155,5 @@
> +function onNetdMessage(data) {
> +  let response = "";
> +  this.incomingBuffer = new ArrayBuffer(512);
> +  this.incomingBytes = new Uint8Array(this.incomingBuffer);
> +  this.incomingBytes.set(data, 0);

Please const the 512 at the top of the file.

Why are you storing these on `this`? For ordinary functions, `this` will be the global scope. Why aren't these just local variables?

Also, it's now possible to instantiate a `Uint8Array` without having to create an `ArrayBuffer` first.

@@ +159,5 @@
> +  this.incomingBytes.set(data, 0);
> +
> +  for (let i = 0; this.incomingBytes[i] != 0 || i < data.length; i++) {
> +    response += String.fromCharCode(this.incomingBytes[i]);
> +  }

What's the purpose of the `Uint8Array` if all you're doing is reading from it? Why not just read from `data`?

@@ +184,5 @@
> + */
> +function doCommand(command, callback) {
> +  try {
> +    if (callback) {
> +      controlCallbacks = callback;

This will make your code prone to race conditions if you ever dispatch one command before the other has returned.

The variable name is already plural... it seems at some point there was command id -> callback mapping here. Please restore it.

@@ +192,5 @@
> +  catch (e) {
> +    dump(e);
> +    return false;
> +  }
> +  return false;

The `return false` inside the `catch` block is superfluous.

@@ +350,5 @@
> + */
> +function sleep(delay) {
> +  let start = new Date().getTime();
> +  while (new Date().getTime() < start + delay);
> +}

WAT.

@@ +368,5 @@
> +    if (result == usbfunc) {
> +      debug("setUSBFunction: ok");
> +      return true;
> +    }
> +    sleep(50);

Please use setTimeout(). This means your function chains may be come asynchronous (callback-driven). But this should only be a small change to `chain()`.

@@ +457,5 @@
> +                         setIpForwardingEnabled,
> +                         stopTethering,
> +                         wifiTetheringSuccess];
> +
> +    chain(params, functionChain, wifiTetheringFail);

You're repeating the call to `chain`, please pull it out.

Also, you can define the function chains just once, as global variables, right? E.g.

  const DISABLE_WIFI_TETHERING_CHAIN = [
    stopSoftAP,
    stopAccessPointDriver,
    wifiFirmwareReload,
    disableNat,
    untetherInterface,
    setIpForwardingEnabled,
    stopTethering,
    wifiTetheringSuccess
  ];

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 67 • 12 years ago

(In reply to Philipp von Weitershausen [:philikon] from comment #65)
> Comment on attachment 644706 [details] [diff] [review]
> Updated tethering implementation V6
> 
> Review of attachment 644706 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +581,5 @@
> > +    }
> > +
> > +    // Disable tethering settings when fail to enable it.     
> > +    if (result == TETHERING_COMMAND_FAIL) {
> > +      let object = (obj === null ? this:obj);
> 
> I don't understand what the point of `object` / `obj` is.
  
This function will be used in case of  
1. Callback function when net_worker postMessage to NetworkManager or 
2. Directly called in NetworkManager.
When obj is not null, it indicates that we have requested wifiWorker to switch to AP mode. So we may have to switch it back. It is not straight forward. So I will modify it in next version. Maybe add one parameters in data.rollback. 
 
> @@ +155,5 @@
> > +function onNetdMessage(data) {
> > +  let response = "";
> > +  this.incomingBuffer = new ArrayBuffer(512);
> > +  this.incomingBytes = new Uint8Array(this.incomingBuffer);
> > +  this.incomingBytes.set(data, 0);
> 
> Please const the 512 at the top of the file.
> 
> Why are you storing these on `this`? For ordinary functions, `this` will be
> the global scope. Why aren't these just local variables?

  I will correct it. 
> 
> Also, it's now possible to instantiate a `Uint8Array` without having to
> create an `ArrayBuffer` first.
> 
> @@ +159,5 @@
> > +  this.incomingBytes.set(data, 0);
> > +
> > +  for (let i = 0; this.incomingBytes[i] != 0 || i < data.length; i++) {
> > +    response += String.fromCharCode(this.incomingBytes[i]);
> > +  }
> 
> What's the purpose of the `Uint8Array` if all you're doing is reading from
> it? Why not just read from `data`?

  I will modify it. 
> 
> @@ +184,5 @@
> > + */
> > +function doCommand(command, callback) {
> > +  try {
> > +    if (callback) {
> > +      controlCallbacks = callback;
> 
> This will make your code prone to race conditions if you ever dispatch one
> command before the other has returned.
> 
> The variable name is already plural... it seems at some point there was
> command id -> callback mapping here. Please restore it.

  I would like to use pending flag to indicate that one command has been sent to netd, but the response is not back. Push the following commands to stack and wait until we receive ack for previous command.  

> 
> @@ +350,5 @@
> > + */
> > +function sleep(delay) {
> > +  let start = new Date().getTime();
> > +  while (new Date().getTime() < start + delay);
> > +}
> 
> WAT.
> 
> @@ +368,5 @@
> > +    if (result == usbfunc) {
> > +      debug("setUSBFunction: ok");
> > +      return true;
> > +    }
> > +    sleep(50);
> 
> Please use setTimeout(). This means your function chains may be come
> asynchronous (callback-driven). But this should only be a small change to
> `chain()`.

  I will modify it.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 69 • 12 years ago

1. Added r=bent in commit message. 
2. Replace JS_GetArrayBufferViewData to JS_GetUint8ArrayData function in NetdReceiver::DispatchNetdEvent::RunTask to make it easy to handle response message in net_worker.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 70 • 12 years ago

1. Updated the patch and added r=philikon

	Philipp von Weitershausen [:philikon]
	Comment 71 • 12 years ago

(In reply to Vincent Chang from comment #67)
> > > +function doCommand(command, callback) {
> > > +  try {
> > > +    if (callback) {
> > > +      controlCallbacks = callback;
> > 
> > This will make your code prone to race conditions if you ever dispatch one
> > command before the other has returned.
> > 
> > The variable name is already plural... it seems at some point there was
> > command id -> callback mapping here. Please restore it.
> 
> I would like to use pending flag to indicate that one command has been
> sent to netd, but the response is not back. Push the following commands to
> stack and wait until we receive ack for previous command.  

What stack? If you use a simple hash map for the callbacks, the event queue will do all that for you.

	Albert [:albert]
	Comment 72 • 12 years ago

As I'm using your patch for bug 746069 (network usage stats) to connect to netd asynchronously I would like to give some comments:

::: dom/system/gonk/NetworkManager.js
@@ -13,70 +13,121 @@ const NETWORKMANAGER_CONTRACTID = "@mozi
+// TODO, get USB RNDIS interface name automatically.(see Bug 776212)
+const DEFAULT_USB_INTERFACE_NAME  = "rndis0";
+const DEFAULT_3G_INTERFACE_NAME   = "rmnet0";
+const DEFAULT_WIFI_INTERFACE_NAME = "wlan0";

Is a good way to set 'wlan0' and 'rmnet0' as constants? I also need to get this interfaces names before the interface is registered to get the stats, but to ensure this interfaces names are correct I'm getting them from NetworkManager. Will be easier to set them as a constants as well.

Otherwise, these params can be obtained from 'wifi.interface' and 'mobiledata.interfaces'. The problem is that 'mobiledata.interfaces' is added in ICS and it is not giving the correct interfaces. If this can be corrected things will be easier.


+  // Enable/disable USB tethering by sending commands to netd.
+  setUSBTethering: function setUSBTethering(enable, tetheringinterface) {
+    let params = this.getTetheringParameters(TETHERING_TYPE_USB, enable, tetheringinterface);
+
+    if (params === null) {
+      this.usbTetheringResultReport(this, {result: TETHERING_COMMAND_FAIL, enable: enable});
+      this.setUSBFunction(USB_FUNCTION_ADB, null);
+      return;
+    }
+
+    let options = {
+      cmd: "setUSBTethering",
+      params: params
+    };
+
+    this.controlMessage(options, function(obj, data) {
+      debug("setUSBTethering: return " + data.ret);
+      if (!data.ret) {
+        // This should not be happened.
+        throw new Error("failed to control USB Tethering");
+      }
+    });
+  },

When sending a command to netd you are using 2 callbacks, one passed in controlMessage function as a parameter and the other that is 'hardcoded' as a constant when the netd response is received. Can you change the implementation to use just one callback?

::: dom/system/gonk/net_worker.js
@@ -10,20 +10,98 @@
  * distributed under the License is distributed on an "AS IS" BASIS,
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
 
 "use strict";
 
-const DEBUG = true;
+const DEBUG = false;
+const TETHERING_COMMAND_SUCCESS_CODE = "200";

For netd success codes are 2xx, not only 200 (system/netd/ResponseCode.h). If this is modified then your patch can be used as a generic way to connect asynchronously to netd, then also names have to be refactored to remove the 'TETHERING' prefix.

+function onNetdMessage(data) {
+  let response = "";
+
+  for (let i = 0; i < data.length; i++) {
+    response += String.fromCharCode(data[i]);
+  }
+
+  let delimiter = response.indexOf(" ");
+  let code = response.substr(0, delimiter);
+  let result = response.substr(delimiter + 1, response.length);
+
+  debug("code: " + code + " result: " + result);
+
+  // Set pending to false before we handle next command. 
+  gCurrentCommand.pending = false; 
+
+  if (controlCallback) { 
+    if (code == TETHERING_COMMAND_SUCCESS_CODE) {
+      controlCallback(false, TETHERING_COMMAND_SUCCESS);
+    } else {
+      controlCallback(true, TETHERING_COMMAND_FAIL);
+    }
+  }
+  // Handle pending commands. 
+  if (gCommandQueue.length > 0) {
+    let data = gCommandQueue.shift();   
+    doCommand(data.command, data.callback);
+  } 
+}

I see that all your requests to netd are commands that return true or false, but in my case I need the result string (result variable in your code) and you are not providing it to callbacks

	Philipp von Weitershausen [:philikon]
	Comment 73 • 12 years ago

Comment on attachment 646508 [details] [diff] [review]
Updated tethering implementation V7

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

Great work! This patch is getting a lot closer to the final product. Still lots of things to address. Some general notes:

* I noticed that your APIs use "WiFi" where as the rest of Gecko seems to use "Wifi". Please follow the existing convention and call it "Wifi" everywhere.

* It would be good if comments and log messages were complete English sentences (with proper orthography and punctuation) that indicated the intent of what's going on. Right now they just seem to be random log markers. For instance, `debug("usb tethering")` is not as helpful as `debug("Starting USB tethering on interface xyz, with settings a, b, c.")`

::: dom/system/gonk/NetworkManager.js
@@ +198,5 @@
> +    let data = e.data;
> +    let id = data.id;
> +    let callback = this.controlCallbacks[id];
> +    if (callback) {
> +      callback(this, data);

I think you want

  callback.call(this, data);

That way you don't need the explicit `object` parameter in the callbacks.

@@ +199,5 @@
> +    let id = data.id;
> +    let callback = this.controlCallbacks[id];
> +    if (callback) {
> +      callback(this, data);
> +      // Net worker may report tethering status actively.  

Better:

  // net_worker may proactively report the tethering status.
  // Do not remove the callbacks for those messages.

@@ +203,5 @@
> +      // Net worker may report tethering status actively.  
> +      if (id != "usbTetheringResultReport" && 
> +          id != "wifiTetheringResultReport" && 
> +          id != "setUSBFunctionResult")
> +        delete this.controlCallbacks[id];

Always use braces, please.

@@ +282,5 @@
> +   * Handle setting changes.
> +   */
> +  handleMozSettingsChanged: function handleMozSettingsChanged(setting) {
> +    switch (setting.key) {
> +      case "tethering.usb.enabled":

It might be a bit cleaner to factor each `case` block into a separate method, e.g.:

  handleMozSettingsChanged: function handleMozSettingsChanged(setting) {
    switch (setting.key) {
      case "tethering.usb.enabled":
        this.handleUSBTetheringToggle(setting.value);
        break;
      case "tethering.wifi.enabled":
        ...

@@ +299,5 @@
> +          if (this.active != null) {
> +            this._tetheringInterface[TETHERING_TYPE_USB].externalinterface = this.active.name;
> +          } else {
> +            let mobile = this.getNetworkInterface(Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE);
> +            if (mobile === null) {

Simpler: if (mobile)

@@ +362,5 @@
> +    let securityId; 
> +    let interfaceip;
> +    let prefix;
> +    let dhcpStartIp;
> +    let dhcpEndIp;

I do recommend you take a look at bug 776294 where we eliminated the prefs service as an intermediate step for the settings. It makes things a lot cleaner.

Can also do it as a follow-up, but in that case please make sure to file it and fix it immediately after this bug.

@@ +389,5 @@
> +          return null;
> +        }
> +        if (securityType != "open" && 
> +            securityType != "wpa-psk" && 
> +            securityType != "wpa2-psk") {

Please const those strings.

@@ +408,5 @@
> +        }
> +
> +        return {
> +          ifname: internalinterface,
> +          wifictrlinterfacename: "wl0.1",

Please const this string.

@@ +422,5 @@
> +          internalifname: internalinterface,
> +          externalifname: externalinterface,
> +          enable: enable,
> +          mode: enable ? "AP":"STA",
> +          link: enable ? "up":"down"

Please const these strings.

@@ +478,5 @@
> +
> +    let options = {
> +      cmd: "setWiFiTethering",
> +      params: params
> +    };

Why not

  params.cmd = "setWiFiTethering";

and then just send `params` to the worker. Saves an object allocation!

@@ +519,5 @@
> +    if (type == TETHERING_TYPE_WIFI) {
> +      debug("notify wifi manager");
> +      let wifi = Cc["@mozilla.org/telephony/system-worker-manager;1"]
> +                   .getService(Ci.nsIInterfaceRequestor)
> +                   .getInterface(Ci.nsIWifi);

Make this a lazy getter on the global scope:

  XPCOMUtils.defineLazyGetter(this, "gWifi", function () {
    return Cc["@mozilla.org/telephony/system-worker-manager;1"]
             .getService(Ci.nsIInterfaceRequestor)
             .getInterface(Ci.nsIWifi);
  });

and then just refer to `gWifi` here and everywhere else.

@@ +620,5 @@
> +    debug("usbTetheringResultReport: " + "enable:" + enable + " result: " + result);
> +
> +    // Disable tethering settings when fail to enable it.     
> +    if (result == TETHERING_COMMAND_FAIL) {
> +      let object = (obj ? obj : this);

With right invocation of `callback` above, you won't need this.

::: dom/system/gonk/net_worker.js
@@ +25,5 @@
>  
>  importScripts("systemlibs.js");
>  
> +function wifiTetheringFail(options) {
> +  let unLoad = false;

Typically things don't get camelCased when you use the "un" prefix.

@@ +36,5 @@
> +  postMessage({id: "wifiTetheringResultReport",
> +               result: TETHERING_COMMAND_FAIL, 
> +               currentState: options.state,
> +               enable: options.params.enable,
> +               unLoad: unLoad});

You're copying some stuff from `options` here. Why not save the object creation and just do something like

  options.id = "wifiTetheringResultReport";
  options.result: TETHERING_COMMAND_FAIL,
  options.unload: unload
  postMessage(options);

This is a pattern we use a lot in ril_worker.js

@@ +42,5 @@
> +  // If one of the stages fails, we try roll back to ensure 
> +  // we don't leave the network systems in limbo.
> +  let functionChain = [stopSoftAP, 
> +                       setIpForwardingEnabled,
> +                       stopTethering];

This chain can be defined as a global variable, right?

@@ +77,5 @@
> + 
> +  // Try to roll back to ensure 
> +  // we don't leave the network systems in limbo.
> +  let functionChain = [setIpForwardingEnabled,
> +                       stopTethering];

Global var?

@@ +163,5 @@
>    // come online that's preferred...
>    setDefaultRouteAndDNS(dhcp);
>  }
>  
> +let controlCallback = Object.create(null);

Please move this next to `gCurrentCommand` and call it `gCurrentCallback` or similar to indicate that it goes along with that variable. Also, please initialize it to `null`. `Object.create(null)` creates a hashmap, but that's not how you're using this variable.

@@ +177,5 @@
> +  }
> +
> +  let delimiter = response.indexOf(" ");
> +  let code = response.substr(0, delimiter);
> +  let result = response.substr(delimiter + 1, response.length);

Since you're assembling the string by hand above, you could just watch for the space character fly by. Something like this:

  let code = "";
  let result = "";

  // The return code is separated from the result by a space character.
  let i = 0;
  while (i < data.length) {
    let octet = data[i];
    i += 1;
    if (octet == 32) {
      break;
    }
    code += String.fromCharCode(octet);
  }

  for (; i < data.length; i++) {
    result += String.fromCharCode(octet);
  }

@@ +182,5 @@
> +
> +  debug("code: " + code + " result: " + result);
> +
> +  // Set pending to false before we handle next command. 
> +  gCurrentCommand.pending = false; 

With the two functions I suggested below, this would become

  gCurrentCommand = gCurrentCallback = null;

@@ +192,5 @@
> +      controlCallback(true, TETHERING_COMMAND_FAIL);
> +    }
> +  }
> +  // Handle pending commands. 
> +  if (gCommandQueue.length > 0) {

This could just be `if (gCommandQueue.length)`, but with the two functions I suggested below, the entire `if` block can be replaced with a single call to `nextNetdCommand()`.

@@ +199,5 @@
> +  } 
> +}
> +
> +let gCommandQueue = [];
> +let gCurrentCommand = {pending: false, command: "", callback: null};

Can just initialize this to `null`.

@@ +209,5 @@
> +  let data = {command: command, callback: callback};
> +
> +  if (gCurrentCommand.pending) {
> +    gCommandQueue.push(data);
> +  } else {

I would suggest separating this a bit, e.g.:

  function doCommand(command, callback) {
    gCommandQueue.push([command, callback]);
    nextNetdCommand();
  }

  function nextNetdCommand() {
    if (!gCommandQueue.length) {
      return;
    }
    [gCurrentCommand, gCurrentCallback] = gCommandQueue.shift();
    postNetdCommand(gCurrentCommand, gCurrentCallback);
  }

@@ +247,5 @@
> +  if (options.enable) {
> +    command = command.concat(SP, "enable");
> +  } else {
> +    command = command.concat(SP, "disable");
> +  }

Why are you using `string.concat()`? This would be much simpler and more efficient with string literals:

  let command;
  if (options.enable) {
    command = "ipfwd enable";
  } else {
    command = "ipfwd disable";
  }

@@ +255,5 @@
> +
> +function startTethering(options, callback) {
> +  let command = "tether start";
> +
> +  command = command.concat(SP, options.startip, SP, options.endip);

let command = "tether start " + options.startip + " " + options.endip;

@@ +369,5 @@
> + * Modify usb function's property to turn on USB RNDIS function
> + */
> +
> +function setUSBFunction(options) {
> +  let path = "persist.sys.usb.config";

Please inline this string literal below.

@@ +376,5 @@
> +  let i = 0;
> +
> +  libcutils.property_set(path, options.usbfunc);
> +  if (report) {
> +    setTimeout(checkUSBFunction, 100, options);

Please const the 100.

@@ +389,5 @@
> +
> +      retry = 0;
> +      return;
> +    }
> +    if (retry++ < 20) {

Please separate the increment from the check.

@@ +437,5 @@
> +    f = funcs[i];
> +    if (f) {
> +      let ret = f(params, next);
> +      if (!ret) {
> +        if (onError) {

if (!ret && onError)

@@ +522,5 @@
> +  } else {
> +    // Disable USB tetehring.
> +    chain(params, gUSBDisableChain, wifiTetheringFail);
> +  }
> +  return true;

Why do all these functions return true? I don't see the return values being used anywhere, but perhaps I'm missing something.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 74 • 12 years ago

> +const DEFAULT_USB_INTERFACE_NAME  = "rndis0";
> +const DEFAULT_3G_INTERFACE_NAME   = "rmnet0";
> +const DEFAULT_WIFI_INTERFACE_NAME = "wlan0";
> 
> Is a good way to set 'wlan0' and 'rmnet0' as constants? I also need to get
> this interfaces names before the interface is registered to get the stats,
> but to ensure this interfaces names are correct I'm getting them from
> NetworkManager. Will be easier to set them as a constants as well.
> 
> Otherwise, these params can be obtained from 'wifi.interface' and
> 'mobiledata.interfaces'. The problem is that 'mobiledata.interfaces' is
> added in ICS and it is not giving the correct interfaces. If this can be
> corrected things will be easier.

 Wifi and 3G interface name will be updated from WifiWorker and active interface name.   

> 
> 
> +  // Enable/disable USB tethering by sending commands to netd.
> +  setUSBTethering: function setUSBTethering(enable, tetheringinterface) {
> +    let params = this.getTetheringParameters(TETHERING_TYPE_USB, enable,
> tetheringinterface);
> +
> +    if (params === null) {
> +      this.usbTetheringResultReport(this, {result: TETHERING_COMMAND_FAIL,
> enable: enable});
> +      this.setUSBFunction(USB_FUNCTION_ADB, null);
> +      return;
> +    }
> +
> +    let options = {
> +      cmd: "setUSBTethering",
> +      params: params
> +    };
> +
> +    this.controlMessage(options, function(obj, data) {
> +      debug("setUSBTethering: return " + data.ret);
> +      if (!data.ret) {
> +        // This should not be happened.
> +        throw new Error("failed to control USB Tethering");
> +      }
> +    });
> +  },
> 
> When sending a command to netd you are using 2 callbacks, one passed in
> controlMessage function as a parameter and the other that is 'hardcoded' as
> a constant when the netd response is received. Can you change the
> implementation to use just one callback?

  I can try to improve it. 
> +const TETHERING_COMMAND_SUCCESS_CODE = "200";
> 
> For netd success codes are 2xx, not only 200 (system/netd/ResponseCode.h).
> If this is modified then your patch can be used as a generic way to connect
> asynchronously to netd, then also names have to be refactored to remove the
> 'TETHERING' prefix.

  I can improve it. 
> 
> +function onNetdMessage(data) {
> +  let response = "";
> +
> +  for (let i = 0; i < data.length; i++) {
> +    response += String.fromCharCode(data[i]);
> +  }
> +
> +  let delimiter = response.indexOf(" ");
> +  let code = response.substr(0, delimiter);
> +  let result = response.substr(delimiter + 1, response.length);
> +
> +  debug("code: " + code + " result: " + result);
> +
> +  // Set pending to false before we handle next command. 
> +  gCurrentCommand.pending = false; 
> +
> +  if (controlCallback) { 
> +    if (code == TETHERING_COMMAND_SUCCESS_CODE) {
> +      controlCallback(false, TETHERING_COMMAND_SUCCESS);
> +    } else {
> +      controlCallback(true, TETHERING_COMMAND_FAIL);
> +    }
> +  }
> +  // Handle pending commands. 
> +  if (gCommandQueue.length > 0) {
> +    let data = gCommandQueue.shift();   
> +    doCommand(data.command, data.callback);
> +  } 
> +}
> 
> I see that all your requests to netd are commands that return true or false,
> but in my case I need the result string (result variable in your code) and
> you are not providing it to callbacks

I can improve it.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 75 • 12 years ago

> > +  let delimiter = response.indexOf(" ");
> > +  let code = response.substr(0, delimiter);
> > +  let result = response.substr(delimiter + 1, response.length);
> 
> Since you're assembling the string by hand above, you could just watch for
> the space character fly by. Something like this:
> 
>   let code = "";
>   let result = "";
> 
>   // The return code is separated from the result by a space character.
>   let i = 0;
>   while (i < data.length) {
>     let octet = data[i];
>     i += 1;
>     if (octet == 32) {
>       break;
>     }
>     code += String.fromCharCode(octet);
>   }
> 
>   for (; i < data.length; i++) {
>     result += String.fromCharCode(octet);
>   }

  Does it mean that using the indexOf and substr string APIs is slower than just passing the space token ? Or others reason ?   

> I would suggest separating this a bit, e.g.:
> 
>   function doCommand(command, callback) {
>     gCommandQueue.push([command, callback]);
>     nextNetdCommand();
>   }
> 
>   function nextNetdCommand() {
>     if (!gCommandQueue.length) {
>       return;
>     }
>     [gCurrentCommand, gCurrentCallback] = gCommandQueue.shift();
>     postNetdCommand(gCurrentCommand, gCurrentCallback);
>   }

  I would like to add one more flag gPending to indicate that if we have gotten the response for previous command. This flag will be set to false in when receive the response in onNetdMessage function. Also, postNetdCommand(gCurrentCommand). 

   function nextNetdCommand() {
     if (!gCommandQueue.length || gPending) {
       return;
     }
     [gCurrentCommand, gCurrentCallback] = gCommandQueue.shift();
     postNetdCommand(gCurrentCommand, gCurrentCallback);
   }

	Philipp von Weitershausen [:philikon]
	Comment 76 • 12 years ago

(In reply to Vincent Chang from comment #75)
> Does it mean that using the indexOf and substr string APIs is slower than
> just passing the space token ? Or others reason ?   

All I'm saying is: since you're looping through the data anyway, why loop through it once and then split it apart if you can catch the character you're looking for in the loop?

>   I would like to add one more flag gPending to indicate that if we have
> gotten the response for previous command. This flag will be set to false in
> when receive the response in onNetdMessage function. Also,
> postNetdCommand(gCurrentCommand). 
> 
>    function nextNetdCommand() {
>      if (!gCommandQueue.length || gPending) {
>        return;
>      }
>      [gCurrentCommand, gCurrentCallback] = gCommandQueue.shift();
>      postNetdCommand(gCurrentCommand, gCurrentCallback);
>    }

Great idea!

	Blake Kaplan (:mrbkap) (inactive)
	Comment 77 • 12 years ago

Comment on attachment 645997 [details] [diff] [review]
Updated setWifiTethering function in nsIWifi interface V2

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

::: dom/wifi/WifiWorker.js
@@ +1012,5 @@
>    }
>  
> +  // Get wifi interface and load wifi driver when enable Ap mode.
> +  manager.setWifiApEnabled = function(enable, callback) {
> +    if (enable) {

You need to deal with manager.enabled somewhere in this function, right?

@@ +1024,5 @@
> +          if (status < 0) {
> +            callback(status, null);
> +            return;
> +          }
> +          WifiNetworkInterface.name = manager.ifname;

Where is WifiNetworkInterface declared? Also, what is it?

@@ +1031,5 @@
> +        });
> +      });
> +    } else {
> +      manager.state = "UNINITIALIZED";
> +      unloadDriver(function(status) { 

On Otoro unloadDriver is a no-op. Is that going to cause bugs here? If so, we need to get the vendor to fix the bug where unloading and reloading the driver doesn't bring up the network interface...

@@ +1036,5 @@
> +        if (status < 0) {
> +          callback(status, null);
> +          return;
> +        }
> +        callback(0, null);

Nit: No need for the if statement, this will do the right thing if you simply do |callback(status, null)|.

@@ +2056,5 @@
> +    debug("Requesting Wifi Tethering from NetworkManager " + enabled);
> +    if (enabled) {
> +      if (WifiManager.enabled) {
> +        // Remove wifi driver and wpa_supplicant before enabling tethering.
> +        WifiManager.setWifiEnabled(false, (function (status) {

The wifi on/off stuff got really complicated with bug 766497, I think you might need to deal with this._stateRequests and figure out how this stuff works if e.g. someone turns on tethering in settings and then quickly turns wifi on and off elsewhere.

@@ +2066,5 @@
> +        }).bind(this));
> +        return;
> +      }
> +    }
> +    this.setWifiApEnabled(enabled, callback.wifiTetheringEnabledChange);

Does this deal with the case where wifi was turned on independently (i.e. is already on) and then tethering was turned off?

::: dom/wifi/nsIWifi.idl
@@ +11,5 @@
>  
> +[scriptable, function, uuid(c675c7c3-5261-480a-98f3-69934c9ab7c6)]
> +interface nsIWifiTetheringCallback : nsISupports
> +{
> +  void wifiTetheringEnabledChange(in jsval result, 

Nitpick: there's trailing whitespace at the end of this line.

Non-nit: Please document what this function means and especially what the jsval result will represent.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 80 • 12 years ago

> > +  // Get wifi interface and load wifi driver when enable Ap mode.
> > +  manager.setWifiApEnabled = function(enable, callback) {
> > +    if (enable) {
> 
> You need to deal with manager.enabled somewhere in this function, right?
  
  Since we will disable wifi before enabling wifi hotspot, the manager.enabled should 
  already be set to false. Do we still need to set it here ?  

> @@ +1024,5 @@
> > +          if (status < 0) {
> > +            callback(status, null);
> > +            return;
> > +          }
> > +          WifiNetworkInterface.name = manager.ifname;
> 
> Where is WifiNetworkInterface declared? Also, what is it?

  It is declared in the beginning of WifiWorker.js. The type is defined in nsINetworkManager.idl. We try to pass wifi interface's name to NetworkManager.js here.  

> > +      manager.state = "UNINITIALIZED";
> > +      unloadDriver(function(status) { 
> 
> On Otoro unloadDriver is a no-op. Is that going to cause bugs here? If so,
> we need to get the vendor to fix the bug where unloading and reloading the
> driver doesn't bring up the network interface...

I found that the driver may still send the beacon if we don't unloadDriver. But it should not happened if we turn wifi interface down. I can check it in more detail.  

> 
> @@ +1036,5 @@
> > +        if (status < 0) {
> > +          callback(status, null);
> > +          return;
> > +        }
> > +        callback(0, null);
> 
> Nit: No need for the if statement, this will do the right thing if you
> simply do |callback(status, null)|.
  
  Is it possible that status may greater than zero ? 

> 
> @@ +2056,5 @@
> > +    debug("Requesting Wifi Tethering from NetworkManager " + enabled);
> > +    if (enabled) {
> > +      if (WifiManager.enabled) {
> > +        // Remove wifi driver and wpa_supplicant before enabling tethering.
> > +        WifiManager.setWifiEnabled(false, (function (status) {
> 
> The wifi on/off stuff got really complicated with bug 766497, I think you
> might need to deal with this._stateRequests and figure out how this stuff
> works if e.g. someone turns on tethering in settings and then quickly turns
> wifi on and off elsewhere.

  I found that there are four functions calls _notifyAfterStateChange and may pop out the   old request and serve the next request in this._stateRequests. I am fine with function 1. But for function 2~4, it is possible that the old request is pop out and start to serve the new request in this._stateRequests while we are still in the middle of WifiManager.setWifiEnabled for old request. We can only 100% sure that the old request have been finished when we are calling this._setWifiEnabledCallback, right ?
   
  1. this._setWifiEnabledCallback
  2. WifiManager.onsupplicantlost  => self._notifyAfterStateChange(true, true);
  3. WifiManager.onsupplicantconnection => self._notifyAfterStateChange(true, false);
  4. WifiManager.onsupplicantfailed => self._notifyAfterStateChange(false, false);

> 
> @@ +2066,5 @@
> > +        }).bind(this));
> > +        return;
> > +      }
> > +    }
> > +    this.setWifiApEnabled(enabled, callback.wifiTetheringEnabledChange);
> 
> Does this deal with the case where wifi was turned on independently (i.e. is
> already on) and then tethering was turned off?

  This callback.wifiTetheringEnabledChange is used to pass status of setWifiApEnabled and wifi interface name to NetworkManager.js.

> > +{
> > +  void wifiTetheringEnabledChange(in jsval result, 
> 
> Nitpick: there's trailing whitespace at the end of this line.
May I know the tool to check trailing whitespace nit ?

	Andreas Gal :gal
	Comment 81 • 12 years ago

Chris, I wonder whether we should consider punting on this for v1.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 82 • 12 years ago

(In reply to Vincent Chang from comment #80)
>   Since we will disable wifi before enabling wifi hotspot, the
> manager.enabled should 
>   already be set to false. Do we still need to set it here ?  

Ah, right.

>   Is it possible that status may greater than zero ? 

That would be a bug in the C library. The contract says that 0 is success and anything else indicates an error.

> 
> > 
> > @@ +2056,5 @@
> > > +    debug("Requesting Wifi Tethering from NetworkManager " + enabled);
> > > +    if (enabled) {
> > > +      if (WifiManager.enabled) {
> > > +        // Remove wifi driver and wpa_supplicant before enabling tethering.
> > > +        WifiManager.setWifiEnabled(false, (function (status) {
> > 
> > The wifi on/off stuff got really complicated with bug 766497, I think you
> > might need to deal with this._stateRequests and figure out how this stuff
> > works if e.g. someone turns on tethering in settings and then quickly turns
> > wifi on and off elsewhere.
> 
>   I found that there are four functions calls _notifyAfterStateChange and
> may pop out the   old request and serve the next request in
> this._stateRequests. I am fine with function 1. But for function 2~4, it is

I think we talked about this on IRC, right? The easiest way to make this work is to extend the machinery there to not always fire a request, but do something else if you pass a special rid/mid.

> May I know the tool to check trailing whitespace nit ?

The splinter review tool shows trailing whitespace. Also, most editors can be told to show trailing whitespace.

	Philipp von Weitershausen [:philikon]
	Comment 83 • 12 years ago

Comment on attachment 650022 [details] [diff] [review]
Updated tethering implementation V8

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

Lots of trailing whitespace.

::: b2g/chrome/content/settings.js
@@ +81,5 @@
>    });
>  })();
>  
> +// =================== Tethering ====================
> +(function NetworkManagerSettingsToPrefs() {

This should no longer be necessary. Please remove.

::: dom/system/gonk/NetworkManager.js
@@ +108,5 @@
> +  this._tetheringInterface = Object.create(null);
> +  this._tetheringInterface[TETHERING_TYPE_USB] = {externalinterface: DEFAULT_3G_INTERFACE_NAME, 
> +                                                  internalinterface: DEFAULT_USB_INTERFACE_NAME};
> +  this._tetheringInterface[TETHERING_TYPE_WIFI] = {externalinterface: DEFAULT_3G_INTERFACE_NAME, 
> +                                                   internalinterface: DEFAULT_WIFI_INTERFACE_NAME};

Please camelCase these member names.

@@ +240,5 @@
> +    let data = e.data;
> +    let id = data.id;
> +    let callback = this.controlCallbacks[id];
> +    if (callback) {
> +      callback(this, data);

Please see my comment 73 about invoking you callbacks with the right `this` and how you can save the explicit `obj` parameter that way.

@@ +369,5 @@
> +  _tetheringInterface: null, 
> +
> +  // Internal state for enable or disable tethering. 
> +  _wifiTetheringState: null,
> +  _usbTetheringState: null,

`_*ThetheringEnabled` is a better name. `state` indicates a multitude of states where in fact it's just a boolean. Also, make the default value `false` please.

That said, I don't understand why you need these when you already have mirror settings in `this.tetheringSettings`. See my comment below on copying RadioInterfaceLayer more closely.

@@ +387,5 @@
> +            enable: enable,
> +            result: { 
> +              code: COMMANDERROR,
> +              reason: "Mobile interface is not registered"
> +            }

Why is `result` a sub-object? Seems like unnecessary and wasteful nesting to me. Just do

  let params = {
    ...
    resultCode: ...,
    resultReason: ...
  };

@@ +390,5 @@
> +              reason: "Mobile interface is not registered"
> +            }
> +          };
> +          this.usbTetheringResultReport(this, params);
> +          dump("Can't enable usb tethering - no active network interface.");

Please use `debug()`, not `dump()` everywhere.

@@ +436,5 @@
> +
> +  /**
> +   * Handle setting changes.
> +   */
> +  handleMozSettingsChanged: function handleMozSettingsChanged(setting) {

This method needs to also update `this.tetheringSettings` for all of the settings that you listed in `handle()`.

I suggest you look at how RadioInterfaceLayer does this. Basically, it only implements `handle()` and calls that method for the TOPIC_MOZSETTINGS_CHANGED observer notification.

@@ +566,5 @@
> +          prefix: prefix,
> +          startip: dhcpStartIp,
> +          endip: dhcpEndIp,
> +          dnsserver1: "",
> +          dnsserver2: "",

In other places we called this simply `dns1` and `dns2`.

::: dom/system/gonk/net_worker.js
@@ +27,5 @@
> +const USB_FUNCTION_RETRY_INTERVAL = 100;
> +
> +// 2xx - Requested action has been successfully completed 
> +const COMMANDOKAY  = 200;
> +const COMMANDERROR = 300;

Better names: NETD_COMMAND_OK, NETD_COMMAND_ERROR.

@@ +130,3 @@
>    let ret = self[message.cmd](message);
> +  if (!message.asynccallback)
> +    postMessage({id: message.id, ret: ret});

Nits:

* Please camelCase the member name, e.g.: `asyncCallback`. But since this is a boolean, I would even recommend changing the name to something like `isAsync`. That seems much clearer to me, especially since the word "callback" can be pretty overloaded.

* Always need braces.

@@ +168,5 @@
>    setDefaultRouteAndDNS(dhcp);
>  }
>  
> +let gCommandQueue = [];
> +let gCurrentCommand = {pending: false, command: null, callback: null};

It seems to me that the `pending` flag expresses global state, not state for a particular command.

Also, I still think a flat collection of global variables as suggested in comment 73 is a bit easier to handle, but I won't find you on this point.

	Philipp von Weitershausen [:philikon]
	Comment 85 • 12 years ago

Comment on attachment 651392 [details] [diff] [review]
Updated tethering implementation V9

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

Very, very nice. Thanks for patience in continuously improving this patch.

r=me with the few nits below addressed. Woo!

::: dom/system/gonk/NetworkManager.js
@@ +404,5 @@
> +      this.tetheringSettings[SETTINGS_USB_ENABLED] = true;
> +      this.enableTethering(TETHERING_TYPE_USB);
> +    } else {
> +      this.tetheringSettings[SETTINGS_USB_ENABLED] = false;
> +      this.disableTethering(TETHERING_TYPE_USB);

Bailout early style would make this slightly more readable:

  if (!enable) {
    ...
    return;
  }
  ...

@@ +481,5 @@
> +    let externalInterface = tetheringinterface.externalInterface;
> +
> +    switch (type) {
> +      case TETHERING_TYPE_WIFI:
> +        ssid = this.tetheringSettings[SETTINGS_WIFI_SSID];

Move long `case` body to a helper method, please (e.g. `getWifiTetheringParameters`).

@@ +533,5 @@
> +        };
> +
> +      case TETHERING_TYPE_USB:
> +        interfaceIp = this.tetheringSettings[SETTINGS_USB_IP];
> +        prefix = this.tetheringSettings[SETTINGS_USB_PREFIX];

Same here.

::: dom/system/gonk/net_worker.js
@@ +72,5 @@
> +}
> +
> +let gUSBFailChain = [stopSoftAP,
> +                      setIpForwardingEnabled,
> +                      stopTethering];

Nit: align

@@ +363,4 @@
>  }
> +
> +function dumpParams(params, type) {
> +  debug("Dump params:");

I think it'd be good adding a

  if (!DEBUG) {
    return;
  }

to the top of this function here.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 86 • 12 years ago

I'm about half way through the review here, sorry for the delay. Should have something soon.

	Philipp von Weitershausen [:philikon]
	Comment 87 • 12 years ago

Comment on attachment 651392 [details] [diff] [review]
Updated tethering implementation V9

One last thing:

>+function wifiTetheringFail(params) {
>+  let unload = false;
>+
>+  // Unload wifi driver when failing to enable tethering.
>+  if (params.enable) {
>+    unload = true;
>+  }
>+  params.unload = unload;
>+  // Notify the main thread.
>+  postMessage(params);
>+
>+  // If one of the stages fails, we try roll back to ensure
>+  // we don't leave the network systems in limbo.
>+
>+  // This parameter is used to disable ipforwarding.
>+  params.enable = false;
>+  chain(params, gWifiFailChain, null);
>+
>+  return true;

You didn't address this from my previous comment 73:

> Why do all these functions return true? I don't see the return values
> being used anywhere, but perhaps I'm missing something.

Please remove all useless return values!

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 88 • 12 years ago

Comment on attachment 646507 [details] [diff] [review]
Updated IPC interface implementation V5

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

You've got a ton of trailing whitespace in this patch. Please remove it all.

And please make sure you address all the comments. There were several from last time that you didn't address here.

::: dom/system/gonk/SystemWorkerManager.cpp
@@ +225,5 @@
> +      return false;
> +    }
> +
> +    size = JS_GetTypedArrayByteLength(obj, cx);
> +    data = JS_GetArrayBufferViewData(obj, cx);

These will both return 0 (null) in some cases, you should check and handle failures.

@@ +233,5 @@
> +    return false;
> +  }
> +
> +  // Reserve one space for '\0'.
> +  if (size > MAX_COMMAND_SIZE - 1) {

What about if size is 0 here? Do you want to throw an exception?

@@ +240,5 @@
> +  }
> +
> +  memcpy(command->mData, data, size);
> +  // Include the null terminate to the command to make netd happy.
> +  command->mData[size] = '\0';

Nit: this is a uint8_t array, just use 0.

@@ +511,5 @@
> +{
> +  nsCOMPtr<nsIWorkerHolder> worker = do_GetService(NETWORKMANAGER_CONTRACTID);
> +  NS_ENSURE_TRUE(worker, NS_ERROR_FAILURE);
> +  jsval workerval;
> +

Nit: this newline went in the wrong place. move it up one line, bring workerval down below it.

::: ipc/netd/Makefile.in
@@ +1,2 @@
> +# ***** BEGIN LICENSE BLOCK *****
> +# Version: MPL 1.1/GPL 2.0/LGPL 2.1

Please upgrade this to the new MPL2 ( http://www.mozilla.org/MPL/headers/ )

::: ipc/netd/Netd.cpp
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#include "Netd.h"

Nit: newline between license header and this line.

@@ +13,5 @@
> +
> +#include "cutils/properties.h"
> +#include "android/log.h"
> +
> +

Nit: remove extra newline.

@@ +23,5 @@
> +namespace mozilla {
> +namespace ipc {
> +
> +static RefPtr<NetdClient> sNetdClient;
> +static RefPtr<NetdConsumer> sNetdConsumer;

> > Or could I use namespace ipc { .... ? Because this code is located
> > in ipc/netd... and remove namespace led to compiler error in 
> > SystemWorkerManager.cpp. 

The reason moving them out of mozilla::ipc caused build errors is that these names (NetdClient/NetdConsumer) are not fully qualified here. Please do this:

  namespace {

    RefPtr<mozilla::ipc::NetdClient> gNetdClient;
    RefPtr<mozilla::ipc::NetdConsumer> gNetdConsumer;

  } // anonymous namespace

  namespace mozilla {
  namespace ipc {

  NetdClient::NetdClient()
  // ...

@@ +25,5 @@
> +
> +static RefPtr<NetdClient> sNetdClient;
> +static RefPtr<NetdConsumer> sNetdConsumer;
> +
> +NetdClient::NetdClient()

You're still not initializing mCurrentWriteOffset (like I mentioned in previous comment). Please do.

@@ +80,5 @@
> +  return true;
> +}
> +
> +void
> +NetdClient::OnFileCanReadWithoutBlocking(int fd)

Nit: Parameters should start with 'a', so please make this 'aFd'.

@@ +84,5 @@
> +NetdClient::OnFileCanReadWithoutBlocking(int fd)
> +{
> +  ssize_t length = 0;
> +  ssize_t pos = 0;
> +  char rcvBuf[MAX_COMMAND_SIZE];  

So, you said before that you would do a little research and see if it's possible to overflow this buffer. For tethering stuff you said you thought less than 100 bytes, but is it possible to receive something unexpected here?

@@ +88,5 @@
> +  char rcvBuf[MAX_COMMAND_SIZE];  
> +
> +  MOZ_ASSERT(fd == mSocket.get());
> +  while (true) {
> +    length = read(fd, &rcvBuf[mReceivedIndex], MAX_COMMAND_SIZE - mReceivedIndex);

You said you'd assert that |length <= MAX_COMMAND_SIZE - mReceivedIndex|, but you haven't yet.

@@ +89,5 @@
> +
> +  MOZ_ASSERT(fd == mSocket.get());
> +  while (true) {
> +    length = read(fd, &rcvBuf[mReceivedIndex], MAX_COMMAND_SIZE - mReceivedIndex);
> +    if (length <= 0) {

Ok, I'm still a little confused about this. Should a length of 0 here really restart the connection? Why?

@@ +106,5 @@
> +      return;
> +    } 
> +
> +    pos = length; 
> +    while (pos-- > 0) {

Please assert |mReceivedIndex < MAX_COMMAND_SIZE| to be safe here.

@@ +111,5 @@
> +      if (rcvBuf[mReceivedIndex] == '\0') {
> +        // We found a line terminator. Each line is formatted as an
> +        // integer response code followed by the rest of the line.
> +        // Fish out the response code.
> +        char *endPtr;

Nit: Please make sure you're consistent with your * location, you have it on the right here and on the left in several places below. I prefer it on the left (|char* endPtr|), but I don't really care as long as you're consistent.

@@ +115,5 @@
> +        char *endPtr;
> +
> +        errno = 0;
> +        int responseCode = strtol(rcvBuf, &endPtr, 10);
> +        if (errno < 0 ) {

The docs say that this can return 0 without setting errno, and I don't see anything guaranteeing that errno is always going to be negative here, so I think you need to make this:

  if (!responseCode || errno)

@@ +118,5 @@
> +        int responseCode = strtol(rcvBuf, &endPtr, 10);
> +        if (errno < 0 ) {
> +          LOG("Can't parse netd's response: %d (%s)", errno, strerror(errno));
> +          mReceivedIndex = 0;
> +          return;

Hm. So if this fails for some reason you can't just set the index to 0 here and return. That could make the next read call overwrite the first part of the next message... This needs to be reworked.

@@ +125,5 @@
> +        if (*endPtr == ' ') {
> +          endPtr++;
> +        }    
> +        // Now fish out the rest of the line after the response code.
> +        nsDependentCString  responseLine(endPtr, &rcvBuf[mReceivedIndex] - endPtr);

Before doing this please assert that |endPtr <= rcvBuf + MAX_COMMAND_SIZE|. Just to be safe.

@@ +126,5 @@
> +          endPtr++;
> +        }    
> +        // Now fish out the rest of the line after the response code.
> +        nsDependentCString  responseLine(endPtr, &rcvBuf[mReceivedIndex] - endPtr);
> +        // TODO, process InterfaceChange(600) and BandwidthControl(601)

Let's make sure there is a bug on file for this.

@@ +128,5 @@
> +        // Now fish out the rest of the line after the response code.
> +        nsDependentCString  responseLine(endPtr, &rcvBuf[mReceivedIndex] - endPtr);
> +        // TODO, process InterfaceChange(600) and BandwidthControl(601)
> +        if (responseCode < 600) {
> +          mIncoming = new NetdCommand;

> > Can we use stack variable here ? The mIncoming will be passed to net
> > worker thread.

Yeah, I meant a stack nsAutoPtr<NetdCommand>. There's no need for this to be a member of NetdClient.

@@ +131,5 @@
> +        if (responseCode < 600) {
> +          mIncoming = new NetdCommand;
> +          // Everything else is considered to be part of the command response.
> +          mIncoming->mSize = length;
> +          memcpy(mIncoming->mData, rcvBuf, length); 

You didn't address my previous comment about this: |length| is wrong here, please see above.

@@ +138,5 @@
> +
> +        if (pos > 0) {
> +          // There is data in the receive buffer beyond the current line.
> +          // Shift it down to the beginning.
> +          memmove(&rcvBuf[0], &rcvBuf[mReceivedIndex + 1], pos);

Please assert |mReceivedIndex < MAX_COMMAND_SIZE - 1| before the memmove here.

@@ +150,5 @@
> +  }   
> +}
> +
> +void
> +NetdClient::OnFileCanWriteWithoutBlocking(int fd)

Nit: Please make this 'aFd'

@@ +161,5 @@
> +    mCurrentNetdCommand = mOutgoingQ.front();
> +    mOutgoingQ.pop();
> +  }
> +
> +  toWrite = mCurrentNetdCommand->mData;

Nit: This variable isn't doing anything here. Just replace |toWrite| below with |mCurrentNetdCommand->mData| and remove this.

@@ +167,5 @@
> +  while (mCurrentWriteOffset < mCurrentNetdCommand->mSize) {
> +    ssize_t write_amount = mCurrentNetdCommand->mSize - mCurrentWriteOffset;
> +    ssize_t written;
> +
> +    written = write (fd, toWrite + mCurrentWriteOffset, write_amount);

Nit: Please address my previous comment about this:

  ssize_t written = write(...)

Also, no space betwen |write (|

@@ +172,5 @@
> +    if (written < 0) {
> +      LOG("Cannot write to network, error %d\n", (int) written);
> +      Restart();
> +      return;
> +    }

Nit: Please add a newline here.

@@ +202,5 @@
> +  mReadWatcher.StopWatchingFileDescriptor();
> +  mWriteWatcher.StopWatchingFileDescriptor();
> +
> +  while (!mOutgoingQ.empty()) {
> +    mOutgoingQ.pop();

Unless I'm mistaken this will leak everything in the queue. I think you need to delete each item.

@@ +206,5 @@
> +    mOutgoingQ.pop();
> +  }
> +  mSocket.dispose();
> +  mReceivedIndex = 0;
> +  mCurrentNetdCommand = NULL;

Need to also zero mCurrentWriteOffset.

Nit: Please use |nullptr| now.

@@ +211,5 @@
> +  Start();
> +}
> +
> +void
> +NetdClient::Start()

I don't understand why this is a static method. It's called with an instance (sNetdClient->Start() in InitNetdIOThread), and all it does is call methods on sNetdClient which should be the same instance. Let's make it a non-static method (you'll have to use NewRunnableMethod in the PostDelayedTask thing).

@@ +223,5 @@
> +  if (!sNetdClient->OpenSocket()) {
> +    // Socket open failed, try again in a second.
> +    LOG("Fail to connect to Netd"); 
> +    if (++sNetdClient->mReConnectTimes > MAX_RECONNECT_TIMES) {
> +      LOG("Fail to connect to Netd after retry %d times", sNetdClient->mReConnectTimes - 1); 

Nit: Just make the arg MAX_RECONNECT_TIMES, you know that's the actual value.

@@ +237,5 @@
> +  sNetdClient->mReConnectTimes = 0;
> +}
> +
> +static bool 
> +InitRndisAddress()

Nit: Move this to the anonymous namespace that you created above, then you can lose the 'static'.

@@ +246,5 @@
> +  char address[kEthernetAddressLength];
> +  int i = 0;
> +  int ret = 0;
> +  int length = 0;
> +  int fd = open(ICS_SYS_USB_RNDIS_MAC, O_WRONLY);

Please use ScopedClose here (http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/FileUtils.h#54 ). Then you won't have to worry about manually closing with early returns.

@@ +248,5 @@
> +  int ret = 0;
> +  int length = 0;
> +  int fd = open(ICS_SYS_USB_RNDIS_MAC, O_WRONLY);
> +
> +  if (fd < 0) {

I don't know enough about posix stuff, but can a valid fd be negative? I thought you were supposed to check against -1 here.

@@ +267,5 @@
> +          address[0], address[1], address[2], 
> +          address[3], address[4], address[5]);
> +  length = strlen(mac); 
> +  ret = write(fd, mac, length); 
> +  if (ret < 0 || ret != length) {

Nit: The < 0 check is unnecessary here, != length is sufficient.

@@ +287,5 @@
> +  property_get("ro.build.version.sdk", propValue, "0");
> +  // Assign rndis address for usb tethering in ICS.
> +  if (atoi(propValue) >= 15) {
> +    result = InitRndisAddress();
> +    if (result < 0 ) {

result is a bool. |if (!result)| is what you want.

@@ +288,5 @@
> +  // Assign rndis address for usb tethering in ICS.
> +  if (atoi(propValue) >= 15) {
> +    result = InitRndisAddress();
> +    if (result < 0 ) {
> +      LOG("fail to give rndis interface an address");

Logging only isn't really enough. If InitRndisAddress failed you should return here and not start the netdclient.

@@ +304,5 @@
> +  sNetdClient = NULL;
> +}
> +
> +void
> +SendNetdCommandIOThread(NetdCommand* aMessage)

Please assert aMessage.

@@ +310,5 @@
> +  MOZ_ASSERT(MessageLoop::current() == XRE_GetIOMessageLoop());
> +
> +  if (!sNetdClient) {
> +    LOG("NetdClient is not ready");
> +    return;

Hm, this would be really bad, but impossible I think. You should just assert sNetdClient.

@@ +315,5 @@
> +  }
> +  
> +  if (sNetdClient->mSocket.get() == INVALID_SOCKET) {
> +    LOG("Netd connection is not established");
> +    return;  

This is a race. If the connection had to be retried or something then you need to wait for it to connect, not just bail out and drop the message. I think you need to always queue it.

Then you should only call OnFileCanWriteWithoutBlocking if the socket is available.

@@ +322,5 @@
> +  sNetdClient->OnFileCanWriteWithoutBlocking(sNetdClient->mSocket.rwget());
> +}
> +
> +void
> +StartNetd(NetdConsumer *aNetdConsumer)

Please assert which thread you expect to be running on.

@@ +326,5 @@
> +StartNetd(NetdConsumer *aNetdConsumer)
> +{
> +  MOZ_ASSERT(aNetdConsumer);
> +
> +  sNetdConsumer = aNetdConsumer;

Hm. Please assert that sNetdConsumer is null before you set it here.

Also, what cleans this up? As far as I can tell nothing does. We can't leave this alive forever (among other things it ends up holding a WorkerCrossThreadDispatcher past worker shutdown) so we need to figure out something here.

@@ +328,5 @@
> +  MOZ_ASSERT(aNetdConsumer);
> +
> +  sNetdConsumer = aNetdConsumer;
> +  XRE_GetIOMessageLoop()->PostTask(
> +      FROM_HERE,

Nit: The PostTask calls here and below are using a 4-space indent instead of 2.

@@ +333,5 @@
> +      NewRunnableFunction(InitNetdIOThread));
> +}
> +
> +void
> +StopNetd()

Please assert which thread you expect to be running on.

@@ +341,5 @@
> +      NewRunnableFunction(ShutdownNetdIOThread));
> +}
> +
> +void
> +SendNetdCommand(NetdCommand* aMessage)

Please assert aMessage.

Also, please comment that this is called from the worker thread. You can't assert it unfortunately but a comment would help here.

::: ipc/netd/Netd.h
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_system_netd_h__

There are a few nits still unpicked from the last version of the patch. Please review the previous comments.

@@ +14,5 @@
> +namespace mozilla {
> +namespace ipc {
> +
> +#define MAX_COMMAND_SIZE  512
> +#define MAX_RECONNECT_TIMES 10

I think this should be moved into the cpp.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 90 • 12 years ago

> > Why do all these functions return true? I don't see the return values
> > being used anywhere, but perhaps I'm missing something.
> 
> Please remove all useless return values!

I would like to remove return value in wifiTetheringFail and usbTetheringFail function.
 
Can I keep the return value in setUSBFunction, setWifiTethering and setUSBTethering ? These functions have the isAsync flag with true, so the return value will not be used. But if we have message with isAsync flag set to false, then return value should be useful. Or can I just keep the code in onmessage and remove the return value in setUSBFunction, setWifiTethering and setUSBTethering.  

self.onmessage = function onmessage(event) {
  let message = event.data;
  if (DEBUG) debug("received message: " + JSON.stringify(message));
  // We have to keep the id in message. It will be used when post the result
  // to NetworkManager later.
  let ret = self[message.cmd](message);
  if (!message.isAsync) {
    postMessage({id: message.id, ret: ret});
  }
};

	Philipp von Weitershausen [:philikon]
	Comment 91 • 12 years ago

(In reply to Vincent Chang from comment #90)
> Can I keep the return value in setUSBFunction, setWifiTethering and
> setUSBTethering ? These functions have the isAsync flag with true, so the
> return value will not be used. But if we have message with isAsync flag set
> to false, then return value should be useful. Or can I just keep the code in
> onmessage and remove the return value in setUSBFunction, setWifiTethering
> and setUSBTethering.  
> 
> self.onmessage = function onmessage(event) {
>   let message = event.data;
>   if (DEBUG) debug("received message: " + JSON.stringify(message));
>   // We have to keep the id in message. It will be used when post the result
>   // to NetworkManager later.
>   let ret = self[message.cmd](message);
>   if (!message.isAsync) {
>     postMessage({id: message.id, ret: ret});
>   }
> };

Ah ok, I missed that. Sounds good then.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 92 • 12 years ago

Thanks for your patience in reviewing this patch and help to make it better. 
I updated the patch to address the comment 85 and 87.

	Philipp von Weitershausen [:philikon]
	Comment 93 • 12 years ago

Comment on attachment 653168 [details] [diff] [review]
Updated tethering implementation V10

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

Thanks for updating the patch! (I already gave you r+, so no need to ask for review again.)

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 94 • 12 years ago

> 
> You've got a ton of trailing whitespace in this patch. Please remove it all.
> 
> And please make sure you address all the comments. There were several from
> last time that you didn't address here.

Sorry, I will remove trailing whitespace and address all the comments in next patch.

> @@ +84,5 @@
> > +NetdClient::OnFileCanReadWithoutBlocking(int fd)
> > +{
> > +  ssize_t length = 0;
> > +  ssize_t pos = 0;
> > +  char rcvBuf[MAX_COMMAND_SIZE];  
> 
> So, you said before that you would do a little research and see if it's
> possible to overflow this buffer. For tethering stuff you said you thought
> less than 100 bytes, but is it possible to receive something unexpected here?

Even if we receive something unexpected, the length will less or equal to    MAX_COMMAND_SIZE - mReceivedIndex and it will process the data which is received.

The problem is if MAX_COMMAND_SIZE - mReceivedIndex equal zero, it probably indicates that we have run out of memory. So maybe I can check if MAX_COMMAND_SIZE - mReceivedIndex is equal to zero before calling read function. The other solution is when we pass zero to read function, the return value of read is zero, too. We then restart ourself and reconnect to netd.   

> 
> @@ +88,5 @@
> > +  char rcvBuf[MAX_COMMAND_SIZE];  
> > +
> > +  MOZ_ASSERT(fd == mSocket.get());
> > +  while (true) {
> > +    length = read(fd, &rcvBuf[mReceivedIndex], MAX_COMMAND_SIZE - mReceivedIndex);
> 
> You said you'd assert that |length <= MAX_COMMAND_SIZE - mReceivedIndex|,
> but you haven't yet.

I will assert length <= MAX_COMMAND_SIZE- mReceivedIndex next to read function. 
But I am wondering if it will happen in this case because the third argument is MAX_COMMAND_SIZE - mReceivedIndex ?
  
while (true) {
  length = read(afd, &rcvBuf[mReceivedIndex], MAX_COMMAND_SIZE - mReceivedIndex);
  MOZ_ASSERT(length <= MAX_COMMAND_SIZE - mReceivedIndex);

> 
> @@ +89,5 @@
> > +
> > +  MOZ_ASSERT(fd == mSocket.get());
> > +  while (true) {
> > +    length = read(fd, &rcvBuf[mReceivedIndex], MAX_COMMAND_SIZE - mReceivedIndex);
> > +    if (length <= 0) {
> 
> Ok, I'm still a little confused about this. Should a length of 0 here really
> restart the connection? Why?

  When I kill the netd manually, I got the return zero here. I added the return value in LOG message. 
I/Gonk    ( 3913): Can't read from netd error: 11 (Try again) length: 0


> @@ +118,5 @@
> > +        int responseCode = strtol(rcvBuf, &endPtr, 10);
> > +        if (errno < 0 ) {
> > +          LOG("Can't parse netd's response: %d (%s)", errno, strerror(errno));
> > +          mReceivedIndex = 0;
> > +          return;
> 
> Hm. So if this fails for some reason you can't just set the index to 0 here
> and return. That could make the next read call overwrite the first part of
> the next message... This needs to be reworked.

I will modify it to below. 

if (!responseCode || errno < 0 ) {
  LOG("Can't parse netd's response: %d (%s)", errno, strerror(errno));
  // Skip this response and move to next one. 
  if (pos > 0) {
    // There is data in the receive buffer beyond the current line.
    // Shift it down to the beginning.
    memmove(&rcvBuf[0], &rcvBuf[mReceivedIndex + 1], pos);
  }
  mReceivedIndex = 0;
  continue;
}

> @@ +126,5 @@
> > +          endPtr++;
> > +        }    
> > +        // Now fish out the rest of the line after the response code.
> > +        nsDependentCString  responseLine(endPtr, &rcvBuf[mReceivedIndex] - endPtr);
> > +        // TODO, process InterfaceChange(600) and BandwidthControl(601)
> 
> Let's make sure there is a bug on file for this.
 
I filed a new bug for this. Bug 783966 - process netd's InterfaceChange(600) and BandwidthControl(601) message.

> You didn't address my previous comment about this: |length| is wrong here,
> please see above.

Sorry, instead of using length, I will modify it to mReceivedIndex. 
 
> @@ +150,5 @@
> > +  }   
> > +}
> > +
> @@ +202,5 @@
> > +  mReadWatcher.StopWatchingFileDescriptor();
> > +  mWriteWatcher.StopWatchingFileDescriptor();
> > +
> > +  while (!mOutgoingQ.empty()) {
> > +    mOutgoingQ.pop();
> 
> Unless I'm mistaken this will leak everything in the queue. I think you need
> to delete each item.
> 

Can I rewrite it to below ? 

void
NetdClient::Restart()
{
  MOZ_ASSERT(MessageLoop::current() == XRE_GetIOMessageLoop());

  mReadWatcher.StopWatchingFileDescriptor();
  mWriteWatcher.StopWatchingFileDescriptor();

  mSocket.dispose();
  mReceivedIndex = 0;
  mCurrentWriteOffset = 0;
  mCurrentNetdCommand = nullptr;
  while (!mOutgoingQ.empty()) {
    mCurrentNetdCommand = mOutgoingQ.front();
    mOutgoingQ.pop();
    mCurrentNetdCommand = nullptr;
  }
  Start();
}

> @@ +211,5 @@
> > +  Start();
> > +}
> > +
> > +void
> > +NetdClient::Start()
> 
> I don't understand why this is a static method. It's called with an instance
> (sNetdClient->Start() in InitNetdIOThread), and all it does is call methods
> on sNetdClient which should be the same instance. Let's make it a non-static
> method (you'll have to use NewRunnableMethod in the PostDelayedTask thing).

May I know why should we make it a non-static method ? 

> 
> @@ +237,5 @@
> > +  sNetdClient->mReConnectTimes = 0;
> > +}
> > +
> > +static bool 
> > +InitRndisAddress()
> 
> Nit: Move this to the anonymous namespace that you created above, then you
> can lose the 'static'.

This function only be used in this file. That why I set it to static. I can just remove the keyword static. 
 
> @@ +288,5 @@
> > +  // Assign rndis address for usb tethering in ICS.
> > +  if (atoi(propValue) >= 15) {
> > +    result = InitRndisAddress();
> > +    if (result < 0 ) {
> > +      LOG("fail to give rndis interface an address");
> 
> Logging only isn't really enough. If InitRndisAddress failed you should
> return here and not start the netdclient.

  InitRndisAddress() function is related to usb tethering. For others service, such as wifi tethering, it still needs to use IPC mechanism to send command to netd. So we just leave a debug message to notify the error here.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 95 • 12 years ago

(In reply to Vincent Chang from comment #94)
> Even if we receive something unexpected, the length will less or equal to   
> MAX_COMMAND_SIZE - mReceivedIndex and it will process the data which is
> received.

Sure. But as we agreed in comment 51 the logic breaks if the buffer fills up. We must either ensure that no command can ever fill the buffer completely or we must handle that possibility.

> The problem is if MAX_COMMAND_SIZE - mReceivedIndex equal zero, it probably
> indicates that we have run out of memory.

You mean you've filled your buffer? Yes. I don't think we can conclude anything about the amount of memory available to the process though.

> So maybe I can check if
> MAX_COMMAND_SIZE - mReceivedIndex is equal to zero before calling read
> function.

And what would you do if it is 0?

> The other solution is when we pass zero to read function, the
> return value of read is zero, too. We then restart ourself and reconnect to
> netd.

Sure, but you'd then discard the whole buffer and lose whatever message data it contained. That doesn't seem like a good idea.

> But I am wondering if it will happen in this case because the third argument
> is MAX_COMMAND_SIZE - mReceivedIndex ?

It shouldn't, that's why we assert it :)

>   When I kill the netd manually, I got the return zero here. I added the
> return value in LOG message. 

Ok.

> > Hm. So if this fails for some reason you can't just set the index to 0 here
> > and return. That could make the next read call overwrite the first part of
> > the next message... This needs to be reworked.
> 
> I will modify it to below. 

Well, that's basically just copying the other code. Can you consolidate this so that we only have one spot where we have to call memcpy?

> Can I rewrite it to below ? 

I guess you could but this seems much better to me:

  while (!mOutgoingQ.empty()) {
    delete mOutgoingQ.front();
    mOutgoingQ.pop();
  }


> > I don't understand why this is a static method. It's called with an instance
> > (sNetdClient->Start() in InitNetdIOThread), and all it does is call methods
> > on sNetdClient which should be the same instance. Let's make it a non-static
> > method (you'll have to use NewRunnableMethod in the PostDelayedTask thing).
> 
> May I know why should we make it a non-static method ? 

I explained this already right there. Static methods are for things that don't need an instance. Your static method is called with an instance, and doesn't do anything but call other methods with that same instance.

> This function only be used in this file. That why I set it to static. I can
> just remove the keyword static.

Putting functions into an anonymous namespace makes them static.

>   InitRndisAddress() function is related to usb tethering. For others
> service, such as wifi tethering, it still needs to use IPC mechanism to send
> command to netd. So we just leave a debug message to notify the error here.

Ah, ok. I understand.

	Eric Shepherd [:sheppy]
	Comment 96 • 12 years ago

I see IDL here, so dev docs will be required for that at a minimum.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 97 • 12 years ago

Comment on attachment 653064 [details] [diff] [review]
Updated setWifiTethering function in nsIWifi interface V3

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

Looks good. Thanks.

::: dom/wifi/WifiWorker.js
@@ +1955,5 @@
>  
> +    // It is callback function's responsibility to handle the pending request.
> +    // So we just return here.
> +    if (this._stateRequests.length > 0
> +        && ("callback" in this._stateRequests[0])) {

Nit: Move the && to the previous line.

@@ +1983,4 @@
>        timer.initWithCallback(function(timer) {
> +        if ("callback" in self._stateRequests[0]) {
> +          let callback = self._stateRequests[0].callback;
> +          callback.call(self);

Nit: No need for callback, just "self._stateRequests[0].callback.call(self)"

@@ +2023,5 @@
> +    this._stateRequests.push(request);
> +    if (this._stateRequests.length === 1) {
> +      if ("callback" in this._stateRequests[0]) {
> +        let callback = this._stateRequests[0].callback;
> +        callback.call(this);

Ditto.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 98 • 12 years ago

(In reply to ben turner [:bent] from comment #95)
> (In reply to Vincent Chang from comment #94)
> > Even if we receive something unexpected, the length will less or equal to   
> > MAX_COMMAND_SIZE - mReceivedIndex and it will process the data which is
> > received.
> 
> Sure. But as we agreed in comment 51 the logic breaks if the buffer fills
> up. We must either ensure that no command can ever fill the buffer
> completely or we must handle that possibility.

Android's NativeDaemonConnector.java declares the buffer with size 4096. So it should be safe if we extend the buffer size from 512 to 4096. 

> > The other solution is when we pass zero to read function, the
> > return value of read is zero, too. We then restart ourself and reconnect to
> > netd.
> 
> Sure, but you'd then discard the whole buffer and lose whatever message data
> it contained. That doesn't seem like a good idea.

Since we extend the buffer size to 4096, we should not have the buffer full here. Moreover, the net_worker.js sends one command to netd and waits for the response before sending the next command. It's also decrease the possibility to have buffer full.   
 
> Well, that's basically just copying the other code. Can you consolidate this
> so that we only have one spot where we have to call memcpy?

May use the Macro or function to consolidate it ? Can I use the goto ?  

> > > I don't understand why this is a static method. It's called with an instance
> > > (sNetdClient->Start() in InitNetdIOThread), and all it does is call methods
> > > on sNetdClient which should be the same instance. Let's make it a non-static
> > > method (you'll have to use NewRunnableMethod in the PostDelayedTask thing).
> > 
> > May I know why should we make it a non-static method ? 
> 
> I explained this already right there. Static methods are for things that
> don't need an instance. Your static method is called with an instance, and
> doesn't do anything but call other methods with that same instance.

As I explained on the IRC, gNetdClient may be null when calling Start function. So it is easier to use static method here.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 99 • 12 years ago

Updated the patch based on comment 88

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 100 • 12 years ago

Comment on attachment 655777 [details] [diff] [review]
Updated IPC interface implementation V6

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

This is really, really close Vincent! Just a few more things to fix here (the biggest is the shutdown crash, below).

::: dom/system/gonk/SystemWorkerManager.cpp
@@ +47,5 @@
>  #ifdef MOZ_WIDGET_GONK
>  using namespace mozilla::system;
>  #endif
>  
> +#define NETWORKMANAGER_CONTRACTID  "@mozilla.org/network/manager;1"

Let's not use the contract id here. Just remove it. Then below where you call do_GetService pass kNetworkManagerCID.

@@ +52,5 @@
> +#define NS_NETWORKMANAGER_CID \
> +  { 0x33901e46, 0x33b8, 0x11e1, \
> +  { 0x98, 0x69, 0xf4, 0x6d, 0x04, 0xd2, 0x5b, 0xcc } }
> +
> +#define ICS_SYS_USB_RNDIS_MAC "/sys/class/android_usb/android0/f_rndis/ethaddr"

Looks like this is unused.

@@ +199,5 @@
> +  }
> +
> +  jsval v = JS_ARGV(cx, vp)[0];
> +
> +  nsAutoPtr<NetdCommand> command(new NetdCommand());

If you move this below all the other JS stuff in this function to right before you actually use it then you won't need the nsAutoPtr at all.

@@ +235,5 @@
> +      return false;
> +    }
> +
> +    size = JS_GetTypedArrayByteLength(obj, cx);
> +    if (size == 0) {

Nit: |if (!size) {|

@@ +239,5 @@
> +    if (size == 0) {
> +      JS_ReportError(cx, "Typed array byte length is zero");
> +      return false;
> +    }
> +    data = JS_GetArrayBufferViewData(obj, cx);

Nit: Newline above here.

@@ +240,5 @@
> +      JS_ReportError(cx, "Typed array byte length is zero");
> +      return false;
> +    }
> +    data = JS_GetArrayBufferViewData(obj, cx);
> +    if (data == NULL) {

Nit: |if (!data) {|

@@ +523,5 @@
>  
>  nsresult
> +SystemWorkerManager::InitNetd(JSContext *cx)
> +{
> +#if 1

Nit: Please remove this.

::: ipc/netd/Netd.cpp
@@ +17,5 @@
> +#include "nsThreadUtils.h"
> +
> +#include "base/message_loop.h"
> +#include "mozilla/FileUtils.h"
> +#include "Netd.h"

Please always make sure to #include the header for your cpp file first so that you know your header can compile on its own.

@@ +148,5 @@
> +void
> +NetdClient::OnFileCanReadWithoutBlocking(int aFd)
> +{
> +  ssize_t length = 0;
> +  ssize_t pos = 0;

I don't think you need pos any longer. Just replace all |pos| with |length| here.

@@ +153,5 @@
> +
> +  MOZ_ASSERT(aFd == mSocket.get());
> +  while (true) {
> +    errno = 0;
> +    length = read(aFd, &mReceiveBuffer[mReceivedIndex], MAX_COMMAND_SIZE - mReceivedIndex);

Before calling read let's also assert mReceivedIndex < MAX_COMMAND_SIZE.

@@ +170,5 @@
> +      Restart();
> +      return;
> +    }
> +    // It is possible that length may less than zero and triggers the assert.
> +    // So we add assert after confirming that length is greater than zero.

Hm. I don't understand. If length is less that zero then it should still be less than (MAX_COMMAND_SIZE - mReceivedIndex) so I don't see how it could trigger the assertion. Let's move this right after the read call.

@@ +182,5 @@
> +        // integer response code followed by the rest of the line.
> +        // Fish out the response code.
> +        errno = 0;
> +        int responseCode = strtol(mReceiveBuffer, nullptr, 10);
> +        if (!responseCode || errno < 0 ) {

Can errno be a positive number? I think you really want |if (!responseCode || errno)|

@@ +186,5 @@
> +        if (!responseCode || errno < 0 ) {
> +          LOG("Can't parse netd's response: %d (%s)", errno, strerror(errno));
> +          // There is data in the receive buffer beyond the current line.
> +          // Shift it down to the beginning.
> +          RESET_RECEIVE_BUFFER(pos);

Ok, I don't think a macro is a good solution to this problem. We can just restructure this part a little and avoid it entirely. Come find me this week and we can talk about it :)

@@ +191,5 @@
> +        }
> +
> +        // TODO, Bug 783966, handle InterfaceChange(600) and BandwidthControl(601).
> +        if (responseCode < 600) {
> +          NetdCommand* response(new NetdCommand());

Nit: |NetdCommand* response = new NetdCommand();|

@@ +234,5 @@
> +  Start();
> +}
> +
> +void
> +NetdClient::Start()

Nit: Add a comment so that it's obvious this is a static method, like this:

  // static
  void
  NetdClient::Start()

@@ +293,5 @@
> +
> +  while (mCurrentWriteOffset < mCurrentNetdCommand->mSize) {
> +    ssize_t write_amount = mCurrentNetdCommand->mSize - mCurrentWriteOffset;
> +    ssize_t written = write(mSocket.get(), mCurrentNetdCommand->mData +
> +                            mCurrentWriteOffset, write_amount);

Nit: It's more obvious that there are three args here if you do this:

  ssize_t written = write(mSocket.get(),
                          mCurrentNetdCommand->mData + mCurrentWriteOffset,
                          write_amount);

That way you don't separate the two numbers that you're adding.

@@ +337,5 @@
> +  // Assign rndis address for usb tethering in ICS.
> +  if (atoi(propValue) >= 15) {
> +    result = InitRndisAddress();
> +    // We don't return here because InitRnsisAddress() function is related to
> +    // usb tethering only. Others service such as wifi tethering still needs

Nit: s/needs/need/

@@ +377,5 @@
> +  XRE_GetIOMessageLoop()->PostTask(
> +    FROM_HERE,
> +    NewRunnableFunction(ShutdownNetdIOThread));
> +
> +  gNetdConsumer = nullptr;

This isn't safe. The IO thread could be just about to send a message using this pointer and then it would crash.

::: ipc/netd/Netd.h
@@ +10,5 @@
> +namespace mozilla {
> +namespace ipc {
> +
> +#define MAX_COMMAND_SIZE  4096
> +#define MAX_RECONNECT_TIMES 10

Nit: Please move MAX_COMMAND_SIZE above the namespace stuff, and please move MAX_RECONNECT_TIMES to the cpp file.

@@ +40,5 @@
> +                   public RefCounted<NetdClient>
> +{
> +public:
> +  typedef std::queue<NetdCommand*> NetdCommandQueue;
> +  NetdClient();

Nit: Newline above here to separate typedef from methods.

@@ +51,5 @@
> +  void Restart();
> +  virtual void OnFileCanReadWithoutBlocking(int aFd);
> +  virtual void OnFileCanWriteWithoutBlocking(int aFd);
> +  bool OpenSocket();
> +  MessageLoopForIO *mIOLoop;

Nit: newline above here to separate the methods from the members.

@@ +65,5 @@
> +};
> +
> +void StartNetd(NetdConsumer *);
> +void StopNetd();
> +void SendNetdCommand(NetdCommand *);

Nit: newline after here.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 101 • 12 years ago

Fix nits in comment 100.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 102 • 12 years ago

Remove trail blank

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 103 • 12 years ago

1. unbitrot and rebase the patch.
2. add hg header.

	Mozilla RelEng Bot
	Comment 105 • 12 years ago

Try run for 9d6973576cb4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9d6973576cb4
Results (out of 250 total builds):
    exception: 1
    success: 226
    warnings: 22
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vchang@mozilla.com-9d6973576cb4

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 106 • 12 years ago

Comment on attachment 656590 [details] [diff] [review]
Updated IPC interface implementation V6.2

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

This looks great! Thanks for all your hard work :)

	Philipp von Weitershausen [:philikon]
	Comment 107 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/15f7b4bfd699
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fda173deaee
https://hg.mozilla.org/integration/mozilla-inbound/rev/100d96d3a274

	Ed Morley [:emorley]
	Comment 108 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/15f7b4bfd699
https://hg.mozilla.org/mozilla-central/rev/7fda173deaee
https://hg.mozilla.org/mozilla-central/rev/100d96d3a274

	Gene Lian (I already quit Mozilla)
	Comment 109 • 12 years ago

Comment on attachment 656593 [details] [diff] [review]
Rebase and merge setWifiTethering function in nsIWifi interface V3.1

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

::: dom/wifi/WifiWorker.js
@@ +2143,5 @@
> +  },
> +
> +  setWifiEnabledInternal: function(enable, callback) {
> +    this.setWifiEnabled({enabled: enable, callback: callback});
> +  },

This part is very weird to me... Please check the .setWifiEnabled(). Shouldn't we call it as below?

this.setWifiEnabled({data: enable, callback: callback});

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 110 • 12 years ago

Fix the nit based on comment 109

	Gene Lian (I already quit Mozilla)
	Comment 111 • 12 years ago

(In reply to Vincent Chang from comment #110)
> Created attachment 657215 [details] [diff] [review]
> Bug fix
> 
> Fix the nit based on comment 109

This one is going to be fixed in the https://bugzilla.mozilla.org/show_bug.cgi?id=729877#c16 as well, which doesn't use the |data| field at all since we won't get message from content processes any more. Now the element type in the request queue would be:

{enabled: XXX, callback: XXX}

where the |callback| is optional, depending on whether the element is added from tethering or not.

	Albert [:albert]
	Comment 112 • 12 years ago

Regarding to netd implementation I see that 1xx response codes are not handled ok. These codes mean that response has not finished and it will continue in following messages. Instead of that, 1xx response code is processed as an error. So, when I launch the "interface list" command I get the following:


Network Worker: Preparing to send 'interface list' command...
Network Worker: Sending 'interface list' command to netd.
Network Worker: Receiving 'interface list' command response from netd.
Network Worker:           ==> Code: 110  Reason: lo
Network Worker: Receiving 'interface list' command response from netd.
Network Worker:           ==> Code: 110  Reason: rmnet0
...
Network Worker: Receiving 'interface list' command response from netd.
Network Worker:           ==> Code: 110  Reason: ip6tnl0
Network Worker: Receiving 'interface list' command response from netd.
Network Worker:           ==> Code: 200  Reason: Interface list completed
Network Worker: {"cmd":"getNetworkInterfaceStats","ifname":"wlan0","connType":0,"report":true,"isAsync":true,"id":"","rxBytes":-1,"txBytes":-1,"date":"2012-09-03T09:19:39.643Z","resultCode":200,"resultReason":"Interface list completed\u0000","error":true,"state":"isIfaceAvailable"}

Is this going to be solve?

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 113 • 12 years ago

(In reply to Albert from comment #112)
> Regarding to netd implementation I see that 1xx response codes are not
> handled ok. These codes mean that response has not finished and it will
> continue in following messages. Instead of that, 1xx response code is
> processed as an error. So, when I launch the "interface list" command I get
> the following:
> 
> 
> Network Worker: Preparing to send 'interface list' command...
> Network Worker: Sending 'interface list' command to netd.
> Network Worker: Receiving 'interface list' command response from netd.
> Network Worker:           ==> Code: 110  Reason: lo
> Network Worker: Receiving 'interface list' command response from netd.
> Network Worker:           ==> Code: 110  Reason: rmnet0
> ...
> Network Worker: Receiving 'interface list' command response from netd.
> Network Worker:           ==> Code: 110  Reason: ip6tnl0
> Network Worker: Receiving 'interface list' command response from netd.
> Network Worker:           ==> Code: 200  Reason: Interface list completed
> Network Worker:
> {"cmd":"getNetworkInterfaceStats","ifname":"wlan0","connType":0,"report":
> true,"isAsync":true,"id":"","rxBytes":-1,"txBytes":-1,"date":"2012-09-03T09:
> 19:39.643Z","resultCode":200,"resultReason":"Interface list
> completed\u0000","error":true,"state":"isIfaceAvailable"}
> 
> Is this going to be solve?

I filed Bug 788081 - Support 1xx response message from netd to address this issue. 
Let us discuss this issue there.