gh-78502: Add a trackfd parameter to mmap.mmap() on Windows#138238
gh-78502: Add a trackfd parameter to mmap.mmap() on Windows#138238serhiy-storchaka merged 7 commits intopython:mainfrom
Conversation
If trackfd is False, the file handle corresponding to fileno will not be duplicated.
If trackfd is False, the file handle corresponding to fileno will not be duplicated.
d1ce256 to
558beef
Compare
|
🤖 New build scheduled with the buildbot fleet by @serhiy-storchaka for commit 558beef 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F138238%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
encukou
left a comment
There was a problem hiding this comment.
I'm not much of a Windows expert, but, the code looks good to me! Thank you!
zooba
left a comment
There was a problem hiding this comment.
Other than that condition, looks fine to me.
Modules/mmapmodule.c
Outdated
|
|
||
| #ifdef MS_WINDOWS | ||
| if (self->file_handle != INVALID_HANDLE_VALUE) { | ||
| if (self->file_handle != INVALID_HANDLE_VALUE || !self->trackfd) { |
There was a problem hiding this comment.
I don't like combining an error condition check with a state check like this, it gives us a way to call GetFileSize(INVALID_HANDLE_VALUE, ...).
Maybe this should be && self->trackfd instead?
There was a problem hiding this comment.
They both are state checks. The behavior is consistent with Posix -- if trackfd is false, we get an OSError. We could raise a different error, but then we need to change the Posix code too. Is it fine to do this in this PR, or better open a separate PR?
There was a problem hiding this comment.
We should never deliberately pass INVALID_HANDLE_VALUE to a Win32 API, but we might in this case because of the or condition.
If trackfd is false, we should just raise an error directly, rather than relying on the OS to generate an error code for us.
There was a problem hiding this comment.
If trackfd is false, we should just raise an error directly, rather than relying on the OS to generate an error code for us.
I agree with that.
Modules/mmapmodule.c
Outdated
|
|
||
| #ifdef MS_WINDOWS | ||
| if (self->file_handle != INVALID_HANDLE_VALUE) { | ||
| if (self->file_handle != INVALID_HANDLE_VALUE || !self->trackfd) { |
There was a problem hiding this comment.
If trackfd is false, we should just raise an error directly, rather than relying on the OS to generate an error code for us.
I agree with that.
Co-authored-by: Victor Stinner <vstinner@python.org>
|
Before merging this PR, it would be convenient to merge #24781. |
…thonGH-138238) If trackfd is False, the file handle corresponding to fileno will not be duplicated.
If trackfd is False, the file handle corresponding to fileno will not be duplicated.
📚 Documentation preview 📚: https://cpython-previews--138238.org.readthedocs.build/