----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54351/#review157936 -----------------------------------------------------------
3rdparty/libprocess/include/process/posix/subprocess.hpp (line 188) <https://reviews.apache.org/r/54351/#comment228601> What if `stdoutfds.write == stderrfds.write`? I guess you will close it here, and then *not* close it in the next if statement? Looking more closely at it, it seems like you just extend the (implicit) logic described in the comment for *not* closing stdin/stdout/stderr because they have already been closed by the dup call. I'd extend the comment to make it clear what's going on. Simething like: ``` ... 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. ``` ``` 3rdparty/libprocess/include/process/posix/subprocess.hpp (lines 329 - 370) <https://reviews.apache.org/r/54351/#comment228603> Is this move actually related to this commit, or is it just an existing bug fix / simplification? 3rdparty/libprocess/src/subprocess.cpp (lines 330 - 338) <https://reviews.apache.org/r/54351/#comment228604> I feel like moving this into the windows `createChildProcess()` function would make it more semetrical with the change above. It seems odd that we would ahve to close out here on windows, but not on posix. Also, lines 318 and 326 above should either be removed (if you follow my suggestion) or changed to be the same as this line (if you don't). - Kevin Klues On Dec. 5, 2016, 12:16 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54351/ > ----------------------------------------------------------- > > (Updated Dec. 5, 2016, 12:16 a.m.) > > > Review request for mesos, Benjamin Hindman and Kevin Klues. > > > Bugs: MESOS-6470 > https://issues.apache.org/jira/browse/MESOS-6470 > > > Repository: mesos > > > Description > ------- > > Allowed subprocess to take duplicated FDs. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/posix/subprocess.hpp > aa4609d9e4c63f824c8cd631f39bbbe71e4f67b4 > 3rdparty/libprocess/include/process/windows/subprocess.hpp > f452f6743d01f0b99010fa5e5bcbaae1358c8241 > 3rdparty/libprocess/src/subprocess.cpp > 284e22e28ae8d2c1486e4a6bea743b8663ce2023 > > Diff: https://reviews.apache.org/r/54351/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
