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? -- Jeff Layton <jlayton@xxxxxxxxxx>
