On Wed, May 27, 2020 at 10:24:14AM +0800, cgxu wrote: > On 5/27/20 10:16 AM, Gao Xiang wrote: > > On Wed, May 27, 2020 at 09:55:17AM +0800, Chao Yu wrote: > > > > ... > > > > > > > > > > > > Originally, we set erofs_listxattr to ->listxattr only > > > > > when the config macro CONFIG_EROFS_FS_XATTR is enabled, > > > > > it means that erofs_listxattr() never returns -EOPNOTSUPP > > > > > in any case, so actually there is no logic change here, > > > > > right? > > > > > > > > Yeah, I agree there is no logic change, so I'm fine with the patch. > > > > But I'm little worry about if return 0 is actually wrong here... > > > > > > > > see the return value at: > > > > http://man7.org/linux/man-pages/man2/listxattr.2.html > > > > > > Yeah, I guess vfs should check that whether lower filesystem has set > > > .listxattr > > > callback function to decide to return that value, something like: > > > > > > static ssize_t > > > ecryptfs_listxattr(struct dentry *dentry, char *list, size_t size) > > > { > > > ... > > > if (!d_inode(lower_dentry)->i_op->listxattr) { > > > rc = -EOPNOTSUPP; > > > goto out; > > > } > > > ... > > > rc = d_inode(lower_dentry)->i_op->listxattr(lower_dentry, list, size); > > > ... > > > } > > > > This approach seems better. ;) Let me recheck all of this. > > Maybe we could submit a patch to fsdevel for some further advice... > > > > I agree that doing the check in vfs layer looks tidy and unified. > However, we should be aware this change may break user space tools.
I think I already sorted out the reason, it actually seems a regression due to multiple commits, let me try to submit a patch for some advice... Thanks, Gao Xiang > > > Thanks, > cgxu >
