| Message ID | 20180722085044.50701-1-nixiaoming@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs/nfsd: Delete invalid assignment statements in nfsd4_decode_exchange_id | expand |
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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
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;
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(-)