Safe fd wrappers #1184
Safe fd wrappers #1184
Conversation
|
|
||
| impl Drop for FileDescriptor { | ||
| fn drop(&mut self) { | ||
| unistd::close(self.0).expect("You've already closed this file descriptor before me =("); |
There was a problem hiding this comment.
close can also fail for reasons like EINTR. You shouldn't panic in such cases.
| @@ -12,7 +12,7 @@ | |||
| #![recursion_limit = "500"] | |||
| #![deny(unused)] | |||
| #![deny(unstable_features)] | |||
| #![deny(missing_copy_implementations)] | |||
| // #![deny(missing_copy_implementations)] | |||
There was a problem hiding this comment.
Presumably you're planning to fix this before merging?
| let this_fd = self.as_raw_fd(); | ||
| let other_fd = other.as_raw_fd(); | ||
|
|
||
| std::mem::forget(other); |
There was a problem hiding this comment.
How about a comment explaining why it's ok to use mem::forget?
| unsafe { Ok(T::from_raw_fd(ret_fd)) } | ||
| } | ||
|
|
||
| /// You must guarantee the RawFd isn't owned by any of fd safe wrappers |
There was a problem hiding this comment.
This comment belongs in a # Safety section.
| std::mem::forget(other); | ||
|
|
||
| // TODO: Make this call unsafe | ||
| let ret_fd = dup2(this_fd, other_fd)?; |
There was a problem hiding this comment.
You need to call dup2 before calling mem::forget, so that you don't deallocate other if dup2 fails. In that event, you should probably return other to the caller. That would make the error type a std::result::Result<T, (nix::Error, T)>
| let this_fd = self.as_raw_fd(); | ||
| let other_fd = other.as_raw_fd(); | ||
|
|
||
| std::mem::forget(other); |
There was a problem hiding this comment.
What happens if a user implements FdOps for his own type, and Drop does something more than just close? In that case, this code would be wrong. Perhaps you should make FdOps a private trait. If not, at least document the strict requirement of drop's behavior.
|
|
||
| impl FromRawFd for StdHandle { | ||
| unsafe fn from_raw_fd(fd: RawFd) -> Self { | ||
| assert!(fd < 3); |
There was a problem hiding this comment.
-1 to magic numbers. Instead of hard-coding 3, you should compare to the libc::STDERR_FILENO etc
Here I'm trying to resolve #594 #678. Wish me luck!