> 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!
> 
> Daniel Pravat wrote:
>     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.
> 
> Michael Park wrote:
>     I'm not quite following. From
>     > Exposing a constructor taking both the error code and the message 
> should be suficient. 
>     
>     It seems like you're saying we should introduce a constructor that takes 
> an error code, something like:
>     ```
>     WindowsSockerError::WindowsSocketError(int code, const std::string& 
> message);
>     ```
>     But as I mentioned, even if we do that, the order of evaluation of the 
> arguments is unspecified.
>     and even if we were to say, well it's "implementation-defined", we still 
> have to be mindful
>     of the code between `::connect` and `WindowsSocketError`. You seem to be 
> supporting this argument
>     with the second quote:
>     > 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.
>     
>     so I'm a bit confused. Could you be a little more specific/concrete?

I was agreeing with you. We can both agree that we need both parameters to the 
constructor (the error message for logging and the error code for the execution 
flow). Your example from the first comment seems to imply you agree with a 
constructor with two parameters. 

You also made a reference to the parameter reorder that may allow one line 
return `return ConnectError(message, ::WSAGetLastError());`. However given this 
error is returned/used only in a few places at this time, the parameters can be 
in any order. 

The user (returning `WindowsError`) has to be aware that last error may be 
overwritten, has to capture it ASAP and later used it to construct 
`WindowsError`.


- 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
> 
>

Reply via email to