Win: Add GetHostNameW fallback for win7 using gethostname#150909
Win: Add GetHostNameW fallback for win7 using gethostname#150909Fulgen301 wants to merge 1 commit intorust-lang:mainfrom
Conversation
|
r? @ChrisDenton rustbot has assigned @ChrisDenton. Use |
| compat_fn_with_fallback! { | ||
| pub static WS2_32: &CStr = c"ws2_32"; | ||
|
|
||
| #[cfg(target_vendor = "win7")] |
There was a problem hiding this comment.
this seems redundant with the outer cfg attr...
| // SAFETY: these parameters specify a valid, writable region of memory. | ||
| unsafe { | ||
| if c::gethostname(buffer.as_mut_ptr().cast(), buffer.len() as i32) == c::SOCKET_ERROR { | ||
| return c::SOCKET_ERROR; | ||
| } |
There was a problem hiding this comment.
The safety comment seems to suffice for these lines, but the unsafe block continues. I think it would be better if each unsafe call had its own unsafe block with its own safety comments.
| if len <= 0 { | ||
| // GetHostNameW reports WSAEFAULT if the buffer is too small | ||
| if c::GetLastError() == c::ERROR_INSUFFICIENT_BUFFER { | ||
| c::SetLastError(c::WSAEFAULT as _); |
There was a problem hiding this comment.
This should call WSASetLastError.
There was a problem hiding this comment.
Technically unnecessary, as WSASetLastError has been the same as SetLastError for the last decades.
|
|
||
| #[cfg(target_vendor = "win7")] | ||
| pub unsafe fn hostname_fallback(name: c::PWSTR, namelen: i32) -> i32 { | ||
| assert!(namelen >= 1); |
There was a problem hiding this comment.
I think this should return SOCKET_ERROR and set the error to WSAEFAULT, just to be safe.
| namelen - 1, | ||
| ); | ||
|
|
||
| if len <= 0 { |
There was a problem hiding this comment.
MultiByteToWideChar always returns zero on error
| if len <= 0 { | |
| if len == 0 { |
|
|
||
| if len <= 0 { | ||
| // GetHostNameW reports WSAEFAULT if the buffer is too small | ||
| if c::GetLastError() == c::ERROR_INSUFFICIENT_BUFFER { |
There was a problem hiding this comment.
If the error is not ERROR_INSUFFICIENT_BUFFER, this will not set the socket error variable to indicate a reason for SOCKET_ERROR. I'm not sure what error value is correct, through...
|
Reminder, once the PR becomes ready for a review, use |
|
cc @roblabla @PaulDance it would seem a bit silly to merge #150905 then immediately merge this PR. Do you have any thoughts on this PR. |
Yes, this PR obsoletes #150905, and seems like the right approach to me. |
|
|
||
| // SAFETY: these parameters specify a valid, writable region of memory. | ||
| unsafe { | ||
| if c::gethostname(buffer.as_mut_ptr().cast(), buffer.len() as i32) == c::SOCKET_ERROR { |
There was a problem hiding this comment.
It may be worth documenting that we're using this function in the "Underlying system calls" section of library/std/src/net/hostname.rs?
There was a problem hiding this comment.
I don't think we document syscalls for the win7 target anywhere(?).
There was a problem hiding this comment.
It's technically not a syscall either.
There was a problem hiding this comment.
The rust documentation has a habit of using the term "syscall" when it means OS API (technically they're not even syscalls on Linux since we go through libc). But yes, I agree it makes sense to document it. E.g. perhaps similarly to SystemTime.
Yes, it is, but I thought doing so would have the advantage of getting a quick and easy fix known to work merged fast while a more comprehensive but complex one would be implemented with ample time concurrently. Considering the above comments, it kind of confirms that. Either way works in the end, though. |
| ); | ||
|
|
||
| if len <= 0 { | ||
| // GetHostNameW reports WSAEFAULT if the buffer is too small |
There was a problem hiding this comment.
| // GetHostNameW reports WSAEFAULT if the buffer is too small | |
| // gethostname reports WSAEFAULT if the buffer is too small |
if it indeed still applies here.
There was a problem hiding this comment.
That was intentional - if GetHostNameW had a too small buffer, it'd report WSAEFAULT. This provides a fallback implementation, so it matches that error behavior.
| pub static WS2_32: &CStr = c"ws2_32"; | ||
|
|
||
| #[cfg(target_vendor = "win7")] | ||
| pub fn GetHostNameW(name: PWSTR, namelen: i32) -> i32 { |
There was a problem hiding this comment.
I guess it could be nice to link the original issue somewhere around here.
|
☔ The latest upstream changes (presumably #151615) made this pull request unmergeable. Please resolve the merge conflicts. |
|
This now needs to be rebased on top of #150905. Some its changes need to be integrated or reverted:
|
GetHostNameWis only available starting with Windows 8, butgethostnamehas been available since Windows Vista. Use it as a fallback and convert the result from ANSI to Unicode.Fixes #150896
This also replaces #150905 - I can rebase it onto the other PR should that one get merged first.