pthread_setname_np/pthread_getname_np wrappers. #1539
Conversation
devnexen
force-pushed
the
pthread_ids
branch
3 times, most recently
from
62e3bd2
to
daa7a93
Compare
linux gnu support only for now.
devnexen
force-pushed
the
pthread_ids
branch
from
daa7a93
to
8a36554
Compare
| /// Obtain the name of the thread | ||
| /// | ||
| /// On linux the name cannot exceed 16 length including null terminator | ||
| #[cfg(all(target_os = "linux", not(target_env = "musl")))] |
There was a problem hiding this comment.
NetBSD and FreeBSD also support these functions. You should include them, though first you'll have to add FreeBSD's to libc.
There was a problem hiding this comment.
I know but I wanted to see first if it would appeal ;-) I added missing libc calls earlier anyway.
src/sys/pthread.rs
Outdated
| /// On linux the name cannot exceed 16 length including null terminator | ||
| #[cfg(all(target_os = "linux", not(target_env = "musl")))] | ||
| pub fn pthread_getname_np(thread: Pthread) -> Result<String> { | ||
| let mut name = [0u8; 16]; |
There was a problem hiding this comment.
The 16 bytes thing is Linux specific. On NetBSD it's controlled by the PTHREAD_MAX_NAMELEN_NP constant, currently 32 bytes. On FreeBSD it's not clearly documented, but the limit is MAXCOMLEN + 1, which is 20.
There was a problem hiding this comment.
very true but as mentionned in the commit message I focus on Linux for now.
src/sys/pthread.rs
Outdated
| /// Set the name of the thread | ||
| #[cfg(all(target_os = "linux", not(target_env = "musl")))] | ||
| pub fn pthread_setname_np(thread: Pthread, name: String) { | ||
| let cname = CString::new(name).expect("name failed"); |
There was a problem hiding this comment.
Better to return an error here instead of panic.
src/sys/pthread.rs
Outdated
|
|
||
| /// Set the name of the thread | ||
| #[cfg(all(target_os = "linux", not(target_env = "musl")))] | ||
| pub fn pthread_setname_np(thread: Pthread, name: String) { |
There was a problem hiding this comment.
You could be a little more generic here and accept any T: Into<Vec<u8>>. That way the caller could provide, for example, a byte string b"nix-rust".
src/sys/pthread.rs
Outdated
| /// | ||
| /// On linux the name cannot exceed 16 length including null terminator | ||
| #[cfg(all(target_os = "linux", not(target_env = "musl")))] | ||
| pub fn pthread_getname_np(thread: Pthread) -> Result<String> { |
There was a problem hiding this comment.
I suggesting returning CString and letting the caller convert it if he wants to.
|
before moving further (libc had been updated since) what do you think ? |
src/sys/pthread.rs
Outdated
| let mut name = [0u8; 16]; | ||
| pub fn pthread_getname_np(thread: Pthread) -> Result<CString> { | ||
| let mut name: Vec<u8> = Vec::with_capacity(16); | ||
| unsafe { name.set_len(16); } |
There was a problem hiding this comment.
This is technically UB. You shouldn't call set_len until after the the FFI function returns successfully, and you should only set it to as many bytes as the FFI function actually wrote. In this case, pthread_getname_np doesn't actually return the length of the name, so you'll have to fully initialize the buffer anyway.
| unsafe { libc::pthread_getname_np(thread, name.as_mut_ptr() as _, name.len()) }; | ||
| let cname = unsafe { CStr::from_ptr(name.as_ptr() as _) }; | ||
| Ok(cname.to_owned().to_string_lossy().to_string()) | ||
| let zi = name.iter().position(|x| *x == b'\0').unwrap(); |
There was a problem hiding this comment.
This three lines can be simplified. If you preinitialize the buffer to zero, then all you'll have to do is CString::new(name).
src/sys/pthread.rs
Outdated
| let res = unsafe { libc::pthread_setname_np(thread, nameptr) }; | ||
| Errno::result(res).map(drop) | ||
| }, | ||
| Err(_) => Ok(()), |
There was a problem hiding this comment.
Don't blindly ignore errors. You should return an error in this case.
| Ok(unsafe { CString::from_vec_unchecked(name) }) | ||
| } | ||
|
|
||
| /// Set the name of the thread |
There was a problem hiding this comment.
A doc test would be good here, especially because it's not obvious what the type of the argument should be.
| #[cfg(all(target_os = "linux", not(target_env = "musl")))] | ||
| fn test_pthread_name_np() { | ||
| let tid = pthread_self(); | ||
| let name = b"nix-rust"; |
There was a problem hiding this comment.
The fact that it takes three lines to put name into the right format suggests that the function's interface isn't well-designed. The goal is to be able to call it in any convenient way, like this:
pthread_setname_np(tid, "foo"); // Rust string
pthread_setname_np(tid, b"foo"); // Byte vector
pthread_setname_np(tid, CString::new("foo")); // C String
test/sys/test_pthread.rs
Outdated
| let tid = pthread_self(); | ||
| let name = b"nix-rust"; | ||
| let namestr = std::str::from_utf8(name).unwrap(); | ||
| pthread_setname_np(tid, name.to_vec()).expect("Should pass"); |
There was a problem hiding this comment.
expect should only be used in combination with a useful message. If you're just going to say "Should pass", then you might as well just use unwrap().
linux support only for now.