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: Split up crypto.c into multiple files #2068

Merged
merged 37 commits into from
Jan 2, 2019

Conversation

hogand
Copy link
Contributor

@hogand hogand commented Dec 20, 2018

This is phase one of revamping crypto.c by splitting it up into many files. I tried to make as few changes as possible other than moving things around.

The order of the functions in each new C file is the same as in crypto.c so it's easier to verify that I didn't accidentally change anything. I used meld to double check each file against master crypto.c.

There should be no change in functionality with this PR. crypto_SUITE passes with OpenSSL 1.1.1a.

Also move some of the common functionality that's used in the NIF
implementations.
Also, move a FIPS check macro to the common openssl_config.h.
crypto.c is now only responsible for declaring NIFs and setup/tear down.
@CLAassistant
Copy link

CLAassistant commented Dec 20, 2018

CLA assistant check
All committers have signed the CLA.

@IngelaAndin IngelaAndin added the team:PS Assigned to OTP team PS label Dec 20, 2018
@HansN HansN self-assigned this Dec 20, 2018
@HansN HansN added the testing currently being tested, tag is used by OTP internal CI label Dec 20, 2018
While it uses chacha20, it doesn't use Erlang chacha20 functions.
Using the same copyright header as crypto.c
@HansN
Copy link
Contributor

HansN commented Dec 21, 2018

Thanks for the PR!
Looks good so far. I've run the crypto and ssh tests with openssl 0.9.8 and 1.0.1 without errors. Half of our nightly tests have now finnished and there was no crypto related error.
I have a comment on four lines in evp_compat.h which I will do in the code.

@HansN HansN requested a review from sverker December 21, 2018 13:48
Copy link
Contributor

@sverker sverker left a comment

Choose a reason for hiding this comment

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

Looks good over all.

To reap further benefit of this modularization, you could move the calls to enif_open_resource_type into *_initialize functions within the corresponding .c file. That would make the resource types and their destructor functions internal to each .c file without the need to be exposed in the .h files.

@hogand
Copy link
Contributor Author

hogand commented Dec 21, 2018

I noticed in the existing code that if FIPS mode is enabled but runs into a problem in initialize() (fails to load or not passed a boolean), it returns 0 which indicates success. It seems like that should return __LINE__ like the rest of the function to indicate failure.

In this PR, since init_atoms() has that FIPS init procedure, initialize() will return __LINE__ when FIPS init fails. That is a change in behavior compared to master, but it seems like a bug fix. I was keeping the same behavior as master until that last commit.

@HansN HansN merged commit d3f7bb2 into erlang:master Jan 2, 2019
@HansN
Copy link
Contributor

HansN commented Jan 2, 2019

Thank you very much for the PR. More are welcome!

@HansN HansN removed the testing currently being tested, tag is used by OTP internal CI label Jan 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:PS Assigned to OTP team PS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants