On Thu, 2021-06-03 at 16:33 +0200, Ilya Dryomov wrote:
> On Thu, Jun 3, 2021 at 4:02 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >
> > On Thu, 2021-06-03 at 15:57 +0200, Ilya Dryomov wrote:
> > > On Thu, Jun 3, 2021 at 3:39 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > > >
> > > > Currently ceph_update_snap_realm returns -EINVAL when it hits a decoding
> > > > error, which is the wrong error code. -EINVAL implies that the user gave
> > > > us a bogus argument to a syscall or something similar. -EIO is more
> > > > descriptive when we hit a decoding error.
> > > >
> > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > > ---
> > > > fs/ceph/snap.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/fs/ceph/snap.c b/fs/ceph/snap.c
> > > > index d07c1c6ac8fb..f8cac2abab3f 100644
> > > > --- a/fs/ceph/snap.c
> > > > +++ b/fs/ceph/snap.c
> > > > @@ -807,7 +807,7 @@ int ceph_update_snap_trace(struct ceph_mds_client *mdsc,
> > > > return 0;
> > > >
> > > > bad:
> > > > - err = -EINVAL;
> > > > + err = -EIO;
> > > > fail:
> > > > if (realm && !IS_ERR(realm))
> > > > ceph_put_snap_realm(mdsc, realm);
> > >
> > > Hi Jeff,
> > >
> > > Is this error code propagated anywhere important?
> > >
> > > The vast majority of functions that have something to do with decoding
> > > use EINVAL as a default (usually out-of-bounds) error. I agree that it
> > > is totally ambiguous, but EIO doesn't seem to be any better to me. If
> > > there is a desire to separate these errors, I think we need to pick
> > > something much more distinctive.
> > >
> >
> > When I see EINVAL, I automatically wonder what bogus argument I passed
> > in somewhere, so I find that particularly deceptive here where the bug
> > is either from the MDS or we had some sort of low-level socket handling
> > problem.
> >
> > OTOH, you have a good point. The callers universally ignore the error
> > code from this function. Perhaps we ought to just log a pr_warn message
> > or something if the decoding fails here instead?
>
> There already is one:
>
> 793 bad:
> 794 err = -EINVAL;
> 795 fail:
> 796 if (realm && !IS_ERR(realm))
> 797 ceph_put_snap_realm(mdsc, realm);
> 798 if (first_realm)
> 799 ceph_put_snap_realm(mdsc, first_realm);
> 800 pr_err("update_snap_trace error %d\n", err);
> 801 return err;
>
> Or do you mean specifically the "bad" label?
>
Well, if we have a distinctive error code there, then we won't need a
separate pr_err message or anything. I still think that -EINVAL is not
descriptive of the issue though. I suppose if -EIO is too vague, we
could use something like -EILSEQ ?
--
Jeff Layton <jlayton@xxxxxxxxxx>
