Changed `os::system()` to return `Option<int>` instead of `int`. The `os::system()` function returned `-1` on error, which is a valid exit code on Windows, e.g., `os::system("exit -1")`, so it was impossible to distinguish a failure from a process returning `-1`. With `Option<int>`, failures will return as `None()`.
Review: https://reviews.apache.org/r/65841/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/330ddcb5 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/330ddcb5 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/330ddcb5 Branch: refs/heads/master Commit: 330ddcb517c7e01003915948d87932990c7b9004 Parents: ca357e9 Author: Akash Gupta <akash-gu...@hotmail.com> Authored: Tue Mar 6 13:11:21 2018 -0800 Committer: Andrew Schwartzmeyer <and...@schwartzmeyer.com> Committed: Tue Mar 6 13:51:56 2018 -0800 ---------------------------------------------------------------------- 3rdparty/stout/include/stout/os/posix/shell.hpp | 18 ++++++++++-------- 3rdparty/stout/include/stout/os/windows/shell.hpp | 14 ++++++++++---- 3rdparty/stout/tests/os/rmdir_tests.cpp | 3 ++- 3rdparty/stout/tests/os_tests.cpp | 12 ++++++------ 4 files changed, 28 insertions(+), 19 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/330ddcb5/3rdparty/stout/include/stout/os/posix/shell.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/stout/include/stout/os/posix/shell.hpp b/3rdparty/stout/include/stout/os/posix/shell.hpp index b878718..b6e3deb 100644 --- a/3rdparty/stout/include/stout/os/posix/shell.hpp +++ b/3rdparty/stout/include/stout/os/posix/shell.hpp @@ -25,6 +25,8 @@ #include <stout/error.hpp> #include <stout/format.hpp> +#include <stout/none.hpp> +#include <stout/option.hpp> #include <stout/try.hpp> #include <stout/os/raw/argv.hpp> @@ -117,22 +119,22 @@ Try<std::string> shell(const std::string& fmt, const T&... t) // Executes a command by calling "/bin/sh -c <command>", and returns -// after the command has been completed. Returns 0 if succeeds, and -// return -1 on error (e.g., fork/exec/waitpid failed). This function -// is async signal safe. We return int instead of returning a Try -// because Try involves 'new', which is not async signal safe. +// after the command has been completed. Returns the exit code on success +// and `None` on error (e.g., fork/exec/waitpid failed). This function +// is async signal safe. We return an `Option<int>` instead of a `Try<int>`, +// because although `Try` does not dynamically allocate, `Error` uses +// `std::string`, which is not async signal safe. // // Note: Be cautious about shell injection // (https://en.wikipedia.org/wiki/Code_injection#Shell_injection) // when using this method and use proper validation and sanitization // on the `command`. For this reason in general `os::spawn` is // preferred if a shell is not required. -inline int system(const std::string& command) +inline Option<int> system(const std::string& command) { pid_t pid = ::fork(); - if (pid == -1) { - return -1; + return None(); } else if (pid == 0) { // In child process. ::execlp( @@ -143,7 +145,7 @@ inline int system(const std::string& command) int status; while (::waitpid(pid, &status, 0) == -1) { if (errno != EINTR) { - return -1; + return None(); } } http://git-wip-us.apache.org/repos/asf/mesos/blob/330ddcb5/3rdparty/stout/include/stout/os/windows/shell.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/stout/include/stout/os/windows/shell.hpp b/3rdparty/stout/include/stout/os/windows/shell.hpp index 1696d08..c0b9efa 100644 --- a/3rdparty/stout/include/stout/os/windows/shell.hpp +++ b/3rdparty/stout/include/stout/os/windows/shell.hpp @@ -24,6 +24,7 @@ #include <stout/error.hpp> #include <stout/foreach.hpp> +#include <stout/none.hpp> #include <stout/option.hpp> #include <stout/os.hpp> #include <stout/try.hpp> @@ -394,17 +395,22 @@ inline int spawn( // Executes a command by calling "cmd /c <command>", and returns -// after the command has been completed. Returns exit code if succeeds, and -// return -1 on error. +// after the command has been completed. Returns the process exit +// code on success and `None` on error. // // Note: Be cautious about shell injection // (https://en.wikipedia.org/wiki/Code_injection#Shell_injection) // when using this method and use proper validation and sanitization // on the `command`. For this reason in general `os::spawn` is // preferred if a shell is not required. -inline int system(const std::string& command) +inline Option<int> system(const std::string& command) { - return os::spawn(Shell::name, {Shell::arg0, Shell::arg1, command}); + // TODO(akagup): Change `os::spawn` to return `Option<int>` as well. + int pid = os::spawn(Shell::name, {Shell::arg0, Shell::arg1, command}); + if (pid == -1) { + return None(); + } + return pid; } http://git-wip-us.apache.org/repos/asf/mesos/blob/330ddcb5/3rdparty/stout/tests/os/rmdir_tests.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/stout/tests/os/rmdir_tests.cpp b/3rdparty/stout/tests/os/rmdir_tests.cpp index 2fd24a8..518afc0 100644 --- a/3rdparty/stout/tests/os/rmdir_tests.cpp +++ b/3rdparty/stout/tests/os/rmdir_tests.cpp @@ -442,7 +442,8 @@ TEST_F(RmdirContinueOnErrorTest, RemoveWithContinueOnError) ASSERT_SOME(os::mkdir(mountPoint_)); ASSERT_SOME(os::touch(regularFile)); - ASSERT_EQ(0, os::system("mount --bind " + mountPoint_ + " " + mountPoint_)); + ASSERT_SOME_EQ(0, os::system( + "mount --bind " + mountPoint_ + " " + mountPoint_)); // Register the mount point for cleanup. mountPoint = Option<string>(mountPoint_); http://git-wip-us.apache.org/repos/asf/mesos/blob/330ddcb5/3rdparty/stout/tests/os_tests.cpp ---------------------------------------------------------------------- diff --git a/3rdparty/stout/tests/os_tests.cpp b/3rdparty/stout/tests/os_tests.cpp index d04c3b9..4221ecd 100644 --- a/3rdparty/stout/tests/os_tests.cpp +++ b/3rdparty/stout/tests/os_tests.cpp @@ -145,14 +145,14 @@ TEST_F(OsTest, Argv) TEST_F(OsTest, System) { - EXPECT_EQ(0, os::system("exit 0")); - EXPECT_EQ(0, os::system(SLEEP_COMMAND(0))); - EXPECT_NE(0, os::system("exit 1")); - EXPECT_NE(0, os::system("invalid.command")); + EXPECT_SOME_EQ(0, os::system("exit 0")); + EXPECT_SOME_EQ(0, os::system(SLEEP_COMMAND(0))); + EXPECT_SOME_NE(0, os::system("exit 1")); + EXPECT_SOME_NE(0, os::system("invalid.command")); // Note that ::system returns 0 for the following two cases as well. - EXPECT_EQ(0, os::system("")); - EXPECT_EQ(0, os::system(" ")); + EXPECT_SOME_EQ(0, os::system("")); + EXPECT_SOME_EQ(0, os::system(" ")); }