GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — src: add node_crypto_common and refactor by jasnell · Pull Request #32016 · nodejs/node

Conversation

jasnell commented Feb 29, 2020

Copy link

Copy Markdown

Member

Two things in one on this commit:

(a) For the QUIC implementation, we need to separate out various bits
from node_crypto.cc to allow them to be reused. That's where this
commit starts.

(b) Quite a bit of the node_crypto.cc code was just messy in terms of
it's organization and lack of error handling and use of Local vs.
MaybeLocal. This cleans that up a bit and hopefully makes certain
parts a bit more manageable also.

There is nothing QUIC specific in this PR but there are pieces in here that are only used by the QUIC implementation currently. I am separating things out to make them easier to review in smaller chunks.

This PR does include a number of changes that are not yet in the QUIC version as I decided once I started separating it out to do some additional cleanup while I was in there.

These should all be internal changes with no changes to public API at all.

Signed-off-by: James M Snell [email protected]

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji

All reactions

👍 1 reaction

jasnell requested a review from addaleax

February 29, 2020 01:19

nodejs-github-bot added c++

Issues and PRs that require attention from people who are familiar with C++.

lib / src Issues and PRs involving general changes in the lib/ or src/ directories. labels

Feb 29, 2020

addaleax approved these changes

Feb 29, 2020

View reviewed changes

Comment thread

src/node_crypto_common.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/node_crypto_common.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/node_crypto_common.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/node_crypto_common.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/node_crypto_common.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

jasnell added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

Mar 2, 2020

nodejs-github-bot commented Mar 2, 2020

Copy link

Copy Markdown

Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29497/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Mar 2, 2020

Copy link

Copy Markdown

Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29498/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sam-github approved these changes

Mar 2, 2020

View reviewed changes

Comment thread

src/node_crypto.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/node_crypto.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Mar 2, 2020

Copy link

Copy Markdown

Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29499/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sam-github reviewed

Mar 2, 2020

View reviewed changes

Comment thread

src/node_crypto.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

sam-github reviewed

Mar 2, 2020

View reviewed changes

Comment thread

src/node_crypto.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.


          src: add node_crypto_common and refactor

e1b0c3d

Two things in one on this commit:

(a) For the QUIC implementation, we need to separate out various bits
    from node_crypto.cc to allow them to be reused. That's where this
    commit starts.

(b) Quite a bit of the node_crypto.cc code was just messy in terms of
    it's organization and lack of error handling and use of Local vs.
    MaybeLocal. This cleans that up a bit and hopefully makes certain
    parts a bit more manageable also.

Signed-off-by: James M Snell <[email protected]>

jasnell force-pushed the crypto-common-refactor branch from af1824a to e1b0c3d Compare

March 2, 2020 22:09

nodejs-github-bot commented Mar 2, 2020

Copy link

Copy Markdown

Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29504/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Mar 2, 2020

Copy link

Copy Markdown

Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29507/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell added a commit that referenced this pull request

Mar 3, 2020


          src: add node_crypto_common and refactor

dd81836

Two things in one on this commit:

(a) For the QUIC implementation, we need to separate out various bits
    from node_crypto.cc to allow them to be reused. That's where this
    commit starts.

(b) Quite a bit of the node_crypto.cc code was just messy in terms of
    it's organization and lack of error handling and use of Local vs.
    MaybeLocal. This cleans that up a bit and hopefully makes certain
    parts a bit more manageable also.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #32016
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>

jasnell commented Mar 3, 2020

Copy link

Copy Markdown

Member Author

Landed in dd81836

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell closed this

Mar 3, 2020

MylesBorins pushed a commit that referenced this pull request

Mar 4, 2020


          src: add node_crypto_common and refactor

755da03

Two things in one on this commit:

(a) For the QUIC implementation, we need to separate out various bits
    from node_crypto.cc to allow them to be reused. That's where this
    commit starts.

(b) Quite a bit of the node_crypto.cc code was just messy in terms of
    it's organization and lack of error handling and use of Local vs.
    MaybeLocal. This cleans that up a bit and hopefully makes certain
    parts a bit more manageable also.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #32016
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>

MylesBorins mentioned this pull request

Mar 4, 2020

v13.10.1 proposal #32099

Merged

codebytere pushed a commit that referenced this pull request

Mar 16, 2020


          src: add node_crypto_common and refactor

aa54245

Two things in one on this commit:

(a) For the QUIC implementation, we need to separate out various bits
    from node_crypto.cc to allow them to be reused. That's where this
    commit starts.

(b) Quite a bit of the node_crypto.cc code was just messy in terms of
    it's organization and lack of error handling and use of Local vs.
    MaybeLocal. This cleans that up a bit and hopefully makes certain
    parts a bit more manageable also.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #32016
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>

codebytere pushed a commit that referenced this pull request

Mar 17, 2020


          src: add node_crypto_common and refactor

e8fdb40

Two things in one on this commit:

(a) For the QUIC implementation, we need to separate out various bits
    from node_crypto.cc to allow them to be reused. That's where this
    commit starts.

(b) Quite a bit of the node_crypto.cc code was just messy in terms of
    it's organization and lack of error handling and use of Local vs.
    MaybeLocal. This cleans that up a bit and hopefully makes certain
    parts a bit more manageable also.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #32016
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>

codebytere mentioned this pull request

Mar 17, 2020

v12.16.2 proposal #32313

Merged

codebytere pushed a commit that referenced this pull request

Mar 30, 2020


          src: add node_crypto_common and refactor

a6a41f4

Two things in one on this commit:

(a) For the QUIC implementation, we need to separate out various bits
    from node_crypto.cc to allow them to be reused. That's where this
    commit starts.

(b) Quite a bit of the node_crypto.cc code was just messy in terms of
    it's organization and lack of error handling and use of Local vs.
    MaybeLocal. This cleans that up a bit and hopefully makes certain
    parts a bit more manageable also.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #32016
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>

Reviewers

addaleax

addaleax approved these changes

+1 more reviewer

sam-github

sam-github approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

c++

Issues and PRs that require attention from people who are familiar with C++.

lib / src

Issues and PRs involving general changes in the lib/ or src/ directories.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.