----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38809/#review100909 -----------------------------------------------------------
3rdparty/libprocess/include/process/socket.hpp (lines 64 - 65) <https://reviews.apache.org/r/38809/#comment158168> Can you fix the wrapping here? Also, I would suggest s/socketfd/s/ to keep the same as before, since `socketfd` is a bit unusual for our code base. Also the doxygen a few lines up refers to 's' still :) 3rdparty/libprocess/src/socket.cpp (lines 42 - 44) <https://reviews.apache.org/r/38809/#comment158169> Just as an aside, it's unfortunate the caller has to do this, we should consider doing the same thing that is done in os/posix/open.hpp with O_CLOEXEC: https://github.com/apache/mesos/blob/5aa050bfaa7338b010b1a522406bde0f15015259/3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/open.hpp#L33 3rdparty/libprocess/src/socket.cpp (lines 77 - 79) <https://reviews.apache.org/r/38809/#comment158171> How about we store a bool for this at the top instead of having multiple variables? ``` // If the caller passed in a descriptor, we do // not own its lifecycle and must not close it! bool owned = s.isNone(); ... ``` - Ben Mahler On Sept. 28, 2015, 9:07 p.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38809/ > ----------------------------------------------------------- > > (Updated Sept. 28, 2015, 9:07 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-3519 > https://issues.apache.org/jira/browse/MESOS-3519 > > > Repository: mesos > > > Description > ------- > > socket: refactor to use Option and fix file descriptor leaks. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/socket.hpp > 817cb3711742871630a13b966563296644008f21 > 3rdparty/libprocess/src/socket.cpp 5879423bd92c5805353a962cbd822a993b0568c5 > > Diff: https://reviews.apache.org/r/38809/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Chi Zhang > >
