You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2020/08/28 20:31:11 UTC
[GitHub] [trafficserver] shinrich opened a new pull request #7150: Additions to enable loading qat_engine
shinrich opened a new pull request #7150:
URL: https://github.com/apache/trafficserver/pull/7150
Had to make some changes so ATS could load the qat_engine (https://github.com/intel/QAT_Engine). The main one was being more adaptable about loading the private key if the crypto engine did not have specific support for private keys (QAT does not).
I also added an explicit ep for the QAT communication. @oknet suggested that in his review of my last async job PR. With the QAT engine being called multiple times in a handshake this seemed like a good optimization.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7150: Additions to enable loading qat_engine
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7150:
URL: https://github.com/apache/trafficserver/pull/7150#issuecomment-683134155
[approve ci docs]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7150: Additions to enable loading qat_engine
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7150:
URL: https://github.com/apache/trafficserver/pull/7150#issuecomment-705762764
Cherry-picked to v9.0.x branch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #7150: Additions to enable loading qat_engine
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #7150:
URL: https://github.com/apache/trafficserver/pull/7150#discussion_r482327252
##########
File path: iocore/net/SSLNetVConnection.cc
##########
@@ -1224,42 +1230,30 @@ SSLNetVConnection::sslServerHandShakeEvent(int &err)
} // Still data in the BIO
}
-#if TS_USE_TLS_ASYNC
- if (SSLConfigParams::async_handshake_enabled) {
- SSL_set_mode(ssl, SSL_MODE_ASYNC);
- }
-#endif
ssl_error_t ssl_error = SSLAccept(ssl);
#if TS_USE_TLS_ASYNC
if (ssl_error == SSL_ERROR_WANT_ASYNC) {
- size_t numfds;
- OSSL_ASYNC_FD *waitfds;
- // Set up the epoll entry for the signalling
- if (SSL_get_all_async_fds(ssl, nullptr, &numfds) && numfds > 0) {
- // Allocate space for the waitfd on the stack, should only be one most all of the time
- waitfds = reinterpret_cast<OSSL_ASYNC_FD *>(alloca(sizeof(OSSL_ASYNC_FD) * numfds));
- if (SSL_get_all_async_fds(ssl, waitfds, &numfds) && numfds > 0) {
- // Temporarily disable regular net
- this->read.triggered = false;
- this->write.triggered = false;
- this->ep.stop(); // Modify used in read_disable doesn't work for edge triggered epol
- // Have to have the read NetState enabled because we are using it for the signal vc
- read.enabled = true;
- PollDescriptor *pd = get_PollDescriptor(this_ethread());
- this->ep.start(pd, waitfds[0], static_cast<NetEvent *>(this), EVENTIO_READ);
- this->ep.type = EVENTIO_READWRITE_VC;
+ // Do we need to set up the async eventfd? Or is it already registered?
+ if (async_ep.fd < 0) {
+ size_t numfds;
+ OSSL_ASYNC_FD *waitfds;
+ // Set up the epoll entry for the signalling
+ if (SSL_get_all_async_fds(ssl, nullptr, &numfds) && numfds > 0) {
+ // Allocate space for the waitfd on the stack, should only be one most all of the time
+ waitfds = reinterpret_cast<OSSL_ASYNC_FD *>(alloca(sizeof(OSSL_ASYNC_FD) * numfds));
Review comment:
It looks like the declaration of waitfds can be moved down to line 1243.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7150: Additions to enable loading qat_engine
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7150:
URL: https://github.com/apache/trafficserver/pull/7150#issuecomment-684010133
Added commit to address @maskit's comments.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7150: Additions to enable loading qat_engine
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7150:
URL: https://github.com/apache/trafficserver/pull/7150#discussion_r479836308
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -927,36 +928,40 @@ SSLPrivateKeyHandler(SSL_CTX *ctx, const SSLConfigParams *params, const std::str
} else {
argkey = Layout::get()->relative_to(params->serverKeyPathOnly, keyPath);
}
- if (!SSL_CTX_use_PrivateKey(ctx, ENGINE_load_private_key(e, argkey, nullptr, nullptr))) {
- SSLError("failed to load server private key from engine");
+ pkey = ENGINE_load_private_key(e, argkey.get(), nullptr, nullptr);
+ if (pkey) {
+ if (!SSL_CTX_use_PrivateKey(ctx, pkey)) {
+ SSLError("failed to load server private key from engine");
+ }
}
+ }
#else
ENGINE *e = nullptr;
Review comment:
```
SSLUtils.cc:939:11: error: unused variable 'e' [-Werror,-Wunused-variable]
ENGINE *e = nullptr;
^
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7150: Additions to enable loading qat_engine
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7150:
URL: https://github.com/apache/trafficserver/pull/7150#discussion_r479837982
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -927,36 +928,40 @@ SSLPrivateKeyHandler(SSL_CTX *ctx, const SSLConfigParams *params, const std::str
} else {
argkey = Layout::get()->relative_to(params->serverKeyPathOnly, keyPath);
}
- if (!SSL_CTX_use_PrivateKey(ctx, ENGINE_load_private_key(e, argkey, nullptr, nullptr))) {
- SSLError("failed to load server private key from engine");
+ pkey = ENGINE_load_private_key(e, argkey.get(), nullptr, nullptr);
+ if (pkey) {
+ if (!SSL_CTX_use_PrivateKey(ctx, pkey)) {
+ SSLError("failed to load server private key from engine");
Review comment:
Not really new, but seems like we should return false here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #7150: Additions to enable loading qat_engine
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7150:
URL: https://github.com/apache/trafficserver/pull/7150#issuecomment-684010009
[approve ci docs]
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #7150: Additions to enable loading qat_engine
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7150:
URL: https://github.com/apache/trafficserver/pull/7150#issuecomment-705762764
Cherry-picked to v9.0.x branch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich merged pull request #7150: Additions to enable loading qat_engine
Posted by GitBox <gi...@apache.org>.
shinrich merged pull request #7150:
URL: https://github.com/apache/trafficserver/pull/7150
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7150: Additions to enable loading qat_engine
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7150:
URL: https://github.com/apache/trafficserver/pull/7150#discussion_r479840181
##########
File path: iocore/net/SSLNetVConnection.cc
##########
@@ -1364,6 +1365,7 @@ SSLNetVConnection::sslServerHandShakeEvent(int &err)
#if TS_USE_TLS_ASYNC
case SSL_ERROR_WANT_ASYNC:
+ SSL_INCREMENT_DYN_STAT(ssl_error_async);
Review comment:
Adding this metric on another commit (PR) would make future debugging easy, because you'd lose the metric if you had to revert this entire change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org