crypto: use CHECK instead in getSSLCiphers #16453
Conversation
jasnell
added
crypto
nodejs-github-bot
added
c++
src/node_crypto.cc
Outdated
| SSL_CTX_free(ctx); | ||
| return env->ThrowError("SSL_new() failed."); | ||
| } | ||
| CHECK_NE(ssl, nullptr); // SS_new failed |
There was a problem hiding this comment.
SSL_new - although the comments are a bit superfluous, IMO.
src/node_crypto.cc
Outdated
|
|
||
| Local<Array> arr = Array::New(env->isolate()); | ||
| STACK_OF(SSL_CIPHER)* ciphers = SSL_get_ciphers(ssl); | ||
|
|
||
| for (int i = 0; i < sk_SSL_CIPHER_num(ciphers); ++i) { | ||
| const SSL_CIPHER* cipher = sk_SSL_CIPHER_value(ciphers, i); | ||
| arr->Set(i, OneByteString(args.GetIsolate(), SSL_CIPHER_get_name(cipher))); | ||
| arr->Set(i, OneByteString(isolate, SSL_CIPHER_get_name(cipher))); |
There was a problem hiding this comment.
This change seems unrelated.
There was a problem hiding this comment.
Unrelated, yes, but while I was in here I figured I'd clean up a bit.
There was a problem hiding this comment.
A commit should not contain unrelated cleanup. It should do what it says on the tin, no more and no less.
There was a problem hiding this comment.
Will update the commit message to include the additional change
There was a problem hiding this comment.
Ah, that's what I get for using pithy one-liners. What I mean is, cleanup is fine but do it in a separate commit. Future code archeologists will thank you for it.
Having said that, this specific cleanup doesn't buy much. It's neither shorter nor faster.
|
CI failures are unrelated. |
jasnell
force-pushed
the
crypto-getsslciphers-refactor
branch
from
a961d46
to
dba4c22
Compare
|
Ping @nodejs/tsc |
The previous throws should never happen, and if they do, they signal a larger issue in core. Make these checks rather than throws.
jasnell
force-pushed
the
crypto-getsslciphers-refactor
branch
from
42fc0d7
to
1ab7c6b
Compare
|
@bnoordhuis ... updated to remove the errant change. PTAL |
|
New CI, just to be safe: https://ci.nodejs.org/job/node-test-pull-request/11003/ |
The previous throws should never happen, and if they do, they signal a larger issue in core. Make these checks rather than throws. PR-URL: #16453 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
|
Landed in df8c6c3 |
The previous throws should never happen, and if they do, they signal a larger issue in core. Make these checks rather than throws. PR-URL: nodejs/node#16453 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
The previous throws should never happen, and if they do, they signal a larger issue in core. Make these checks rather than throws. PR-URL: nodejs/node#16453 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
The previous throws should never happen, and if they do, they signal a larger issue in core. Make these checks rather than throws
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
crypto