Implment linkat #1101

jlb6740 · 2019-07-25T02:07:24Z

This adds the linkat function which is part of POSIX:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/linkat.html
and widely implmented on Unix-Family platforms.

jlb6740 · 2019-07-25T02:11:59Z

test/test_unistd.rs

+    linkat(Some(dirfd), symoldfilename, Some(dirfd), newfilename, LinkatFlags::SymlinkFollow).unwrap();
+
+    let err_result =  readlink(&newfilepath, &mut buf).unwrap_err();
+    assert!(err_result == Error::Sys(Errno::EINVAL));


Could not find a convenient way to tell I've created a hard link and not another soft link so I simply try to follow the hardlink and when that fails check the error.

It's easy to tell. Just stat both old file and new file.

Thanks ... yesterday I googled "rust stat file" and using std::fs::metadata came up. When I compared the metadata for the original file, the symlink, and the hardlink the metadata all looked the same. I then found something for libc::stat but it was describing a struct. I didn't notice there was also a function call of the same name available. Following rust-lang/libc#403 I've been able to see the filetype and number of hardlinks for a file through stat and will post this update.

jlb6740 · 2019-07-25T21:15:47Z

@asomer Another example where mac is doing something different than all other targets. You have any thoughts on what flag it is expecting?

I am testing not following the symlink. http://pubs.opengroup.org/onlinepubs/9699919799/functions/linkat.html

Says to use

AT_SYMLINK_FOLLOW
If path1 names a symbolic link, a new link for the target of the symbolic link is created.

If I want to test creating a hardlink to the symbolic link I simply don't add this flag. On my linux host I can pass 0 for example. In NIX I pass AtFlags::empty().

I found this first thread from a few years back that says it is not supported:
https://discussions.apple.com/thread/5759427

And I couldn't find it here:
https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/

But another link I see this reference here:
https://www.unix.com/man-page/mojave/2/linkat/

If linkat() is passed the special value AT_FDCWD in the fd1 or fd2 parameter, the current working directory is used for the respective name
argument. If both fd1 and fd2 have value AT_FDCWD, the behavior is identical to a call to link(). Unless flag contains the
AT_SYMLINK_FOLLOW flag, if name1 names a symbolic link, a new link is created for the symbolic link name1 and not its target. On OS X, not
assigning AT_SYMLINK_FOLLOW to flag may result in some filesystems returning an error.

where it is said not assigning anything results in an error. Any comments on what I should be adding as a FLAG? The last sentence makes it sound like this is inconsistently supported on mac and that some systems will only accept this call if AT_SYMLINK_FOLLOW is passed to it?

jlb6740 · 2019-07-28T23:47:05Z

@asomer In this scenario can we just a check that allows ENOTSOP error or should I just get rid of this test all together and comment that NOSYMLINKFOLLOW is not supported for all targets?

asomers · 2019-07-31T20:54:59Z

Since the OSX man page documents that AT_SYMLINK_FOLLOW is required, you should just disable that test on OSX.

jlb6740 · 2019-07-31T21:53:48Z

Since the OSX man page documents that AT_SYMLINK_FOLLOW is required, you should just disable that test on OSX.

Ok, make sense. What is the process for doing that .. I guess manually triggering this patch to be retested while omitting specific targets?

jlb6740 · 2019-07-31T21:56:26Z

Since the OSX man page documents that AT_SYMLINK_FOLLOW is required, you should just disable that test on OSX.

Ok, make sense. What is the process for doing that .. I guess manually triggering this patch to be retested while omitting specific targets?

Ahh .. I see a travis.yml. I'll try to modify that!

jlb6740 · 2019-07-31T22:07:07Z

Actually .. will

Since the OSX man page documents that AT_SYMLINK_FOLLOW is required, you should just disable that test on OSX.

Ok, make sense. What is the process for doing that .. I guess manually triggering this patch to be retested while omitting specific targets?

Ahh .. I see a travis.yml. I'll try to modify that!

I see modifying the travis.xml would disable all tests for the target. I see I should just add an attribute based guard as is done on another test.

#[cfg(not(any(target_os = "ios", target_os = "macos")))]

