Return-Path: X-Original-To: apmail-mesos-dev-archive@www.apache.org Delivered-To: apmail-mesos-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 25E3A1035F for ; Mon, 29 Dec 2014 23:01:42 +0000 (UTC) Received: (qmail 57709 invoked by uid 500); 29 Dec 2014 23:01:42 -0000 Delivered-To: apmail-mesos-dev-archive@mesos.apache.org Received: (qmail 57637 invoked by uid 500); 29 Dec 2014 23:01:42 -0000 Mailing-List: contact dev-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@mesos.apache.org Delivered-To: mailing list dev@mesos.apache.org Received: (qmail 57611 invoked by uid 99); 29 Dec 2014 23:01:40 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 29 Dec 2014 23:01:40 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 741C71D3B04; Mon, 29 Dec 2014 23:01:39 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============4957084775974875918==" MIME-Version: 1.0 Subject: Re: Review Request 29406: Introduce libevent ssl socket. From: "Benjamin Hindman" To: "Niklas Nielsen" , "Benjamin Hindman" Cc: "Joris Van Remoortere" , "mesos" Date: Mon, 29 Dec 2014 23:01:39 -0000 Message-ID: <20141229230139.31225.54654@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Benjamin Hindman" X-ReviewGroup: mesos X-ReviewRequest-URL: https://reviews.apache.org/r/29406/ X-Sender: "Benjamin Hindman" References: <20141224204135.10620.38542@reviews.apache.org> In-Reply-To: <20141224204135.10620.38542@reviews.apache.org> Reply-To: "Benjamin Hindman" X-ReviewRequest-Repository: mesos-git --===============4957084775974875918== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29406/#review66306 ----------------------------------------------------------- 3rdparty/libprocess/src/libevent.cpp The practice in the code base is s/void (void/void(void/, please update here and below, thanks! 3rdparty/libprocess/src/libevent.cpp Please use ThreadLocal until we can just use __thread please. 3rdparty/libprocess/src/libevent.cpp 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 Why is this capitalized? It doesn't look like a constant. 3rdparty/libprocess/src/libevent_ssl_socket.cpp Let's give these defaults, and some comments explaining their purpose please! 3rdparty/libprocess/src/libevent_ssl_socket.cpp 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 s/char */char* / Here and everywhere else please! 3rdparty/libprocess/src/libevent_ssl_socket.cpp s/long)pthread/long) pthread/ 3rdparty/libprocess/src/libevent_ssl_socket.cpp One parameter per line please. Here and everywhere else! 3rdparty/libprocess/src/libevent_ssl_socket.cpp 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 value == NULL Here and everywhere else please! 3rdparty/libprocess/src/libevent_ssl_socket.cpp s/l/value/ to be consistent with the naming above? Here and everywhere else please! 3rdparty/libprocess/src/libevent_ssl_socket.cpp 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 s/ctx/ssl/ to be consistent with the rest of the code in this review. 3rdparty/libprocess/src/libevent_ssl_socket.cpp 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 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 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 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 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 Can we create scopes for these lock blocks? How about: bufferevent_lock(bev); { ...; } bufferevent_unlock(bev); 3rdparty/libprocess/src/libevent_ssl_socket.cpp Unused? 3rdparty/libprocess/src/libevent_ssl_socket.cpp 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 > > --===============4957084775974875918==--