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