[GitHub] [trafficserver] zwoop commented on a change in pull request #8337: Adding TLS session key logging capability

[GitBox <gi...@apache.org>]

zwoop commented on a change in pull request #8337:
URL: https://github.com/apache/trafficserver/pull/8337#discussion_r713085335



##########
File path: iocore/net/SSLClientUtils.cc
##########
@@ -234,6 +235,12 @@ SSLInitClientContext(const SSLConfigParams *params)
     SSL_CTX_sess_set_new_cb(client_ctx, ssl_new_session_callback);
   }
 
+  if (TLSKeyLogger::is_enabled()) {
+#if OPENSSL_VERSION_NUMBER >= 0x010100000
+    SSL_CTX_set_keylog_callback(client_ctx, TLSKeyLogger::ssl_keylog_cb);

Review comment:
       Why not put the conditional inside the OPENSSL_VERSION_NUMBER check? And probably an "unlikely" as well ?

##########
File path: iocore/net/SSLUtils.cc
##########
@@ -100,10 +102,97 @@ static int ssl_vc_index = -1;
 static ink_mutex *mutex_buf      = nullptr;
 static bool open_ssl_initialized = false;
 
-/* Using pthread thread ID and mutex functions directly, instead of
- * ATS this_ethread / ProxyMutex, so that other linked libraries
- * may use pthreads and openssl without confusing us here. (TS-2271).
- */
+TLSKeyLogger::TLSKeyLogger()
+{

Review comment:
       These simple ctor's etc., why not put them in the include file where they become implicitly inlined? I'd say anything short than a few simple lines should go there :-).




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org