Add chmod, fchmod, fchmodat functions #857

antage · 2018-02-05T19:48:45Z

No description provided.

asomers · 2018-02-06T04:37:50Z

CHANGELOG.md

@@ -8,6 +8,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 ### Added

 - Added `alarm`. ([#830](https://github.com/nix-rust/nix/pull/830))
+- Added `chmod`, `fchmod`, `fchmodat`.


Please add a link to the PR, as other entries do.

asomers · 2018-02-06T04:38:31Z

src/sys/stat.rs

@@ -122,3 +122,88 @@ pub fn fstatat<P: ?Sized + NixPath>(dirfd: RawFd, pathname: &P, f: AtFlags) -> R
    Ok(dst)
 }

+/// The `chmod` function shall change `Mode::S_ISUID`, `Mode::S_ISGID`, `Mode::S_ISVTX`,


The doccomment block must begin with a brief description, < 80 characters, followed by a blank line. And it's not necessary to copy the description from opengroup; nobody is going to check Nix's docs to understand what the underlying system call's compliance requirements are. Just write what a Rust programmer would need to know.

asomers · 2018-02-06T04:47:05Z

src/sys/stat.rs

+///
+/// See also [chmod(2)](http://pubs.opengroup.org/onlinepubs/9699919799/functions/chmod.html).
+pub fn chmod<P: ?Sized + NixPath>(path: &P, mode: Mode) -> Result<()> {
+    let res = try!(path.with_nix_path(|cstr| unsafe {


You can replace try! with ?. See https://blog.rust-lang.org/2016/11/10/Rust-1.13.html for a description of how it works.

asomers · 2018-02-06T04:48:11Z

src/sys/stat.rs

+        libc::chmod(cstr.as_ptr(), mode.bits() as mode_t)
+    }));
+
+    Errno::result(res).map(drop)


I prefer .map(|_| () ). I think it's more obvious what you're trying to accomplish.

antage · 2018-02-06T17:08:06Z

Done.

asomers

Please squash your commits when done. Since we use Bors, we can't autosquash on merge.

asomers · 2018-02-09T16:03:46Z

src/sys/stat.rs

+///
+/// # References
+///
+/// See also [chmod(2)](http://pubs.opengroup.org/onlinepubs/9699919799/functions/chmod.html).


If it's going to be in a References block, then you don't need "See also"

antage · 2018-02-10T02:47:49Z

@asomers Fixed and squashed.

Susurrus

@asomers want to chime in here at all?

Susurrus · 2018-02-21T03:32:25Z

src/sys/stat.rs

@@ -122,3 +122,48 @@ pub fn fstatat<P: ?Sized + NixPath>(dirfd: RawFd, pathname: &P, f: AtFlags) -> R
    Ok(dst)
 }

+/// Change the file permission bits of the file named by the pathname.


Change to "Change the file permission bits of a file specified by a path"

Susurrus · 2018-02-21T03:33:45Z

src/sys/stat.rs

+    Errno::result(res).map(|_| ())
+}
+
+/// Change the file permission bits of the file specified by the file descriptor.


"Change the file permission bits of the file specified by the file descriptor." -> "Change the file permission bits of the file specified by a file descriptor"

Susurrus · 2018-02-21T03:44:33Z

src/sys/stat.rs

