----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29406/#review87783 -----------------------------------------------------------
Reviewed `libevent_ssl_socket.{hpp,cpp}`. I don't have much confidence at all around whether we free memory properly. The calls to `SSL_free`, `delete request`, `bufferevent_free(bev); bev = NULL;` occur in many places :( Otherwise things look good to me. 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140225> Now that we don't capture `this` and use `self`, the `NOTE` is inaccurate. > Note: The 'self' needs to be explicitly captured because we're not using it in the body of the lambda. 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140207> `bev` is unused and also shadows the member variable. Omit? 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140208> `bev` is unused and also shadows the member variable. Omit? 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140209> `bev` is unused and also shadows the member variable. Omit? 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140210> ```cpp current_connect_request->promise.fail( "Failed connect: connection closed"); ``` 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140229> Remove newline. 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140217> This fits in one line. 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140218> No need for `else`. 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140216> Maybe `s/weak_socket/weak_self/`? 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140214> Add space after `//`: `// executed.` 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140215> `s/un-necessarily/unnecessarily/` 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140220> Now that we don't capture `this` and use `self`, the `NOTE` is inaccurate. > Note: The 'self' needs to be explicitly captured because we're not using it in the body of the lambda. 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140212> No need for `else`. 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140219> Now that we don't capture `this` and use `self`, the `NOTE` is inaccurate. > Note: The 'self' needs to be explicitly captured because we're not using it in the body of the lambda. 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140213> The `size` is captured here but not used? 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140221> No need for `else`. 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140222> Now that we don't capture `this` and use `self`, the `NOTE` is inaccurate. > Note: The 'self' needs to be explicitly captured because we're not using it in the body of the lambda. 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140223> No need for `else`. 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140224> Now that we don't capture `this` and use `self`, the `NOTE` is inaccurate. > Note: The 'self' needs to be explicitly captured because we're not using it in the body of the lambda. 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140230> Remove newline. 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140226> Can we move this below the `bufferevent_setcb` call? That way we construct it as late as possible, and we can also do: `Socket socket = Socket::Impl::socket(std::move(impl));` 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140227> `char error_buffer[1024] = {};` 3rdparty/libprocess/src/libevent_ssl_socket.cpp <https://reviews.apache.org/r/29406/#comment140228> `ERR_error_string_n(openssl_error, error_buffer, sizeof(error_buffer));` Also, I don't think we need `sizeof(error_buffer) - 1`. > ERR_error_string_n() is a variant of ERR_error_string() that writes at most `len` characters (__including the terminating 0__) and truncates the string if necessary. For ERR_error_string_n(), buf may not be NULL. 3rdparty/libprocess/src/openssl.cpp <https://reviews.apache.org/r/29406/#comment140231> Remove newline. - Michael Park On June 13, 2015, 8:02 a.m., Joris Van Remoortere wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29406/ > ----------------------------------------------------------- > > (Updated June 13, 2015, 8:02 a.m.) > > > Review request for mesos, Benjamin Hindman and Michael Park. > > > Bugs: MESOS-1913 > https://issues.apache.org/jira/browse/MESOS-1913 > > > Repository: mesos > > > Description > ------- > > Requires: > configure --enable-libevent --enable-libevent-socket --enable-ssl > New environment variables: > ``` > SSL_ENABLED=(false|0,true|1) > SSL_CERT_FILE=(path to certificate) > SSL_KEY_FILE=(path to key) > SSL_VERIFY_CERT=(false|0,true|1) > SSL_REQUIRE_CERT=(false|0,true|1) > SSL_VERIFY_DEPTH=(4) > SSL_CA_DIR=(path to CA directory) > SSL_CA_FILE=(path to CA file) > SSL_CIPHERS=(accepted ciphers separated by ':') > SSL_ENABLE_SSL_V2=(false|0,true|1) > SSL_ENABLE_SSL_V3=(false|0,true|1) > SSL_ENABLE_TLS_V1_0=(false|0,true|1) > SSL_ENABLE_TLS_V1_1=(false|0,true|1) > SSL_ENABLE_TLS_V1_2=(false|0,true|1) > ``` > > Only TLSV1.2 is enabled by default. > Use the `ENABLE_SSL_V*` and `ENABLE_TLS_V*` environment variables to open up > more protocols. > Use the `SSL_CIPHERS` environment variable to restrict or open up the > supported ciphers. > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am 489ce359f383d819299335cbaa8c95724b0c6ac2 > 3rdparty/libprocess/include/process/address.hpp > 729f5cd7ea981e43a33c1fe9d99d58b906a31158 > 3rdparty/libprocess/include/process/socket.hpp > b8c2274de535ac473e49a09165b601c96d3ebe8b > 3rdparty/libprocess/src/libevent.hpp > f6cc72178613a30446629532a773afccfd404212 > 3rdparty/libprocess/src/libevent.cpp > fb038597358135a06c1927d079cb7cb09fea7452 > 3rdparty/libprocess/src/libevent_ssl_socket.hpp PRE-CREATION > 3rdparty/libprocess/src/libevent_ssl_socket.cpp PRE-CREATION > 3rdparty/libprocess/src/openssl.hpp PRE-CREATION > 3rdparty/libprocess/src/openssl.cpp PRE-CREATION > 3rdparty/libprocess/src/process.cpp > aadd7bb0ae12b93336900c76d8d5aaa4421ea198 > 3rdparty/libprocess/src/socket.cpp 0e1cebb19e21c706b152d35a0b8722924c971a35 > > Diff: https://reviews.apache.org/r/29406/diff/ > > > Testing > ------- > > make check (uses non-ssl socket) > benchmarks using ssl sockets > master, slave, framework, webui launch with ssl sockets > > > Thanks, > > Joris Van Remoortere > >