Skip to content

unistd: add {get,set}domainname #816

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

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

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Dec 10, 2017

This implements getdomainname(2) and setdomainname(2), which are
common UNIX extensions not covered by POSIX. References:

src/unistd.rs Outdated
/// and store it in a provided buffer. The buffer will be populated with bytes up
/// to the length of the provided slice including a NUL terminating byte.
#[cfg(target_os = "linux")]
pub fn getdomainname<'a>(buffer: &'a mut [u8]) -> Result<&'a CStr> {
Copy link
Contributor Author

@lucab lucab Dec 10, 2017

Choose a reason for hiding this comment

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

I'm not super-happy about the ergonomics of this signature, but I just followed gethostname example. Can we rework these to allocate internally and return an owning CString?

Copy link
Member

Choose a reason for hiding this comment

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

I agree. I too would rather allocate internally.

src/unistd.rs Outdated
@@ -756,6 +756,36 @@ pub fn gethostname<'a>(buffer: &'a mut [u8]) -> Result<&'a CStr> {
})
}

/// Set the NIS domain name (see
/// [setdomainname(2)](http://man7.org/linux/man-pages/man2/setdomainname.2.html)).
#[cfg(target_os = "linux")]
Copy link
Member

Choose a reason for hiding this comment

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

Both of these functions are available on FreeBSD too. And while I haven't checked, I would bet that they're available on all of the BSDs based on their age.

Copy link
Contributor

Choose a reason for hiding this comment

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

They're also declared on emscripten, so I'd add both on those platforms as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a quick check and indeed they seem to be available on all BSD (plus ES) too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like libc does not expose them, so rust-lang/libc#868 needs to be fixed first.

lucab added a commit to lucab/nix that referenced this pull request Dec 11, 2017
This changes `gethostname` signature to be more idiomatic, by
internally allocating the buffer for result.

Ref: nix-rust#816 (comment)
@lucab
Copy link
Contributor Author

lucab commented Dec 11, 2017

Rebased, PTAL. In this iteration:

@asomers
Copy link
Member

asomers commented Dec 11, 2017

libc's maintainers probably aren't going to add the definitions themselves, even for reported issues. Like Nix, they mostly rely on pull requests from their users. Could you please open a PR for libc?

@lucab
Copy link
Contributor Author

lucab commented Jan 18, 2018

This got stale due to lack of time on my side, but I'll eventually get back to this (just not high priority at the moment). In the meanwhile, @gnzlbg is pushing forward the BSD libc fix, kudos to them.

lucab added a commit to lucab/nix that referenced this pull request Jan 27, 2018

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
This changes `gethostname` signature to be more idiomatic, by
internally allocating the buffer for result.

Ref: nix-rust#816 (comment)
@lucab lucab force-pushed the ups/domainname branch 4 times, most recently from 8dfbb87 to 91aa28e Compare January 27, 2018 12:19
@lucab
Copy link
Contributor Author

lucab commented Jan 27, 2018

Rebased to take care of BSD and Apple, PTAL.

