You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by ji...@apache.org on 2016/12/05 05:42:34 UTC
[2/9] mesos git commit: Allowed subprocess to take duplicated FDs.
Allowed subprocess to take duplicated FDs.
Review: https://reviews.apache.org/r/54351
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/c7f953aa
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/c7f953aa
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/c7f953aa
Branch: refs/heads/master
Commit: c7f953aadcead297d8119e0c3564b8a2f67b518e
Parents: d6bc482
Author: Jie Yu <yu...@gmail.com>
Authored: Sat Dec 3 23:53:00 2016 -0800
Committer: Jie Yu <yu...@gmail.com>
Committed: Sun Dec 4 21:41:51 2016 -0800
----------------------------------------------------------------------
.../include/process/posix/subprocess.hpp | 66 +++++++++++---------
.../include/process/windows/subprocess.hpp | 29 +++++----
3rdparty/libprocess/src/subprocess.cpp | 12 ++--
3 files changed, 62 insertions(+), 45 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/c7f953aa/3rdparty/libprocess/include/process/posix/subprocess.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/include/process/posix/subprocess.hpp b/3rdparty/libprocess/include/process/posix/subprocess.hpp
index aa4609d..60f40ea 100644
--- a/3rdparty/libprocess/include/process/posix/subprocess.hpp
+++ b/3rdparty/libprocess/include/process/posix/subprocess.hpp
@@ -27,6 +27,7 @@
#include <stout/check.hpp>
#include <stout/error.hpp>
#include <stout/foreach.hpp>
+#include <stout/hashset.hpp>
#include <stout/nothing.hpp>
#include <stout/lambda.hpp>
#include <stout/none.hpp>
@@ -70,6 +71,15 @@ inline pid_t defaultClone(const lambda::function<int()>& func)
namespace internal {
+inline void close(const hashset<int>& fds)
+{
+ foreach (int fd, fds) {
+ if (fd >= 0) {
+ os::close(fd);
+ }
+ }
+}
+
// This function will invoke `os::close` on all specified file
// descriptors that are valid (i.e., not `None` and >= 0).
inline void close(
@@ -77,17 +87,14 @@ inline void close(
const OutputFileDescriptors& stdoutfds,
const OutputFileDescriptors& stderrfds)
{
- int fds[6] = {
- stdinfds.read, stdinfds.write.getOrElse(-1),
- stdoutfds.read.getOrElse(-1), stdoutfds.write,
- stderrfds.read.getOrElse(-1), stderrfds.write
- };
-
- foreach (int fd, fds) {
- if (fd >= 0) {
- os::close(fd);
- }
- }
+ close({
+ stdinfds.read,
+ stdinfds.write.getOrElse(-1),
+ stdoutfds.read.getOrElse(-1),
+ stdoutfds.write,
+ stderrfds.read.getOrElse(-1),
+ stderrfds.write
+ });
}
@@ -98,10 +105,13 @@ inline Try<Nothing> cloexec(
const OutputFileDescriptors& stdoutfds,
const OutputFileDescriptors& stderrfds)
{
- int fds[6] = {
- stdinfds.read, stdinfds.write.getOrElse(-1),
- stdoutfds.read.getOrElse(-1), stdoutfds.write,
- stderrfds.read.getOrElse(-1), stderrfds.write
+ hashset<int> fds = {
+ stdinfds.read,
+ stdinfds.write.getOrElse(-1),
+ stdoutfds.read.getOrElse(-1),
+ stdoutfds.write,
+ stderrfds.read.getOrElse(-1),
+ stderrfds.write
};
foreach (int fd, fds) {
@@ -167,6 +177,10 @@ inline int childMain(
// parent has closed stdin/stdout/stderr when calling this
// function (in that case, a dup'ed file descriptor may have the
// same file descriptor number as stdin/stdout/stderr).
+ //
+ // We also need to ensure that we don't "double close" any file
+ // descriptors in the case where one of stdinfds.read,
+ // stdoutfds.write, or stdoutfds.write are equal.
if (stdinfds.read != STDIN_FILENO &&
stdinfds.read != STDOUT_FILENO &&
stdinfds.read != STDERR_FILENO) {
@@ -174,12 +188,15 @@ inline int childMain(
}
if (stdoutfds.write != STDIN_FILENO &&
stdoutfds.write != STDOUT_FILENO &&
- stdoutfds.write != STDERR_FILENO) {
+ stdoutfds.write != STDERR_FILENO &&
+ stdoutfds.write != stdinfds.read) {
::close(stdoutfds.write);
}
if (stderrfds.write != STDIN_FILENO &&
stderrfds.write != STDOUT_FILENO &&
- stderrfds.write != STDERR_FILENO) {
+ stderrfds.write != STDERR_FILENO &&
+ stderrfds.write != stdinfds.read &&
+ stderrfds.write != stdoutfds.write) {
::close(stderrfds.write);
}
@@ -313,6 +330,10 @@ inline Try<pid_t> cloneChild(
return error;
}
+ // Close the child-ends of the file descriptors that are created by
+ // this function.
+ internal::close({stdinfds.read, stdoutfds.write, stderrfds.write});
+
if (blocking) {
os::close(pipes[0]);
@@ -329,12 +350,6 @@ inline Try<pid_t> cloneChild(
os::close(pipes[1]);
- // Close the child-ends of the file descriptors that are created
- // by this function.
- os::close(stdinfds.read);
- os::close(stdoutfds.write);
- os::close(stderrfds.write);
-
// Ensure the child is killed.
::kill(pid, SIGKILL);
@@ -357,11 +372,6 @@ inline Try<pid_t> cloneChild(
// Ensure the child is killed.
::kill(pid, SIGKILL);
- // Close the child-ends of the file descriptors that are created
- // by this function.
- os::close(stdinfds.read);
- os::close(stdoutfds.write);
- os::close(stderrfds.write);
return Error("Failed to synchronize child process");
}
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/c7f953aa/3rdparty/libprocess/include/process/windows/subprocess.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/include/process/windows/subprocess.hpp b/3rdparty/libprocess/include/process/windows/subprocess.hpp
index f452f67..e2ba752 100644
--- a/3rdparty/libprocess/include/process/windows/subprocess.hpp
+++ b/3rdparty/libprocess/include/process/windows/subprocess.hpp
@@ -23,6 +23,7 @@
#include <stout/error.hpp>
#include <stout/foreach.hpp>
+#include <stout/hashset.hpp>
#include <stout/option.hpp>
#include <stout/os.hpp>
#include <stout/try.hpp>
@@ -44,6 +45,15 @@ using OutputFileDescriptors = Subprocess::IO::OutputFileDescriptors;
namespace internal {
+inline void close(const hashset<HANDLE>& fds)
+{
+ foreach (HANDLE fd, fds) {
+ if (fd != INVALID_HANDLE_VALUE) {
+ os::close(fd);
+ }
+ }
+}
+
// This function will invoke `os::close` on all specified file
// descriptors that are valid (i.e., not `None` and >= 0).
inline void close(
@@ -51,17 +61,14 @@ inline void close(
const OutputFileDescriptors& stdoutfds,
const OutputFileDescriptors& stderrfds)
{
- HANDLE fds[6] = {
- stdinfds.read, stdinfds.write.getOrElse(INVALID_HANDLE_VALUE),
- stdoutfds.read.getOrElse(INVALID_HANDLE_VALUE), stdoutfds.write,
- stderrfds.read.getOrElse(INVALID_HANDLE_VALUE), stderrfds.write
- };
-
- foreach (HANDLE fd, fds) {
- if (fd != INVALID_HANDLE_VALUE) {
- os::close(fd);
- }
- }
+ close({
+ stdinfds.read,
+ stdinfds.write.getOrElse(INVALID_HANDLE_VALUE),
+ stdoutfds.read.getOrElse(INVALID_HANDLE_VALUE),
+ stdoutfds.write,
+ stderrfds.read.getOrElse(INVALID_HANDLE_VALUE),
+ stderrfds.write
+ });
}
// Retrieves system environment in a `std::map`, ignoring
http://git-wip-us.apache.org/repos/asf/mesos/blob/c7f953aa/3rdparty/libprocess/src/subprocess.cpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/src/subprocess.cpp b/3rdparty/libprocess/src/subprocess.cpp
index b3efb9c..340fc32 100644
--- a/3rdparty/libprocess/src/subprocess.cpp
+++ b/3rdparty/libprocess/src/subprocess.cpp
@@ -327,17 +327,17 @@ Try<Subprocess> subprocess(
return error;
}
+ // Close the child-ends of the file descriptors that are created
+ // by this function.
+ // TODO(jieyu): We should move the closing of FDs to
+ // 'createChildProcess' to be consistent with the posix path.
+ internal::close({stdinfds.read, stdoutfds.write, stderrfds.write});
+
process.data->processInformation = processInformation.get();
process.data->pid = processInformation.get().dwProcessId;
#endif // __WINDOWS__
}
- // Close the child-ends of the file descriptors that are created
- // by this function.
- os::close(stdinfds.read);
- os::close(stdoutfds.write);
- os::close(stderrfds.write);
-
// For any pipes, store the parent side of the pipe so that
// the user can communicate with the subprocess.
process.data->in = stdinfds.write;