diff mbox series

[net-next,8/8] SUNRPC: use min() to simplify the code

Message ID 20240822133908.1042240-9-lizetao1@huawei.com (mailing list archive)
State New, archived
Headers show
Series Some modifications to optimize code readability | expand

Commit Message

Li Zetao Aug. 22, 2024, 1:39 p.m. UTC
When reading pages in xdr_read_pages, the number of XDR encoded bytes
should be less than the len of aligned pages, so using min() here is
very semantic.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 net/sunrpc/xdr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Simon Horman Aug. 24, 2024, 6:07 p.m. UTC | #1
On Thu, Aug 22, 2024 at 09:39:08PM +0800, Li Zetao wrote:
> When reading pages in xdr_read_pages, the number of XDR encoded bytes
> should be less than the len of aligned pages, so using min() here is
> very semantic.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>  net/sunrpc/xdr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 62e07c330a66..6746e920dbbb 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -1602,7 +1602,7 @@ unsigned int xdr_read_pages(struct xdr_stream *xdr, unsigned int len)
>  	end = xdr_stream_remaining(xdr) - pglen;
>  
>  	xdr_set_tail_base(xdr, base, end);
> -	return len <= pglen ? len : pglen;
> +	return min(len, pglen);

Both len and pglen are unsigned int, so this seems correct to me.

And the code being replaced does appear to be a min() operation in
both form and function.

Reviewed-by: Simon Horman <horms@kernel.org>

However, I don't believe SUNRPC changes usually don't go through next-next.
So I think this either needs to be reposted or get some acks from
Chuck Lever (already CCed).

Chuck, perhaps you can offer some guidance here?

>  }
>  EXPORT_SYMBOL_GPL(xdr_read_pages);
>  
> -- 
> 2.34.1
> 
>
Chuck Lever III Aug. 24, 2024, 6:21 p.m. UTC | #2
On Sat, Aug 24, 2024 at 07:07:50PM +0100, Simon Horman wrote:
> On Thu, Aug 22, 2024 at 09:39:08PM +0800, Li Zetao wrote:
> > When reading pages in xdr_read_pages, the number of XDR encoded bytes
> > should be less than the len of aligned pages, so using min() here is
> > very semantic.
> > 
> > Signed-off-by: Li Zetao <lizetao1@huawei.com>
> > ---
> >  net/sunrpc/xdr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> > index 62e07c330a66..6746e920dbbb 100644
> > --- a/net/sunrpc/xdr.c
> > +++ b/net/sunrpc/xdr.c
> > @@ -1602,7 +1602,7 @@ unsigned int xdr_read_pages(struct xdr_stream *xdr, unsigned int len)
> >  	end = xdr_stream_remaining(xdr) - pglen;
> >  
> >  	xdr_set_tail_base(xdr, base, end);
> > -	return len <= pglen ? len : pglen;
> > +	return min(len, pglen);
> 
> Both len and pglen are unsigned int, so this seems correct to me.
> 
> And the code being replaced does appear to be a min() operation in
> both form and function.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> However, I don't believe SUNRPC changes usually don't go through next-next.
> So I think this either needs to be reposted or get some acks from
> Chuck Lever (already CCed).
> 
> Chuck, perhaps you can offer some guidance here?
> 
> >  }
> >  EXPORT_SYMBOL_GPL(xdr_read_pages);
> >  
> > -- 
> > 2.34.1
> > 
> > 

Changes to net/sunrpc/ can go through Anna and Trond's NFS client
trees, through the NFSD tree, or via netdev, but they are typically
taken through the NFS-related trees.

Unless the submitter or the relevant maintainers prefer otherwise,
I don't see a problem with this one going through netdev. Let me
know otherwise.

Acked-by: Chuck Lever <chuck.lever@oracle.com>
Trond Myklebust Aug. 24, 2024, 6:33 p.m. UTC | #3
On Sat, 2024-08-24 at 14:21 -0400, Chuck Lever wrote:
> On Sat, Aug 24, 2024 at 07:07:50PM +0100, Simon Horman wrote:
> > On Thu, Aug 22, 2024 at 09:39:08PM +0800, Li Zetao wrote:
> > > When reading pages in xdr_read_pages, the number of XDR encoded
> > > bytes
> > > should be less than the len of aligned pages, so using min() here
> > > is
> > > very semantic.
> > > 
> > > Signed-off-by: Li Zetao <lizetao1@huawei.com>
> > > ---
> > >  net/sunrpc/xdr.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> > > index 62e07c330a66..6746e920dbbb 100644
> > > --- a/net/sunrpc/xdr.c
> > > +++ b/net/sunrpc/xdr.c
> > > @@ -1602,7 +1602,7 @@ unsigned int xdr_read_pages(struct
> > > xdr_stream *xdr, unsigned int len)
> > >  	end = xdr_stream_remaining(xdr) - pglen;
> > >  
> > >  	xdr_set_tail_base(xdr, base, end);
> > > -	return len <= pglen ? len : pglen;
> > > +	return min(len, pglen);
> > 
> > Both len and pglen are unsigned int, so this seems correct to me.
> > 
> > And the code being replaced does appear to be a min() operation in
> > both form and function.
> > 
> > Reviewed-by: Simon Horman <horms@kernel.org>
> > 
> > However, I don't believe SUNRPC changes usually don't go through
> > next-next.
> > So I think this either needs to be reposted or get some acks from
> > Chuck Lever (already CCed).
> > 
> > Chuck, perhaps you can offer some guidance here?
> > 
> > >  }
> > >  EXPORT_SYMBOL_GPL(xdr_read_pages);
> > >  
> > > -- 
> > > 2.34.1
> > > 
> > > 
> 
> Changes to net/sunrpc/ can go through Anna and Trond's NFS client
> trees, through the NFSD tree, or via netdev, but they are typically
> taken through the NFS-related trees.
> 
> Unless the submitter or the relevant maintainers prefer otherwise,
> I don't see a problem with this one going through netdev. Let me
> know otherwise.
> 
> Acked-by: Chuck Lever <chuck.lever@oracle.com>
> 
> 

What is the value of this change? Unless the current code is actually
broken or somehow difficult to read, then I much prefer to leave it
untouched so that any future back ports of fixes to code around that
line remain trivial.

So NACK to this change for now.
diff mbox series

Patch

diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
index 62e07c330a66..6746e920dbbb 100644
--- a/net/sunrpc/xdr.c
+++ b/net/sunrpc/xdr.c
@@ -1602,7 +1602,7 @@  unsigned int xdr_read_pages(struct xdr_stream *xdr, unsigned int len)
 	end = xdr_stream_remaining(xdr) - pglen;
 
 	xdr_set_tail_base(xdr, base, end);
-	return len <= pglen ? len : pglen;
+	return min(len, pglen);
 }
 EXPORT_SYMBOL_GPL(xdr_read_pages);