Implement unlinkat by jlb6740 · Pull Request #1058 · nix-rust/nix

jlb6740 · 2019-05-21T23:59:29Z

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

Changes made based on comments from pull request nix-rust#1058

jlb6740 · 2019-05-28T21:20:29Z

@asomers I am a little uncertain as to the github way to amend a pull request. Instead of amending and doing a force push I instead followed some fairly old instructions here https://stackoverflow.com/questions/9790448/how-to-update-a-pull-request-from-forked-repo and pushed the patch. I am able to see this second commit. I assume this is all part of the same original pull request but please advise if there is more that I should do to have this latest patch reviewed. Thanks.

asomers · 2019-05-28T23:38:18Z

The easiest way is to amend and/or rebase your changes locally, and then do a force push, like this:

git checkout -b mybranch
# Edit some files
git commit
git push -u git@github.com:myusername/myproject.git
# Edit some more
git amend
git push -f

jlb6740

Previous concerns are hopefully addressed here but let me know if there are more comments.

jlb6740 · 2019-05-29T18:35:33Z

@asomers .. Ok. I was afraid that would erase comment history asking for the update but I wasn't sure and also I wasn't sure if it mattered to the project. The update ce88edd is here but I don't see a way to ask for a new pull request so I assume you can see and are notified of it? Let me know if you need me to somehow resubmit. Thanks.

asomers · 2019-05-30T00:23:30Z


+
+#[test]
+#[should_panic]


You should the expected("whatever") string to should_panic to ensure that it's only catching the desired panic, and no others.

Hi ... Thanks for the tip. Update #3 includes this change.

asomers · 2019-05-30T00:23:54Z

+
+    // Attempt unlink dir at relative path without proper flag
+    unlinkat(Some(dirfd), dirname, UnlinkatFlags::NoRemoveDir).unwrap();
+    assert!(dirpath.exists());


This line is dead code, assuming the line above panics.

Thanks. You are right. Not sure why I left this in but it is now removed.

