Skip to content

crypto: simplify exportKeyingMaterial - #31922

Closed
tniessen wants to merge 2 commits into
nodejs:masterfrom
tniessen:crypto-simplify-exportkeyingmaterial
Closed

crypto: simplify exportKeyingMaterial#31922
tniessen wants to merge 2 commits into
nodejs:masterfrom
tniessen:crypto-simplify-exportkeyingmaterial

Conversation

@tniessen

@tniessentniessen commented Feb 23, 2020

Copy link
Copy Markdown
Member
  1. Use bool instead of int.
  2. Don't call Buffer::HasInstance. We shouldn't silently ignore non-buffers, and ByteSource::FromBuffer will CHECK that the input is a Buffer anyway.
  3. Rename key to context, because that's what it is.
  4. Rename useContext to use_context for consistency with the rest of node_crypto.cc.
  5. Use IsUndefined instead of IsNull, because the JavaScript layer will always either pass undefined or a Buffer.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@tniessentniessen added crypto Issues and PRs related to the crypto subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Feb 23, 2020
addaleax
addaleax approved these changes Feb 24, 2020
View reviewed changes
Comment threadsrc/node_crypto.cc Outdated
@tniessen@addaleax
Update src/node_crypto.cc
8a70768
Co-Authored-By: Anna Henningsen <github@addaleax.net>
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Feb 29, 2020

Copy link
Copy Markdown
Collaborator

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

@Trott

Trott commented Mar 1, 2020

Copy link
Copy Markdown
Member

Landed in 51cae73

Trott pushed a commit that referenced this pull request Mar 1, 2020
@tniessen@Trott
crypto: simplify exportKeyingMaterial
51cae73
PR-URL: #31922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@TrottTrott closed this Mar 1, 2020
MylesBorins pushed a commit that referenced this pull request Mar 4, 2020
@tniessen@MylesBorins
crypto: simplify exportKeyingMaterial
0fc45f8
PR-URL: #31922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Mar 4, 2020
Merged
@targos

targos commented Apr 20, 2020

Copy link
Copy Markdown
Member

depends on #31814 to land on v12.x

targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@tniessen@targos
crypto: simplify exportKeyingMaterial
cfaa156
PR-URL: nodejs#31922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
@tniessen@targos
crypto: simplify exportKeyingMaterial
0847bc3
PR-URL: #31922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@targostargos mentioned this pull request May 2, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@targostargostargos left review comments
@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@cjihrigcjihrigcjihrig approved these changes
+1 more reviewer
@devnexendevnexendevnexen approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.cryptoIssues and PRs related to the crypto subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@tniessen@nodejs-github-bot@Trott@targos@jasnell@addaleax@cjihrig@devnexen