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

Implement RFC 1252 expanding the OpenOptions structure #30872

Merged
merged 5 commits into from
Jan 20, 2016
Merged

Implement RFC 1252 expanding the OpenOptions structure #30872

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
42f4dd0
Implement RFC 1252 expanding the OpenOptions structure
pitdicker Jan 13, 2016
7a1817c
Move `custom_flags` to `OpenOptionsExt`
pitdicker Jan 13, 2016
1230a08
Fix doctests
pitdicker Jan 14, 2016
9c56918
Addressed comments
pitdicker Jan 15, 2016
ae30294
Remove raw pointer from OpenOptions struct
pitdicker Jan 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
194 changes: 147 additions & 47 deletions src/libstd/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ impl<'a> Seek for &'a File {
}

impl OpenOptions {
/// Creates a blank net set of options ready for configuration.
/// Creates a blank new set of options ready for configuration.
///
/// All options are initially set to `false`.
///
Expand All @@ -384,7 +384,8 @@ impl OpenOptions {
/// ```no_run
/// use std::fs::OpenOptions;
///
/// let file = OpenOptions::new().open("foo.txt");
/// let mut options = OpenOptions::new();
/// let file = options.read(true).open("foo.txt");
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn new() -> OpenOptions {
Expand Down Expand Up @@ -413,6 +414,9 @@ 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this clarify that the contents don't get immediately overwritten, only when an actual call to write is made? E.g. emphasize that the cursor is at the beginning so written data will tromp existing data.

Also as a side note could you make sure to format this comment and the ones below to 80 characters in width? (the style of the surrounding module)

///
/// # Examples
///
/// ```no_run
Expand All @@ -429,13 +433,29 @@ impl OpenOptions {
///
/// This option, when true, means that writes will append to a file instead
/// of overwriting previous contents.
/// 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.
///
/// 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.
///
/// # Examples
///
/// ```no_run
/// use std::fs::OpenOptions;
///
/// let file = OpenOptions::new().write(true).append(true).open("foo.txt");
/// let file = OpenOptions::new().append(true).open("foo.txt");
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn append(&mut self, append: bool) -> &mut OpenOptions {
Expand All @@ -447,6 +467,8 @@ impl OpenOptions {
/// If a file is successfully opened with this option set it will truncate
/// the file to 0 length if it already exists.
///
/// The file must be opened with write access for truncate to work.
///
/// # Examples
///
/// ```no_run
Expand All @@ -464,29 +486,60 @@ impl OpenOptions {
/// This option indicates whether a new file will be created if the file
/// does not yet already exist.
///
/// The file must be opened with write or append access in order to create
/// a new file.
///
/// # Examples
///
/// ```no_run
/// use std::fs::OpenOptions;
///
/// let file = OpenOptions::new().create(true).open("foo.txt");
/// let file = OpenOptions::new().write(true).create(true).open("foo.txt");
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn create(&mut self, create: bool) -> &mut OpenOptions {
self.0.create(create); self
}

/// Sets the option to always create a new file.
///
/// This option indicates whether a new file will be created.
/// 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.
///
/// The file must be opened with write or append access in order to create
/// a new file.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this explicitly call out the error mode of failing if the file already exists? (e.g. also calling out that it's atomic)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O, of course. That is practically its only reason of existence :)

///
/// # Examples
///
/// ```no_run
/// #![feature(expand_open_options)]
/// use std::fs::OpenOptions;
///
/// let file = OpenOptions::new().write(true).create_new(true).open("foo.txt");
/// ```
#[unstable(feature = "expand_open_options",
reason = "recently added",
issue = "30014")]
pub fn create_new(&mut self, create_new: bool) -> &mut OpenOptions {
self.0.create_new(create_new); self
}

/// Opens a file at `path` with the options specified by `self`.
///
/// # Errors
///
/// 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 with read access.
/// * 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)
/// * Invalid combinations of open options (truncate without write access,
/// no access mode set, etc)
///
/// # Examples
///
Expand Down Expand Up @@ -2098,61 +2151,108 @@ mod tests {

let mut r = OO::new(); r.read(true);
let mut w = OO::new(); w.write(true);
let mut rw = OO::new(); rw.write(true).read(true);

match r.open(&tmpdir.join("a")) {
Ok(..) => panic!(), Err(..) => {}
}

// Perform each one twice to make sure that it succeeds the second time
// (where the file exists)
check!(c(&w).create(true).open(&tmpdir.join("b")));
assert!(tmpdir.join("b").exists());
check!(c(&w).create(true).open(&tmpdir.join("b")));
check!(w.open(&tmpdir.join("b")));

check!(c(&rw).create(true).open(&tmpdir.join("c")));
assert!(tmpdir.join("c").exists());
let mut rw = OO::new(); rw.read(true).write(true);
let mut a = OO::new(); a.append(true);
let mut ra = OO::new(); ra.read(true).append(true);

let invalid_options = if cfg!(windows) { "The parameter is incorrect" }
else { "Invalid argument" };

// Test various combinations of creation modes and access modes.
//
// Allowed:
// creation mode | read | write | read-write | append | read-append |
// :-----------------------|:-----:|:-----:|:----------:|:------:|:-----------:|
// not set (open existing) | X | X | X | X | X |
// create | | X | X | X | X |
// truncate | | X | X | | |
// create and truncate | | X | X | | |
// create_new | | X | X | X | X |
//
// tested in reverse order, so 'create_new' creates the file, and 'open existing' opens it.

// write-only
check!(c(&w).create_new(true).open(&tmpdir.join("a")));
check!(c(&w).create(true).truncate(true).open(&tmpdir.join("a")));
check!(c(&w).truncate(true).open(&tmpdir.join("a")));
check!(c(&w).create(true).open(&tmpdir.join("a")));
check!(c(&w).open(&tmpdir.join("a")));

// read-only
error!(c(&r).create_new(true).open(&tmpdir.join("b")), invalid_options);
error!(c(&r).create(true).truncate(true).open(&tmpdir.join("b")), invalid_options);
error!(c(&r).truncate(true).open(&tmpdir.join("b")), invalid_options);
error!(c(&r).create(true).open(&tmpdir.join("b")), invalid_options);
check!(c(&r).open(&tmpdir.join("a"))); // try opening the file created with write_only

// read-write
check!(c(&rw).create_new(true).open(&tmpdir.join("c")));
check!(c(&rw).create(true).truncate(true).open(&tmpdir.join("c")));
check!(c(&rw).truncate(true).open(&tmpdir.join("c")));
check!(c(&rw).create(true).open(&tmpdir.join("c")));
check!(rw.open(&tmpdir.join("c")));

check!(c(&w).append(true).create(true).open(&tmpdir.join("d")));
assert!(tmpdir.join("d").exists());
check!(c(&w).append(true).create(true).open(&tmpdir.join("d")));
check!(c(&w).append(true).open(&tmpdir.join("d")));

check!(c(&rw).append(true).create(true).open(&tmpdir.join("e")));
assert!(tmpdir.join("e").exists());
check!(c(&rw).append(true).create(true).open(&tmpdir.join("e")));
check!(c(&rw).append(true).open(&tmpdir.join("e")));

check!(c(&w).truncate(true).create(true).open(&tmpdir.join("f")));
assert!(tmpdir.join("f").exists());
check!(c(&w).truncate(true).create(true).open(&tmpdir.join("f")));
check!(c(&w).truncate(true).open(&tmpdir.join("f")));

check!(c(&rw).truncate(true).create(true).open(&tmpdir.join("g")));
assert!(tmpdir.join("g").exists());
check!(c(&rw).truncate(true).create(true).open(&tmpdir.join("g")));
check!(c(&rw).truncate(true).open(&tmpdir.join("g")));

check!(check!(File::create(&tmpdir.join("h"))).write("foo".as_bytes()));
check!(c(&rw).open(&tmpdir.join("c")));

// append
check!(c(&a).create_new(true).open(&tmpdir.join("d")));
error!(c(&a).create(true).truncate(true).open(&tmpdir.join("d")), invalid_options);
error!(c(&a).truncate(true).open(&tmpdir.join("d")), invalid_options);
check!(c(&a).create(true).open(&tmpdir.join("d")));
check!(c(&a).open(&tmpdir.join("d")));

// read-append
check!(c(&ra).create_new(true).open(&tmpdir.join("e")));
error!(c(&ra).create(true).truncate(true).open(&tmpdir.join("e")), invalid_options);
error!(c(&ra).truncate(true).open(&tmpdir.join("e")), invalid_options);
check!(c(&ra).create(true).open(&tmpdir.join("e")));
check!(c(&ra).open(&tmpdir.join("e")));

// Test opening a file without setting an access mode
let mut blank = OO::new();
error!(blank.create(true).open(&tmpdir.join("f")), invalid_options);

// Test write works
check!(check!(File::create(&tmpdir.join("h"))).write("foobar".as_bytes()));

// Test write fails for read-only
check!(r.open(&tmpdir.join("h")));
{
let mut f = check!(r.open(&tmpdir.join("h")));
assert!(f.write("wut".as_bytes()).is_err());
}

// Test write overwrites
{
let mut f = check!(c(&w).open(&tmpdir.join("h")));
check!(f.write("baz".as_bytes()));
}
{
let mut f = check!(c(&r).open(&tmpdir.join("h")));
let mut b = vec![0; 6];
check!(f.read(&mut b));
assert_eq!(b, "bazbar".as_bytes());
}

// Test truncate works
{
let mut f = check!(c(&w).truncate(true).open(&tmpdir.join("h")));
check!(f.write("foo".as_bytes()));
}
assert_eq!(check!(fs::metadata(&tmpdir.join("h"))).len(), 3);

// Test append works
assert_eq!(check!(fs::metadata(&tmpdir.join("h"))).len(), 3);
{
let mut f = check!(c(&w).append(true).open(&tmpdir.join("h")));
let mut f = check!(c(&a).open(&tmpdir.join("h")));
check!(f.write("bar".as_bytes()));
}
assert_eq!(check!(fs::metadata(&tmpdir.join("h"))).len(), 6);

// Test .append(true) equals .write(true).append(true)
{
let mut f = check!(c(&w).truncate(true).open(&tmpdir.join("h")));
check!(f.write("bar".as_bytes()));
let mut f = check!(c(&w).append(true).open(&tmpdir.join("h")));
check!(f.write("baz".as_bytes()));
}
assert_eq!(check!(fs::metadata(&tmpdir.join("h"))).len(), 3);
assert_eq!(check!(fs::metadata(&tmpdir.join("h"))).len(), 9);
}

#[test]
Expand Down
32 changes: 31 additions & 1 deletion src/libstd/sys/unix/ext/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,46 @@ pub trait OpenOptionsExt {
///
/// If a new file is created as part of a `File::open_opts` call then this
/// specified `mode` will be used as the permission bits for the new file.
/// If no `mode` is set, the default of `0o666` will be used.
/// The operating system masks out bits with the systems `umask`, to produce
/// the final permissions.
#[stable(feature = "fs_ext", since = "1.1.0")]
fn mode(&mut self, mode: raw::mode_t) -> &mut Self;

/// 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.
///
/// Custom flags can only set flags, not remove flags set by Rusts options.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this clarify that any previously set custom flags are overwritten?

///
/// # Examples
///
/// ```rust,ignore
/// extern crate libc;
/// use std::fs::OpenOptions;
/// use std::os::unix::fs::OpenOptionsExt;
///
/// let mut options = OpenOptions::new();
/// options.write(true);
/// if cfg!(unix) { options.custom_flags(libc::O_NOFOLLOW); }
/// let file = options.open("foo.txt");
/// ```
#[unstable(feature = "expand_open_options",
reason = "recently added",
issue = "30014")]
fn custom_flags(&mut self, flags: i32) -> &mut Self;
}

#[stable(feature = "fs_ext", since = "1.1.0")]
impl OpenOptionsExt for OpenOptions {
fn mode(&mut self, mode: raw::mode_t) -> &mut OpenOptions {
self.as_inner_mut().mode(mode); self
}

fn custom_flags(&mut self, flags: i32) -> &mut OpenOptions {
self.as_inner_mut().custom_flags(flags); self
}
}

// Hm, why are there casts here to the returned type, shouldn't the types always
Expand Down Expand Up @@ -262,4 +293,3 @@ impl DirBuilderExt for fs::DirBuilder {
self
}
}

Loading