alexcrichton / filetime

Accessing file timestamps in a platform-agnostic fashion in Rust
Apache License 2.0
122 stars 56 forks source link

Implement set_file_mtime and set_file_atime for Unix platforms #36

Closed antonok-edm closed 5 years ago

antonok-edm commented 5 years ago

I added set_file_mtime() and set_file_atime() as requested in #26. I use the UTIME_OMIT argument if possible, and fall back to just resetting the omitted time to its current value.

The two new functions are gated for Unix targets only; I haven't implemented Windows or Redox versions as I am not familiar with those operating systems and cannot test them.

alexcrichton commented 5 years ago

Thanks for the PR! I didn't know about the UTIME_OMIT trick, and it seems quite nifty!

I'd prefer, however, that no API is exposed from this crate which may panic or do extra syscalls (as this is intended to largely just be one syscall). To that end could the specialized API only be exposed on platforms which support UTIME_OMIT?

The typical way to structure this in the API as well is to have something like a top-level linux module which indicates Linux-specific functionality is being used.

antonok-edm commented 5 years ago

Makes sense, but I'm not quite sure how to go forward with that. As far as I am aware, UTIME_OMIT is supported for any platform with utimensat. Currently in the linux module there is a runtime check for the presence of the utimensat syscall, which falls back on utimes if it's not found. If only one time is requested, I don't see any way to perform that fallback without either panicking or using extra syscalls to "fake" the support for UTIME_OMIT. I think the panic would be much less user friendly, which is why I opted for the second option.

Just to clarify - none of the unimplemented!() calls I added should be reachable since the upper level module doesn't expose a way to set only one of the times on Windows and Redox targets. I opted for unimplemented!() rather than unreachable!() because I am not sure if the appropriate system calls exist for those platforms; I figured it may be possible for someone else to add the appropriate logic in the future if they do.

alexcrichton commented 5 years ago

Oh good point about the runtime detection! Perhaps we could make this an opt-in feature then to indicate "the crate must support at least this kernel" and then utimensat could be used unconditionally?

antonok-edm commented 5 years ago

Ok, I added a utimensat feature that will prevent the library from attempting to call utimes.

By the way, I just noticed you use your own cfg-if crate for conditional features -- what's the difference between that and the cfg! macro in the standard library?

alexcrichton commented 5 years ago

Ok thanks! Would it be possible to add a new API though to unix/mod.rs which doesn't change the signatures of existing functions? It'd be great to avoid the new panics in the other modules (like windows).

Additionally, I think we should continue to discover utimensat directly and avoid panics, although I think we can reasonably just return an error if the platform doesn't have the syscall (like the kernel is too old).

As a somewhat orthogonal thing, it may be best to use libc::syscall rather than libc::dlsym to find the utimensat function perhaps?

Eh2406 commented 5 years ago

FYI, according to the windows docs you can pass null to lpLastAccessTime or lpLastWriteTime to not change the value.

rbtcollins commented 5 years ago

I have an implementation for Windows, will be using it for new set_file_handle_times function I'm adding for tar-rs - I'd love it if this patch were merged first, then I could layer on it.

@alexcrichton

alexcrichton commented 5 years ago

This was merged with https://github.com/alexcrichton/filetime/pull/40