| Message ID | CAOi1vP-MTz5sfpYGW_PipSOqKafQoPecSQY4Zq7OAHeUd-o9qw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16/10/17 11:12:53 +0200, Ilya Dryomov wrote: > On Sat, Oct 14, 2017 at 12:01 PM, Christos Gkekas <chris.gekas@gmail.com> wrote: > > Variable 'err' is set but never used, so should be removed. > > > > Signed-off-by: Christos Gkekas <chris.gekas@gmail.com> > > --- > > fs/ceph/mds_client.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > > index f23c820..7cd4fd9 100644 > > --- a/fs/ceph/mds_client.c > > +++ b/fs/ceph/mds_client.c > > @@ -3801,7 +3801,6 @@ void ceph_mdsc_handle_fsmap(struct ceph_mds_client *mdsc, struct ceph_msg *msg) > > 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); > > @@ -3852,7 +3851,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; > > This doesn't seem right. I think the original intent was to return > -EINVAL if decoding fails (like we do in many other places) and -ENOENT > if there is no filesystem with that name. This is user-visible: it's > propagated all the way up and reported by mount(8). > > I'd rather we do something like this: > > commit 2b330133e10e7f53f0b169335c3b6000782e9123 > Author: Ilya Dryomov <idryomov@gmail.com> > Date: Mon Oct 16 10:32:50 2017 +0200 > > ceph: -EINVAL on decoding failure in ceph_mdsc_handle_fsmap() > > Don't set ->mdsmap_err to -ENOENT unconditionally, and drop unneeded > return statement while at it. > > Signed-off-by: Ilya Dryomov <idryomov@gmail.com> > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > index 51f8af685a1f..aaa8879c8098 100644 > --- a/fs/ceph/mds_client.c > +++ b/fs/ceph/mds_client.c > @@ -3878,14 +3878,14 @@ void ceph_mdsc_handle_fsmap(struct > ceph_mds_client *mdsc, struct ceph_msg *msg) > goto err_out; > } > return; > + > bad: > pr_err("error decoding fsmap\n"); > err_out: > mutex_lock(&mdsc->mutex); > - mdsc->mdsmap_err = -ENOENT; > + mdsc->mdsmap_err = err; > __wake_requests(mdsc, &mdsc->waiting_for_map); > mutex_unlock(&mdsc->mutex); > - return; > } > > /* > > Thanks, > > Ilya Thanks very much for your thoughts, this makes sense. Regards, Christos -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c index 51f8af685a1f..aaa8879c8098 100644 --- a/fs/ceph/mds_client.c +++ b/fs/ceph/mds_client.c @@ -3878,14 +3878,14 @@ void ceph_mdsc_handle_fsmap(struct ceph_mds_client *mdsc, struct ceph_msg *msg) goto err_out; } return; + bad: pr_err("error decoding fsmap\n"); err_out: mutex_lock(&mdsc->mutex); - mdsc->mdsmap_err = -ENOENT; + mdsc->mdsmap_err = err; __wake_requests(mdsc, &mdsc->waiting_for_map); mutex_unlock(&mdsc->mutex); - return; }