classification
Title: shutil doesn't copy extended attributes
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: hynek Nosy List: Arfrever, benjamin.peterson, eric.araujo, hynek, neologix, pitrou, python-dev
Priority: low Keywords: patch

Created on 2012-02-22 02:17 by pitrou, last changed 2012-05-12 21:59 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
xattr.diff hynek, 2012-04-12 16:22 Support for extended attributes review
copy2-xattr.diff hynek, 2012-05-05 15:45 xattr support for copy2 only review
copy2-xattr-with-better-protection.diff hynek, 2012-05-05 20:34 review
Messages (25)
msg153922 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-02-22 02:17
shutil doesn't provide any function which copies extended attributes on files. Note that "cp -a" does copy xattrs, but "cp -p" doesn't (and copy2() claims to work like "cp -p").
msg154182 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-25 05:34
> copy2() claims to work like "cp -p"
It probably does, for a behavior of “cp -p” that predates extended attributes <wink>.

Do you think the best way is to always copy xattrs, add a new parameter, add a new copy function?
msg154224 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-02-25 09:46
I'd tend to always copy xattrs – it seems that's what the user would expect to happen. A new parameter to _forbid_ it might make sense. However, I feel that there are already enough parameters in place. :-/
msg154231 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-02-25 10:24
I'm also in favor of adding extended attributes to copy2:
"""
Similar to shutil.copy(), but metadata is copied as well 
"""

extended attributes are metadata. And there are already too many copy functions...
msg154232 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-02-25 10:27
If nobody objects, I'd cook up a patch.
msg154251 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-25 14:41
Sounds good.
msg157653 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-04-06 09:52
This ticket has a small catch:

There are several namespaces. According to http://en.wikipedia.org/wiki/Xattr#Linux :

- user: can be set by anyone
- trusted: root only
- security: root only
- system: even root can’t do that, at least not in my vm

I’m writing a shutil.copyxattr() first which could simple get another argument for the namespaces that should be copied.

However what to do inside copy2()?

I’m tending to either:

1. copy only user.*
2. ignore errors in any namespace != user

Personally, I find the second approach rather non-deterministic.

So I’d suggest:

- copyxattr has an extra argument called namespaces with default being ['user'], so that in theory someone who wants to do something more sophisticated can do it.
- copy2 copies only user.* though because that’s what you usually want.

Please let me know what you think about it.
msg157903 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-09 21:10
> I’m writing a shutil.copyxattr() first which could simple get another
> argument for the namespaces that should be copied.

Sounds good to me :-)

> However what to do inside copy2()?
> 
> I’m tending to either:
> 
> 1. copy only user.*
> 2. ignore errors in any namespace != user
> 
> Personally, I find the second approach rather non-deterministic.

But it's also more practical, e.g. when running as root you would
probably be surprised if only a subset of xattrs get copied, wouldn't
you? “Practicality beats purity.”

For reference, here is part of the documentation for GNU cp's "-a"
option:

