Change TIO* constants to c_ulong, as ioctl() requires c_ulong #704

mneumann · 2017-07-30T14:14:26Z

This for example fixes code like:

https://github.com/a8m/pb/blob/master/src/tty/unix.rs#L22

where we would need an "as c_ulong" otherwise.

This for example fixes code like: https://github.com/a8m/pb/blob/master/src/tty/unix.rs#L22 where we would need an "as c_ulong" otherwise.

alexcrichton · 2017-07-30T17:43:04Z

Unfortunately this is a breaking change for now so I'll tag this as 1.0 cleanup

mneumann · 2017-08-02T08:58:24Z

The problem is that for some operating systems these constants are already defined in (rusty) libc as u_long, whereas for some others these are (incorrectly) defined as c_int. Many libraries are only tested on Linux, where these constants are defined as u_long, so these libraries are broken on other OSes like FreeBSD or DragonFly. This leads to a lot of work to fix these small things. Even worse, this leads to code like TIOCGETD as c_ulong all over the place in these libraries, which is only correct as long as ioctl() really is c_ulong.

I assume a 1.0 release is light years away. Having things broken on BSDs is bad IMHO (it frustrates me to spend so much time fixing things like that :(). Can we at least introduce a type ioctl_fun = c_ulong, so that other libraries can safely cast any TIO* constant (any ioctl() request code) to the correct type?

alexcrichton · 2017-08-04T09:33:39Z

Oh I agree it's frustrating! On the other hand though this is a breaking change. I hope that 1.0 is not lightyears away, though. Hopefully just on the order of months.

mneumann · 2017-08-07T08:58:18Z

@alexcrichton: Thanks! I understand it's a breaking change, but I'd argue that it's currently broken on these platforms as dependent crates won't currently build on Free- and DragonFly BSD. A few months I can wait :)

This is a temporary fix to: rust-lang/libc#704 and closes: murarth#12

This is a temporary fix to: rust-lang/libc#704

asomers · 2018-04-26T14:44:05Z

It's not a breaking change if it only affects code that doesn't compile as-is. Can we please have this pre-1.0, unless somebody's aware of code that is using the incorrect types yet somehow actually works?

alexcrichton · 2018-04-26T22:49:22Z

@asomers code which works around these being the wrong types will break if this lands, and it's definitely possible to write working code today that breaks if these types change

mneumann · 2018-04-29T19:36:40Z

@asomers @alexcrichton I think code that works around this will do an "as u32" cast or an "into()", so whatever type libc exports, it will not break anything.

alexcrichton · 2018-04-30T14:57:00Z

This breaks code like let x: c_uint = TIOCEXCL because the type changes

mneumann · 2018-04-30T20:47:57Z

@alexcrichton : Correct, that would break this code. But, that code would be broken anyway, as x would be passed to ioctl, which expects an u_long ;-). Well, I can live with the workaround, as it doesn't involve much code, but on the other hand, this workaround will very likely never be removed from the code :). A more general question arises, how to proceed with bugs in the API (and this is one). Imagine that we would have exposed ioctl with an invalid signature... would we change it and break the API?

alexcrichton · 2018-05-01T17:42:03Z

Sorry but we're just not accepting breaking changes right now, if and when 1.0 happens we can change this but until that time FreeBSD is a "high enough tier" that we don't accept breakage for it.

asomers · 2018-05-01T17:44:57Z

Speaking as a FreeBSD committer, the potential breakage that would be caused by merging this PR is less bad than the actual breakage that is caused by not merging it. This bug breaks a lot of crates that were originally developed on Linux when they try to go to FreeBSD.

alexcrichton · 2018-05-02T02:57:42Z

If a crater-like run could be performed showing that this doesn't have any practical breakage in the ecosystem on FreeBSD then I think we would be able to make this change, but without that knowledge we simply cannot make breaking changes blindly

Mark-Simulacrum · 2018-05-02T03:10:09Z

Would it be sufficient to patch libc with this merged and then run Crater in check-only mode targeting FreeBSD? Or is more than that needed?

alexcrichton · 2018-05-02T19:05:13Z

I believe so yeah, but I don't think there's a lot of precedent for doing this on non-Linux platforms

After migrating from ioctl-rs to libc for `ioctl()` calls, compilation fails on FreeBSD and DragonFlyBSD with type errors because some TIO* constants are defined as `c_uint` when `ioctl()` expects the `request` parameter to be `c_ulong`. Fixing the types in libc is considered a breaking change, so probably won't happen until the next major version bump [1]. A reasonable work-around is to cast the constants to the expected type with `into()`: ioctl(fd, TIOCEXCL.into()) This allows the constants to be used similarly to how one would use a `#define` in C, by letting the compiler convert the constant to the appropriate type [2]. Since Rust is more strict about numerical type conversions, this requires an explicit call to `into()`, which invokes type inference to work out the final type and avoids the need to name types explicitly (such as `TIOCEXCL as c_ulong`). Thus, this is portable across platforms with different types for the `request` parameter of `ioctl()`. This also won't break when libc changes the declared type for TIO* constants to the correct type. [1] rust-lang/libc#704 [2] Only lossless conversions (from smaller to larger type) implement `From` and `Into`. If TIO* constants were defined with a larger type than what `ioctl()` expects, then this would not compile.