jlb6740 · 2019-08-01T16:46:36Z

@asomers Ok, cool. Tests passed and/or were excluded as expected. It think this is ready but needs review.

src/unistd.rs

jlb6740 · 2019-08-12T17:17:37Z

Thanks ... Let me know if there is anything else.

src/unistd.rs

asomers · 2019-08-13T14:52:56Z

src/unistd.rs

+/// with file descriptor `olddirfd` instead of the current working directory and similiarly for 
+/// `newpath` and file descriptor `newdirfd`. In case `flag` is LinkatFlags::SymlinkFollow and 
+/// `oldpath` names a symoblic link, a new link for the target of the symbolic link is created.
+/// The function shall fail if either olddirfd or newdirfd is None, returning ENOENT and EEXIST 


WTF? No it shall not fail. That's not what will happen.

WTF? No it shall not fail. That's not what will happen.

What should happen? The spec I am referring to is here: http://pubs.opengroup.org/onlinepubs/9699919799/functions/linkat.html

Failing is language the spec uses in the ERRORS section to describe errors returned. i.e.

ERRORS
These functions shall fail if:
[EEXIST]
[ENOENT]

When I tested by inserting "None" at the calls in test_linkat_file, ENOENT and EEXIST were returned.

Please comment on what you think should happen. If failing with ENOENT and EEXIST is not correct behavior then an update to the patch is needed.

What should happen when olddirfd is None is that you call libc::linkat with the AT_FDCWD argument, which the operating system interprets to mean that oldpath is relative to the current directory. Similarly with newdirfd.

@asomers Ok .. I see, thanks. I saw this mentioned and read the spec to mean this special value could be passed in directly by the user. However in case of None, instead of returning an error, we could define behavior such that this value is used in this case as well. I'll make this update and use newdirfd as a reference. Thanks.

No, Johnnie. The code that you wrote already uses AT_FDCWD. How did you not notice that?

Hi @asomers Revisit this patch .. to your last comment, code that I wrote doesn't use it but code that I call "at_rawfd" does. This is not seen unless you peak into the implementation of at_rawfd. But my comment was just to have a quick reply and thank you for your suggestion.. I did not peak into at_rawfd beforehand to see this and interpret this. Also keep in mind, for me when contributing some small functions and test cases for nix I am not only doing this to help nix and projects that use it, but also it is being done for me as a good introduction to Rust. That is to say interpreting Rust syntax and semantics is nowhere close to second nature yet for me so please do not read too much into things that may be obvious to you.

But to recap the change request, you initially asked if I could update the comments for linkat on what happens when None is passed as a file descriptor .. a good suggestion. To see, I modifying the test for test_linkat_file to observe what actually happens. In doing so, I saw an error was returned and wrote that in the comments, but only after reviewing the docs at opengroup.org and not seeing explicit direction on what should happen or any contradiction to what I actually observed. Your response ultimately was that AT_FDCWD should be passed, and in peeking into at_rawfd I see it already does this. So interpreting what this is supposed to mean I think I was misled by my testing experiment and should not have interpreted the returned errors the way I did. That said, I still am not sure why something like this fails:

Line: 620
linkat(None, oldfilename, Some(dirfd), newfilename, LinkatFlags::SymlinkFollow).unwrap();
or
Line: 620
linkat(Some(dirfd), oldfilename, None, newfilename, LinkatFlags::SymlinkFollow).unwrap();

fails even if I create a foo.txt in the cur directory (to cover the first example).

echo "foo" > foo.txt
cargo test test_linkat_file

Do you have insight into what I am missing here and how to test the behavior for None assuming the implementation in unistd.rs is correct?

Do you know how to use rust-gdb and strace? If not, then you should learn those tools. They'll help you to debug your problem.

@asomers Thanks for the suggestion. The comment has been updated. Perhaps the patch is good now, but let me know if you'd like more changes. Thanks.

Hi @asmoers .. I am not sure the etiquette concerning who marks "Resolve conversation" for change requests such as this. Do you have more comment for this request?

jlb6740 · 2019-08-13T18:46:09Z

You still haven't removed them. Please don't mark conversations as resolved unless you've actually resolved them.

