Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: use CHECK instead in getSSLCiphers #16453

Closed
wants to merge 1 commit into from

Conversation

@jasnell
Copy link
Member

@jasnell jasnell commented Oct 24, 2017

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), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

src/node_crypto.cc Outdated
SSL_CTX_free(ctx);
return env->ThrowError("SSL_new() failed.");
}
CHECK_NE(ssl, nullptr); // SS_new failed

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 24, 2017
Member

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)));

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 24, 2017
Member

This change seems unrelated.

This comment has been minimized.

@jasnell

jasnell Oct 24, 2017
Author Member

Unrelated, yes, but while I was in here I figured I'd clean up a bit.

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 25, 2017
Member

A commit should not contain unrelated cleanup. It should do what it says on the tin, no more and no less.

This comment has been minimized.

@jasnell

jasnell Oct 25, 2017
Author Member

Will update the commit message to include the additional change

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 26, 2017
Member

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.

@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 25, 2017

CI failures are unrelated.

@jasnell jasnell force-pushed the jasnell:crypto-getsslciphers-refactor branch Oct 25, 2017
@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 25, 2017

@jasnell jasnell added this to In Progress in Error Codes Oct 25, 2017
Copy link
Member

@mcollina mcollina left a comment

LGTM

The previous throws should never happen, and if they do, they
signal a larger issue in core. Make these checks rather than
throws.
@jasnell jasnell force-pushed the jasnell:crypto-getsslciphers-refactor branch to 1ab7c6b Oct 26, 2017
@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 26, 2017

@bnoordhuis ... updated to remove the errant change. PTAL

@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 26, 2017

jasnell added a commit that referenced this pull request Oct 26, 2017
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>
@jasnell
Copy link
Member Author

@jasnell jasnell commented Oct 26, 2017

Landed in df8c6c3

@jasnell jasnell closed this Oct 26, 2017
addaleax added a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
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>
@targos targos moved this from In Progress to Done in Error Codes Oct 29, 2017
addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Error Codes
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.