| Message ID | 20240822133908.1042240-9-lizetao1@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | Some modifications to optimize code readability | expand |
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 > >
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>
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 --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);
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(-)