Getting service process on Mac to handle having things moved/changed underneath it.

chrome/common/launchd_mac.mm

Index: chrome/common/launchd_mac.mm
diff --git a/chrome/common/launchd_mac.mm b/chrome/common/launchd_mac.mm
new file mode 100644
index 0000000000000000000000000000000000000000..ce3a319ee3416523060549362c831d9d9646123f
--- /dev/null
+++ b/chrome/common/launchd_mac.mm
@@ -0,0 +1,189 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/common/launchd_mac.h"
+
+#import <Foundation/Foundation.h>
+#include <launch.h>
+
+#include "base/mac/mac_util.h"
+#include "base/mac/scoped_cftyperef.h"
+#include "base/mac/scoped_nsautorelease_pool.h"
+#include "base/process_util.h"
+#include "base/stringprintf.h"
+#include "base/sys_string_conversions.h"
+#include "third_party/GTM/Foundation/GTMServiceManagement.h"
+
+namespace {
+
+std::string FileSystemPathFromCFURL(CFURLRef url) {
+  base::mac::ScopedCFTypeRef<CFStringRef> path(
+      CFURLCopyFileSystemPath(url, kCFURLPOSIXPathStyle));

[Mark Mentovai, 2011/03/21 16:50:49]

Might path be NULL? If url is bad in some way, like not being a file URL,
perhaps?

[dmac, 2011/03/21 22:59:19]

Obsolete.

+
+  // TODO(dmaclach): Turn this into a more general utility

[Mark Mentovai, 2011/03/21 16:50:49]

This comment applies to the entire function, right? Put it at the top of the
“meat” inside the {braces}.

[dmac, 2011/03/21 22:59:19]

Obsolete.

+  // http://crbug.com/76928
+  CFIndex max = CFStringGetMaximumSizeOfFileSystemRepresentation(path);

[Mark Mentovai, 2011/03/21 16:50:49]

I’d just use -[NSString fileSystemRepresentation] here so you don’t need to
worry about your own buffer.

  return std::string([(NSString*)path.get() fileSystemRepresentation]);

You may need a ScopedNSAutoreleasePool to get away with this depending on where
it runs.

(The fact that we already have a one-line way to do this provided by Foundation
is why I figured your TODO applied to the entire CFURL-to-string function and
not just the CFString-to-string portion, but I see your bug is titled for the
CFString-to-string conversion, so I think it’d be invalid/wontfix as-is. If you
want a CFURL-to-string utility function in something like mac_util, that might
be a different story.)

[dmac, 2011/03/21 22:59:19]

Obsolete.

+  std::vector<char> buffer(max);
+  CHECK(CFStringGetFileSystemRepresentation(path, &buffer[0], max));
+  return std::string(&buffer[0]);
+}
+
+CFURLRef CopyPListURL(Launchd::Domain domain,

[Mark Mentovai, 2011/03/21 16:50:49]

CopyPlistURL, lowercase L, as you’ve done elsewhere.

[dmac, 2011/03/21 22:59:19]

Done.

+                      Launchd::Type type,
+                      CFStringRef name) {
+  base::mac::ScopedNSAutoreleasePool pool;
+  NSArray* library_paths = NSSearchPathForDirectoriesInDomains(
+      NSLibraryDirectory,
+      domain,

[Mark Mentovai, 2011/03/21 16:50:49]

Come up with a classier way to break these lines up. How about:

  NSArray* library_paths =
      NSSearchPathForDirectoriesInDomains(NSLibraryDirectory, domain, YES);

Note the use of YES in a Foundation call instead of true. It makes no difference
here in the object code, but you should be in the habit of using YES when the
prototype says BOOL.

[dmac, 2011/03/21 22:59:19]

Done.

+      true);
+  DCHECK_EQ([library_paths count], 1U);
+  NSString* library_path = [library_paths objectAtIndex:0];
+
+  NSString *launch_dir_name = (type == Launchd::Daemon) ? @"LaunchDaemons"
+                                                        : @"LaunchAgents";
+  NSString* launch_dir =
+    [library_path stringByAppendingPathComponent:launch_dir_name];

[Mark Mentovai, 2011/03/21 16:50:49]

4-space the continuation line.

[dmac, 2011/03/21 22:59:19]

Done.

+
+  NSError* err;
+  if (![[NSFileManager defaultManager] createDirectoryAtPath:launch_dir
+                                 withIntermediateDirectories:YES
+                                                  attributes:nil
+                                                       error:&err]) {
+    LOG(ERROR) << "CopyPListURL " << base::mac::NSToCFCast(err);
+    return NULL;
+  }
+
+  NSString* plist_file_path

[Mark Mentovai, 2011/03/21 16:50:49]

= goes on this line, not the next.

[dmac, 2011/03/21 22:59:19]

I hate that. Done.

+      = [launch_dir stringByAppendingPathComponent:base::mac::CFToNSCast(name)];
+  plist_file_path = [plist_file_path stringByAppendingPathExtension:@"plist"];
+  NSURL *url = [[NSURL alloc] initFileURLWithPath:plist_file_path

[Mark Mentovai, 2011/03/21 16:50:49]

You’ve been putting the *s on the type and not the variable name, stick with
that for consistency.

[dmac, 2011/03/21 22:59:19]

Done.

+                                      isDirectory:NO];
+  return base::mac::NSToCFCast(url);
+}
+
+}

[Mark Mentovai, 2011/03/21 16:50:49]

}  // namespace

[dmac, 2011/03/21 22:59:19]

Done.

+
+Launchd* Launchd::g_instance_ = NULL;
+bool Launchd::g_set_to_singleton_ = false;

[Mark Mentovai, 2011/03/21 16:50:49]

I don’t see why you need this.

[dmac, 2011/03/21 22:59:19]

Done.

+
+Launchd* Launchd::GetInstance() {
+  if (!g_instance_) {
+    g_instance_ = Singleton<Launchd>::get();
+    g_set_to_singleton_ = true;
+  }
+  return g_instance_;
+}
+
+void Launchd::SetInstance(Launchd* instance) {
+  CHECK(!g_set_to_singleton_);
+  g_instance_ = instance;
+}
+
+Launchd::~Launchd() { }
+
+CFDictionaryRef Launchd::CopyLaunchdExports() {
+  return GTMCopyLaunchdExports();

[Mark Mentovai, 2011/03/21 16:50:49]

I haven’t read this code or its API documentation, I’m assuming it’s correct.
Same through RemoveLaunchdJob.

[dmac, 2011/03/21 22:59:19]

OK.. it's been working for a while now.

+}
+
+CFDictionaryRef Launchd::CopyLaunchdJobDictionary(CFStringRef label) {
+  return GTMSMJobCopyDictionary(label);
+}
+
+CFDictionaryRef Launchd::CopyLaunchdDictionaryByCheckingIn(CFErrorRef* error) {
+  return GTMSMJobCheckIn(error);
+}
+
+bool Launchd::RemoveLaunchdJob(CFStringRef label, CFErrorRef* error) {
+  return GTMSMJobRemove(label, error);
+}
+
+bool Launchd::ShutdownLaunchdJob(Domain domain,
+                                 Type type,
+                                 CFStringRef name,
+                                 CFStringRef cf_session_type) {
+  base::mac::ScopedCFTypeRef<CFURLRef> cf_url(CopyPListURL(domain, type, name));
+  std::string path(FileSystemPathFromCFURL(cf_url));

[Mark Mentovai, 2011/03/21 16:50:49]

Isn’t it kind of obnoxious to use CopyPlistURL, which deals with CFStrings and
then turns it into a CFURL at the last moment, and then have to get a string
back from the CFURL by calling FileSystemPathFromCFURL?

[dmac, 2011/03/21 22:59:19]

Done. It made sense in the earlier CL because some places it was used as a
CFURLRef and others as a NSURL. Recent changes tipped the balance back to NSURL.

+  std::string session_type(base::SysCFStringRefToUTF8(cf_session_type));
+  std::vector<std::string> argv;
+  argv.push_back("/bin/launchctl");
+  argv.push_back("unload");
+  argv.push_back("-S");
+  argv.push_back(session_type);
+  argv.push_back(path);
+  base::ProcessHandle handle;
+  return base::LaunchAppInNewProcessGroup(argv,
+                                          base::environment_vector(),
+                                          base::file_handle_mapping_vector(),
+                                          NO,
+                                          &handle);
+}
+
+bool Launchd::RestartLaunchdJob(Domain domain,
+                                Type type,
+                                CFStringRef name,
+                                CFStringRef cf_session_type) {
+  base::mac::ScopedCFTypeRef<CFURLRef> cf_url(CopyPListURL(domain, type, name));
+  std::string path(FileSystemPathFromCFURL(cf_url));
+  std::string session_type(base::SysCFStringRefToUTF8(cf_session_type));
+  std::vector<std::string> argv;
+  argv.push_back("/bin/bash");
+  argv.push_back("--noprofile");
+  argv.push_back("-c");
+  std::string command = base::StringPrintf(
+      "/bin/launchctl unload -S %s \"%s\";/bin/launchctl load -S %s \"%s\";",
+      session_type.c_str(), path.c_str(),

[Mark Mentovai, 2011/03/21 16:50:49]

Didn’t I mention doing proper sanitization and escaping on shell arguments in a
previous review? You need to do that for all arguments here.

+      session_type.c_str(), path.c_str());
+  argv.push_back(command);
+  base::ProcessHandle handle;
+  return base::LaunchAppInNewProcessGroup(argv,
+                                          base::environment_vector(),
+                                          base::file_handle_mapping_vector(),
+                                          NO,
+                                          &handle);
+}
+
+CFMutableDictionaryRef Launchd::ReadLaunchdPlist(Domain domain,
+                                                 Type type,
+                                                 CFStringRef name) {
+  base::mac::ScopedNSAutoreleasePool pool;
+  base::mac::ScopedCFTypeRef<CFURLRef> cf_url(CopyPListURL(domain, type, name));
+  NSURL* ns_url = base::mac::CFToNSCast(cf_url);
+  NSMutableDictionary* plist

[Mark Mentovai, 2011/03/21 16:50:49]

= goes on this line.

+     = [[NSMutableDictionary alloc] initWithContentsOfURL:ns_url];

[Mark Mentovai, 2011/03/21 16:50:49]

Continuation line uses a 4-space indent.

[Mark Mentovai, 2011/03/21 16:50:49]

OK, I see, the caller owns the returned dictionary. Rename the function or
document the ownership in the header (as I mentioned in the comment I made in
the header).

+  return base::mac::NSToCFCast(plist);
+}
+
+bool Launchd::WriteLaunchdPlist(Domain domain,
+                                Type type,
+                                CFStringRef name,
+                                CFDictionaryRef dict) {
+  base::mac::ScopedNSAutoreleasePool pool;
+  base::mac::ScopedCFTypeRef<CFURLRef> cf_url(CopyPListURL(domain, type, name));
+  base::mac::ScopedCFTypeRef<CFDataRef> cf_data(
+      CFPropertyListCreateXMLData(kCFAllocatorDefault,
+                                  dict));

[Mark Mentovai, 2011/03/21 16:50:49]

dict fits on the previous line for sure.

+  Boolean good = false;

[Mark Mentovai, 2011/03/21 16:50:49]

Just bool, not Boolean.

+  if (cf_url.get() && cf_data.get()) {
+    SInt32 error = 0;
+    good = CFURLWriteDataAndPropertiesToResource(cf_url, cf_data, NULL, &error);
+    if (!good) {
+      LOG(ERROR) << "WriteLaunchdPlist " << error;
+    }
+  }
+  return good;
+}
+
+bool Launchd::DeleteLaunchDPlist(Domain domain, Type type, CFStringRef name) {
+  base::mac::ScopedCFTypeRef<CFURLRef> url(CopyPListURL(domain, type, name));
+  if (!url) {
+    return false;
+  }
+  SInt32 error = 0;
+  if (!CFURLDestroyResource(url, &error)) {
+    LOG(ERROR) << "DeleteLaunchDPlist: " << error;
+    return false;
+  }
+  return true;
+}
+

[Mark Mentovai, 2011/03/21 16:50:49]

Dump the blank line at EOF.