refactor: move "s2n_libcrypto_is" methods into s2n_libcrypto.h #5117
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lrstewart
force-pushed
the
libcrypto_cleanup
branch
from
597b1e1 to
95e5a0a
Compare
lrstewart
force-pushed
the
libcrypto_cleanup
branch
from
95e5a0a to
b80c2a9
Compare
maddeleine
approved these changes
Feb 17, 2025
| @@ -15,8 +15,6 @@ | |||
|
|
|||
| #include "s2n_pq.h" | |||
|
|
|||
| #include "crypto/s2n_openssl.h" | |||
There was a problem hiding this comment.
I manually checked that any macro defined/modified in s2n_openssl.h isn't being #ifdef'ed or #if defined() anywhere in the codebase. Therefore, it is safe to remove #include s2n_openssl.h in these locations.
CarolYeh910
approved these changes
Feb 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Release Summary:
Description of changes:
This change should not change any code behavior.
It bothers me that we have a bunch of methods with the prefix "s2n_libcrypto", implemented in "s2n_libcrypto.c", but declared in "s2n_openssl.h". This PR moves them to s2n_libcrypto.h, where you'd expect to find them. It also renames the s2n_openssl_test to the s2n_libcrypto_test, because it was only testing the "s2n_libcrypto" methods.
As part of that move, I also tried to update our s2n_openssl.h includes. Mostly if s2n_openssl.h isn't included somewhere, you'll get an error because a symbol it declares is missing. EXCEPT for OPENSSL_VERSION_NUMBER: it redefines OPENSSL_VERSION_NUMBER for libressl
s2n-tls/crypto/s2n_openssl.h
Lines 26 to 33 in 8201205
There's probably a less error prone way to handle that, but I'm just here about the names :)
So to cleanup the includes I:
I also added s2n_libcrypto.h as an include to files that failed to compile due to missing "s2n_libcrypto_is" symbols.
We definitely need a more comprehensive include cleanup, but I just didn't want to make the situation worse with this move.
Testing:
Existing tests pass. This should just be moving files around.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.