Re: [PATCH V2] fs:ceph: Remove unused variables in ceph_mdsmap_decode()

[Ilya Dryomov <idryomov@gmail.com>]

On Wed, Jul 22, 2020 at 5:59 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Wed, 2020-07-22 at 15:53 +0200, Ilya Dryomov wrote:
> > On Wed, Jul 22, 2020 at 3:39 PM Jia Yang <jiayang5@huawei.com> wrote:
> > > Fix build warnings:
> > >
> > > fs/ceph/mdsmap.c: In function ‘ceph_mdsmap_decode’:
> > > fs/ceph/mdsmap.c:192:7: warning:
> > > variable ‘info_cv’ set but not used [-Wunused-but-set-variable]
> > > fs/ceph/mdsmap.c:177:7: warning:
> > > variable ‘state_seq’ set but not used [-Wunused-but-set-variable]
> > > fs/ceph/mdsmap.c:123:15: warning:
> > > variable ‘mdsmap_cv’ set but not used [-Wunused-but-set-variable]
> > >
> > > Use ceph_decode_skip_* instead of ceph_decode_*, because p is
> > > increased in ceph_decode_*.
> > >
> > > Signed-off-by: Jia Yang <jiayang5@huawei.com>
> > > ---
> > >  fs/ceph/mdsmap.c | 10 ++++------
> > >  1 file changed, 4 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/fs/ceph/mdsmap.c b/fs/ceph/mdsmap.c
> > > index 889627817e52..7455ba83822a 100644
> > > --- a/fs/ceph/mdsmap.c
> > > +++ b/fs/ceph/mdsmap.c
> > > @@ -120,7 +120,7 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end)
> > >         const void *start = *p;
> > >         int i, j, n;
> > >         int err;
> > > -       u8 mdsmap_v, mdsmap_cv;
> > > +       u8 mdsmap_v;
> > >         u16 mdsmap_ev;
> > >
> > >         m = kzalloc(sizeof(*m), GFP_NOFS);
> > > @@ -129,7 +129,7 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end)
> > >
> > >         ceph_decode_need(p, end, 1 + 1, bad);
> > >         mdsmap_v = ceph_decode_8(p);
> > > -       mdsmap_cv = ceph_decode_8(p);
> > > +       ceph_decode_skip_8(p, end, bad);
> >
> > Hi Jia,
> >
> > The bounds are already checked in ceph_decode_need(), so using
> > ceph_decode_skip_*() is unnecessary.  Just increment the position
> > with *p += 1, staying consistent with ceph_decode_8(), which does
> > not bounds check.
> >
>
> I suggested using ceph_decode_skip_*, mostly just because it's more
> self-documenting and I didn't think it that significant an overhead.
> Just incrementing the pointer will also work too, of course.

Either is fine (the overhead is negligible), but I prefer to be
consistent: either ceph_decode_need() + unsafe variants or safe
variants (i.e. ceph_decode_*_safe / ceph_decode_skip_*).

>
> While you're doing that though, please also make note of what would have
> been decoded there too. So in this case, something like this is what I'd
> suggest:
>
>         *p += 1;        /* mdsmap_cv */
>
> These sorts of comments are helpful later, esp. with a protocol like
> ceph that continually has fields being deprecated.

Yup, definitely useful and done in many other places.

Thanks,

                Ilya