Add various pty functions #556

Susurrus · 2017-03-15T17:28:25Z

grantpt
posix_openpt
ptsname/ptsname_r
unlockpt

I've refactored my code to use these functions and they work correctly at least in the non-error case. Only issue is what to do for failure in ptsname when converting strings. This should effectively never happen. I think maybe adding an Unknown to the Error type and returning that might be useful.

Mic92 · 2017-03-17T15:33:31Z

src/pty.rs

+pub fn ptsname(fd: RawFd) -> Result<String> {
+    let name_ptr = unsafe { libc::ptsname(fd) };
+    if name_ptr.is_null() {
+        return Err(Errno::ENOTTY.into());


Is Err(Error::last().into()) not safer here? I mean according to the manpage ENOTTY is the only error, but better safe then sorry.

Yeah, that's probably the correct thing to do, though I do think they'll end up with the same functionality. I'll change it.

Susurrus · 2017-03-17T15:49:32Z

@Mic92 Also do you have any comment on the two FIXMEs I have in there? I didn't see a way to return a generic unknown error.

Mic92 · 2017-03-17T16:13:15Z

src/pty.rs

+    }
+
+    let name_cstr = unsafe { CStr::from_ptr(name_ptr) };
+    let name = match name_cstr.to_str() {


It is probably safe to ignore utf8 validation errors as we get the string from the operating system:
see https://github.com/nix-rust/nix/blob/master/src/sys/utsname.rs#L65

If it is not safe, then it is better to return the value as it is.

Susurrus · 2017-03-17T17:50:42Z

@Mic92 I think I've resolved both issues. Let me know what you think. I left them as separate commits so they're easier to review, but I'll squash them before they're pulled.

Mic92 · 2017-03-17T19:34:23Z

src/pty.rs

    };
-    Ok(name)
+    Ok(name.to_string_lossy().into_owned())


....replacing any invalid UTF-8 sequences with U+FFFD REPLACEMENT CHARACTER...
Is the unmodified value of this function not meant to be as an input for functions like open(2)?

We should never receive invalid UTF-8 sequences from the OS, so that should never happen. I'd prefer to use from_utf8_unchecked (as you mentioned in your comment), which doesn't do any error detection, but I couldn't figure out how to use it. And note that using from_utf8_unchecked has the exact same problem, but instead of fixing invalid UTF-8 strings it will just crash later when it's attempted to be decoded as proper UTF-8.

Basically if we use the String type as the output from this function, it must be valid UTF-8 regardless of how we convert it. So the question really is, does ptsname*() always return valid UTF-8. I think the answer is yes, but I don't really know.

Just wanted to note that gethostname returns CStr. So to be consistent I would either change both to String or both to CStr.

Yeah, I would think that gethostname should return a Result<String> instead of returning it in an output argument as a CStr. @posborne Any comments here?

Susurrus · 2017-04-17T18:15:10Z

@nix-rust/nix-maintainers Any comments on the APIs I've exposed here. Unlike with gethostname I'm exposing a String type, though a &str might be more appropriate. I do think we should stop exposing CStr types in our APIs as I see nix as being the interface between Rust and libc and that conversion as part of it.

asomers

I think this module could benefit from some type-safety, and it would be nice to eliminate the extra data copy in ptsname_r

asomers · 2017-05-12T16:32:53Z

src/pty.rs

+/// `grantpt()` changezs the mode and owner of the slave pseudoterminal device corresponding to the
+/// master pseudoterminal referred to by `fd`. This is a necessary step towards opening the slave.
+#[inline]
+pub fn grantpt(fd: RawFd) -> Result<()> {


How about creating a Newtype like struct PseudoTerminal(RawFd) to provide some static type checking on these functions? It could also close itself on Drop.

Hmm, interesting. Would this be as simple as implementing that type and also impl From<RawFd> for it? We'd need to make sure that this can be easily used both with std and other nix functions. Additionally, is this sound in the context of #594?

I don't think it should implement From<RawFd>, because that would make it too easy to accidentally convert an inappropriate value. Conversion should always be explicit. In fact, are there any other Rust functions, in nix or in std, that deal with pseudoterminals?

Yeah, I agree, maybe just AsRawFd is a better trait instead? But no, nothing else really deals with this. I have some code in my serialport-rs library here that deals with this same thing, and that's like the only other thing working with safe ptty abstractions.

AsRawFd sounds good.

So in continuing to think about this, I don't think it makes sense to provide an abstraction over RawFd. The use case of these ttys is generally what you can see in serialport-rs. Most commonly you call read and write, both of which take a RawFd. Additionally, the other functions I'm adding here all take RawFd. So every call you'd be doing fn(port.as_raw_fd()), so it's just a needless abstraction that doesn't actually add any safety. I think it's the role of a library like serialport-rs to provide abstractions on top of nix should it be desired.

What if the newtype implemented Into<RawFd>? Then fn could automatically coerce it, as long as fn's argument is a T: Into<RawFd>

But if you implement Into you also implement From, that's what my comment was about before. It's also bidirectional, so we're back to where we started. And you'd still need to use .into() everywhere since not all functions use T: Into<RawFd>. Since the topic of changing function signatures came up, there are 168 functions in libc that have fd's as inputs (as c_int, not RawFd) and 88 functions in nix that have RawFd inputs.

Actually, From implies Into, but not the other way around. If this newtype implements Into<RawFd> , that does not automatically create an implementation for From<Newtype> on RawFd. Do you think that typing .into() is too much work to justify the free type checking on the pty functions? If the newtype implements Drop, that could save some typing too, because you wouldn't have to automatically close the ptys.

asomers · 2017-05-12T16:38:28Z

src/pty.rs

+    }
+
+    let name = unsafe {
+        CStr::from_ptr(name_buf.as_ptr())


The subsequent Cow::into_owned() will perform a data copy. How about something like this instead?

let mut name_buf = OsString::with_capacity(64); libc::ptsname(fd, name.to_str().as_ptr(), 64); return name_buf.into_string()

Two things: Is OsString the right type rather than CString? The docs don't clarify what the difference between the two really is, so I'm not certain.

Also, that code you provided doesn't compile. because name.to_str().as_ptr() doesn't return a mutable pointer and I don't see a way to get one. Additionally, I'm not certain how to map the error types. I'm thinking we should add to nix::Error a StringConversion type and map the into_string() failure to that, otherwise I think this should be an unwrap() or expect() since I don't think this will ever fail.

Take a look at getcwdin unistd.rs. It actually uses a Vec as a buffer, before transforming it into an OsString

Is that what you'd recommend doing instead of what you originally suggested?

My original suggestion was just a guess, a starting point. I didn't know exactly how to make it work. But getcwd presents exactly the same problem as this function, so its solution will probably work.

asomers · 2017-05-12T16:39:24Z

src/pty.rs

+    }
+
+    let name = unsafe {
+        CStr::from_ptr(name_ptr)


How about a test that calls ptsname on two separate pseudoterminals, and checks that their names are not the same? That would enforce that this function performs a data copy (as I think it probably should).

Wouldn't you instead want to call ptsname() on the same ptty and check that they don't point to the same underlying buffer? I'm unclear what the test you suggested actually tests.

Yeah, that could do it. I'm really just trying to think up ways to improve the coverage.

asomers · 2017-05-12T16:44:42Z

test/test_pty.rs

+/// Test equivalence of `ptsname` and `ptsname_r`
+#[test]
+#[cfg(any(target_os = "android", target_os = "linux"))]
+fn test_ptsname() {


The test name is not very accurate.

asomers · 2017-05-12T16:47:08Z

test/test_pty.rs

+    // Open the slave device
+    let slave_fd = open(Path::new(&slave_name), O_RDWR, stat::Mode::empty()).unwrap();
+    assert!(slave_fd > 0);
+}


This test leaks its file descriptors.

asomers · 2017-05-12T16:47:24Z

test/test_pty.rs

+    let slave_name = ptsname(master_fd).unwrap();
+    let slave_name_r = ptsname_r(master_fd).unwrap();
+    assert_eq!(slave_name, slave_name_r);
+}


This test leaks its file descriptors, too.

Susurrus · 2017-05-16T04:24:01Z

I'm still working on how to best return a String with the least number of data copies (gonna reach out on IRC tomorrow). But for now I've pushed an updated version that adds a datatype. I can use it in my serialport-rs lib pretty ergonomically.

One question I have is if errors when calling close on a PtyMaster fd should it panic or just silently ignore the error? The signalfd module does, but I don't know if that's how we should do things, though I'm doing the same thing here.

Susurrus · 2017-05-16T04:36:21Z

Here's the commit to serialport-rs that adjusts it for these changes in nix. In general I think it's pretty clean looking.

asomers · 2017-05-16T14:35:08Z

test/test_pty.rs

+#[test]
+#[cfg(any(target_os = "android", target_os = "linux"))]
+fn test_ptsname_equivalence() {
+    // Open a new PTTY master


s/PTTY/PTY here and below

asomers · 2017-05-16T14:36:12Z

test/test_pty.rs

+    let slave_name1 = ptsname(&master_fd).unwrap();
+    let slave_name2 = ptsname(&master_fd).unwrap();
+    assert!(slave_name1 == slave_name2);
+    assert!(slave_name1.as_ptr() != slave_name2.as_ptr());


This assertion probably deserves a comment. It's not obvious why a consumer should care about this behavior.

asomers · 2017-05-16T14:48:42Z

The new type looks nice, and it fixes all of the file descriptor leaks in the tests. The integration into serialport-rs looks good too.

I have conflicting feelings about errors during drop. On the one hand, the most likely error is an EBADF, which suggests a programming bug, so the user should be informed. OTOH, since PtyMaster implements AsRawFd, the library can't really assume that the user hasn't already closed it. So I suppose we should ignore errors. But that raises another question: Will PtyMaster.into_raw_fd() invoke drop? If so, then we're screwed. I don't think so, because std::net::TcpStream implements both traits. But it would be nice to verify that with a test.

Susurrus · 2017-05-16T18:32:25Z

Yes, we need to explicitly forget the type. This is actually done in std::net::TcpStream via the into_raw() call. Additionally we ignore any errors when closing for the same reason they do in std and I added as similar comment.

Alright, I think this really should be everything that we need.

asomers · 2017-05-16T19:34:34Z

Did you ever figure out a way to reduce the data copies in ptsname_r?

Susurrus · 2017-05-16T19:44:22Z

Whoops! I knew I was forgetting something. I'll hop on IRC again today and see if I can figure out the "right" way to do this.

Susurrus · 2017-05-16T21:46:57Z

Popped onto IRC and found a reasonable implementation that should be pretty efficient. It's still not clear if String is the right type to return, but at least this will error hard if there's every a problem rather than us assuming it's safe. It did require me expanding the Error type, but I think it's reasonable.

asomers

Looks like some debugging code accidentally made it into the PR

asomers · 2017-05-17T02:54:12Z

src/lib.rs

            Error::Sys(errno) => io::Error::from_raw_os_error(errno as i32),
+


I don't think you meant to leave a space here

asomers · 2017-05-17T02:54:31Z

src/pty.rs

+pub fn ptsname_r(fd: &PtyMaster) -> Result<String> {
+    let mut name_buf = vec![0u8; 64];
+    let name_buf_ptr = name_buf.as_mut_ptr() as *mut libc::c_char;
+    println!("{}/{}", name_buf.len(), name_buf.capacity());


Please remove this debugging code

asomers · 2017-05-17T02:54:46Z

src/pty.rs

+    if unsafe { libc::ptsname_r(fd.as_raw_fd(), name_buf_ptr, name_buf.capacity()) } != 0 {
+        return Err(Error::last().into());
+    }
+    println!("{}/{}", name_buf.len(), name_buf.capacity());


This one too.

* grantpt * ptsname/ptsname_r * posix_openpt * unlockpt

Susurrus · 2017-05-17T03:41:01Z

Sorry, I thought I did one last look over of it, must have forgot! Okay, maybe now this is looking finalized.

asomers · 2017-05-17T03:48:11Z

bors r+

556: Add various pty functions r=asomers * grantpt * posix_openpt * ptsname/ptsname_r * unlockpt I've refactored my code to use these functions and they work correctly at least in the non-error case. Only issue is what to do for failure in `ptsname` when converting strings. This should effectively never happen. I think maybe adding an `Unknown` to the `Error` type and returning that might be useful.

bors · 2017-05-17T04:21:16Z

Build succeeded

continuous-integration/travis-ci/push

Susurrus force-pushed the pty branch from 86282ce to a247129 Compare March 15, 2017 20:34

Mic92 reviewed Mar 17, 2017

View reviewed changes

Susurrus force-pushed the pty branch 4 times, most recently from 3b71257 to 96a7553 Compare April 20, 2017 05:33

asomers requested changes May 12, 2017

View reviewed changes

Susurrus force-pushed the pty branch from 96a7553 to 34cb198 Compare May 16, 2017 04:14

Susurrus force-pushed the pty branch 2 times, most recently from e8b7137 to ea2d03f Compare May 16, 2017 04:25

asomers reviewed May 16, 2017

View reviewed changes

Susurrus force-pushed the pty branch 2 times, most recently from aa9d724 to 51f7199 Compare May 16, 2017 18:31

Susurrus force-pushed the pty branch from 51f7199 to dc2dfb8 Compare May 16, 2017 21:46

asomers requested changes May 17, 2017

View reviewed changes

Add various pty functions

4e97520

* grantpt * ptsname/ptsname_r * posix_openpt * unlockpt

Susurrus force-pushed the pty branch from dc2dfb8 to 4e97520 Compare May 17, 2017 03:40

asomers approved these changes May 17, 2017

View reviewed changes

bors bot merged commit 4e97520 into nix-rust:master May 17, 2017

Susurrus mentioned this pull request May 17, 2017

Add openpty #456

Merged

Susurrus mentioned this pull request May 25, 2017

Enable utsname on MacOS #607

Closed

Susurrus deleted the pty branch July 13, 2017 19:13

		Error::Sys(errno) => io::Error::from_raw_os_error(errno as i32),

Add various pty functions #556

Add various pty functions #556

Conversation

Susurrus commented Mar 15, 2017 • edited Loading

Mic92 Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Mar 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Mar 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Apr 17, 2017

asomers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented May 16, 2017

Susurrus commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asomers commented May 16, 2017

Susurrus commented May 16, 2017

asomers commented May 16, 2017

Susurrus commented May 16, 2017

Susurrus commented May 16, 2017

asomers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented May 17, 2017

asomers commented May 17, 2017

bors bot commented May 17, 2017

Build succeeded

Susurrus commented Mar 15, 2017 •

edited

Loading

Mic92 Mar 17, 2017 •

edited

Loading

Mic92 Mar 20, 2017 •

edited

Loading