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

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!


- Michael


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