git commit: [TS-3085] Large POSTs over (relatively) slower connections failing in ats5 Call ERR_get_error_line_data() via SSL_CLR_ERR_INCR_DYN_STAT to clean up the error queue after processing an SSL_

2014-09-24 Thread sudheerv 
Repository: trafficserver
Updated Branches:
  refs/heads/master 17bef772a - d12327d84


[TS-3085] Large POSTs over (relatively) slower connections failing in ats5
Call ERR_get_error_line_data() via SSL_CLR_ERR_INCR_DYN_STAT to clean up
the error queue after processing an SSL_ERROR_SSL. Also, added consistent
wrappers to SSL I/O functions and removed errno based looping on SSL_Write
per James Peach's recommendations.  Note that, this change removes the apparent
logic of reattempting SSL_Write on transient error cases (such as ENOBUF), since
openSSL documentation doesn't indicate that errno is set during SSL_Write.
https://www.openssl.org/docs/ssl/SSL_write.html


Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/d12327d8
Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/d12327d8
Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/d12327d8

Branch: refs/heads/master
Commit: d12327d841d07bfe1fa772c42272fbc908a39324
Parents: 17bef77
Author: Sudheer Vinukonda sudhe...@yahoo-inc.com
Authored: Wed Sep 24 13:32:56 2014 +
Committer: Sudheer Vinukonda sudhe...@yahoo-inc.com
Committed: Wed Sep 24 13:32:56 2014 +

--
 iocore/net/P_SSLUtils.h | 12 +++
 iocore/net/SSLNetVConnection.cc | 69 
 iocore/net/SSLUtils.cc  | 59 ++
 3 files changed, 93 insertions(+), 47 deletions(-)
--


http://git-wip-us.apache.org/repos/asf/trafficserver/blob/d12327d8/iocore/net/P_SSLUtils.h
--
diff --git a/iocore/net/P_SSLUtils.h b/iocore/net/P_SSLUtils.h
index 82c41b4..269f009 100644
--- a/iocore/net/P_SSLUtils.h
+++ b/iocore/net/P_SSLUtils.h
@@ -38,6 +38,8 @@ struct SSLCertLookup;
 class SSLNetVConnection;
 struct RecRawStatBlock;
 
