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

cgxu cgxu519 at mykernel.net
Wed May 27 12:24:14 AEST 2020


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


More information about the Linux-erofs mailing list