Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-86768: check if fd is seekable in os.lseek on Windows#133137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
Modules/posixmodule.c Outdated
| if (result >= 0) { | ||
| LARGE_INTEGER distance, newdistance; | ||
| distance.QuadPart = position; | ||
| if (SetFilePointerEx(h, distance, &newdistance, how)) { | ||
| result = newdistance.QuadPart; | ||
| } else { | ||
| result = -1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not simply use_lseeki64() after checkingGetFileType()? It looks much simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I thought Windows's CRT will call_get_osfhandle to convert the POSIX'sfd to Windows'shandle internally, which we've already called, so useSetFilePointerEx with thehandle directly will gain some performance benefit. But I run the small benchmark script which mentioned at the top of this PR, and didn't see any noticeable performance regret, so updated to use_lseeki64 directly.
Modules/posixmodule.c Outdated
| if (errno == 0) { | ||
| errno = winerror_to_errno(GetLastError()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not usePyErr_SetFromWindowsErr(0)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
SetFilePointerEx will set the error toGetLastError instead oferrno, so we should check it. But after changed to call_lseeki64 directly, there is no need for this line of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It may be slightly simpler if setresult to -1 initially.
Modules/posixmodule.c Outdated
| @@ -219,6 +219,7 @@ | |||
| # if defined(MS_WINDOWS_DESKTOP) || defined(MS_WINDOWS_SYSTEM) | |||
| # define HAVE_SYMLINK | |||
| # endif /* MS_WINDOWS_DESKTOP | MS_WINDOWS_SYSTEM */ | |||
| extern int winerror_to_errno(int); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No longer used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM. 👍
Uh oh!
There was an error while loading.Please reload this page.
This change will introduce a performance regression:
Before the change:
After the change:
However I think it's acceptable because we added a check in the implementation and
os.lseekusually won't been called too many times in the real world.