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.

Sign up for GitHub

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

Jump to bottom

crypto: add internal error codes #37650

Merged
merged 1 commit into from
Mar 13, 2021
Merged

crypto: add internal error codes #37650

merged 1 commit into from
Mar 13, 2021

Conversation

RaisinTen
Copy link
Contributor

According to addaleax's comment here: #37555 (review)

Maybe we could also expand CryptoErrorVector in the future to forward error codes, rather than just plain strings, so that the errors become more programatically accessible?

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 7, 2021
RaisinTen
RaisinTen commented Mar 7, 2021
View reviewed changes
src/crypto/crypto_util.cc Outdated Show resolved Hide resolved
@RaisinTen
Copy link
Contributor Author

cc @addaleax @jasnell

addaleax
addaleax approved these changes Mar 8, 2021
View reviewed changes
src/crypto/crypto_util.cc Outdated Show resolved Hide resolved
src/crypto/crypto_util.cc Outdated Show resolved Hide resolved
@RaisinTen RaisinTen marked this pull request as ready for review March 9, 2021 14:22
@RaisinTen RaisinTen force-pushed the crypto/add-internal-error-codes branch 2 times, most recently from 8bdf37e to 703bbf0 Compare March 9, 2021 14:31
addaleax
addaleax approved these changes Mar 9, 2021
View reviewed changes
src/crypto/crypto_util.cc Outdated
}

template<typename... Args>
void CryptoErrorStore::Insert(const NodeCryptoError error, Args&&... args) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might need to go into a crypto_util-inl.h file at some point (because template functions are implicitly also inline functions), but if this works right now, it should be fine 👍

Copy link
Contributor Author

@RaisinTen RaisinTen Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax I don't think we have a crypto_util-inl.h.
I did find -name crypto_util-inl.h and couldn't find it. 👀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sorry for being unclear – this would be the first function in that file :)

Copy link
Contributor Author

@RaisinTen RaisinTen Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax Moved it to src/crypto/crypto_util-inl.h. I noticed a couple of other template functions and classes in src/crypto/crypto_util.h. Should we move those to src/crypto/crypto_util-inl.h as well (in another PR)?

Also,

../src/crypto/crypto_util.h:351:14: error: no matching member function for call to 'TrackField'
    tracker->TrackField("errors", errors_);
    ~~~~~~~~~^~~~~~~~~~

To fix this, should I again change CryptoErrorStore to inherit from std::vector<std::string> or is there another way?

@RaisinTen RaisinTen added the wip Issues and PRs that are still a work in progress. label Mar 10, 2021
@RaisinTen RaisinTen force-pushed the crypto/add-internal-error-codes branch 2 times, most recently from 5231d3d to 4ff9e88 Compare March 10, 2021 13:09
@nodejs-github-bot
Copy link
Collaborator

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

@RaisinTen RaisinTen removed the wip Issues and PRs that are still a work in progress. label Mar 11, 2021
@RaisinTen
Copy link
Contributor Author

cc @addaleax @jasnell
Could this have another review please?
I had to change NO_ERROR to OK because msvc seems to have a constant going by the same name.

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 11, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 12, 2021 •
edited by RaisinTen
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/36654/ ❤️

@RaisinTen @aduh95
crypto: add internal error codes
a28cb93 
PR-URL: nodejs#37650
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@aduh95 aduh95 force-pushed the crypto/add-internal-error-codes branch from 6ff54c7 to a28cb93 Compare March 13, 2021 08:54
@aduh95
Copy link
Contributor

aduh95 commented Mar 13, 2021

Landed in a28cb93

@aduh95 aduh95 merged commit a28cb93 into nodejs:master Mar 13, 2021
@RaisinTen RaisinTen deleted the crypto/add-internal-error-codes branch March 13, 2021 11:53
danielleadams pushed a commit that referenced this pull request Mar 16, 2021
@RaisinTen @danielleadams
crypto: add internal error codes
6ef1d33 
PR-URL: #37650
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@danielleadams danielleadams mentioned this pull request Mar 16, 2021
Merged
danielleadams pushed a commit that referenced this pull request Mar 16, 2021
@RaisinTen @danielleadams
crypto: add internal error codes
a2d08d5 
PR-URL: #37650
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@RaisinTen @nodejs-github-bot @aduh95 @jasnell @addaleax @targos