----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27960/#review61453 -----------------------------------------------------------
3rdparty/libprocess/include/process/socket.hpp <https://reviews.apache.org/r/27960/#comment103046> Why isn't 'get' just returning the int socket? It seems weird to have get() return this implementation. 3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/27960/#comment103051> Let's log the failure please! ;-) 3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/27960/#comment103083> I'm guessing you've made this a reference because you don't want to pay the extra copy? Why not just pass this as pointer then? Because this is an asynchronous call using a non-const reference is dangerous because people might actually attempt to pass the reference and then we'll likely segfault. 3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/27960/#comment103064> Please put a newline between these stanzas of code so that it's easier to see this is the next part of this function. Thanks! 3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/27960/#comment103060> I think you're just returning something because you used 'then' below. Let's instead just use 'onReady' below and then we don't need to return anything here. 3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/27960/#comment103055> As much as we're able I'd love us to use consistent naming. It just makes it easier to match things up in the code. In this case, it would be great to name the variables the same as the function parameters: const size_t length = 80 * 1024; char* data = new char[length]; memset(data, 0, length); However, to do this you'll also need to change the first parameter in decode_read_ready. This is why I had picked 'size' for parameters/arguments and 'length' for return values in the past. 3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/27960/#comment103066> What happens when someone closes the socket and we're in the middle of a read? In particular, we just want to make sure that whatever does happen one of onReady or onFailed gets invoked so that we clean up our allocations. 3rdparty/libprocess/src/process.cpp <https://reviews.apache.org/r/27960/#comment103049> Why not have 'get()' return the int file descriptor? Then you can just do: return io::read(get(), data, length); In order to make sure that the socket has been created. I also think the implicit cast operator should be implemented in terms of get() too: operator int () const { return impl->get(); } And are you sure that 'get' needs to be const? It would be nice if we didn't have to do 'mutable int s'. - Benjamin Hindman On Nov. 13, 2014, 2:52 a.m., Joris Van Remoortere wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27960/ > ----------------------------------------------------------- > > (Updated Nov. 13, 2014, 2:52 a.m.) > > > Review request for mesos, Benjamin Hindman and Niklas Nielsen. > > > Bugs: MESOS-1330 > https://issues.apache.org/jira/browse/MESOS-1330 > > > Repository: mesos-git > > > Description > ------- > > See Summary. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/socket.hpp > 66838814236fc064a2463399fb15f4a815879bf5 > 3rdparty/libprocess/src/process.cpp > a34b8702b01dec9c954552de0b923866d172c453 > > Diff: https://reviews.apache.org/r/27960/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Joris Van Remoortere > >
