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

FD_ISSET: take a *const fd_set instead of *mut fd_set #1725

Merged
merged 1 commit into from
Jul 13, 2021

Conversation

asomers
Copy link
Contributor

@asomers asomers commented Apr 8, 2020

FD_ISSET does not modify its fd_set argument, so it may as well take a
const pointer. AFAICT the only reason to take a *mut pointer is because
the Linux man page documents it that way (though since glibc implements
it as a macro, the constedness is undefined). But since libc implements
it directly rather than calling a (nonexistent on most platforms) C
function, we're defining the API ourselves.

@rust-highfive
Copy link

r? @JohnTitor

(rust_highfive has picked a reviewer for you, use r? to override)

@zombiezen
Copy link

While I was researching my nix PR, I came across this page about the const-ness of that argument. I'm not enough of an OS expert to opine on whether the arguments are convincing, but thought I'd pass along in case it's useful.

@JohnTitor
Copy link
Member

Uhm, it's reasonable indeed but I'd prefer as-is since it may cause some confusion by difference with the original definition.

@asomers
Copy link
Contributor Author

asomers commented Apr 14, 2020

By "original definition" do you mean existing versions of libc, or what's documented in C libraries? AFAIK the only place in C that describes the pointer as mutable is in some man pages. But that's a misnomer since the function is really a macro, and its arguments don't have constedness. And some operating systems document FD_ISSET as it really is: as a macro. So I don't think we should feel bound by any constedness described in some OS's man pages.

@JohnTitor
Copy link
Member

Fair enough, but on the other hand, it makes the current signature stricter, I think we shouldn't do that without any strong reason or announcement.

@asomers
Copy link
Contributor Author

asomers commented Sep 13, 2020

Apparently libc does not object to changing the signatures of other functions, even when that causes downstream breakage. Since that's the case, why not merge this PR?
#1891

@bors
Copy link
Contributor

bors commented Feb 13, 2021

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

@JohnTitor
Copy link
Member

Triage: Re-visiting, I still, however, don't see much motivation for this.
@rust-lang/libc Do y'all have any thoughts on it?

@asomers
Copy link
Contributor Author

asomers commented Jul 13, 2021

The motivation is const-correctness. Right now for example, Nix forces users to use a mutable reference in a place where an immutable reference should suffice, just because libc defines FD_ISSET wrongly.
https://docs.rs/nix/0.22.0/nix/sys/select/struct.FdSet.html#method.contains

FD_ISSET does not modify its fd_set argument, so it may as well take a
const pointer.  AFAICT the only reason to take a *mut pointer is because
the Linux man page documents it that way (though since glibc implements
it as a macro, the constedness is undefined).  But since libc implements
it directly rather than calling a (nonexistent on most platforms) C
function, we're defining the API ourselves.
@Amanieu
Copy link
Member

Amanieu commented Jul 13, 2021

A *mut will coerce to a *const, so this will most likely not break anything. Also I would agree with @asomers that *const is the correct type to use here.

@JohnTitor
Copy link
Member

Thanks for clarifying both!
@Amanieu I'm unfamiliar with this, so feel free to r=you if it makes sense to you.

@Amanieu
Copy link
Member

Amanieu commented Jul 13, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jul 13, 2021

📌 Commit 3eafb3b has been approved by Amanieu

@bors
Copy link
Contributor

bors commented Jul 13, 2021

⌛ Testing commit 3eafb3b with merge 4a53e6d...

@bors
Copy link
Contributor

bors commented Jul 13, 2021

☀️ Test successful - checks-actions, checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13
Approved by: Amanieu
Pushing 4a53e6d to master...

@bors bors merged commit 4a53e6d into rust-lang:master Jul 13, 2021
@asomers asomers deleted the fd_isset branch July 13, 2021 14:12
asomers added a commit to asomers/nix that referenced this pull request Jul 13, 2021
There was never any good reason to use mutable receives in the first
place.  Nix originally did so because libc defined FD_ISSET as taking a
mutable receiver, which it did based on an inaccurate Linux man page.
But that's fixed now.

rust-lang/libc#1725
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants