* [PATCH] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id @ 2018-07-22 8:50 nixiaoming 2018-07-22 18:12 ` Chuck Lever 0 siblings, 1 reply; 6+ messages in thread From: nixiaoming @ 2018-07-22 8:50 UTC (permalink / raw) To: bfields, jlayton; +Cc: linux-nfs, linux-kernel, nixiaoming dummy = be32_to_cpup(p++); dummy = be32_to_cpup(p++); Assigning value to "dummy" here, but that stored value is overwritten before it can be used. delete invalid assignment statements in nfsd4_decode_exchange_id Signed-off-by: n00202754 <nixiaoming@huawei.com> --- fs/nfsd/nfs4xdr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c index a96843c..8e78541 100644 --- a/fs/nfsd/nfs4xdr.c +++ b/fs/nfsd/nfs4xdr.c @@ -1392,8 +1392,8 @@ nfsd4_decode_exchange_id(struct nfsd4_compoundargs *argp, /* ssp_window and ssp_num_gss_handles */ READ_BUF(8); - dummy = be32_to_cpup(p++); - dummy = be32_to_cpup(p++); + be32_to_cpup(p++); + be32_to_cpup(p++); break; default: goto xdr_error; -- 2.10.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id 2018-07-22 8:50 [PATCH] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id nixiaoming @ 2018-07-22 18:12 ` Chuck Lever 2018-07-22 18:33 ` Trond Myklebust 0 siblings, 1 reply; 6+ messages in thread From: Chuck Lever @ 2018-07-22 18:12 UTC (permalink / raw) To: nixiaoming; +Cc: Bruce Fields, jlayton, Linux NFS Mailing List, linux-kernel > On Jul 22, 2018, at 4:50 AM, nixiaoming <nixiaoming@huawei.com> wrote: > > dummy = be32_to_cpup(p++); > dummy = be32_to_cpup(p++); > Assigning value to "dummy" here, but that stored value > is overwritten before it can be used. > > delete invalid assignment statements in nfsd4_decode_exchange_id > > Signed-off-by: n00202754 <nixiaoming@huawei.com> > --- > fs/nfsd/nfs4xdr.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > index a96843c..8e78541 100644 > --- a/fs/nfsd/nfs4xdr.c > +++ b/fs/nfsd/nfs4xdr.c > @@ -1392,8 +1392,8 @@ nfsd4_decode_exchange_id(struct nfsd4_compoundargs *argp, > > /* ssp_window and ssp_num_gss_handles */ > READ_BUF(8); > - dummy = be32_to_cpup(p++); > - dummy = be32_to_cpup(p++); > + be32_to_cpup(p++); > + be32_to_cpup(p++); If these values are not used, what's the point of byte swapping them? Surely "p += 2;" should be enough. > break; > default: > goto xdr_error; -- Chuck Lever ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id 2018-07-22 18:12 ` Chuck Lever @ 2018-07-22 18:33 ` Trond Myklebust 2018-07-22 19:01 ` Chuck Lever 0 siblings, 1 reply; 6+ messages in thread From: Trond Myklebust @ 2018-07-22 18:33 UTC (permalink / raw) To: nixiaoming, chuck.lever; +Cc: bfields, linux-kernel, linux-nfs, jlayton On Sun, 2018-07-22 at 14:12 -0400, Chuck Lever wrote: > > On Jul 22, 2018, at 4:50 AM, nixiaoming <nixiaoming@huawei.com> > > wrote: > > > > dummy = be32_to_cpup(p++); > > dummy = be32_to_cpup(p++); > > Assigning value to "dummy" here, but that stored value > > is overwritten before it can be used. > > > > delete invalid assignment statements in nfsd4_decode_exchange_id > > > > Signed-off-by: n00202754 <nixiaoming@huawei.com> > > --- > > fs/nfsd/nfs4xdr.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > > index a96843c..8e78541 100644 > > --- a/fs/nfsd/nfs4xdr.c > > +++ b/fs/nfsd/nfs4xdr.c > > @@ -1392,8 +1392,8 @@ nfsd4_decode_exchange_id(struct > > nfsd4_compoundargs *argp, > > > > /* ssp_window and ssp_num_gss_handles */ > > READ_BUF(8); > > - dummy = be32_to_cpup(p++); > > - dummy = be32_to_cpup(p++); > > + be32_to_cpup(p++); > > + be32_to_cpup(p++); > > If these values are not used, what's the point of byte swapping them? > Surely "p += 2;" should be enough. > > > > break; > > default: > > goto xdr_error; > Given that the value of 'p' isn't used either, why not just delete those two lines altogether? -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id 2018-07-22 18:33 ` Trond Myklebust @ 2018-07-22 19:01 ` Chuck Lever 2018-07-22 19:08 ` Trond Myklebust 0 siblings, 1 reply; 6+ messages in thread From: Chuck Lever @ 2018-07-22 19:01 UTC (permalink / raw) To: Trond Myklebust Cc: nixiaoming, Bruce Fields, linux-kernel, Linux NFS Mailing List, jlayton > On Jul 22, 2018, at 2:33 PM, Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Sun, 2018-07-22 at 14:12 -0400, Chuck Lever wrote: >>> On Jul 22, 2018, at 4:50 AM, nixiaoming <nixiaoming@huawei.com> >>> wrote: >>> >>> dummy = be32_to_cpup(p++); >>> dummy = be32_to_cpup(p++); >>> Assigning value to "dummy" here, but that stored value >>> is overwritten before it can be used. >>> >>> delete invalid assignment statements in nfsd4_decode_exchange_id >>> >>> Signed-off-by: n00202754 <nixiaoming@huawei.com> >>> --- >>> fs/nfsd/nfs4xdr.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c >>> index a96843c..8e78541 100644 >>> --- a/fs/nfsd/nfs4xdr.c >>> +++ b/fs/nfsd/nfs4xdr.c >>> @@ -1392,8 +1392,8 @@ nfsd4_decode_exchange_id(struct >>> nfsd4_compoundargs *argp, >>> >>> /* ssp_window and ssp_num_gss_handles */ >>> READ_BUF(8); >>> - dummy = be32_to_cpup(p++); >>> - dummy = be32_to_cpup(p++); >>> + be32_to_cpup(p++); >>> + be32_to_cpup(p++); >> >> If these values are not used, what's the point of byte swapping them? >> Surely "p += 2;" should be enough. >> >> >>> break; >>> default: >>> goto xdr_error; >> > > Given that the value of 'p' isn't used either, why not just delete > those two lines altogether? Sounds OK, READ_BUF is tracking progress through the buffer, and it already updates "p" as a side-effect. Might there be some nearby instances of open-coded "p" updates that could also be removed, for similar reasons? -- Chuck Lever ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id 2018-07-22 19:01 ` Chuck Lever @ 2018-07-22 19:08 ` Trond Myklebust 2018-07-23 2:16 ` Nixiaoming 0 siblings, 1 reply; 6+ messages in thread From: Trond Myklebust @ 2018-07-22 19:08 UTC (permalink / raw) To: chuck.lever; +Cc: bfields, linux-kernel, nixiaoming, linux-nfs, jlayton On Sun, 2018-07-22 at 15:01 -0400, Chuck Lever wrote: > > On Jul 22, 2018, at 2:33 PM, Trond Myklebust < > > trondmy@hammerspace.com> wrote: > > > > On Sun, 2018-07-22 at 14:12 -0400, Chuck Lever wrote: > > > > On Jul 22, 2018, at 4:50 AM, nixiaoming <nixiaoming@huawei.com> > > > > wrote: > > > > > > > > dummy = be32_to_cpup(p++); > > > > dummy = be32_to_cpup(p++); > > > > Assigning value to "dummy" here, but that stored value > > > > is overwritten before it can be used. > > > > > > > > delete invalid assignment statements in > > > > nfsd4_decode_exchange_id > > > > > > > > Signed-off-by: n00202754 <nixiaoming@huawei.com> > > > > --- > > > > fs/nfsd/nfs4xdr.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > > > > index a96843c..8e78541 100644 > > > > --- a/fs/nfsd/nfs4xdr.c > > > > +++ b/fs/nfsd/nfs4xdr.c > > > > @@ -1392,8 +1392,8 @@ nfsd4_decode_exchange_id(struct > > > > nfsd4_compoundargs *argp, > > > > > > > > /* ssp_window and ssp_num_gss_handles */ > > > > READ_BUF(8); > > > > - dummy = be32_to_cpup(p++); > > > > - dummy = be32_to_cpup(p++); > > > > + be32_to_cpup(p++); > > > > + be32_to_cpup(p++); > > > > > > If these values are not used, what's the point of byte swapping > > > them? > > > Surely "p += 2;" should be enough. > > > > > > > > > > break; > > > > default: > > > > goto xdr_error; > > > > Given that the value of 'p' isn't used either, why not just delete > > those two lines altogether? > > Sounds OK, READ_BUF is tracking progress through the buffer, > and it already updates "p" as a side-effect. > > Might there be some nearby instances of open-coded "p" updates > that could also be removed, for similar reasons? > Probably, yes. I believe that we've said before (and Bruce agreed at the time) that we should get rid of READ_BUF() in knfsd as it tends to obfuscate these assignments to 'p'. Does anyone have any free cycles to work on that? -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id 2018-07-22 19:08 ` Trond Myklebust @ 2018-07-23 2:16 ` Nixiaoming 0 siblings, 0 replies; 6+ messages in thread From: Nixiaoming @ 2018-07-23 2:16 UTC (permalink / raw) To: Trond Myklebust, chuck.lever; +Cc: bfields, linux-kernel, linux-nfs, jlayton On Mon, Jul 23, 2018 at 3:08 AM, Trond Myklebust wrote: >On Sun, 2018-07-22 at 15:01 -0400, Chuck Lever wrote: >> > On Jul 22, 2018, at 2:33 PM, Trond Myklebust < >> > trondmy@hammerspace.com> wrote: >> > >> > On Sun, 2018-07-22 at 14:12 -0400, Chuck Lever wrote: >> > > > On Jul 22, 2018, at 4:50 AM, nixiaoming <nixiaoming@huawei.com> >> > > > wrote: >> > > > >> > > > dummy = be32_to_cpup(p++); >> > > > dummy = be32_to_cpup(p++); >> > > > Assigning value to "dummy" here, but that stored value >> > > > is overwritten before it can be used. >> > > > >> > > > delete invalid assignment statements in >> > > > nfsd4_decode_exchange_id >> > > > >> > > > Signed-off-by: n00202754 <nixiaoming@huawei.com> >> > > > --- >> > > > fs/nfsd/nfs4xdr.c | 4 ++-- >> > > > 1 file changed, 2 insertions(+), 2 deletions(-) >> > > > >> > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c >> > > > index a96843c..8e78541 100644 >> > > > --- a/fs/nfsd/nfs4xdr.c >> > > > +++ b/fs/nfsd/nfs4xdr.c >> > > > @@ -1392,8 +1392,8 @@ nfsd4_decode_exchange_id(struct >> > > > nfsd4_compoundargs *argp, >> > > > >> > > > /* ssp_window and ssp_num_gss_handles */ >> > > > READ_BUF(8); >> > > > - dummy = be32_to_cpup(p++); >> > > > - dummy = be32_to_cpup(p++); >> > > > + be32_to_cpup(p++); >> > > > + be32_to_cpup(p++); >> > > >> > > If these values are not used, what's the point of byte swapping >> > > them? >> > > Surely "p += 2;" should be enough. >> > > >> > > >> > > > break; >> > > > default: >> > > > goto xdr_error; >> > >> > Given that the value of 'p' isn't used either, why not just delete >> > those two lines altogether? >> >> Sounds OK, READ_BUF is tracking progress through the buffer, >> and it already updates "p" as a side-effect. >> >> Might there be some nearby instances of open-coded "p" updates >> that could also be removed, for similar reasons? >> > >Probably, yes. I believe that we've said before (and Bruce agreed at >the time) that we should get rid of READ_BUF() in knfsd as it tends to >obfuscate these assignments to 'p'. Does anyone have any free cycles to >work on that? > >-- >Trond Myklebust >Linux NFS client maintainer, Hammerspace >trond.myklebust@hammerspace.com > > Thank you for your review, I will update the patch as soon as possible based on your comments. Thanks ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-07-23 2:16 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-22 8:50 [PATCH] fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id nixiaoming 2018-07-22 18:12 ` Chuck Lever 2018-07-22 18:33 ` Trond Myklebust 2018-07-22 19:01 ` Chuck Lever 2018-07-22 19:08 ` Trond Myklebust 2018-07-23 2:16 ` Nixiaoming
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®