| Message ID | 20200722134604.3026-1-jiayang5@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] fs:ceph: Remove unused variables in ceph_mdsmap_decode() | expand |
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. > if (mdsmap_v >= 4) { > u32 mdsmap_len; > ceph_decode_32_safe(p, end, mdsmap_len, bad); > @@ -174,7 +174,6 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end) > u64 global_id; > u32 namelen; > s32 mds, inc, state; > - u64 state_seq; > u8 info_v; > void *info_end = NULL; > struct ceph_entity_addr addr; > @@ -189,9 +188,8 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end) > info_v= ceph_decode_8(p); > if (info_v >= 4) { > u32 info_len; > - u8 info_cv; > ceph_decode_need(p, end, 1 + sizeof(u32), bad); > - info_cv = ceph_decode_8(p); > + ceph_decode_skip_8(p, end, bad); Ditto. > info_len = ceph_decode_32(p); > info_end = *p + info_len; > if (info_end > end) > @@ -210,7 +208,7 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end) > mds = ceph_decode_32(p); > inc = ceph_decode_32(p); > state = ceph_decode_32(p); > - state_seq = ceph_decode_64(p); > + ceph_decode_skip_64(p, end, bad); Ditto. Thanks, Ilya
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. 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. > > if (mdsmap_v >= 4) { > > u32 mdsmap_len; > > ceph_decode_32_safe(p, end, mdsmap_len, bad); > > @@ -174,7 +174,6 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end) > > u64 global_id; > > u32 namelen; > > s32 mds, inc, state; > > - u64 state_seq; > > u8 info_v; > > void *info_end = NULL; > > struct ceph_entity_addr addr; > > @@ -189,9 +188,8 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end) > > info_v= ceph_decode_8(p); > > if (info_v >= 4) { > > u32 info_len; > > - u8 info_cv; > > ceph_decode_need(p, end, 1 + sizeof(u32), bad); > > - info_cv = ceph_decode_8(p); > > + ceph_decode_skip_8(p, end, bad); > > Ditto. > > > info_len = ceph_decode_32(p); > > info_end = *p + info_len; > > if (info_end > end) > > @@ -210,7 +208,7 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end) > > mds = ceph_decode_32(p); > > inc = ceph_decode_32(p); > > state = ceph_decode_32(p); > > - state_seq = ceph_decode_64(p); > > + ceph_decode_skip_64(p, end, bad); > > Ditto. > > Thanks, > > Ilya
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
On 2020/7/23 0:23, Ilya Dryomov wrote: > 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 comments are understandable and useful. I will also use it. Thanks a lot. >> 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 > . >
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); if (mdsmap_v >= 4) { u32 mdsmap_len; ceph_decode_32_safe(p, end, mdsmap_len, bad); @@ -174,7 +174,6 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end) u64 global_id; u32 namelen; s32 mds, inc, state; - u64 state_seq; u8 info_v; void *info_end = NULL; struct ceph_entity_addr addr; @@ -189,9 +188,8 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end) info_v= ceph_decode_8(p); if (info_v >= 4) { u32 info_len; - u8 info_cv; ceph_decode_need(p, end, 1 + sizeof(u32), bad); - info_cv = ceph_decode_8(p); + ceph_decode_skip_8(p, end, bad); info_len = ceph_decode_32(p); info_end = *p + info_len; if (info_end > end) @@ -210,7 +208,7 @@ struct ceph_mdsmap *ceph_mdsmap_decode(void **p, void *end) mds = ceph_decode_32(p); inc = ceph_decode_32(p); state = ceph_decode_32(p); - state_seq = ceph_decode_64(p); + ceph_decode_skip_64(p, end, bad); err = ceph_decode_entity_addr(p, end, &addr); if (err) goto corrupt;
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(-)