Skip to content

Implement NSFileManager.copyItemAtPath #248

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

Closed
wants to merge 1 commit into from
Closed
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
33 changes: 32 additions & 1 deletion Foundation/NSFileManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,38 @@ public class NSFileManager : NSObject {
}

public func copyItemAtPath(srcPath: String, toPath dstPath: String) throws {
NSUnimplemented()
let fd_from = open(srcPath, O_RDONLY)
if fd_from < 0 {
throw _NSErrorWithErrno(errno, reading: true, path: dstPath, url: nil, extraUserInfo: nil)
}
defer { precondition(close(fd_from) >= 0) }
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be a throwing case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Defer can't throw.

I could rewrite without defer, but it will be more brittle.

let permission = (try! attributesOfItemAtPath(srcPath)[NSFilePosixPermissions] as! NSNumber).unsignedShortValue
let fd_to = open(dstPath, O_WRONLY | O_CREAT | O_EXCL, permission)
Copy link
Contributor

Choose a reason for hiding this comment

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

what about directories?

if fd_to < 0 {
throw _NSErrorWithErrno(errno, reading: false, path: dstPath, url: nil, extraUserInfo: nil)
}
defer { precondition(close(fd_to) >= 0) }

var buf = [UInt8](count: 4096, repeatedValue: 0)

while true {
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps a stat or fstat might avoid the need for a potentially infinite loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do an fstat on each file and compare their off_t. The problem is, this introduces 2 new syscalls into a hot loop, and we pay that tax every 4096 bytes copied.

We already find out the bytes read and written from read and write respectively, so this also introduces a second source of truth.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was suggesting that the stat could be done before the loop to determine the file size ahead of time to know how much would need to be read.

let nread = read(fd_from, &buf, buf.count)
Copy link
Contributor

Choose a reason for hiding this comment

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

Darwin and BSDs have the API copyfile and linux has the similar API called sendfile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On linux, sendfile only works for sockets, not files.

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 guess this changed recently; sendfile works for files in Linux kernels above 2.6.33. source

In that case we should probably use sendfile/copyfile, as long as we're satisfied to take a dependency on that kernel or later. Can you confirm we can do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

well copyfile we can definitely bank on for Darwin, for official support we support Ubuntu 14 & 15, so I believe sendfile is available on both of those versions (14 is Linux Kernel 3.13 so we should be in the clear here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using sendfile on Linux results in error

error: use of unresolved identifier 'sendfile'

I gather that this doesn't come free with Glibc?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be; perhaps the import isn't in the Glibc overlay for linux for #include <sys/sendfile.h>
It could be re-exposed like _CFOpenFileWithMode has to be re-exported to swift.

it would be totally reasonable to make a platform abstraction for copyfile in CFPlatform.c since Darwin is different than linux and if ever there is a Windows port there is a different function there too.

if nread < 0 { throw _NSErrorWithErrno(errno, reading: true, path: srcPath, url: nil, extraUserInfo: nil) }
Copy link
Contributor

Choose a reason for hiding this comment

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

what about EAGAIN?

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 don't believe EAGAIN can be raised here. The Darwin manpage has

[EAGAIN] The file was marked for non-blocking I/O, and no data were ready to be read.

In this case the file was not marked for non-blocking I/O, and the descriptor does not leak anywhere that someone might set it.

Linux manpage is similar.

if nread == 0 { break }
var writeSlice = buf[0..<nread]

while true {
var nwritten: Int! = nil
writeSlice.withUnsafeBufferPointer({ (ptr) -> () in
nwritten = write(fd_to, ptr.baseAddress, ptr.count)
})
if nwritten < 0 {
throw _NSErrorWithErrno(errno, reading: false, path: dstPath, url: nil, extraUserInfo: nil)
}
writeSlice = writeSlice[writeSlice.startIndex.advancedBy(nwritten)..<writeSlice.endIndex]
if writeSlice.count == 0 { break }
}
}
}

public func moveItemAtPath(srcPath: String, toPath dstPath: String) throws {
Expand Down
25 changes: 24 additions & 1 deletion TestFoundation/TestNSFileManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ class TestNSFileManger : XCTestCase {
("test_directoryEnumerator", test_directoryEnumerator),
("test_pathEnumerator",test_pathEnumerator),
("test_contentsOfDirectoryAtPath", test_contentsOfDirectoryAtPath),
("test_subpathsOfDirectoryAtPath", test_subpathsOfDirectoryAtPath)
("test_subpathsOfDirectoryAtPath", test_subpathsOfDirectoryAtPath),
("test_copyItemAtPath", test_copyItemAtPath)
]
}

Expand Down Expand Up @@ -366,4 +367,26 @@ class TestNSFileManger : XCTestCase {
XCTFail("Failed to clean up files")
}
}

func test_copyItemAtPath() {
let inFile = "/tmp/test_copyItemAtPath_\(NSUUID().UUIDString)"
let outFile = "/tmp/test_copyItemAtPath_\(NSUUID().UUIDString)"

let data = NSMutableData()
var nibble = [0,1,2,3,4,5]
while data.length < 10000 {
data.appendBytes(&nibble, length: nibble.count)
}
do {
try data.writeToFile(inFile, options: [])
try NSFileManager.defaultManager().copyItemAtPath(inFile, toPath: outFile)
}
catch {
XCTFail()
}

let outData = NSData(contentsOfFile: outFile)
XCTAssert(data.isEqual(outData))

}
}