Re: [PATCH v4 1/8] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip

From: "xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>
To: Dave Chinner <david@fromorbit.com>
Cc: Christian Brauner <brauner@kernel.org>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	"ceph-devel@vger.kernel.org" <ceph-devel@vger.kernel.org>,
	"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
	"linux-xfs@vger.kernel.org" <linux-xfs@vger.kernel.org>,
	"viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
	"djwong@kernel.org" <djwong@kernel.org>,
	"jlayton@kernel.org" <jlayton@kernel.org>,
	"ntfs3@lists.linux.dev" <ntfs3@lists.linux.dev>,
	"chao@kernel.org" <chao@kernel.org>,
	"linux-f2fs-devel@lists.sourceforge.net"
	<linux-f2fs-devel@lists.sourceforge.net>
Subject: Re: [PATCH v4 1/8] fs: move sgid strip operation from inode_init_owner into inode_sgid_strip
Date: Thu, 21 Apr 2022 01:11:57 +0000	[thread overview]
Message-ID: <6260BDB8.3020001@fujitsu.com> (raw)
In-Reply-To: <20220420215252.GO1544202@dread.disaster.area>

on 2022/4/21 5:52, Dave Chinner wrote:
> On Wed, Apr 20, 2022 at 01:27:39AM +0000, xuyang2018.jy@fujitsu.com wrote:
>> on 2022/4/19 22:05, Christian Brauner wrote:
>>> On Tue, Apr 19, 2022 at 07:47:07PM +0800, Yang Xu wrote:
>>>> This has no functional change. Just create and export inode_sgid_strip api for
>>>> the subsequent patch. This function is used to strip S_ISGID mode when init
>>>> a new inode.
>>>>
>>>> Acked-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>>>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>>>> ---
>>>>    fs/inode.c         | 22 ++++++++++++++++++----
>>>>    include/linux/fs.h |  3 ++-
>>>>    2 files changed, 20 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/fs/inode.c b/fs/inode.c
>>>> index 9d9b422504d1..3215e61a0021 100644
>>>> --- a/fs/inode.c
>>>> +++ b/fs/inode.c
>>>> @@ -2246,10 +2246,8 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
>>>>    		/* Directories are special, and always inherit S_ISGID */
>>>>    		if (S_ISDIR(mode))
>>>>    			mode |= S_ISGID;
>>>> -		else if ((mode&   (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)&&
>>>> -			 !in_group_p(i_gid_into_mnt(mnt_userns, dir))&&
>>>> -			 !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
>>>> -			mode&= ~S_ISGID;
>>>> +		else
>>>> +			inode_sgid_strip(mnt_userns, dir,&mode);
>>>>    	} else
>>>>    		inode_fsgid_set(inode, mnt_userns);
>>>>    	inode->i_mode = mode;
>>>> @@ -2405,3 +2403,19 @@ struct timespec64 current_time(struct inode *inode)
>>>>    	return timestamp_truncate(now, inode);
>>>>    }
>>>>    EXPORT_SYMBOL(current_time);
>>>> +
>>>> +void inode_sgid_strip(struct user_namespace *mnt_userns,
>>>> +		      const struct inode *dir, umode_t *mode)
>>>> +{
>>>
>>> I think with Willy agreeing in an earlier version with me and you
>>> needing to resend anyway I'd say have this return umode_t instead of
>>> passing a pointer.
>>
>> IMO, I am fine with your and Willy way. But I need a reason otherwise
>> I can't convince myself why not use mode pointer directly.
>
> You should listen to experienced developers like Willy and Christian
> when they say "follow existing coding conventions".  Indeed, Darrick
> has also mentioned he'd prefer it to return the new mode, and I'd
> also prefer that it returns the new mode.
OK. I just don't know  the "follow existing coding conventions" reason. 
Now, I know and understand.
>
>> I have asked you and Willy before why return umode_t value is better,
>> why not modify mode pointer directly? Since we have use mode as
>> argument, why not modify mode pointer directly in function?
>
> If the function had mulitple return status (e.g. an error or a mode)
> the convention is to pass the mode output variable by reference and
> return the error status. But there is only one return value from
> this function - the mode - and hence it should be returned in the
> return value, not passed by reference.
>
> Passing by reference unnecessarily makes the code more complex and
> less mainatainable.  Code that returns a single value is easy to
> understand, is more flexible in the way callers can use it and it's
> simpler to maintain.
OK,  it sounds reasonable and will use return value.

ps: Of course, I will remember this in my mind. Thanks for your replay.

Best Regards
Yang Xu

>
> Cheers,
>
> Dave.