Add cfmakeraw/cfsetspeed #527

Susurrus · 2017-02-25T20:00:07Z

We'll see how this tests on other platforms, but it's supported on BSDs and Linux supposedly.

fiveop

If we do this, can we do it right from the start (for these two functions): We should use the libc FFI bindings and if they do not exist create and get the merged first.

Otherwise looks good, thank you.

Susurrus · 2017-02-26T21:38:41Z

Sure, that definitely seems like the right play. I've already started here: rust-lang/libc#541

While I've been waiting for that PR I started trying to use those functions and I don't really know how to do this. if I just use libc::tcsetspeed then I get errors about the termios datatypes not matching up. So my question is where is the interface between nix and libc here? Do we use libc::Termios or do we want to use the current one? If we want to use the current one, how do I adapt that to libc::termios::termios?

For example if I implement it like this:

diff --git a/src/sys/termios.rs b/src/sys/termios.rs
index e8df1ed..954ec85 100644
--- a/src/sys/termios.rs
+++ b/src/sys/termios.rs
@@ -1,5 +1,5 @@
 use {Errno, Result};
-use libc::c_int;
+use libc::{self, c_int};
 use std::mem;
 use std::os::unix::io::RawFd;
 
@@ -599,6 +599,12 @@ pub fn cfgetospeed(termios: &Termios) -> BaudRate {
     }
 }
 
