Re: [PATCH] ceph: Delete unused variables in mds_client

[Christos Gkekas]

On 12/10/17 15:45:42 -0700, Gregory Farnum wrote:
> On Thu, Oct 12, 2017 at 2:55 PM, Christos Gkekas <chris.gekas@xxxxxxxxx> wrote:
> > Remove variables in mds_client that are set but never used.
> 
> Hmm, it looks like all the values removed here (except err) *are* used
> and this patch would break the world. Specifically...
> 
> >
> > Signed-off-by: Christos Gkekas <chris.gekas@xxxxxxxxx>
> > ---
> >  fs/ceph/mds_client.c | 11 -----------
> >  1 file changed, 11 deletions(-)
> >
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index f23c820..cadf9b6 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -3797,11 +3797,8 @@ void ceph_mdsc_handle_fsmap(struct ceph_mds_client *mdsc, struct ceph_msg *msg)
> >         void *p = msg->front.iov_base;
> >         void *end = p + msg->front.iov_len;
> >         u32 epoch;
> > -       u32 map_len;
> >         u32 num_fs;
> >         u32 mount_fscid = (u32)-1;
> > -       u8 struct_v, struct_cv;
> > -       int err = -EINVAL;
> >
> >         ceph_decode_need(&p, end, sizeof(u32), bad);
> >         epoch = ceph_decode_32(&p);
> > @@ -3809,10 +3806,6 @@ void ceph_mdsc_handle_fsmap(struct ceph_mds_client *mdsc, struct ceph_msg *msg)
> >         dout("handle_fsmap epoch %u\n", epoch);
> >
> >         ceph_decode_need(&p, end, 2 + sizeof(u32), bad);
> > -       struct_v = ceph_decode_8(&p);
> > -       struct_cv = ceph_decode_8(&p);
> > -       map_len = ceph_decode_32(&p);
> > -
> 
> p here is a pointer into a buffer which was sent to us over the
> network (msg->front.iov_base, shown at top of patch). ceph_decode is
> reading data out of that buffer into these variables and advancing p
> for the subsequent readers. If you drop these statements, we start
> trying to read values at the wrong offsets, and obviously nothing
> makes sense.
> 
> >         ceph_decode_need(&p, end, sizeof(u32) * 3, bad);
> >         p += sizeof(u32) * 2; /* skip epoch and legacy_client_fscid */
> >
> > @@ -3820,12 +3813,9 @@ void ceph_mdsc_handle_fsmap(struct ceph_mds_client *mdsc, struct ceph_msg *msg)
> >         while (num_fs-- > 0) {
> >                 void *info_p, *info_end;
> >                 u32 info_len;
> > -               u8 info_v, info_cv;
> >                 u32 fscid, namelen;
> >
> >                 ceph_decode_need(&p, end, 2 + sizeof(u32), bad);
> > -               info_v = ceph_decode_8(&p);
> > -               info_cv = ceph_decode_8(&p);
> >                 info_len = ceph_decode_32(&p);
> >                 ceph_decode_need(&p, end, info_len, bad);
> >                 info_p = p;
> > @@ -3852,7 +3842,6 @@ void ceph_mdsc_handle_fsmap(struct ceph_mds_client *mdsc, struct ceph_msg *msg)
> >                                    0, true);
> >                 ceph_monc_renew_subs(&fsc->client->monc);
> >         } else {
> > -               err = -ENOENT;
> >                 goto err_out;
> 
> Amusingly, this bit does seem to be correct, as the err_out block just
> unconditionally sets mdsc->mdsmap_err to -ENOENT (and is only
> reachable from this goto).
> 
> >         }
> >         return;
> 
> I assume this patch was generated by some code cleanliness tool, so it
> appears to need some work... :)
> -Greg

Oops, you are absolutely right. GCC is warning about those variables
being unused, but it is clearly wrong. They are used and are indeed very
important for decoding the input data as you correctly highlighted.

Thanks for your time and sorry for sending this across.
Will resend another patch just removing 'err'.

Christos Gkekas