Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9837031:
Implement DaemonController Start(), Stop() and GetStatus() for Mac. (Closed)
|
Created:
8 years, 9 months ago by Lambros Modified:
8 years, 8 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement DaemonController Start(), Stop() and GetStatus() for Mac.
BUG=None
TEST=Start/stop the service from the Web UI
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129676
Patch Set 1 #
Total comments: 17
Patch Set 2 : Fixes from review #Patch Set 3 : Rebase onto new DaemonController API, and implement load/save config. #Patch Set 4 : Change service string #
Total comments: 12
Patch Set 5 : Fix nits. #Messages
Total messages: 12 (0 generated)
sergeyu: Please have a look at my use of threading. garykac: FYI. Please tell me what script location and service name you want. jamiewalch: Everything else. http://codereview.chromium.org/9837031/diff/1/remoting/remoting.gyp File remoting/remoting.gyp (right): http://codereview.chromium.org/9837031/diff/1/remoting/remoting.gyp#newcode1078 remoting/remoting.gyp:1078: 'remoting_host_plugin', remoting_unittests seems to also need the ServiceManagement framework (which the plugin now needs).
I'm about ready to sign off on this, once my comments have been addressed, but I'd like someone with a bit more Mac experience to review the launchd stuff, particularly the calls in StopService. http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... File remoting/host/plugin/daemon_controller_mac.cc (right): http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... remoting/host/plugin/daemon_controller_mac.cc:26: #define kServiceName "com.google.chrome_remote_desktop" Any reason not to make this a const char[]? http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... remoting/host/plugin/daemon_controller_mac.cc:57: base::Thread root_thread_; I think auth_thread might be a better name, since it doesn't actually run as root. http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... remoting/host/plugin/daemon_controller_mac.cc:86: return remoting::DaemonController::STATE_NOT_INSTALLED; Until we have an installation story, I think you should return NOT_IMPLEMENTED here, with a TODO to fix it up later. Otherwise the web-app will display the "start" UI but you won't be able to do anything with it. http://codereview.chromium.org/9837031/diff/1/remoting/remoting.gyp File remoting/remoting.gyp (right): http://codereview.chromium.org/9837031/diff/1/remoting/remoting.gyp#newcode1078 remoting/remoting.gyp:1078: 'remoting_host_plugin', On 2012/03/23 02:06:11, Lambros wrote: > remoting_unittests seems to also need the ServiceManagement framework (which the > plugin now needs). It doesn't seem right to link in the entire host plugin just to pull in a framework dependency. Also, if this dependency is new, I don't understand how the unit test can need this framework, since only the new dependency needs it. Am I missing something?
http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... File remoting/host/plugin/daemon_controller_mac.cc (right): http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... remoting/host/plugin/daemon_controller_mac.cc:35: const char kStartStopTool[] = "/Library/PrivilegedHelperTools/me2me.sh"; Since this is a system directory that includes files from other apps/services, we should add our 'com.google.chrome_remote_desktop." prefix to this file (and any other file we add here). I've already done this with the files used in the installer.
http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... File remoting/host/plugin/daemon_controller_mac.cc (right): http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... remoting/host/plugin/daemon_controller_mac.cc:57: base::Thread root_thread_; On 2012/03/23 03:24:05, Jamie wrote: > I think auth_thread might be a better name, since it doesn't actually run as > root. +1 http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... remoting/host/plugin/daemon_controller_mac.cc:106: base::Bind(&DaemonControllerMac::DoStart, base::Unretained(this))); Add comment that it is safe to use base::Unretained() because this object owns the thread, and so it always outlives it. http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... remoting/host/plugin/daemon_controller_mac.cc:143: const char* arguments[] = {command, NULL}; nit: add spaces after { and before }. http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... remoting/host/plugin/daemon_controller_mac.cc:165: launch_data_t message = launch_data_alloc(LAUNCH_DATA_DICTIONARY); Maybe move ScopedLaunchData from chrome/browser/mac to base/mac and use it here? (shouldn't block this CL).
http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... File remoting/host/plugin/daemon_controller_mac.cc (right): http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... remoting/host/plugin/daemon_controller_mac.cc:26: #define kServiceName "com.google.chrome_remote_desktop" On 2012/03/23 03:24:05, Jamie wrote: > Any reason not to make this a const char[]? The CFSTR() macro seems to need a literal string. The same situation occurs elsewhere in Chrome, for example, chrome/common/service_process_util_mac.mm so I guess #define is OK. http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... remoting/host/plugin/daemon_controller_mac.cc:35: const char kStartStopTool[] = "/Library/PrivilegedHelperTools/me2me.sh"; On 2012/03/23 16:28:29, garykac wrote: > Since this is a system directory that includes files from other apps/services, > we should add our 'com.google.chrome_remote_desktop." prefix to this file (and > any other file we add here). > > I've already done this with the files used in the installer. Done. http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... remoting/host/plugin/daemon_controller_mac.cc:57: base::Thread root_thread_; On 2012/03/23 17:56:48, sergeyu wrote: > On 2012/03/23 03:24:05, Jamie wrote: > > I think auth_thread might be a better name, since it doesn't actually run as > > root. > > +1 Done. http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... remoting/host/plugin/daemon_controller_mac.cc:86: return remoting::DaemonController::STATE_NOT_INSTALLED; On 2012/03/23 03:24:05, Jamie wrote: > Until we have an installation story, I think you should return NOT_IMPLEMENTED > here, with a TODO to fix it up later. Otherwise the web-app will display the > "start" UI but you won't be able to do anything with it. Done. http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... remoting/host/plugin/daemon_controller_mac.cc:106: base::Bind(&DaemonControllerMac::DoStart, base::Unretained(this))); On 2012/03/23 17:56:48, sergeyu wrote: > Add comment that it is safe to use base::Unretained() because this object owns > the thread, and so it always outlives it. Done. http://codereview.chromium.org/9837031/diff/1/remoting/host/plugin/daemon_con... remoting/host/plugin/daemon_controller_mac.cc:143: const char* arguments[] = {command, NULL}; On 2012/03/23 17:56:48, sergeyu wrote: > nit: add spaces after { and before }. Done. http://codereview.chromium.org/9837031/diff/1/remoting/remoting.gyp File remoting/remoting.gyp (right): http://codereview.chromium.org/9837031/diff/1/remoting/remoting.gyp#newcode1078 remoting/remoting.gyp:1078: 'remoting_host_plugin', > It doesn't seem right to link in the entire host plugin just to pull in a > framework dependency. Also, if this dependency is new, I don't understand how > the unit test can need this framework, since only the new dependency needs it. > Am I missing something? You're right, remoting_unittests shouldn't depend on the whole plugin. Now I've removed this line it seems to work OK. Not sure why I thought I needed this.
lgtm
PTAL. This CL is now dependent on http://codereview.chromium.org/9837098/
lgtm, but it might be worth running this by Dave or someone with more Mac experience than me. http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... File remoting/host/plugin/daemon_controller_mac.cc (right): http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... remoting/host/plugin/daemon_controller_mac.cc:104: return remoting::DaemonController::STATE_NOT_IMPLEMENTED; Nit: You're already in namespace remoting. No need to specify it here I don't think. http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... remoting/host/plugin/daemon_controller_mac.cc:174: base::JSONWriter::OPTIONS_PRETTY_PRINT, Optional: Pretty-printing doesn't seem necessary. If there's a simpler API that doesn't take an options parameter, it might be worth using that instead.
Dave, could you have a look at the Mac bits?
Some comments http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... File remoting/host/plugin/daemon_controller_mac.cc (right): http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... remoting/host/plugin/daemon_controller_mac.cc:33: #define kConfigDir "/Library/PrivilegedHelperTools/" normally we wouldn't hardcode a path like this. There isn't a easy helper for PrivilegedHelperTools that I can find, but we would normally get the Library directory using NSSearchPathForDirectoriesInDomains. http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... remoting/host/plugin/daemon_controller_mac.cc:206: base::mac::AuthorizationCreateToRunAsRoot(CFSTR(""))); No prompt? http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... remoting/host/plugin/daemon_controller_mac.cc:214: OSStatus status = base::mac::ExecuteWithPrivilegesAndWait( Doing it this way is somewhat deprecated on Lion. Please see http://atnan.com/blog/2012/02/29/modern-privileged-helper-tools-using-smjobbl... for some details. http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... remoting/host/plugin/daemon_controller_mac.cc:216: kStartStopTool, instead of running kStartStopTool directly with full privileges, would it be better to run it via sandbox-exec and control it's potential for evil? Also, possibly check to make sure that it's owner and chmod values are correct before firing it off?
http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... File remoting/host/plugin/daemon_controller_mac.cc (right): http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... remoting/host/plugin/daemon_controller_mac.cc:104: return remoting::DaemonController::STATE_NOT_IMPLEMENTED; On 2012/03/29 05:20:29, Jamie wrote: > Nit: You're already in namespace remoting. No need to specify it here I don't > think. Done. http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... remoting/host/plugin/daemon_controller_mac.cc:174: base::JSONWriter::OPTIONS_PRETTY_PRINT, On 2012/03/29 05:20:29, Jamie wrote: > Optional: Pretty-printing doesn't seem necessary. If there's a simpler API that > doesn't take an options parameter, it might be worth using that instead. Done. http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... remoting/host/plugin/daemon_controller_mac.cc:206: base::mac::AuthorizationCreateToRunAsRoot(CFSTR(""))); On 2012/03/29 14:52:41, dmac wrote: > No prompt? Added TODO. http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... remoting/host/plugin/daemon_controller_mac.cc:214: OSStatus status = base::mac::ExecuteWithPrivilegesAndWait( On 2012/03/29 14:52:41, dmac wrote: > Doing it this way is somewhat deprecated on Lion. Please see > http://atnan.com/blog/2012/02/29/modern-privileged-helper-tools-using-smjobbl... > for some details. This is non-trivial, unfortunately. The SM framework is only available in sdk 10.6 and we still use 10.5. Even then, the proposed solutions are quite elaborate. http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... remoting/host/plugin/daemon_controller_mac.cc:216: kStartStopTool, Raised bug-report and added TODO for this. On 2012/03/29 14:52:41, dmac wrote: > instead of running kStartStopTool directly with full privileges, would it be > better to run it via sandbox-exec and control it's potential for evil? > > Also, possibly check to make sure that it's owner and chmod values are correct > before firing it off?
lgtm http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... File remoting/host/plugin/daemon_controller_mac.cc (right): http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... remoting/host/plugin/daemon_controller_mac.cc:216: kStartStopTool, On 2012/03/29 18:20:44, Lambros wrote: > Raised bug-report and added TODO for this. > > On 2012/03/29 14:52:41, dmac wrote: > > instead of running kStartStopTool directly with full privileges, would it be > > better to run it via sandbox-exec and control it's potential for evil? > > > > Also, possibly check to make sure that it's owner and chmod values are correct > > before firing it off? > OK... I would consider this to be of high priority from a security standpoint. |
