On Wed, Jul 22, 2020 at 5:59 PM Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx> > > > --- > > > 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