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 Unix.utimes on Windows wrt DST setting #1445

Merged
merged 4 commits into from Dec 10, 2017

Conversation

Projects
None yet
5 participants
@nojb
Copy link
Contributor

nojb commented Oct 25, 2017

See MPR#7660. In short, Unix.utimes can shift the passed timestamp by one hour depending on the DST setting of the operating system. A similar bug for Unix.stat was fixed in #1057.

The fix consists in using the native Windows call SetFileTime instead of _wutime to implement Unix.utimes.

@dra27
Copy link
Contributor

dra27 left a comment

A few comments. I've not had time to verify, but I think setting the last access time to something other than the current time won't work - see https://msdn.microsoft.com/en-us/library/windows/desktop/ms724933(v=vs.85).aspx (particularly the weird trick with calling SetFileTime with a funny value).

/* */
/* OCaml */
/* */
/* Xavier Leroy, projet Cristal, INRIA Rocquencourt */

This comment has been minimized.

@dra27

dra27 Oct 25, 2017

Contributor

This should be your name!

This comment has been minimized.

@nojb

nojb Oct 25, 2017

Author Contributor

Fixed, thanks.


#include <windows.h>

static void convert_time(double unixTime, FILETIME* ft)

This comment has been minimized.

@dra27

dra27 Oct 25, 2017

Contributor

This is a duplicate (or at least closely related) to the same function in stat.c - this can be factored into unixsupport.c instead, I think?

This comment has been minimized.

@nojb

nojb Oct 25, 2017

Author Contributor

The function in stat.c does a FILETIME -> double conversion, while here we use its inverse double -> FILETIME.

This comment has been minimized.

@dra27

dra27 Oct 26, 2017

Contributor

Yes, indeed - in which case, could they both go into unixsupport.c, but with different names?

This comment has been minimized.

@nojb

nojb Oct 26, 2017

Author Contributor

Yes, then can! But I would rather keep that change out of the present PR just for simplicity.

SystemTimeToFileTime(&systemTime, &lastAccessTime);
caml_enter_blocking_section();
res = SetFileTime(hFile, NULL, &lastAccessTime, &lastAccessTime);
caml_leave_blocking_section();

This comment has been minimized.

@dra27

dra27 Oct 25, 2017

Contributor

This would be slightly clearer if either you memcpy lastAccessTime to lastModificationTime or have a pointer so that the call to SetFileTime can appear after this if (i.e. the if is about setting up the parameters to the call to SetFileTime only)

This comment has been minimized.

@nojb

nojb Oct 25, 2017

Author Contributor

Indeed, I added a call to memcpy as suggested.

wpath = caml_stat_strdup_to_utf16(String_val(path));
caml_enter_blocking_section();
hFile = CreateFile(wpath,
GENERIC_WRITE,

This comment has been minimized.

@dra27

dra27 Oct 25, 2017

Contributor

I think FILE_WRITE_ATTRIBUTES is sufficient here.

This comment has been minimized.

@nojb

nojb Oct 25, 2017

Author Contributor

Indeed, fixed.

@nojb

This comment has been minimized.

Copy link
Contributor Author

nojb commented Oct 25, 2017

Setting the last access time seems works on my machine - I pushed an updated test to also check this.

@xavierleroy xavierleroy added this to the 4.07-or-later milestone Oct 25, 2017

@xavierleroy
Copy link
Contributor

xavierleroy left a comment

Looks good except the test.

Unix.utimes txt t0 t0;
let t1 = (Unix.stat txt).Unix.st_mtime in
let t2 = (Unix.stat txt).Unix.st_atime in
Printf.printf "t0 = %.3f\nt1 = %.3f\nt0-.t1 = %.3f\nt0-.t2 = %.3f\n%!" t0 t1 (t0 -. t1) (t0 -. t2)

This comment has been minimized.

@xavierleroy

xavierleroy Oct 26, 2017

Contributor

This test is awfully not portable and will be no end of trouble. There is no guarantee that the file system supports sub-second resolution for file times (see FAT for a counterexample). POSIX.1 stat returns file times as a time_t i.e. an integer number of seconds. utimes may not be supported in which case utime is used which also uses time_t. Please rewrite this test to just check that the times are what you expect up to N seconds. Or remove the test from the testsuite.

This comment has been minimized.

@dra27

dra27 Oct 26, 2017

Contributor

It's great that you always add tests, though! In addition to Xavier's portability comments, can I suggest using a different time for the two parameters (that guards coding mistakes getting parameters the wrong way around and so forth) and also testing the 0.0 case?

This comment has been minimized.

@nojb

nojb Oct 26, 2017

Author Contributor

@xavierleroy Good point; since I would rather have a test than not, I amended as suggested (with N = 10, should be enough for FAT).

@dra27 I added a test for the 0.0 case, but do not test the atime at all, since it is difficult to do this reliably (see comment in test file).

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Oct 26, 2017

GitHub seems to have eaten my previous attempt at commenting - on further reading, I find I'd misinterpreted what the 0xFFFFFFFFF thing is all about. That's for reading/writing a file without altering the last access/last write stamps and is indeed not applicable here.

@nojb nojb force-pushed the nojb:fix_unix_utimes_windows_dst branch from fafb2e2 to 1f2b5ba Oct 26, 2017

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Nov 6, 2017

@xavierleroy @dra27 Are you happy with the new test?

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Nov 26, 2017

I am happy with the new test. Unless @dra27 objects, I move that we merge this PR.

@xavierleroy xavierleroy modified the milestones: consider-for-4.07, 4.07 Nov 26, 2017

@gasche gasche merged commit f0edbc1 into ocaml:trunk Dec 10, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

gasche commented Dec 10, 2017

(I used the "Resolve conflicts" button to resolve the conflicts (in Changes) in this PR, and I am not terribly happy with the result. It generated an extra conflict-solving commit in the PR branch, whose diff mixes changes from the PR and conflicting changes from trunk (so it doesn't make sense in isolation), and then the whole thing is merged back in trunk. If that was to be done again I would use the command-line to rebase manually.)

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.