gnzlbg · 2019-02-20T19:01:35Z

@mneumann could you rebase this on top of master ?

While this only affects FreeBSD, the APIs it touches are not super rare. I'd like to merge this tentatively and do a release that only contains this change, and then, update libc in rust-lang/rust with this change.

If nobody complains for a reasonable amount of time, we can assume that this did not broke anybody. Otherwise, we'll have to yank that libc release, and revert this change from master, but we can hopefully do that during the nightly cycle as usual.

asomers · 2019-02-20T20:12:53Z

@gnzlbg isn't this what crater is for?

gnzlbg · 2019-02-20T20:24:24Z

@asomers crater can check whether a rust-lang/rust PR breaks something, so when we upgrade libc in rust-lang/rust, we could do a crater run to check that. However, AFAIK, crater cannot be used yet to check whether a PR to a crate on crates.io would break its dependencies.

IIRC, using the bundled libc actually requires a nightly feature (feature(libc)), so I don't know how many crates are doing that instead of using libc from crates.io, so I don't know how useful the crater run would be. We should do this anyways though, but it might well be that doing a crates.io release breaks users that the crater run won't detect.

bors · 2020-05-07T22:11:39Z

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

Change TIO* constants to c_ulong, as ioctl() requires c_ulong

4b88ff0

This for example fixes code like: https://github.com/a8m/pb/blob/master/src/tty/unix.rs#L22 where we would need an "as c_ulong" otherwise.

alexcrichton added the breakage-candidate label Jul 30, 2017

gchers added a commit to gchers/linefeed that referenced this pull request Sep 3, 2017

Fix ioctl() call compilation error for freebsd

08e67d8

This is a temporary fix to: rust-lang/libc#704 and closes: murarth#12

This was referenced Sep 3, 2017

Can't compile on FreeBSD 11.0 - mismatched types at ioctl() call murarth/linefeed#12

Closed

Fix ioctl() call compilation error for freebsd murarth/linefeed#13

Merged

gchers added a commit to gchers/linefeed that referenced this pull request Sep 5, 2017

Fix ioctl() call error on freebsd/dragonfly

28ed48a

This is a temporary fix to: rust-lang/libc#704

kennytm mentioned this pull request Nov 12, 2017

Fix compilation on more platforms rust-lang/cargo#4713

Merged

neersighted mentioned this pull request Apr 15, 2018

Make unix_term.rs compile on BSD platforms console-rs/console#5

Merged

dcuddeback mentioned this pull request Jun 21, 2018

Fix FreeBSD ioctl request constants #1025

Closed

JohnTitor added the S-blocked label Apr 19, 2021

workingjubilee mentioned this pull request Jul 22, 2022

MSRV policy for libc crate rust-lang/libs-team#72

Open

pietroalbini deleted the branch rust-lang:master May 15, 2023 15:04

pietroalbini closed this May 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change TIO* constants to c_ulong, as ioctl() requires c_ulong #704

Change TIO* constants to c_ulong, as ioctl() requires c_ulong #704

mneumann commented Jul 30, 2017

alexcrichton commented Jul 30, 2017

mneumann commented Aug 2, 2017

alexcrichton commented Aug 4, 2017

mneumann commented Aug 7, 2017

asomers commented Apr 26, 2018

alexcrichton commented Apr 26, 2018

mneumann commented Apr 29, 2018

alexcrichton commented Apr 30, 2018

mneumann commented Apr 30, 2018

alexcrichton commented May 1, 2018

asomers commented May 1, 2018

alexcrichton commented May 2, 2018

Mark-Simulacrum commented May 2, 2018

alexcrichton commented May 2, 2018

gnzlbg commented Feb 20, 2019

asomers commented Feb 20, 2019

gnzlbg commented Feb 20, 2019

bors commented May 7, 2020

Change TIO* constants to c_ulong, as ioctl() requires c_ulong #704

Change TIO* constants to c_ulong, as ioctl() requires c_ulong #704

Conversation

mneumann commented Jul 30, 2017

alexcrichton commented Jul 30, 2017

mneumann commented Aug 2, 2017

alexcrichton commented Aug 4, 2017

mneumann commented Aug 7, 2017

asomers commented Apr 26, 2018

alexcrichton commented Apr 26, 2018

mneumann commented Apr 29, 2018

alexcrichton commented Apr 30, 2018

mneumann commented Apr 30, 2018

alexcrichton commented May 1, 2018

asomers commented May 1, 2018

alexcrichton commented May 2, 2018

Mark-Simulacrum commented May 2, 2018

alexcrichton commented May 2, 2018

gnzlbg commented Feb 20, 2019

asomers commented Feb 20, 2019

gnzlbg commented Feb 20, 2019

bors commented May 7, 2020