Add renameat, AT_FDCWD #1097
Add renameat, AT_FDCWD #1097
Conversation
scottlamb
force-pushed
the
pr-renameat
branch
2 times, most recently
from
09a9198
to
856c842
Compare
src/fcntl.rs
Outdated
| pub fn renameat<P1: ?Sized + NixPath, P2: ?Sized + NixPath>(old_dirfd: RawFd, old_path: &P1, | ||
| new_dirfd: RawFd, new_path: &P2) | ||
| -> Result<()> { | ||
| let res = try!(try!(old_path.with_nix_path(|old_cstr| { |
There was a problem hiding this comment.
? is preferred over try!. It's supported by Nix's MSRV.
src/fcntl.rs
Outdated
| @@ -164,6 +167,26 @@ pub fn openat<P: ?Sized + NixPath>(dirfd: RawFd, path: &P, oflag: OFlag, mode: M | |||
| Errno::result(fd) | |||
| } | |||
|
|
|||
| pub fn rename<P1: ?Sized + NixPath, P2: ?Sized + NixPath>(old: &P1, new: &P2) -> Result<()> { | |||
There was a problem hiding this comment.
Is there any reason to use rename when renameat is supported? None that I know of.
There was a problem hiding this comment.
None that I know of either. I added it because I saw in other cases (such as open and openat directly above) both variants were there. Would you like me to remove it?
There was a problem hiding this comment.
Yeah, you should remove it. Supporting legacy APIs in existing code makes sense, but much like CloudABI, we shouldn't add brand new APIs that are already legacy.
There was a problem hiding this comment.
Done, and squashed my commits.
| renameat(old_dirfd, "old", new_dirfd, "new").unwrap(); | ||
| assert_eq!(renameat(old_dirfd, "old", new_dirfd, "new").unwrap_err(), | ||
| Error::Sys(Errno::ENOENT)); | ||
| close(old_dirfd).unwrap(); |
There was a problem hiding this comment.
How about a check that new_dir/"new" exists?
|
Thanks for the review! |
scottlamb
force-pushed
the
pr-renameat
branch
2 times, most recently
from
0f4a321
to
8824118
Compare
|
Sorry, but you'll need to rebase your branch on top of master, rather than merging master into your branch. |
|
Done. Sorry, not a real experienced git user. |
|
It's ok; none of us are experienced git users. Basically nobody is. But it looks like you rebased onto a too-old version of master, and now there's a conflict in the CHANGELOG. Could you please update your master and rebase again? |
|
Is this better? I did: I tried something a little different before: first I did a merge from github's UI, then when you pointed out I needed a merge, I tried a |
|
Hmm, actually, looks like #1058 had a different approach to |
renameat is (somewhat oddly, imho) in stdio.h. I put it in nix::fcntl because there's no nix::stdio and all its friends (including the AT_* constants) are in nix::fcntl.
|
Okay, now has an interface more consistent with #1058. |
Build succeeded
|
nix-rust/nix#1097 is merged so it does what we need now.
No description provided.