add ptsname_r shim for macos #4
Conversation
on macos provided bash, by default it outputs; The default interactive shell is now zsh. To update your account to use zsh, please run `chsh -s /bin/zsh`.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
ethanpailes
left a comment
ethanpailes
left a comment
There was a problem hiding this comment.
Thank you so much for this patch!
Overall, it looks great, I mostly just have feedback about getting the change to follow project standards when it comes to saftey.
| if let Some(fd) = self.pty { | ||
| // Safety: the vector's memory is valid for the duration | ||
| // of the call | ||
| unsafe { |
There was a problem hiding this comment.
I'd like to maintain the style that every unsafe block has a saftey comment above it.
There was a problem hiding this comment.
I mistakenly moved this one, updated; 559aa75
| //! Based on: https://tarq.net/posts/ptsname-on-osx-with-rust/ | ||
| #[cfg(any(target_os = "macos"))] | ||
| pub unsafe fn ptsname_r( |
There was a problem hiding this comment.
Please add a "Saftey" comment that explains the formal precondtions that any call site must meet in order to use this function correctly (should be pretty easy, just stuff like "buf needs to point to allocated memory" and "buflen can't be longer than the allocation" and "fd must be an open file descriptor").
| return *libc::__error(); | ||
| } | ||
|
|
||
| let mut len = 0; |
There was a problem hiding this comment.
Totally fine to leave it like this, but for this sort of null-seaking, memchr (either the pure rust crate https://crates.io/crates/memchr, or the libc function https://docs.rs/libc/0.2.177/libc/fn.memchr.html) will do this a lot faster because they take advantage of SIMD hardware acceleration. It definitely won't matter for performance since the strings are so small here, so this is mostly just me being nerd sniped.
There was a problem hiding this comment.
TIL thank you. To not bring a new dependency, went for the libc one; 3bca717
| return libc::ERANGE; | ||
| } | ||
|
|
||
| libc::memcpy( |
There was a problem hiding this comment.
Let's use std::ptr::copy (which is how you pronounce libc::memmove in rust) here. It is safer because it can handle overlapping ranges, and on modern branch predicting CPUs the extra branch it requires is undetectable in microbenchmarks.
| let master_fd = unsafe { libc::posix_openpt(libc::O_RDWR | libc::O_NOCTTY) }; | ||
| assert!(master_fd >= 0, "Failed to open master PTY"); | ||
|
|
||
| unsafe { |
There was a problem hiding this comment.
Please add a Saftey comment. I know it's just a test, but having them on every block makes auditing easier.
There was a problem hiding this comment.
Updated each unsafe section in the tests, not sure about the wording though. How all these sounds like? 559aa75
|
|
||
| let mut buf = [0u8; 2]; | ||
| let result = | ||
| unsafe { ptsname_r(master_fd, buf.as_mut_ptr() as *mut libc::c_char, buf.len()) }; |
| #[test] | ||
| fn test_ptsname_r_invalid_fd() { | ||
| let mut buf = vec![0u8; 1024]; | ||
| let result = unsafe { ptsname_r(-1, buf.as_mut_ptr() as *mut libc::c_char, buf.len()) }; |
| #[cfg(any(target_os = "macos"))] | ||
| #[test] | ||
| fn test_ptsname_r_null_buffer() { | ||
| let master_fd = unsafe { libc::posix_openpt(libc::O_RDWR | libc::O_NOCTTY) }; |
| let master_fd = unsafe { libc::posix_openpt(libc::O_RDWR | libc::O_NOCTTY) }; | ||
|
|
||
| if master_fd >= 0 { | ||
| unsafe { |
| let result = unsafe { ptsname_r(master_fd, std::ptr::null_mut(), 1024) }; | ||
|
|
||
| unsafe { | ||
| libc::close(master_fd); |
|
Looks great. Thanks! Looks like there is a bit of lint bitrot (https://github.com/shell-pool/shpool_pty/actions/runs/19173539720/job/54819707170?pr=4) that needs fixing and then we can merge. |
|
If you don't have access to that lint output for some reason, you should be able to repro it by running |
They all should be fixed now. |
|
Not sure why that test is spinning. At this point is seems like it is going to time out. |
I think I broke something. On macOS tests pass but when I try to run on a Linux vm it just hangs. |
|
I think you might want to abort the run. It seems to be due to replacing |
|
I'm kinda supprised it ran for over an hour without timing out. |
|
My guess is |
| let mut cmd = Command::new("bash"); | ||
| cmd.env_clear(); | ||
|
|
||
| // On macOS, silence the deprecation warning; | ||
| // https://github.com/apple-oss-distributions/bash/blob/e86b2aa8e37a31f8fce56366d1abaf08a3fac7d2/bash-3.2/shell.c#L760-L765 | ||
| #[cfg(target_os = "macos")] | ||
| { | ||
| cmd.env("BASH_SILENCE_DEPRECATION_WARNING", "1"); | ||
| } |
There was a problem hiding this comment.
Looks like /bin/sh is just bash in disguise on macOS. Would it be OK to stick to bash and just silence this annoying warning?
There was a problem hiding this comment.
Yeah, that seems fine to me.
|
Found my way here from shell-pool/shpool#48 Looks like most if not all the feedback has been taken up. Do you guys need any help testing this? It would be great if @seruman wouldn't have to maintain a fork for folks like me! |
| let mut cmd = Command::new("bash"); | ||
| cmd.env_clear(); | ||
|
|
||
| // On macOS, silence the deprecation warning; | ||
| // https://github.com/apple-oss-distributions/bash/blob/e86b2aa8e37a31f8fce56366d1abaf08a3fac7d2/bash-3.2/shell.c#L760-L765 | ||
| #[cfg(target_os = "macos")] | ||
| { | ||
| cmd.env("BASH_SILENCE_DEPRECATION_WARNING", "1"); | ||
| } |
There was a problem hiding this comment.
Yeah, that seems fine to me.
|
@seruman in the future, when you finish responding to a review, you can click the little recycle icon on the upper right of the PR page to notify the reviewer that you are done responding and request a re-review. 
|
|
@ethanpailes thanks for the heads up, sorry that i totally forgot this PR. |
No worries, I forgot about it too. Thank you for the patch! I'll cut a new release so we can pick up the change in the main shpool repo. |
|
I got a little too eager on merging it looks like. I posted #5 as a followup. |
|
@seruman I just cut and pushed 0.3.2 to crates.io (https://crates.io/crates/shpool_pty) with this change. If you have a macos port for the main shpool repo that you want to merge in I would be happy to help you with that. |
thank you, yes i do. i'm a little caught up though, it might take some time before getting to it. |
|
Awesome! No rush |
3 participants
Related to; shell-pool/shpool#183
I was not able to build shell-pool/shpool;
With a quick search I found these references:
This PR adds a small macOS-only shim so that from the caller’s perspective, it still looks like
ibc::ptsname_r, to make as little as change possible.I have little to no experience in Rust or systems programming, so this change
may not be idiomatic or does not even make sense in the first place so feel
free to update/feedback or even discard.
I've been using it on on my shpool fork for couple some time just fine.
Disclaimer; got help from LLM tools, especially for tests.
Had to change tests to use
shinstead ofbashfor portability as on my version of macOS -26.0.1-, Apple providedbashgreets you with;And seems like it is baked in.