git commit: Adds a "setup" function to process::subprocess.

[be...@apache.org]

Repository: mesos
Updated Branches:
  refs/heads/master 7601fd1e1 -> b23460e98


Adds a "setup" function to process::subprocess.

Adds the ability run a "setup" function to process::subprocess within
the forked child context, after fork and before exec.

NOTE: Such lambda must not contain any async unsafe code. For details
on async safety, see POSIX.1-2004 on async-signal-safe functions, also
referenced in the signal man-pages:
http://man7.org/linux/man-pages/man7/signal.7.html.

Review: https://reviews.apache.org/r/19259


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/b23460e9
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/b23460e9
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/b23460e9

Branch: refs/heads/master
Commit: b23460e982e7ac8dbb0012343194c74202ce8988
Parents: 7601fd1
Author: Till Toenshoff <to...@me.com>
Authored: Thu Apr 3 16:24:54 2014 -0700
Committer: Benjamin Hindman <be...@gmail.com>
Committed: Thu Apr 3 16:24:54 2014 -0700

----------------------------------------------------------------------
 .../libprocess/include/process/subprocess.hpp   |  18 ++-
 3rdparty/libprocess/src/subprocess.cpp          |  17 ++-
 .../libprocess/src/tests/subprocess_tests.cpp   | 143 +++++++++++++++++++
 3 files changed, 171 insertions(+), 7 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/b23460e9/3rdparty/libprocess/include/process/subprocess.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/include/process/subprocess.hpp b/3rdparty/libprocess/include/process/subprocess.hpp
index 6c51c0b..e9d7b31 100644
--- a/3rdparty/libprocess/include/process/subprocess.hpp
+++ b/3rdparty/libprocess/include/process/subprocess.hpp
@@ -8,6 +8,7 @@
 
 #include <process/future.hpp>
 
+#include <stout/lambda.hpp>
 #include <stout/memory.hpp>
 #include <stout/option.hpp>
 #include <stout/os.hpp>
@@ -39,7 +40,8 @@ private:
   Subprocess() : data(new Data()) {}
   friend Try<Subprocess> subprocess(
       const std::string& command,
-      const std::map<std::string, std::string>& environment);
+      const Option<std::map<std::string, std::string> >& environment,
+      const Option<lambda::function<int()> >& setup);
 
   struct Data
   {
@@ -65,13 +67,19 @@ private:
 };
 
 
