> On March 26, 2016, 8:05 p.m., Daniel Pravat wrote: > > 3rdparty/libprocess/include/process/network.hpp, line 75 > > <https://reviews.apache.org/r/45314/diff/1/?file=1314163#file1314163line75> > > > > We tried to avoid axecuting too much code between ::connect > > WSAGetLastError() calls. > > Constructing the parameter for WindowsSocketError may overwrite the > > last error. It should be better to pass in the return from > > WSAGetLastError() as a parameter to the constructor, that is evaluate > > before the std::string parameter. > > Michael Park wrote: > Gave this some thought. I'm inclined to keep this as is. > > Even if you're suggesting that we provide `::WSAGetLastError()` on every > callsite like: > > ``` > int result = ::connect(...); > > // BE AWARE! > > // Even ignoring `::WSAGetLastError()` being Windows-specific. > return ConnectError(::WSAGetLastError(), "Failed to connect to " + > stringify(address)); > ``` > > The order of evaluation of the parameters is unspecified. > > Even if it were to be specified (left-to-right), we still need to be > mindful of the code between > `::connect` and `ConnectError`. We would run into the same sequence of > code execution if someone > were to pull out the expression and constructed a variable. > > ``` > int result = ::connect(...); > > // BE AWARE! > > std::string message = "Failed to connect to " + stringify(address); // > BE AWARE! > return ConnectError(::WSAGetLastError(), message); > ``` > > The only way we would completely solve this is if we were to say either: > `::WSAGetLastError()` must be > called immediately after `::connect`, or ensure that nothing between > `::connect` and the call to > `::WSAGetLastError()` can overwrite the error code. The latter is what we > currently do, and I don't think > that reordering just the `::WSAGetLastError` call and the argument to > `ConnectError` is a big win. > > Please share your ideas and solutions!
Exposing a constructor taking both the error code and the message should be suficient. The convenience provided by the parameters reordering is not adding a lot of value given the small number of instance where the error code is interpreted. - Daniel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45314/#review125549 ----------------------------------------------------------- On March 24, 2016, 8:23 p.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45314/ > ----------------------------------------------------------- > > (Updated March 24, 2016, 8:23 p.m.) > > > Review request for mesos, Alex Clemmer and Joris Van Remoortere. > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/network.hpp > 9976257d2d13316062bc95a22ab564ca0df165e5 > 3rdparty/libprocess/src/poll_socket.cpp > 6e6634b4b352e3723096521843546cf56ec6dd8b > > Diff: https://reviews.apache.org/r/45314/diff/ > > > Testing > ------- > > `make check` > > > Thanks, > > Michael Park > >
