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

[cgxu <cgxu519@mykernel.net>]

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.


Thanks,
cgxu