+typedef int ssl_error_t;
+
 enum SSL_Stats
 {
   ssl_origin_server_expired_cert_stat,
@@ -113,6 +115,12 @@ void SSLInitializeStatistics();
 // Release SSL_CTX and the associated data
 void SSLReleaseContext(SSL_CTX* ctx);
 
+// Wrapper functions to SSL I/O routines
+ssl_error_t SSLWriteBuffer(SSL * ssl, const void * buf, size_t nbytes, size_t 
nwritten);
+ssl_error_t SSLReadBuffer(SSL * ssl, void * buf, size_t nbytes, size_t nread);
+ssl_error_t SSLAccept(SSL *ssl);
+ssl_error_t SSLConnect(SSL * ssl);
+
 // Log an SSL error.
 #define SSLError(fmt, ...) SSLDiagnostic(DiagsMakeLocation(), false, NULL, 
fmt, ##__VA_ARGS__)
 #define SSLErrorVC(vc,fmt, ...) SSLDiagnostic(DiagsMakeLocation(), false, vc, 
fmt, ##__VA_ARGS__)
@@ -120,6 +128,10 @@ void SSLReleaseContext(SSL_CTX* ctx);
 #define SSLDebug(fmt, ...) SSLDiagnostic(DiagsMakeLocation(), true, NULL, fmt, 
##__VA_ARGS__)
 #define SSLDebugVC(vc,fmt, ...) SSLDiagnostic(DiagsMakeLocation(), true, vc, 
fmt, ##__VA_ARGS__)
 
+#define SSL_CLR_ERR_INCR_DYN_STAT(x, fmt, ...) \
+  SSLDiagnostic(DiagsMakeLocation(), true, NULL, fmt, ##__VA_ARGS__); \
+  RecIncrRawStat(ssl_rsb, NULL, (int) x, 1);
+
 void SSLDiagnostic(const SrcLoc loc, bool debug, SSLNetVConnection * vc, 
const char * fmt, ...) TS_PRINTFLIKE(4, 5);
 
 // Return a static string name for a SSL_ERROR constant.

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/d12327d8/iocore/net/SSLNetVConnection.cc
--
diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc
index b4269e7..0f26679 100644
--- a/iocore/net/SSLNetVConnection.cc
+++ b/iocore/net/SSLNetVConnection.cc
@@ -172,24 +172,6 @@ debug_certificate_name(const char * msg, X509_NAME * name)
   BIO_free(bio);
 }
 
-static inline int
-do_SSL_write(SSL * ssl, void *buf, int size)
-{
-  int r = 0;
-  do {
-// need to check into SSL error handling
-// to see if this is good enough.
-r = SSL_write(ssl, (const char *) buf, size);
-if (r = 0)
-  return r;
-else
-  r = -errno;
-  } while (r == -EINTR || r == -ENOBUFS || r == -ENOMEM);
-
-  return r;
-}
-
-
 static int
 ssl_read_from_net(SSLNetVConnection * sslvc, EThread * lthread, int64_t ret)
 {
@@ -199,7 +181,8 @@ ssl_read_from_net(SSLNetVConnection * sslvc, EThread * 
lthread, int64_t ret)
   int event = SSL_READ_ERROR_NONE;
   int64_t bytes_read;
   int64_t block_write_avail;
-  int sslErr = SSL_ERROR_NONE;
+  ssl_error_t sslErr = SSL_ERROR_NONE;
+  int nread = 0;
 
   for (bytes_read = 0; (b != 0)  (sslErr == SSL_ERROR_NONE); b = b-next) {
 block_write_avail = b-write_avail();
@@ -209,23 +192,22 @@ ssl_read_from_net(SSLNetVConnection * sslvc, EThread * 
lthread, int64_t ret)
 int64_t offset = 0;
 // while can be replaced with if - need to test what works faster with 
openssl
 while (block_write_avail  0) {
-  int rres = SSL_read(sslvc-ssl, b-end() + offset,

Re: git commit: [TS-3085] Large POSTs over (relatively) slower connections failing in ats5 Call ERR_get_error_line_data() via SSL_CLR_ERR_INCR_DYN_STAT to clean up the error queue after processing an

2014-09-24 Thread James Peach 
On Sep 24, 2014, at 6:40 AM, sudhe...@apache.org wrote:

 Repository: trafficserver
 Updated Branches:
  refs/heads/master 17bef772a - d12327d84
 
 
 [TS-3085] Large POSTs over (relatively) slower connections failing in ats5
 Call ERR_get_error_line_data() via SSL_CLR_ERR_INCR_DYN_STAT to clean up
 the error queue after processing an SSL_ERROR_SSL. Also, added consistent
 wrappers to SSL I/O functions and removed errno based looping on SSL_Write
 per James Peach's recommendations.  Note that, this change removes the 
 apparent
 logic of reattempting SSL_Write on transient error cases (such as ENOBUF), 
 since
 openSSL documentation doesn't indicate that errno is set during SSL_Write.
 https://www.openssl.org/docs/ssl/SSL_write.html
 
 
 Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
 Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/d12327d8
 Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/d12327d8
 Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/d12327d8
 
 Branch: refs/heads/master
 Commit: d12327d841d07bfe1fa772c42272fbc908a39324
 Parents: 17bef77
 Author: Sudheer Vinukonda sudhe...@yahoo-inc.com
 Authored: Wed Sep 24 13:32:56 2014 +
 Committer: Sudheer Vinukonda sudhe...@yahoo-inc.com
 Committed: Wed Sep 24 13:32:56 2014 +
 
 --
 iocore/net/P_SSLUtils.h | 12 +++
 iocore/net/SSLNetVConnection.cc | 69 
 iocore/net/SSLUtils.cc  | 59 ++
 3 files changed, 93 insertions(+), 47 deletions(-)
 --
 
 
 http://git-wip-us.apache.org/repos/asf/trafficserver/blob/d12327d8/iocore/net/P_SSLUtils.h
 --
 diff --git a/iocore/net/P_SSLUtils.h b/iocore/net/P_SSLUtils.h
 index 82c41b4..269f009 100644
 --- a/iocore/net/P_SSLUtils.h
 +++ b/iocore/net/P_SSLUtils.h
 @@ -38,6 +38,8 @@ struct SSLCertLookup;
 class SSLNetVConnection;
 struct RecRawStatBlock;
 
 +typedef int ssl_error_t;
 +
 enum SSL_Stats
 {
   ssl_origin_server_expired_cert_stat,
 @@ -113,6 +115,12 @@ void SSLInitializeStatistics();
 // Release SSL_CTX and the associated data
 void SSLReleaseContext(SSL_CTX* ctx);
 
 +// Wrapper functions to SSL I/O routines
 +ssl_error_t SSLWriteBuffer(SSL * ssl, const void * buf, size_t nbytes, 
 size_t nwritten);
 +ssl_error_t SSLReadBuffer(SSL * ssl, void * buf, size_t nbytes, size_t 
 nread);
 +ssl_error_t SSLAccept(SSL *ssl);
 +ssl_error_t SSLConnect(SSL * ssl);
 +
 // Log an SSL error.
 #define SSLError(fmt, ...) SSLDiagnostic(DiagsMakeLocation(), false, NULL, 
 fmt, ##__VA_ARGS__)
 #define SSLErrorVC(vc,fmt, ...) SSLDiagnostic(DiagsMakeLocation(), false, vc, 
 fmt, ##__VA_ARGS__)
 @@ -120,6 +128,10 @@ void SSLReleaseContext(SSL_CTX* ctx);
 #define SSLDebug(fmt, ...) SSLDiagnostic(DiagsMakeLocation(), true, NULL, 
 fmt, ##__VA_ARGS__)
 #define SSLDebugVC(vc,fmt, ...) SSLDiagnostic(DiagsMakeLocation(), true, vc, 
 fmt, ##__VA_ARGS__)
 
 +#define SSL_CLR_ERR_INCR_DYN_STAT(x, fmt, ...) \
 +  SSLDiagnostic(DiagsMakeLocation(), true, NULL, fmt, ##__VA_ARGS__); \
 +  RecIncrRawStat(ssl_rsb, NULL, (int) x, 1);
 +

Pleas wrap this in a do { } while(0) ...

 void SSLDiagnostic(const SrcLoc loc, bool debug, SSLNetVConnection * vc, 
 const char * fmt, ...) TS_PRINTFLIKE(4, 5);
 
 // Return a static string name for a SSL_ERROR constant.
 
 http://git-wip-us.apache.org/repos/asf/trafficserver/blob/d12327d8/iocore/net/SSLNetVConnection.cc
 --
 diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc
 index b4269e7..0f26679 100644
 --- a/iocore/net/SSLNetVConnection.cc
 +++ b/iocore/net/SSLNetVConnection.cc
 @@ -172,24 +172,6 @@ debug_certificate_name(const char * msg, X509_NAME * 
 name)
   BIO_free(bio);
 }
 
 -static inline int
 -do_SSL_write(SSL * ssl, void *buf, int size)
 -{
 -  int r = 0;
 -  do {
 -// need to check into SSL error handling
 -// to see if this is good enough.
 -r = SSL_write(ssl, (const char *) buf, size);
 -if (r = 0)
 -  return r;
 -else
 -  r = -errno;
 -  } while (r == -EINTR || r == -ENOBUFS || r == -ENOMEM);
 -
 -  return r;
 -}
 -
 -
 static int
 ssl_read_from_net(SSLNetVConnection * sslvc, EThread * lthread, int64_t ret)
 {
 @@ -199,7 +181,8 @@ ssl_read_from_net(SSLNetVConnection * sslvc, EThread * 
 lthread, int64_t ret)
   int event = SSL_READ_ERROR_NONE;
   int64_t bytes_read;
   int64_t block_write_avail;
 -  int sslErr = SSL_ERROR_NONE;
 +  ssl_error_t sslErr = SSL_ERROR_NONE;
 +  int nread = 0;
 
   for (bytes_read = 0; (b != 0)  (sslErr == SSL_ERROR_NONE); b = b-next) {
 block_write_avail = b-write_avail();
 @@ -209,23 +192,22 @@ ssl_read_from_net(SSLNetVConnection * sslvc, EThread * 
 lthread, int64_t ret)
 int64_t