Skip to content
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

Fix ioctl() call compilation error for freebsd #13

Merged
merged 1 commit into from Sep 6, 2017

Conversation

gchers
Copy link

@gchers gchers commented Sep 3, 2017

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.

// 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) };
Copy link
Owner

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.

@gchers
Copy link
Author

gchers commented Sep 5, 2017

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:
Just in case, I post here for reference, in order:
platform, TIOCGWINSZ type, ioctl() request type
for the current libc, and mark with * the wrong entries.
I omit openbsd -like entries for which TIOCGWINSZ is not defined; I suspect linefeed doesn't support them anyway.

linux:
mips-unknown-linux-gnu, c_ulong, c_ulong
arm-unknown-linux-gnueabihf, c_ulong, c_ulong
i686-unknown-linux-gnu, c_ulong, c_ulong
x86_64-unknown-linux-gnu, c_ulong, c_ulong
aarch64-unknown-linux-gnu, c_ulong, c_ulong
x86_64-unknown-linux-musl, c_int, c_int

linux-android:
arm-linux-androideabi, c_int, c_int

freebsd:
* x86_64-unknown-freebsd, c_uint, c_ulong
* x86_64-unknown-dragonfly, c_uint, c_ulong

apple:
x86_64-apple-darwin, c_ulong, c_ulong
i686-apple-darwin, c_ulong, c_ulong

other:
asmjs-unknown-emscripten, c_int, c_int
wasm32-unknown-emscripten, c_int, c_int
sparc64-unknown-linux-gnu, c_ulong, c_ulong

@@ -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,
Copy link
Owner

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.

@murarth
Copy link
Owner

murarth commented Sep 5, 2017

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 libc doesn't define TIOCGWINSZ for OpenBSD or does OpenBSD truly not implement this ioctl request?

@murarth
Copy link
Owner

murarth commented Sep 5, 2017

I've just realized something that will greatly simplify the implementation of this fix: For each integer type in Rust, there exist From implementations to convert losslessly from each smaller integer type of the same signedness, e.g. from u32 to u64. So, the only change that needs to be made to the original code is to pass TIOCGWINSZ.into() to ioctl. (Though you can preserve the comment about the FreeBSD/Dragonfly issue that explains why this is necessary.)

Because a From implementation also exists to convert a value of any type into itself, TIOCGWINSZ.into() can be used on all platforms (provided that the signedness matches for the type of the constant TIOCGWINSZ and the type expected by ioctl, which does appear to be the case).

@gchers
Copy link
Author

gchers commented Sep 5, 2017

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].
I'll: i) try compiling linefeed on an openbsd tomorrow (which I suspect will fail), ii) look for its actual value in ioctl.h (it may be different from the one in freebsd).

[1] https://man.openbsd.org/tty.4#TIOCGWINSZ

@murarth
Copy link
Owner

murarth commented Sep 5, 2017

Looks good. Go ahead and squash the commits and I'll merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't compile on FreeBSD 11.0 - mismatched types at ioctl() call
2 participants