-
Notifications
You must be signed in to change notification settings - Fork 708
feat: add homeDirectory accessor to FileSystem #3392
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this! It mostly looks good but there are a few structural changes I'd like made so that it's consistent with the rest of the module.
| } | ||
|
|
||
| #if canImport(Darwin) || canImport(Glibc) || canImport(Musl) || canImport(Android) | ||
| private static func _homeDirectoryFromPasswordDatabase() -> Result<FilePath, Errno> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you refactor this slightly so that it's getpwduid_r and has the uid passed in?
| var result: UnsafeMutablePointer<passwd>? = nil | ||
| var buffer = [CChar](repeating: 0, count: 256) | ||
|
|
||
| let uid: uid_t = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The syscalls and libc calls in NIOFileSystem are structured so that Syscalls.swift contains top-level functions called system_* and libc_* which call through to the appropriate Darwin/Glibc/Musl etc. so that the caller doesn't need to do that switching.
Then layered on top of that are the functions on the Syscall and Libc enums which call though to system_*/libc_* but use Swift types instead of C types (e.g. String in place of char pointer).
To that end, could you add a system_getuid() function to Syscall.swift and System.getuid() function to Syscall.swift?
| guard let baseAddress = pointer.baseAddress else { | ||
| return CInt(ERANGE) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check shouldn't be necessary, buffer won't be empty, it's okay to ! it below.
| }() | ||
|
|
||
| while true { | ||
| let error: CInt = buffer.withUnsafeMutableBufferPointer { pointer in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than dealing with CInt we have a bunch of helpers that deal in Result types. They also handle calling the underlying function again if it was interrupted, so here I think you need a construct like:
let result = buffer.withUnsafeMutableBufferPointer { pointer in
withUnsafeMutablePointer(to &result) { resultPointer in
nothingOrErrno(retryOnInterrupt: true) {
libc_getpwuid_r(uid, &pwd, pointer.baseAddress!, pointer.count, resultPointer)
}
}
}That allows you to switch on the result below and use the Errno type rather than CInt.
| } | ||
|
|
||
| @_spi(Testing) | ||
| public static func homeDirectory(errno: Errno, location: SourceLocation) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are structured to follow the underlying syscall so this should be called getpwuid_r
| return .success(FilePath(String(cString: profile))) | ||
| } | ||
| return .failure(.noSuchFileOrDirectory) | ||
| #else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The #else should have the same guarding as the _homeDirectoryFromPasswordDatabase function does. At the moment it's !os(Windows) which isn't the same as Darwin || Glibc || Musl || Android
|
@glbrntt Thanks for the feedback. Sure, I'll make those changes and update the PR! |
|
hey @cnaples79 are you still working on this? |
Summary
homeDirectoryproperty onFileSystemProtocoland the concrete file system implementationgetpwuid_rfallbackRationale
Changes
homeDirectorygetpwuid_rFixes #3381