+    mode: Mode,
+    flag: AtFlags,
+) -> Result<()> {
+    let res = path.with_nix_path(|cstr| unsafe {


This function actually has 3 modes:

Path relative to the cwd of the calling process

Path relative to the specified directory

Absolute path

I'd therefore suggest replacing dirfd and path with the following:

pub enum PathMode { RelPath(P), RelPath(P, dirfd), AbsPath(P), }``` Then internally we can do the whole `AT_FDCWD` dance and also `debug_assert!` that those paths are correctly relative or absolute.

There's no point in providing both chmod and fchmodat(PathMode::AbsPath); they're identical. Also, I don't think we should force a programmatic distinction between absolute and relative paths, because there are likely to be situations where a Nix consumer doesn't know which type of path it's dealing with, without inspecting it. How about we provide chmod, fchmod, and fchmodat, where the latter asserts that fd is nonnegative?

asserts that fd is nonnegative

We can not because AT_FDCWD is negative integer usually.

I'm wondering if we can't wrap up these 3 functions and 5 modes into a different object that provides a nicer API than these functions separately? There is definite overlap between these functions, and I don't like the terrible combinatorics of the inputs to fchmodat. Any ideas here?

I think we shouldn't merge fchmod and chmod/fchmodat because these functions are very different (path vs fd as main argument).
We can get rid of chmod as it duplicates fchmodat and leave fchmodat with the signature:

pub fn fchmodat<P: ?Sized + NixPath>(path: P, mode: Mode, dirfd: Option<RawFd>, nofollow_symlink: bool) -> Result<()>

Here None in dirfd substitutes AT_FDCWD constant.

@asomers, @Susurrus The following is a summary of proposed changes:

Get rid of chmod function.

Add FchmodatFlag enum instead of AtFlags.

Change a signature of fchmodat function to: pub fn fchmodat<P: ?Sized + NixPath>(dirfd: Option<RawFd>, path: P, mode: Mode, flag: FchmodatFlag) -> Result<()>.

Do you have any objections or/and proposals?

Yes, that sounds good to me.

@Susurrus Any response?

@antage @Susurrus is AFK. Go ahead and make the changes we discussed, then I can approve the PR.

@asomers ok.

antage · 2018-03-23T16:39:05Z

@asomers Done the proposed changes.

asomers · 2018-03-23T16:58:06Z

src/sys/stat.rs

+/// then the mode of the symbolic link is changed.
+///
+/// `fchmod(None, path, mode, FchmodatFlags::FollowSymlink)` is identical to
+/// a call `libc::chmod(path, mode)`. It's reason why `chmod` is unimplemented


s/It's reason/That's/

asomers · 2018-03-23T16:58:58Z

src/sys/stat.rs

+///
+/// `fchmod(None, path, mode, FchmodatFlags::FollowSymlink)` is identical to
+/// a call `libc::chmod(path, mode)`. It's reason why `chmod` is unimplemented
+/// in `nix` crate.


s/in nix/in the nix/

asomers · 2018-03-23T17:02:24Z

test/test_stat.rs

+    let mut mode1 = Mode::empty();
+    mode1.insert(Mode::S_IRUSR);
+    mode1.insert(Mode::S_IWUSR);
+    fchmodat(Some(dirfd), &filename, mode1, FchmodatFlags::FollowSymlink).unwrap();


Here you're supplying an absolute path for filename. I think you should just supply the relative path, "foo.txt".

antage · 2018-03-23T18:34:58Z

@asomers Fixed.

asomers · 2018-03-24T15:46:52Z

Thanks, @antage. It all looks good now.

bors r+

857: Add chmod, fchmod, fchmodat functions r=asomers a=antage

bors · 2018-03-24T16:48:52Z

Build succeeded

asomers requested changes Feb 6, 2018

View reviewed changes

asomers reviewed Feb 9, 2018

View reviewed changes

Susurrus suggested changes Feb 21, 2018

View reviewed changes

asomers requested changes Mar 23, 2018

View reviewed changes

Add fchmod, fchmodat functions.

d315804

asomers approved these changes Mar 24, 2018

View reviewed changes

bors bot added a commit that referenced this pull request Mar 24, 2018

Merge #857

868fd3c

857: Add chmod, fchmod, fchmodat functions r=asomers a=antage

bors bot merged commit d315804 into nix-rust:master Mar 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add chmod, fchmod, fchmodat functions #857

Add chmod, fchmod, fchmodat functions #857

antage commented Feb 5, 2018

asomers Feb 6, 2018

asomers Feb 6, 2018

asomers Feb 6, 2018

asomers Feb 6, 2018

antage commented Feb 6, 2018

asomers left a comment

asomers Feb 9, 2018

antage commented Feb 10, 2018

Susurrus left a comment

Susurrus Feb 21, 2018

Susurrus Feb 21, 2018

Susurrus Feb 21, 2018

asomers Feb 21, 2018

antage Feb 21, 2018

Susurrus Feb 23, 2018

antage Feb 23, 2018

antage Feb 26, 2018

asomers Feb 27, 2018

antage Mar 11, 2018

asomers Mar 21, 2018

antage Mar 21, 2018

antage commented Mar 23, 2018

asomers Mar 23, 2018

asomers Mar 23, 2018

asomers Mar 23, 2018

antage commented Mar 23, 2018

asomers commented Mar 24, 2018

bors bot commented Mar 24, 2018

Add chmod, fchmod, fchmodat functions #857

Add chmod, fchmod, fchmodat functions #857

Conversation

antage commented Feb 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antage commented Feb 6, 2018

asomers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antage commented Feb 10, 2018

Susurrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antage commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antage commented Mar 23, 2018

asomers commented Mar 24, 2018

bors bot commented Mar 24, 2018

Build succeeded