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

Add API for setting last accessed and last modified file times #2735

Merged
merged 5 commits into from
Jun 10, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Sources/CNIOLinux/include/CNIOLinux.h
Original file line number Diff line number Diff line change
Expand Up @@ -133,5 +133,8 @@ extern const int CNIOLinux_AT_EMPTY_PATH;
extern const unsigned int CNIOLinux_RENAME_NOREPLACE;
extern const unsigned int CNIOLinux_RENAME_EXCHANGE;

extern const unsigned long CNIOLinux_UTIME_OMIT;
extern const unsigned long CNIOLinux_UTIME_NOW;

#endif
#endif
4 changes: 4 additions & 0 deletions Sources/CNIOLinux/shim.c
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ void CNIOLinux_i_do_nothing_just_working_around_a_darwin_toolchain_bug(void) {}
#include <assert.h>
#include <time.h>
#include <sys/ioctl.h>
#include <sys/stat.h>

_Static_assert(sizeof(CNIOLinux_mmsghdr) == sizeof(struct mmsghdr),
"sizes of CNIOLinux_mmsghdr and struct mmsghdr differ");
Expand Down Expand Up @@ -211,4 +212,7 @@ const unsigned int CNIOLinux_RENAME_NOREPLACE = RENAME_NOREPLACE;
const unsigned int CNIOLinux_RENAME_EXCHANGE = RENAME_EXCHANGE;
const int CNIOLinux_AT_EMPTY_PATH = AT_EMPTY_PATH;

const unsigned long CNIOLinux_UTIME_OMIT = UTIME_OMIT;
const unsigned long CNIOLinux_UTIME_NOW = UTIME_NOW;

#endif
50 changes: 50 additions & 0 deletions Sources/NIOFileSystem/FileHandle.swift
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,16 @@ extension _HasFileHandle {
public func close() async throws {
try await self.fileHandle.close()
}

public func setTimes(
lastAccess: FileInfo.Timespec?,
lastDataModification: FileInfo.Timespec?
) async throws {
try await self.fileHandle.setTimes(
lastAccess: lastAccess,
lastDataModification: lastDataModification
)
}
}

/// Implements ``FileHandleProtocol`` by making system calls to interact with the local file system.
Expand Down Expand Up @@ -148,6 +158,16 @@ public struct FileHandle: FileHandleProtocol {
public func close() async throws {
try await self.systemFileHandle.close()
}

public func setTimes(
lastAccess: FileInfo.Timespec?,
lastDataModification: FileInfo.Timespec?
) async throws {
try await self.systemFileHandle.setTimes(
lastAccess: lastAccess,
lastDataModification: lastDataModification
)
}
}

/// Implements ``ReadableFileHandleProtocol`` by making system calls to interact with the local
Expand All @@ -173,6 +193,16 @@ public struct ReadFileHandle: ReadableFileHandleProtocol, _HasFileHandle {
public func readChunks(in range: Range<Int64>, chunkLength: ByteCount) -> FileChunks {
self.fileHandle.systemFileHandle.readChunks(in: range, chunkLength: chunkLength)
}

public func setTimes(
lastAccess: FileInfo.Timespec?,
lastDataModification: FileInfo.Timespec?
) async throws {
try await self.fileHandle.systemFileHandle.setTimes(
lastAccess: lastAccess,
lastDataModification: lastDataModification
)
}
}

/// Implements ``WritableFileHandleProtocol`` by making system calls to interact with the local
Expand Down Expand Up @@ -203,6 +233,16 @@ public struct WriteFileHandle: WritableFileHandleProtocol, _HasFileHandle {
public func close(makeChangesVisible: Bool) async throws {
try await self.fileHandle.systemFileHandle.close(makeChangesVisible: makeChangesVisible)
}

public func setTimes(
lastAccess: FileInfo.Timespec?,
lastDataModification: FileInfo.Timespec?
) async throws {
try await self.fileHandle.systemFileHandle.setTimes(
lastAccess: lastAccess,
lastDataModification: lastDataModification
)
}
}

