Closed
Bug 1038531
Opened 9 years ago
Closed 9 years ago
Unify NetworkService/Networker/WifiNetUtil/WifiUtil
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 affected, b2g-v2.1S affected, b2g-v2.2 fixed, b2g-master fixed)
RESOLVED
FIXED
2.1 S2 (15aug)
People
(Reporter: hchang, Assigned: hchang)
References
Details
(Whiteboard: [p=10][ft:ril])
Attachments
(1 file, 8 obsolete files)
81.99 KB,
patch
|
Details | Diff | Splinter Review |
B2G has two threads to execute network commands asynchronously: 1) NetworkWorker.cpp::gWorkerThread for communicating with netd as well as calling libnetutils.so functions. 2) WifiProxyService::mControlThread for calling libnetutils.so functions (mixed with libhardware_legacy::wifi.c) The latter one is only used by wifi and can be extracted to NetworkWorker. One downside of doing this is non-netd commands would block each other while they can run concurrently. Introducing a thread pool for non-netd command might be a good solution. This bug may be going to be a meta bug if the work becomes complicated but now I intend to let it be a single bug. The action items are: 1) Put binding of libnetutils.so in WifiProxyService to NetworkService 2) Change the implementation of WifiNetUtil.jsm or replace the use of WifiNetUtil.jsm with NetworkService. 3) Bring in a thread pool to NetworkWorker for non-netd commands The removal of libnetutils.so from WifiProxyService also increase the concurrency of WifiProxyService. (wifi.c commands would be blocked by network commands for now)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → hchang
Assignee | ||
Updated•9 years ago
|
Whiteboard: [p=10][ft:ril]
Depends on: 994564
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Preliminary verified on devices. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=7ceb6648e5e0
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
The scope of this bug will be limited to WIP v2. There will be other followups for this bug: 1) Remove the indirection use of NetworkOptions. 2) Make the callback arguments more consistent for all kinds of commands. 3) Remove the NetworkService js wrapping. (TBD)
Assignee | ||
Comment 5•9 years ago
|
||
The main purpose of this patch is to move around all the network commands to a single point to execute. Originally there are two worker threads to handle network commands. One is WifiProxyService and the other is NetworkWorker. This patch moves around the network commands executed in WifiProxyService to NetworkWorker. The benefit of doing so is the network commands would no longer block wifi commands in WifiProxyService as described in bug 994564. Followings are some notes regarding this patch: 1) nsINetworkService is extended to support more commands including: - configureInterface - enableInterface - disableInterface - dhcpRequest - resetConnections These commands are originally supported in WifiProxyService [1] 2) nsINetworkService::setDefaultRouteAndDNS is reduced to simply setDefaultRoute since we want to keep nsINetworkService as simple as possible. This change requires NetworkManager to define a helper function to combine a couple of network commands instead of calling setDefaultRouteAndDNS directly. 3) Move around some properties from WifiOptions to NetworkOptions. Some duplicated name are suffixed with "long". 4) Remove the dependency of NetUtils from WifiUtils 5) Remove the copy ctor of NetworkParams since the compiler-generated member-wise copy ctor is good enough. [1] http://hg.mozilla.org/mozilla-central/file/613e79262240/dom/wifi/WifiUtils.cpp#l451
Attachment #8460121 -
Attachment is obsolete: true
Attachment #8461429 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8462323 -
Flags: review?(fabrice)
Comment 6•9 years ago
|
||
Comment on attachment 8462323 [details] [diff] [review] Patch for review Review of attachment 8462323 [details] [diff] [review]: ----------------------------------------------------------------- Very nice Henry! Can you check the comments? I didn't try the patch on device either, but I'd like to do that before landing. ::: dom/system/gonk/NetworkUtils.cpp @@ +1100,5 @@ > + // The native command refers to simply libnetutil C function calls. > + // They will be processed synchronously in the worker thread and > + // notify the callback handler registered by NetworkUtils client. > + if (ExecuteNativeCommand(aOptions)) { > + DEBUG("Native command \"%s\" executed.", NS_ConvertUTF16toUTF8(aOptions.mCmd).get()); nit: indentation @@ +1165,5 @@ > } > } > > +bool NetworkUtils::ExecuteNativeCommand(NetworkParams& aOptions) { > + NS_ConvertUTF16toUTF8 ifname = NS_ConvertUTF16toUTF8(aOptions.mIfname); NS_ConvertUTF16toUTF8 ifname(aOptions.mIfname) ? @@ +1195,5 @@ > + }; > + > + // Loop until we find the command name which matches aOptions.mCmd. > + CommandHandler handler = nullptr; > + for (size_t i = 0; i < CNT_OF_ARRAY(COMMAND_HANDLER_TABLE); i++) { Please check if we can replace CNT_OF_ARRAY by MOZ_ARRAY_LENGTH @@ +1200,5 @@ > + if (aOptions.mCmd.EqualsASCII(COMMAND_HANDLER_TABLE[i].mCommandName)) { > + handler = COMMAND_HANDLER_TABLE[i].mCommandHandler; > + break; > + } > + } Why not build a hashtable instead of the array to speed up the lookup? @@ +1433,5 @@ > + > +int32_t NetworkUtils::enableInterface(NetworkParams& aOptions, > + NetworkResultOptions& aResult) { > + NS_ConvertUTF16toUTF8 autoIfname(aOptions.mIfname); > + return mNetUtils->do_ifc_enable(autoIfname.get()); you can inline as: (also in the next two methods. return mNetUtils->do_ifc_enable(NS_ConvertUTF16toUTF8(aOptions.mIfname).get()); ::: dom/system/gonk/nsINetworkService.idl @@ +150,5 @@ > + * - gateway: long > + * - dns1: long > + * - dns2: long > + * - server: long > + * nit: trailing whitespace. @@ +284,5 @@ > * Set DNS. > * > * @param networkInterface > * The network interface which contains the DNS we want to set. > + * here too. @@ +432,5 @@ > + * Issue a DHCP client request. > + * > + * @param networkInterface > + * The network interface which we wnat to do the DHCP request on. > + * here too, and in all the following comments. ::: dom/webidl/NetworkOptions.webidl @@ +59,5 @@ > + long ipaddr; // for "ifc_configure". > + long mask; // for "ifc_configure". > + long gateway_long; // for "ifc_configure". > + long dns1_long; // for "ifc_configure". > + long dns2_long; // for "ifc_configure". nit: align the comments with the ones already there. @@ +87,5 @@ > boolean success = false; // for "setDhcpServer". > DOMString curExternalIfname = ""; // for "updateUpStream". > DOMString curInternalIfname = ""; // for "updateUpStream". > + > + // remove that empty comment
Attachment #8462323 -
Flags: review?(fabrice) → feedback+
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #6) > Comment on attachment 8462323 [details] [diff] [review] > Patch for review > > Review of attachment 8462323 [details] [diff] [review]: > ----------------------------------------------------------------- > > Very nice Henry! Can you check the comments? I didn't try the patch on > device either, but I'd like to do that before landing. > Thanks! I tried on flame and ICS emulator and everything is going well. Actually some commands are covered by wifi test cases such as dhcp request. > > + // Loop until we find the command name which matches aOptions.mCmd. > > + CommandHandler handler = nullptr; > > + for (size_t i = 0; i < CNT_OF_ARRAY(COMMAND_HANDLER_TABLE); i++) { > > Please check if we can replace CNT_OF_ARRAY by MOZ_ARRAY_LENGTH > I knew there is a template version but didn't know gecko has it! Thanks for your reminding! > @@ +1200,5 @@ > > + if (aOptions.mCmd.EqualsASCII(COMMAND_HANDLER_TABLE[i].mCommandName)) { > > + handler = COMMAND_HANDLER_TABLE[i].mCommandHandler; > > + break; > > + } > > + } > > Why not build a hashtable instead of the array to speed up the lookup? > I doubt hashing/binary search would be much faster than linear lookup on such a small size table. But I am glad to change it if you think we should do it for scalability at this time. > @@ +1433,5 @@ > > + > > +int32_t NetworkUtils::enableInterface(NetworkParams& aOptions, > > + NetworkResultOptions& aResult) { > > + NS_ConvertUTF16toUTF8 autoIfname(aOptions.mIfname); > > + return mNetUtils->do_ifc_enable(autoIfname.get()); > > you can inline as: (also in the next two methods. > return > mNetUtils->do_ifc_enable(NS_ConvertUTF16toUTF8(aOptions.mIfname).get()); > Thanks :)
Assignee | ||
Updated•9 years ago
|
Target Milestone: --- → 2.1 S2 (15aug)
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #6) > Comment on attachment 8462323 [details] [diff] [review] > Patch for review > > @@ +1195,5 @@ > > + }; > > + > > + // Loop until we find the command name which matches aOptions.mCmd. > > + CommandHandler handler = nullptr; > > + for (size_t i = 0; i < CNT_OF_ARRAY(COMMAND_HANDLER_TABLE); i++) { > > Please check if we can replace CNT_OF_ARRAY by MOZ_ARRAY_LENGTH > Unfortunately MOZ_ARRAY_LENGTH doesn't compile on ICS emulator which uses the following compiler options [1] to compile NetworkUtils.cpp. The root cause is the local type is used to instantiate the template, which is not allowed in c++03. Even thought it's supported by c++0x, gcc 4.4 still doesn't support it even with 'std=gnu++0x' option [2]. I'd like to hoist the type definition to get around this on ICS emulator. [1] /usr/bin/ccache /home/hchang/dev/b2g-emu-ics/prebuilt/linux-x86/toolchain/arm-linux-androideabi-4.4.x/bin/arm-linux-androideabi-g++ -o NetworkUtils.o -c [omitted] -std=gnu++0x [omitted] /home/hchang/dev/b2g-emu-ics/mozilla-central/dom/system/gonk/NetworkUtils.cpp [2] https://gcc.gnu.org/gcc-4.4/cxx0x_status.html
Assignee | ||
Comment 10•9 years ago
|
||
Try run: https://tbpl.mozilla.org/?tree=Try&rev=ca8cdc98eece
Attachment #8462323 -
Attachment is obsolete: true
Attachment #8464510 -
Attachment is obsolete: true
Assignee | ||
Comment 11•9 years ago
|
||
Set dependent on bug 1043114
Assignee | ||
Comment 12•9 years ago
|
||
Hi Fabrice, Just let you know I am waiting for another bug's (1043114) landing and will rebase this patch. Also, I've addressed your comments on comment 7 and 9. I will be asking for another review after rebasing. Thanks!
Flags: needinfo?(fabrice)
Comment 13•9 years ago
|
||
(In reply to Henry Chang [:henry] from comment #12) > Hi Fabrice, > > Just let you know I am waiting for another bug's (1043114) landing > and will rebase this patch. Also, I've addressed your comments on > comment 7 and 9. I will be asking for another review after rebasing. > Thanks! Sounds good, thanks!
Flags: needinfo?(fabrice)
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8465223 -
Attachment is obsolete: true
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8474350 -
Attachment is obsolete: true
Assignee | ||
Comment 16•9 years ago
|
||
Comment on attachment 8474351 [details] [diff] [review] Patch V4 Hi Fabrice, Since the dependent bug has landed, I have rebased and am requesting for review again. In this rebased patch, I remove the use of "isAsync" and the concept of "native command". All handlers can either return a result code by linux convention or a complex "NetworkResultOption" or "Pending" if the result is obtained asynchronously. Jessica, Edgar, Please feel free to leave your comments since I know you have been working on same stuff. Thanks!
Comment 17•9 years ago
|
||
(In reply to Henry Chang [:henry] from comment #16) > Comment on attachment 8474351 [details] [diff] [review] > Patch V4 > > Hi Fabrice, > > Since the dependent bug has landed, I have rebased and am > requesting for review again. In this rebased patch, I remove > the use of "isAsync" and the concept of "native command". > All handlers can either return a result code by linux > convention or a complex "NetworkResultOption" or "Pending" > if the result is obtained asynchronously. > > Jessica, Edgar, > Please feel free to leave your comments since I know you > have been working on same stuff. > > Thanks! I haven't gone into details, but the idea of getting rid of 'isAsync' sounds good to me. Thanks :)
Flags: needinfo?(jjong)
Comment 19•9 years ago
|
||
Comment on attachment 8474351 [details] [diff] [review] Patch V4 Review of attachment 8474351 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Henry, I like that a lot! You will need review from a DOM peer to land the dom/webidl changes though. I'm flagging mrbkap. ::: dom/system/gonk/NetworkUtils.cpp @@ +409,5 @@ > + mResult.mResultCode = aResultCode; > + if (aResultCode != SUCCESS) { > + // The returned value is sometimes negative, make sure we pass a positive > + // error number to strerror. > + mResult.mReason = NS_ConvertUTF8toUTF16(strerror(abs(aResultCode))); strerror is not thread safe. Can this be a problem here? stererror_r is always better. ::: dom/wifi/WifiUtils.h @@ +96,3 @@ > nsAutoPtr<WifiHotspotUtils> mWifiHotspotUtils; > > + int32_t mSdkVersion; nit: could probably be a uint32_t
Attachment #8474351 -
Flags: review?(mrbkap)
Attachment #8474351 -
Flags: review?(fabrice)
Attachment #8474351 -
Flags: review+
Comment 20•9 years ago
|
||
Comment on attachment 8474351 [details] [diff] [review] Patch V4 Review of attachment 8474351 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the webidl changes.
Attachment #8474351 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 21•9 years ago
|
||
Thanks Fabrice and Blake!
Assignee | ||
Comment 22•9 years ago
|
||
Waiting for full try run result: https://tbpl.mozilla.org/?tree=Try&rev=ef4db44a83a0
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 23•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/59611423c368
Keywords: checkin-needed
Comment 24•9 years ago
|
||
Backed out for Marionette-webapi perma-fail. https://hg.mozilla.org/integration/b2g-inbound/rev/e934b3bc1fea https://tbpl.mozilla.org/php/getParsedLog.php?id=47308241&tree=B2g-Inbound
Assignee | ||
Comment 25•9 years ago
|
||
Assignee | ||
Comment 26•9 years ago
|
||
Try run result: https://tbpl.mozilla.org/?tree=Try&rev=7d7c9bdacff4
Assignee | ||
Updated•9 years ago
|
Attachment #8474351 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8482178 -
Attachment is obsolete: true
Assignee | ||
Comment 27•9 years ago
|
||
(In reply to Henry Chang [:henry] from comment #26) > Try run result: > > https://tbpl.mozilla.org/?tree=Try&rev=7d7c9bdacff4 Marionette test failure mentioned in comment #24 is fixed.
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 28•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/767e3d6ae319
Keywords: checkin-needed
Comment 29•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/767e3d6ae319
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
status-b2g-v2.0:
--- → wontfix
status-b2g-v2.0M:
--- → wontfix
status-b2g-v2.1:
--- → fixed
status-b2g-v2.1S:
--- → fixed
status-b2g-v2.2:
--- → fixed
status-b2g-master:
--- → fixed
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•