> On Nov. 7, 2019, 4:33 p.m., Greg Mann wrote: > > 3rdparty/libprocess/src/ssl/socket_wrapper.hpp > > Lines 24-28 (patched) > > <https://reviews.apache.org/r/71660/diff/2/?file=2170109#file2170109line24> > > > > What do you think about making `SSLSocketWrapper` a true wrapper of the > > underlying socket? In other words, let's make the wrapper a subclass of > > `SocketImpl` and have it hold a `SocketImpl` member which is the underlying > > socket that it's using for communication. We could add a `create()` method > > and a constructor like this: > > ``` > > Try<std::shared_ptr<SocketImpl>> SSLSocketWrapper::create(const > > SocketImpl& socketImpl_) > > { > > openssl::initialize(); > > > > if (!openssl::flags().enabled) { > > return Error("SSL is disabled"); > > } > > > > return std::make_shared<SSLSocketWrapper>(socketImpl_); > > } > > > > > > SSLSocketWrapper::SSLSocketWrapper(const SocketImpl& socketImpl_) > > : socketImpl(socketImpl_), > > ssl(nullptr) {} > > ``` > > > > then we could call methods like `connect()` and `accept()` directly on > > the held socket within the wrapper code. > > > > It's a small structural change, but I think it would help to > > communicate to readers how the underlying socket and the SSL wrapper are > > related. Further, it makes it more obvious how we could evolve the wrapper > > class into some kind of `Server` and `Client` objects which are initialized > > with a socket. > > > > WDYT? > > Greg Mann wrote: > The `SocketWrapperBIOData` could also hold a `SocketImpl` member instead > of an `int_fd`, right?
The main implementation hurdle I see from making `SSLSocketWrapper` hold a `SocketImpl` member, is that the `SSLSocketWrapper` itself then cannot easily be a subclass of `SocketImpl`. When I tried to add a `PollSocketImpl` member variable, the underlying socket `int_fd` ownership became obscure, since both the SSLSocketWrapper and the PollSocketImpl need to own the `int_fd` in order to be useful. Having the `SSLSocketWrapper` subclass a `SocketImpl` is useful for all the areas that use Socket objects. This obstraction lets us parameterize tons of our tests for SSL and non-SSL. - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71660/#review218573 ----------------------------------------------------------- On Oct. 23, 2019, 6:41 p.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71660/ > ----------------------------------------------------------- > > (Updated Oct. 23, 2019, 6:41 p.m.) > > > Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till > Toenshoff. > > > Bugs: MESOS-10009 > https://issues.apache.org/jira/browse/MESOS-10009 > > > Repository: mesos > > > Description > ------- > > This creates some new files implementing the SocketImpl class, > in preparation for implementing generic SSL sockets. > > The new class is used whenever SSL is enabled, but Libevent is not. > > > Diffs > ----- > > 3rdparty/libprocess/src/CMakeLists.txt > 40c8ef935104bab4ea2f8b2b5919450b55165f60 > 3rdparty/libprocess/src/openssl.cpp > bd05866950e1043d9585a7c5fdc7b2147a233fd3 > 3rdparty/libprocess/src/socket.cpp 606a1c46e50936251c29af4b996007c480b2a135 > 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION > 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/71660/diff/2/ > > > Testing > ------- > > cmake .. -ENABLE_SSL=1 # No Libevent here! > > > Thanks, > > Joseph Wu > >