-// Environment is combined with the OS environment and overrides where
-// necessary.
+// The Environment is combined with the OS environment and overrides
+// where necessary.
+// The setup function is run after forking but before executing the
+// command. If the return value of that setup function is non-zero,
+// then that is what the subprocess status will be;
+// status = setup && command.
+// NOTE: Take extra care about the design of the setup function as it
+// must not contain any async unsafe code.
 // TODO(dhamon): Add an option to not combine the two environments.
 Try<Subprocess> subprocess(
     const std::string& command,
-    const std::map<std::string, std::string>& environment =
-      std::map<std::string, std::string>());
+    const Option<std::map<std::string, std::string> >& environment = None(),
+    const Option<lambda::function<int()> >& setup = None());
 
 } // namespace process {
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/b23460e9/3rdparty/libprocess/src/subprocess.cpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/src/subprocess.cpp b/3rdparty/libprocess/src/subprocess.cpp
index e09c808..27a4355 100644
--- a/3rdparty/libprocess/src/subprocess.cpp
+++ b/3rdparty/libprocess/src/subprocess.cpp
@@ -105,7 +105,8 @@ Envp::~Envp()
 // Runs the provided command in a subprocess.
 Try<Subprocess> subprocess(
     const string& command,
-    const map<string, string>& environment)
+    const Option<map<string, string> >& environment,
+    const Option<lambda::function<int()> >& setup)
 {
   // Create pipes for stdin, stdout, stderr.
   // Index 0 is for reading, and index 1 is for writing.
@@ -127,7 +128,12 @@ Try<Subprocess> subprocess(
     return ErrnoError("Failed to create pipe");
   }
 
-  internal::Envp envp(environment);
+  // We need to do this construction before doing the fork as it
+  // might not be async-safe.
+  // TODO(tillt): Consider optimizing this to not pass an empty map
+  // into the constructor or even further to use execl instead of
+  // execle once we have no user supplied environment.
+  internal::Envp envp(environment.get(map<string, string>()));
 
   pid_t pid;
   if ((pid = fork()) == -1) {
@@ -160,6 +166,13 @@ Try<Subprocess> subprocess(
     os::close(stdoutPipe[1]);
     os::close(stderrPipe[1]);
 
+    if (setup.isSome()) {
+      int status = setup.get()();
+      if (status != 0) {
+        _exit(status);
+      }
+    }
+
     execle("/bin/sh", "sh", "-c", command.c_str(), (char*) NULL, envp());
 
     ABORT("Failed to execle '/bin sh -c ", command.c_str(), "'\n");

http://git-wip-us.apache.org/repos/asf/mesos/blob/b23460e9/3rdparty/libprocess/src/tests/subprocess_tests.cpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/src/tests/subprocess_tests.cpp b/3rdparty/libprocess/src/tests/subprocess_tests.cpp
index 9413ecc..8d85c05 100644
--- a/3rdparty/libprocess/src/tests/subprocess_tests.cpp
+++ b/3rdparty/libprocess/src/tests/subprocess_tests.cpp
@@ -16,6 +16,7 @@
 #include <stout/gtest.hpp>
 #include <stout/list.hpp>
 #include <stout/os/read.hpp>
+#include <stout/path.hpp>
 
 using namespace process;
 
@@ -379,3 +380,145 @@ TEST(Subprocess, environmentOverride)
   ASSERT_TRUE(WIFEXITED(status));
   ASSERT_EQ(0, WEXITSTATUS(status));
 }
+
+
+int setupChdir(const string& directory)
+{
+  // Keep everything async-signal safe.
+  if (::chdir(directory.c_str()) == -1) {
+    return errno;
+  }
+
+  return 0;
+}
+
+
+int setupSetsid(void)
+{
+  // Keep everything async-signal safe.
+  if (::setsid() == -1) {
+    return errno;
+  }
+
+  return 0;
+}
+
+
+TEST(Subprocess, setup)
+{
+  Clock::pause();
+
+  Try<string> directory = os::mkdtemp();
+  ASSERT_SOME(directory);
+
+  // chdir().
+  Try<Subprocess> s = subprocess(
+      "echo hello world > file",
+      None(),
+      lambda::bind(&setupChdir, directory.get()));
+
+  ASSERT_SOME(s);
+
+  // Advance time until the internal reaper reaps the subprocess.
+  while (s.get().status().isPending()) {
+    Clock::advance(Seconds(1));
+    Clock::settle();
+  }
+
+  AWAIT_ASSERT_READY(s.get().status());
+  ASSERT_SOME(s.get().status().get());
+
+  // Make sure 'file' is there and contains 'hello world'.
+  const string& path = path::join(directory.get(), "file");
+  EXPECT_TRUE(os::exists(path));
+  EXPECT_SOME_EQ("hello world\n", os::read(path));
+
+  os::rmdir(directory.get());
+
+  // setsid().
+  s = subprocess(
+      "sleep 60",
+      None(),
+      lambda::bind(&setupSetsid));
+
+  ASSERT_SOME(s);
+
+  // Verify the process is in a different session.
+  EXPECT_NE(getsid(0), getsid(s.get().pid()));
+
+  kill(s.get().pid(), SIGTERM);
+
+  // Advance time until the internal reaper reaps the subprocess.
+  while (s.get().status().isPending()) {
+    Clock::advance(Seconds(1));
+    Clock::settle();
+  }
+
+  AWAIT_ASSERT_READY(s.get().status());
+  ASSERT_SOME(s.get().status().get());
+
+  int status = s.get().status().get().get();
+  ASSERT_TRUE(WIFSIGNALED(status));
+  ASSERT_EQ(SIGTERM, WTERMSIG(status));
+
+  Clock::resume();
+}
+
+
+int setupStatus(int ret)
+{
+  return ret;
+}
+
+
+TEST(Subprocess, setupStatus)
+{
+  Clock::pause();
+
+  // Exit 0 && setup 1.
+  Try<Subprocess> s = subprocess(
+      "exit 0",
+      None(),
+      lambda::bind(&setupStatus, 1));
+
+  ASSERT_SOME(s);
+
+  // Advance time until the internal reaper reaps the subprocess.
+  while (s.get().status().isPending()) {
+    Clock::advance(Seconds(1));
+    Clock::settle();
+  }
+
+  AWAIT_ASSERT_READY(s.get().status());
+  ASSERT_SOME(s.get().status().get());
+
+  int status = s.get().status().get().get();
+
+  // Verify we received the setup returned value instead of the
+  // command status.
+  ASSERT_EQ(1, WEXITSTATUS(status));
+
+  // Exit 1 && setup 0.
+  s = subprocess(
+      "exit 1",
+      None(),
+      lambda::bind(&setupStatus, 0));
+
+  ASSERT_SOME(s);
+
+  // Advance time until the internal reaper reaps the subprocess.
+  while (s.get().status().isPending()) {
+    Clock::advance(Seconds(1));
+    Clock::settle();
+  }
+
+  AWAIT_ASSERT_READY(s.get().status());
+  ASSERT_SOME(s.get().status().get());
+
+  status = s.get().status().get().get();
+
+  // Verify we received the command status.
+  ASSERT_EQ(1, WEXITSTATUS(status));
+
+  Clock::resume();
+}