`-a'
`--archive'
     Preserve as much as possible of the structure and attributes of the
     original files in the copy (but do not attempt to preserve internal
     directory structure; i.e., `ls -U' may list the entries in a copied
     directory in a different order).  Try to preserve SELinux security
     context and extended attributes (xattr), but ignore any failure to
     do that and print no corresponding diagnostic.  Equivalent to `-dR
     --preserve=all' with the reduced diagnostics.

Meaning that "cp -a" tries to copy all xattrs and silences errors when
it's not possible to do so.

"cp --preserve=all" seems to have a similar error-silencing behaviour:

    `all'
          Preserve all file attributes.  Equivalent to specifying all
          of the above, but with the difference that failure to
          preserve SELinux security context or extended attributes does
          not change `cp''s exit status.  In contrast to `-a', all but
          `Operation not supported' warnings are output.
msg158147 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-04-12 16:22
Ok, so I’ve added a function `copyxattr()` and `copy2()` tries to copy all possible namespaces.

Tests pass on Linux and Mac OS X.
msg159738 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-04-30 23:09
Hynek, did you get a notification of my review on Rietveld?
msg159739 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-05-01 00:32
I didn't. :/ I'll look into it tomorrow.
msg159751 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-05-01 11:23
I have answered to the (two weeks old :-/) review. There are three open questions in there we'll have to figure out before I fix the patch:

- should copyxattr() remove xattrs in dst that aren't present in src? Make it an option like `remove_missing_xattr`?

- use "None" for `namespaces` in copyxattrs() to indicate we want to copy all of them?

- add a ignore_errors option?

ISTM, that "all namespaces" don't make much sense without ignore_errors as there seem to be some internal xattr etc. 

Suggestion: copyxattrs() has ignore_errors as default and returns a list of xattr it couldn't copy as (xattr, exception) tuples? Or an "on error" handler like in rmtree? I'd prefer the first one as ISTM that failures happen more often than not.
msg159791 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-05-02 12:00
I really like the idea of adding extended attributes copy to shutil.cop2().
However, I'm not convinced that exposing the raw copyxattr() is
necessary (I can't really come up with a use case for this).

So I'd suggest make copyxattr() private, and wait until someone asks
for the functionality.
msg159792 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-05-02 12:07
> I really like the idea of adding extended attributes copy to shutil.cop2().
> However, I'm not convinced that exposing the raw copyxattr() is
> necessary (I can't really come up with a use case for this).
> 
> So I'd suggest make copyxattr() private, and wait until someone asks
> for the functionality.

I agree. If someone needs it, (s)he will be able to advice on useful
semantics re replacement. For copy2() it’s a non-issue.
msg159995 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-05-05 15:45
So here's an updated patch. I've stripped everything away we don't need for copy2 integration. It catches EPERM, ENOTSUP and ENODATA (for race conditions).

Happy reviewing. :)
msg160012 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-05 17:51
The copyxattr() function should be private (_copyxattr()).
For some reason the tests are failing here:

======================================================================
ERROR: test_copy2_xattr (test.test_shutil.TestShutil)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/antoine/cpython/default/Lib/test/test_shutil.py", line 410, in test_copy2_xattr
    os.setxattr(src, 'user.foo', b'42')
OSError: [Errno 95] Operation not supported

======================================================================
ERROR: test_copyxattr (test.test_shutil.TestShutil)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/antoine/cpython/default/Lib/test/test_shutil.py", line 296, in test_copyxattr
    os.setxattr(src, 'user.foo', b'42')
OSError: [Errno 95] Operation not supported
msg160018 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-05-05 18:54
> The copyxattr() function should be private (_copyxattr()).

Ok. I presumed that not adding it to __all__ is "private enough".

> For some reason the tests are failing here:
> 
> ======================================================================
> ERROR: test_copy2_xattr (test.test_shutil.TestShutil)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "/home/antoine/cpython/default/Lib/test/test_shutil.py", line 410, in test_copy2_xattr
>     os.setxattr(src, 'user.foo', b'42')
> OSError: [Errno 95] Operation not supported
> 
> ======================================================================
> ERROR: test_copyxattr (test.test_shutil.TestShutil)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "/home/antoine/cpython/default/Lib/test/test_shutil.py", line 296, in test_copyxattr
>     os.setxattr(src, 'user.foo', b'42')
> OSError: [Errno 95] Operation not supported

Looks like your file system Python uses for tmp files doesn't support
xattr. That's bad because you can't verify. How should I cope with that?
try/catch on the first setxattr() and skip the test if it fails? Is the
an official way to do that?
msg160027 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-05 19:24
> Looks like your file system Python uses for tmp files doesn't support
> xattr. That's bad because you can't verify. How should I cope with that?
> try/catch on the first setxattr() and skip the test if it fails? Is the
> an official way to do that?

Well, apparently the extended attributes tests in test_os get skipped
with the following message: "no non-broken extended attribute support".
Perhaps you need the same condition somewhere.
msg160040 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-05-05 20:34
Ok, I've extracted the xattr checker from test_os.py and transformed it into a caching decorator like skip_unless_symlinks.

_copyxattr rename is also done.
msg160361 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-10 18:28
The patch looks ok, but it would be nice if someone with non-broken xattr support could test it. Although I suppose you did run the test suite, Hynek?
msg160362 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-05-10 19:06
I did. The only error I got was

ERROR: test_idna (test.test_socket.GeneralModuleTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vagrant/p2/Lib/test/test_socket.py", line 1182, in test_idna
    socket.gethostbyname('испытание.python.org')
socket.gaierror: [Errno -3] Temporary failure in name resolution

I presume it's unrelated. :)
msg160484 - (view) Author: Roundup Robot (python-dev) Date: 2012-05-12 17:05
New changeset 85824b819bcb by Antoine Pitrou in branch 'default':
Issue #14082: shutil.copy2() now copies extended attributes, if possible.
http://hg.python.org/cpython/rev/85824b819bcb
msg160485 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-12 17:05
Pushed now. Hopefully the buildbots won't moan.
msg160487 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-12 17:25
Looks like we have our first buildbot failure:


======================================================================
FAIL: test_copyxattr (test.test_shutil.TestShutil)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.krah-fedora/build/Lib/test/test_shutil.py", line 346, in test_copyxattr
    self.assertEqual(['user.bar'], os.listxattr(dst))
AssertionError: Lists differ: ['user.bar'] != ['security.selinux', 'user.bar...

First differing element 0:
user.bar
security.selinux

Second list contains 1 additional elements.
First extra element 1:
user.bar

- ['user.bar']
+ ['security.selinux', 'user.bar']
msg160497 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-12 21:59
Hynek's patch (communicated over IRC, committed in 8d85f9920878) seems to have fixed the failure.
History
Date User Action Args
2012-05-12 21:59:28pitrousetstatus: open -> closed

messages: + msg160497
2012-05-12 17:25:08pitrousetstatus: closed -> open

messages: + msg160487
2012-05-12 17:05:55pitrousetstatus: open -> closed
resolution: fixed
messages: + msg160485

stage: patch review -> resolved
2012-05-12 17:05:04python-devsetnosy: + python-dev
messages: + msg160484
2012-05-10 19:06:41hyneksetmessages: + msg160362
2012-05-10 18:28:26pitrousetmessages: + msg160361
2012-05-05 20:34:59hyneksetfiles: + copy2-xattr-with-better-protection.diff

messages: + msg160040
2012-05-05 19:24:49pitrousetmessages: + msg160027
2012-05-05 18:54:11hyneksetmessages: + msg160018
2012-05-05 17:51:25pitrousetmessages: + msg160012
2012-05-05 15:45:10hyneksetfiles: + copy2-xattr.diff

messages: + msg159995
2012-05-02 12:07:39hyneksetmessages: + msg159792
2012-05-02 12:00:49neologixsetmessages: + msg159791
2012-05-01 11:23:31hyneksetassignee: hynek
messages: + msg159751
2012-05-01 00:32:58hyneksetmessages: + msg159739
2012-04-30 23:09:16pitrousetmessages: + msg159738
2012-04-29 15:46:01hyneksetstage: patch review
2012-04-12 16:22:19hyneksetfiles: + xattr.diff
keywords: + patch
messages: + msg158147
2012-04-09 21:10:22pitrousetmessages: + msg157903
2012-04-06 09:52:18hyneksetmessages: + msg157653
2012-02-26 08:08:07Arfreversetnosy: + Arfrever
2012-02-25 14:41:00eric.araujosetmessages: + msg154251
2012-02-25 10:27:37hyneksetmessages: + msg154232
2012-02-25 10:24:44neologixsetmessages: + msg154231
2012-02-25 09:46:04hyneksetmessages: + msg154224
2012-02-25 05:34:38eric.araujosetnosy: + eric.araujo

messages: + msg154182
title: shutil doesn't support extended attributes -> shutil doesn't copy extended attributes
2012-02-22 02:17:32pitroucreate