new_last_{os,errno}_error #6381
Conversation
WalkthroughThis PR systematically refactors error handling across the codebase by introducing new errno/OS error construction helpers ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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/multiprocessing.rs (1)
10-17: Critical bug: inverted error condition forclosesocket.The
closesocketWindows API returns 0 on success andSOCKET_ERROR(-1) on failure. The current logic returns an error whenres == 0, which is the success case.Apply this diff to fix the logic:
fn closesocket(socket: usize, vm: &VirtualMachine) -> PyResult<()> { let res = unsafe { WinSock::closesocket(socket as SOCKET) }; - if res == 0 { + if res != 0 { Err(vm.new_last_os_error()) } else { Ok(()) } }
🧹 Nitpick comments (3)
crates/stdlib/src/socket.rs (1)
2484-2497: Verifylast_os_error()correctness afterclosesocketon Windows
close_innernow usesstd::io::Error::last_os_error()whenclose(x)fails (ignoringECONNRESET). On Unix this is the right way to surfaceerrnofromclose(2). On Windows this maps toclosesocket, which reports errors via WinSock (WSAGetLastError). Please double‑check that in your target toolchainstd::io::Error::last_os_error()observes the same error code that socket operations set; if not, it may be safer to reuse the existing socket error plumbing (e.g. viaerrno_io_error()orErrorExt) here as well.crates/vm/src/stdlib/posix.rs (1)
2339-2345:sysconfstill treats all-1returns as errors—consider checkingerrnoWhile you’re adjusting
sysconf’s error constructor, note that:let r = unsafe { libc::sysconf(name.0) }; if r == -1 { return Err(vm.new_last_errno_error()); }will raise for any
-1, even whensysconfsucceeds but returns-1witherrno == 0(which POSIX/CPython interpret as “no limit”). A more faithful implementation would be:use nix::errno::Errno; Errno::clear(); let r = unsafe { libc::sysconf(name.0) }; if r == -1 && Errno::last_raw() != 0 { return Err(vm.new_last_errno_error()); } Ok(r)This is a pre-existing behavior, but this refactor is a good opportunity to align with POSIX semantics.
crates/common/src/os.rs (1)
36-49: Consider usinglibcfor more direct errno access on non-Windows.The non-Windows
get_errno()implementation creates anio::Errorjust to extract the errno value, which is slightly indirect. A more direct approach would use libc's errno access.#[cfg(not(windows))] pub fn get_errno() -> i32 { - std::io::Error::last_os_error().posix_errno() + unsafe { *libc::__errno_location() } }However, the current implementation is portable and correct, so this is a minor optimization that could be deferred.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Lib/test/test_os.pyis excluded by!Lib/**Lib/test/test_ssl.pyis excluded by!Lib/**Lib/test/test_support.pyis excluded by!Lib/**
📒 Files selected for processing (18)
crates/common/src/crt_fd.rs(2 hunks)crates/common/src/fileutils.rs(1 hunks)crates/common/src/os.rs(2 hunks)crates/stdlib/src/fcntl.rs(7 hunks)crates/stdlib/src/multiprocessing.rs(3 hunks)crates/stdlib/src/overlapped.rs(7 hunks)crates/stdlib/src/resource.rs(1 hunks)crates/stdlib/src/socket.rs(5 hunks)crates/vm/src/stdlib/io.rs(1 hunks)crates/vm/src/stdlib/msvcrt.rs(2 hunks)crates/vm/src/stdlib/nt.rs(24 hunks)crates/vm/src/stdlib/os.rs(7 hunks)crates/vm/src/stdlib/posix.rs(16 hunks)crates/vm/src/stdlib/signal.rs(1 hunks)crates/vm/src/stdlib/time.rs(2 hunks)crates/vm/src/stdlib/winapi.rs(6 hunks)crates/vm/src/vm/vm_new.rs(2 hunks)crates/vm/src/windows.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/stdlib/src/resource.rscrates/vm/src/stdlib/msvcrt.rscrates/vm/src/windows.rscrates/common/src/fileutils.rscrates/stdlib/src/fcntl.rscrates/vm/src/stdlib/nt.rscrates/vm/src/stdlib/posix.rscrates/stdlib/src/multiprocessing.rscrates/stdlib/src/overlapped.rscrates/vm/src/stdlib/winapi.rscrates/stdlib/src/socket.rscrates/vm/src/stdlib/time.rscrates/common/src/os.rscrates/vm/src/stdlib/signal.rscrates/vm/src/stdlib/os.rscrates/common/src/crt_fd.rscrates/vm/src/stdlib/io.rscrates/vm/src/vm/vm_new.rs
🧠 Learnings (5)
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/stdlib/src/fcntl.rscrates/vm/src/stdlib/nt.rscrates/vm/src/stdlib/os.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Applied to files:
crates/stdlib/src/fcntl.rscrates/vm/src/stdlib/os.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
Applied to files:
crates/stdlib/src/fcntl.rscrates/vm/src/stdlib/os.rs
📚 Learning: 2025-12-09T08:46:58.660Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6358
File: crates/vm/src/exception_group.rs:173-185
Timestamp: 2025-12-09T08:46:58.660Z
Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.
Applied to files:
crates/vm/src/stdlib/os.rscrates/vm/src/vm/vm_new.rs
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files in the `Lib/` directory; modifications should be minimal and only to work around RustPython limitations
Applied to files:
crates/vm/src/stdlib/os.rs
🧬 Code graph analysis (7)
crates/common/src/fileutils.rs (1)
crates/common/src/os.rs (2)
errno_io_error(25-29)errno_io_error(32-34)
crates/vm/src/stdlib/posix.rs (1)
crates/vm/src/stdlib/os.rs (1)
fs_metadata(12-21)
crates/stdlib/src/socket.rs (1)
crates/common/src/os.rs (2)
errno_io_error(25-29)errno_io_error(32-34)
crates/vm/src/stdlib/os.rs (1)
crates/common/src/os.rs (2)
errno_io_error(25-29)errno_io_error(32-34)
crates/common/src/crt_fd.rs (1)
crates/common/src/os.rs (2)
errno_io_error(25-29)errno_io_error(32-34)
crates/vm/src/stdlib/io.rs (1)
crates/stdlib/src/fcntl.rs (1)
fcntl(59-90)
crates/vm/src/vm/vm_new.rs (1)
crates/common/src/os.rs (2)
errno_io_error(25-29)errno_io_error(32-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run snippets and cpython tests (macos-latest)
🔇 Additional comments (32)
crates/stdlib/src/resource.rs (1)
163-163: LGTM!Using
vm.new_last_errno_error()is correct here sincelibc::getrlimitis a POSIX function that sets errno on failure.crates/common/src/fileutils.rs (1)
10-21: LGTM!Using
errno_io_error()forlibc::fstatfailure is appropriate since it's a libc function that sets errno. This aligns with the PR's centralized errno-based error handling approach.crates/vm/src/stdlib/signal.rs (1)
293-303: LGTM!Using
vm.new_last_errno_error()is correct here sincesiginterruptis a POSIX function that sets errno on failure.crates/common/src/crt_fd.rs (2)
35-43: LGTM!Using
errno_io_error()is correct for CRT functions that set errno. The added comment clearly explains the distinction between CRT errno and Windows GetLastError().
342-354: LGTM!The comment correctly documents that
_get_osfhandleis a CRT function that sets errno rather than GetLastError(), makingerrno_io_error()the appropriate error constructor.crates/stdlib/src/multiprocessing.rs (2)
19-29: LGTM!Using
vm.new_last_os_error()is correct for Windows socket functions since they set WSAGetLastError() (which is retrieved via GetLastError()).
31-41: LGTM!Using
vm.new_last_os_error()is correct for thesendWindows socket function.crates/vm/src/stdlib/winapi.rs (6)
80-95: LGTM!Using
vm.new_last_os_error()is correct forGetStdHandlesince it's a Windows API that sets GetLastError() on failure.
157-168: LGTM!Using
vm.new_last_os_error()is correct forGetFileTypeWindows API.
292-310: LGTM!Using
vm.new_last_os_error()is correct forOpenProcessWindows API.
416-421: LGTM!Using
vm.new_last_os_error()is correct for theInitializeProcThreadAttributeListfailure path.
455-463: LGTM!Using
vm.new_last_os_error()is correct forWaitForSingleObjectWindows API.
516-535: LGTM!Using
vm.new_last_os_error()is correct forOpenMutexWWindows API.crates/vm/src/stdlib/io.rs (1)
3991-3995: LGTM!Using
vm.new_last_errno_error()is correct for thefcntlsyscall failure since it's a POSIX function that sets errno. This is consistent with the similar pattern used incrates/stdlib/src/fcntl.rsas shown in the relevant code snippets.crates/vm/src/stdlib/msvcrt.rs (1)
81-88: setmode/open_osfhandle now correctly usevm.new_last_errno_error()Using
vm.new_last_errno_error()when_setmode/open_osfhandlereturn-1matches their errno-based failure semantics and centralizes POSIX-style error construction in the VM. No issues spotted.Also applies to: 91-98
crates/stdlib/src/fcntl.rs (1)
11-12: POSIX error paths correctly migrated tovm.new_last_errno_error()All updated branches (
fcntl,ioctl,flock,lockf) now convertret < 0intovm.new_last_errno_error(), which is the right choice for errno-based libc calls and keeps behavior consistent across the module. No functional regressions evident.Also applies to: 58-90, 92-143, 145-215
crates/vm/src/windows.rs (1)
44-55:WindowsSysResult::into_pyresultnow correctly reports last OS errorThe condition inversion to
if !self.is_err()is semantically equivalent, and switching the failure case tovm.new_last_os_error()is appropriate for Win32-style return values (HANDLE/BOOL). This keeps the wrapper aligned with Windows APIs.crates/vm/src/stdlib/time.rs (1)
816-832: Windows time errors now correctly mapped viavm.new_last_os_error()Using
vm.new_last_os_error()forQueryPerformanceFrequencyandGetSystemTimeAdjustmentfailures matches their Win32 “0 == error + GetLastError” contract and aligns this module with the new VM error API.Also applies to: 857-873
crates/stdlib/src/overlapped.rs (1)
240-261: Overlapped I/O now consistently usesvm.new_last_os_error()For all the affected Win32 calls (cancel, events, IOCP, GQCS), the new
vm.new_last_os_error()branches correctly trigger on0/NULL orINVALID_HANDLE_VALUEand surface the last OS error via the VM. Resource handling (closing handles, marking completion) is unchanged.Also applies to: 267-297, 303-323, 325-359, 361-411
crates/stdlib/src/socket.rs (2)
1534-1575: Socket option errors now flow througherrno_io_errorandIoOrPyExceptionRouting
getsockopt/setsockoptfailures throughcrate::common::os::errno_io_error().into()is consistent with the rest of this module: you get a platform-appropriateio::Errorthat is then converted toIoOrPyExceptionand ultimately to a PythonOSError. Control flow and range checks are unchanged.Also applies to: 1578-1616
2155-2166: The premise aboutif_indextonameerror handling on Windows requires verification against the actual C binding implementationWhile the review comment's general principle about distinguishing errno vs. GetLastError on Windows is sound, the specific application to
if_indextonamemay be questionable. According to Microsoft documentation,if_indextonameon Windows does not setGetLastErrorwhen it fails—it simply returns NULL without an error code. This suggests that neithervm.new_last_errno_error()norvm.new_last_os_error()would capture meaningful error information on Windows for this particular function. Verify whether the C binding actually provides a Windows implementation and, if so, what error reporting mechanism it uses. Ifif_indextonameis unavailable on Windows or uses a different error model, the current uniform error handling may need a different approach than the one suggested.crates/vm/src/stdlib/nt.rs (2)
15-23: CRT/errno-based paths correctly switched tovm.new_last_errno_error()For MSVC/CRT calls that signal failure via
errno(_cwait,_wspawnv/_wspawnve,_wexecv/_wexecve,_umask,libc::dup,libc::dup2), the newvm.new_last_errno_error()branches are the right abstraction and keep behavior localized in the VM. The added imports (crt_fd,ToWideString,DirFd, etc.) match actual usage in this file.Also applies to: 160-179, 278-318, 320-395, 870-878, 987-1015
182-208: Win32 API error handling now consistently uses last-OS-error helpersFor the Win32 calls (
GenerateConsoleCtrlEvent,OpenProcess/TerminateProcess,GetConsoleScreenBufferInfoand its CONOUT$ fallback,GetFullPathNameW,GetVolumePathNameW,GetDiskFreeSpaceExW,GetHandleInformation,GetLogicalDriveStringsW,FindFirstVolumeW,CreateDirectoryW,CreatePipe), the updated code correctly:
- Detects failure via
0/NULL/INVALID_HANDLE_VALUE.- Constructs Python exceptions via
vm.new_last_os_error()(orio::Error::last_os_error()in the pure-Rust helpers likeraw_set_handle_inheritable), which is exactly what the new VM API is for.No behavioral issues spotted; resource cleanup logic is preserved.
Also applies to: 210-259, 517-547, 549-635, 637-644, 674-688, 691-708, 880-914
crates/vm/src/stdlib/posix.rs (1)
445-474: POSIX errno-based wrappers now consistently usevm.new_last_errno_error()/vm.new_last_os_error()Across the updated POSIX helpers:
- Redox
symlinkandmknodbranches now detect< 0/!= 0and surface errors viavm.new_last_errno_error().nice,sched_get_priority_{max,min},sched_getscheduler,sched_setscheduler,sched_getparam,sched_setparam,kill,_fcopyfile,getpriority, andsetpriorityall wrap libc/nix calls that useerrnoon failure and now consistently convert those failures viavm.new_last_errno_error().- Linux
getrandommaps a negative syscall return (caught via thetry_intofailure) tovm.new_last_os_error(), which on Unix is effectively “last errno” and aligns with the new VM API.Overall this standardizes the errno-to-Python mapping without changing control flow or return values on success.
Also applies to: 679-737, 747-756, 760-778, 862-903, 911-953, 1728-1736, 1773-1780, 1891-1921, 2451-2473
crates/vm/src/vm/vm_new.rs (2)
10-10: LGTM!Import correctly expanded to include
ToPyExceptionwhich is needed by the new error conversion methods.
205-221: Well-designed API for platform-specific error handling.The two methods correctly distinguish between Windows API errors (
GetLastError) and CRT errno errors. The documentation clearly explains when to use each method, making it easy for callers to choose the appropriate one. The implementation properly delegates toerrno_io_error()fromcommon::oswhich handles the Windows errno-to-winerror translation.crates/common/src/os.rs (1)
22-34: LGTM!The platform-specific implementations of
errno_io_error()are correct:
- Windows: Reads CRT errno, converts to Windows error code, then creates
io::Error- Non-Windows: Directly uses
last_os_error()since POSIX errno is the OS errorcrates/vm/src/stdlib/os.rs (5)
5-5: LGTM!Import correctly updated to remove
PyBaseExceptionRefwhich is now accessed via full path where needed.
24-35: LGTM!The
IntoPyExceptionimplementations correctly use the full path for the return type and maintain the existing conversion logic.
334-348: LGTM!Correct use of
errno_io_error()for libc functions (mkdirat,mkdir) which set errno. The error handling withIOErrorBuilder::with_filenameproperly preserves the path context.
1154-1158: LGTM!Clean simplification of error handling. Using
vm.new_last_os_error()is correct here:
- On Windows:
SetFilePointersetsGetLastError(), whichnew_last_os_error()reads- On non-Windows:
libc::lseeksets errno, andnew_last_os_error()is equivalent tonew_last_errno_error()
1481-1481: LGTM!Correct use of
vm.new_last_errno_error()for Linux syscall error handling. Thecopy_file_rangesyscall sets errno on failure, whichnew_last_errno_error()properly reads.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.