-
Notifications
You must be signed in to change notification settings - Fork 19
Fix ioctl() call compilation error for freebsd #13
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
Conversation
src/unix/terminal.rs
Outdated
// NOTE: this is a temporary fix to a libc bug described in: | ||
// https://github.com/rust-lang/libc/pull/704 | ||
#[cfg(target_os = "freebsd")] | ||
let res = unsafe { ioctl(fd, TIOCGWINSZ as u64, &mut winsz) }; |
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.
A couple minor points: I believe attributes on expressions landed in stable rustc only a few days ago, so I think it would be better to implement this as cfg
-marked functions, to continue supporting the old stable. Also, I think the cast would be better done as as c_ulong
.
Hi, this should solve with backward compatibility. NOTE_1: I wasn't sure how to create a more generic ioctl() wrapper (i.e., one that could accept either c_int or c_ulong for argument request, which is the case for non-freebsd platforms), so I simply created a wrapper for the call to ioctl() with request=TIOCGWINSZ. NOTE_2:
|
src/unix/terminal.rs
Outdated
@@ -6,7 +6,7 @@ use std::sync::atomic::{AtomicUsize, ATOMIC_USIZE_INIT, Ordering}; | |||
use std::time::Duration; | |||
|
|||
use libc::{ | |||
c_int, c_ushort, c_void, | |||
c_int, c_ushort, c_ulong, c_void, |
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.
This generates an unused_imports
warning on non-freebsd platforms. You can instead add use libc::c_ulong;
inside the freebsd version of ioctl_wrapper
.
If dragonfly has the same issue, you can go ahead and apply this fix for dragonfly, as well. I'd like to support any platform that can reasonably be supported. Do you know whether it is a bug that |
I've just realized something that will greatly simplify the implementation of this fix: For each integer type in Rust, there exist Because a |
Such an elegant solution! Sweet, I was really hoping there was something like that. As for your openbsd question. TIOCGWINSZ is defined, but not in rust libc [1]. |
Looks good. Go ahead and squash the commits and I'll merge it. |
This is a temporary fix to: rust-lang/libc#704
This is a temporary fix to rust-lang/libc#704 and fixes #12.
May want to remove when rust libc accepts rust-lang/libc#704.