| Message ID | cb6173ec393a23949ec40c9747fc7f75fed2591f.1571838908.git.jstancek@redhat.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | timer_create01: accept kernel ENOTSUPP | expand |
----- Original Message ----- > ENOTSUP in userspace is alias for EOPNOTSUPP, so the test still fails. > Add definition of kernel's ENOTSUPP and use that. > > Signed-off-by: Jan Stancek <jstancek@redhat.com> I think I misread the original commit. Seems we want this to fail for ENOTSUPP.
Hi! > > ENOTSUP in userspace is alias for EOPNOTSUPP, so the test still fails. > > Add definition of kernel's ENOTSUPP and use that. > > > > Signed-off-by: Jan Stancek <jstancek@redhat.com> > > I think I misread the original commit. Seems we want this to fail for ENOTSUPP. Indeed.
On Wed, Oct 23, 2019 at 09:58:53AM -0400, Jan Stancek wrote: > > > ----- Original Message ----- > > ENOTSUP in userspace is alias for EOPNOTSUPP, so the test still fails. > > Add definition of kernel's ENOTSUPP and use that. > > > > Signed-off-by: Jan Stancek <jstancek@redhat.com> > > I think I misread the original commit. Seems we want this to fail for ENOTSUPP. > You are right. We want this to succeed with EOPNOTSUPP, as it's a valid error from kernel space. However, we don't want to validate kernels that wrongly return ENOTSUPP, as it's not a valid userspace errno. Any kernel code that exposes ENOTSUPP to userspace is buggy and should be fixed. So, in fact, we would like to flag such return values as failures/kernel bugs. Cascardo. > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
On 10/23/19 4:04 PM, Thadeu Lima de Souza Cascardo wrote: > You are right. We want this to succeed with EOPNOTSUPP, as it's a valid error > from kernel space. Actually, man says that EOPNOTSUPP is only valid for socket operations. So no, we should not go out of our way to explicitly check timer errors against EOPNOTSUPP either. (It's also a waste of time because on Linux, ENOTSUP == EOPNOTSUPP).
Hi! > > You are right. We want this to succeed with EOPNOTSUPP, as it's a valid error > > from kernel space. > > Actually, man says that EOPNOTSUPP is only valid for socket operations. > So no, we should not go out of our way to explicitly check timer errors > against EOPNOTSUPP either. (It's also a waste of time because on Linux, > ENOTSUP == EOPNOTSUPP). Beware that kernel defines ENOTSUP that is not equal to EOPNOTSUPP and in this case this value leaked to userspace leading to invalid userspace errno value.
On Wed, Oct 23, 2019 at 04:35:11PM +0200, Cyril Hrubis wrote: > Hi! > > > You are right. We want this to succeed with EOPNOTSUPP, as it's a valid error > > > from kernel space. > > > > Actually, man says that EOPNOTSUPP is only valid for socket operations. > > So no, we should not go out of our way to explicitly check timer errors > > against EOPNOTSUPP either. (It's also a waste of time because on Linux, > > ENOTSUP == EOPNOTSUPP). > > Beware that kernel defines ENOTSUP that is not equal to EOPNOTSUPP and > in this case this value leaked to userspace leading to invalid userspace > errno value. That was ENOTSUPP (the internal kernel error, defined as 524). ENOTSUP, defined as EOPNOTSUPP, is the userspace error I guess Martin is saying should not be used either. In that case, we need to fix the kernel to return EINVAL instead. Looking at older changes here, I see commit 98d6f4dd84a134d942827584a3c5f67ffd8ec35f ("alarmtimer: return EINVAL instead of ENOTSUPP if rtcdev doesn't exist") claiming exactly this. Though it was about clock_getres and clock_gettime, quoting from that commit: " Second, Posix and Linux man pages agree that clock_gettime and clock_getres should return EINVAL if clk_id argument is invalid. While the arugment that the clockid is valid, but just not supported on this hardware could be made, this is just a technicality that doesn't help userspace applicaitons, and only complicates error handling. " > > -- > Cyril Hrubis > chrubis@suse.cz > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
Hi! > > Beware that kernel defines ENOTSUP that is not equal to EOPNOTSUPP and > > in this case this value leaked to userspace leading to invalid userspace > > errno value. > > That was ENOTSUPP (the internal kernel error, defined as 524). ENOTSUP, defined > as EOPNOTSUPP, is the userspace error I guess Martin is saying should not be > used either. Ah, right, I misunderstand that. > In that case, we need to fix the kernel to return EINVAL instead. Looking at > older changes here, I see commit 98d6f4dd84a134d942827584a3c5f67ffd8ec35f > ("alarmtimer: return EINVAL instead of ENOTSUPP if rtcdev doesn't exist") > claiming exactly this. Though it was about clock_getres and clock_gettime, > quoting from that commit: > > " > Second, Posix and Linux man pages agree that clock_gettime and > clock_getres should return EINVAL if clk_id argument is invalid. > While the arugment that the clockid is valid, but just not supported > on this hardware could be made, this is just a technicality that > doesn't help userspace applicaitons, and only complicates error > handling. > " I would disagree, if you check latest POSIX it has: [ENOTSUP] The implementation does not support the creation of a timer attached to the CPU-time clock that is specified by clock_id and associated with a process or thread different from the process or thread invoking timer_create(). https://pubs.opengroup.org/onlinepubs/9699919799/ So the implementation is required to return ENOTSUPP in certain cases anyways so applying it to CLOCK_REALTIME_ALARM and CLOCK_BOOTTIME_ALARM certainly makes sense.
Hi! > I would disagree, if you check latest POSIX it has: > > [ENOTSUP] > The implementation does not support the creation of a timer attached > to the CPU-time clock that is specified by clock_id and associated > with a process or thread different from the process or thread > invoking timer_create(). > > https://pubs.opengroup.org/onlinepubs/9699919799/ > > So the implementation is required to return ENOTSUPP in certain cases ^ And again, should have been ENOTSUP! > anyways so applying it to CLOCK_REALTIME_ALARM and > CLOCK_BOOTTIME_ALARM certainly makes sense. > > -- > Cyril Hrubis > chrubis@suse.cz > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
On Wed, Oct 23, 2019 at 05:36:31PM +0200, Cyril Hrubis wrote: > Hi! > > > Beware that kernel defines ENOTSUP that is not equal to EOPNOTSUPP and > > > in this case this value leaked to userspace leading to invalid userspace > > > errno value. > > > > That was ENOTSUPP (the internal kernel error, defined as 524). ENOTSUP, defined > > as EOPNOTSUPP, is the userspace error I guess Martin is saying should not be > > used either. > > Ah, right, I misunderstand that. > > > In that case, we need to fix the kernel to return EINVAL instead. Looking at > > older changes here, I see commit 98d6f4dd84a134d942827584a3c5f67ffd8ec35f > > ("alarmtimer: return EINVAL instead of ENOTSUPP if rtcdev doesn't exist") > > claiming exactly this. Though it was about clock_getres and clock_gettime, > > quoting from that commit: > > > > " > > Second, Posix and Linux man pages agree that clock_gettime and > > clock_getres should return EINVAL if clk_id argument is invalid. > > While the arugment that the clockid is valid, but just not supported > > on this hardware could be made, this is just a technicality that > > doesn't help userspace applicaitons, and only complicates error > > handling. > > " > > I would disagree, if you check latest POSIX it has: > > [ENOTSUP] > The implementation does not support the creation of a timer attached > to the CPU-time clock that is specified by clock_id and associated > with a process or thread different from the process or thread > invoking timer_create(). > > https://pubs.opengroup.org/onlinepubs/9699919799/ > > So the implementation is required to return ENOTSUPP in certain cases > anyways so applying it to CLOCK_REALTIME_ALARM and > CLOCK_BOOTTIME_ALARM certainly makes sense. > So, if this is a matter of EOPNOTSUPP versus ENOTSUP (the userspace ones), then the code that is applied to LTP uses ENOTSUP, which is what POSIX uses, so all fine from the LTP standpoint. To be honest, I am relieved about not getting to go through the process of fixing this in the kernel once again. Maybe we should even do the opposite and make clock_gettime and clock_getres return ENOTSUP/EOPNOTSUPP. Cascardo. > -- > Cyril Hrubis > chrubis@suse.cz
diff --git a/include/lapi/errno.h b/include/lapi/errno.h new file mode 100644 index 000000000000..22c2d4d279d8 --- /dev/null +++ b/include/lapi/errno.h @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2019 Linux Test Project + */ + +#ifndef _LAPI_ERRNO_H_ +#define _LAPI_ERRNO_H_ + +#define K_ENOTSUPP 524 /* Operation is not supported */ + +#endif diff --git a/testcases/kernel/syscalls/timer_create/timer_create01.c b/testcases/kernel/syscalls/timer_create/timer_create01.c index 1d0e400d3260..968f917e7876 100644 --- a/testcases/kernel/syscalls/timer_create/timer_create01.c +++ b/testcases/kernel/syscalls/timer_create/timer_create01.c @@ -27,6 +27,7 @@ #include <time.h> #include "tst_test.h" #include "tst_safe_macros.h" +#include "lapi/errno.h" #include "lapi/common_timers.h" static struct notif_type { @@ -81,7 +82,7 @@ static void run(unsigned int n) if (TST_RET != 0) { if (possibly_unsupported(clock) && - (TST_ERR == EINVAL || TST_ERR == ENOTSUP)) { + (TST_ERR == EINVAL || TST_ERR == K_ENOTSUPP)) { tst_res(TPASS | TTERRNO, "%s unsupported, failed as expected", get_clock_str(clock));
ENOTSUP in userspace is alias for EOPNOTSUPP, so the test still fails. Add definition of kernel's ENOTSUPP and use that. Signed-off-by: Jan Stancek <jstancek@redhat.com> --- include/lapi/errno.h | 11 +++++++++++ testcases/kernel/syscalls/timer_create/timer_create01.c | 3 ++- 2 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 include/lapi/errno.h