[PATCH] erofs: code cleanup by removing ifdef macro surrounding

Gao Xiang hsiangkao at redhat.com
Wed May 27 12:35:19 AEST 2020


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
> 



More information about the Linux-erofs mailing list