Use cfg_select in a bunch more places#7740
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR replaces many scattered ChangesType alias consolidation with cfg_select!
Conditional function/method logic refactoring
High-level runtime & VM conditionalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/stdlib/src/faulthandler.rs (1)
151-167:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix decimal buffer slice offset in
dump_decimal.At Line 167, the function writes digits into the tail of
buf, but emits a prefix slice. That can output leading\0bytes and malformed decimal text in fault dumps.Suggested fix
- puts_bytes(fd, &buf[..buf.len() - i]); + puts_bytes(fd, &buf[i..]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/stdlib/src/faulthandler.rs` around lines 151 - 167, The dump_decimal function writes digits right-aligned into buf and then slices the output incorrectly: replace the problematic puts_bytes(fd, &buf[..buf.len() - i]) with puts_bytes(fd, &buf[i..]) so that the emitted slice starts at index i (the first digit) and does not include leading zero bytes; update the call site in dump_decimal accordingly (referencing buf, i, and puts_bytes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/stdlib/src/openssl.rs`:
- Around line 1566-1569: The binding certs is only created under the ossl300 cfg
so code that references certs later will fail to compile on other targets;
update the cfg_select! expression that defines certs (the let certs =
cfg_select! { ... } using ctx.cert_store().all_certificates() and
ctx.cert_store().objects().iter().filter_map(|x| x.x509())) so it always
declares certs on all targets—either by providing a non-ossl300 arm that yields
an equivalent iterator/collection type (or an empty iterator) with matching item
type, or by moving the let certs = ... outside the conditional and having
cfg_select! return only the inner expression; keep references to
ctx.cert_store() and the existing iterator transformation
(objects().iter().filter_map(|x| x.x509())) when building the non-ossl300 arm.
- Around line 1310-1316: Change the cfg gate used where SSL_CTX_get_num_tickets
and SSL_CTX_set_num_tickets are referenced from ossl110 to ossl111: locate the
cfg_select! blocks that wrap the calls to sys::SSL_CTX_get_num_tickets and
sys::SSL_CTX_set_num_tickets (references around the methods using self.ctx() /
ctx.as_ptr()) and replace the ossl110 arm with ossl111 so the calls are only
enabled for OpenSSL 1.1.1 and later; keep the fallback arms (e.g., _ => Ok(0) or
identical behavior) unchanged.
---
Outside diff comments:
In `@crates/stdlib/src/faulthandler.rs`:
- Around line 151-167: The dump_decimal function writes digits right-aligned
into buf and then slices the output incorrectly: replace the problematic
puts_bytes(fd, &buf[..buf.len() - i]) with puts_bytes(fd, &buf[i..]) so that the
emitted slice starts at index i (the first digit) and does not include leading
zero bytes; update the call site in dump_decimal accordingly (referencing buf,
i, and puts_bytes).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0048983b-d7bc-4286-afd7-a13984451f9a
📒 Files selected for processing (23)
crates/common/src/rc.rscrates/common/src/refcount.rscrates/common/src/str.rscrates/host_env/src/crt_fd.rscrates/stdlib/src/faulthandler.rscrates/stdlib/src/openssl.rscrates/stdlib/src/socket.rscrates/stdlib/src/ssl.rscrates/vm/src/buffer.rscrates/vm/src/codecs.rscrates/vm/src/exceptions.rscrates/vm/src/getpath.rscrates/vm/src/stdlib/_ctypes/function.rscrates/vm/src/stdlib/_ctypes/simple.rscrates/vm/src/stdlib/_io.rscrates/vm/src/stdlib/_signal.rscrates/vm/src/stdlib/os.rscrates/vm/src/stdlib/time.rscrates/vm/src/version.rscrates/vm/src/vm/mod.rscrates/vm/src/vm/vm_object.rssrc/interpreter.rssrc/lib.rs
| cfg_select! { | ||
| ossl110 => { | ||
| let ctx = self.ctx(); | ||
| let num = unsafe { sys::SSL_CTX_get_num_tickets(ctx.as_ptr()) }; | ||
| Ok(num) | ||
| } | ||
| _ => Ok(0), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In OpenSSL, what minimum version introduced SSL_CTX_get_num_tickets and SSL_CTX_set_num_tickets?
💡 Result:
The minimum OpenSSL version that introduced both SSL_CTX_get_num_tickets and SSL_CTX_set_num_tickets is 1.1.1.
Citations:
- 1: https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_num_tickets.html
- 2: https://docs.openssl.org/3.1/man3/SSL_CTX_set_num_tickets/
- 3: https://docs.openssl.org/1.1.1/man3/SSL_CTX_set_num_tickets/
- 4: https://manpages.opensuse.org/Leap-16.0/openssl-3-doc/SSL_new_session_ticket.33ssl
- 5: https://docs.openssl.org/master/man3/SSL_CTX_set_num_tickets
- 6: https://man.archlinux.org/man/SSL_CTX_set_num_tickets.3.en
🏁 Script executed:
# Check the actual code at the specified lines in openssl.rs
cd crates/stdlib/src && head -1350 openssl.rs | tail -100 | head -50Repository: RustPython/RustPython
Length of output: 2008
🏁 Script executed:
# Find all usages of SSL_CTX_get_num_tickets and SSL_CTX_set_num_tickets
rg "SSL_CTX_(?:get|set)_num_tickets" crates/stdlib/src/Repository: RustPython/RustPython
Length of output: 312
🏁 Script executed:
# Check for cfg_select macro definitions and gate patterns
rg "cfg_select|ossl110|ossl111" crates/stdlib/src/openssl.rs | head -40Repository: RustPython/RustPython
Length of output: 1055
🏁 Script executed:
# Get exact lines around 1310-1316 and 1331-1342
sed -n '1305,1320p' crates/stdlib/src/openssl.rs
echo "---"
sed -n '1326,1345p' crates/stdlib/src/openssl.rsRepository: RustPython/RustPython
Length of output: 1347
🏁 Script executed:
# Verify that ossl111 is used elsewhere for similar version-gated APIs to confirm the pattern
rg -A 3 'cfg_select.*ossl111 =>' crates/stdlib/src/openssl.rs | head -30Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Search for how ossl111 gate is used in cfg_select blocks
rg 'cfg_select!' crates/stdlib/src/openssl.rs -A 2 | grep -A 2 'ossl111'Repository: RustPython/RustPython
Length of output: 154
Change version gates from ossl110 to ossl111 for SSL_CTX_get_num_tickets and SSL_CTX_set_num_tickets
SSL_CTX_get_num_tickets and SSL_CTX_set_num_tickets were introduced in OpenSSL 1.1.1, not 1.1.0. Using the ossl110 gate will cause build or link failures on OpenSSL 1.1.0 targets. Both occurrences must be updated to use ossl111.
Required fix
cfg_select! {
- ossl110 => {
+ ossl111 => {
let ctx = self.ctx();
let num = unsafe { sys::SSL_CTX_get_num_tickets(ctx.as_ptr()) };
Ok(num)
}
_ => Ok(0),
} cfg_select! {
- ossl110 => {
+ ossl111 => {
let ctx = self.builder();
let result = unsafe { sys::SSL_CTX_set_num_tickets(ctx.as_ptr(), value as usize) };
if result != 1 {
return Err(vm.new_value_error("failed to set num tickets."));
}
Ok(())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cfg_select! { | |
| ossl110 => { | |
| let ctx = self.ctx(); | |
| let num = unsafe { sys::SSL_CTX_get_num_tickets(ctx.as_ptr()) }; | |
| Ok(num) | |
| } | |
| _ => Ok(0), | |
| cfg_select! { | |
| ossl111 => { | |
| let ctx = self.ctx(); | |
| let num = unsafe { sys::SSL_CTX_get_num_tickets(ctx.as_ptr()) }; | |
| Ok(num) | |
| } | |
| _ => Ok(0), | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/stdlib/src/openssl.rs` around lines 1310 - 1316, Change the cfg gate
used where SSL_CTX_get_num_tickets and SSL_CTX_set_num_tickets are referenced
from ossl110 to ossl111: locate the cfg_select! blocks that wrap the calls to
sys::SSL_CTX_get_num_tickets and sys::SSL_CTX_set_num_tickets (references around
the methods using self.ctx() / ctx.as_ptr()) and replace the ossl110 arm with
ossl111 so the calls are only enabled for OpenSSL 1.1.1 and later; keep the
fallback arms (e.g., _ => Ok(0) or identical behavior) unchanged.
ShaharNaveh
left a comment
There was a problem hiding this comment.
I really like this change
A sort of follow-up to #7636. I tried to not overdo it here, and I think most of the places I chose are genuinely easier to understand than before.
Summary by CodeRabbit