More warnings etc. #18
Conversation
In theory, readdir has two separate thread-safety concerns. (Reentrancy for signals handlers should not concern us, although POSIX seems to mostly worry even about reentrancy, with thread-safety following from it.) Is readdir thread-safe when used on different (DIR *) directory pointers concurrently? And is it thread-safe on the same directory pointer. Since all state can be kept in the pointer, it stands to reason that no self-respecting UNIXoid would be unsafe in the case that different threads work on different pointers. (Though Noze is never doing concurrent reads from the same directory pointer anyway.) But even on the same (DIR *) directory pointer, readdir is thread safe -- on both Linux (according to documentation) and Darwin (according to libc source code).
Our behaviour is still undefined though, just in a different way.
This change is API-incompatible, because readirSync now throws, and readdir accepts a callback with an Error argument.
Changes the API -- in the name of safety though.
|
As an Always Right Institute project we only accept stellar commits. |
| @@ -9,34 +9,30 @@ import Dispatch | |||
| import xsys | |||
| import core | |||
|
|
|||
| public func readdir(_ path: String, cb: @escaping ( [ String ]? ) -> Void) { | |||
| public func readdir(_ path: String, cb: @escaping ( Error?, [ String ]? ) -> Void) { | |||
There was a problem hiding this comment.
I don't want to be picky, but I'd prefer if pull-requests would do just a single thing. Hiding API changes like that in a Swift-3.1-warning-fix pull is a little inconvenient ;->
There was a problem hiding this comment.
Sure thing, I shall henceforth try to submit patch series as separate pull requests instead of having many commits on one branch!
... As a picky submitter myself, I feel that my nice, logically separate commits had a pretty cool split going on between the warning fix and the API change, which is now lost from history because it seems you squashed things into fewer commits instead of cherry-picking mine. But I'll happily take most of the blame for not submitting in stages. Mea culpa!
And thanks for merging!
There was a problem hiding this comment.
Sigh. Looks like I didn't use git properly, srz about that, my fault. When merging your branch into my own feature branch for review, the git pull must have squashed the stuff (the sole reason for my complaint - it just looked like a single big patch to me). I just copied what GitHub was telling me :-)
| public var errno : Int32 { return Glibc.errno } | ||
| public var errno : Int32 { | ||
| get { return Glibc.errno } | ||
| set { Glibc.errno = newValue } |
There was a problem hiding this comment.
What? Do we really want to set errno??? :-) I don't think so.
There was a problem hiding this comment.
From man readdir:
If the end of the directory stream is reached, NULL is returned and errno is not changed. If an error occurs, NULL is returned and errno is set appropriately. If an error occurs, NULL is returned and errno is set appropriately.
Really weird. Posix bug? ;-)
By @lichtblau: - fs.readdir() now provides an error in the callback - fs.readdirSync() throws on error - do not use readdir_r but readdir
|
I split the pull into separate commits per change and pushed it into develop. |
Beware:
Tested: