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

[Christos Gkekas]

On 16/10/17 11:12:53 +0200, Ilya Dryomov wrote:
> On Sat, Oct 14, 2017 at 12:01 PM, Christos Gkekas <chris.gekas@xxxxxxxxx> wrote:
> > Variable 'err' is set but never used, so should be removed.
> >
> > Signed-off-by: Christos Gkekas <chris.gekas@xxxxxxxxx>
> > ---
> >  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@xxxxxxxxx>
> 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@xxxxxxxxx>
> 
> 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