You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2014/06/17 20:51:00 UTC
Review Request 22688: Refactored subprocess to support more IO redirect
modes.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22688/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Bugs: MESOS-1487
https://issues.apache.org/jira/browse/MESOS-1487
Repository: mesos-git
Description
-------
See summary.
This patch only updates the references in libprocess. The update for mesos will be submitted shortly.
Diffs
-----
3rdparty/libprocess/include/process/subprocess.hpp e9d7b31
3rdparty/libprocess/src/subprocess.cpp 9f8f37f
3rdparty/libprocess/src/tests/subprocess_tests.cpp 7d890bf
Diff: https://reviews.apache.org/r/22688/diff/
Testing
-------
3rdparty/libprocess/tests --gtest_repeat=100 --gtest_filter=*Subproces*
Thanks,
Jie Yu
Re: Review Request 22688: Refactored subprocess to support more IO redirect
modes.
Posted by Jie Yu <yu...@gmail.com>.
> On June 17, 2014, 9:43 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 250
> > <https://reviews.apache.org/r/22688/diff/2/?file=611764#file611764line250>
> >
> > With dup'ed file descriptors you can kill this check.
Ben, I still prefer to having this check here even if we dup'ed the fd. The reason is because: what if the parent has a closed stdin/stdout/stderr, when calling ::dup, the returned file descriptor is gonna be one of them (stdin/stdout/stderr). And this can cause the stdin/stdout/stderr file descriptor of the subprocess to be closed , which is not what we wanted?
What do you think?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22688/#review45986
-----------------------------------------------------------
On June 17, 2014, 6:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22688/
> -----------------------------------------------------------
>
> (Updated June 17, 2014, 6:51 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-1487
> https://issues.apache.org/jira/browse/MESOS-1487
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
> This patch only updates the references in libprocess. The update for mesos will be submitted shortly.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp e9d7b31
> 3rdparty/libprocess/src/subprocess.cpp 9f8f37f
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp 7d890bf
>
> Diff: https://reviews.apache.org/r/22688/diff/
>
>
> Testing
> -------
>
> 3rdparty/libprocess/tests --gtest_repeat=100 --gtest_filter=*Subproces*
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22688: Refactored subprocess to support more IO redirect
modes.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On June 17, 2014, 9:43 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 250
> > <https://reviews.apache.org/r/22688/diff/2/?file=611764#file611764line250>
> >
> > With dup'ed file descriptors you can kill this check.
>
> Jie Yu wrote:
> Ben, I still prefer to having this check here even if we dup'ed the fd. The reason is because: what if the parent has a closed stdin/stdout/stderr, when calling ::dup, the returned file descriptor is gonna be one of them (stdin/stdout/stderr). And this can cause the stdin/stdout/stderr file descriptor of the subprocess to be closed , which is not what we wanted?
>
> What do you think?
Ah yes, I agree: we'll always do the dup there but we only want to close if it's not STDIN_FILENO.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22688/#review45986
-----------------------------------------------------------
On June 17, 2014, 6:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22688/
> -----------------------------------------------------------
>
> (Updated June 17, 2014, 6:51 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-1487
> https://issues.apache.org/jira/browse/MESOS-1487
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
> This patch only updates the references in libprocess. The update for mesos will be submitted shortly.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp e9d7b31
> 3rdparty/libprocess/src/subprocess.cpp 9f8f37f
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp 7d890bf
>
> Diff: https://reviews.apache.org/r/22688/diff/
>
>
> Testing
> -------
>
> 3rdparty/libprocess/tests --gtest_repeat=100 --gtest_filter=*Subproces*
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22688: Refactored subprocess to support more IO redirect
modes.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22688/#review45986
-----------------------------------------------------------
Ship it!
This is groovy. Looking forward to it really helping a ton. No more io::redirect! Hurrah!
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/22688/#comment81151>
Why aren't you letting the compiler do this? ;-) Seriously though, six lines of os::close is simpler than putting them all in the array and looping through them! And, ironically, it's shorter. And, it's how we've closed other file descriptors in this file.
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/22688/#comment81155>
See the comment below about using dup'ed file descriptors so we don't have to do these checks (otherwise this totally needs lots of comments). Note that you'll still need to check for >= 0 so let's add a comment above this function that describes the preconditions, i.e., that we'll call os::cloexec on all file descriptors in these pairs that are >= 0. You can probably ignore the >=0 check for internal::close since you're ignoring errors.
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/22688/#comment81152>
s/already/always/
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/22688/#comment81154>
If we dup all of the file descriptors that get passed to us then it will simplify the cloexec'ing and closing that we have to do later on.
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/22688/#comment81156>
With dup'ed file descriptors you can kill this check.
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/22688/#comment81153>
Trailing comment?
With dup'ed file descriptors you can always close. Then it probably makes sense to remind the reader that we don't have any other file descriptors to close unless we're a pipe which is handled below.
- Benjamin Hindman
On June 17, 2014, 6:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22688/
> -----------------------------------------------------------
>
> (Updated June 17, 2014, 6:51 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-1487
> https://issues.apache.org/jira/browse/MESOS-1487
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
> This patch only updates the references in libprocess. The update for mesos will be submitted shortly.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp e9d7b31
> 3rdparty/libprocess/src/subprocess.cpp 9f8f37f
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp 7d890bf
>
> Diff: https://reviews.apache.org/r/22688/diff/
>
>
> Testing
> -------
>
> 3rdparty/libprocess/tests --gtest_repeat=100 --gtest_filter=*Subproces*
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22688: Refactored subprocess to support more IO redirect
modes.
Posted by Jie Yu <yu...@gmail.com>.
> On June 17, 2014, 10:31 p.m., Ian Downes wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 158
> > <https://reviews.apache.org/r/22688/diff/2/?file=611764#file611764line158>
> >
> > What about if os::open accepted O_CLOEXEC and did the best thing - used it on open if supported by glibc or else used fctl after it was opened.
That's my original code, but later I realized that we don't have pipe2 on bsd. In that case, we anyway cannot guarantee atomicity.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22688/#review45998
-----------------------------------------------------------
On June 17, 2014, 6:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22688/
> -----------------------------------------------------------
>
> (Updated June 17, 2014, 6:51 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-1487
> https://issues.apache.org/jira/browse/MESOS-1487
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
> This patch only updates the references in libprocess. The update for mesos will be submitted shortly.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp e9d7b31
> 3rdparty/libprocess/src/subprocess.cpp 9f8f37f
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp 7d890bf
>
> Diff: https://reviews.apache.org/r/22688/diff/
>
>
> Testing
> -------
>
> 3rdparty/libprocess/tests --gtest_repeat=100 --gtest_filter=*Subproces*
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22688: Refactored subprocess to support more IO redirect
modes.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22688/#review45998
-----------------------------------------------------------
3rdparty/libprocess/include/process/subprocess.hpp
<https://reviews.apache.org/r/22688/#comment81159>
And overwritten or appended to if it does?
3rdparty/libprocess/src/subprocess.cpp
<https://reviews.apache.org/r/22688/#comment81161>
What about if os::open accepted O_CLOEXEC and did the best thing - used it on open if supported by glibc or else used fctl after it was opened.
- Ian Downes
On June 17, 2014, 11:51 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22688/
> -----------------------------------------------------------
>
> (Updated June 17, 2014, 11:51 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-1487
> https://issues.apache.org/jira/browse/MESOS-1487
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
> This patch only updates the references in libprocess. The update for mesos will be submitted shortly.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp e9d7b31
> 3rdparty/libprocess/src/subprocess.cpp 9f8f37f
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp 7d890bf
>
> Diff: https://reviews.apache.org/r/22688/diff/
>
>
> Testing
> -------
>
> 3rdparty/libprocess/tests --gtest_repeat=100 --gtest_filter=*Subproces*
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22688: Refactored subprocess to support more IO redirect
modes.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22688/
-----------------------------------------------------------
(Updated June 18, 2014, 1:08 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Fixed a bug due to typo.
Bugs: MESOS-1487
https://issues.apache.org/jira/browse/MESOS-1487
Repository: mesos-git
Description
-------
See summary.
This patch only updates the references in libprocess. The update for mesos will be submitted shortly.
Diffs (updated)
-----
3rdparty/libprocess/include/process/subprocess.hpp e9d7b31
3rdparty/libprocess/src/subprocess.cpp 9f8f37f
3rdparty/libprocess/src/tests/subprocess_tests.cpp 7d890bf
Diff: https://reviews.apache.org/r/22688/diff/
Testing
-------
3rdparty/libprocess/tests --gtest_repeat=100 --gtest_filter=*Subproces*
Tested on both mac and linux.
Thanks,
Jie Yu
Re: Review Request 22688: Refactored subprocess to support more IO redirect
modes.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22688/
-----------------------------------------------------------
(Updated June 17, 2014, 11:07 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Addressed comments.
Bugs: MESOS-1487
https://issues.apache.org/jira/browse/MESOS-1487
Repository: mesos-git
Description
-------
See summary.
This patch only updates the references in libprocess. The update for mesos will be submitted shortly.
Diffs (updated)
-----
3rdparty/libprocess/include/process/subprocess.hpp e9d7b31
3rdparty/libprocess/src/subprocess.cpp 9f8f37f
3rdparty/libprocess/src/tests/subprocess_tests.cpp 7d890bf
Diff: https://reviews.apache.org/r/22688/diff/
Testing (updated)
-------
3rdparty/libprocess/tests --gtest_repeat=100 --gtest_filter=*Subproces*
Tested on both mac and linux.
Thanks,
Jie Yu