bpo-24564: shutil.copystat(): ignore EINVAL on os.setxattr()#8601
bpo-24564: shutil.copystat(): ignore EINVAL on os.setxattr()#8601giampaolo wants to merge 1 commit intopython:masterfrom giampaolo:24564-shutil-xattr-einval
Conversation
serhiy-storchaka
changed the title
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
LGTM in general. Just added minor suggestions. And would be nice to add tests.
| 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): |
| 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, |
| 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. |
7 participants
https://bugs.python.org/issue24564
https://bugs.python.org/issue24564