fs: properly handle ENOTSUP in copyXAttrs#245
Conversation
fs/copy_linux.go
Outdated
| xattrKeys, err := sysx.LListxattr(src) | ||
| if err != nil { | ||
| if err == unix.ENOTSUP { |
There was a problem hiding this comment.
Oh! Actually; this is handling an error returned by sysx, which is not a raw stdlib / syscall package, perhaps better to use errors.Is() here?
Some discussion with @robmry and @djs55 (we happened to be looking into a similar issue), and there's some discussion if EOPNOTSUPP should be handled here; this was related to https://elixir.bootlin.com/linux/v6.10/source/fs/fuse/xattr.c#L23
There was a problem hiding this comment.
perhaps better to use errors.Is() here
Makes sense, pushed the change
there's some discussion if
EOPNOTSUPPshould be handled here
It seems pretty reasonable to me, since the fuse_getxattr call would also fail in this use case. LMK and I can add add a check for this condition
Filesystems without xattr support do not have any xattrs to copy. The syscall for this will return ENOTSUP to indicate this, but continuity will treat it as a regular error. This change will return nil if this call returns ENOTSUP. Signed-off-by: David Son <[email protected]>
sondavidb
force-pushed
the
properly-handle-fs-without-xattrs
branch
from
627d7a1 to
c494f3d
Compare
|
haven't had time to look deeper, but this CI test failure is strange; I don't think we have flakes in this project, so is this being caused by the change: |
|
Please rebase to pass the CI |
AkihiroSuda
left a comment
AkihiroSuda
left a comment
There was a problem hiding this comment.
Merging, assuming that the merge commit will pass the CI
4 participants
Fixes #244. More context in the issue.
Filesystems without xattr support do not have any xattrs to copy. The syscall for this will return ENOTSUP to indicate this, but continuity will treat it as a regular error. This change will return nil if this call returns ENOTSUP.