Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Chuck Lever III" <chuck.lever@oracle.com>
Cc:     "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v1 3/5] SUNRPC: Clean up xdr_commit_encode()
In-reply-to: <BCB98EE4-9824-4147-8D21-47110914D51F@oracle.com>
References: <165445865736.1664.4497130284832282034.stgit@bazille.1015granger.net>,
 <165445911819.1664.8436212544546306528.stgit@bazille.1015granger.net>,
 <165456394819.22243.15333379326870400835@noble.neil.brown.name>,
 <BCB98EE4-9824-4147-8D21-47110914D51F@oracle.com>
Date:   Tue, 07 Jun 2022 12:37:58 +1000
Message-id: <165456947808.22243.6052931047455826981@noble.neil.brown.name>
Precedence: bulk

On Tue, 07 Jun 2022, Chuck Lever III wrote:
> 
> > On Jun 6, 2022, at 9:05 PM, NeilBrown <neilb@suse.de> wrote:
> > 
> > On Mon, 06 Jun 2022, Chuck Lever wrote:
> >> Both the ::iov_len field and the third parameter of memcpy() and
> >> memmove() are size_t. There's no reason for the implicit conversion
> >> from size_t to int and back. Change the type of @shift to make the
> >> code easier to read and understand.
> > 
> > I wouldn't object to "shift" being renamed "frag1bytes" to make the
> > connection with xdr_get_next_encode_buffer() more blatant..
> 
> I thought of that. Readers would wonder why frag1bytes in
> xdr_commit_encode() was a size_t, but in xdr_get_next_encode_buffer()
> it's a signed int. It started to become a longer string to pull.
> Maybe it's worth it?
> 

Probably worth it.  Not necessarily worth it today.  I have no strong
preference.

NeilBrown

> 
> > Reviewed-by: NeilBrown <neilb@suse.de>
> > 
> > Thanks,
> > NeilBrown
> > 
> > 
> >> 
> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >> ---
> >> net/sunrpc/xdr.c |    4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> >> index 08a85375b311..de8f71468637 100644
> >> --- a/net/sunrpc/xdr.c
> >> +++ b/net/sunrpc/xdr.c
> >> @@ -933,14 +933,16 @@ EXPORT_SYMBOL_GPL(xdr_init_encode);
> >>  */
> >> inline void xdr_commit_encode(struct xdr_stream *xdr)
> >> {
> >> -	int shift = xdr->scratch.iov_len;
> >> +	size_t shift = xdr->scratch.iov_len;
> >> 	void *page;
> >> 
> >> 	if (shift == 0)
> >> 		return;
> >> +
> >> 	page = page_address(*xdr->page_ptr);
> >> 	memcpy(xdr->scratch.iov_base, page, shift);
> >> 	memmove(page, page + shift, (void *)xdr->p - page);
> >> +
> >> 	xdr_reset_scratch_buffer(xdr);
> >> }
> >> EXPORT_SYMBOL_GPL(xdr_commit_encode);
> >> 
> >> 
> >> 
> 
> --
> Chuck Lever
> 
> 
> 
>