Skip to content

Remove Copy from PollFd #2631

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

Merged
merged 1 commit into from
Apr 22, 2025
Merged

Conversation

JonathonReinhart
Copy link
Contributor

@JonathonReinhart JonathonReinhart commented Apr 14, 2025

What does this PR do

PollFd implementing Copy makes it easy to accidentally refer to the wrong object after putting one into an array.

This PR removes Copy from PollFd. This fixes #2630.

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API

@JonathonReinhart
Copy link
Contributor Author

I'll wait for a positive signal from maintainers that this is a good solution before I add a changelog entry.

@asomers
Copy link
Member

asomers commented Apr 14, 2025

This looks like a good approach to me. Could you please add a doctest that shows how to properly combine PollFd::new, poll, and PollFd::any (or PollFd::events or similar)? You could probably copy the current docs for PollFd::new and move them into the PollFd structure.

@JonathonReinhart JonathonReinhart force-pushed the pollfd-no-copy branch 4 times, most recently from 17b4048 to 2c7c5fd Compare April 15, 2025 17:02
@JonathonReinhart
Copy link
Contributor Author

Thanks @asomers. I believe This is ready for review and merge.

I'm not sure why tier3 (i686-unknown-hurd-gnu) is failing; it appears to be unrelated to this change.

@SteveLauC
Copy link
Member

SteveLauC commented Apr 16, 2025

I'm not sure why tier3 (i686-unknown-hurd-gnu) is failing; it appears to be unrelated to this change.

Let me take a look at this

Update: Looks like libc 0.2.172 is broken on hurd, it is not related to this PR

Update: I will fix it in #2632

@SteveLauC
Copy link
Member

Hi @JonathonReinhart, you can rebase your branch to fix the CI issues

PollFd implementing Copy makes it easy to accidentally refer to the
wrong object after putting one into an array. Remove Copy to force move
semantics.

This also updates some related docs to improve overall clarity.

This fixes nix-rust#2630
@JonathonReinhart
Copy link
Contributor Author

Looks like this is ready to be merged.

@SteveLauC SteveLauC added this pull request to the merge queue Apr 22, 2025
Merged via the queue into nix-rust:master with commit 6a1c5b8 Apr 22, 2025
40 checks passed
@JonathonReinhart JonathonReinhart deleted the pollfd-no-copy branch April 22, 2025 02:50
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.

PollFd implementing Copy is a footgun
3 participants