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

Fix different behaviour of OpenOptions between Windows en Linux #26772

Closed
wants to merge 8 commits into from
Closed

Fix different behaviour of OpenOptions between Windows en Linux #26772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cdf6361
Fix different behaviour of OpenOptions between Windows en Linux
pitdicker Jul 4, 2015
b8c1737
Remove non-working work-around
pitdicker Jul 4, 2015
da20380
Make append have the same flags as write on Windows
pitdicker Jul 4, 2015
e375197
Clarify documentation and examples
pitdicker Jul 4, 2015
cae20d0
Add extra tests
pitdicker Jul 12, 2015
f7bf079
Some extra documentation
pitdicker Jul 12, 2015
ecbbe3a
WIP
pitdicker Aug 6, 2015
0f5f111
Set generic access flags to unbreak truncate
pitdicker Aug 14, 2015
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
120 changes: 89 additions & 31 deletions src/libstd/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,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 @@ -368,7 +368,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,13 +414,15 @@ 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)`.
///
/// # 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 @@ -431,6 +434,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 @@ -448,12 +453,15 @@ impl OpenOptions {
/// This option indicates whether a new file will be created if the file
/// does not yet already exist.
///
/// The access mode (e.g. read, write and/or append) does not influence whether
/// a new file is created, or with which permissions it is created.
///
/// # 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 {
Expand Down Expand Up @@ -1960,61 +1968,111 @@ 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);
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);

// Test opening a non-existing file
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")));
// Test various combinations of access modes with open options.
// Tested combinations:
// - .create(true).open() when file does not exist
// - .create(true).open() when file exists
// - .open() when file exists
// - .truncate.open() when file exists
check!(c(&r).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(&r).create(true).open(&tmpdir.join("b")));
check!(r.open(&tmpdir.join("b")));
error!(c(&r).truncate(true).open(&tmpdir.join("b")), invalid_options);

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

check!(c(&w).append(true).create(true).open(&tmpdir.join("d")));
check!(c(&rw).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).create(true).open(&tmpdir.join("d")));
check!(rw.open(&tmpdir.join("d")));
check!(c(&rw).truncate(true).open(&tmpdir.join("d")));

check!(c(&rw).append(true).create(true).open(&tmpdir.join("e")));
check!(c(&a).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(&a).create(true).open(&tmpdir.join("e")));
check!(a.open(&tmpdir.join("e")));
error!(c(&a).truncate(true).open(&tmpdir.join("e")), invalid_options);

check!(c(&w).truncate(true).create(true).open(&tmpdir.join("f")));
check!(c(&ra).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")));
check!(c(&ra).create(true).open(&tmpdir.join("f")));
check!(ra.open(&tmpdir.join("f")));
error!(c(&ra).truncate(true).open(&tmpdir.join("f")), invalid_options);

// Test combination of .create(true) and .truncate(true)
// Tested combinations:
// - .truncate.open() when file does not exist
// - .create(true).truncate.open() when file does not exist
// - .create(true).truncate.open() when file exists
assert!(c(&w).truncate(true).open(&tmpdir.join("g")).is_err());
check!(c(&w).create(true).truncate(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!(c(&w).create(true).truncate(true).open(&tmpdir.join("g")));

check!(check!(File::create(&tmpdir.join("h"))).write("foo".as_bytes()));
// Test open options have effect
// 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(), 9);

// Test opening a file without setting an access mode
let mut blank = OO::new();
if cfg!(unix) {
error!(blank.create(true).open(&tmpdir.join("i")), invalid_options);
}
if cfg!(windows) {
let mut f = check!(blank.create(true).open(&tmpdir.join("i")));
let mut b = vec![0; 6];
assert!(f.read(&mut b).is_err());
}
assert_eq!(check!(fs::metadata(&tmpdir.join("h"))).len(), 3);
}

#[test]
Expand Down
54 changes: 44 additions & 10 deletions src/libstd/sys/unix/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,9 +213,13 @@ impl DirEntry {
impl OpenOptions {
pub fn new() -> OpenOptions {
OpenOptions {
flags: 0,
flags: libc::O_CLOEXEC,
read: false,
write: false,
append: false,
truncate: false,
create: false,
create_new: false,
mode: 0o666,
}
}
Expand All @@ -229,15 +233,19 @@ impl OpenOptions {
}

pub fn append(&mut self, append: bool) {
self.flag(libc::O_APPEND, append);
self.append = append;
}

pub fn truncate(&mut self, truncate: bool) {
self.flag(libc::O_TRUNC, truncate);
self.truncate = truncate;
}

pub fn create(&mut self, create: bool) {
self.flag(libc::O_CREAT, create);
self.create = create;
}

pub fn create_new(&mut self, create_new: bool) {
self.create_new = create_new;
}

pub fn mode(&mut self, mode: raw::mode_t) {
Expand All @@ -251,6 +259,37 @@ impl OpenOptions {
self.flags &= !bit;
}
}

fn get_flags(&self) -> io::Result<c_int> {
flags = self.flags;

// Access mode
match (self.read, self.write | self.append) {
(true, true) => flags |= libc::O_RDWR,
(false, true) => flags |= libc::O_WRONLY,
(true, false) => flags |= libc::O_RDONLY,
(false, false) => return Err(Error::from_raw_os_error(libc::EINVAL)),
}
if self.append {flags |= libc::O_APPEND};

// Flags
if self.create_new {
flags |= libc::O_CREAT | libc::O_EXCL;
} else {
if self.truncate {
if !self.write || self.append {
// The result of using O_TRUNC with O_RDONLY or O_APPEND is undefined.
// Not allowed, for consistency with Windows.
return Err(Error::from_raw_os_error(libc::EINVAL))
} else {
flags |= libc::O_TRUNC;
}
}
if self.create {flags |= libc::O_CREAT};
}

Ok(flags)
}
}

impl File {
Expand All @@ -260,12 +299,7 @@ impl File {
}

pub fn open_c(path: &CStr, opts: &OpenOptions) -> io::Result<File> {
let flags = opts.flags | match (opts.read, opts.write) {
(true, true) => libc::O_RDWR,
(false, true) => libc::O_WRONLY,
(true, false) |
(false, false) => libc::O_RDONLY,
};
let flags = try!(opts.get_flags());
let fd = try!(cvt_r(|| unsafe {
libc::open(path.as_ptr(), flags, opts.mode)
}));
Expand Down
22 changes: 9 additions & 13 deletions src/libstd/sys/windows/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,12 +176,14 @@ impl OpenOptions {

fn get_desired_access(&self) -> libc::DWORD {
self.desired_access.unwrap_or({
let mut base = if self.read {libc::FILE_GENERIC_READ} else {0} |
if self.write {libc::FILE_GENERIC_WRITE} else {0};
if self.append {
base &= !libc::FILE_WRITE_DATA;
base |= libc::FILE_APPEND_DATA;
}
let base = if self.read {libc::GENERIC_READ} else {0} |
if self.append {
/* append has the same bits set as those that are implied
for write, but without FILE_WRITE_DATA */
libc::FILE_GENERIC_WRITE & !libc::FILE_WRITE_DATA
} else if self.write {
libc::GENERIC_WRITE
} else {0};
base
})
}
Expand All @@ -201,13 +203,7 @@ impl OpenOptions {
(true, true) => libc::CREATE_ALWAYS,
(true, false) => libc::OPEN_ALWAYS,
(false, false) => libc::OPEN_EXISTING,
(false, true) => {
if self.write && !self.append {
libc::CREATE_ALWAYS
} else {
libc::TRUNCATE_EXISTING
}
}
(false, true) => libc::TRUNCATE_EXISTING,
}
})
}
Expand Down