◐ Shell
clean mode source ↗

mmap for windows by youknowone · Pull Request #6329 · RustPython/RustPython

youknowone

25-46: LGTM! Imports properly organized for cross-platform support.

The imports are correctly gated with platform-specific cfg attributes:

  • Unix-specific: nix, rustpython_common::crt_fd
  • Windows-specific: Windows handle APIs, file system APIs, and suppress_iph macro

48-71: LGTM! validate_advice correctly restricted to Unix platforms.

The madvise constants and validation are Unix-specific, so the cfg(unix) guard is appropriate. The implementation correctly validates advice values for different Unix variants.

181-188: LGTM! Unified data access via as_slice().

The as_slice() method provides a clean abstraction for accessing the underlying memory across both Read and Write variants of MmapObj. This simplifies data access throughout the codebase.

207-231: LGTM! Proper resource cleanup implemented.

The close_handle() method correctly closes file descriptors on Unix and handles on Windows, using atomic swap to avoid double-close. The Drop implementation ensures cleanup on object destruction. Handle/fd values are properly set to invalid sentinels after closing.

233-277: LGTM! Platform-specific constructor arguments with validation.

The MmapNewArgs struct is appropriately split for Unix and Windows with platform-specific fields:

  • Unix: flags, prot
  • Windows: tagname

The validate_new_args() method correctly checks for negative values and returns the validated size.

369-462: LGTM! Unix constructor implementation preserved.

The Unix implementation correctly:

  • Validates arguments and determines map_size from file when length is 0
  • Handles access mode conversion
  • Duplicates the file descriptor to maintain independent ownership
  • Creates appropriate mmap types based on access mode

778-780: LGTM! close() now properly closes underlying handle.

The close method correctly calls close_handle() to release the fd/handle resource in addition to dropping the mmap.

850-870: LGTM! madvise correctly gated for Unix (excluding redox).

The madvise implementation is appropriately restricted to Unix platforms that support it, excluding redox. The call to validate_advice and libc::madvise is correct.

992-998: Unix resize remains unimplemented - placeholder is correct.

The Unix resize method correctly returns an error indicating mremap() is not available. This is appropriate as the TODO comment indicates future implementation is planned.

1156-1183: LGTM! Windows size() implementation correct.

The Windows implementation correctly:

  • Returns mmap length for anonymous mappings (no underlying file)
  • Uses GetFileSize for file-backed mappings
  • Properly combines high and low 32-bit values for 64-bit file size
  • Handles GetFileSize error cases

1261-1289: LGTM! Handle lifetime managed correctly with forget.

The past review concern about handle closing on error has been addressed. The current implementation:

  1. Creates File from raw handle (takes ownership)
  2. Creates mmap using the File reference
  3. Uses std::mem::forget(file) to prevent File from closing the handle on drop
  4. Returns the mmap result

If mmap creation fails and returns early via ? on lines 1279, 1281, or 1282, the File is dropped before the forget call, which correctly closes the handle. However, this is actually incorrect behavior for the caller's expectations.

Wait, let me re-read the past review comment... The past comment says "the caller (in resize) still expects to own the handle after an error."

Looking at the resize function (lines 1073-1096), after calling create_mmap_windows, if it returns an error, the code attempts to restore the original mmap using the same handle. But if create_mmap_windows closed the handle on error, then the restore attempt would fail or use an invalid handle.

Actually, I see the fix was applied: the code no longer uses ? inside the match. Instead:

  • Line 1277: Creates the result without ?
  • Line 1286: Calls forget unconditionally
  • Line 1288: Returns the result (propagates error if any)

This ensures forget is always called, preventing the handle from being closed even on error. This is correct for the caller's expectations.

However, there's still an issue: If the mmap APIs return an error, we call forget, then return the error. The handle remains open and owned by the caller, but the caller (resize) expects it to be in the PyMmap's handle field, which it already is. So this is actually correct.

Based on learnings, the implementation correctly follows the pattern established in the codebase for Windows handle management.

464-609: Code approved — handle cleanup is correct on all paths.

The Windows constructor properly manages the duplicated handle across all error cases:

  • Pre-transfer errors (lines 516–562): All explicitly close duplicated_handle before returning.
  • Post-transfer on mmap failure (line 585): The OwnedHandle automatically drops when mmap_result fails, triggering its Drop impl which calls CloseHandle.
  • Success path (lines 588–589): The raw handle is extracted and OwnedHandle is forgotten, preventing Drop from closing the handle prematurely.

No resource leaks exist.

1000-1115: Correct the error recovery analysis; the resize implementation properly handles file size mismatches.

The concern about error recovery at lines 1073-1096 is not valid. Windows API guarantees that if SetEndOfFile fails, the file size remains unchanged on disk. Since the code already drops the mmap at line 1055 before attempting SetFilePointerEx/SetEndOfFile, recreating the mmap with old_size after SetEndOfFile failure is correct—the file will still be at its original size.

The only remaining minor consideration is that the pointer cast at lines 1042-1048 (casting *const u8 to *mut u8) is valid but could use a mutable reference for clarity, though this is a style preference rather than a correctness issue.

Likely an incorrect or invalid review comment.