Re: [Xen-devel] [PATCH for-next 1/9] gcov: return ENOSYS for unimplemented gcov domctl

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Nov 06, 2017 at 05:06:18AM -0700, Jan Beulich wrote:
> >>> On 06.11.17 at 12:16, <ian.jackson@xxxxxxxxxxxxx> wrote:
> > Jan Beulich writes ("Re: [PATCH for-next 1/9] gcov: return ENOSYS for 
> > unimplemented gcov domctl"):
> >> On 26.10.17 at 11:19, <roger.pau@xxxxxxxxxx> wrote:
> >> > --- a/xen/common/gcov/gcov.c
> >> > +++ b/xen/common/gcov/gcov.c
> >> > @@ -239,7 +239,7 @@ int sysctl_gcov_op(struct xen_sysctl_gcov_op *op)
> >> >          break;
> >> >  
> >> >      default:
> >> > -        ret = -EINVAL;
> >> > +        ret = -ENOSYS;
> >> >          break;
> >> >      }
> >> 
> >> Very certainly ENOSYS is not in any way better. Despite the many
> >> misuses of it, we've started enforcing that this wouldn't be spread.
> >> -EOPNOTSUPP may be fine here, but -EINVAL is suitable as well.
> >> -ENOSYS exclusively means that a _top level_ hypercall is
> >> unimplemented (i.e. with very few exceptions there should be
> >> exactly one place where it gets returned, which is in the main
> >> hypercall dispatch code).
> > 
> > The distinction between unimplemented status of a top-level hypercall
> > and unimplemented status of a sub-op is rarely useful to the caller.
> > 
> > Conversely, the distinction between an unimplemented facility, and a
> > facility which is exists but is being used improperly, is vitally
> > important to anyone who is trying to write compatibility code.
> > 
> > I don't mind if you want to insist on the former distinction,
> > reserving ENOSYS for top-level hypercalls and EOPNOTSUPP for other
> > functions.
> > 
> > But I absolutely do mind the use of EINVAL for "unsupported function".
> > I appreciate that much of the hypervisor has historically used EINVAL
> > this way, but this is (a) a pain for callers (b) evil, bad, and wrong
> > (c) unnecessary since EOPNOTSUPP is available.  We should at least not
> > perpetrate any more of this.  In an unreleased API we should change it
> > before release.

This API has actually been released since ~2013 IIRC, when it was
added to Xen.

> Okay, so EOPNOTSUPP is it then, which is also my preference
> (due to there being so many uses of EINVAL elsewhere). I've
> merely mentioned that EINVAL would be suitable since,
> technically speaking, the value in a "sub-operation" field being
> invalid is no different from this being the case for the value in
> any other field.

If I don't get any more comments I will re-send this patch separately
using EOPNOTSUPP instead of ENOSYS. I will also keep the Acks gathered
so far unless anyone objects.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel