New NaCl zygote implementation 2

base/process_util_posix.cc

Index: base/process_util_posix.cc
diff --git a/base/process_util_posix.cc b/base/process_util_posix.cc
index a2398edd0af67c71219d9c2de9dd073d79bf9584..f8014419d8a1d54a14fdb7d35fa356c17b6bf2af 100644
--- a/base/process_util_posix.cc
+++ b/base/process_util_posix.cc
@@ -508,7 +508,9 @@ bool LaunchAppImpl(
     const file_handle_mapping_vector& fds_to_remap,
     bool wait,
     ProcessHandle* process_handle,
-    bool start_new_process_group) {
+    bool start_new_process_group,
+    bool use_clone,
+    int clone_flags) {
   pid_t pid;
   InjectiveMultimap fd_shuffle1, fd_shuffle2;
   fd_shuffle1.reserve(fds_to_remap.size());
@@ -516,7 +518,16 @@ bool LaunchAppImpl(
   scoped_array<char*> argv_cstr(new char*[argv.size() + 1]);
   scoped_array<char*> new_environ(AlterEnvironment(env_changes, environ));
 
-  pid = fork();
+  if (use_clone) {
+#if defined(OS_LINUX)
+    pid = syscall(__NR_clone, clone_flags, 0, 0, 0);
+#else
+    NOTREACHED();
+    LOG(ERROR) << "Tried to use clone() on non-Linux system.";

[darin (slow to review), 2011/06/24 16:32:19]

it seems like we should only reach this code in a debug build.  i recommend
collapsing NOTREACHED and LOG(ERROR) together into just:

  NOTREACHED() << "Tried to use clone() on non-Linux system.";

i.e., there doesn't seem to be much of a benefit to shipping the string "Tried
to use clone() on non-Linux system." in release builds on other platforms.

[Brad Chen, 2011/06/24 16:52:12]

Done.

+#endif
+  } else {
+    pid = fork();
+  }
   if (pid < 0) {
     PLOG(ERROR) << "fork";
     return false;
@@ -617,7 +628,10 @@ bool LaunchApp(
     bool wait,
     ProcessHandle* process_handle) {
   return LaunchAppImpl(argv, env_changes, fds_to_remap,
-                       wait, process_handle, false);
+                       wait, process_handle,
+                       false,  // don't start new process group
+                       false,  // don't use clone()
+                       0);     // clone flags
 }
 
 bool LaunchAppInNewProcessGroup(
@@ -627,7 +641,21 @@ bool LaunchAppInNewProcessGroup(
     bool wait,
     ProcessHandle* process_handle) {
   return LaunchAppImpl(argv, env_changes, fds_to_remap, wait,
-                       process_handle, true);
+                       process_handle,
+                       true,   // start new process group
+                       false,  // don't use clone()
+                       0);     // clone flags
+}
+
+BASE_API bool LaunchAppWithClone(const std::vector<std::string>& argv,
+                                 const file_handle_mapping_vector& fds_to_remap,
+                                 bool wait, ProcessHandle* process_handle,
+                                 int clone_flags) {
+  base::environment_vector no_env;
+  return LaunchAppImpl(argv, no_env, fds_to_remap, wait, process_handle,
+                       false,  // don't start new process group
+                       true,  // use clone()

[darin (slow to review), 2011/06/24 16:32:19]

nit: elsewhere, you stack the comments in a column.  maybe it should be like
that here as well?

nit: doesn't need to be part of this CL, but generally when you end up with a
callsite that consists of a list of booleans, it is helpful to define some
enums.  that can help make the callsites self documenting so you don't need to
type out the comments and hope that they don't get out of sync with the code.

[Brad Chen, 2011/06/24 16:52:12]

Fixed.
Will fix on a separate CL.

+                       clone_flags);
 }
 
 bool LaunchApp(const std::vector<std::string>& argv,