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.

Sign up for GitHub

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

[Pitch] Add non-throwing FileDescriptor.currentOffset helper #50

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[Pitch] Add non-throwing FileDescriptor.currentOffset helper #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions Sources/System/FileOperations.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,12 @@ extension FileDescriptor {
try _seek(offset: offset, from: whence).get()
}

/// Return the current offset for the given file descriptor.
@_alwaysEmitIntoClient
public var currentOffset: Int64 {
try! _seek(offset: 0, from: .current).get()
}
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 for files that have no concept of position?

 [ESPIPE]           Fildes is associated with a pipe, socket, or FIFO.

That being said, I do like the idea of a very simple AEIC helper.

Copy link
Author

Choose a reason for hiding this comment

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

Great point, I must have missed that one in my first pass. Here’s a review of the error codes on Darwin and how we might handle each:

  • EBADF: Implies that the instance was constructed with an invalid descriptor using init(rawValue:), or is being called after close(), or something else. I think it makes sense to trap here.
  • EINVAL: The seek location and whence aren’t provided by the client. This would presumably be a bug in System, and should trap.
  • ENXIO: Only applies to SEEK_DATA and SEEK_HOLE.
  • EOVERFLOW: File is larger than would fit in an Int64. Should probably trap?
  • ESPIPE: No concept of a current location. This seems much more likely than the other cases, and so a trap here might be more surprising to the caller.

According to https://man7.org/linux/man-pages/man2/lseek.2.html, calling lseek(2) on a terminal device will fail with ESPIPE. I just tested this on Darwin with stdin, and it doesn’t fail, although the return value is probably meaningless.

My first inclination was to change the return type to Int64?. We could still trap for the above documented cases, but on ESPIPE return nil. It’s already up to the user to understand what kind of file descriptor they’re working with, which they could use to decide whether to force unwrap. However, a given file descriptor is either always going to return a valid offset, or always return nil, and being required to force unwrap when you know you have a regular file is unfortunate (although still far more convenient than try! fd.seek(offset: 0, from: .current)).

Depending on System’s source compatibility story, a second option is a throwing accessor, resulting in try fd.currentOffset. This doesn’t discard any information, while providing a convenient shorthand.

I think it might boil down to this: one has to know whether this method is valid to call on a given file descriptor already. Given the API options I’ve described so far, all of the following would trap: fd.currentOffset, fd.currentOffset! or try! fd.currentOffset.

If we can require Swift 5.5, I think I’m most in favour of the last one. I don’t think the optional version of the API is enough of an improvement over the current patch. If requiring Swift 5.5 isn’t an option, maybe we could make it a method instead, or perhaps guard it with an #if compiler() directive?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also inclined towards the throwing computed var. I'll have to check how to deal with the required tools versions.


@usableFromInline
internal func _seek(
offset: Int64, from whence: FileDescriptor.SeekOrigin
Expand Down
11 changes: 11 additions & 0 deletions Tests/SystemTests/FileOperationsTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,17 @@ final class FileOperationsTest: XCTestCase {
// TODO: Test writeAll, writeAll(toAbsoluteOffset), closeAfter
}

func testCurrentOffset() throws {
let fd = try FileDescriptor.open("/tmp/a.txt", .readWrite, options: [.create, .truncate], permissions: .ownerReadWrite)
XCTAssertEqual(fd.currentOffset, 0)

try fd.writeAll("abc".utf8)
XCTAssertEqual(fd.currentOffset, 3)

try fd.seek(offset: -1, from: .current)
XCTAssertEqual(fd.currentOffset, 2)
}

func testAdHocOpen() {
// Ad-hoc test touching a file system.
do {
Expand Down