Updated process::subprocess to replace environment. Review: https://reviews.apache.org/r/35561
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/14b49f31 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/14b49f31 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/14b49f31 Branch: refs/heads/master Commit: 14b49f31840ff1523b31007c21b12c604700323f Parents: 870ee5d Author: Benjamin Hindman <[email protected]> Authored: Sun Jun 14 11:17:32 2015 -0700 Committer: Benjamin Hindman <[email protected]> Committed: Wed Jun 24 17:27:24 2015 -0700 ---------------------------------------------------------------------- .../libprocess/include/process/subprocess.hpp | 11 ++--- 3rdparty/libprocess/src/subprocess.cpp | 42 ++++++++++++++------ .../libprocess/src/tests/subprocess_tests.cpp | 7 ++-- 3 files changed, 40 insertions(+), 20 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/14b49f31/3rdparty/libprocess/include/process/subprocess.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/include/process/subprocess.hpp b/3rdparty/libprocess/include/process/subprocess.hpp index 425b119..869e3d9 100644 --- a/3rdparty/libprocess/include/process/subprocess.hpp +++ b/3rdparty/libprocess/include/process/subprocess.hpp @@ -205,8 +205,9 @@ private: * @param out Redirection specification for stdout. * @param err Redirection specification for stderr. * @param flags Flags to be stringified and appended to 'argv'. - * @param environment Environment variables to add or overwrite - * existing environment variables. + * @param environment Environment variables to use for the new + * subprocess or if None (the default) then the new subprocess + * will inherit the environment of the current process. * @param setup Function to be invoked after forking but before * exec'ing. NOTE: Take extra care not to invoke any * async unsafe code in the body of this function. @@ -214,7 +215,6 @@ private: * subprocess. * @return The subprocess or an error if one occured. */ -// TODO(dhamon): Add an option to not combine the two environments. Try<Subprocess> subprocess( const std::string& path, std::vector<std::string> argv, @@ -239,8 +239,9 @@ Try<Subprocess> subprocess( * @param in Redirection specification for stdin. * @param out Redirection specification for stdout. * @param err Redirection specification for stderr. - * @param environment Environment variables to add or overwrite - * existing environment variables. + * @param environment Environment variables to use for the new + * subprocess or if None (the default) then the new subprocess + * will inherit the environment of the current process. * @param setup Function to be invoked after forking but before * exec'ing. NOTE: Take extra care not to invoke any * async unsafe code in the body of this function. http://git-wip-us.apache.org/repos/asf/mesos/blob/14b49f31/3rdparty/libprocess/src/subprocess.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/subprocess.cpp b/3rdparty/libprocess/src/subprocess.cpp index f41f5e2..5b41f0d 100644 --- a/3rdparty/libprocess/src/subprocess.cpp +++ b/3rdparty/libprocess/src/subprocess.cpp @@ -20,8 +20,6 @@ #include <stout/try.hpp> #include <stout/unreachable.hpp> -#include <stout/os/execenv.hpp> - using std::map; using std::string; using std::vector; @@ -110,7 +108,7 @@ static int childMain( const Subprocess::IO& in, const Subprocess::IO& out, const Subprocess::IO& err, - os::ExecEnv* envp, + char** envp, const Option<lambda::function<int()>>& setup, int stdinFd[2], int stdoutFd[2], @@ -160,7 +158,7 @@ static int childMain( } } - os::execvpe(path.c_str(), argv, (*envp)()); + os::execvpe(path.c_str(), argv, envp); ABORT(string("Failed to os::execvpe in childMain: ") + strerror(errno)); } @@ -315,19 +313,32 @@ Try<Subprocess> subprocess( // The real arguments that will be passed to 'os::execvpe'. We need // to construct them here before doing the clone as it might not be - // async signal safe. + // async signal safe to perform the memory allocation. char** _argv = new char*[argv.size() + 1]; for (int i = 0; i < argv.size(); i++) { _argv[i] = (char*) argv[i].c_str(); } _argv[argv.size()] = NULL; - // We need to do this construction before doing the clone 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. - os::ExecEnv envp(environment.get(map<string, string>())); + // Like above, we need to construct the environment that we'll pass + // to 'os::execvpe' as it might not be async-safe to perform the + // memory allocations. + char** envp = os::environ(); + + if (environment.isSome()) { + // NOTE: We add 1 to the size for a NULL terminator. + envp = new char*[environment.get().size() + 1]; + + size_t index = 0; + foreachpair (const string& key, const string& value, environment.get()) { + string entry = key + "=" + value; + envp[index] = new char[entry.size() + 1]; + strncpy(envp[index], entry.c_str(), entry.size() + 1); + ++index; + } + + envp[index] = NULL; + } // Determine the function to clone the child process. If the user // does not specify the clone function, we will use the default. @@ -342,7 +353,7 @@ Try<Subprocess> subprocess( in, out, err, - &envp, + envp, setup, stdinFd, stdoutFd, @@ -350,6 +361,13 @@ Try<Subprocess> subprocess( delete[] _argv; + // Need to delete 'envp' if we had environment variables passed to + // us and we needed to allocate the space. + if (environment.isSome()) { + CHECK_NE(os::environ(), envp); + delete[] envp; + } + if (pid == -1) { // Save the errno as 'close' below might overwrite it. ErrnoError error("Failed to clone"); http://git-wip-us.apache.org/repos/asf/mesos/blob/14b49f31/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 b5cfc8d..348b22d 100644 --- a/3rdparty/libprocess/src/tests/subprocess_tests.cpp +++ b/3rdparty/libprocess/src/tests/subprocess_tests.cpp @@ -756,13 +756,14 @@ TEST_F(SubprocessTest, EnvironmentOverride) Clock::pause(); // Ensure we override an existing environment variable. - os::setenv("MESSAGE", "hello"); + os::setenv("MESSAGE1", "hello"); + os::setenv("MESSAGE2", "world"); map<string, string> environment; - environment["MESSAGE"] = "goodbye"; + environment["MESSAGE2"] = "goodbye"; Try<Subprocess> s = subprocess( - "echo $MESSAGE", + "echo $MESSAGE1 $MESSAGE2", Subprocess::PIPE(), Subprocess::PIPE(), Subprocess::PIPE(),
