Fix close error message #433
Conversation
From the macOS manpage: > Upon successful completion, a value of 0 is returned. Otherwise, a value > of -1 is returned and the global integer variable errno is set to > indicate the error. I'm not certain whether the existing code may be there for cross-platform support, so I changed it to check for -1 and only then consult the global `errno`. This should preserve existing behavior if there's a cross-platform angle here. rdar://117861543
neonichu
requested review from
abertelrud,
tomerd and
MaxDesiatov
as code owners
|
@swift-ci please smoke test |
|
@swift-ci please test |
Co-authored-by: Max Desiatov <[email protected]>
|
@swift-ci please test |
|
@swift-ci please test windows |
|
cc @weissi , does this looks right to you? |
|
|
@swift-ci please test windows |
|
Looks like the Windows CI error is persistent, cc @shahmishal |
|
@swift-ci please test windows |
| case .close(let err): | ||
| let errorMessage: String | ||
| if err == -1 { // if the return code is -1, we need to consult the global `errno` | ||
| errorMessage = strerror(errno) |
There was a problem hiding this comment.
you need to copy this string immediately or else it's UB and you might get garbage or worse:
man strerror:
BUGS
For unknown error numbers, the strerror() function will return its result
in a static buffer which may be overwritten by subsequent calls.
So you want to save a String(cString: strerror(errno)) + " (\(errno))" or something. I'd say let's not lose the actual errno number.
There was a problem hiding this comment.
Looks like we are basically doing that, the strerror here is not the libc function, but a local one https://github.com/apple/swift-tools-support-core/blob/main/Sources/TSCBasic/misc.swift#L320-L345
| return "close error: \(strerror(errno))" | ||
| case .close(let err): | ||
| let errorMessage: String | ||
| if err == -1 { // if the return code is -1, we need to consult the global `errno` |
There was a problem hiding this comment.
Yes, errno is only guaranteed to overridden when the syscall fails, i.e. returns -1.
From the macOS manpage:
I'm not certain whether the existing code may be there for cross-platform support, so I changed it to check for -1 and only then consult the global
errno. This should preserve existing behavior if there's a cross-platform angle here.rdar://117861543