Add openpty #456

cactorium · 2016-11-02T13:43:55Z

Lemme know if anything needs to be fixed

cactorium · 2016-11-02T14:34:46Z

So it's only failing on aarch64, because apparently some iotctl used in openpty() isn't implemented on it. Should I just add a #[cfg(not(target_arch="aarch64"))] to the test?

posborne · 2016-11-04T18:11:22Z

So it's only failing on aarch64, because apparently some iotctl used in openpty() isn't implemented on it. Should I just add a #[cfg(not(target_arch="aarch64"))] to the test?

I think our preference would be to work with libc upstream to fix aarch64. Or are you saying that openpty isn't supported at all on aarch64?

cactorium · 2016-11-04T18:42:04Z

I'm not sure; the test run is failing with test test_pty::test_openpty ... Unsupported ioctl: cmd=0x80045430 (which is TIOCGPTN for future reference), and returning Sys(ENOSYS), which means "unimplemented" according to the man pages. So it's either in libc using the wrong syscalls on this platform (I'm guessing pretty unlikely), the aarch64 kernel not implementing it (also unlikely), or the QEMU emulation of the system not implementing it (probably this one?). Do you have any ideas on how to figure out which versions of these things are being run?

lucab · 2017-02-11T15:34:55Z

I think you may have hit https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg04286.html.

kamalmarhubi · 2017-02-14T06:15:32Z

Do you have any ideas on how to figure out which versions of these things are being run?

@posborne do you know offhand the version of qemu that's in your docker image?

posborne · 2017-02-17T04:54:42Z

@posborne do you know offhand the version of qemu that's in your docker image?

Unfortunately, no. The base image I built stuff off of at the time was 15.10.

cactorium · 2017-02-17T08:06:29Z

I think it's 0.10.50? I have no idea how Docker works, but I think I managed to pull the Docker image and start a container, and from inside a shell in that container:

docker pull posborne/rust-cross:android
# there was a lot of messing around between these two commands, but I think you only need these two
docker run --rm -it --name foo -p 5000:80 posborne/rust-cross:android /bin/sh

/android/sdk/tools/emulator-arm -qemu -version gave me

QEMU PC emulator version 0.10.50Android, Copyright (c) 2003-2008 Fabrice Bellard

lucab · 2017-02-17T09:42:48Z

@cactorium that's the android emulator version. Moreover the concerning failure is the one on aarch64-unknown-linux-gnu

Looking inside posborne/rust-cross:arm, qemu version seems to be:

# qemu-system-aarch64 --version
QEMU emulator version 2.3.0 (Debian 1:2.3+dfsg-5ubuntu9.2), Copyright (c) 2003-2008 Fabrice Bellard

It may need a bump to 2.7 or 2.8.

posborne · 2017-02-17T17:25:04Z

Yeah, we can certainly bump. I would really like to move the CI to something like https://github.com/japaric/trust

lucab · 2017-02-17T22:27:04Z

/cc @japaric

berkowski · 2017-02-25T19:24:47Z

@posborne I could take a stab at getting trust CI up and running for nix. I'd like to learn more about it for my own projects anyway.

posborne · 2017-02-25T19:35:24Z

@posborne I could take a stab at getting trust CI up and running for nix. I'd like to learn more about it for my own projects anyway.

@berkowski That would be awesome.

Susurrus · 2017-04-17T18:09:01Z

@cactorium Can you rebase this so we can give it a run through now that we're testing code on more platforms?

cactorium · 2017-04-17T19:00:22Z

Can do!

Susurrus · 2017-04-17T19:08:42Z

@cactorium Sorry I didn't notice this the first time, but it looks like this should really be one commit, as you have a few smaller fixing commits. Can you squash this down into one? That would help me review it.

I'm also curious how this will end up interacting with #527. @cactorium Can you take a look at that issue, it kind of exploded into talking about how best to deal with the termios in nix? I don't want to merge this or that other one until we really know what we're getting ourselves into.

@berkowski If you have time can you take a look over this as well? This is probably of interest to you.

cactorium · 2017-04-17T19:14:01Z

Err I think I messed up the commit history on this PR a lot, I guess pulling would've been a better choice than an actual rebase. Would you guys have any advice on cleaning up the commit history? I'll look at the other PR in a bit

Susurrus · 2017-04-17T19:20:14Z

Do a git reset upstream/master --hard or use origin if that's what you called the nix-rust/nix repo.
Then cherry-pick your 3 main commits like cherry-pick 397840a 0e3fcdb 75f44c9.

Then you can squash them with git rebase -i and force-push again.

Susurrus · 2017-04-17T19:55:25Z

src/pty.rs

+    let mut master: libc::c_int = -1;
+    let c_termios = match &termios {
+        &Some(ref termios) => termios as *const Termios,
+        &None => 0 as *const Termios,


I think ptr::null() would be more appropriate

Susurrus · 2017-04-17T19:55:38Z

src/pty.rs

+    };
+    let c_winsize = match &winsize {
+        &Some(ref ws) => ws as *const Winsize,
+        &None => 0 as *const Winsize,


Again, I think ptr::null() would be more appropriate

Susurrus · 2017-04-17T19:55:53Z

src/pty.rs

+        libc::openpty(
+            &mut master as *mut libc::c_int,
+            &mut slave as *mut libc::c_int,
+            0 as *mut libc::c_char,


Again I think ptr::null() would be more appropriate

Susurrus · 2017-04-17T19:58:58Z

test/test_pty.rs

+
+#[test]
+fn test_openpty() {
+    // TODO: figure out the right termios settings to pass in here


Is this a TODO you intend to fix as part of this PR? If we're testing this I think it'd be nice if we could test the case where a termios and winsize struct is passed in. Especially if we change the function declaration as I mentioned earlier.

Any advice on what flags to play with? The only ones I can think of messing with don't do much other than turn CR on/off

It doesn't really matter what flags you set so long as the function gets tested with None and Some(_) types really. If the CR thing works, go ahead and add a test that tests that.

After looking into it, there's not really a nice way to initialize a Termios object in a platform-agnostic manner until it implements Default, so I'd argue for pushing that fix later in a separate PR

Could you file a ticket against libc about this then? I'd actually prefer if we could resolve that before merging this if it's relatively easy.

Default should always set the fields to valid values, it kind of defeats the point if it doesn't. For the termios struct I'm pretty certain for the termios struct that all zeros is valid.

But for your tests, just create a termios struct out of uninitialized memory and use the various methods available to set the struct to your desired values. Does that work?

So I ended up using tcgetattr to populate the Termios. Apparently this breaks on OS X because they don't support tcgetattr?

Edit: Either that or it's failing when trying to initialize a pty without carriage returns, it's hard to tell from the logs

Yeah, tcgetattr(stdin) is failing on OS X, added two print!() statements around that call and only the first one shows up in the log. Two obvious options are to either disable the test on OS X or switch back to just using (None, None)

You can only use tcgetattr on ttys, which I don't think stdin is, which explains why it fails on mac. What I would recommend is calling tcgetattr on one of the pttys you create and then use that to open one with that termios. So you'd do both of these in the same test. I'd make that a separate test than the one you already have that is testing reading & writing from a master/slave pair.

Huh, I'm pretty sure it is on Linux-based platforms at least. From my understanding, the terminal emulator spawns a pty, gives the slave (or master? one of them) to the shell program, and draws stuff to the screen based on what it receives on its end. Anyways, that fix did it, both tests passing on all important test cases

Susurrus · 2017-04-17T20:10:34Z

src/pty.rs

+/// 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 openpty(winsize: Option<Winsize>, termios: Option<Termios>) -> Result<OpenptyResult> {


First, can't both of be borrows?

Second, we should be able to use a slightly different function declaration to give us better ergonomics when using a real struct for either of the arguments, something like:

pub fn openpty<T: Into<Option<&Winsize>>, U: Into<Option<&Termios>>>(winsize: T, termios: U) -> ... {

So now you can use .into() to convert the options to the struct representations and the caller doesn't need to wrap their struct reference in a Some. I don't know if the syntax above is exactly correct, but I think it should be. The gtk-rs project did this quite successfully for all their string types and it's nice (here is an example).

Susurrus · 2017-04-20T05:36:20Z

@cactorium Can you please also add an entry to the CHANGELOG?

Susurrus · 2017-04-22T16:32:54Z

test/test_pty.rs

+#[test]
+fn test_openpty() {
+    let pty = openpty(None, None).unwrap();
+    assert!(pty.master > 0); // must be valid file descriptors


Please capitalize the "m" in "must" and move this comment to the above line if you want to keep it, though I don't see this comment as very useful. Maybe just remove these trailing comments from the code.

Susurrus · 2017-04-22T16:33:32Z

Can you add another test using a valid winsize argument? I can't easily figure out what that struct is, but it'd be nice to have verification that that works as well.

cactorium · 2017-04-23T03:57:25Z

Well, that sets the size of the terminal window, and there's AFAIK really no nice way to test for it that doesn't involve ioctl

homu · 2017-05-17T04:21:35Z

☔ The latest upstream changes (presumably ba5c837) made this pull request unmergeable. Please resolve the merge conflicts.

Susurrus · 2017-05-17T04:25:31Z

@cactorium Can you rebase this now that #556 just merged?

cactorium · 2017-05-18T12:19:15Z

Done

Susurrus · 2017-05-18T14:39:42Z

src/pty.rs

+use sys::termios::Termios;
+use {Errno, Result, Error, fcntl};
+
+pub struct OpenptyResult {


This needs a doccomment since it's a public type.

Susurrus · 2017-05-18T14:41:22Z

src/pty.rs

+            c_winsize as *mut Winsize)
+    };
+
+    let _ = try!(Errno::result(ret));


Can't this just be Errno::result(ret)?; instead? Can you explain what this line is doing for me?

Fixed! I forgot about the new ? operator, which is why I used a try! instead

Susurrus · 2017-05-18T14:42:18Z

test/test.rs

@@ -13,6 +13,7 @@ mod sys;
 mod test_fcntl;
 mod test_net;
 mod test_nix_path;
+mod test_pty;


Yeah, we should really alphabetize this list.

Susurrus · 2017-05-18T14:42:52Z

test/test_pty.rs

+    assert!(pty.master > 0);
+    assert!(pty.slave > 0);
+
+    // writing to one should be readable on the other one


Can you capitalize the first word in your line comments? Please do that to all of them as well.

Susurrus · 2017-05-18T14:44:06Z

test/test_pty.rs

+    assert_eq!(len, echoed_string2.len());
+    assert_eq!(&buf[0..len], echoed_string2.as_bytes());
+
+    close(pty.master).unwrap();


Don't you normally want to close the slave first? Is there anywhere that suggest that closing the master will close the slave as well at the OS-level?

I'm not sure; the best documentation I could find seemed to imply either way was okay (although nothing implies that closing either end will close the other one):

If fildes refers to the master side of a pseudo-terminal, and this is the last close, a SIGHUP signal shall be sent to the controlling process, if any, for which the slave side of the pseudo-terminal is the controlling terminal. It is unspecified whether closing the master side of the pseudo-terminal flushes all queued input and output.

If fildes refers to the slave side of a STREAMS-based pseudo-terminal, a zero-length message may be sent to the master.

cactorium · 2017-05-29T13:21:24Z

Sorry for taking so long to push changes, it's been a busy few weeks

cactorium · 2017-06-03T18:28:49Z

Didn't notice the build fail, it's fixed now

Susurrus · 2017-06-03T19:04:27Z

Looks good. Now just blocking on #590.

Susurrus · 2017-06-11T20:47:57Z

@cactorium Can you rebase this on HEAD now that our new testing framework has merged? This is RTM after it passes those tests.

cactorium · 2017-06-12T12:35:56Z

Done

Susurrus · 2017-06-12T21:11:17Z

@asomers Can you check into why your FreeBSD buildbot isn't running?

cactorium · 2017-06-12T22:23:04Z

Isn't that just disabled? It's got that DISABLE_TESTS=1 flag on all the recent builds I looked at: https://travis-ci.org/nix-rust/nix/builds

Susurrus · 2017-06-12T22:30:39Z

It's not run through Travis CI. We have a separate build server that runs tests on FreeBSD. @asomers runs it.

asomers · 2017-06-12T22:36:37Z

Buildbot saw the most recent change, yet did not decide to do a build. I'll dig deeper after I get home tonight.

asomers · 2017-06-13T15:04:21Z

I couldn't find anything obviously wrong with the BuildBot server. I've written to the mailing list for help. but in the meantime, I suggest we don't hold up PRs waiting for it to complete. Let's go ahead and merge anything compiles for i686-unknown-freebsd, which is done through Travis.

Susurrus · 2017-06-13T15:06:48Z

bors r+

456: Add openpty r=Susurrus Lemme know if anything needs to be fixed

bors · 2017-06-13T17:07:19Z

Build succeeded

cactorium force-pushed the master branch from 75f44c9 to 38cb514 Compare April 17, 2017 19:52

Susurrus suggested changes Apr 17, 2017

View reviewed changes

cactorium force-pushed the master branch 2 times, most recently from 60d4319 to b6ed40d Compare April 17, 2017 23:58

cactorium force-pushed the master branch 4 times, most recently from c59e711 to 4909877 Compare April 21, 2017 02:42

Susurrus reviewed Apr 22, 2017

View reviewed changes

cactorium force-pushed the master branch from 4909877 to 736bf3d Compare April 23, 2017 03:50

cactorium force-pushed the master branch from 736bf3d to 3964e74 Compare May 18, 2017 12:18

Susurrus approved these changes May 18, 2017

View reviewed changes

Susurrus reviewed May 18, 2017

View reviewed changes

cactorium force-pushed the master branch 2 times, most recently from 54c46ff to 66cd9af Compare June 3, 2017 16:46

Susurrus changed the title ~~Add openpty~~ [RTM, blocking on #590] Add openpty Jun 3, 2017

Susurrus changed the title ~~[RTM, blocking on #590] Add openpty~~ Add openpty Jun 11, 2017

Add openpty()

45b7b1b

cactorium force-pushed the master branch from 66cd9af to 45b7b1b Compare June 12, 2017 12:34

bors bot added a commit that referenced this pull request Jun 13, 2017

Merge #456

6783ffc

456: Add openpty r=Susurrus Lemme know if anything needs to be fixed

bors bot merged commit 45b7b1b into nix-rust:master Jun 13, 2017

Add openpty #456

Add openpty #456

Conversation

cactorium commented Nov 2, 2016

cactorium commented Nov 2, 2016

posborne commented Nov 4, 2016

cactorium commented Nov 4, 2016

lucab commented Feb 11, 2017

kamalmarhubi commented Feb 14, 2017 • edited Loading

posborne commented Feb 17, 2017

cactorium commented Feb 17, 2017

lucab commented Feb 17, 2017

posborne commented Feb 17, 2017

lucab commented Feb 17, 2017

berkowski commented Feb 25, 2017

posborne commented Feb 25, 2017

Susurrus commented Apr 17, 2017

cactorium commented Apr 17, 2017

Susurrus commented Apr 17, 2017

cactorium commented Apr 17, 2017

Susurrus commented Apr 17, 2017

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

cactorium Apr 20, 2017 • edited Loading

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 Apr 20, 2017

Choose a reason for hiding this comment

Susurrus commented Apr 22, 2017

cactorium commented Apr 23, 2017

homu commented May 17, 2017

Susurrus commented May 17, 2017

cactorium commented May 18, 2017

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 May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cactorium May 29, 2017 • edited Loading

Choose a reason for hiding this comment

cactorium commented May 29, 2017

cactorium commented Jun 3, 2017

Susurrus commented Jun 3, 2017

Susurrus commented Jun 11, 2017

cactorium commented Jun 12, 2017

Susurrus commented Jun 12, 2017

cactorium commented Jun 12, 2017

Susurrus commented Jun 12, 2017

asomers commented Jun 12, 2017

asomers commented Jun 13, 2017

Susurrus commented Jun 13, 2017

bors bot commented Jun 13, 2017

Build succeeded

kamalmarhubi commented Feb 14, 2017 •

edited

Loading

cactorium Apr 20, 2017 •

edited

Loading

Susurrus May 18, 2017 •

edited

Loading

cactorium May 29, 2017 •

edited

Loading