Re: Review Request 65474: Updated socket shutdown to return SocketError.

2018-02-05 Thread Till Toenshoff 


> On Feb. 3, 2018, 12:41 a.m., Andrei Budnik wrote:
> > 3rdparty/libprocess/include/process/socket.hpp
> > Line 192 (original), 193 (patched)
> > 
> >
> > What about returning `Nothing()` for not connected sockets here:
> > ```
> > if (::shutdown(s, how) < 0 && errno != ENOTCONN) {
> > ```
> > and in `LibeventSSLSocketImpl::shutdown()`?
> > 
> > It will be 2-liner change (excluding `#ifdef __WINDOWS__` stuff).
> > 
> > We already have such errno check in 
> > https://github.com/apache/mesos/blob/master/3rdparty/stout/include/stout/os/windows/close.hpp#L40-L43

We'ld still need `ENOTCONN` vs. `WSANOTCONN` `#ifdef WINDOWS` nastynes as on 
this (non stout) level we have to. We would also need to make assumptions about 
the expected state. IMO that would be ok here: 
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/libevent_ssl_socket.cpp#L168
 cause we have context allowing to assert no existing connection on that 
socket. But it is not ok here: 
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/socket.hpp#L192
 cause we just don't know what to expect - that needs to be decided on higher 
levels.


- Till


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65474/#review196769
---


On Feb. 2, 2018, 5:50 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65474/
> ---
> 
> (Updated Feb. 2, 2018, 5:50 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8513
> https://issues.apache.org/jira/browse/MESOS-8513
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Socket::shutdown returns SocketError allowing for higher level
> functions to have more control over logging errors where needed.
> Also switches SocketManager::close's shutdown failure logging
> back to ERROR level as we are now able to filter out expected
> failures.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> ae6154d5d142f65352e00f37b4e66d0b62fdc3c2 
>   3rdparty/libprocess/src/http.cpp cc41fa6f671cf029a46722299eded1a27da210d3 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 640fa676ef570f7fcf3f96249662837497a2c76c 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 521b0cfbccd3599524b1407ef70880f4538941df 
>   3rdparty/libprocess/src/process.cpp 
> ba9bc291bb6741e32b3a066fe90771311d21852a 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> b1a0ea614d7483e683ac056cf822ce816221babb 
> 
> 
> Diff: https://reviews.apache.org/r/65474/diff/3/
> 
> 
> Testing
> ---
> 
> make check, visual logging inspection
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 65474: Updated socket shutdown to return SocketError.

2018-02-02 Thread Andrei Budnik 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65474/#review196769
---




3rdparty/libprocess/include/process/socket.hpp
Line 192 (original), 193 (patched)


What about returning `Nothing()` for not connected sockets here:
```
if (::shutdown(s, how) < 0 && errno != ENOTCONN) {
```
and in `LibeventSSLSocketImpl::shutdown()`?

It will be 2-liner change (excluding `#ifdef __WINDOWS__` stuff).

We already have such errno check in 
https://github.com/apache/mesos/blob/master/3rdparty/stout/include/stout/os/windows/close.hpp#L40-L43


- Andrei Budnik


On Feb. 2, 2018, 5:50 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65474/
> ---
> 
> (Updated Feb. 2, 2018, 5:50 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8513
> https://issues.apache.org/jira/browse/MESOS-8513
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Socket::shutdown returns SocketError allowing for higher level
> functions to have more control over logging errors where needed.
> Also switches SocketManager::close's shutdown failure logging
> back to ERROR level as we are now able to filter out expected
> failures.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> ae6154d5d142f65352e00f37b4e66d0b62fdc3c2 
>   3rdparty/libprocess/src/http.cpp cc41fa6f671cf029a46722299eded1a27da210d3 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 640fa676ef570f7fcf3f96249662837497a2c76c 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 521b0cfbccd3599524b1407ef70880f4538941df 
>   3rdparty/libprocess/src/process.cpp 
> ba9bc291bb6741e32b3a066fe90771311d21852a 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> b1a0ea614d7483e683ac056cf822ce816221babb 
> 
> 
> Diff: https://reviews.apache.org/r/65474/diff/3/
> 
> 
> Testing
> ---
> 
> make check, visual logging inspection
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 65474: Updated socket shutdown to return SocketError.

2018-02-02 Thread Benjamin Mahler 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65474/#review196756
---


Ship it!





3rdparty/libprocess/src/libevent_ssl_socket.cpp
Line 168 (original), 168 (patched)


Note that here we should probably just directly call shutdown on the fd (in 
your second patch).


- Benjamin Mahler


On Feb. 2, 2018, 5:50 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65474/
> ---
> 
> (Updated Feb. 2, 2018, 5:50 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8513
> https://issues.apache.org/jira/browse/MESOS-8513
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Socket::shutdown returns SocketError allowing for higher level
> functions to have more control over logging errors where needed.
> Also switches SocketManager::close's shutdown failure logging
> back to ERROR level as we are now able to filter out expected
> failures.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> ae6154d5d142f65352e00f37b4e66d0b62fdc3c2 
>   3rdparty/libprocess/src/http.cpp cc41fa6f671cf029a46722299eded1a27da210d3 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 640fa676ef570f7fcf3f96249662837497a2c76c 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 521b0cfbccd3599524b1407ef70880f4538941df 
>   3rdparty/libprocess/src/process.cpp 
> ba9bc291bb6741e32b3a066fe90771311d21852a 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> b1a0ea614d7483e683ac056cf822ce816221babb 
> 
> 
> Diff: https://reviews.apache.org/r/65474/diff/3/
> 
> 
> Testing
> ---
> 
> make check, visual logging inspection
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 65474: Updated socket shutdown to return SocketError.

2018-02-01 Thread Till Toenshoff 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65474/#review196688
---




3rdparty/libprocess/include/process/socket.hpp
Line 193 (original), 193 (patched)


This one should be a `SocketError()`


- Till Toenshoff


On Feb. 2, 2018, 5:50 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65474/
> ---
> 
> (Updated Feb. 2, 2018, 5:50 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8513
> https://issues.apache.org/jira/browse/MESOS-8513
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Socket::shutdown returns SocketError allowing for higher level
> functions to have more control over logging errors where needed.
> Also switches SocketManager::close's shutdown failure logging
> back to ERROR level as we are now able to filter out expected
> failures.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> ae6154d5d142f65352e00f37b4e66d0b62fdc3c2 
>   3rdparty/libprocess/src/http.cpp cc41fa6f671cf029a46722299eded1a27da210d3 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 640fa676ef570f7fcf3f96249662837497a2c76c 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 521b0cfbccd3599524b1407ef70880f4538941df 
>   3rdparty/libprocess/src/process.cpp 
> ba9bc291bb6741e32b3a066fe90771311d21852a 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> b1a0ea614d7483e683ac056cf822ce816221babb 
> 
> 
> Diff: https://reviews.apache.org/r/65474/diff/1/
> 
> 
> Testing
> ---
> 
> make check, visual logging inspection
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>