Repository: mesos Updated Branches: refs/heads/master 878ec8ac4 -> 1ea9497fa
MESOS-3142 Refactoring os::shell - patch 1/2. Refactoring os::shell. See MESOS-3142 for more details. Review: https://reviews.apache.org/r/36978 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/8c8c239b Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/8c8c239b Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/8c8c239b Branch: refs/heads/master Commit: 8c8c239bb7cdc79d0ca56be2e0c9321333b5c115 Parents: 878ec8a Author: Marco Massenzio <[email protected]> Authored: Wed Aug 12 11:41:23 2015 -0700 Committer: Benjamin Hindman <[email protected]> Committed: Fri Aug 14 10:10:24 2015 -0700 ---------------------------------------------------------------------- .../3rdparty/stout/include/stout/os.hpp | 13 ++---- .../stout/include/stout/os/posix/shell.hpp | 48 ++++++++++++++++---- .../stout/include/stout/os/windows/shell.hpp | 2 +- .../3rdparty/stout/tests/os_tests.cpp | 45 ++++++++++++++---- 4 files changed, 82 insertions(+), 26 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/8c8c239b/3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp b/3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp index ab767cc..5141c13 100644 --- a/3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp +++ b/3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp @@ -295,14 +295,11 @@ inline Try<std::list<std::string> > find( // Creates a tar 'archive' with gzip compression, of the given 'path'. inline Try<Nothing> tar(const std::string& path, const std::string& archive) { - Try<int> status = - shell(NULL, "tar -czf %s %s", archive.c_str(), path.c_str()); - - if (status.isError()) { - return Error("Failed to archive " + path + ": " + status.error()); - } else if (status.get() != 0) { - return Error("Non-zero exit status when archiving " + path + - ": " + stringify(status.get())); + Try<std::string> tarOut = + os::shell("tar %s %s %s", "-czf", archive.c_str(), path.c_str()); + + if (tarOut.isError()) { + return Error("Failed to archive " + path + ": " + tarOut.error()); } return Nothing(); http://git-wip-us.apache.org/repos/asf/mesos/blob/8c8c239b/3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp b/3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp index 53f14e1..68fc1fd 100644 --- a/3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp +++ b/3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp @@ -27,10 +27,30 @@ namespace os { -// Runs a shell command formatted with varargs and return the return value -// of the command. Optionally, the output is returned via an argument. -// TODO(vinod): Pass an istream object that can provide input to the command. -inline Try<int> shell(std::ostream* os, const std::string fmt, ...) +/** + * Runs a shell command with optional arguments. + * + * This assumes that a successful execution will result in the exit code + * for the command to be `EXIT_SUCCESS`; in this case, the contents + * of the `Try` will be the contents of `stdout`. + * + * If the exit code is non-zero or the process was signaled, we will + * return an appropriate error message; but *not* `stderr`. + * + * If the caller needs to examine the contents of `stderr` it should + * be redirected to `stdout` (using, e.g., "2>&1 || true" in the command + * string). The `|| true` is required to obtain a success exit + * code in case of errors, and still obtain `stderr`, as piped to + * `stdout`. + * + * @param fmt the formatting string that contains the command to execute + * in the underlying shell. + * @param varargs optional arguments for `fmt`. + * + * @return the output from running the specified command with the shell; or + * an error message if the command's exit code is non-zero. + */ +inline Try<std::string> shell(const std::string fmt, ...) { va_list args; va_start(args, fmt); @@ -44,6 +64,7 @@ inline Try<int> shell(std::ostream* os, const std::string fmt, ...) } FILE* file; + std::ostringstream stdout; if ((file = popen(command.get().c_str(), "r")) == NULL) { return Error("Failed to run '" + command.get() + "'"); @@ -53,9 +74,7 @@ inline Try<int> shell(std::ostream* os, const std::string fmt, ...) // NOTE(vinod): Ideally the if and while loops should be interchanged. But // we get a broken pipe error if we don't read the output and simply close. while (fgets(line, sizeof(line), file) != NULL) { - if (os != NULL) { - *os << line; - } + stdout << line; } if (ferror(file) != 0) { @@ -68,7 +87,20 @@ inline Try<int> shell(std::ostream* os, const std::string fmt, ...) return Error("Failed to get status of '" + command.get() + "'"); } - return status; + if (WIFSIGNALED(status)) { + return Error( + "Running '" + command.get() + "' was interrupted by signal '" + + strsignal(WTERMSIG(status)) + "'"); + } else if ((WEXITSTATUS(status) != EXIT_SUCCESS)) { + LOG(ERROR) << "Command '" << command.get() + << "' failed; this is the output:\n" << stdout.str(); + return Error( + "Failed to execute '" + command.get() + "'; the command was either " + "not found or exited with a non-zero exit status: " + + stringify(WEXITSTATUS(status))); + } + + return stdout.str(); } } // namespace os { http://git-wip-us.apache.org/repos/asf/mesos/blob/8c8c239b/3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp b/3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp index 4b7a7ba..01e59de 100644 --- a/3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp +++ b/3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp @@ -27,7 +27,7 @@ namespace os { // Runs a shell command formatted with varargs and return the return value // of the command. Optionally, the output is returned via an argument. // TODO(vinod): Pass an istream object that can provide input to the command. -inline Try<int> shell(std::ostream* os, const std::string fmt, ...) +inline Try<std::string> shell(const std::string fmt, ...) { UNIMPLEMENTED; } http://git-wip-us.apache.org/repos/asf/mesos/blob/8c8c239b/3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp b/3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp index 2556bd4..37cfcb7 100644 --- a/3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp +++ b/3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp @@ -882,21 +882,21 @@ TEST_F(OsTest, ProcessExists) TEST_F(OsTest, User) { - std::ostringstream user_; - EXPECT_SOME_EQ(0, os::shell(&user_ , "id -un")); + Try<string> user_ = os::shell("id -un"); + EXPECT_SOME(user_); Result<string> user = os::user(); - ASSERT_SOME_EQ(strings::trim(user_.str()), user); + ASSERT_SOME_EQ(strings::trim(user_.get()), user); - std::ostringstream uid_; - EXPECT_SOME_EQ(0, os::shell(&uid_, "id -u")); - Try<uid_t> uid = numify<uid_t>(strings::trim(uid_.str())); + Try<string> uid_ = os::shell("id -u"); + EXPECT_SOME(uid_); + Try<uid_t> uid = numify<uid_t>(strings::trim(uid_.get())); ASSERT_SOME(uid); EXPECT_SOME_EQ(uid.get(), os::getuid(user.get())); - std::ostringstream gid_; - EXPECT_SOME_EQ(0, os::shell(&gid_, "id -g")); - Try<gid_t> gid = numify<gid_t>(strings::trim(gid_.str())); + Try<string> gid_ = os::shell("id -g"); + EXPECT_SOME(gid_); + Try<gid_t> gid = numify<gid_t>(strings::trim(gid_.get())); ASSERT_SOME(gid); EXPECT_SOME_EQ(gid.get(), os::getgid(user.get())); @@ -948,6 +948,33 @@ TEST_F(OsTest, Libraries) } +TEST_F(OsTest, Shell) +{ + Try<string> result = os::shell("echo %s", "hello world"); + EXPECT_SOME_EQ("hello world\n", result); + + result = os::shell("foobar"); + EXPECT_ERROR(result); + + // The `|| true`` necessary so that os::shell() sees a success + // exit code and returns stdout (which we have piped stderr to). + result = os::shell("ls /tmp/foobar889076 2>&1 || true"); + ASSERT_SOME(result); + EXPECT_TRUE(strings::contains(result.get(), "No such file or directory")); + + // Testing a more ambitious command that mutates the filesystem. + const string path = "/tmp/os_tests.txt"; + result = os::shell("touch %s", path.c_str()); + EXPECT_SOME_EQ("", result); + EXPECT_TRUE(os::exists(path)); + + // Let's clean up, and ensure this worked too. + result = os::shell("rm %s", path.c_str()); + EXPECT_SOME_EQ("", result); + EXPECT_FALSE(os::exists("/tmp/os_tests.txt")); +} + + TEST_F(OsTest, Mknod) { // mknod requires root permission.
