bpo-24564: shutil.copystat(): ignore EINVAL on os.setxattr() #8601
Conversation
serhiy-storchaka
changed the title
| @@ -297,15 +297,16 @@ def _copyxattr(src, dst, *, follow_symlinks=True): | |||
| try: | |||
| names = os.listxattr(src, follow_symlinks=follow_symlinks) | |||
| except OSError as e: | |||
| if e.errno not in (errno.ENOTSUP, errno.ENODATA): | |||
| if e.errno not in {errno.ENOTSUP, errno.ENODATA, errno.EINVAL}: | |||
There was a problem hiding this comment.
There is no benefit from using a set instead of a tuple. Building a set is more expensive than builtin a tuple.
| if e.errno not in {errno.ENOTSUP, errno.ENODATA, errno.EINVAL}: | |
| if e.errno not in (errno.ENOTSUP, errno.ENODATA, errno.EINVAL): |
| raise | ||
| return | ||
| for name in names: | ||
| try: | ||
| value = os.getxattr(src, name, follow_symlinks=follow_symlinks) | ||
| os.setxattr(dst, name, value, follow_symlinks=follow_symlinks) | ||
| except OSError as e: | ||
| if e.errno not in (errno.EPERM, errno.ENOTSUP, errno.ENODATA): | ||
| if e.errno not in {errno.EPERM, errno.ENOTSUP, errno.ENODATA, |
There was a problem hiding this comment.
| if e.errno not in {errno.EPERM, errno.ENOTSUP, errno.ENODATA, | |
| if e.errno not in (errno.EPERM, errno.ENOTSUP, errno.ENODATA, |
| raise | ||
| return | ||
| for name in names: | ||
| try: | ||
| value = os.getxattr(src, name, follow_symlinks=follow_symlinks) | ||
| os.setxattr(dst, name, value, follow_symlinks=follow_symlinks) | ||
| except OSError as e: | ||
| if e.errno not in (errno.EPERM, errno.ENOTSUP, errno.ENODATA): | ||
| if e.errno not in {errno.EPERM, errno.ENOTSUP, errno.ENODATA, | ||
| errno.EINVAL}: |
There was a problem hiding this comment.
| errno.EINVAL}: | |
| errno.EINVAL): |
|
I removed the " needs backport to 3.6" label, the 3.6 branch no longer accept bugfixes (only security fixes are accepted): https://devguide.python.org/#status-of-python-branches |
|
Hey @serhiy-storchaka @giampaolo! I'm a first time CPython contributor and I was taking a look at bpo-36850 that ties into bpo-24564. Are we still waiting for a higher authority to approve ignoring EINVAL on os.setxattr() (I reviewed the bpos with @abadger and we didn't see anybody saying yay/nay)? I can add some unit tests with mock, but I'm not sure how useful that may be. How are filesystem copy() methods tested with network filesystems? Should we run a Docker container with CIFS/NFS and try to copy over a small file to local with xatttrs()? Not sure how that may integrate with the Python test suite. |
|
The patch is OK except that tuples should be used instead of sets (as per Serhiy’s review comment). The PR points to a GiT repo/branch which no longer exists though so a new PR should be submitted. I am currently traveling and am short on time though, so if you want to take over be my guest. (and no, no need of additional tests) |
|
@giampaolo thank you for the response! I will open a new pull request with these changes and the tuple/set changes. |
|
closing as a new PR superceded this one. |
https://bugs.python.org/issue24564
https://bugs.python.org/issue24564