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.

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

Add a method for setting permissions directly on an open file. #37886

Merged
merged 1 commit into from
Nov 23, 2016

Conversation

Stebalien
Copy link
Contributor

@Stebalien Stebalien commented Nov 19, 2016

On unix like systems, the underlying file corresponding to any given path may change at any time. This function makes it possible to set the permissions of the a file corresponding to a File object even if its path changes.

@retep998, what's the best way to do this on Windows? I looked into SetFileInformationByHandle but couldn't find a way to do it atomically risking clobbering access time information.

This is a first step towards fixing #37885. This function doesn't have to be public but this is useful functionality that should probably be exposed.

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@Stebalien
Copy link
Contributor Author

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned aturon Nov 19, 2016
@retep998
Copy link
Member

I looked into SetFileInformationByHandle but couldn't find a way to do it atomically risking clobbering access time information.

Just set the file times to 0. That way they won't update and you'll only modify the file attributes.

@Stebalien
Copy link
Contributor Author

Is that documented anywhere? I couldn't find that information on MSDN.

@retep998
Copy link
Member

retep998 commented Nov 20, 2016

I couldn't find any documentation about it myself, but I'm pretty sure it is a feature. Even SetFileAttributes relies on that behavior. I actually discovered this behavior by looking at SetFileAttributes's implementation where it calls NtSetInformationFile (the syscall behind SetFileInformationByHandle) and passes a FILE_BASIC_INFORMATION with the filetimes set to 0.

@Stebalien
Copy link
Contributor Author

Stebalien commented Nov 20, 2016

Grumble... undocumented APIs... anti-trust settlement... sigh (but thanks).

@@ -417,6 +417,24 @@ impl File {
Ok(PathBuf::from(OsString::from_wide(subst)))
}
}

pub fn set_permissions(&self, perm: FilePermissions) -> io::Result<()> {
let mut info = c::FILE_END_OF_FILE_INFO {
Copy link
Member

Choose a reason for hiding this comment

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

Wrong struct name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

let mut info = c::FILE_END_OF_FILE_INFO {
CreationTime: 0,
LastAccessTime: 0,
LastWriteTime: 0,
Copy link
Member

Choose a reason for hiding this comment

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

This field isn't listed in your FILE_BASIC_INFO definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

@Stebalien Stebalien force-pushed the set-perm branch 3 times, most recently from 2f404f5 to 1ebe3ef Compare November 20, 2016 21:57
@Stebalien Stebalien changed the title WIP: Add a method for setting permissions directly on an open file. Add a method for setting permissions directly on an open file. Nov 21, 2016
@Stebalien
Copy link
Contributor Author

So, the question is, does this need an RFC (it's a new public API but it mirrors methods like fs::metadata)?

@retep998
Copy link
Member

This is small enough that it shouldn't need an RFC, although it might still go through the rfcbot merge process with the libs subteam.

@alexcrichton
Copy link
Member

Looks good to me, thanks @Stebalien! Yeah it's ok to not have an RFC for this. Could you file a tracking issue for this, though, and fill that out? I'll tag it afterwards and then r+ so we can land.

@Stebalien
Copy link
Contributor Author

Done.

/// prefer [`File::set_permissions`].
///
/// [`File`]: struct.File.html
/// [`File::set_permissions`]: struct.File.html#method.set_permissions
Copy link
Contributor Author

@Stebalien Stebalien Nov 21, 2016

Choose a reason for hiding this comment

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

Before you merge, would you like me to keep this note or wait till File::set_permissions stabilizes?

Copy link
Member

Choose a reason for hiding this comment

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

Ah let's leave this out until the method stabilizes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@BurntSushi BurntSushi added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 21, 2016
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Nov 21, 2016

📌 Commit 11e8120 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Nov 22, 2016

⌛ Testing commit 11e8120 with merge 510d8cc...

bors added a commit that referenced this pull request Nov 22, 2016
Add a method for setting permissions directly on an open file.

On unix like systems, the underlying file corresponding to any given path may change at any time. This function makes it possible to set the permissions of the a file corresponding to a `File` object even if its path changes.

@retep998, what's the best way to do this on Windows? I looked into `SetFileInformationByHandle` but couldn't find a way to do it atomically risking clobbering access time information.

This is a first step towards fixing #37885. This function doesn't *have* to be public but this is useful functionality that should probably be exposed.
@bors
Copy link
Contributor

bors commented Nov 22, 2016

💔 Test failed - auto-win-msvc-64-opt-rustbuild

On unix like systems, the underlying file corresponding to any given path may
change at any time. This function makes it possible to set the permissions of
the a file corresponding to a `File` object even if its path changes.
@Stebalien
Copy link
Contributor Author

(should be fixed, sorry).

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Nov 23, 2016

📌 Commit 1aaca5f has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Nov 23, 2016

⌛ Testing commit 1aaca5f with merge ccdc26f...

bors added a commit that referenced this pull request Nov 23, 2016
Add a method for setting permissions directly on an open file.

On unix like systems, the underlying file corresponding to any given path may change at any time. This function makes it possible to set the permissions of the a file corresponding to a `File` object even if its path changes.

@retep998, what's the best way to do this on Windows? I looked into `SetFileInformationByHandle` but couldn't find a way to do it atomically risking clobbering access time information.

This is a first step towards fixing #37885. This function doesn't *have* to be public but this is useful functionality that should probably be exposed.
@bors bors merged commit 1aaca5f into rust-lang:master Nov 23, 2016
@bors bors mentioned this pull request Nov 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants