This is an automatically generated e-mail. To reply, visit:

3rdparty/libprocess/include/process/io.hpp (line 145)

    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)

    Same line please.

3rdparty/libprocess/src/io.cpp (line 71)

    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)

    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)

    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)

    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)

    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 

3rdparty/libprocess/src/io.cpp (line 582)

    This indentation should be +2 not +4.

3rdparty/libprocess/src/io.cpp (line 588)

    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

Reply via email to