Addressed comments

Author: pitdicker

src/libstd/fs.rs

@@ -414,8 +414,8 @@ impl OpenOptions {
     /// This option, when true, will indicate that the file should be
     /// `write`-able if opened.
     ///
-    /// If a file already exist, the contents of that file get overwritten, but it is
-    /// not truncated.
+    /// If a file already exist, any write calls on the file will overwrite its
+    /// contents, without truncating it.
     ///
     /// # Examples
     ///
@@ -436,19 +436,20 @@ impl OpenOptions {
     /// Note that setting `.write(true).append(true)` has the same effect as
     /// setting only `.append(true)`.
     ///
-    /// For most filesystems the operating system guarantees all writes are atomic:
-    /// no writes get mangled because another process writes at the same time.
+    /// For most filesystems the operating system guarantees all writes are
+    /// atomic: no writes get mangled because another process writes at the same
+    /// time.
     ///
-    /// One maybe obvious note when using append-mode: make sure that all data that
-    /// belongs together, is written the the file in one operation. This can be done
-    /// by concatenating strings before passing them to `write()`, or using a buffered
-    /// writer (with a more than adequately sized buffer) and calling `flush()` when the
-    /// message is complete.
+    /// One maybe obvious note when using append-mode: make sure that all data
+    /// that belongs together, is written the the file in one operation. This
+    /// can be done by concatenating strings before passing them to `write()`,
+    /// or using a buffered writer (with a more than adequately sized buffer)
+    /// and calling `flush()` when the message is complete.
     ///
-    /// If a file is opened with both read and append access, beware that after opening
-    /// and after every write the position for reading may be set at the end of the file.
-    /// So before writing save the current position (using `seek(SeekFrom::Current(0))`,
-    /// and restore it before the next read.
+    /// If a file is opened with both read and append access, beware that after
+    /// opening and after every write the position for reading may be set at the
+    /// end of the file. So before writing save the current position (using
+    /// `seek(SeekFrom::Current(0))`, and restore it before the next read.
     ///
     /// # Examples
     ///
@@ -507,7 +508,12 @@ impl OpenOptions {
     /// No file is allowed to exist at the target location, also no (dangling)
     /// symlink.
     ///
-    /// if `.create_new(true)` is set, `.create()` and `.truncate()` are ignored.
+    /// This option is usefull because it as atomic. Otherwise between checking
+    /// whether a file exists and creating a new one, the file may have been
+    /// created by another process (a TOCTOU race condition / attack).
+    ///
+    /// If `.create_new(true)` is set, `.create()` and `.truncate()` are
+    /// ignored.
     ///
     /// The file must be opened with write or append access in order to create
     /// a new file.
@@ -518,7 +524,9 @@ impl OpenOptions {
     /// #![feature(expand_open_options)]
     /// use std::fs::OpenOptions;
     ///
-    /// let file = OpenOptions::new().write(true).create_new(true).open("foo.txt");
+    /// let file = OpenOptions::new().write(true)
+    ///                              .create_new(true)
+    ///                              .open("foo.txt");
     /// ```
     #[unstable(feature = "expand_open_options",
                reason = "recently added",
@@ -534,7 +542,8 @@ impl OpenOptions {
     /// This function will return an error under a number of different
     /// circumstances, to include but not limited to:
     ///
-    /// * Opening a file that does not exist without setting `create` or `create_new`.
+    /// * Opening a file that does not exist without setting `create` or
+    ///   `create_new`.
     /// * Attempting to open a file with access that the user lacks
     ///   permissions for
     /// * Filesystem-level errors (full disk, etc)

src/libstd/sys/unix/ext/fs.rs

@@ -107,10 +107,11 @@ pub trait OpenOptionsExt {
 
     /// Pass custom flags to the `flags` agument of `open`.
     ///
-    /// The bits that define the access mode are masked out with `O_ACCMODE`, to ensure
-    /// they do not interfere with the access mode set by Rusts options.
+    /// The bits that define the access mode are masked out with `O_ACCMODE`, to
+    /// ensure they do not interfere with the access mode set by Rusts options.
     ///
     /// Custom flags can only set flags, not remove flags set by Rusts options.
+    /// This options overwrites any previously set custom flags.
     ///
     /// # Examples
     ///
@@ -121,7 +122,9 @@ pub trait OpenOptionsExt {
     ///
     /// let mut options = OpenOptions::new();
     /// options.write(true);
-    /// if cfg!(unix) { options.custom_flags(libc::O_NOFOLLOW); }
+    /// if cfg!(unix) {
+    ///     options.custom_flags(libc::O_NOFOLLOW);
+    /// }
     /// let file = options.open("foo.txt");
     /// ```
     #[unstable(feature = "expand_open_options",

src/libstd/sys/unix/fs.rs

@@ -275,13 +275,15 @@ impl OpenOptions {
 
     fn get_creation_mode(&self) -> io::Result<c_int> {
         match (self.write, self.append) {
-            (true,  false) => {}
-            (false, false) => if self.truncate || self.create || self.create_new {
-                                  return Err(Error::from_raw_os_error(libc::EINVAL));
-                              },
-            (_,     true)  => if self.truncate && !self.create_new {
-                                  return Err(Error::from_raw_os_error(libc::EINVAL));
-                              },
+            (true, false) => {}
+            (false, false) =>
+                if self.truncate || self.create || self.create_new {
+                    return Err(Error::from_raw_os_error(libc::EINVAL));
+                },
+            (_, true) =>
+                if self.truncate && !self.create_new {
+                    return Err(Error::from_raw_os_error(libc::EINVAL));
+                },
         }
 
         Ok(match (self.create, self.truncate, self.create_new) {

src/libstd/sys/windows/ext/fs.rs

@@ -27,9 +27,9 @@ pub trait OpenOptionsExt {
     /// with the specified value.
     ///
     /// This will override the `read`, `write`, and `append` flags on the
-    /// `OpenOptions` structure. This method provides fine-grained control
-    /// over the permissions to read, write and append data, attributes
-    /// (like hidden and system) and extended attributes.
+    /// `OpenOptions` structure. This method provides fine-grained control over
+    /// the permissions to read, write and append data, attributes (like hidden
+    /// and system) and extended attributes.
     ///
     /// # Examples
     ///
@@ -38,18 +38,19 @@ pub trait OpenOptionsExt {
     /// use std::fs::OpenOptions;
     /// use std::os::windows::fs::OpenOptionsExt;
     ///
-    /// // Open without read and write permission, for example if you only need to call `stat()`
-    /// // on the file
+    /// // Open without read and write permission, for example if you only need
+    /// // to call `stat()` on the file
     /// let file = OpenOptions::new().access_mode(0).open("foo.txt");
     /// ```
     fn access_mode(&mut self, access: u32) -> &mut Self;
 
     /// Overrides the `dwShareMode` argument to the call to `CreateFile` with
     /// the specified value.
     ///
-    /// By default `share_mode` is set to `FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE`.
-    /// Specifying less permissions denies others to read from, write to and/or
-    /// delete the file while it is open.
+    /// By default `share_mode` is set to
+    /// `FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE`. Specifying
+    /// less permissions denies others to read from, write to and/or delete the
+    /// file while it is open.
     ///
     /// # Examples
     ///
@@ -58,17 +59,20 @@ pub trait OpenOptionsExt {
     /// use std::fs::OpenOptions;
     /// use std::os::windows::fs::OpenOptionsExt;
     ///
+    /// // Do not allow others to read or modify this file while we have it open
+    /// // for writing
     /// let file = OpenOptions::new().write(true)
-    ///                              .share_mode(0) // Do not allow others to read or modify
+    ///                              .share_mode(0)
     ///                              .open("foo.txt");
     /// ```
     fn share_mode(&mut self, val: u32) -> &mut Self;
 
-    /// Sets extra flags for the `dwFileFlags` argument to the call to `CreateFile2`
-    /// (or combines it with `attributes` and `security_qos_flags` to set the
-    /// `dwFlagsAndAttributes` for `CreateFile`).
+    /// Sets extra flags for the `dwFileFlags` argument to the call to
+    /// `CreateFile2` (or combines it with `attributes` and `security_qos_flags`
+    /// to set the `dwFlagsAndAttributes` for `CreateFile`).
     ///
     /// Custom flags can only set flags, not remove flags set by Rusts options.
+    /// This options overwrites any previously set custom flags.
     ///
     /// # Examples
     ///
@@ -79,7 +83,9 @@ pub trait OpenOptionsExt {
     ///
     /// let mut options = OpenOptions::new();
     /// options.create(true).write(true);
-    /// if cfg!(windows) { options.custom_flags(winapi::FILE_FLAG_DELETE_ON_CLOSE); }
+    /// if cfg!(windows) {
+    ///     options.custom_flags(winapi::FILE_FLAG_DELETE_ON_CLOSE);
+    /// }
     /// let file = options.open("foo.txt");
     /// ```
     #[unstable(feature = "expand_open_options",
@@ -89,15 +95,16 @@ pub trait OpenOptionsExt {
 
     /// Sets the `dwFileAttributes` argument to the call to `CreateFile2` to
     /// the specified value (or combines it with `custom_flags` and
-    /// `security_qos_flags` to set the `dwFlagsAndAttributes` for `CreateFile`).
+    /// `security_qos_flags` to set the `dwFlagsAndAttributes` for
+    /// `CreateFile`).
     ///
-    /// If a _new_ file is created because it does not yet exist and `.create(true)` or
-    /// `.create_new(true)` are specified, the new file is given the attributes declared
-    /// with `.attributes()`.
+    /// If a _new_ file is created because it does not yet exist and
+    ///`.create(true)` or `.create_new(true)` are specified, the new file is
+    /// given the attributes declared with `.attributes()`.
     ///
     /// If an _existing_ file is opened with `.create(true).truncate(true)`, its
-    /// existing attributes are preserved and combined with the ones declared with
-    /// `.attributes()`.
+    /// existing attributes are preserved and combined with the ones declared
+    /// with `.attributes()`.
     ///
     /// In all other cases the attributes get ignored.
     ///
@@ -119,10 +126,6 @@ pub trait OpenOptionsExt {
     /// the specified value (or combines it with `custom_flags` and `attributes`
     /// to set the `dwFlagsAndAttributes` for `CreateFile`).
     fn security_qos_flags(&mut self, flags: u32) -> &mut OpenOptions;
-
-    /// Sets the `lpSecurityAttributes` argument to the call to `CreateFile` to
-    /// the specified value.
-    fn security_attributes(&mut self, attrs: sys::c::LPSECURITY_ATTRIBUTES) -> &mut OpenOptions;
 }
 
 #[unstable(feature = "open_options_ext",
@@ -148,10 +151,6 @@ impl OpenOptionsExt for OpenOptions {
     fn security_qos_flags(&mut self, flags: u32) -> &mut OpenOptions {
         self.as_inner_mut().security_qos_flags(flags); self
     }
-
-    fn security_attributes(&mut self, attrs: sys::c::LPSECURITY_ATTRIBUTES) -> &mut OpenOptions {
-        self.as_inner_mut().security_attributes(attrs); self
-    }
 }
 
 /// Extension methods for `fs::Metadata` to access the raw fields contained

src/libstd/sys/windows/fs.rs

@@ -209,13 +209,15 @@ impl OpenOptions {
         const ERROR_INVALID_PARAMETER: i32 = 87;
 
         match (self.write, self.append) {
-            (true,  false) => {}
-            (false, false) => if self.truncate || self.create || self.create_new {
-                                  return Err(Error::from_raw_os_error(ERROR_INVALID_PARAMETER));
-                              },
-            (_,     true)  => if self.truncate && !self.create_new {
-                                  return Err(Error::from_raw_os_error(ERROR_INVALID_PARAMETER));
-                              },
+            (true, false) => {}
+            (false, false) =>
+                if self.truncate || self.create || self.create_new {
+                    return Err(Error::from_raw_os_error(ERROR_INVALID_PARAMETER));
+                },
+            (_, true) =>
+                if self.truncate && !self.create_new {
+                    return Err(Error::from_raw_os_error(ERROR_INVALID_PARAMETER));
+                },
         }
 
         Ok(match (self.create, self.truncate, self.create_new) {