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.

Sign up for GitHub

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

Jump to bottom

Don't implement Clone on Dir, SignalFd, and PtyMaster #1382

Merged
merged 1 commit into from Feb 13, 2021
Merged

Don't implement Clone on Dir, SignalFd, and PtyMaster #1382

merged 1 commit into from Feb 13, 2021

Conversation

asomers
Copy link
Member

@asomers asomers commented Feb 2, 2021

Since they close their file descriptors on Drop, it's almost impossible
to use Clone without creating a double-close situation.

Also, check for EBADF in SignalFd::drop and Dir::drop.

@asomers asomers requested a review from Susurrus February 2, 2021 03:02
@asomers
Don't implement Clone on Dir, SignalFd, and PtyMaster
c9cb83a 
Since they close their file descriptors on Drop, it's almost impossible
to use Clone without creating a double-close situation.

Also, check for EBADF in SignalFd::drop and Dir::drop.
@asomers
Copy link
Member Author

asomers commented Feb 13, 2021

bors r+

bors bot added a commit that referenced this pull request Feb 13, 2021
@bors @asomers
Merge #1382
609d9c2 
1382: Don't implement Clone on Dir, SignalFd, and PtyMaster r=asomers a=asomers

Since they close their file descriptors on Drop, it's almost impossible
to use Clone without creating a double-close situation.

Also, check for EBADF in SignalFd::drop and Dir::drop.

Co-authored-by: Alan Somers <asomers@gmail.com>
@bors
Copy link
Contributor

bors bot commented Feb 13, 2021

Build failed:

@asomers
Copy link
Member Author

asomers commented Feb 13, 2021

bors retry

@bors
Copy link
Contributor

bors bot commented Feb 13, 2021

Build succeeded:

@bors bors bot merged commit 0bfa49a into nix-rust:master Feb 13, 2021
@asomers asomers deleted the dir_clone branch February 13, 2021 04:02
This was referenced Mar 12, 2021
Merged
Closed
Closed
@coolreader18
Copy link
Contributor

This is causing problems for me - I don't think drop implementations should panic on a syscall error. File et al in std just ignore the result of the close() call, since I think it can be assumed that if there is an error like EBADF then that was achieved through FromRawFd or calling into libc or something like that. At the very least, I'd appreciate an option to close a Dir without it panicking on error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Susurrus Susurrus Awaiting requested review from Susurrus

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@asomers @coolreader18