----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/#review96342 -----------------------------------------------------------
3rdparty/libprocess/include/process/io.hpp (line 145) <https://reviews.apache.org/r/36404/#comment151656> I would like us to document the parameters please, specifically it's not obvious the difference between 'size' and 'limit'. IIUC we'll return right away even if we haven't read all of 'size'? Same for the overload below. 3rdparty/libprocess/src/io.cpp (lines 69 - 70) <https://reviews.apache.org/r/36404/#comment151672> Same line please. 3rdparty/libprocess/src/io.cpp (line 71) <https://reviews.apache.org/r/36404/#comment151673> How about adding a comment that if 'fd' is not a socket we'll "do the right thing" because 'recv' will fail with ENOTSOCK and we'll propagate that out. 3rdparty/libprocess/src/io.cpp (lines 274 - 286) <https://reviews.apache.org/r/36404/#comment151671> This is the "old style", in the "new style" we just dupliate the file descriptor so that if someone closes the file descriptor passed to us we don't either read from a closed file descriptor or WORSE read from a newly opened file descriptor that we shouldn't be reading from. See 'io::read(int fd)' for an example. Note that the other ones need to get changed as well but I've done that for the Mesos on Windows work (in the my github/benh/mesos mesos-on-windows branch, which was necessary to do there because we can't support os::isNonblock on Windows). 3rdparty/libprocess/src/io.cpp (line 400) <https://reviews.apache.org/r/36404/#comment151670> The other internal::_* functions were originally created to (A) deal with the fact we didn't have C++11 and (B) because they were recursive. We don't need that for 'peek' in this case, and in the other cases we can ultimately remove the need for the internal function and just use a recursive lambda. So, how about killing internal::_peek? 3rdparty/libprocess/src/io.cpp (line 407) <https://reviews.apache.org/r/36404/#comment151662> I just checked and we've used 'length' throughout this file for this variable name, let's keep it consistent for now and do the same here please. 3rdparty/libprocess/src/io.cpp (line 577) <https://reviews.apache.org/r/36404/#comment151658> The declaration uses the variable named 'limit' but here it's 'size' which sort of implies a different semantics? Either way, we should use the same name please. 3rdparty/libprocess/src/io.cpp (line 582) <https://reviews.apache.org/r/36404/#comment151659> This indentation should be +2 not +4. 3rdparty/libprocess/src/io.cpp (line 588) <https://reviews.apache.org/r/36404/#comment151661> This appears to be an unused variable? Or am I missing something? Which will also render the TODO(bmahler) above useless. - Benjamin Hindman On Aug. 6, 2015, 3:26 a.m., Artem Harutyunyan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36404/ > ----------------------------------------------------------- > > (Updated Aug. 6, 2015, 3:26 a.m.) > > > Review request for mesos, Joris Van Remoortere and Joseph Wu. > > > Bugs: MESOS-2964 > https://issues.apache.org/jira/browse/MESOS-2964 > > > Repository: mesos > > > Description > ------- > > JIRA: https://issues.apache.org/jira/browse/MESOS-2964 > > > Diffs > ----- > > 3rdparty/libprocess/include/process/io.hpp > 975923f40f82357f31b89428f24d01df6a8ac9fc > 3rdparty/libprocess/src/io.cpp 4a6e18a17012994d358099ad32d4c282fea3b0b1 > 3rdparty/libprocess/src/tests/io_tests.cpp > c642b3333ab9e2845668767ad237985cb9ce1109 > > Diff: https://reviews.apache.org/r/36404/diff/ > > > Testing > ------- > > - Added a test case for process::io::peek > - make check > > > Thanks, > > Artem Harutyunyan > >
