improve acl_get / acl_set error handling (master) #8178
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 @@
## master #8178 +/- ##
==========================================
- Coverage 83.49% 83.45% -0.05%
==========================================
Files 67 67
Lines 12046 12061 +15
Branches 2185 2189 +4
==========================================
+ Hits 10058 10065 +7
- Misses 1389 1397 +8
Partials 599 599 ☔ View full report in Codecov by Sentry. |
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
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.
ThomasWaldmann
force-pushed
the
acl-error-handling-master
branch
from
7929be0
to
4e5bf28
Compare
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.
Fixes #4049.
Fixes #8139.