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.

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

std: net: Add function to return the system hostname #135141

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

orowith2os
Copy link

@orowith2os orowith2os commented Jan 6, 2025

Resolves rust-lang/libs-team#330
Tracking issue: #135142

Now you can retrieve the system hostname, without relying on anything other than std!

This is my first pull request to the Rust standard library (or anything inside rust-lang/rust, period, quite possible rust-lang itself), so there are bound to be issues. I'll try my best to resolve them.

@rustbot
Copy link
Collaborator

rustbot commented Jan 6, 2025

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joboet (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 6, 2025
@rust-log-analyzer

This comment has been minimized.

library/std/src/net/hostname.rs Outdated Show resolved Hide resolved
library/std/src/net/mod.rs Outdated Show resolved Hide resolved
@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 6, 2025
@rustbot

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@orowith2os orowith2os force-pushed the std-net-gethostname branch from 64aec6c to 2748acd Compare January 6, 2025 01:02
@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 6, 2025
@orowith2os
Copy link
Author

forgive all the updates, I was having troubles updating my branch properly. Figured it out.

@rust-log-analyzer

This comment has been minimized.

@orowith2os orowith2os force-pushed the std-net-gethostname branch from 2748acd to a6af22f Compare January 6, 2025 01:11
@rust-log-analyzer

This comment has been minimized.

@orowith2os orowith2os force-pushed the std-net-gethostname branch from a6af22f to 518f66c Compare January 6, 2025 01:39
@rust-log-analyzer

This comment has been minimized.

@orowith2os orowith2os force-pushed the std-net-gethostname branch 2 times, most recently from b942cde to 2cfa066 Compare January 6, 2025 01:54
@rust-log-analyzer

This comment has been minimized.

@orowith2os orowith2os force-pushed the std-net-gethostname branch from 2cfa066 to 9761a27 Compare January 6, 2025 02:17
@rust-log-analyzer

This comment has been minimized.

@orowith2os orowith2os force-pushed the std-net-gethostname branch 2 times, most recently from ff9bb72 to 141870c Compare January 6, 2025 02:32
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2025
@orowith2os
Copy link
Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 16, 2025
@orowith2os
Copy link
Author

Sorry for that last push - I updated the doc comment on gethostname to specify when it should be able to error out, so it's not just a black box.

@joboet
Copy link
Member

joboet commented Jan 17, 2025

Right, that covers most platforms. Some targets are still missing, though (to find these I basically went through the directories in sys/pal and looked for platforms with a net module):

  • wasm32-wasip1 and wasm32-wasip2 (tier 2)
  • x86_64-fortanix-unknown-sgx (tier 2)
  • aarch64-kmc-solid_asp3 (tier 3)
  • riscv32imac-unknown-xous-elf (tier 3)
  • x86_64-unknown-hermit (tier 3)

@joshtriplett
Copy link
Member

DNS names can only be ASCII, but hostnames are not just DNS names. I've seen hostnames for phones and computers that have Unicode characters in them, and I would be unsurprised to discover hostnames that use local character sets.

That said, if we define this to be a form of hostname that works as a valid DNS component, then 👍 for making it String.

@orowith2os
Copy link
Author

@joboet would it be fine to mark them as TODO in the tracking issue?

@joboet
Copy link
Member

joboet commented Jan 17, 2025

@joboet would it be fine to mark them as TODO in the tracking issue?

For the tier 3 targets, yeah that's fine. The tier 2 targets are built in the merge CI, so they'll need to compile (stubbing the function out is fine).

@joboet
Copy link
Member

joboet commented Jan 21, 2025

DNS names can only be ASCII, but hostnames are not just DNS names. I've seen hostnames for phones and computers that have Unicode characters in them, and I would be unsurprised to discover hostnames that use local character sets.

That said, if we define this to be a form of hostname that works as a valid DNS component, then 👍 for making it String.

T-libs-api, what do you think? I think this should be clarified before merging, since OsString is not the correct output type on Windows – gethostname can return any bytes, whereas OsString must be correct WTF-8, so it either can't represent all possible outputs or can hold too many outputs, depending on whether you consider non-ASCII hostnames to be invalid.

@rustbot label +I-libs-api-nominated

@rustbot rustbot added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jan 21, 2025
@Amanieu
Copy link
Member

Amanieu commented Jan 21, 2025

We discussed this in the libs-api meeting: on Windows this should call GetHostByNameW to get a UTF-16 string, which can then be converted to an OsString.

@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Jan 21, 2025
@orowith2os
Copy link
Author

Then I'll keep it as OsString, but I won't implement the Windows bits. I'll leave that to someone willing to pick it up.

@joboet
Copy link
Member

joboet commented Jan 24, 2025

@orowith2os I've opened orowith2os#1 on your fork and implemented the Windows bit there.

…code resilient against bad platform behaviour
@rustbot rustbot added the O-unix Operating system: Unix-like label Jan 24, 2025
@joboet
Copy link
Member

joboet commented Jan 24, 2025

I shouldn't review my own code, so
r? @ChrisDenton (you probably know best about the Windows stuff)

@joboet joboet assigned ChrisDenton and unassigned joboet Jan 24, 2025
Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

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

The code looks fine. Note though that Windows 7 doesn't have GetHostNameW so it'd need. a fallback, However, it's also tier 3 so can be left to a follow up by the target maintainer.

// always enough.
let mut buffer = [0; 256];
// SAFETY: these parameters specify a valid, writable region of memory.
let ret = unsafe { c::GetHostNameW(buffer.as_mut_ptr(), buffer.len() as i32) };
Copy link
Member

Choose a reason for hiding this comment

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

I guess my one nitpick is that maybe the max buffer len could be a constant instead of using as i32, which is often a code smell but in this case it's narrowly scoped enough that I don't feel strongly about it.

@valaphee
Copy link

valaphee commented Jan 27, 2025

Didn't know that there is a dedicated GetHostName function in WSA, would have expected GetComputerNameExW. Implemented since Windows 2000, and doesn't require WS2 init.

joboet added a commit to joboet/rust that referenced this pull request Feb 2, 2025
As per rust-lang#117276, this PR moves `sys_common::net` and the `sys::pal::net` into the newly created `sys::net` module. In order to support rust-lang#135141, I've moved all the current network code into a separate `connection` module, future functions like `hostname` can live in separate modules.

I'll probably do a follow-up PR and clean up some of the actual code, this is mostly just a reorganization.
joboet added a commit to joboet/rust that referenced this pull request Feb 2, 2025
As per rust-lang#117276, this PR moves `sys_common::net` and the `sys::pal::net` into the newly created `sys::net` module. In order to support rust-lang#135141, I've moved all the current network code into a separate `connection` module, future functions like `hostname` can live in separate modules.

I'll probably do a follow-up PR and clean up some of the actual code, this is mostly just a reorganization.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-run-make Area: port run-make Makefiles to rmake.rs O-unix Operating system: Unix-like O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add net::hostname to retrieve the devices host name