Gethostname sethostname updates #455
Conversation
| /// On some systems, the host name is limited to as few as 64 bytes. An error | ||
| /// will be return if the name is not valid or the current process does not have | ||
| /// permissions to update the host name. | ||
| pub fn sethostname(name: &str) -> Result<()> { |
There was a problem hiding this comment.
I'm not convinced &str is the right thing here. From your commit message:
Although the underlying C API does take a pointer to a set of characters, it is a requirement of almost every operating system that these bytes not contain a premature NUL character or other
special characters. In other words, you want a&str. Changing this to make the API make a bit more sense.
&str can contain a NUL byte anywhere, and is UTF-8, which may contain "special characters". Not really sure what to do here though!
PS commit message subject says gethostname when it should be sethostname.
There was a problem hiding this comment.
&str can contain a NUL byte anywhere, and is UTF-8, which may contain "special characters". Not really sure what to do here though!
That's kind of where I landed as well. Thinking about this more, I'm wondering what you think about the following:
pub fn sethostname<S: AsRef<OsStr>>(name: S) -> Result<()> {This would allow a greater variety of types (including strings) to work with the function and communicates a little more clearly that we are basically going to let the OS do validation.
There was a problem hiding this comment.
I think I prefer it. @fiveop thoughts?
| pub fn gethostname(name: &mut [u8]) -> Result<()> { | ||
| let ptr = name.as_mut_ptr() as *mut c_char; | ||
| let len = name.len() as size_t; | ||
| /// Get the host name and store it int the provided buffer, returning the actual |
|
|
||
| let res = unsafe { libc::gethostname(ptr, len) }; | ||
| Errno::result(res).map(drop) | ||
| Errno::result(res).map(move |_| { |
There was a problem hiding this comment.
I don't think so; I'll remove it. Not sure what I was thinking.
| let ptr = name.as_mut_ptr() as *mut c_char; | ||
| let len = name.len() as size_t; | ||
| /// Get the host name and store it int the provided buffer, returning the actual | ||
| /// length stored in the buffer (see |
There was a problem hiding this comment.
the return value is a &CStr not any length?
There was a problem hiding this comment.
Yeah, I need to update this doc. When I was first writing this I though the system call returned the length but it does not.
|
☔ The latest upstream changes (presumably #460) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Any update on this? I'm interested in having a more idiomatic interface for hostname handling exposed through nix. |
Sorry for the delay. I kinda forgot I had opened this awhile back. I'll update it and we'll see about getting things merged. |
posborne
force-pushed
the
gethostname-sethostname-updates
branch
2 times, most recently
from
cd055e7 to
2d4c44a
Compare
Although the underlying C API does take a pointer to a set of characters, it is a requirement of almost every operating system that these bytes not contain a premature NUL character or other special characters. In other words, you want a `&str`. Changing this to make the API make a bit more sense. Signed-off-by: Paul Osborne <osbpau@gmail.com>
Previously gethostname just mutated a buffer. We now provide a slightly more usable (but still allocation free) API that ensures that the returned buffer is NUL-terminated. We give back a `&CStr` instead of requiring that the user do all of the conversions from `&[u8]` when we know we are dealing with a `&CStr`. Signed-off-by: Paul Osborne <osbpau@gmail.com>
posborne
force-pushed
the
gethostname-sethostname-updates
branch
from
2d4c44a to
3491234
Compare
Baed on discussions on the related PR, sethostname now takes an `S: AsRef<OsStr>` in order to allow for a greater range of inputs that allow for a more fluid interface. Signed-off-by: Paul Osborne <osbpau@gmail.com>
|
Ok, @kamalmarhubi @fiveop I believe all comments on this one are addressed now (and I've remembered that this PR exists). Ready to go? |
|
@homu r=kamalmarhubi :) |
|
📌 Commit 2c02ee9 has been approved by |
|
⚡ Test exempted - status |
…lmarhubi Gethostname sethostname updates These changes have been previously discussed some with #452 but we decided it made sense to separate things out a bit more. r? @kamalmarhubi @fiveop
These changes have been previously discussed some with #452 but we decided it made sense to separate things out a bit more.
r? @kamalmarhubi @fiveop