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.

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

Fix Pervasives.LargeFiles functions under Windows #1719

Merged
merged 3 commits into from Apr 30, 2018

Conversation

Projects
None yet
3 participants
@alainfrisch
Copy link
Contributor

commented Apr 11, 2018

io.c uses lseek, but under Windows, one must use _lseeki64 to
have 64-bit offsets. Before this commit, calling e.g.
LargeFile.in_channel_length on large file would result in an
exception being raised ("System error: Invalid argument").

Notes:

  • There are occurrences of lseek in startup.c, but they use
    long offsets anyway (which means 32-bit under Windows).

  • Unix.LargeFile is not affected (it uses SetFilePointer).

One should probably add some tests, but one might not want to spend
time creating large files. @dra27 mentions:

it looks like you can use fsutil to create sparse files on NTFS, which might mean we could add tests for these?

Fix Pervasives.LargeFiles functions under Windows
io.c uses lseek, but under Windows, one must use _lseeki64 to
have 64-bit offsets.  Before this commit, calling e.g.
LargeFile.in_channel_length on large file would result in an
exception being raised ("System error: Invalid argument").

Notes:

 - There are occurrences of lseek in startup.c, but they use
   long offsets anyway (which means 32-bit under Windows).

 - Unix.LargeFile is not affected (it uses SetFilePointer).
@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2018

I believe we should integrate this bug fix, even without a proper automated non-regression test. Of course, I tested it manually. (And we are using it in production at LexiFi.)

@nojb @dra27 : would one of you have time to review the PR?

@damiendoligez : do you think this should go to 4.07?

@nojb

nojb approved these changes Apr 25, 2018

Copy link
Contributor

left a comment

LGTM.

Just FTR, this is another bug that would have been caught with reasonable compiler warnings activated (we were passing a variable of type file_offset which is 64-bit wide under Windows to the 32-bit lseek).

alainfrisch added some commits Apr 25, 2018

@alainfrisch alainfrisch merged commit 8a21fa1 into trunk Apr 30, 2018

0 of 4 checks passed

continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2018

Ok, it's probably too later for 4.07. Merged to trunk only.

@damiendoligez

This comment has been minimized.

Copy link
Member

commented May 28, 2018

@alainfrisch It's still time to cherry-pick to 4.07 if you feel strongly about it.
(but you'll have to fix the Changes file in both branches)

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2018

Thanks. The bug has been there forever under Windows and nobody noticed, so I think the fix can wait for the next release.

@alainfrisch alainfrisch deleted the fix_large_file_lseek_windows branch Jul 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.