pty: Add forkpty by kkuehlz · Pull Request #1042 · nix-rust/nix

kkuehlz · 2019-04-09T11:13:46Z

No description provided.

kkuehlz · 2019-04-09T11:16:38Z

Note: forkpty re-added to linux in rust-lang/libc#1313

kkuehlz · 2019-04-09T21:32:13Z

Err... failed CI for OSX but it passed all the tests?

asomers · 2019-04-09T21:34:45Z

It hung on OSX. Probably a bug in test_forkpty. It also appears to be hanging on FreeBSD. I'll terminate it.

kkuehlz · 2019-04-10T03:20:22Z

@asomers yeah... i think i relied on linux kernel for how it handles a read after write. will fix in a bit.

kkuehlz · 2019-04-11T01:11:31Z

@asomers Got CI passing. Can you take a look

asomers · 2019-04-11T01:44:54Z

+        Child => {
+            write(0, string.as_bytes()).unwrap();
+            pause();  // we need the child to stay alive until the parent calls read
+        },


In a Rust test, you cannot return from a forked child. That can cause deadlocks. Instead, you must _exit(0). The SIGTERM will probably suffice, but I would _exit(0) just in case.

asomers · 2019-04-11T01:46:37Z

+/// the values in `winsize`. If `termios` is not `None`, the pseudoterminal's
+/// terminal settings of the slave will be set to the values in `termios`.
+#[inline]
+pub fn forkpty<'a, 'b, T: Into<Option<&'a Winsize>>, U: Into<Option<&'b Termios>>>(winsize: T, termios: U) -> Result<ForkptyResult> {


80 cols, please.

asomers · 2019-04-11T01:47:07Z

+/// If `winsize` is not `None`, the window size of the slave will be set to
+/// the values in `winsize`. If `termios` is not `None`, the pseudoterminal's
+/// terminal settings of the slave will be set to the values in `termios`.
+#[inline]


Why inline such a big function? In fact, it's rarely a good idea to use #[inline] at all. Better to let the compiler decide.

asomers · 2019-04-11T01:52:19Z

+
+    let mut master: libc::c_int = unsafe { mem::uninitialized() };
+    let res = {
+        match (termios.into(), winsize.into()) {


I think you could considerably shorten this block by using something like termios.into().unwrap_or(ptr::null_mut()).

asomers · 2019-04-11T01:53:23Z

+/// This is returned by `forkpty`. Note that this type does *not* implement `Drop`, so the user
+/// must manually close the file descriptors.
+#[derive(Clone, Copy)]
+#[allow(missing_debug_implementations)]


Why not derive Debug?

Now that you've derived Debug, you should remove the #[allow(missing_debug_implementations)]

asomers · 2019-04-11T01:55:26Z

+    let pty = forkpty(None, None).unwrap();
+    match pty.fork_result {
+        Child => {
+            write(0, string.as_bytes()).unwrap();


Yikes! Can you eliminate that magic number?

kkuehlz · 2019-04-11T07:13:44Z

@asomers Made the suggested changes.

asomers · 2019-04-14T15:46:24Z

+/// This is returned by `forkpty`. Note that this type does *not* implement `Drop`, so the user
+/// must manually close the file descriptors.
+#[derive(Clone, Copy)]
+#[allow(missing_debug_implementations)]


Now that you've derived Debug, you should remove the #[allow(missing_debug_implementations)]

asomers · 2019-04-14T15:55:53Z

+        None => ptr::null_mut(),
+    };
+
+    let win = match winsize.into() {


FYI, instead of match winsize.into() you could equivalently do winsize.into().map(|ws| ws as *const Winsize).unwrap_or(ptr::null_mut()). It's just a matter of which style you prefer. Either is acceptable.

kkuehlz · 2019-04-14T23:28:03Z

@asomers Thanks for the comments. I made the changes. Since the initial code was based largely off of openpty a lot of the review comments could be applied to that function; I am happy to make another PR to refactor that.

kkuehlz · 2019-04-21T08:36:05Z

@asomers anything left to fix for this?

asomers · 2019-04-21T14:47:52Z

+
+    Ok(ForkptyResult {
+        master: master,
+        fork_result: fork_result,


If libc::forkpty fails, then master will be uninitialized here. For that matter, the return value will be wrong, too. You should ensure that it returns Err if libc::forkpty fails.

Errno::result(res) should convert to an Err if forkpty fails, and ? on line 320 should propagate the error.

Ahh, I didn't see that little ?.

asomers

Ok, this looks good. All you need to do is add a CHANGELOG entry and squash.

asomers · 2019-04-23T14:55:22Z

+
+    Ok(ForkptyResult {
+        master: master,
+        fork_result: fork_result,


Ahh, I didn't see that little ?.

kkuehlz · 2019-04-23T21:38:32Z

@asomers done

asomers

Thanks for your contribution!

bors r+

1042: pty: Add forkpty r=asomers a=keur Co-authored-by: Kevin Kuehler <keur@ocf.berkeley.edu>

bors · 2019-04-23T22:20:03Z

Build succeeded

continuous-integration/travis-ci/push
FreeBSD 11.2

kkuehlz force-pushed the forkpty branch 2 times, most recently from 94bf05f to e0f1a90 Compare April 9, 2019 20:48

kkuehlz force-pushed the forkpty branch 2 times, most recently from 648b81d to 67688aa Compare April 10, 2019 22:58

asomers reviewed Apr 11, 2019

View reviewed changes

asomers reviewed Apr 14, 2019

View reviewed changes

kkuehlz force-pushed the forkpty branch from 3cec8da to 3a95beb Compare April 14, 2019 23:25

asomers reviewed Apr 21, 2019

View reviewed changes

asomers reviewed Apr 23, 2019

View reviewed changes

kkuehlz force-pushed the forkpty branch from 3a95beb to dcbc3c8 Compare April 23, 2019 20:17

pty: Add forkpty

26bd036

kkuehlz force-pushed the forkpty branch from dcbc3c8 to 26bd036 Compare April 23, 2019 20:19

asomers approved these changes Apr 23, 2019

View reviewed changes

bors bot added a commit that referenced this pull request Apr 23, 2019

Merge #1042

71b35a0

1042: pty: Add forkpty r=asomers a=keur Co-authored-by: Kevin Kuehler <keur@ocf.berkeley.edu>

bors bot merged commit 26bd036 into nix-rust:master Apr 23, 2019

Conversation

kkuehlz commented Apr 9, 2019

Uh oh!

kkuehlz commented Apr 9, 2019

Uh oh!

kkuehlz commented Apr 9, 2019

Uh oh!

asomers commented Apr 9, 2019

Uh oh!

kkuehlz commented Apr 10, 2019

Uh oh!

kkuehlz commented Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkuehlz commented Apr 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkuehlz commented Apr 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kkuehlz commented Apr 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkuehlz Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asomers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkuehlz commented Apr 23, 2019

Uh oh!

asomers left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Apr 23, 2019

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kkuehlz commented Apr 11, 2019 •

edited

Loading

kkuehlz commented Apr 14, 2019 •

edited

Loading

kkuehlz Apr 23, 2019 •

edited

Loading