linux-kernel - Re: [PATCH 3/5] nfsd: use DEFINE_SHOW_ATTRIBUTE to define client_info

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <0376BA94-9B04-47FF-986E-8DD1D3FF1EE5@oracle.com>
Date:   Thu, 22 Sep 2022 17:08:41 +0000
From:   Chuck Lever III <chuck.lever@...cle.com>
To:     ChenXiaoSong <chenxiaosong2@...wei.com>
CC:     Jeff Layton <jlayton@...nel.org>,
        Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "yi.zhang@...wei.com" <yi.zhang@...wei.com>,
        "zhangxiaoxu5@...wei.com" <zhangxiaoxu5@...wei.com>
Subject: Re: [PATCH 3/5] nfsd: use DEFINE_SHOW_ATTRIBUTE to define
 client_info_fops



> On Sep 22, 2022, at 12:31 PM, ChenXiaoSong <chenxiaosong2@...wei.com> wrote:
> 
> Use DEFINE_SHOW_ATTRIBUTE helper macro to simplify the code.
> 
> inode is converted from seq_file->file instead of seq_file->private in
> client_info_show().

Generally speaking, the patch description should explain why
a change is being made. We can already see that @inode is
converted by looking at the diff below, so the above comment
doesn't add value. Same remark applies to 4/5.

What might work better: "The underlying inode needed by
client_info_show() is already available via struct seq_file.
Thus it is safe to replace client_info_open() with the open
function implicitly defined by DEFINE_SHOW_ATTRIBUTE()."

I'll apply these as-is to nfsd for-next to get them into the
testing pipeline. Thanks!


> Signed-off-by: ChenXiaoSong <chenxiaosong2@...wei.com>
> ---
> fs/nfsd/nfs4state.c | 14 ++------------
> 1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c5d199d7e6b4..4017721a04c7 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2478,7 +2478,7 @@ static const char *cb_state2str(int state)
> 
> static int client_info_show(struct seq_file *m, void *v)
> {
> -	struct inode *inode = m->private;
> +	struct inode *inode = file_inode(m->file);
> 	struct nfs4_client *clp;
> 	u64 clid;
> 
> @@ -2518,17 +2518,7 @@ static int client_info_show(struct seq_file *m, void *v)
> 	return 0;
> }
> 
> -static int client_info_open(struct inode *inode, struct file *file)
> -{
> -	return single_open(file, client_info_show, inode);
> -}
> -
> -static const struct file_operations client_info_fops = {
> -	.open		= client_info_open,
> -	.read		= seq_read,
> -	.llseek		= seq_lseek,
> -	.release	= single_release,
> -};
> +DEFINE_SHOW_ATTRIBUTE(client_info);
> 
> static void *states_start(struct seq_file *s, loff_t *pos)
> 	__acquires(&clp->cl_lock)
> -- 
> 2.31.1
> 

--
Chuck Lever