improve acl_get / acl_set error handling (1.4-maint) #8125
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## 1.4-maint #8125 +/- ##
=============================================
+ Coverage 82.61% 82.84% +0.23%
=============================================
Files 38 38
Lines 10973 10981 +8
Branches 2100 2102 +2
=============================================
+ Hits 9065 9097 +32
+ Misses 1345 1326 -19
+ Partials 563 558 -5 ☔ View full report in Codecov by Sentry. |
ThomasWaldmann
force-pushed
the
acl-error-handling-1.4
branch
from
8ad150b to
8b0ef42
Compare
ThomasWaldmann
force-pushed
the
acl-error-handling-1.4
branch
from
1dcdb29 to
96aba25
Compare
ThomasWaldmann
force-pushed
the
acl-error-handling-1.4
branch
2 times, most recently
from
6fc86db to
49539a4
Compare
ThomasWaldmann
force-pushed
the
acl-error-handling-1.4
branch
4 times, most recently
from
86c1bf2 to
be8e9b7
Compare
Also did a small structural refactors there.
- ACLs are not working, if ENOTSUP ("Operation not supported") happens
- fix check for macOS
On macOS borg uses "acl_extended", not "acl_access" and
also the ACL text format is a bit different.
- remove unused global / import - use is_linux and is_darwin - rename darwin acl test method
ThomasWaldmann
force-pushed
the
acl-error-handling-1.4
branch
from
be8e9b7 to
a538013
Compare
but do not silence other OSErrors.
Previously, these conditions were handled the same (just return): - no extended acl here - some error happened (e.g. ACLs unsupported, bad file descriptor, file not found, permission error, ...) Now there will be OSErrors for the error cases.
This is NOT a bug fix, because the previous code contained a check for symlinks before that line - because symlinks can not have ACLs under Linux. Now, this "is it a symlink" check is removed to simplify the code and the "nofollow" variant of acl_extended_file* is used to look at the symlink fs object (in the symlink case). It then should tell us that this does NOT have an extended ACL (because symlinks can't have ACLs) and so we return there. Overall the code gets simpler and looks less suspect.
We use path when raising OSErrors, even if we have an fd.
... to implement same semantics as on linux (only store ACL if it defines permissions other than those defined by the traditional file permissions). Looks like there is no call working with an fd on FreeBSD.
Previously: - acl_get just returned for lpathconf returning EINVAL - acl_get silently ignored all other lpathconf errors and implied it is not a NFS4 acl Now: - not sure why the EINVAL silent return was done, but it seems wrong. guess it could be the system not implementing a check for nfs4. but in that case guess we still would like to get the default and access ACL!? Thus, I removed the silent return. - raise OSError for all lpathconf errors Cosmetic: add a nfs4_acl boolean, so the code reads better.
They are only used at one place, move them there rather than having globals.
ThomasWaldmann
force-pushed
the
acl-error-handling-1.4
branch
from
a538013 to
7ba843b
Compare
ThomasWaldmann
changed the title
|
OK, looks like nobody wants to review this, so we'll test it as is in next beta... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This definitely will need some review and practical testing.
Fixes #4049.
Fixes #8139.