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

implement ttyname and ttyname_r #1259

Merged
merged 1 commit into from
Jul 3, 2020
Merged

implement ttyname and ttyname_r #1259

merged 1 commit into from
Jul 3, 2020

Conversation

doy
Copy link
Contributor

@doy doy commented Jun 16, 2020

this fixes #1204.

i know this at least works on linux, but i'm not super sure about other platforms - happy to add conditionals if it makes sense.

@doy doy force-pushed the ttyname branch 8 times, most recently from 8133e94 to ad026a1 Compare June 19, 2020 05:35
@doy
Copy link
Contributor Author

doy commented Jun 27, 2020

(i'd really like to see this change, but i don't have access to a dev machine with osx - is there a way forward here?)

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Apart from the inline comments, this change looks good. As for the OSX test failure, I can't help you because I don't have an OSX machine, either. Try to borrow a friend's if possible. If not, then you might try "debugging-through-travis" at least to see the exact errno, and the output of isatty(fd.as_raw_fd()).

Finally, if all else fails, my belief is that proprietary operating systems are supported on a "best-effort" basis, and you can disable the function on OSX.

src/unistd.rs Outdated
/// function is marked as `unsafe` to reflect that.
///
/// For a threadsafe and non-`unsafe` alternative, see `ttyname_r()`.
#[inline]
Copy link
Member

Choose a reason for hiding this comment

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

Don't use #[inline] for new code. The compiler can figure it out, at least when LTO is enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

return Err(Error::Sys(Errno::last()));
}

let nul = buf.iter().position(|c| *c == b'\0').unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Instead of manually searching for NUL, why not just use CStr::from_ptr? In fact, you should technically be using OsString here instead of CString, because the string comes from the operating system and not from another programming language. So you should declare the buf as a Vec, and then use OsString::from_vec followed by to_string_lossy.

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 think returning a PathBuf is actually better than String here, and using OsString for that makes sense. that does mean that i do have to still search for the nul myself though, since OsString won't do that (it'll just create a string with a bunch of trailing nuls). going through CStr (and then being forced to allocate a copy into a second Vec) doesn't seem worth it to avoid that?

src/unistd.rs Outdated
///
/// For a threadsafe and non-`unsafe` alternative, see `ttyname_r()`.
#[inline]
pub unsafe fn ttyname(fd: RawFd) -> Result<String> {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a good reason why anybody would ever want ttyname? Since the invention of ttyname_r, I think the latter should be used in all cases. If you don't know of a use case where ttyname_r does not suffice, then I'd just as soon we don't bind ttyname at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, good point - i was mainly copying from the ptsname interface, but i agree that it's really not necessary.

@doy
Copy link
Contributor Author

doy commented Jun 30, 2020

alright, i'll play around with travis some more.

@doy doy force-pushed the ttyname branch 15 times, most recently from f3df1e6 to 5a2107c Compare July 2, 2020 06:27
@doy doy requested a review from asomers July 2, 2020 06:35
@doy
Copy link
Contributor Author

doy commented Jul 2, 2020

looks like it was just an issue with the way i was structuring the test - ttyname seems to fail on osx when called on a pty master for some reason, but that's basically never a useful thing to do, so it's probably fine to just test something more along the lines of something people would actually use this for. there are still a couple failing builds, but it looks like every pull request for the past few weeks has been failing those builds, so i don't think they are related.

@asomers
Copy link
Member

asomers commented Jul 3, 2020

The powerpc64 build has only been failing for a few days, and it's fixed on Nix's master branch. You'll have to rebase.

@doy
Copy link
Contributor Author

doy commented Jul 3, 2020

done

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 3, 2020

Merge conflict.

@doy
Copy link
Contributor Author

doy commented Jul 3, 2020

rebased again

@doy
Copy link
Contributor Author

doy commented Jul 3, 2020

test_aio_drop also seems flaky https://travis-ci.org/github/nix-rust/nix/jobs/704771690

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 3, 2020

Build succeeded:

@bors bors bot merged commit a2f40c2 into nix-rust:master Jul 3, 2020
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.

Support for ttyname and ttyname_r
2 participants