-
Notifications
You must be signed in to change notification settings - Fork 675
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
Added BaudRate enum for termios #518
Conversation
src/sys/termios.rs
Outdated
10014 => BaudRate::B2500000, | ||
10015 => BaudRate::B3000000, | ||
10016 => BaudRate::B3500000, | ||
10017 => BaudRate::B4000000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this and many of the other constants you define here are completely incorrect as, looking at the linux kernel source (https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/termbits.h#L128), the constants there are in octal. These numbers are decimal and, as such, every constant you reference here greater than 7 is wrong.
Since there is better testing infrastructure for all libc variants in the libc
crate, I believe we should introduce the constants there and add the type-safe enumeration providing access to this constants in nix. That has been our basic approach. Since libc
uses ctest
, it will verify the correctness of the constants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, my mistake on the octals. Good catch, thanks.
I will make the correction and push again while looking into libc
Could we push the constants up to libc? Can happen after this PR. |
Working on it (see rust-lang/libc#530). Just need to fixup a few more issues before it'll be merged. I'll rework the |
Oh awesome. I was hoping to help but just found confusing facts. :/ |
New baud constants landed in rust-lang/libc#530, we'll use them.
Updated to use the newly-landed baud constants from libc (rust-lang/libc#530). Any thoughts on adding tests to |
No, as with many things testing some of this stuff is a little tricky as there are not great devices in the kernel for doing all sorts of things. It might be possible to set/get the baud rate on a PTY, but I'm OK with merging this change without that. |
B460800 => BaudRate::B460800, | ||
#[cfg(any(target_os = "netbsd", target_os = "freebsd"))] | ||
B921600 => BaudRate::B921600, | ||
b @ _ => unreachable!("Invalid baud constant: {}", b), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this arm is necessary. The compiler should be able to reason about this branch not being reachable and generate a compile error if something is off (which is better than this runtime error).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it needs to be there to cover all branch cases. speed_t
is just an alias to something like c_uint
so you need a catch-all case at the end to cover all possible values of c_uint
.
For example, if you remove the similar case for the linux
group of constants at line 452 you'll get the compile error:
Compiling nix v0.8.0 (file:///home/zac/repos/rust/nix)
error[E0004]: non-exhaustive patterns: `_` not covered
--> /home/zac/repos/rust/nix/src/sys/termios.rs:420:23
|
419 | match s {
| ^ pattern `_` not covered
error: aborting due to previous error
error: Could not compile `nix`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah. You are right.
@homu r+ |
📌 Commit 8b6242e has been approved by |
Added BaudRate enum for termios Issue #514 Does not provide `BaudRate::EXTA` or `BaudRate::EXTB` constants. These seem to alias to `B19200` and `B38400` respectively and so break the 1:1 mapping needed by `From`. I don't know their historic use.
💔 Test failed - status |
@homu retry |
Added BaudRate enum for termios Issue #514 Does not provide `BaudRate::EXTA` or `BaudRate::EXTB` constants. These seem to alias to `B19200` and `B38400` respectively and so break the 1:1 mapping needed by `From`. I don't know their historic use.
☀️ Test successful - status |
Issue #514
Does not provide
BaudRate::EXTA
orBaudRate::EXTB
constants. These seem to alias toB19200
andB38400
respectively and so break the 1:1 mapping needed byFrom
. I don't know their historic use.