You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by an...@apache.org on 2018/03/06 23:24:30 UTC
[5/8] mesos git commit: Changed `os::system()` to return
`Option` instead of `int`.
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 <ak...@hotmail.com>
Authored: Tue Mar 6 13:11:21 2018 -0800
Committer: Andrew Schwartzmeyer <an...@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(" "));
}