| Message ID | 165445911819.1664.8436212544546306528.stgit@bazille.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix NFSv3 READDIRPLUS failures | expand |
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.. 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); > > >
> 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? > 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
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 > > > >
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);
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. Signed-off-by: Chuck Lever <chuck.lever@oracle.com> --- net/sunrpc/xdr.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)