mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mcyp...@gmail.com>
Subject Re: Review Request 29406: Introduce libevent ssl socket.
Date Sat, 13 Jun 2015 08:05:34 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message