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

unistd: make getpeereid take AsFd rather than a raw FD #2137

Merged
merged 5 commits into from
Sep 30, 2023

Conversation

woodruffw
Copy link
Contributor

I noticed that the sockopt APIs now take AsFd, so I figured this API should be made to do the same 🙂

AsFd returns a BorrowedFd that is correct by construction, meaning that this API rejects more error states at the type level (such as passing in fd = -1). As such, I've removed the test_getpeereid_invalid_fd testcase, since its error state is no longer reachable from Rust users of this API.

`AsFd` returns a `BorrowedFd` that is correct by construction,
meaning that this API rejects more error states at the type
level (such as passing in `fd = -1`).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB: Unrelated formatting changes in this file (made automatically). Happy to revert if you'd prefer to leave them out.

@woodruffw woodruffw marked this pull request as ready for review September 25, 2023 00:27
@SteveLauC
Copy link
Member

Ref #1750

src/unistd.rs Outdated Show resolved Hide resolved
@SteveLauC
Copy link
Member

BTW, the module unistd is partially covered in PR #1863, and it does have some work that is not done yet, see this comment, would you like to finish that part in this PR, which would be great!

@woodruffw
Copy link
Contributor Author

would you like to finish that part in this PR, which would be great!

It looks like there are some unresolved design questions in that PR's discussion, so I don't want to make this changeset harder to merge by adding those in.

Does that seem reasonable?

@SteveLauC
Copy link
Member

Sorry for being unclear, I was suggesting finishing the part that is missing from that PR (i.e., the following functions):

  1. pub fn tcgetpgrp(fd: c_int) -> Result<Pid>
  2. pub fn tcsetpgrp(fd: c_int, pgrp: Pid) -> Result<()>
  3. pub fn fpathconf(fd: RawFd, var: PathconfVar) -> Result<Option<c_long>>
  4. pub fn ttyname(fd: RawFd) -> Result<PathBuf>
  5. pub fn getpeereid(fd: RawFd) -> Result<(Uid, Gid)>

It should be okay to simply replace the RawFd (or c_int) with a trait generic argument, just like what's done in this PR:)

@woodruffw
Copy link
Contributor Author

Sorry for being unclear, I was suggesting finishing the part that is missing from that PR (i.e., the following functions):

Oh, gotcha! Yeah, I can definitely do that; sorry for the confusion on my part!

@woodruffw
Copy link
Contributor Author

Done -- all 5 of those APIs now take an AsFd bounds instead, and I've update the tests.

@woodruffw
Copy link
Contributor Author

Gentle ping on this, now that #2132 is semi-resolved 🙂

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@SteveLauC SteveLauC added this pull request to the merge queue Sep 30, 2023
Merged via the queue into nix-rust:master with commit 90e3c3a Sep 30, 2023
35 checks passed
@woodruffw woodruffw deleted the ww/getpeereid-asfd branch September 30, 2023 04:40
@SteveLauC SteveLauC mentioned this pull request Nov 12, 2023
29 tasks
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.

3 participants