Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support os.ftruncate on Windows #67856

Closed
zooba opened this issue Mar 15, 2015 · 18 comments
Closed

Support os.ftruncate on Windows #67856

zooba opened this issue Mar 15, 2015 · 18 comments
Assignees
@zooba
Labels
OS-windows type-feature A feature request or enhancement

Comments

@zooba
Copy link
Member

zooba commented Mar 15, 2015

BPO 23668
Nosy @vstinner, @tjguk, @zware, @serhiy-storchaka, @zooba
Files
  • 23668_1.diff
  • 23668_2.diff
  • 23668_2.patch
  • 23668_2.patch: Regenerated for Rietveld
  • 23668_3.patch
    		•

    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:

    assignee = 'https://github.com/zooba'
closed_at = <Date 2015-04-27.05:32:23.111>
created_at = <Date 2015-03-15.01:41:38.618>
labels = ['type-feature', 'OS-windows']
title = 'Support os.ftruncate on Windows'
updated_at = <Date 2015-04-27.05:32:23.110>
user = 'https://github.com/zooba'

    bugs.python.org fields:

    activity = <Date 2015-04-27.05:32:23.110>
actor = 'steve.dower'
assignee = 'steve.dower'
closed = True
closed_date = <Date 2015-04-27.05:32:23.111>
closer = 'steve.dower'
components = ['Windows']
creation = <Date 2015-03-15.01:41:38.618>
creator = 'steve.dower'
dependencies = []
files = ['38488', '38499', '38500', '38505', '38614']
hgrepos = []
issue_num = 23668
keywords = ['patch']
message_count = 18.0
messages = ['238113', '238135', '238144', '238149', '238152', '238153', '238184', '238185', '238214', '238752', '238841', '238843', '240003', '240007', '240014', '240385', '240537', '240566']
nosy_count = 6.0
nosy_names = ['vstinner', 'tim.golden', 'python-dev', 'zach.ware', 'serhiy.storchaka', 'steve.dower']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue23668'
versions = ['Python 3.5']
    @zooba
    Copy link
    Member Author

    zooba commented Mar 15, 2015

    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.

    @zooba zooba self-assigned this Mar 15, 2015
    @zooba zooba added OS-windows type-feature A feature request or enhancement labels Mar 15, 2015
    @serhiy-storchaka
    Copy link
    Member

    Rietveld doesn't accept patches in git format.

    @zooba
    Copy link
    Member Author

    zooba commented Mar 15, 2015

    It normally does... I'll regenerate the patch when I get a chance later today.

    @zooba
    Copy link
    Member Author

    zooba commented Mar 15, 2015

    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).

    @zooba
    Copy link
    Member Author

    zooba commented Mar 15, 2015

    Hmm... doesn't even know that the issue has been changed. Reuploading with a different extension.

    @zooba
    Copy link
    Member Author

    zooba commented Mar 15, 2015

    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?

    @serhiy-storchaka
    Copy link
    Member

    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?

    @vstinner
    Copy link
    Member

    I reviewed 23668_2.patch.

    @zooba
    Copy link
    Member Author

    zooba commented Mar 16, 2015

    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.

    @zooba
    Copy link
    Member Author

    zooba commented Mar 21, 2015

    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...

    @vstinner
    Copy link
    Member

    Is _chsize_s() available on all Windows versions and all Visual Studio
    (msvcrt) versions?
    Le 21 mars 2015 04:26, "Steve Dower" <report@bugs.python.org> a écrit :

    Steve Dower added the comment:

    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...

    ----------
    Added file: http://bugs.python.org/file38614/23668_3.patch

    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue23668\>

    @vstinner vstinner changed the title Support os.[f]truncate on Windows Support os. Mar 21, 2015
    @zooba
    Copy link
    Member Author

    zooba commented Mar 21, 2015

    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).

    @zooba zooba changed the title Support os. Support os.ftruncate on Windows Mar 21, 2015
    @zooba
    Copy link
    Member Author

    zooba commented Apr 3, 2015

    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.

    @vstinner
    Copy link
    Member

    vstinner commented Apr 3, 2015

    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.

    @zooba
    Copy link
    Member Author

    zooba commented Apr 3, 2015

    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?

    @vstinner
    Copy link
    Member

    vstinner commented Apr 9, 2015

    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().

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 12, 2015

    New changeset 505bf6086ec5 by Steve Dower in branch 'default':
    Issue bpo-23668: Adds support for os.truncate and os.ftruncate on Windows
    https://hg.python.org/cpython/rev/505bf6086ec5

    New changeset eba85ae7d128 by Steve Dower in branch 'default':
    Issue bpo-23668: Suppresses invalid parameter handler around chsize calls.
    https://hg.python.org/cpython/rev/eba85ae7d128

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 12, 2015

    New changeset cb7ca578a0c3 by Steve Dower in branch 'default':
    Issue bpo-23668: Regenerates posixmodule.c.h for new ifdefs
    https://hg.python.org/cpython/rev/cb7ca578a0c3

    @zooba zooba closed this as completed Apr 27, 2015
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @zooba zooba

    Labels
    OS-windows type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @vstinner @zooba @serhiy-storchaka