> On Dec. 2, 2019, 11:23 p.m., Greg Mann wrote: > > 3rdparty/libprocess/src/ssl/socket_wrapper.cpp > > Lines 449 (patched) > > <https://reviews.apache.org/r/71665/diff/5/?file=2174446#file2174446line449> > > > > What do you mean "instead of in `accept`"? Aren't we in `accept` right > > now?
Oops, I originally put this comment in an override of `listen` instead. > On Dec. 2, 2019, 11:23 p.m., Greg Mann wrote: > > 3rdparty/libprocess/src/ssl/socket_wrapper.cpp > > Lines 460-464 (patched) > > <https://reviews.apache.org/r/71665/diff/5/?file=2174446#file2174446line460> > > > > Is it possible to switch `PollSocketImpl::accept()` to use a weak > > reference instead of adding this code here? Playing around with this a bit more, it may be necessary to make the change in PollSocketImpl. On Windows, there is no equivalent of `io::poll`, so I need to make the Windows poll socket into a weak pointer anyway. We can limit the change of shared pointers into weak pointers to the `accept` and `connect` methods, since those are the two methods called by the SSL socket. But it wouldn't hurt to change all the poll socket's shared pointers into weak pointers (there might be a slight performance penalty associated with grabbing all these shared pointer locks though). > On Dec. 2, 2019, 11:23 p.m., Greg Mann wrote: > > 3rdparty/libprocess/src/ssl/socket_wrapper.cpp > > Lines 506 (patched) > > <https://reviews.apache.org/r/71665/diff/5/?file=2174446#file2174446line506> > > > > What is the "configure callback"? Could you be more explicit/verbose > > here? This terminology refers to this note in `libprocess/src/openssl.hpp`: ``` // Callback for setting SSL options after the TCP connection was // established but before the TLS handshake has started. Try<Nothing> configure_socket( SSL* ssl, Mode mode, const Address& peer, const Option<std::string>& peer_hostname); ``` This code was copied from the libevent SSL socket's accept logic too. > On Dec. 2, 2019, 11:23 p.m., Greg Mann wrote: > > 3rdparty/libprocess/src/ssl/socket_wrapper.cpp > > Lines 557-558 (patched) > > <https://reviews.apache.org/r/71665/diff/5/?file=2174446#file2174446line557> > > > > Why vlog this failure case but not the others? Should we have logging > > for all these cases, or none? This log was also copied from libevent. I can add more logging where it makes sense to. - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71665/#review218881 ----------------------------------------------------------- On Nov. 11, 2019, 11:40 a.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71665/ > ----------------------------------------------------------- > > (Updated Nov. 11, 2019, 11:40 a.m.) > > > Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till > Toenshoff. > > > Bugs: MESOS-10010 > https://issues.apache.org/jira/browse/MESOS-10010 > > > Repository: mesos > > > Description > ------- > > This fills in some of the SSL socket implementation, > in particular the constructor, destructor, connect(), > and accept() methods. > > Much of the setup and verification is taken verbatim from the > libevent socket implementation. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/socket.hpp > 48860f8646d388685f0a60ad2a2f613b1f4be61a > 3rdparty/libprocess/src/ssl/socket_wrapper.hpp PRE-CREATION > 3rdparty/libprocess/src/ssl/socket_wrapper.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/71665/diff/5/ > > > Testing > ------- > > cmake --build . --target libprocess-tests > > Successfully connected to Google :D > With something like this: > ``` > set_environment_variables({ > {"LIBPROCESS_SSL_ENABLED", "true"}, > {"LIBPROCESS_SSL_KEY_FILE", key_path().string()}, > {"LIBPROCESS_SSL_CERT_FILE", certificate_path().string()} > }); > > Try<Socket> client = Socket::create(SocketImpl::Kind::SSL); > ASSERT_SOME(client); > > AWAIT_ASSERT_READY(client->connect( > network::inet::Address(net::IP::parse("216.58.194.206").get(), 443), > openssl::create_tls_client_config(None()))); > ``` > > > Thanks, > > Joseph Wu > >