@asomers The change must have been unknowingly reverted before I pushed. I believe the newline is gone now but I no longer see the conversation to close. Does that happen in the github UI when an issue such as a newline is resolved as expected?

asomers

How about a test that sets olddirfd and newdirfd to None?

asomers · 2019-10-12T18:08:55Z

src/unistd.rs

+/// with file descriptor `olddirfd` instead of the current working directory and similiarly for
+/// `newpath` and file descriptor `newdirfd`. In case `flag` is LinkatFlags::SymlinkFollow and
+/// `oldpath` names a symoblic link, a new link for the target of the symbolic link is created.
+/// If either olddirfd or newdirfd is None, AT_FDCWD is used respectively where pathname is then


Need backticks on this line. Also, "pathname" should be "oldpath" and/or "newpath".

asomers · 2019-10-12T18:15:03Z

test/test_unistd.rs

+    assert!(newfilepath.exists());
+}
+
+#[cfg(target_os = "freebsd")]


Why is this test restricted to FreeBSD?

Hi @asmoers .. I am not sure the etiquette concerning who marks "Resolve conversation" for change requests such as this. Do you have more comment for this request?

jlb6740 · 2019-10-16T23:27:12Z

@asomers Thanks for the suggestions. I think they've been handled. In particular I added tests for the cases of setting olddirfd and newdirfd to None. I think the test cover the input as we'd like, but if you have something else in mind I can definitely make a change no problem. I also updated the CHANGELOG file. Let me know anything else you'd like to see. Thanks.

asomers · 2019-10-19T14:29:22Z

src/unistd.rs

+/// with file descriptor `olddirfd` instead of the current working directory and similiarly for
+/// `newpath` and file descriptor `newdirfd`. In case `flag` is LinkatFlags::SymlinkFollow and
+/// `oldpath` names a symoblic link, a new link for the target of the symbolic link is created.
+/// If either `olddirfd` or `newdirfd` is None, AT_FDCWD is used respectively where `oldpath`


Missing backticks for "None" and "AT_FDCWD"

asomers · 2019-10-19T14:29:36Z

src/unistd.rs

+/// `oldpath` names a symoblic link, a new link for the target of the symbolic link is created.
+/// If either `olddirfd` or `newdirfd` is None, AT_FDCWD is used respectively where `oldpath`
+/// and/or `newpath` is then interpreted relative to the current working directory of the calling
+/// process. If either `oldpath` or `newpath` is absolute, then dirfd is ignored.


Missing backticks for dirfd

Ok. Guess I was only adding backticks for the parameter names. Thanks for the correction. Updated now.

asomers · 2019-10-26T21:52:18Z

Where did all of the whitespace changes come from? They seem unrelated. If you're going to modify whitespace that's unrelated to your PR, it needs to be in a separate commit (not necessarily a separate PR). Also, though it's too late now, in the future please don't force-push until after review is complete and I ask you to squash.

jlb6740 · 2019-10-30T01:33:05Z

Also, though it's too late now, in the future please don't force-push until after review is complete and I ask you to squash.

Ok, will do.

At some point I've changed IDE settings to remove trailing white spaces to adhere to standards for a different project. In that previous push I removed a few trailing whitespaces that existed for an unrelated function as you pointed out. In the latest commit I added the whitespace back. I am thinking that is the change you wanted but if it is not then please let me know. Thanks.

And btw .. the force-push you see for the latest commit is just my text editor also refusing to allow me to keep trailing whitespace, so I had a few tries at it.

asomers · 2019-10-30T02:54:38Z

Ok, it looks good. Now you can squash and force-push.

This adds the linkat function which is part of POSIX: http://pubs.opengroup.org/onlinepubs/9699919799/functions/linkat.html and widely implmented on Unix-Family platforms. Add back trailing whitespace removed on previous force push

jlb6740 · 2019-11-04T06:19:52Z

Ok, it looks good. Now you can squash and force-push.

Ok, thanks a lot. Hopefully this is good to go.

asomers

bors r+

