* [PATCH] ceph: Delete unused variables in mds_client @ 2017-10-12 21:55 Christos Gkekas 2017-10-12 22:45 ` Gregory Farnum 0 siblings, 1 reply; 3+ messages in thread From: Christos Gkekas @ 2017-10-12 21:55 UTC (permalink / raw) To: Yan, Zheng, Sage Weil, Ilya Dryomov, ceph-devel, linux-kernel Cc: Christos Gkekas Remove variables in mds_client that are set but never used. Signed-off-by: Christos Gkekas <chris.gekas@gmail.com> --- 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); - 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; } return; -- 2.7.4 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ceph: Delete unused variables in mds_client 2017-10-12 21:55 [PATCH] ceph: Delete unused variables in mds_client Christos Gkekas @ 2017-10-12 22:45 ` Gregory Farnum 2017-10-14 8:42 ` Christos Gkekas 0 siblings, 1 reply; 3+ messages in thread From: Gregory Farnum @ 2017-10-12 22:45 UTC (permalink / raw) To: Christos Gkekas Cc: Yan, Zheng, Sage Weil, Ilya Dryomov, ceph-devel, linux-kernel On Thu, Oct 12, 2017 at 2:55 PM, Christos Gkekas <chris.gekas@gmail.com> 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@gmail.com> > --- > 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ceph: Delete unused variables in mds_client 2017-10-12 22:45 ` Gregory Farnum @ 2017-10-14 8:42 ` Christos Gkekas 0 siblings, 0 replies; 3+ messages in thread From: Christos Gkekas @ 2017-10-14 8:42 UTC (permalink / raw) To: Gregory Farnum Cc: Yan, Zheng, Sage Weil, Ilya Dryomov, ceph-devel, linux-kernel On 12/10/17 15:45:42 -0700, Gregory Farnum wrote: > On Thu, Oct 12, 2017 at 2:55 PM, Christos Gkekas <chris.gekas@gmail.com> 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@gmail.com> > > --- > > 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-10-14 8:42 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-12 21:55 [PATCH] ceph: Delete unused variables in mds_client Christos Gkekas 2017-10-12 22:45 ` Gregory Farnum 2017-10-14 8:42 ` Christos Gkekas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox