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

Clone files on Darwin rather than copying them #2823

Merged
merged 2 commits into from
Aug 5, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
42 changes: 22 additions & 20 deletions Sources/NIOFileSystem/FileSystem.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1172,6 +1172,27 @@ extension FileSystem {
)
}

#if canImport(Darwin)
// COPYFILE_CLONE clones the file if possible and will fallback to doing a copy.
// COPYFILE_ALL is shorthand for:
// COPYFILE_STAT | COPYFILE_ACL | COPYFILE_XATTR | COPYFILE_DATA
let flags = copyfile_flags_t(COPYFILE_CLONE) | copyfile_flags_t(COPYFILE_ALL)
return Libc.copyfile(
from: sourcePath,
to: destinationPath,
state: nil,
flags: flags
).mapError { errno in
FileSystemError.copyfile(
errno: errno,
from: sourcePath,
to: destinationPath,
location: .here()
)
}

#elseif canImport(Glibc) || canImport(Musl) || canImport(Bionic)

let openSourceResult = self._openFile(
forReadingAt: sourcePath,
options: OpenOptions.Read(followSymbolicLinks: true)
Expand Down Expand Up @@ -1231,25 +1252,6 @@ extension FileSystem {
let copyResult: Result<Void, FileSystemError>
copyResult = source.fileHandle.systemFileHandle.sendableView._withUnsafeDescriptorResult { sourceFD in
destination.fileHandle.systemFileHandle.sendableView._withUnsafeDescriptorResult { destinationFD in
#if canImport(Darwin)
// COPYFILE_CLONE clones the file if possible and will fallback to doing a copy.
// COPYFILE_ALL is shorthand for:
// COPYFILE_STAT | COPYFILE_ACL | COPYFILE_XATTR | COPYFILE_DATA
let flags = copyfile_flags_t(COPYFILE_CLONE) | copyfile_flags_t(COPYFILE_ALL)
return Libc.fcopyfile(
from: sourceFD,
to: destinationFD,
state: nil,
flags: flags
).mapError { errno in
FileSystemError.fcopyfile(
errno: errno,
from: sourcePath,
to: destinationPath,
location: .here()
)
}
#elseif canImport(Glibc) || canImport(Musl) || canImport(Bionic)
var offset = 0

while offset < sourceInfo.size {
Expand Down Expand Up @@ -1277,7 +1279,6 @@ extension FileSystem {
}
}
return .success(())
#endif
} onUnavailable: {
makeOnUnavailableError(path: destinationPath, location: .here())
}
Expand All @@ -1287,6 +1288,7 @@ extension FileSystem {

let closeResult = destination.fileHandle.systemFileHandle.sendableView._close(materialize: true)
return copyResult.flatMap { closeResult }
#endif
}

private func copySymbolicLink(
Expand Down
53 changes: 52 additions & 1 deletion Sources/NIOFileSystem/FileSystemError+Syscall.swift
Original file line number Diff line number Diff line change
Expand Up @@ -992,6 +992,38 @@ extension FileSystemError {
from sourcePath: FilePath,
to destinationPath: FilePath,
location: SourceLocation
) -> Self {
Self._copyfile(
"fcopyfile",
errno: errno,
from: sourcePath,
to: destinationPath,
location: location
)
}

@_spi(Testing)
public static func copyfile(
errno: Errno,
from sourcePath: FilePath,
to destinationPath: FilePath,
location: SourceLocation
) -> Self {
Self._copyfile(
"copyfile",
errno: errno,
from: sourcePath,
to: destinationPath,
location: location
)
}

private static func _copyfile(
_ name: String,
errno: Errno,
from sourcePath: FilePath,
to destinationPath: FilePath,
location: SourceLocation
) -> Self {
let code: Code
let message: String
Expand All @@ -1016,6 +1048,25 @@ extension FileSystemError {
Can't copy file from '\(sourcePath)' to '\(destinationPath)', the destination \
path already exists.
"""
case .fileExists:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update the error tests? They check that errnos get mapped to an appropriate file system error code.

code = .fileAlreadyExists
message = """
Unable to create file at path '\(destinationPath)', no existing file options were set \
which implies that no file should exist but a file already exists at the \
specified path.
"""
case .tooManyOpenFiles:
code = .unavailable
message = """
Unable to open the source ('\(sourcePath)') or destination ('\(destinationPath)') files, \
too many file descriptors are open.
"""
case .noSuchFileOrDirectory:
code = .notFound
message = """
Unable to open or create file at path '\(sourcePath)', either a component of the \
path did not exist or the named file to be opened did not exist.
"""
default:
code = .unknown
message = "Can't copy file from '\(sourcePath)' to '\(destinationPath)'."
Expand All @@ -1024,7 +1075,7 @@ extension FileSystemError {
return FileSystemError(
code: code,
message: message,
systemCall: "fcopyfile",
systemCall: name,
errno: errno,
location: location
)
Expand Down
18 changes: 18 additions & 0 deletions Sources/NIOFileSystem/Internal/System Calls/Syscall.swift
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,24 @@ public enum Libc {
}
#endif

#if canImport(Darwin)
@_spi(Testing)
public static func copyfile(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for this please?

func test_fcopyfile() throws {
?

from source: FilePath,
to destination: FilePath,
state: copyfile_state_t?,
flags: copyfile_flags_t
) -> Result<Void, Errno> {
source.withPlatformString { sourcePath in
destination.withPlatformString { destinationPath in
nothingOrErrno(retryOnInterrupt: false) {
libc_copyfile(sourcePath, destinationPath, state, flags)
}
}
}
}
#endif

@_spi(Testing)
public static func remove(
_ path: FilePath
Expand Down
19 changes: 18 additions & 1 deletion Sources/NIOFileSystem/Internal/System Calls/Syscalls.swift
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ internal func libc_remove(
}

#if canImport(Darwin)
/// copyfile(3): Copy a file from one file to another.
/// fcopyfile(3): Copy a file from one file to another.
internal func libc_fcopyfile(
_ from: CInt,
_ to: CInt,
Expand All @@ -416,6 +416,23 @@ internal func libc_fcopyfile(
}
#endif

#if canImport(Darwin)
/// copyfile(3): Copy a file from one file to another.
internal func libc_copyfile(
_ from: UnsafePointer<CInterop.PlatformChar>,
_ to: UnsafePointer<CInterop.PlatformChar>,
_ state: copyfile_state_t?,
_ flags: copyfile_flags_t
) -> CInt {
#if ENABLE_MOCKING
if mockingEnabled {
return mock(from, to, state, flags)
}
#endif
return copyfile(from, to, state, flags)
}
#endif

/// getcwd(3): Get working directory pathname
internal func libc_getcwd(
_ buffer: UnsafeMutablePointer<CInterop.PlatformChar>,
Expand Down
14 changes: 7 additions & 7 deletions Tests/NIOFileSystemIntegrationTests/FileSystemTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -659,14 +659,14 @@ final class FileSystemTests: XCTestCase {
/// This is is not quite the same as sequential, different code paths are used.
/// Tests using this ensure use of the parallel paths (which are more complex) while keeping actual
/// parallelism to minimal levels to make debugging simpler.
private let minimalParallel: CopyStrategy = try! .parallel(maxDescriptors: 2)
private static let minimalParallel: CopyStrategy = try! .parallel(maxDescriptors: 2)

func testCopyEmptyDirectorySequential() async throws {
try await testCopyEmptyDirectory(.sequential)
}

func testCopyEmptyDirectoryParallelMinimal() async throws {
try await testCopyEmptyDirectory(minimalParallel)
try await testCopyEmptyDirectory(Self.minimalParallel)
}

func testCopyEmptyDirectoryParallelDefault() async throws {
Expand All @@ -690,7 +690,7 @@ final class FileSystemTests: XCTestCase {
}

func testCopyOnGeneratedTreeStructureParallelMinimal() async throws {
try await testAnyCopyStrategyOnGeneratedTreeStructure(minimalParallel)
try await testAnyCopyStrategyOnGeneratedTreeStructure(Self.minimalParallel)
}

func testCopyOnGeneratedTreeStructureParallelDefault() async throws {
Expand Down Expand Up @@ -737,7 +737,7 @@ final class FileSystemTests: XCTestCase {
}

func testCopySelectivelyParallelMinimal() async throws {
try await testCopySelectively(minimalParallel)
try await testCopySelectively(Self.minimalParallel)
}

func testCopySelectivelyParallelDefault() async throws {
Expand Down Expand Up @@ -780,7 +780,7 @@ final class FileSystemTests: XCTestCase {
}

func testCopyCancelledPartWayThroughParallelMinimal() async throws {
try await testCopyCancelledPartWayThrough(minimalParallel)
try await testCopyCancelledPartWayThrough(Self.minimalParallel)
}

func testCopyCancelledPartWayThroughParallelDefault() async throws {
Expand Down Expand Up @@ -927,7 +927,7 @@ final class FileSystemTests: XCTestCase {
}

func testCopyNonExistentFileParallelMinimal() async throws {
try await testCopyNonExistentFile(minimalParallel)
try await testCopyNonExistentFile(Self.minimalParallel)
}

func testCopyNonExistentFileParallelDefault() async throws {
Expand All @@ -953,7 +953,7 @@ final class FileSystemTests: XCTestCase {
}

func testCopyToExistingDestinationParallelMinimal() async throws {
try await testCopyToExistingDestination(minimalParallel)
try await testCopyToExistingDestination(Self.minimalParallel)
}

func testCopyToExistingDestinationParallelDefault() async throws {
Expand Down
19 changes: 19 additions & 0 deletions Tests/NIOFileSystemTests/FileSystemErrorTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,10 @@ final class FileSystemErrorTests: XCTestCase {
.fcopyfile(errno: .badFileDescriptor, from: "src", to: "dst", location: here)
}

assertCauseIsSyscall("copyfile", here) {
.copyfile(errno: .badFileDescriptor, from: "src", to: "dst", location: here)
}

assertCauseIsSyscall("sendfile", here) {
.sendfile(errno: .badFileDescriptor, from: "src", to: "dst", location: here)
}
Expand Down Expand Up @@ -523,6 +527,21 @@ final class FileSystemErrorTests: XCTestCase {
}
}

func testErrnoMapping_copyfile() {
self.testErrnoToErrorCode(
expected: [
.notSupported: .invalidArgument,
.permissionDenied: .permissionDenied,
.invalidArgument: .invalidArgument,
.fileExists: .fileAlreadyExists,
.tooManyOpenFiles: .unavailable,
.noSuchFileOrDirectory: .notFound,
]
) { errno in
.copyfile(errno: errno, from: "src", to: "dst", location: .fixed)
}
}

func testErrnoMapping_fcopyfile() {
self.testErrnoToErrorCode(
expected: [
Expand Down
14 changes: 14 additions & 0 deletions Tests/NIOFileSystemTests/Internal/SyscallTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,20 @@ final class SyscallTests: XCTestCase {
#endif
}

func test_copyfile() throws {
#if canImport(Darwin)

let testCases: [MockTestCase] = [
MockTestCase(name: "copyfile", .noInterrupt, "foo", "bar", "nil", 0) { _ in
try Libc.copyfile(from: "foo", to: "bar", state: nil, flags: 0).get()
}
]
testCases.run()
#else
throw XCTSkip("'copyfile' is only supported on Darwin")
#endif
}

func test_remove() throws {
let testCases: [MockTestCase] = [
MockTestCase(name: "remove", .noInterrupt, "somepath") { _ in
Expand Down
Loading