asomers · 2019-05-30T00:25:14Z

 - Add `forkpty`
  ([#1042](https://github.com/nix-rust/nix/pull/1042))
+- Add `unlinkat`
+  ([#TBD](https://github.com/nix-rust/nix/pull/TBD))


Don't forget to replace the TBD.

You missed a TBD.

Changes made based on comments from pull request nix-rust#1058

asomers · 2019-06-11T02:47:40Z

+    let dirfd = fcntl::open(tempdir.path(), fcntl::OFlag::empty(), stat::Mode::empty()).unwrap();
+
+    // Attempt unlink dir at relative path without proper flag
+    unlinkat(Some(dirfd), dirname, UnlinkatFlags::NoRemoveDir).unwrap();


You don't need should_panic for this. Just replace the unwrap with an unwrap_err and check the result.

jlb6740 · 2019-06-24T20:28:58Z

@asomers I tried to be more specific by matching on the error message, but there is a discrepancy in the error returned by i686-appledarwin and x86_64-apple-darwin "Sys(EPERM)", and every other target being tested Sys(EISDIR) https://travis-ci.org/nix-rust/nix/builds/541975264?utm_source=github_status&utm_medium=notification. I will resubmit and have to be a less specific about the type of error expected. Not sure if this indicates a bug on those apple targets or not?

asomers

This is not a case of Apple being wrong, this is a case of POSIX being stupid. EPERM is the error that POSIX says should be returned in this case. But it looks like both FreeBSD And Linux choose to return EISDIR instead, which makes more sense. I suggest that your test should accept either error.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/unlink.html

jlb6740 · 2019-06-24T21:23:42Z

I see. Expecting either is the better option but it's not clear to me the expect way to account for this when assigning a string to expected for the match. Any pointer?

This is not a case of Apple being wrong, this is a case of POSIX being stupid. EPERM is the error that POSIX says should be returned in this case. But it looks like both FreeBSD And Linux choose to return EISDIR instead, which makes more sense. I suggest that your test should accept either error.
https://pubs.opengroup.org/onlinepubs/9699919799/functions/unlink.html

asomers · 2019-06-24T21:49:39Z

It's easy if you do as I suggested earlier and use unwrap_err instead of should_panic.

jlb6740 · 2019-06-24T22:06:45Z

This is way too complicated. The right way to test code that ought to panic is with the #[should_panic()] attribute. See sys/test_aio.rs for an example.
You should the expected("whatever") string to should_panic to ensure that it's only catching the desired panic, and no others.

I used should_panic and expected based on your suggests. I am just revisiting this patch today and didn't even notice that latest comment ... I went right to understanding the failure that precedes it. I'll change to using un_wrap error, but I'd like some learnings here. Is this only being suggested now because we realize we want to match against multiple potential error messages or should I have avoided should_panic from the beginning? In general when do you suggest using should_panic? I noticed a few other tests in this folder that use the syntax, but not many have elected to.

asomers · 2019-06-24T22:18:41Z

It's true. When you first posted this PR, I didn't see the forest for the trees. The std::panic::catch_unwind was such a big red flag that I failed to realize that the test didn't even need to panic at all. The takeaway is:

should_panic is usually the correct way to test code that ought to panic. std::panic::catch_unwind should rarely be used unless you're doing something like implementing should_panic.
The best way to test code that returns an error is to simply look at the error it returns. Asserting that it returned ok (which is what unwrap does) is roundabout and overkill. In tests/sys/test_aio.rs I used should_panic because the panics I was trying to exercise lay in the crate's source, in src/sys/aio.rs. But that's not the case here. Your panic lays in the test. It's the unwrap statement that's panicing.

jlb6740 · 2019-06-25T02:05:55Z

@asomers Ok, thanks. So to recap from the two bullets .. the first is saying generally prefer using should_panic, while the second bullet is saying but when it it's possible to just compare the error directly then to do that? It still seems a little ambigious considering I thought the point of "expected" was to compare the error message though in this specific case it is not as helpful since we'd like to accept multiple error values. If those apple targets had returned EISDIR the same as FreeBSD and Linux (or they all returned EPERM) I am still unsure what would be the preferred way to check the error? In any case I made an update here: https://gist.github.com/jlb6740/9848944cbab4a1ab5165a3f5ee2b3e1f. If it looks ok, I'll push this and update the CHANGELOG as well.

Thanks.

asomers · 2019-06-25T02:24:09Z

The reason why this test shouldn't use should_panic is because the code that you're trying to test doesn't panic. It simply returns an error. So the best way to test that is to just check the error code. Don't make the test more complicated than it needs to be.

jlb6740 · 2019-06-25T17:00:56Z

The reason why this test shouldn't use should_panic is because the code that you're trying to test doesn't panic. It simply returns an error. So the best way to test that is to just check the error code. Don't make the test more complicated than it needs to be.

Ok ... Thanks. I think I see your point. I'll go ahead and resubmit. Any more suggestion just let me know. Thanks for your patients.

Changes made based on comments from pull request nix-rust#1058

jlb6740 · 2019-06-25T19:58:59Z

@asomers Github is showing me a "Changes requested" message but I think the previous requests have been addressed. Clicking "resolve conversations" doesn't seem to get rid of the message .. not sure what github wants from me to make the message go away. If there is anything else you see amiss just let me know. My latest update seemed ok, but I did need a rebase in order to pass all tests.

jlb6740 · 2019-07-09T21:00:26Z

Hi @asomers,

Was there anything left here you'd like updated? I think commit 4 should have resolved the remaining requests?

asomers · 2019-07-09T21:22:56Z

  ([#1042](https://github.com/nix-rust/nix/pull/1042))
 - Add `sched_yield`
  ([#1050](https://github.com/nix-rust/nix/pull/1050))
+- Add `unlinkat`


Your changelog entry got dislocated during a rebase.

Hi ... what do you mean by dislocated? For the latest patch this is the change I intended for changelog.md:

- ([#TBD](https://github.com/nix-rust/nix/pull/1058)) + ([#1058](https://github.com/nix-rust/nix/pull/1058))

Is this what you can see? Is there another push needed for this file?

asomers · 2019-07-09T21:24:30Z

+    // Attempt unlink dir at relative path without proper flag
+    let err_result = unlinkat(Some(dirfd), dirname, UnlinkatFlags::NoRemoveDir).unwrap_err();
+    match err_result {
+        Error::Sys(Errno::EISDIR) => println!("{}", err_result),


passing tests shouldn't print anything. Just do assert!(err_result == Error::Sys(Errno::EISDIR) || err_result == Error::Sys(Errno::EPERM))

I see .. good catch/comment. I've corrected this in the 5th commit.

jlb6740 · 2019-07-10T18:32:18Z

@asomers I've made changes to test/test_unistd.rs .. but maybe didn't address CHANGELOG.md comment? Let me know. Thanks.

asomers · 2019-07-10T18:40:02Z

The changelog entry is in the section for release 0.14. It should be in the unreleased section.

jlb6740 · 2019-07-10T20:25:46Z

The changelog entry is in the section for release 0.14. It should be in the unreleased section.

Ahh .. I see. I've moved it to the "Added" part of the "Unreleased" section. Thanks.

asomers

Just remove the extra newlines and squash your commits, then I think we can merge this.

asomers · 2019-07-11T16:50:37Z

+    assert!(!filepath.exists());
+ }
+
+


Too many newlines here.

Hi @asomers .. Done. Last commit should address this.

asomers · 2019-07-11T20:16:14Z

There are conflicts now that you must resolve.

Changes made based on comments from pull request nix-rust#1058

jlb6740 · 2019-07-13T20:29:40Z

I've rebased and pushed ... Looks like the latest push is passing all tests.

asomers · 2019-07-13T21:15:52Z

You still haven't squashed your commits, though.

jlb6740 · 2019-07-14T21:54:23Z

You still haven't squashed your commits, though.

Ok ... I am familiar with the best way to push and merge patches to gerrit but obviously I am still new to github. I was thinking my commits would be squashed automatically when you merged here. Is the work flow to squash on my local branch and then push here (i.e. git rebase -i HEAD~7), or does github provide some UI for this. I am seeing this: https://help.github.com/en/articles/about-pull-request-merges#squash-and-merge-your-pull-request-commits but I can't find this button or tell if it is for me or the person doing the merging?

asomers · 2019-07-14T21:57:05Z

Many projects squash commits when they merge the PR. However, Nix uses the bors merge bot, so that isn't possible for us. Instead, you must squash yourself.

This adds the unlinkat function, which is part of POSIX: http://pubs.opengroup.org/onlinepubs/9699919799/functions/unlinkat.html and widely implmented on Unix-family platforms. Implement unlinkat - Update comments to unlink at and breakup tests Changes made based on comments from pull request nix-rust#1058 Implement unlinkat - Update should_panic for more precise check Updae should_panic attribute for more precise check. Also remove some dead code and note the pull request ID. Implement unlinkat - Update error handling to use unwrap_err The previous patch failed testing on two targets that returned EPERM as the Sys error instead of EISDIR. This patch changes from using the should_panic attribute to using the unwrap_err and matching against the result. Implement Unlinkat - Passing tests should not print anything. Fix. Implement unlinkat - Update location of commit comment in the CHANGELOG Implement unlinkat - Remove newline

jlb6740 · 2019-07-14T22:08:15Z

Many projects squash commits when they merge the PR. However, Nix uses the bors merge bot, so that isn't possible for us. Instead, you must squash yourself.

You respond quick .. thanks. Ok. So do a git rebase -i and then do a force push. I'll give that a try.
Thanks!

jlb6740 · 2019-07-16T17:19:22Z

@asmoers Hi .. Not sure how bors works or if anything else is needed on my end for the pull request. Let me know. I'll be starting another api, mimicking what's been done here.

asomers

bors r+

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

bors · 2019-07-16T17:50:08Z

Build succeeded

continuous-integration/travis-ci/push
FreeBSD 11.2

This adds the unlinkat function, which is part of POSIX: http://pubs.opengroup.org/onlinepubs/9699919799/functions/unlinkat.html and widely implmented on Unix-family platforms. Implement unlinkat - Update comments to unlink at and breakup tests Changes made based on comments from pull request nix-rust#1058 Implement unlinkat - Update should_panic for more precise check Updae should_panic attribute for more precise check. Also remove some dead code and note the pull request ID. Implement unlinkat - Update error handling to use unwrap_err The previous patch failed testing on two targets that returned EPERM as the Sys error instead of EISDIR. This patch changes from using the should_panic attribute to using the unwrap_err and matching against the result. Implement Unlinkat - Passing tests should not print anything. Fix. Implement unlinkat - Update location of commit comment in the CHANGELOG Implement unlinkat - Remove newline

asomers requested changes May 22, 2019

View reviewed changes

Comment thread src/unistd.rs Outdated

Comment thread test/test_unistd.rs Outdated

Comment thread test/test_unistd.rs

jlb6740 added a commit to jlb6740/nix that referenced this pull request May 28, 2019

Implement unlinkat - Update comments to unlink at and breakup tests

ce88edd

Changes made based on comments from pull request nix-rust#1058

jlb6740 commented May 29, 2019

View reviewed changes

asomers requested changes May 30, 2019

View reviewed changes

jlb6740 force-pushed the implement_unlinkat branch from ce88edd to dd9fdd5 Compare June 5, 2019 21:43

jlb6740 added a commit to jlb6740/nix that referenced this pull request Jun 5, 2019

Implement unlinkat - Update comments to unlink at and breakup tests

16ad363

Changes made based on comments from pull request nix-rust#1058

asomers requested changes Jun 11, 2019

View reviewed changes

asomers requested changes Jun 24, 2019

View reviewed changes

Comment thread CHANGELOG.md Outdated

jlb6740 force-pushed the implement_unlinkat branch 2 times, most recently from d45da51 to 8d7467e Compare June 25, 2019 19:06

jlb6740 added a commit to jlb6740/nix that referenced this pull request Jun 25, 2019

Implement unlinkat - Update comments to unlink at and breakup tests

f6bce53

Changes made based on comments from pull request nix-rust#1058

asomers reviewed Jul 9, 2019

View reviewed changes

asomers reviewed Jul 11, 2019

View reviewed changes

jlb6740 force-pushed the implement_unlinkat branch from cfa7003 to 6f86db1 Compare July 11, 2019 23:57

jlb6740 added a commit to jlb6740/nix that referenced this pull request Jul 11, 2019

Implement unlinkat - Update comments to unlink at and breakup tests

44535fa

Changes made based on comments from pull request nix-rust#1058

jlb6740 force-pushed the implement_unlinkat branch from 6f86db1 to bcb557b Compare July 13, 2019 03:24

jlb6740 added a commit to jlb6740/nix that referenced this pull request Jul 13, 2019

Implement unlinkat - Update comments to unlink at and breakup tests

a1b46c3

Changes made based on comments from pull request nix-rust#1058

jlb6740 force-pushed the implement_unlinkat branch from bcb557b to 273fb63 Compare July 14, 2019 22:11

asomers approved these changes Jul 16, 2019

View reviewed changes

bors bot merged commit 273fb63 into nix-rust:master Jul 16, 2019

scottlamb mentioned this pull request Jul 17, 2019

Add renameat, AT_FDCWD #1097

Merged



		#[test]
		#[should_panic]

Conversation

jlb6740 commented May 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jlb6740 commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asomers commented May 28, 2019

Uh oh!

jlb6740 left a comment

Choose a reason for hiding this comment

Uh oh!

jlb6740 commented May 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlb6740 commented Jun 24, 2019

Uh oh!

asomers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jlb6740 commented Jun 24, 2019

Uh oh!

asomers commented Jun 24, 2019

Uh oh!

jlb6740 commented Jun 24, 2019

Uh oh!

asomers commented Jun 24, 2019

Uh oh!

jlb6740 commented Jun 25, 2019

Uh oh!

asomers commented Jun 25, 2019

Uh oh!

jlb6740 commented Jun 25, 2019

Uh oh!

jlb6740 commented Jun 25, 2019

Uh oh!

jlb6740 commented Jul 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlb6740 Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlb6740 commented Jul 10, 2019

Uh oh!

asomers commented Jul 10, 2019

Uh oh!

jlb6740 commented Jul 10, 2019

Uh oh!

asomers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asomers commented Jul 11, 2019

Uh oh!

jlb6740 commented May 28, 2019 •

edited

Loading

jlb6740 Jul 10, 2019 •

edited

Loading