Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.63%. Comparing base (bcc295d) to head (f3c7813).
Report is 1 commits behind head on main.

@@           Coverage Diff           @@
##             main    #2089   +/-   ##
=======================================
  Coverage   94.63%   94.63%           
=======================================
  Files         102      102           
  Lines       23400    23408    +8     
=======================================
+ Hits        22145    22153    +8     
  Misses       1255     1255           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Benchmark results

Instruction counts

Significant differences

There are no significant instruction count differences

Other differences

Wall-time

Significant differences

There are no significant wall-time differences

Other differences

Additional information

Historical results

Checkout details:

• Base repo: https://github.com/rustls/rustls.git
• Base branch: main (bcc295d)
• Candidate repo: https://github.com/brodycj/rustls.git
• Candidate branch: default-crypto-provider-improvements (f3c7813)

Having not reviewed #2088 yet I think I'd appreciate some elaboration in the accompanying text to help this change set stand alone.

At the moment the commit message only says "default crypto provider improvements" and doesn't get into any motivation or enumerate any improvements. Similarly the PR description says it makes it easier to configure the provider in 2088, but doesn't expand on how.

The actual mechanics of the diff seem reasonable (once djc's feedback is addressed).

With the one clarification that I requested. all suggestions sounds good. I will try to get this proposal fixed within the next 1-2 days. Thanks!

I have now updated the description, rebased, and included some more info in the commit message to include the details of the what & why (with # removed from the issue & PR number references to avoid extra GitHub spamming) ... keeping this PR in draft status for now thanks!

@djc thanks for taking the time to review this!

I may need a few days to go through your comments.... I would like to quote myself from what I wrote in both the updated PR description & in the updated commit message in terms of general motivation - please let me know if my motivation or any part of my motivation does not sound clear enough:

This refactoring & cleanup would help make it easier to update & maintain the code that manages the default crypto provider.

The code needed to store the default crypto provider already depends on the build environment, requiring #[cfg statements sprinkled through some of the existing import statements, rustls::crypto::CryptoProvider API functions, and configuration-specific static variables in rustls/src/crypto/mod.rs.

This issue with the various bits of conditional code is expected to compound as more build options are added over time, for example:

• replace alloc::arc::Arc with alloc::rc::Rc, with help from some type aliasing, to help support targets with non-atomic pointers
• option to use once_cell with critical-section crate option, as needed to support non-atomic pointers - and it should be possible to turn this option off in case there may be a more performant solution for certain build targets

Moving the default crypto provider functionality into an internal module would simplify the ability to keep or remove this functionality depending on the build target.

I think this refactoring should offer some further benefits, regardless of these anticipated updated:

• improve readability of the default crypto provider code in general
• improve rustls::crypto::CryptoProvider::install_default API consistency for std vs non-std build config in general

@djc thanks for taking the time to review this!

I may need a few days to go through your comments....

@brodycj Checking in on this PR: will you have time to address Djc's feedback?

I have now addressed @djc most recent comments & rebased into a clean commit, with an updated commit message that contains the updated justification. Heads up that I will likely unplug for the rest of this week due to a religious holiday. Thanks!

rebased again with a few more updates & fixes:

• updates from main
• use map_err() to simplify internal fn for no-std (with minor reformatting)
• remove internal type alias for PROCESS_DEFAULT_PROVIDER (I still like it, but definitely less concise with the type alias)

Thanks!