From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ilya Dryomov Subject: Re: [PATCH V2] fs:ceph: Remove unused variables in ceph_mdsmap_decode() Date: Wed, 22 Jul 2020 18:23:47 +0200 Message-ID: References: <20200722134604.3026-1-jiayang5@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Return-path: Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41470 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726642AbgGVQXv (ORCPT ); Wed, 22 Jul 2020 12:23:51 -0400 Received: from mail-io1-xd43.google.com (mail-io1-xd43.google.com [IPv6:2607:f8b0:4864:20::d43]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2CE76C0619DC for ; Wed, 22 Jul 2020 09:23:51 -0700 (PDT) Received: by mail-io1-xd43.google.com with SMTP id z6so3183119iow.6 for ; Wed, 22 Jul 2020 09:23:51 -0700 (PDT) In-Reply-To: Sender: ceph-devel-owner@vger.kernel.org List-ID: To: Jeff Layton Cc: Jia Yang , Ceph Development On Wed, Jul 22, 2020 at 5:59 PM Jeff Layton wrote: > > On Wed, 2020-07-22 at 15:53 +0200, Ilya Dryomov wrote: > > On Wed, Jul 22, 2020 at 3:39 PM Jia Yang wrote: > > > Fix build warnings: > > > > > > fs/ceph/mdsmap.c: In function =E2=80=98ceph_mdsmap_decode=E2=80=99: > > > fs/ceph/mdsmap.c:192:7: warning: > > > variable =E2=80=98info_cv=E2=80=99 set but not used [-Wunused-but-set= -variable] > > > fs/ceph/mdsmap.c:177:7: warning: > > > variable =E2=80=98state_seq=E2=80=99 set but not used [-Wunused-but-s= et-variable] > > > fs/ceph/mdsmap.c:123:15: warning: > > > variable =E2=80=98mdsmap_cv=E2=80=99 set but not used [-Wunused-but-s= et-variable] > > > > > > Use ceph_decode_skip_* instead of ceph_decode_*, because p is > > > increased in ceph_decode_*. > > > > > > Signed-off-by: Jia Yang > > > --- > > > 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 =3D *p; > > > int i, j, n; > > > int err; > > > - u8 mdsmap_v, mdsmap_cv; > > > + u8 mdsmap_v; > > > u16 mdsmap_ev; > > > > > > m =3D 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 =3D ceph_decode_8(p); > > > - mdsmap_cv =3D 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 +=3D 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 +=3D 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