-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support os.ftruncate on Windows #67856
Comments
With _chsize_s (which supports 64-bit sizes, unlike _chsize), this seems fairly trivial to do, but I'll put a patch up for it in case there's something I've missed. |
Rietveld doesn't accept patches in git format. |
It normally does... I'll regenerate the patch when I get a chance later today. |
Regenerated the patch file. Rietveld may not have liked that the parent changeset in the previous patch doesn't exist (I pulled this out of my patch queue). |
Hmm... doesn't even know that the issue has been changed. Reuploading with a different extension. |
I get a 405 error if I try and upload the patch on http://bugs.python.org/review/23668/ My other patches from last night worked fine, so maybe Rietveld doesn't like this issue for some reason? |
At first glance the patch looks good, but I did not test it. Are there tests for truncate() with large files (> 4 GiB)? ftruncate() is used also im mmap.resize(). Do you want to provide a patch also for mmap? |
I reviewed 23668_2.patch. |
It looks like mmap uses pure Win32 APIs for the Windows implementation, and so _chsize_s isn't necessary. There is a comment "todo: need ... a 'chsize' analog" in the file, but I'm not actually sure what that means. I've made some minor changes from the review, but I had a question about possibly defining HAVE_[F]TRUNCATE in *.c rather than *.h. See the review, and I'll update the patch once we've figured that out. |
Updated the patch, since there's been a lot of checkins. I also removed the pyconfig.h changes and updated the #ifdef in posixmodule.c to enable truncate/ftruncate and define PATH_HAVE_FTRUNCATE. And I know in the last review I said I'd switch to _Py_wopen(), but if I do that there's no way to avoid passing _O_CREAT, so I opted to stick with _wopen() and pass _O_NOINHERIT instead. Hopefully Reitveld handles this patch file. I'm sure I'm not doing anything differently from normal... |
Is _chsize_s() available on all Windows versions and all Visual Studio
|
Yep, all the way back to VS 2005 and Windows 95. Not sure why it wasn't used previously (_chsize doesn't support 64-bit values, which is a reasonable reason to not use it). |
I responded to Victor's suggestion about _Py_open instead of _open, but on rereading I see that it also handles EINTR. AFAICT (from https://msdn.microsoft.com/en-us/library/5814770t.aspx, mainly), Windows isn't ever going to return EINTR from the CRT. I don't especially want to have to modify _Py_open to take another parameter to suppress creating a new file, but if someone else does then I'm happy to use it. |
Sorry, I still fail to see how _Py_open() or _Py_open_noraise() add the O_CREAT flag: they are thin wrapper to the underyling open() function. I cannot see O_CREAT in Python/fileutils.c. Could you please show me that the line number adding O_CREAT implictly? I don't understand. |
Sorry, _Py_open requires a double encoding (wchar->char->wchar), which is why I'm not going to use it. _Py_wfopen takes a mode string rather than _O_* flags, and so implicitly includes _O_CREAT. Guessing we should add _Py_wopen? |
23668_3.patch looks good to me. I agree that handling EINTR is not needed on Windows, and so there is no need for an helper function like _Py_open_noraise(). |
New changeset 505bf6086ec5 by Steve Dower in branch 'default': New changeset eba85ae7d128 by Steve Dower in branch 'default': |
New changeset cb7ca578a0c3 by Steve Dower in branch 'default': |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: