----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52058/#review150485 -----------------------------------------------------------
src/slave/containerizer/fetcher.cpp (line 765) <https://reviews.apache.org/r/52058/#comment218472> Add a line here. src/slave/containerizer/fetcher.cpp (line 770) <https://reviews.apache.org/r/52058/#comment218470> I think this should be more like: ``` if (chownOut.isError() || chownErr.isError()) { os::close(out.get()); os::close(err.get()); return Failure(...); } ``` And the `Failure` message can indicate which one failed, ie. `stdout` or `stderr` or both (if you want to expose that bit of information). src/slave/containerizer/fetcher.cpp (line 772) <https://reviews.apache.org/r/52058/#comment218467> If there is an error in either `chownOut` or `chownErr`, there is a `os::close(out.get())` and then we return `Failure`... which means we never get to do a `os::close(err.get())`. So, a potential leak in file descriptor! src/slave/containerizer/fetcher.cpp (line 774) <https://reviews.apache.org/r/52058/#comment218468> Also, the `Failure` message indicates the failure is with `stdout` which may not be true if only `chownErr.isError()` is true. src/slave/containerizer/fetcher.cpp (line 779) <https://reviews.apache.org/r/52058/#comment218471> Kill this block if you consolidate the error handling for `chownOut` and `chownErr`. - Anindya Sinha On Sept. 20, 2016, 11:32 p.m., Megha Sharma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52058/ > ----------------------------------------------------------- > > (Updated Sept. 20, 2016, 11:32 p.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-5218 > https://issues.apache.org/jira/browse/MESOS-5218 > > > Repository: mesos > > > Description > ------- > > Fetcher code changes the ownership of entire sandbox directory > recursively to the taskuser and as a resut also changes the > ownership of files laid out by other entities in the sandbox. > > > Diffs > ----- > > src/slave/containerizer/fetcher.cpp > 4d3fc521bf8a7c438c48e31c549f108ac3602b3f > src/tests/containerizer/mesos_containerizer_tests.cpp > 96e24500a12825161553eb050da389088b122695 > > Diff: https://reviews.apache.org/r/52058/diff/ > > > Testing > ------- > > make check on linux > > > Thanks, > > Megha Sharma > >
