mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 29406: Introduce libevent ssl socket.
Date Mon, 29 Dec 2014 23:01:39 GMT

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



3rdparty/libprocess/src/libevent.cpp
<https://reviews.apache.org/r/29406/#comment109708>

    The practice in the code base is s/void (void/void(void/, please update here and below,
thanks!



3rdparty/libprocess/src/libevent.cpp
<https://reviews.apache.org/r/29406/#comment109705>

    Please use ThreadLocal until we can just use __thread please.



3rdparty/libprocess/src/libevent.cpp
<https://reviews.apache.org/r/29406/#comment109706>

    It appears as though you're creating a timer event  in order to do the async loop interrupt.
This does match the libev model fairly well, but is there a better way to do it in libevent?
In particular, can you just inject an event in a thread-safe way directly in run_in_event_loop
by calling event_active? As in, do you have to make an event pending first with event_add?
    
    Either way, a little more documentation here would go a long way. libev has a mechanism
to interrupt the event loop ("async" watchers) so it's slightly more self-documenting.



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109724>

    Why is this capitalized? It doesn't look like a constant.



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109709>

    Let's give these defaults, and some comments explaining their purpose please!



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109710>

    Let's get some comments explaining what each of these structs, functions, and variables
are used for please!



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109711>

    s/char */char* /
    
    Here and everywhere else please!



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109712>

    s/long)pthread/long) pthread/



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109715>

    One parameter per line please. Here and everywhere else!



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109716>

    How about we format this as:
    
    struct CRYPTO_dynlock_value* value = (struct CRYPTO_dynlock_value*)
      malloc(sizeof(struct CRYPTO_dynlock_value));



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109714>

    value == NULL
    
    Here and everywhere else please!



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109717>

    s/l/value/ to be consistent with the naming above? Here and everywhere else please!



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109718>

    Even though I commented on this above, just a friendly reminder to do 'bev != NULL' here
and everywhere else please!



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109720>

    s/ctx/ssl/ to be consistent with the rest of the code in this review.



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109719>

    This is safe to do outside of the event loop? What if the event loop thread is currently
executing the 'recvCb' call? It looks like maybe 'recvCb' and friends needs to take either
a Socket or better the shared_ptr<LibeventSSLSocketImpl> so that we don't prematurely
call delete. And then a comment explaining why it's therefore safe to call these functions
in the destructor would be great!



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109721>

    Maybe another helpful comment here that calling free automatically closes the socket which
is why we don't need to do anything special here?



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109722>

    Let's fully spell out the word 'Callback' here and everywhere else to stay consistent
with our codebase please.



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109702>

    Let's just use CHECK everywhere, rather than both assert and CHECK. And for a case like
this, please use CHECK_NOTNULL. And note that CHECK_NOTNULL returns the pointer, so feel free
to use it to wrapper, for example:
    
    T* t = CHECK_NOTNULL(...pointer...);



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109700>

    Can we create scopes for these lock blocks? How about:
    
    bufferevent_lock(bev);
    {
      ...;
    }
    bufferevent_unlock(bev);



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109725>

    Unused?



3rdparty/libprocess/src/libevent_ssl_socket.cpp
<https://reviews.apache.org/r/29406/#comment109726>

    We really need to document the fact that we're passing in a file descriptor here but we're
overriding it when we do a connect. It's not obvious that we are doing this or why we need
to do this and it could be very confusing to someone coming across the code later. Probably
a good place for this comment would be both here and in LibeventSSLSocketImpl::connect where
you pass in -1 to bufferevent_openssl_socket_new.
    
    Also, it looks like we might be pretty close to being able kill the version of Socket::create
that takes the file descriptor anyway, and I think we want to strive for that long term?


- Benjamin Hindman


On Dec. 24, 2014, 8:41 p.m., Joris Van Remoortere wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29406/
> -----------------------------------------------------------
> 
> (Updated Dec. 24, 2014, 8:41 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Niklas Nielsen.
> 
> 
> Bugs: MESOS-1913
>     https://issues.apache.org/jira/browse/MESOS-1913
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Requires:
> configure --enable-libevent --enable-libevent-socket --enable-ssl
> New environment variables:
> USE_SSL=(0,1)
> SSL_CERT=(path to certificate)
> SSL_KEY=(path to key)
> SSL_VERIFY_CERT=(0,1)
> SSL_REQUIRE_CERT=(0,1)
> SSL_CA_DIR=(path to CA directory)
> SSL_CA_FILE=(path to CA file)
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 75870ac754e500bb4ca689201bde677fa7d854d0 
>   3rdparty/libprocess/include/process/socket.hpp 7e1e3f22583f44a9aea8259bafedc2877ad2e633

>   3rdparty/libprocess/src/libevent.hpp PRE-CREATION 
>   3rdparty/libprocess/src/libevent.cpp PRE-CREATION 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp 028b33e7ecb7e0a39334ac4ab0279ee327a72a56 
>   3rdparty/libprocess/src/socket.cpp PRE-CREATION 
> 
> 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