From 071fd5a2723a8d82146b29d17ffe30ac4d430483 Mon Sep 17 00:00:00 2001 From: Tobias Bucher Date: Thu, 26 Feb 2015 23:38:31 +0100 Subject: [PATCH] Implement proper file closing If you don't want to panic in the destructor, you can implement a wrapper struct that logs the error or similar. By making the explicit error the default, the user is forced to make an active decision about how they deal with errors on file closing. --- src/libstd/fs/mod.rs | 14 +++++++++++--- src/libstd/sys/unix/fd.rs | 32 +++++++++++++++++++++++--------- src/libstd/sys/unix/fs2.rs | 4 ++++ src/libstd/sys/windows/handle.rs | 19 +++++++++++++++++-- 4 files changed, 55 insertions(+), 14 deletions(-) diff --git a/src/libstd/fs/mod.rs b/src/libstd/fs/mod.rs index 64ec025a5c423..cdf0132b28698 100644 --- a/src/libstd/fs/mod.rs +++ b/src/libstd/fs/mod.rs @@ -126,6 +126,14 @@ impl File { OpenOptions::new().write(true).create(true).truncate(true).open(path) } + /// Close the file. + /// + /// Use this function if you want to avoid panicking when the destructor of + /// the file is run. + pub fn close(self) -> io::Result<()> { + self.inner.close() + } + /// Returns the original path that was used to open this file. pub fn path(&self) -> Option<&Path> { Some(&self.path) @@ -262,8 +270,8 @@ impl OpenOptions { /// /// * Opening a file that does not exist with read access. /// * Attempting to open a file with access that the user lacks - /// permissions for - /// * Filesystem-level errors (full disk, etc) + /// permissions for. + /// * Filesystem-level errors (full disk, etc). pub fn open(&self, path: &P) -> io::Result { let path = path.as_path(); let inner = try!(fs_imp::File::open(path, &self.0)); @@ -273,7 +281,7 @@ impl OpenOptions { // tradition before the introduction of opendir(3). We explicitly // reject it because there are few use cases. if cfg!(not(any(target_os = "linux", target_os = "android"))) && - try!(inner.file_attr()).is_dir() { + try!(inner.file_attr()).is_dir() { Err(Error::new(ErrorKind::InvalidInput, "is a directory", None)) } else { Ok(File { path: path.to_path_buf(), inner: inner }) diff --git a/src/libstd/sys/unix/fd.rs b/src/libstd/sys/unix/fd.rs index 327d117823ee3..89ce27629e75b 100644 --- a/src/libstd/sys/unix/fd.rs +++ b/src/libstd/sys/unix/fd.rs @@ -26,6 +26,26 @@ impl FileDesc { FileDesc { fd: fd } } + pub fn close(self) -> io::Result<()> { + let fd = self.fd; + // Don't run the destructor after running the `close` function. + unsafe { mem::forget(self) }; + + // Closing stdio file handles makes no sense, so never do it. Note that + // this is relied upon in the destructor. + if fd <= libc::STDERR_FILENO { + return Ok(()); + } + + // Also, note that closing the file descriptor is never retried on + // error. The reason for this is that if an error occurs, we don't + // actually know if the file descriptor was closed or not, and if we + // retried (for something like EINTR), we might close another valid + // file descriptor (opened after we closed ours). + try!(cvt(unsafe { libc::close(fd) })); + Ok(()) + } + pub fn raw(&self) -> c_int { self.fd } /// Extract the actual filedescriptor without closing it. @@ -60,14 +80,8 @@ impl AsInner for FileDesc { impl Drop for FileDesc { fn drop(&mut self) { - // closing stdio file handles makes no sense, so never do it. Also, note - // that errors are ignored when closing a file descriptor. The reason - // for this is that if an error occurs we don't actually know if the - // file descriptor was closed or not, and if we retried (for something - // like EINTR), we might close another valid file descriptor (opened - // after we closed ours. - if self.fd > libc::STDERR_FILENO { - let _ = unsafe { libc::close(self.fd) }; - } + // Close the file descriptor. + let fd = mem::replace(self, FileDesc::new(libc::STDIN_FILENO)); + fd.close().unwrap(); } } diff --git a/src/libstd/sys/unix/fs2.rs b/src/libstd/sys/unix/fs2.rs index 72e0b8dd36c66..b6c1c57b26a29 100644 --- a/src/libstd/sys/unix/fs2.rs +++ b/src/libstd/sys/unix/fs2.rs @@ -218,6 +218,10 @@ impl File { Ok(File(FileDesc::new(fd))) } + pub fn close(self) -> io::Result<()> { + self.0.close() + } + pub fn file_attr(&self) -> io::Result { let mut stat: libc::stat = unsafe { mem::zeroed() }; try!(cvt(unsafe { libc::fstat(self.0.raw(), &mut stat) })); diff --git a/src/libstd/sys/windows/handle.rs b/src/libstd/sys/windows/handle.rs index 99de659be41ed..019138f450dda 100644 --- a/src/libstd/sys/windows/handle.rs +++ b/src/libstd/sys/windows/handle.rs @@ -10,9 +10,10 @@ use prelude::v1::*; -use libc::{self, HANDLE}; use io; use io::ErrorKind; +use libc::{self, HANDLE}; +use mem; use ptr; use sys::cvt; @@ -26,6 +27,19 @@ impl Handle { Handle(handle) } + pub fn close(self) -> io::Result<()> { + let handle = self.0; + unsafe { mem::forget(self) }; + + // Don't try to close the invalid handle, this is relied upon in the destructor. + if handle == libc::INVALID_HANLE_VALUE { + return Ok(()); + } + + try!(cvt(unsafe { libc::CloseHandle(self.0); })); + Ok(()) + } + pub fn raw(&self) -> HANDLE { self.0 } pub fn read(&self, buf: &mut [u8]) -> io::Result { @@ -39,7 +53,8 @@ impl Handle { impl Drop for Handle { fn drop(&mut self) { - unsafe { let _ = libc::CloseHandle(self.0); } + let handle = mem::replace(self, Handle::new(libc::INVALID_HANDLE_VALUE)); + handle.close().unwrap(); } }