Skip to content

xattr: move to platform pkg, use cython, use bytes, use fd#3918

Merged
ThomasWaldmann merged 8 commits intoborgbackup:masterfrom
ThomasWaldmann:platform-xattr
Jul 7, 2018
Merged

xattr: move to platform pkg, use cython, use bytes, use fd#3918
ThomasWaldmann merged 8 commits intoborgbackup:masterfrom
ThomasWaldmann:platform-xattr

Conversation

@ThomasWaldmann
Copy link
Copy Markdown
Member

this code used to live in borg.xattr and used ctypes
(and was the only ctypes-using code in borg).

the low level code now was converted to cython and
the platform code moved to platform package.

got rid of the code that tried to find the libc.

@ThomasWaldmann ThomasWaldmann changed the title xattr: move to platform package, use cython, fixes #2495 WIP xattr: move to platform package, use cython, fixes #2495 Jun 21, 2018
@ThomasWaldmann
Copy link
Copy Markdown
Member Author

ThomasWaldmann commented Jun 21, 2018

Linux and macOS looks ok (see travis), will do other tests locally.

Please review.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jun 21, 2018

Codecov Report

Merging #3918 into master will increase coverage by 0.23%.
The diff coverage is 82.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3918      +/-   ##
==========================================
+ Coverage   85.04%   85.28%   +0.23%     
==========================================
  Files          36       37       +1     
  Lines        9263     9185      -78     
  Branches     1545     1527      -18     
==========================================
- Hits         7878     7833      -45     
+ Misses        945      917      -28     
+ Partials      440      435       -5
Impacted Files Coverage Δ
src/borg/archive.py 84% <100%> (+1.41%) ⬆️
src/borg/platform/__init__.py 90.47% <100%> (+1.58%) ⬆️
src/borg/xattr.py 67.21% <65.85%> (-15.96%) ⬇️
src/borg/platform/base.py 82.72% <71.42%> (-0.93%) ⬇️
src/borg/platform/xattr.py 88.88% <88.88%> (ø)
src/borg/archiver.py 85% <0%> (+0.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2178e73...34a51eb. Read the comment docs.

@ThomasWaldmann
Copy link
Copy Markdown
Member Author

tried freebsd12:

  • with fakeroot: lots of EISDIR fails all over the place (not xattr related)
  • without fakeroot: "invalid argument" for listxattr('input/fusexattr')

@ThomasWaldmann
Copy link
Copy Markdown
Member Author

ThomasWaldmann commented Jul 4, 2018

about freebsd12 issues:

  • the EISDIR issues seen with fakeroot are nothing new. freebsd10 testing in 1.1-maint just did not have fakeroot installed.
  • the issues seen in fuse xattr tests seem to be a bug in freebsd12 fuse code, normal native fs works ok with xattrs. so, not a borg issue. also, the same issues happen with master code.

general issues:

  • the currently hardcoded XATTR_FAKEROOT flag influences which tests are executed and how, likely this should get reviewed / improved / removed.

consider api change:

  • getxattr returns None for empty value - check why it is not just b'' as one would expect.
  • setxattr accepts a None value - check why it is not just b'' as one would expect.
  • the latter 2 items somehow feel like if "None to represent empty" makes sense somehow, it is done at the wrong place (should be transformed one layer higher).
  • listxattr returns list-of-str (requiring fs.decode() calls), could be list-of-X with X being "whatever getattr/setattr" on this platform wants
  • getattr/setattr gets a str for the attr name, requiring a fs.encode() call, could be whatever needed (bytes)

cleanup:

  • we already have borg.xattr.get_all, could also have borg.xattr.set_all
  • get_all and set_all would deal with transforming native attr name datatype to str (for storage into borg Item) or from str (when restoring from a borg Item)

@borgbackup borgbackup deleted a comment from tim-seoss Jul 4, 2018
@ThomasWaldmann ThomasWaldmann force-pushed the platform-xattr branch 3 times, most recently from a48452c to 5dd5ed5 Compare July 5, 2018 17:57
@ThomasWaldmann ThomasWaldmann mentioned this pull request Jul 5, 2018
@ThomasWaldmann ThomasWaldmann changed the title WIP xattr: move to platform package, use cython, fixes #2495 xattr: move to platform pkg, use cython, use bytes, use fd Jul 7, 2018
this code used to live in borg.xattr and used ctypes
(and was the only ctypes-using code in borg).

the low level code now was converted to cython and
the platform code moved to platform package.

got rid of the code that tried to find the libc.
- getxattr should only return bytes, not None
- setxattr should not get a None value, just bytes
- remove unneeded tmp vars
also: follow_symlinks param defaults to False (we do never use True)

fix tests, xattrs are set via FD now.
if there is EPERM just for a single attr, it now still collects the
other attrs (previous behaviour: just return empty xattrs dict).
when processing regular files, use a fd to query xattrs.

when the file was modified and we chunked it, we have it open anyways.

if not, we open the file once and then query xattrs, in the hope that
this is more efficient than the path based calls.

guess it is less prone to race conditions in any case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants