msg153922 - (view) |
Author: Antoine Pitrou (pitrou) * |
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) * |
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) * |
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) * |
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) * |
Date: 2012-02-25 10:27 |
If nobody objects, I'd cook up a patch.
|
msg154251 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2012-02-25 14:41 |
Sounds good.
|
msg157653 - (view) |
Author: Hynek Schlawack (hynek) * |
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) * |
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) * |
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) * |
Date: 2012-04-30 23:09 |
Hynek, did you get a notification of my review on Rietveld?
|
msg159739 - (view) |
Author: Hynek Schlawack (hynek) * |
Date: 2012-05-01 00:32 |
I didn't. :/ I'll look into it tomorrow.
|
msg159751 - (view) |
Author: Hynek Schlawack (hynek) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2012-05-12 17:05 |
Pushed now. Hopefully the buildbots won't moan.
|
msg160487 - (view) |
Author: Antoine Pitrou (pitrou) * |
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) * |
Date: 2012-05-12 21:59 |
Hynek's patch (communicated over IRC, committed in 8d85f9920878) seems to have fixed the failure.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:27 | admin | set | github: 58290 |
2012-05-12 21:59:28 | pitrou | set | status: open -> closed
messages:
+ msg160497 |
2012-05-12 17:25:08 | pitrou | set | status: closed -> open
messages:
+ msg160487 |
2012-05-12 17:05:55 | pitrou | set | status: open -> closed resolution: fixed messages:
+ msg160485
stage: patch review -> resolved |
2012-05-12 17:05:04 | python-dev | set | nosy:
+ python-dev messages:
+ msg160484
|
2012-05-10 19:06:41 | hynek | set | messages:
+ msg160362 |
2012-05-10 18:28:26 | pitrou | set | messages:
+ msg160361 |
2012-05-05 20:34:59 | hynek | set | files:
+ copy2-xattr-with-better-protection.diff
messages:
+ msg160040 |
2012-05-05 19:24:49 | pitrou | set | messages:
+ msg160027 |
2012-05-05 18:54:11 | hynek | set | messages:
+ msg160018 |
2012-05-05 17:51:25 | pitrou | set | messages:
+ msg160012 |
2012-05-05 15:45:10 | hynek | set | files:
+ copy2-xattr.diff
messages:
+ msg159995 |
2012-05-02 12:07:39 | hynek | set | messages:
+ msg159792 |
2012-05-02 12:00:49 | neologix | set | messages:
+ msg159791 |
2012-05-01 11:23:31 | hynek | set | assignee: hynek messages:
+ msg159751 |
2012-05-01 00:32:58 | hynek | set | messages:
+ msg159739 |
2012-04-30 23:09:16 | pitrou | set | messages:
+ msg159738 |
2012-04-29 15:46:01 | hynek | set | stage: patch review |
2012-04-12 16:22:19 | hynek | set | files:
+ xattr.diff keywords:
+ patch messages:
+ msg158147
|
2012-04-09 21:10:22 | pitrou | set | messages:
+ msg157903 |
2012-04-06 09:52:18 | hynek | set | messages:
+ msg157653 |
2012-02-26 08:08:07 | Arfrever | set | nosy:
+ Arfrever
|
2012-02-25 14:41:00 | eric.araujo | set | messages:
+ msg154251 |
2012-02-25 10:27:37 | hynek | set | messages:
+ msg154232 |
2012-02-25 10:24:44 | neologix | set | messages:
+ msg154231 |
2012-02-25 09:46:04 | hynek | set | messages:
+ msg154224 |
2012-02-25 05:34:38 | eric.araujo | set | nosy:
+ eric.araujo
messages:
+ msg154182 title: shutil doesn't support extended attributes -> shutil doesn't copy extended attributes |
2012-02-22 02:17:32 | pitrou | create | |