* [PATCH V2] fs:ceph: Remove unused variables in ceph_mdsmap_decode()
@ 2020-07-22 13:46 Jia Yang
2020-07-22 13:53 ` Ilya Dryomov
0 siblings, 1 reply; 5+ messages in thread
From: Jia Yang @ 2020-07-22 13:46 UTC (permalink / raw)
To: jlayton, idryomov; +Cc: ceph-devel, jiayang5
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);
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;
--
2.26.0.106.g9fadedd
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH V2] fs:ceph: Remove unused variables in ceph_mdsmap_decode()
2020-07-22 13:46 [PATCH V2] fs:ceph: Remove unused variables in ceph_mdsmap_decode() Jia Yang
@ 2020-07-22 13:53 ` Ilya Dryomov
2020-07-22 15:59 ` Jeff Layton
0 siblings, 1 reply; 5+ messages in thread
From: Ilya Dryomov @ 2020-07-22 13:53 UTC (permalink / raw)
To: Jia Yang; +Cc: Jeff Layton, Ceph Development
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2] fs:ceph: Remove unused variables in ceph_mdsmap_decode()
2020-07-22 13:53 ` Ilya Dryomov
@ 2020-07-22 15:59 ` Jeff Layton
2020-07-22 16:23 ` Ilya Dryomov
0 siblings, 1 reply; 5+ messages in thread
From: Jeff Layton @ 2020-07-22 15:59 UTC (permalink / raw)
To: Ilya Dryomov, Jia Yang; +Cc: Ceph Development
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
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2] fs:ceph: Remove unused variables in ceph_mdsmap_decode()
2020-07-22 15:59 ` Jeff Layton
@ 2020-07-22 16:23 ` Ilya Dryomov
2020-07-23 1:38 ` Jia Yang
0 siblings, 1 reply; 5+ messages in thread
From: Ilya Dryomov @ 2020-07-22 16:23 UTC (permalink / raw)
To: Jeff Layton; +Cc: Jia Yang, Ceph Development
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2] fs:ceph: Remove unused variables in ceph_mdsmap_decode()
2020-07-22 16:23 ` Ilya Dryomov
@ 2020-07-23 1:38 ` Jia Yang
0 siblings, 0 replies; 5+ messages in thread
From: Jia Yang @ 2020-07-23 1:38 UTC (permalink / raw)
To: Ilya Dryomov, Jeff Layton; +Cc: Ceph Development
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
> .
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-23 1:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-22 13:46 [PATCH V2] fs:ceph: Remove unused variables in ceph_mdsmap_decode() Jia Yang
2020-07-22 13:53 ` Ilya Dryomov
2020-07-22 15:59 ` Jeff Layton
2020-07-22 16:23 ` Ilya Dryomov
2020-07-23 1:38 ` Jia Yang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.