-
Notifications
You must be signed in to change notification settings - Fork 654
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 more *at functions. #562
Conversation
0c43c15
to
b7fa4c0
Compare
That's all folks. I have all methods for my fuse. |
98eb8a0
to
6077eef
Compare
@Mic92 this is heroic. I'm going to take a look over the weekend. Sorry for the delay! |
I retried the failing test and it works now so that's a plus. |
I would also add large file support to all methods in a second pull request (64bit fields on 32-bit systems), to align the with glibc/musl/rust. Ideally this should be done before the next release. |
☔ The latest upstream changes (presumably #536) made this pull request unmergeable. Please resolve the merge conflicts. |
This is great @Mic92, just needs a rebase by the looks of it (the curse of the changelog). The only thing I would add is that we have been trying to add docs to more of the functions we are adding. I do not think this is a blocker for getting this included but would be a nice bonus for this change or a future PR. E.g. Docs for fork: https://github.com/nix-rust/nix/blob/master/src/unistd.rs#L47. The main value is providing a quick link to relevant man pages and a quick overview (some of the names of the functions are somewhat cryptic). |
I have a doubt on this (and on other fn taking I'm not sure if it is a valid concern or not, but shouldn't all these functions take a |
A reference does not buy you anything: https://play.rust-lang.org/?gist=1739781faf61fcec4afaf65bbaadd968&version=stable&backtrace=0 |
Merge conflict resolved. Man page references added. |
src/unistd.rs
Outdated
@@ -356,6 +375,13 @@ pub fn getcwd() -> Result<PathBuf> { | |||
} | |||
} | |||
|
|||
// According to the POSIX specification, -1 is used to indicate that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment doesn't make sense here. Probably want to put it back in the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use this function in four different flavors of chown. Should I copied the comment to each of them? Why it does not make it sense here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you're referring to uid_t
and gid_t
which aren't used in this function. The description to this should be more broad (not referring to those specific use cases) and then you can put comments stating why you need to use this function at the call site that refers to specific variables.
src/sys/stat.rs
Outdated
@@ -53,6 +55,7 @@ bitflags! { | |||
} | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This extra line should be removed.
src/unistd.rs
Outdated
@@ -601,6 +672,69 @@ pub fn lseek64(fd: RawFd, offset: libc::off64_t, whence: Whence) -> Result<libc: | |||
Errno::result(res).map(|r| r as libc::off64_t) | |||
} | |||
|
|||
/// call the link function to create a link to a file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please capitalize the first word and add a period to the end of the first line of all these doc comments. If you run cargo doc --no-deps --open
you can easily review their appearance to make sure things look good there.
src/sys/stat.rs
Outdated
|
||
/// Change file timestamps with nanosecond precision | ||
/// (see [utimensat(2)](http://man7.org/linux/man-pages/man2/utimensat.2.html)) | ||
pub fn utimensat<P: ?Sized + NixPath>(dirfd: RawFd, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dirfd
and pathname
actually combo together to form a ternary input type: one allowing a RawFd
, another a relative path from the current working directory, and a third using an absolute path. I think we should use a ternary enum type here instead of the dirfd
and pathname
inputs to make this function call safer.
src/sys/stat.rs
Outdated
use sys::time::TimeSpec; | ||
|
||
pub enum UtimeSpec { | ||
Now, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This enum is public and as such needs docs.
2449a0c
to
b3a3a25
Compare
There a common theme through a lot of these functions of having a
I'm thinking an enum encompassing these states would be better instead of having two separate variables. I think an ideal solution would be to have |
test/test_stat.rs
Outdated
use std::os::unix::prelude::AsRawFd; | ||
use tempfile::NamedTempFile; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put non-std/core imports down in their own group.
src/sys/stat.rs
Outdated
Now, | ||
/// The corresponding file timestamp is left unchanged. | ||
Omit, | ||
Time(TimeSpec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one needs a comment too.
src/sys/stat.rs
Outdated
Time(TimeSpec) | ||
} | ||
|
||
fn to_timespec(time: &UtimeSpec) -> libc::timespec { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a doccomment for this function? Nix devs should know if they can or should use it elsewhere in this function.
CHANGELOG.md
Outdated
- Added `nix::unistd::{openat, fstatat, readlink, readlinkat, rename, renameat, mknodat}` | ||
([#551](https://github.com/nix-rust/nix/pull/551)) | ||
- Added `nix::unistd::{openat, fstatat, readlink, readlinkat, rename, renameat, mknodat, unlinkat}` | ||
([#497](https://github.com/nix-rust/nix/pull/551)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the PR numbering change he was intentional.
61a42c0
to
6aa502e
Compare
@Susurrus do you want to implement that enum for these functions send me a pull request? I don't see how Rust's typesystem is expressive enough to ensure a path is always relative/absolute without runtime checks. |
My suggestion is to have a: pub enum AtPath {
AbsPath(NixPath),
RelativeTo(RawFd, NixPath),
RelativeCwd(NixPath),
} Now we could add runtime checks through having some conversion path from |
I wanted to point out that I also preferred the arguments @bugaevc who was working on pub enum AccessFlags {
FileExists,
FileExistsAndPermissions(PermFlags),
} where |
@Mic92 I wanted to circle back to this and see if we could get this merged? I'm working on some stuff in fcntl but it should get merged within a day or so. Then I'd like to get this PR merged if possible. Still willing to push this through? |
I am back from holiday on Thursday. |
What happened to this? |
I made multiple pull requests out of it. |
No description provided.