src/unistd.rs Outdated
let ptr = buffer.as_mut_ptr() as *mut c_char;
let len = buffer.len() as size_t;
pub fn gethostname() -> Result<CString> {
const MAXHOSTNAMELEN: usize = 256;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should document why we use the value we use here. Is there some reference example you pulled this from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is _POSIX_HOST_NAME_MAX plus the trailing null, from http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html.

I'll rename, add a reference, and leave the trailing byte out of this.

src/unistd.rs Outdated
unsafe { CStr::from_ptr(buffer.as_ptr() as *const c_char) }
unsafe {
// Ensure returned string is always null-terminated
*ptr.offset((MAXHOSTNAMELEN - 1) as isize) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't you instead use a MAXHOSTNAMELEN of 255 here and be able to skip this step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's probably better, I'll do.

lucab added a commit to lucab/nix that referenced this pull request Jan 29, 2018
This changes `gethostname` signature to be more idiomatic, by
internally allocating the buffer for result.

Ref: nix-rust#816 (comment)
@lucab
Copy link
Contributor Author

lucab commented Jan 29, 2018

@Susurrus I adjusted and documented POSIX_HOST_NAME_MAX as suggested, PTAL.

lucab added a commit to lucab/nix that referenced this pull request Feb 2, 2018
This changes `gethostname` signature to be more idiomatic, by
internally allocating the buffer for result.

Ref: nix-rust#816 (comment)
@lucab
Copy link
Contributor Author

lucab commented Feb 2, 2018

Gentle ping for a review.

src/unistd.rs Outdated
pub fn gethostname() -> Result<CString> {
// Hostname maximum length as defined by POSIX,
// http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html
const POSIX_HOST_NAME_MAX: usize = 255;
Copy link
Contributor

Choose a reason for hiding this comment

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

According to this link the max host name length is 64 on Linux, and that's confirmed on my system. _POSIX_HOST_NAME_MAX on my system is 255, however. We should figure out the appropriate values here and get them into libc to use them here.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for putting this into libc.

src/unistd.rs Outdated
///
/// # Examples
///
/// ```no_run
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this not run in CI?

Copy link
Member

Choose a reason for hiding this comment

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

Why use a CString here but an OsStr for setdomainname? We should be consistent in our choice of C vs OS strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asomers This follows the split currently in master for the hostname, see gethostname vs sethostname types. Perhaps it can be left out of this PR? Otherwise, in which direction do you want to resolve this?

Copy link
Member

Choose a reason for hiding this comment

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

Most Nix consumers will probably be using ordinary Rust Strings, and converting between String and OsString is easier than converting between String and CString. So I think that our API should prefer to expose OsString/OsStr.

src/unistd.rs Outdated

/// Set the NIS domain name (see
/// [setdomainname(2)](http://man7.org/linux/man-pages/man2/setdomainname.2.html)).
#[cfg(not(target_os = "android"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer to use whitelists instead of blacklists for listing targets. Please do so here and for getdomainname.

src/unistd.rs Outdated
Errno::result(res).map(|_| {
unsafe {
// Ensure returned string is always null-terminated
*ptr.offset(POSIX_HOST_NAME_MAX as isize) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need to do this because gethostname won't overrun the buffer. Same for the other functions.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, according to the man page the result is not guaranteed to be null-terminated if the buffer was insufficiently large. So it's a good idea to manually null terminate it.

#[cfg(not(target_os = "android"))]
fn test_getdomainname() {
let dn = getdomainname();
assert!(dn.is_ok(), "Error: {:?}", dn.unwrap_err());
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace this with dn.expect("") on the above line.

#[test]
fn test_gethostname() {
let hn = gethostname();
assert!(hn.is_ok(), "Error: {:?}", hn.unwrap_err());
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace this with hn.expect("") on the above line.

CHANGELOG.md Outdated
@@ -6,10 +6,14 @@ This project adheres to [Semantic Versioning](http://semver.org/).
## [Unreleased]

### Added
- Added `nix::unistd::{getdomainname, setdomainname}`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please specify which platforms this applies to.

src/unistd.rs Outdated
pub fn gethostname() -> Result<CString> {
// Hostname maximum length as defined by POSIX,
// http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html
const POSIX_HOST_NAME_MAX: usize = 255;
Copy link
Member

Choose a reason for hiding this comment

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

+1 for putting this into libc.

src/unistd.rs Outdated
Errno::result(res).map(|_| {
unsafe {
// Ensure returned string is always null-terminated
*ptr.offset(POSIX_HOST_NAME_MAX as isize) = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Actually, according to the man page the result is not guaranteed to be null-terminated if the buffer was insufficiently large. So it's a good idea to manually null terminate it.

src/unistd.rs Outdated
*ptr.offset(POSIX_HOST_NAME_MAX as isize) = 0;
CString::from_raw(ptr)
}
})
Copy link
Member

Choose a reason for hiding this comment

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

If libc::gethostname returns ENAMETOOLONG, then it's nix's fault and there's nothing that our consumer can do about it. We should therefore panic! on ENAMETOOLONG.

Copy link
Contributor

Choose a reason for hiding this comment

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

@asomers Does this comment still stand or is it not relevant anymore?

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately GitHub has forgotten the context of that comment. But I think my objection was because he tried to handle that error condition and didn't do it correctly. Now, he isn't handling any error conditions, which is better.

src/unistd.rs Outdated
#[cfg(not(target_os = "android"))]
pub fn setdomainname<S: AsRef<OsStr>>(name: S) -> Result<()> {
let ptr = name.as_ref().as_bytes().as_ptr() as *const c_char;
let len = name.as_ref().len() as namelen_t;
Copy link
Member

Choose a reason for hiding this comment

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

I think it would probably be more pedantically correct to do len = name.as_ref().as_bytes().len().

src/unistd.rs Outdated
///
/// # Examples
///
/// ```no_run
Copy link
Member

Choose a reason for hiding this comment

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

Why use a CString here but an OsStr for setdomainname? We should be consistent in our choice of C vs OS strings.

src/unistd.rs Outdated
/// ```
#[cfg(not(target_os = "android"))]
pub fn getdomainname() -> Result<CString> {
const MAXDOMAINNAMELEN: usize = 256;
Copy link
Member

Choose a reason for hiding this comment

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

According to the man page, MAXHOSTNAMELEN applies to the domain name, too.

@Susurrus
Copy link
Contributor

@lucab Are you planning on coming back to this?

@lucab
Copy link
Contributor Author

lucab commented Feb 21, 2018

@Susurrus yes but I need to figure out the story with HOST_NAME_MAX Linux and POSIX, and forward to libc.

@Susurrus
Copy link
Contributor

Susurrus commented Apr 7, 2018

@lucab Did the HOST_NAME_MAX get sorted yet?

@lucab
Copy link
Contributor Author

lucab commented May 16, 2018

So, the full story is as follows.

Host names returned by gethostname() are limited to HOST_NAME_MAX bytes (not including the terminating null), which is defined by each OS.
According to POSIX limits, the minimum acceptable value should be _POSIX_HOST_NAME_MAX which is equal to 255.
However it looks like glibc is not POSIX compliant here, and defines HOST_NAME_MAX as 64. Instead, musl took a different decision and set it to 255.
But, the Linux kernel implementation effectively limits it to 64.
Additionally, it looks like some systems (freebsd, macos) try to avoid fixing HOST_NAME_MAX, using sysconf(_SC_HOST_NAME_MAX) instead. This latter seems to be indeed the correct POSIX way.

@Susurrus @asomers what would you like to do out of this? I'm inclined to just switch to using sysconf everywhere and falling back to _POSIX_HOST_NAME_MAX if that fails, as it seems reasonably clean and minimize the allocation in the happy case, without pushing this mess forward in libc.

@asomers
Copy link
Member

asomers commented May 16, 2018

Yes, sysconf sounds like the safest approach.

lucab added a commit to lucab/nix that referenced this pull request May 21, 2018
This changes `gethostname` signature to be more idiomatic, by
internally allocating the buffer for result.

Ref: nix-rust#816 (comment)
@lucab lucab force-pushed the ups/domainname branch 2 times, most recently from b13c577 to d904bce Compare May 21, 2018 14:08
lucab added a commit to lucab/nix that referenced this pull request May 21, 2018
This changes `gethostname` signature to be more idiomatic, by
internally allocating the buffer for result.

Ref: nix-rust#816 (comment)
@lucab lucab force-pushed the ups/domainname branch 3 times, most recently from 85e119a to 09ff9d5 Compare May 22, 2018 11:48
lucab added a commit to lucab/nix that referenced this pull request May 22, 2018
This changes `gethostname` signature to be more idiomatic, by
internally allocating the buffer for result.

Ref: nix-rust#816 (comment)
@lucab
Copy link
Contributor Author

lucab commented May 22, 2018

Travis is red due to android missing POSIX constants in libc crate, see rust-lang/libc#990.

@lucab lucab force-pushed the ups/domainname branch from 09ff9d5 to 13f25a5 Compare May 22, 2018 17:49
lucab added a commit to lucab/nix that referenced this pull request May 22, 2018
This changes `gethostname` signature to be more idiomatic, by
internally allocating the buffer for result.

Ref: nix-rust#816 (comment)
@lucab
Copy link
Contributor Author

lucab commented May 22, 2018

Rebased, this is now green. I think I've addressed all comments in reviews so far, but at this point this PR is quite different from its initial version. PTAL.

src/unistd.rs Outdated
let buf = vec![0; buf_len as usize];
let ptr = unsafe { CString::from_vec_unchecked(buf).into_raw() };

let res = unsafe { libc::gethostname(ptr, buf_len as usize) };
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to convert the buffer to a CString first. You can just do libc::gethostname(buf.as_mut() as *mut c_char, buf_len as usize

src/unistd.rs Outdated
let cstr = unsafe {
// Ensure CString is null-terminated
*ptr.offset((buf_len - 1) as isize) = 0;
CString::from_raw(ptr)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think CString::from_raw is necessary. You can use OsStringExt::from_vec(buf) directly.

#[cfg(any(target_os = "dragonfly", target_os = "freebsd", target_os = "ios",
target_os = "linux", target_os = "macos", target_os = "netbsd",
target_os = "openbsd"))]
pub fn getdomainname() -> Result<OsString> {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto my comments about CString as in gethostname

@lucab lucab force-pushed the ups/domainname branch from 13f25a5 to befac8e Compare May 25, 2018 14:10
lucab added a commit to lucab/nix that referenced this pull request May 25, 2018
This changes `gethostname` signature to be more idiomatic, by
internally allocating the buffer for result.

Ref: nix-rust#816 (comment)
@lucab
Copy link
Contributor Author

lucab commented May 25, 2018

@asomers I was a bit unsure about the safety of your comments so I added some idempotency checks to the tests. You are indeed right, so I pushed the rework and the additional testing logic. Everything is green, PTAL.

@asomers
Copy link
Member

asomers commented May 25, 2018

Ok, everything looks good to me! It only needs a squash. @Susurrus do you have any comments?

lucab and others added 2 commits May 27, 2018 08:32
This changes `gethostname` signature to be more idiomatic, by
internally allocating the buffer for result.

Ref: nix-rust#816 (comment)
This implements `getdomainname(2)` and `setdomainname(2)`, which are
common UNIX extensions not covered by POSIX. References:
 * http://man7.org/linux/man-pages/man2/getdomainname.2.html
 * http://man7.org/linux/man-pages/man2/setdomainname.2.html
@lucab lucab force-pushed the ups/domainname branch from befac8e to 6d80f37 Compare May 27, 2018 08:32
@lucab
Copy link
Contributor Author

lucab commented May 27, 2018

@asomers ack. I squashed one commit out. I'm keeping two commits for the two separate changelog entries. Let me know if you prefer everything together.

Copy link
Contributor

@Susurrus Susurrus left a comment

Choose a reason for hiding this comment

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

The commit split looks good to me, but a few minor things should get cleaned up still.

/// # Examples
///
/// ```
/// use nix::unistd;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please prefix this line with a # so they don't render in the final help. You can then also delete the blank line below.

/// ```
/// use nix::unistd;
///
/// let domainname_os = unistd::getdomainname().expect("Failed getting domain name");
Copy link
Contributor

Choose a reason for hiding this comment

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

You can reuse the name domainname here because of Rust's variable shadowing. Doing this is idiomatic. Please rename this variable to domainname.

pub fn getdomainname() -> Result<OsString> {
// Minimum hostname maximum length as defined by POSIX,
// http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html
const _POSIX_HOST_NAME_MAX: c_long = 255;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't this constant pulled from libc? I didn't see anywhere in the discussion a comment touching on this.

src/unistd.rs Outdated
*ptr.offset(POSIX_HOST_NAME_MAX as isize) = 0;
CString::from_raw(ptr)
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

@asomers Does this comment still stand or is it not relevant anymore?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants