jborg / attic

Deduplicating backup program
Other
1.11k stars 104 forks source link

Please make xattr support optional #239

Closed 0-wiz-0 closed 9 years ago

0-wiz-0 commented 9 years ago

NetBSD does not support xattr, but attic unconditionally uses it. That makes attic do nothing on NetBSD:

Traceback (most recent call last):
  File "/usr/pkg/bin/attic3.4", line 2, in <module>
    from attic.archiver import main
  File "/usr/pkg/lib/python3.4/site-packages/attic/archiver.py", line 13, in <module>
    from attic.archive import Archive, ArchiveChecker
  File "/usr/pkg/lib/python3.4/site-packages/attic/archive.py", line 16, in <module>
    from attic import xattr
  File "/usr/pkg/lib/python3.4/site-packages/attic/xattr.py", line 251, in <module>
    raise Exception('Unsupported platform: %s' % sys.platform)
Exception: Unsupported platform: netbsd7
ThomasWaldmann commented 9 years ago

I searched for "xattr netbsd" and found some stuff referring to it - it did not seem unsupported.

So, are you sure about that netbsd does not support it? Or might it be because of your mount options or the fs you use?

Another thing is what attic does if it finds a platform without some attic-side xattr support. I guess we should just switch off xattr support then, give a warning maybe, but not traceback & die, but continue.

0-wiz-0 commented 9 years ago

Sorry for the confusing bug report. NetBSD does support extended attributes, but only on UFS1 file systems; the default nowadays is UFS2.

My request was rather in the direction of your last paragraph -- that attic still continues even if it can't find xattr support.

ThomasWaldmann commented 9 years ago

@0-wiz-0 : Ah, that makes it more clear. I will have a look at making xattr platform support optional (but I can't test on netbsd).

If you like, maybe join us on IRC #attic on chat.freenode.net. Guess as you will be the first netbsd tester, it'll need some careful testing soon.

ThomasWaldmann commented 9 years ago

I just updated PR #235 with a dummy xattr implementation, please try it.

(I did a small test myself by crippling the linux platform check, so it considered my linux as unsupported - all unit tests still worked.) If you find any xattr issues, tell here.

If you find any netbsd platform issue (which are not xattr related), please open separate tickets.

jumper444 commented 9 years ago

Thomas, Thank you for these changes. I saw an update notice by email and reviewed what you did. The changes are similar to what I was doing to bypass (or dummy/force) the platform and xattr checks in the code and try to get it working on cygwin. Despite doing something similar I'm more comfortable that somebody with knowledge of the code implemented a fix.

I will be using/testing in cygwin shortly and comment if I find anything.

From what you changed Attic will now simply just read and write standard files on unsupported platforms, right? It won't then go back and try to set things like ownership, hidden, read only (or whatever various file attributes consist of.) Am I understanding this properly?

ThomasWaldmann commented 9 years ago

I only changed xattr/acl related stuff.

There might be additional problems with other stuff, you'll find out. :)

jumper444 commented 9 years ago

jborg/Attic 0.14 is the 'master' and hasn't been updated in a while. Your "merge-all" tag seems to be the better version at this point with many fixes. (which is not at pypi if/until jborg accepts)

However the 'fix_xattr' changes discussed here are off jborg/master. Not sure why this was done unless you are giving a baseline for testing.

Wouldn't it be better for me to combine the 'merge-all' and 'fix_attr' tag points and then use that code for testing/operation?

ThomasWaldmann commented 9 years ago

I do bugfixes usually based on jborg/attic branch master (which is duplicated at attic/merge branch master) so everybody can apply them easily.

I merge all fixes and uncritical improvements into attic/merge branch merge, so all that stuff can be tested together.

I merge everything from merge branch + more critical / bigger changes into attic/merge branch merge-all.

So, use whatever pleases you most. In any case, be careful. :)

jumper444 commented 9 years ago

I tested the "fix_xattr" changes (using the Thomas 'merge-all' branch code, not jborg master). Short answer it appears to work.

I ran the code on current (last day or two) Cygwin (on XP sp3) and did the following: -create a repository, list, delete, create another; etc. looks ok; -create archive of about 30,000 files, list contents, etc; looks ok; -create a 2nd, 3rd, 4th archive; each about 30,000 files (all differing by about 100 files representing 1 month of normal system usage) (Total data stored after dedupe about 2G). -list (now have 4 archives in repository; each differing by a few hundred files); looks ok -extract each archive individually and binary/full compare to original data; no differences -check repository; no problems found -delete archives #1 and #3 from repository -extract archive #2 and binary compare again to make sure deletes didn't corrupt remaining data; no problem -add an archive again (#5) of 30,000 files (about 100 file changes mixed in); -check repository; no problems -extract archive #5 and binary compare to original data; no differences

SO...the conclusion is that it looks like this fix allows Attic to run in Cygwin without problems based on my rather normal usage. Please consider rolling this change into "merge-all" so it can eventually make it into Attic official release.

I have a suggestion to consider but don't know if it fits into a release package. Since you are turning off ACL and/or Xattr when you have the dummy/pass functions (Instead of halting on an error) on the non-recognized OS's, perhaps the program should have a print warning just after "API_VERSION=2" on platform.py (and also maybe on line 253 of xattr.py where you've removed the "Unsupported Platform" exception). This warning would print a single line when Attic is run that says, for example, "Warning: ACLs/Xattr not supported on this platform". It would alert a user although maybe something like this should just be silent and instead be in the documentation. I don't know.

mehertz commented 9 years ago

I've followed the same process as jumper444 and i've been able to initialize and add to a remote repository from, Cygwin, so that seems to work as well.

I am however then unable to access the created the repository on the server using attic 0.14 ("attic: Error: Unsupported payload type 3. A newer version is required to access this repository."). That's fair enough, i'm sure if I used the version of attic from the merge-all branch it would work. I just wanted to ask when the next release is planned, as it would be nice to create backups on Windows and be able to restore them from a version of Attic installable from pip.

jumper444 commented 9 years ago

Everything I was doing was local on a single computer/drive (and USB connected drives) I wasn't using any remote/server connectivity for what it is worth.

jumper444 commented 9 years ago

Yes...I was using the "merge-all" code as my base, however I had made similar "fix_attr" type changes to the jborg/master (0.14) at some earlier points and I didn't receive any errors. (Although again I was only using local drives and single computer.) Never saw that error and don't know the reason.

mehertz commented 9 years ago

Yes, if I apply the fix_xattr changes to master then i'm able to use Attic on Cygwin perfectly fine as well. The unsupported payload error is originating from a commit made in the merge-all code - but i'm sure that's known and is not a bug.

ThomasWaldmann commented 9 years ago

Just to clarify:

So, the "unsupported payload type 3" error you got from 0.14 is expected - that is the new more flexible stuff from merge-all branch.

jumper444 commented 9 years ago

My testing/work was ONLY on the merge-all code (with fix_attr patches) not 0.14... so the reasoning above would confirm why I didn't get a 'payload' error.

When I said I had also used 0.14 master earlier I was only casually speaking about how I had sorta bumped around trying to solve the Cygwin problem myself (ultimately making similar changes to Thomas that seemed to work properly). However I have not been interoperating between the master and merge-all branches and between their repositories. In fact I was unaware that a merge-all repository wasn't backwards compatible. I guess the error code could be a bit more friendly, but these are likely growing pains of a new project.

I still think fix_attr change works and is ready for master. (Somewhere though on either the instructions or the command line it should probably warn that Cygwin usage is still 'experimental' perhaps.)

EDIT: i guess I shouldn't say my testing was "only" on merge-all because if I made similar changes to Thomas on master 0.14 and ran some backups....then I guess I DID sorta try it out there. But everything I've commented on above has been merge-all related.

0-wiz-0 commented 9 years ago

I've tried out the referenced patch on top of attic-0.14, and could init, backup (2x), list, and restore on NetBSD. So this looks good to go in from my side. Thank you!