1101: Implment linkat r=asomers a=jlb6740 This adds the linkat function which is part of POSIX: http://pubs.opengroup.org/onlinepubs/9699919799/functions/linkat.html and widely implmented on Unix-Family platforms. Co-authored-by: Johnnie Birch <45402135+jlb6740@users.noreply.github.com>

bors · 2019-11-17T00:00:25Z

Build succeeded

continuous-integration/travis-ci/push
FreeBSD 11.2

jlb6740 commented Jul 25, 2019

View reviewed changes

jlb6740 force-pushed the implement_linkat branch 3 times, most recently from 4a9c4fe to 5002f7a Compare July 25, 2019 20:49

jlb6740 force-pushed the implement_linkat branch 2 times, most recently from 4a45294 to 6b00bf6 Compare July 31, 2019 22:05

asomers reviewed Aug 7, 2019

View reviewed changes

src/unistd.rs Outdated Show resolved Hide resolved

src/unistd.rs Show resolved Hide resolved

jlb6740 force-pushed the implement_linkat branch from 6b00bf6 to 820ab8c Compare August 12, 2019 17:14

asomers requested changes Aug 13, 2019

View reviewed changes

jlb6740 force-pushed the implement_linkat branch from 820ab8c to 9326d7e Compare August 13, 2019 18:40

zmlcc mentioned this pull request Sep 29, 2019

Add support for *at functions #1132

Open

14 tasks

jlb6740 force-pushed the implement_linkat branch 2 times, most recently from 74046ca to a27248b Compare October 7, 2019 20:15

asomers requested changes Oct 12, 2019

View reviewed changes

jlb6740 force-pushed the implement_linkat branch 5 times, most recently from 99d58a3 to b8ffb63 Compare October 16, 2019 21:10

asomers reviewed Oct 19, 2019

View reviewed changes

jlb6740 force-pushed the implement_linkat branch from b8ffb63 to ea75be9 Compare October 23, 2019 16:58

jlb6740 force-pushed the implement_linkat branch 3 times, most recently from 5f03caa to 854ee25 Compare October 30, 2019 01:29

jlb6740 force-pushed the implement_linkat branch 3 times, most recently from 2d22219 to b83cb8c Compare November 4, 2019 05:41

Implment linkat

848825b

This adds the linkat function which is part of POSIX: http://pubs.opengroup.org/onlinepubs/9699919799/functions/linkat.html and widely implmented on Unix-Family platforms. Add back trailing whitespace removed on previous force push

jlb6740 force-pushed the implement_linkat branch from b83cb8c to 848825b Compare November 4, 2019 06:19

asomers approved these changes Nov 16, 2019

View reviewed changes

bors bot merged commit 848825b into nix-rust:master Nov 17, 2019

Implment linkat #1101

Implment linkat #1101

Conversation

jlb6740 commented Jul 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlb6740 commented Jul 25, 2019 • edited

jlb6740 commented Jul 28, 2019

asomers commented Jul 31, 2019

jlb6740 commented Jul 31, 2019

jlb6740 commented Jul 31, 2019

jlb6740 commented Jul 31, 2019 • edited

jlb6740 commented Aug 1, 2019

jlb6740 commented Aug 12, 2019

Choose a reason for hiding this comment

jlb6740 Aug 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlb6740 Aug 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlb6740 Aug 28, 2019 • edited

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

jlb6740 commented Aug 13, 2019 • edited

asomers 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

jlb6740 commented Oct 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlb6740 Oct 23, 2019 • edited

Choose a reason for hiding this comment

asomers commented Oct 26, 2019

jlb6740 commented Oct 30, 2019 • edited

asomers commented Oct 30, 2019

jlb6740 commented Nov 4, 2019

asomers left a comment

Choose a reason for hiding this comment

bors bot commented Nov 17, 2019

Build succeeded

jlb6740 commented Jul 25, 2019 •

edited

jlb6740 commented Jul 31, 2019 •

edited

jlb6740 Aug 13, 2019 •

edited

jlb6740 Aug 13, 2019 •

edited

jlb6740 Aug 28, 2019 •

edited

jlb6740 commented Aug 13, 2019 •

edited

jlb6740 Oct 23, 2019 •

edited

jlb6740 commented Oct 30, 2019 •

edited