/// Implements ``ReadableAndWritableFileHandleProtocol`` by making system calls to interact with the
Expand Down Expand Up @@ -247,6 +287,16 @@ public struct ReadWriteFileHandle: ReadableAndWritableFileHandleProtocol, _HasFi
public func close(makeChangesVisible: Bool) async throws {
try await self.fileHandle.systemFileHandle.close(makeChangesVisible: makeChangesVisible)
}

public func setTimes(
lastAccess: FileInfo.Timespec?,
lastDataModification: FileInfo.Timespec?
) async throws {
try await self.fileHandle.systemFileHandle.setTimes(
lastAccess: lastAccess,
lastDataModification: lastDataModification
)
}
}

/// Implements ``DirectoryFileHandleProtocol`` by making system calls to interact with the local
Expand Down
27 changes: 27 additions & 0 deletions Sources/NIOFileSystem/FileHandleProtocol.swift
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,26 @@ public protocol FileHandleProtocol {
///
/// After closing the handle calls to other functions will throw an appropriate error.
func close() async throws

/// Sets the file's last access and last data modification times to the given values.
///
/// If **either** time is `nil`, the current value will not be changed.
/// If **both** times are `nil`, then both times will be set to the current time.
Comment on lines +149 to +150
Copy link
Contributor

@MahdiBM MahdiBM Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO "clamp/update the value to the closest valid value" is good/fine, but the API design still bothers me.

I would have preferred these 2 lines to have been obvious in the API design.
Perhaps using an (structified) enum as the single parameter that the function accepts. Or anything better. Maybe splitting up the responsibility into 2/3 different functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's clear from setTimes(lastAccess: t) that lastDataModification isn't modified.

It's fair to say though that setting both to nil isn't an obvious API: setTimes(lastAccess: nil, lastDataModification: nil) but that's why we also have the touch() wrapper which many developers are familiar with.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we had separate methods for each, we'd unavoidably have to make two separate sys calls, which is expensive. We'd also not be setting the times to the exact same value, as the current time will have changed if we use the UTIME_NOW timespec.

However, I've added two new convenience methods that default one of the parameters to nil to have nicer API: setLastAccessTime(to:) and setLastDataModificationTime(to:).

///
/// > Important: Times are only considered valid if their nanoseconds components are one of the following:
/// > - `UTIME_NOW` (you can use ``FileInfo/Timespec/now`` to get a Timespec set to this value),
/// > - `UTIME_OMIT` (you can use ``FileInfo/Timespec/omit`` to get a Timespec set to this value),,
/// > - Greater than zero and no larger than 1000 million
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove this line as we clamp the value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was better to explain that we will clamp/update the value to the closest valid value instead. Otherwise if people are willingly using an invalid value they could be confused as to why the times are being set to the "wrong" thing. Let me know if you don't agree.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah I'm happy with that.

///
/// - Parameters:
/// - lastAccessTime: The new value of the file's last access time, as time elapsed since the Epoch.
/// - lastDataModificationTime: The new value of the file's last data modification time, as time elapsed since the Epoch.
///
/// - Throws: If there's an error updating the times. If this happens, the original values won't be modified.
func setTimes(
lastAccess: FileInfo.Timespec?,
lastDataModification: FileInfo.Timespec?
) async throws
}

// MARK: - Readable
Expand Down Expand Up @@ -486,6 +506,13 @@ extension WritableFileHandleProtocol {
) async throws -> Int64 {
try await self.write(contentsOf: buffer.readableBytesView, toAbsoluteOffset: offset)
}

/// Sets the file's last access and last data modification times to the current time.
///
/// - Throws: If there's an error updating the times. If this happens, the original values won't be modified.
public func touch() async throws {
try await self.setTimes(lastAccess: nil, lastDataModification: nil)
}
}

/// A file handle which is suitable for reading and writing.
Expand Down
25 changes: 25 additions & 0 deletions Sources/NIOFileSystem/FileInfo.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@ import SystemPackage
import Darwin
#elseif canImport(Glibc)
import Glibc
import CNIOLinux
#elseif canImport(Musl)
import Musl
import CNIOLinux
#endif

/// Information about a file system object.
Expand Down Expand Up @@ -145,6 +147,29 @@ extension FileInfo {

/// A time interval consisting of whole seconds and nanoseconds.
public struct Timespec: Hashable, Sendable {
#if canImport(Darwin)
private static let UTIME_OMIT_INT = Int(UTIME_OMIT)
private static let UTIME_NOW_INT = Int(UTIME_NOW)
#elseif canImport(Glibc) || canImport(Musl)
private static let UTIME_OMIT_INT = Int(CNIOLinux_UTIME_OMIT)
private static let UTIME_NOW_INT = Int(CNIOLinux_UTIME_NOW)
gjcairo marked this conversation as resolved.
Show resolved Hide resolved
#endif

/// A timespec where the seconds are set to zero and the nanoseconds set to `UTIME_OMIT`.
/// In syscalls such as `futimens`, this means the time component set to this value will be ignored.
public static var omit = Self.init(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 public static var alert! 🚨

This should either be a let or a computed property. Same goes for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also let's avoid Self.init(...) and just use Self(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh god, why did I use var. Good catch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easily done 🙂

seconds: 0,
nanoseconds: Self.UTIME_OMIT_INT
)

/// A timespec where the seconds are set to zero and the nanoseconds set to `UTIME_NOW`.
/// In syscalls such as `futimens`, this means the time component set to this value will be
/// be set to the current time or the largest value supported by the platform, whichever is smaller.
public static var now = Self.init(
seconds: 0,
nanoseconds: Self.UTIME_NOW_INT
)

/// The number of seconds.
public var seconds: Int

Expand Down
38 changes: 38 additions & 0 deletions Sources/NIOFileSystem/FileSystemError+Syscall.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1066,6 +1066,44 @@ extension FileSystemError {
location: location
)
}

@_spi(Testing)
public static func futimens(
gjcairo marked this conversation as resolved.
Show resolved Hide resolved
errno: Errno,
path: FilePath,
lastAccessTime: FileInfo.Timespec?,
lastDataModificationTime: FileInfo.Timespec?,
location: SourceLocation
) -> FileSystemError {
let code: FileSystemError.Code
let message: String

switch errno {
case .permissionDenied, .notPermitted:
code = .permissionDenied
message = "Not permited to change last access or last data modification times for \(path)."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
message = "Not permited to change last access or last data modification times for \(path)."
message = "Not permitted to change last access or last data modification times for \(path)."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ this one didn't get fixed


case .readOnlyFileSystem:
code = .unsupported
message = "Not permitted to change last access or last data modification times for \(path): this is a read-only file system."

case .badFileDescriptor:
code = .closed
message = "Could not change last access or last data modification dates for \(path): file is closed."

default:
code = .unknown
message = "Could not change last access or last data modification dates for \(path)."
}

return FileSystemError(
code: code,
message: message,
systemCall: "futimens",
errno: errno,
location: location
)
}
}

#endif
10 changes: 10 additions & 0 deletions Sources/NIOFileSystem/Internal/System Calls/Syscall.swift
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,16 @@ public enum Syscall {
}
}
#endif

@_spi(Testing)
public static func futimens(
fileDescriptor fd: FileDescriptor,
times: UnsafePointer<timespec>?
) -> Result<Void, Errno> {
nothingOrErrno(retryOnInterrupt: false) {
system_futimens(fd.rawValue, times)
}
}
}

@_spi(Testing)
Expand Down
12 changes: 12 additions & 0 deletions Sources/NIOFileSystem/Internal/System Calls/Syscalls.swift
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,18 @@ internal func system_sendfile(
}
#endif

internal func system_futimens(
_ fd: CInt,
_ times: UnsafePointer<timespec>?
) -> CInt {
#if ENABLE_MOCKING
if mockingEnabled {
return mock(fd, times)
}
#endif
return futimens(fd, times)
}

// MARK: - libc

/// fdopendir(3): Opens a directory stream for the file descriptor
Expand Down
Loading