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

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

[Chengguang Xu]

Define erofs_listxattr and erofs_xattr_handlers to NULL when
CONFIG_EROFS_FS_XATTR is not enabled, then we can remove many
ugly ifdef macros in the code.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
Only compile tested.

 fs/erofs/inode.c | 6 ------
 fs/erofs/namei.c | 2 --
 fs/erofs/super.c | 4 +---
 fs/erofs/xattr.h | 7 ++-----
 4 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
index 3350ab65d892..7dd4bbe9674f 100644
--- a/fs/erofs/inode.c
+++ b/fs/erofs/inode.c
@@ -311,27 +311,21 @@ int erofs_getattr(const struct path *path, struct kstat *stat,
 
 const struct inode_operations erofs_generic_iops = {
 	.getattr = erofs_getattr,
-#ifdef CONFIG_EROFS_FS_XATTR
 	.listxattr = erofs_listxattr,
-#endif
 	.get_acl = erofs_get_acl,
 };
 
 const struct inode_operations erofs_symlink_iops = {
 	.get_link = page_get_link,
 	.getattr = erofs_getattr,
-#ifdef CONFIG_EROFS_FS_XATTR
 	.listxattr = erofs_listxattr,
-#endif
 	.get_acl = erofs_get_acl,
 };
 
 const struct inode_operations erofs_fast_symlink_iops = {
 	.get_link = simple_get_link,
 	.getattr = erofs_getattr,
-#ifdef CONFIG_EROFS_FS_XATTR
 	.listxattr = erofs_listxattr,
-#endif
 	.get_acl = erofs_get_acl,
 };
 
diff --git a/fs/erofs/namei.c b/fs/erofs/namei.c
index 3abbecbf73de..52f201e03c62 100644
--- a/fs/erofs/namei.c
+++ b/fs/erofs/namei.c
@@ -244,9 +244,7 @@ static struct dentry *erofs_lookup(struct inode *dir,
 const struct inode_operations erofs_dir_iops = {
 	.lookup = erofs_lookup,
 	.getattr = erofs_getattr,
-#ifdef CONFIG_EROFS_FS_XATTR
 	.listxattr = erofs_listxattr,
-#endif
 	.get_acl = erofs_get_acl,
 };
 
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index b514c67e5fc2..8e46d204a0c2 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -408,10 +408,8 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_time_gran = 1;
 
 	sb->s_op = &erofs_sops;
-
-#ifdef CONFIG_EROFS_FS_XATTR
 	sb->s_xattr = erofs_xattr_handlers;
-#endif
+
 	/* set erofs default mount options */
 	erofs_default_options(sbi);
 
diff --git a/fs/erofs/xattr.h b/fs/erofs/xattr.h
index 50966f1c676e..e4e5093f012c 100644
--- a/fs/erofs/xattr.h
+++ b/fs/erofs/xattr.h
@@ -76,11 +76,8 @@ static inline int erofs_getxattr(struct inode *inode, int index,
 	return -EOPNOTSUPP;
 }
 
-static inline ssize_t erofs_listxattr(struct dentry *dentry,
-				      char *buffer, size_t buffer_size)
-{
-	return -EOPNOTSUPP;
-}
+#define erofs_listxattr (NULL)
+#define erofs_xattr_handlers (NULL)
 #endif	/* !CONFIG_EROFS_FS_XATTR */
 
 #ifdef CONFIG_EROFS_FS_POSIX_ACL
-- 
2.20.1

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

[Gao Xiang]

Hi Chengguang,

On Tue, May 26, 2020 at 05:03:43PM +0800, Chengguang Xu wrote:
> Define erofs_listxattr and erofs_xattr_handlers to NULL when
> CONFIG_EROFS_FS_XATTR is not enabled, then we can remove many
> ugly ifdef macros in the code.
> 
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> ---
> Only compile tested.
> 
>  fs/erofs/inode.c | 6 ------
>  fs/erofs/namei.c | 2 --
>  fs/erofs/super.c | 4 +---
>  fs/erofs/xattr.h | 7 ++-----
>  4 files changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> index 3350ab65d892..7dd4bbe9674f 100644
> --- a/fs/erofs/inode.c
> +++ b/fs/erofs/inode.c
> @@ -311,27 +311,21 @@ int erofs_getattr(const struct path *path, struct kstat *stat,
>  
>  const struct inode_operations erofs_generic_iops = {
>  	.getattr = erofs_getattr,
> -#ifdef CONFIG_EROFS_FS_XATTR
>  	.listxattr = erofs_listxattr,
> -#endif

It seems equivalent. And it seems ext2 and f2fs behave in the same way...
But I'm not sure whether we'd return 0 (if I didn't see fs/xattr.c by mistake)
or -EOPNOTSUPP here... Some thoughts about this?

Anyway, I'm fine with that if return 0 is okay here, but I'd like to know your
and Chao's thoughts about this... I will play with it later as well.

Reviewed-by: Gao Xiang <hsiangkao@redhat.com>

Thanks,
Gao Xiang

>  	.get_acl = erofs_get_acl,
>  };
>  
>  const struct inode_operations erofs_symlink_iops = {
>  	.get_link = page_get_link,
>  	.getattr = erofs_getattr,
> -#ifdef CONFIG_EROFS_FS_XATTR
>  	.listxattr = erofs_listxattr,
> -#endif
>  	.get_acl = erofs_get_acl,
>  };
>  
>  const struct inode_operations erofs_fast_symlink_iops = {
>  	.get_link = simple_get_link,
>  	.getattr = erofs_getattr,
> -#ifdef CONFIG_EROFS_FS_XATTR
>  	.listxattr = erofs_listxattr,
> -#endif
>  	.get_acl = erofs_get_acl,
>  };
>  
> diff --git a/fs/erofs/namei.c b/fs/erofs/namei.c
> index 3abbecbf73de..52f201e03c62 100644
> --- a/fs/erofs/namei.c
> +++ b/fs/erofs/namei.c
> @@ -244,9 +244,7 @@ static struct dentry *erofs_lookup(struct inode *dir,
>  const struct inode_operations erofs_dir_iops = {
>  	.lookup = erofs_lookup,
>  	.getattr = erofs_getattr,
> -#ifdef CONFIG_EROFS_FS_XATTR
>  	.listxattr = erofs_listxattr,
> -#endif
>  	.get_acl = erofs_get_acl,
>  };
>  
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index b514c67e5fc2..8e46d204a0c2 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -408,10 +408,8 @@ static int erofs_fill_super(struct super_block *sb, void *data, int silent)
>  	sb->s_time_gran = 1;
>  
>  	sb->s_op = &erofs_sops;
> -
> -#ifdef CONFIG_EROFS_FS_XATTR
>  	sb->s_xattr = erofs_xattr_handlers;
> -#endif
> +
>  	/* set erofs default mount options */
>  	erofs_default_options(sbi);
>  
> diff --git a/fs/erofs/xattr.h b/fs/erofs/xattr.h
> index 50966f1c676e..e4e5093f012c 100644
> --- a/fs/erofs/xattr.h
> +++ b/fs/erofs/xattr.h
> @@ -76,11 +76,8 @@ static inline int erofs_getxattr(struct inode *inode, int index,
>  	return -EOPNOTSUPP;
>  }
>  
> -static inline ssize_t erofs_listxattr(struct dentry *dentry,
> -				      char *buffer, size_t buffer_size)
> -{
> -	return -EOPNOTSUPP;
> -}
> +#define erofs_listxattr (NULL)
> +#define erofs_xattr_handlers (NULL)
>  #endif	/* !CONFIG_EROFS_FS_XATTR */
>  
>  #ifdef CONFIG_EROFS_FS_POSIX_ACL
> -- 
> 2.20.1
> 
> 

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

[cgxu]

On 5/26/20 5:49 PM, Gao Xiang wrote:
> Hi Chengguang,
> 
> On Tue, May 26, 2020 at 05:03:43PM +0800, Chengguang Xu wrote:
>> Define erofs_listxattr and erofs_xattr_handlers to NULL when
>> CONFIG_EROFS_FS_XATTR is not enabled, then we can remove many
>> ugly ifdef macros in the code.
>>
>> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
>> ---
>> Only compile tested.
>>
>>   fs/erofs/inode.c | 6 ------
>>   fs/erofs/namei.c | 2 --
>>   fs/erofs/super.c | 4 +---
>>   fs/erofs/xattr.h | 7 ++-----
>>   4 files changed, 3 insertions(+), 16 deletions(-)
>>
>> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
>> index 3350ab65d892..7dd4bbe9674f 100644
>> --- a/fs/erofs/inode.c
>> +++ b/fs/erofs/inode.c
>> @@ -311,27 +311,21 @@ int erofs_getattr(const struct path *path, struct kstat *stat,
>>   
>>   const struct inode_operations erofs_generic_iops = {
>>   	.getattr = erofs_getattr,
>> -#ifdef CONFIG_EROFS_FS_XATTR
>>   	.listxattr = erofs_listxattr,
>> -#endif
> 
> It seems equivalent. And it seems ext2 and f2fs behave in the same way...

I posted similar patch for ext2 and Jack merged to
his tree the other day, though that series also
included a real bugfix. I also posted similar patch
to f2fs, so if erofs and f2fs merge these patches
then all three will behave in the same way, ;-)

You may refer below link for the detail.

https://lore.kernel.org/linux-ext4/20200522044035.24190-2-cgxu519@mykernel.net/


> But I'm not sure whether we'd return 0 (if I didn't see fs/xattr.c by mistake)
> or -EOPNOTSUPP here... Some thoughts about this? >
> Anyway, I'm fine with that if return 0 is okay here, but I'd like to know your
> and Chao's thoughts about this... I will play with it later as well.

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?


Thanks,
cgxu

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

[Gao Xiang]

On Tue, May 26, 2020 at 06:29:00PM +0800, cgxu wrote:
> On 5/26/20 5:49 PM, Gao Xiang wrote:
> > Hi Chengguang,
> > 
> > On Tue, May 26, 2020 at 05:03:43PM +0800, Chengguang Xu wrote:
> > > Define erofs_listxattr and erofs_xattr_handlers to NULL when
> > > CONFIG_EROFS_FS_XATTR is not enabled, then we can remove many
> > > ugly ifdef macros in the code.
> > > 
> > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> > > ---
> > > Only compile tested.
> > > 
> > >   fs/erofs/inode.c | 6 ------
> > >   fs/erofs/namei.c | 2 --
> > >   fs/erofs/super.c | 4 +---
> > >   fs/erofs/xattr.h | 7 ++-----
> > >   4 files changed, 3 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
> > > index 3350ab65d892..7dd4bbe9674f 100644
> > > --- a/fs/erofs/inode.c
> > > +++ b/fs/erofs/inode.c
> > > @@ -311,27 +311,21 @@ int erofs_getattr(const struct path *path, struct kstat *stat,
> > >   const struct inode_operations erofs_generic_iops = {
> > >   	.getattr = erofs_getattr,
> > > -#ifdef CONFIG_EROFS_FS_XATTR
> > >   	.listxattr = erofs_listxattr,
> > > -#endif
> > 
> > It seems equivalent. And it seems ext2 and f2fs behave in the same way...
> 
> I posted similar patch for ext2 and Jack merged to
> his tree the other day, though that series also
> included a real bugfix. I also posted similar patch
> to f2fs, so if erofs and f2fs merge these patches
> then all three will behave in the same way, ;-)
> 
> You may refer below link for the detail.
> 
> https://lore.kernel.org/linux-ext4/20200522044035.24190-2-cgxu519@mykernel.net/

Thanks for your link...

> 
> 
> > But I'm not sure whether we'd return 0 (if I didn't see fs/xattr.c by mistake)
> > or -EOPNOTSUPP here... Some thoughts about this? >
> > Anyway, I'm fine with that if return 0 is okay here, but I'd like to know your
> > and Chao's thoughts about this... I will play with it later as well.
> 
> 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

Thanks,
Gao Xiang

> 
> 
> Thanks,
> cgxu
> 

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

[Chao Yu]

On 2020/5/26 18:35, Gao Xiang wrote:
> On Tue, May 26, 2020 at 06:29:00PM +0800, cgxu wrote:
>> On 5/26/20 5:49 PM, Gao Xiang wrote:
>>> Hi Chengguang,
>>>
>>> On Tue, May 26, 2020 at 05:03:43PM +0800, Chengguang Xu wrote:
>>>> Define erofs_listxattr and erofs_xattr_handlers to NULL when
>>>> CONFIG_EROFS_FS_XATTR is not enabled, then we can remove many
>>>> ugly ifdef macros in the code.
>>>>
>>>> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
>>>> ---
>>>> Only compile tested.
>>>>
>>>>   fs/erofs/inode.c | 6 ------
>>>>   fs/erofs/namei.c | 2 --
>>>>   fs/erofs/super.c | 4 +---
>>>>   fs/erofs/xattr.h | 7 ++-----
>>>>   4 files changed, 3 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/fs/erofs/inode.c b/fs/erofs/inode.c
>>>> index 3350ab65d892..7dd4bbe9674f 100644
>>>> --- a/fs/erofs/inode.c
>>>> +++ b/fs/erofs/inode.c
>>>> @@ -311,27 +311,21 @@ int erofs_getattr(const struct path *path, struct kstat *stat,
>>>>   const struct inode_operations erofs_generic_iops = {
>>>>   	.getattr = erofs_getattr,
>>>> -#ifdef CONFIG_EROFS_FS_XATTR
>>>>   	.listxattr = erofs_listxattr,
>>>> -#endif
>>>
>>> It seems equivalent. And it seems ext2 and f2fs behave in the same way...
>>
>> I posted similar patch for ext2 and Jack merged to
>> his tree the other day, though that series also
>> included a real bugfix. I also posted similar patch
>> to f2fs, so if erofs and f2fs merge these patches
>> then all three will behave in the same way, ;-)
>>
>> You may refer below link for the detail.
>>
>> https://lore.kernel.org/linux-ext4/20200522044035.24190-2-cgxu519@mykernel.net/
> 
> Thanks for your link...
> 
>>
>>
>>> But I'm not sure whether we'd return 0 (if I didn't see fs/xattr.c by mistake)
>>> or -EOPNOTSUPP here... Some thoughts about this? >
>>> Anyway, I'm fine with that if return 0 is okay here, but I'd like to know your
>>> and Chao's thoughts about this... I will play with it later as well.

I'm okay with this change, please feel free to add:

Reviewed-by: Chao Yu <yuchao0@huawei.com>

>>
>> 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);
...
}


> 
> Thanks,
> Gao Xiang
> 
>>
>>
>> Thanks,
>> cgxu
>>
> 
> .
> 

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

[Gao Xiang]

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...

Thanks,
Gao Xiang

> 
> 
> > 
> > Thanks,
> > Gao Xiang
> > 
> >>
> >>
> >> Thanks,
> >> cgxu
> >>
> > 
> > .
> > 
> 

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

[cgxu]

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

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

[Gao Xiang]

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
>