| Message ID | 20171026091938.59247-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Roger Pau Monne writes ("[PATCH for-next 1/9] gcov: return ENOSYS for unimplemented gcov domctl"):
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
...
> default:
> - ret = -EINVAL;
> + ret = -ENOSYS;
> break;
> }
Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
I think this is a bugfix which should go into 4.10. Julien ?
(Subject line changed by me.)
Ian.
Hi, On 27/10/17 14:59, Ian Jackson wrote: > Roger Pau Monne writes ("[PATCH for-next 1/9] gcov: return ENOSYS for unimplemented gcov domctl"): >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > ... >> default: >> - ret = -EINVAL; >> + ret = -ENOSYS; >> break; >> } > > Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com> > > I think this is a bugfix which should go into 4.10. Julien ? > (Subject line changed by me.) Yes I agree. Release-acked-by: Julien Grall <julien.grall@linaro.org> Cheers,
>>> On 26.10.17 at 11:19, <roger.pau@citrix.com> 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). Jan
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@citrix.com> 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.
Thanks for your attention.
Ian.
>>> On 06.11.17 at 12:16, <ian.jackson@eu.citrix.com> 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@citrix.com> 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. 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. Jan
On Mon, Nov 06, 2017 at 05:06:18AM -0700, Jan Beulich wrote: > >>> On 06.11.17 at 12:16, <ian.jackson@eu.citrix.com> 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@citrix.com> 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.
On Tue, Nov 07, 2017 at 09:41:58AM +0000, Roger Pau Monné wrote: > > 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. > Please send a new patch. I believe this one is already applied.
diff --git a/xen/common/gcov/gcov.c b/xen/common/gcov/gcov.c index 2f18f6d176..35653fd8d8 100644 --- 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; }
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wei.liu2@citrix.com> --- xen/common/gcov/gcov.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)