-
Notifications
You must be signed in to change notification settings - Fork 103
give specific path to xattr.is_enabled(), disable symlink setattr call ... #235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…l that always fails
raising "Unsupported platform" (and making attic completely unusable) just because there is no xattr support isn't necessary.
the API_VERSION there was not in sync (and that was even true, as "st" was missing in acl_get()) - fixed.
|
This change didn't make it into the latest version 0.15 of Attic even though I thought it was ready. What is the critieria or timeframe for something like this? |
|
Symlink xattrs are actually mostly supported on Linux. There are restrictions on xattrs for non-file/directory inodes (which includes symlinks and other special inodes). I'm not sure if they are documented anywhere other than the source code. See: From the source code, it appears that user.* attributes are allowed only on files and directories, but other attributes, e.g. trusted.* and security.* are allowed on all inodes. I'm not sure what the rules are on other operating systems, though. |
|
well, as I said, I disabled the symlink xattr call because it always failed, see the comment in the archiver tests. |
|
These two comments don't make sense to me. The changes above removed/changed code which PREVENTED Attic from running on Windows Cygwin. Before these changes it would error out with Unsupported Platform (and a few other errors if you tried to correct that one). The changes bypassed attribute code which doesn't exist or work on Cygwin and therefore allowed Attic to seemingly work. I used and tested it. Thus: the JMBS comment makes no sense to me. He is explaining xattrs on Linux which is, yes, what Attic is/was doing seemingly fine. Then confirms about 'not sure with other OS's'. Yes..specifically Cygwin which was the reason for the changed. Then the following comment by Thomas also is confusing. "I disabled it because it always failed"? Well...yes, attic always failed on Cygwin..by coding design. Then you fixed it. Then it was ready for merging? Then I was thinking it would get merged into master. Please see this other issue which I thought was the basis for this 'pull request' and the changes: Sorry if I'm missing something here in the comments or process. |
|
@jumper444: i clarified my last comment. In general, there are a lot of pull requests and @jborg is currently low on time, so I'ld bet he is focussing on the most critical ones first (like security, crashes, data corruption). That said, I'ld also like to see the xattr stuff fixed as it is failing some tests for me every time I do work based on master branch. If you want to try code that has more stuff merged, see https://github.com/attic/merge, branches "merge" and "merge-all". Be careful. |
|
half of this PR's code is now present in attic 0.15, other half still missing. |
|
closing this pull request, seems unwanted. |
...that always fails