----------------------------------------------------------- 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 > >