+pub fn cfmakeraw(termios: &mut Termios) {
+    unsafe {
+        libc::cfmakeraw(termios);
+    }
+}
+
 pub fn cfsetispeed(termios: &mut Termios, baud: BaudRate) -> Result<()> {
     Errno::result(unsafe {
         ffi::cfsetispeed(termios, baud as speed_t)
@@ -611,6 +617,12 @@ pub fn cfsetospeed(termios: &mut Termios, baud: BaudRate) -> Result<()> {
     }).map(drop)
 }
 
+pub fn cfsetspeed(termios: &mut Termios, baud: BaudRate) -> Result<()> {
+    Errno::result(unsafe {
+        libc::cfsetspeed(termios, baud as speed_t)
+    }).map(drop)
+}
+
 pub fn tcgetattr(fd: RawFd) -> Result<Termios> {
     let mut termios = unsafe { mem::uninitialized() };

then I get these errors:

error[E0308]: mismatched types
   --> src/sys/termios.rs:604:25
    |
604 |         libc::cfmakeraw(termios);
    |                         ^^^^^^^ expected struct `libc::libc::termios`, found struct `sys::termios::ffi::consts::Termios`
    |
    = note: expected type `*mut libc::libc::termios`
    = note:    found type `&mut sys::termios::ffi::consts::Termios`

error[E0308]: mismatched types
   --> src/sys/termios.rs:622:26
    |
622 |         libc::cfsetspeed(termios, baud as speed_t)
    |                          ^^^^^^^ expected struct `libc::libc::termios`, found struct `sys::termios::ffi::consts::Termios`
    |
    = note: expected type `*mut libc::libc::termios`
    = note:    found type `&mut sys::termios::ffi::consts::Termios`

Any guidance here as to how to properly wrap libc's datatypes in nix?

posborne · 2017-02-27T00:59:49Z

Any guidance here as to how to properly wrap libc's datatypes in nix?

I think we probably want to use nix's type at the interface level (which provide greater safety) and ensure that proper conversion functions into the libc struct format exist. The memory layout of the two is required to be the same at present, so we just need to (unsafely) transmute between those types.

Susurrus · 2017-02-27T02:10:06Z

Is there a convention for this elsewhere in nix I can copy? I'm uncertain whether implementing From would be better than doing it manually in each location and figured there should be consensus on this in nic already.

fiveop · 2017-02-27T06:37:14Z

Yes. Look for example at SigSet in src/sys/signal.rs. It is just an opaque wrapper for libc::sigset_t. This way, libc takes care of the different layouts on different platforms, and we don't have to have multiple definitions.

Susurrus · 2017-02-27T17:25:49Z

So I've been making some progress on this, but I wanted to understand how high-level you wanted the API. Given the API designed for SigSet I imagine that all the cf* functions in termios should be methods on the Termios struct rather than these standalone functions. Additionally should I replace tcgetattr with a from_fd function as the Termios crate has? Now the question is should the tc* functions all be methods on the RawFd type instead of stand alone functions?

Susurrus · 2017-02-27T21:28:29Z

So I have everything all set for refactoring src/sys/termios.rs to use libc and adding cfmakeraw and cfsetspeed. Now I'm not quite certain what the difference is between the src and src/sys modules, but hopefully this code is in the right place.

fiveop · 2017-02-28T13:06:47Z

Good work. There are at least two problems with flags under OSX left.

Regarding your question about making the functions into methods of Termios, like it was done with Signal: I do not think, that we have yet discussed what style we prefer. I prefer plain functions with arguments for nix. My reason for this is, that I see libc as the place for a raw binding, nix as a place for a safe binding, as far as that's possible, and other libraries can build upon that to create idiomatic wrappers, whatever idiomatic means at that point in time.

The module structure derives from the C file header paths. So, we have src/sys/signal.rs because the corresponding C header gets included with #include <sys/signal.h>.

Susurrus · 2017-02-28T16:52:33Z

This is actually blocking on rust-lang/libc#542 where it looks like they don't have two of the constants we need. Once that's merged that should fix this.

Susurrus · 2017-02-28T18:33:58Z

Rebased and pushed again now that libc has been updated with the proper constants. This should succeed on OS X now whenever Travis gets around to it.

Susurrus · 2017-02-28T20:05:25Z

Passing now on Mac. CI hasn't finished but all the necessary platforms have passed.

Susurrus · 2017-02-28T22:31:22Z

Missed a couple of baud rate constants in my original patch, so I've re-added those for the appropriate OSes. Also I alphabetized the #[cfg that use target_os cause I was having trouble comparing them. And finally I ran things through rustfmt which is why some of the trivial formatting has changed. I assumed that was a direction this project will go (rustfmt as part of CI), so I took the initiative to do it now. If that was inappropriate let me know and I can undo it.

kamalmarhubi · 2017-03-01T14:23:47Z

@Susurrus I'd prefer rustfmt in a separate change.

Susurrus · 2017-03-01T16:50:32Z

@kamalmarhubi Other projects I've worked on have only allowed style changes for code that's being modified so that git blame gives correct results on the first try. I don't know if there's existing policy, but all the style changes I made were only in areas that I was already touching. If you still want me to separate out the style changes, you want them as a separate commit before or after my other changes, or would you like them as a separate PR, or should I just wait on style stuff in general because running rustfmt on nix shows that most of it doesn't follow the style guidelines and will need cleanup later.

Susurrus · 2017-03-01T17:10:34Z

I should also note that by doing this the Termios struct losing public access to all of its members. This "breaks" the struct itself as you cannot get or set any of its fields. I'm going to modify this change to support accessors for each field. Is there any specific naming convention I should use? For example should I change the getter/setter names to be more appropriate like set_input_flags() instead of set_c_iflag() and so on? If so, then we'd end up with:

Termios::set_input_flags();
Termios::set_output_flags();
Termios::set_control_flags();
Termios::set_local_flags();
Termios::set_line_discipline();
Termios::set_special_characters();
Termios::set_input_speed();
Termios::set_output_speed();

and corresponding functions where set is replaced with get.

The problem with this however is that these setters & getters partially overlap with existing termios functionality, specifically the cfget?speed and cfset?speed functions. I don't know what the best solution here is as previous conversations have stated that higher-level APIs should be delegated downstream where libc is trying to be as minimal as a wrapper as possible for a safe API around libc.

kamalmarhubi · 2017-03-01T18:31:05Z

Other projects I've worked on have only allowed style changes for code that's being modified so that git blame gives correct results on the first try.

Ah, that's rust-lang/rustfmt#434 which I should really finish some day :/

This makes sense. I guess I'd just ask if you could revert the blatantly obvious unrelated formatting changes. If not I won't argue hard.

kamalmarhubi · 2017-03-01T18:36:33Z

I should also note that by doing this the Termios struct losing public access to all of its members. This "breaks" the struct itself as you cannot get or set any of its fields.

Could you add Deref<libc::termios> and DerefMut<libc::termios> impls and see if that works?

@fiveop @posborne ^^^^ potential new convention?

posborne · 2017-03-03T05:21:02Z

Could you add Dereflibc::termios and DerefMutlibc::termios impls and see if that works?

Yeah, I've seen that used and implemented that in a few spots (not in nix) before and it can work quite nicely to succinctly provide access to fields in a field that still provides control.

posborne · 2017-03-03T05:22:11Z

Also, this change in general looks very good. Like seeing more of this finally get moved to libc.

fiveop · 2017-03-03T17:16:49Z

Regarding the accessors. In the cases that I can remember, only the constructor and read only accessors were necessary. So the accessors where named after the fields (e.g. foo() for a field foo). If this is not the case for Termios and there is no convention documented in CONVENTION.md, just make a suggestion. (I prefer set_foo and get_foo. What is the convention in other Rust projects (in particular in the standard library?)

Susurrus · 2017-03-03T22:25:27Z

So it seems like there's two directions we can take, head towards setters & getters or just expose the underlying libc::termios::termios struct. It would be nice to use the InputFlags and related fields simply with the Termios struct, like they currently work, but then delegate all the low-level stuff to libc. I'm still not certain how this will work out, but since I have a library that uses this functionality, I'll have on it locally a bit and see what ends up being ergonomic.

Susurrus · 2017-03-05T02:04:38Z

Okay, so I implemented Deref and DerefMut and I'm not certain I like the ergonomics. In my code I have a bunch of stuff that used to look like:

        use termios::{CSIZE, CS5, CS6, CS7, CS8};
        match self.termios.c_cflag & CSIZE {
            CS8 => Some(DataBits::Eight),
            CS7 => Some(DataBits::Seven),
            CS6 => Some(DataBits::Six),
            CS5 => Some(DataBits::Five),

            _ => None,
        }

For this I can't just put .bits() after every constant (necessary for the CS* datatypes to match the termios.c_cflag ones) because it's a syntax error in a match statement.

Another use case of mine would, with this change, look like:

        use nix::termios::{CRTSCTS, IXON, IXOFF};

        if self.termios.c_cflag & CRTSCTS.bits() != 0 {
            Some(FlowControl::Hardware)
        } else if self.termios.c_iflag & (IXON | IXOFF).bits() != 0 {
            Some(FlowControl::Software)
        } else {
            Some(FlowControl::None)
        }

So there's a liberal sprinkling of .bits() everywhere to translate between the low-level termios flag types and those provided by nix.

It really seems like we're straddling in between two levels of API. A low-level API roughly similar to that provided by libc and a higher level one that makes the bit-flags easier to work with. And I think we have to choose what we want, basically just pass through the termios flag types or actually try to encapsulate them and provide higher-level functions to manipulate the entire termios struct.

Susurrus · 2017-03-06T21:20:06Z

@posborne @fiveop Any comments on this? I don't really know what directly to take this API and could use some help.

homu · 2017-04-15T21:11:58Z

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

Susurrus · 2017-07-09T23:25:00Z

Alright, I think this is GTM. Wrote up a couple of tests, as much as I think is reasonable to do here. I have a PR against my serialport-rs crate that utilizes this functionality and it seems to work correctly. Can I get a final review from someone in case I missed something?

asomers

The code looks good. I didn't have time to review the tests; I'll try to do that later.

asomers · 2017-07-09T23:39:16Z

test/test_pty.rs

@@ -149,7 +149,8 @@ fn test_openpty_with_termios() {
        close(pty.slave).unwrap();
        termios
    };
-    termios.c_oflag &= !ONLCR;
+    // Make sure newlines are not tranformed so the data is preserved when sent.


s/tranformed/transformed

asomers · 2017-07-09T23:43:29Z

src/sys/termios.rs

+//!
+//! ```
+//! # use self::nix::sys::termios::{CS5, CSIZE, Termios};
+//! # let mut termios = unsafe { Termios::default_uninit() };


Why does the example code have lines commented out?

This isn't a comment, it just means that these lines are excluded from rendering when generating docs. default_uninit() shouldn't be used by external code, it's just to give me an object that I can use for this. I also do it for use statements that just add visual noise.

Ahh, that makes sense. As you can see, I'm a doccomment nube.

No worries, there's lots of weird syntax stuff with doccomments. Also note that # isn't a comment in Rust :-)

asomers · 2017-07-09T23:44:15Z

src/sys/termios.rs

+/// Stores settings for the termios API
+///
+/// This is a wrapper around the `libc::termios` struct that provides a safe interface for the
+/// standard fields. The only way to obtain an instance of this struct is to extract it from an


s/The only way/The only safe way/

asomers · 2017-07-10T00:26:33Z

src/sys/termios.rs

+    /// This can be used for interfacing with other FFI functions like:
+    ///
+    /// ```rust
+    /// # extern crate libc;


Again, why the comments in the example?

asomers · 2017-07-10T00:28:28Z

src/sys/termios.rs

+    /// consistent.
+    // FIXME: Switch this over to use pub(crate)
+    #[doc(hidden)]
+    pub unsafe fn get_libc_termios_mut(&mut self) -> *mut libc::termios {


If this function and get_libc_termios are not intended for public consumption, then why not make them private methods?

Susurrus · 2017-07-10T02:44:02Z

Fixed your current review comments about spellings and switched around how public some of the helper functions are. For those we're waiting on pub(crate) to stabilize to properly limit their exposure, but for now we hide them from the docs.

asomers · 2017-07-10T03:37:12Z

test/sys/test_termios.rs

+    let mut len = 0;
+    while len < buf.len() {
+        // get_mut would be better than split_at_mut, but it requires nightly
+        len += write(f, &buf[0..]).unwrap();


Are you sure this is right? What advances buf?

asomers · 2017-07-10T03:38:58Z

test/sys/test_termios.rs

+        assert!(pty.master > 0);
+        assert!(pty.slave > 0);
+        let termios = tcgetattr(pty.master).unwrap();
+        close(pty.master).unwrap();


This is a double close. See issue #659. There are a few more instances of it below.

ndusart · 2017-07-10T12:40:54Z

src/sys/termios.rs

+/// This is a wrapper around the `libc::termios` struct that provides a safe interface for the
+/// standard fields. The only safe way to obtain an instance of this struct is to extract it from
+/// an open port using `tcgetattr()`.
+pub struct Termios {


Termios is not Clone anymore

Susurrus · 2017-07-11T02:40:55Z

I think I've addressed everyone's issues for this PR, so if you could take one last look at it @ndusart and @asomers and tell bors if we're all good, I'd appreciate it!

asomers · 2017-07-11T03:47:43Z

I don't see any solution for the double close bug in test_termios.rs. Did you forget that one?

This reduces the boilerplate necessary when wrapping libc constants into groups via enums

This also removes the incorrect TCSASOFT definition as an enum type because it's actually a bitfield.

Susurrus · 2017-07-11T03:49:58Z

Indeed! I think I got one but not the other. Should be fixed now.

asomers · 2017-07-11T03:54:35Z

That fixed the double close. But I just noticed that we don't have a CHANGELOG entry for the new functions. Could you please add one?

Susurrus · 2017-07-11T04:12:33Z

@asomers Indeed I did forget the changelog. I was waiting until I got the iOS builds sorted and then I forgot about it, thanks for the reminder. While I was in there I did a little cleanup of the CHANGELOG (left it as a separate commit), so it should be a little more readable now.

asomers · 2017-07-11T04:21:53Z

Looks like we're finally done. Congrats @Susurrus, you've worked hard on this one.

bors r+

527: Add cfmakeraw/cfsetspeed r=asomers We'll see how this tests on other platforms, but it's supported on BSDs and Linux supposedly.

Susurrus · 2017-07-11T04:33:59Z

Well thanks to everyone to helped out with this PR, with all the reviews and discussions I think we actually finalized on a pretty decent API here! I will say, this is my longest PR I've ever done, since it took 4.5 months to get this thing merged! But it was well worth the effort.

bors · 2017-07-11T04:57:38Z

Build succeeded

fiveop previously requested changes Feb 26, 2017

View reviewed changes

Susurrus force-pushed the cf_more branch from 78ce86e to 07904d8 Compare February 27, 2017 22:09

Susurrus force-pushed the cf_more branch from 07904d8 to 62c4062 Compare February 28, 2017 18:30

Susurrus mentioned this pull request Feb 28, 2017

Any plans for 0.8.0? #519

Closed

Susurrus force-pushed the cf_more branch from 62c4062 to 2c9d3e3 Compare February 28, 2017 22:28

Susurrus mentioned this pull request Mar 1, 2017

TRUST CI: x86_64-unknown-freebsd compilation error - Baud Rates #531

Closed

Susurrus mentioned this pull request Apr 7, 2017

Nix 0.8.0 fails to build on FreeBSD 10.3 #567

Closed

Susurrus force-pushed the cf_more branch from a006890 to 8288e98 Compare July 9, 2017 20:33

asomers reviewed Jul 10, 2017

View reviewed changes

Susurrus force-pushed the cf_more branch from 8288e98 to 34cdc17 Compare July 10, 2017 02:39

asomers requested changes Jul 10, 2017

View reviewed changes

ndusart reviewed Jul 10, 2017

View reviewed changes

Susurrus force-pushed the cf_more branch 2 times, most recently from 4f52ab7 to c79b27f Compare July 10, 2017 19:35

Susurrus added 6 commits July 10, 2017 20:49

Add a libc_enum! macro

30d820f

This reduces the boilerplate necessary when wrapping libc constants into groups via enums

Use libc types for termios

e352935

This also removes the incorrect TCSASOFT definition as an enum type because it's actually a bitfield.

Add cfmakeraw and cfsetspeed

3b66681

Add tcgetsid()

55d68d9

Enable termios on iOS and move it to Tier 2

072637f

Alphabetize Tier 1 supported platforms

9f1b233

Susurrus force-pushed the cf_more branch from c79b27f to 9f1b233 Compare July 11, 2017 03:49

Susurrus added 2 commits July 10, 2017 21:10

Cleanup the changelog

d1e7733

Update changelog for termios PR

0599d91

asomers approved these changes Jul 11, 2017

View reviewed changes

bors bot added a commit that referenced this pull request Jul 11, 2017

Merge #527

17f87d4

527: Add cfmakeraw/cfsetspeed r=asomers We'll see how this tests on other platforms, but it's supported on BSDs and Linux supposedly.

bors bot merged commit 0599d91 into nix-rust:master Jul 11, 2017

Susurrus deleted the cf_more branch July 11, 2017 13:56

Add cfmakeraw/cfsetspeed #527

Add cfmakeraw/cfsetspeed #527

Conversation

Susurrus commented Feb 25, 2017

fiveop left a comment

Choose a reason for hiding this comment

Susurrus commented Feb 26, 2017

posborne commented Feb 27, 2017

Susurrus commented Feb 27, 2017

fiveop commented Feb 27, 2017

Susurrus commented Feb 27, 2017

Susurrus commented Feb 27, 2017

fiveop commented Feb 28, 2017

Susurrus commented Feb 28, 2017

Susurrus commented Feb 28, 2017 • edited Loading

Susurrus commented Feb 28, 2017

Susurrus commented Feb 28, 2017

kamalmarhubi commented Mar 1, 2017

Susurrus commented Mar 1, 2017

Susurrus commented Mar 1, 2017

kamalmarhubi commented Mar 1, 2017 • edited Loading

kamalmarhubi commented Mar 1, 2017

posborne commented Mar 3, 2017

posborne commented Mar 3, 2017

fiveop commented Mar 3, 2017

Susurrus commented Mar 3, 2017

Susurrus commented Mar 5, 2017

Susurrus commented Mar 6, 2017

homu commented Apr 15, 2017

Susurrus commented Jul 9, 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

Susurrus Jul 10, 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 Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jul 11, 2017

asomers commented Jul 11, 2017

Susurrus commented Jul 11, 2017

asomers commented Jul 11, 2017

Susurrus commented Jul 11, 2017

asomers commented Jul 11, 2017

Susurrus commented Jul 11, 2017

bors bot commented Jul 11, 2017

Build succeeded

Susurrus commented Feb 28, 2017 •

edited

Loading

kamalmarhubi commented Mar 1, 2017 •

edited

Loading

Susurrus Jul 10, 2017 •

edited

Loading