This doesn't look like it addresses Alan's last comment about "unifying the SSL error reporting logic"
> On Jan 12, 2016, at 7:48 AM, shinr...@apache.org wrote: > > Repository: trafficserver > Updated Branches: > refs/heads/master 9399a7641 -> bb40d788b > > > [TS-4024] wire tracing enhancements. This closes #337. > > > Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo > Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/bb40d788 > Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/bb40d788 > Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/bb40d788 > > Branch: refs/heads/master > Commit: bb40d788bb092670b05e3d084bc14e330e3afa96 > Parents: 9399a76 > Author: ericcarlschwartz <eschwartz1...@gmail.com> > Authored: Sat Nov 14 13:26:58 2015 -0800 > Committer: shinrich <shinr...@yahoo-inc.com> > Committed: Tue Jan 12 09:44:00 2016 -0600 > > ---------------------------------------------------------------------- > iocore/net/SSLConfig.cc | 8 ++++---- > iocore/net/SSLNetVConnection.cc | 39 ++++++++++++++++++++++++++---------- > mgmt/RecordsConfig.cc | 8 ++++---- > proxy/http/HttpSM.cc | 31 ++++++++++++++-------------- > 4 files changed, 52 insertions(+), 34 deletions(-) > ---------------------------------------------------------------------- > > > http://git-wip-us.apache.org/repos/asf/trafficserver/blob/bb40d788/iocore/net/SSLConfig.cc > ---------------------------------------------------------------------- > diff --git a/iocore/net/SSLConfig.cc b/iocore/net/SSLConfig.cc > index 7bb60fe..63540ce 100644 > --- a/iocore/net/SSLConfig.cc > +++ b/iocore/net/SSLConfig.cc > @@ -307,10 +307,10 @@ SSLConfigParams::initialize() > REC_ReadConfigInt32(ssl_allow_client_renegotiation, > "proxy.config.ssl.allow_client_renegotiation"); > > // SSL Wire Trace configurations > - REC_ReadConfigInteger(ssl_wire_trace_enabled, > "proxy.config.ssl.wire_trace_enabled"); > + REC_EstablishStaticConfigInt32(ssl_wire_trace_enabled, > "proxy.config.ssl.wire_trace_enabled"); > if (ssl_wire_trace_enabled) { > // wire trace specific source ip > - REC_ReadConfigStringAlloc(ssl_wire_trace_addr, > "proxy.config.ssl.wire_trace_addr"); > + REC_EstablishStaticConfigStringAlloc(ssl_wire_trace_addr, > "proxy.config.ssl.wire_trace_addr"); > if (ssl_wire_trace_addr) { > ssl_wire_trace_ip = new IpAddr(); > ssl_wire_trace_ip->load(ssl_wire_trace_addr); > @@ -318,8 +318,8 @@ SSLConfigParams::initialize() > ssl_wire_trace_ip = NULL; > } > // wire trace percentage of requests > - REC_ReadConfigInteger(ssl_wire_trace_percentage, > "proxy.config.ssl.wire_trace_percentage"); > - REC_ReadConfigStringAlloc(ssl_wire_trace_server_name, > "proxy.config.ssl.wire_trace_server_name"); > + REC_EstablishStaticConfigInt32(ssl_wire_trace_percentage, > "proxy.config.ssl.wire_trace_percentage"); > + REC_EstablishStaticConfigStringAlloc(ssl_wire_trace_server_name, > "proxy.config.ssl.wire_trace_server_name"); > } else { > ssl_wire_trace_addr = NULL; > ssl_wire_trace_ip = NULL; > > http://git-wip-us.apache.org/repos/asf/trafficserver/blob/bb40d788/iocore/net/SSLNetVConnection.cc > ---------------------------------------------------------------------- > diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc > index 7199efa..dc48c63 100644 > --- a/iocore/net/SSLNetVConnection.cc > +++ b/iocore/net/SSLNetVConnection.cc > @@ -286,12 +286,16 @@ ssl_read_from_net(SSLNetVConnection *sslvc, EThread > *lthread, int64_t &ret) > Debug("ssl.error", "[SSL_NetVConnection::ssl_read_from_net] > SSL_ERROR_ZERO_RETURN"); > break; > case SSL_ERROR_SSL: > - default: > - TraceIn(trace, sslvc->get_remote_addr(), sslvc->get_remote_port(), > "SSL Error: sslErr=%d, errno=%d", sslErr, errno); > + default: { > + char buf[512]; > + unsigned long e = ERR_peek_last_error(); > + ERR_error_string_n(e, buf, sizeof(buf)); > + TraceIn(trace, sslvc->get_remote_addr(), sslvc->get_remote_port(), > "SSL Error: sslErr=%d, ERR_get_error=%ld (%s) errno=%d", > + sslErr, e, buf, errno); > event = SSL_READ_ERROR; > ret = errno; > SSL_CLR_ERR_INCR_DYN_STAT(sslvc, ssl_error_ssl, > "[SSL_NetVConnection::ssl_read_from_net]: errno=%d", errno); > - break; > + } break; > } // switch > break; > } // while( block_write_avail > 0 ) > @@ -833,11 +837,15 @@ SSLNetVConnection::load_buffer_and_write(int64_t > towrite, int64_t &wattempted, i > Debug("ssl.error", "SSL_write-SSL_ERROR_ZERO_RETURN"); > break; > case SSL_ERROR_SSL: > - default: > - TraceOut(trace, get_remote_addr(), get_remote_port(), "SSL Error: > sslErr=%d, errno=%d", err, errno); > + default: { > + char buf[512]; > + unsigned long e = ERR_peek_last_error(); > + ERR_error_string_n(e, buf, sizeof(buf)); > + TraceIn(trace, get_remote_addr(), get_remote_port(), "SSL Error: > sslErr=%d, ERR_get_error=%ld (%s) errno=%d", err, e, buf, > + errno); > r = -errno; > SSL_CLR_ERR_INCR_DYN_STAT(this, ssl_error_ssl, "SSL_write-SSL_ERROR_SSL > errno=%d", errno); > - break; > + } break; > } > return (r); > } > @@ -1232,10 +1240,15 @@ SSLNetVConnection::sslServerHandShakeEvent(int &err) > TraceIn(trace, get_remote_addr(), get_remote_port(), "SSL server > handshake ERROR_WANT_ACCEPT"); > return EVENT_CONT; > > - case SSL_ERROR_SSL: > + case SSL_ERROR_SSL: { > SSL_CLR_ERR_INCR_DYN_STAT(this, ssl_error_ssl, > "SSLNetVConnection::sslServerHandShakeEvent, SSL_ERROR_SSL errno=%d", errno); > - TraceIn(trace, get_remote_addr(), get_remote_port(), "SSL server > handshake ERROR_SSL"); > + char buf[512]; > + unsigned long e = ERR_peek_last_error(); > + ERR_error_string_n(e, buf, sizeof(buf)); > + TraceIn(trace, get_remote_addr(), get_remote_port(), > + "SSL server handshake ERROR_SSL: sslErr=%d, ERR_get_error=%ld > (%s) errno=%d", ssl_error, e, buf, errno); > return EVENT_ERROR; > + } > > case SSL_ERROR_ZERO_RETURN: > TraceIn(trace, get_remote_addr(), get_remote_port(), "SSL server > handshake ERROR_ZERO_RETURN"); > @@ -1335,15 +1348,19 @@ SSLNetVConnection::sslClientHandShakeEvent(int &err) > > > case SSL_ERROR_SSL: > - default: > + default: { > err = errno; > // FIXME -- This triggers a retry on cases of cert validation errors.... > Debug("ssl", "SSLNetVConnection::sslClientHandShakeEvent, SSL_ERROR_SSL"); > SSL_CLR_ERR_INCR_DYN_STAT(this, ssl_error_ssl, > "SSLNetVConnection::sslClientHandShakeEvent, SSL_ERROR_SSL errno=%d", errno); > Debug("ssl.error", "SSLNetVConnection::sslClientHandShakeEvent, > SSL_ERROR_SSL"); > - TraceIn(trace, get_remote_addr(), get_remote_port(), "SSL client > handshake SSL_ERROR"); > + char buf[512]; > + unsigned long e = ERR_peek_last_error(); > + ERR_error_string_n(e, buf, sizeof(buf)); > + TraceIn(trace, get_remote_addr(), get_remote_port(), > + "SSL client handshake ERROR_SSL: sslErr=%d, ERR_get_error=%ld > (%s) errno=%d", ssl_error, e, buf, errno); > return EVENT_ERROR; > - break; > + } break; > } > return EVENT_CONT; > } > > http://git-wip-us.apache.org/repos/asf/trafficserver/blob/bb40d788/mgmt/RecordsConfig.cc > ---------------------------------------------------------------------- > diff --git a/mgmt/RecordsConfig.cc b/mgmt/RecordsConfig.cc > index c390ac5..34e180d 100644 > --- a/mgmt/RecordsConfig.cc > +++ b/mgmt/RecordsConfig.cc > @@ -1304,13 +1304,13 @@ static const RecordElement RecordsConfig[] = > , > {RECT_CONFIG, "proxy.config.ssl.handshake_timeout_in", RECD_INT, "0", > RECU_RESTART_TS, RR_NULL, RECC_INT, "[0-65535]", RECA_NULL} > , > - {RECT_CONFIG, "proxy.config.ssl.wire_trace_enabled", RECD_INT, "0", > RECU_RESTART_TS, RR_NULL, RECC_INT, "[0-2]", RECA_NULL} > + {RECT_CONFIG, "proxy.config.ssl.wire_trace_enabled", RECD_INT, "0", > RECU_DYNAMIC, RR_NULL, RECC_INT, "[0-2]", RECA_NULL} > , > - {RECT_CONFIG, "proxy.config.ssl.wire_trace_addr", RECD_STRING, NULL , > RECU_RESTART_TS, RR_NULL, RECC_IP, "[0-255]\\.[0-255]\\.[0-255]\\.[0-255]", > RECA_NULL} > + {RECT_CONFIG, "proxy.config.ssl.wire_trace_addr", RECD_STRING, NULL , > RECU_DYNAMIC, RR_NULL, RECC_IP, "[0-255]\\.[0-255]\\.[0-255]\\.[0-255]", > RECA_NULL} > , > - {RECT_CONFIG, "proxy.config.ssl.wire_trace_percentage", RECD_INT, "0", > RECU_RESTART_TS, RR_NULL, RECC_INT, "[0-100]", RECA_NULL} > + {RECT_CONFIG, "proxy.config.ssl.wire_trace_percentage", RECD_INT, "0", > RECU_DYNAMIC, RR_NULL, RECC_INT, "[0-100]", RECA_NULL} > , > - {RECT_CONFIG, "proxy.config.ssl.wire_trace_server_name", RECD_STRING, NULL > , RECU_RESTART_TS, RR_NULL, RECC_STR, ".*", RECA_NULL} > + {RECT_CONFIG, "proxy.config.ssl.wire_trace_server_name", RECD_STRING, NULL > , RECU_DYNAMIC, RR_NULL, RECC_STR, ".*", RECA_NULL} > , > > //############################################################################## > //# > > http://git-wip-us.apache.org/repos/asf/trafficserver/blob/bb40d788/proxy/http/HttpSM.cc > ---------------------------------------------------------------------- > diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc > index 5fae64c..ab97380 100644 > --- a/proxy/http/HttpSM.cc > +++ b/proxy/http/HttpSM.cc > @@ -5636,24 +5636,25 @@ HttpSM::attach_server_session(HttpServerSession *s) > > // es - is this a concern here in HttpSM? Does it belong somewhere else? > // Get server and client connections > - UnixNetVConnection *server_vc = (UnixNetVConnection > *)(server_session->get_netvc()); > + UnixNetVConnection *server_vc = dynamic_cast<UnixNetVConnection > *>(server_session->get_netvc()); > UnixNetVConnection *client_vc = (UnixNetVConnection > *)(ua_session->get_netvc()); > SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(client_vc); > - if (ssl_vc != NULL) { // if incoming connection is SSL > - bool client_trace = ssl_vc->getSSLTrace(); > - if (client_trace) { > - // get remote address and port to mark corresponding traces > - const sockaddr *remote_addr = ssl_vc->get_remote_addr(); > - uint16_t remote_port = ssl_vc->get_remote_port(); > - server_vc->setOriginTrace(true); > - server_vc->setOriginTraceAddr(remote_addr); > - server_vc->setOriginTracePort(remote_port); > - } else { > - server_vc->setOriginTrace(false); > - server_vc->setOriginTraceAddr(NULL); > - server_vc->setOriginTracePort(0); > + bool associated_connection = false; > + if (server_vc) { // if server_vc isn't a PluginVC > + if (ssl_vc) { // if incoming connection is SSL > + bool client_trace = ssl_vc->getSSLTrace(); > + if (client_trace) { > + // get remote address and port to mark corresponding traces > + const sockaddr *remote_addr = ssl_vc->get_remote_addr(); > + uint16_t remote_port = ssl_vc->get_remote_port(); > + server_vc->setOriginTrace(true); > + server_vc->setOriginTraceAddr(remote_addr); > + server_vc->setOriginTracePort(remote_port); > + associated_connection = true; > + } > } > - } else { > + } > + if (!associated_connection && server_vc) { > server_vc->setOriginTrace(false); > server_vc->setOriginTraceAddr(NULL); > server_vc->setOriginTracePort(0); >
