LinuxLists.cc - [PATCH 0/5] Miscellaneous trivial fixes

[permalink] [raw]

Subject: [PATCH 1/5] selftests: skip mincore.check_file_mmap when fs lacks needed support

Report mincore.check_file_mmap as SKIP instead of FAIL if the underlying
filesystem lacks support of O_TMPFILE or fallocate since such failures
are not really related to mincore functionality.

Cc: Ricardo Cañuelo <[email protected]>
Signed-off-by: Cristian Marussi <[email protected]>
---
This can happen especially on test-automation systems where rootfs can
be configured as being on NFS or virtual disks.
---
.../selftests/mincore/mincore_selftest.c | 20 +++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/mincore/mincore_selftest.c b/tools/testing/selftests/mincore/mincore_selftest.c
index e54106643337..4c88238fc8f0 100644
--- a/tools/testing/selftests/mincore/mincore_selftest.c
+++ b/tools/testing/selftests/mincore/mincore_selftest.c
@@ -207,15 +207,21 @@ TEST(check_file_mmap)

errno = 0;
fd = open(".", O_TMPFILE | O_RDWR, 0600);
- ASSERT_NE(-1, fd) {
- TH_LOG("Can't create temporary file: %s",
- strerror(errno));
+ if (fd < 0) {
+ ASSERT_EQ(errno, EOPNOTSUPP) {
+ TH_LOG("Can't create temporary file: %s",
+ strerror(errno));
+ }
+ SKIP(goto out_free, "O_TMPFILE not supported by filesystem.");
}
errno = 0;
retval = fallocate(fd, 0, 0, FILE_SIZE);
- ASSERT_EQ(0, retval) {
- TH_LOG("Error allocating space for the temporary file: %s",
- strerror(errno));
+ if (retval) {
+ ASSERT_EQ(errno, EOPNOTSUPP) {
+ TH_LOG("Error allocating space for the temporary file: %s",
+ strerror(errno));
+ }
+ SKIP(goto out_close, "fallocate not supported by filesystem.");
}

/*
@@ -271,7 +277,9 @@ TEST(check_file_mmap)
}

munmap(addr, FILE_SIZE);
+out_close:
close(fd);
+out_free:
free(vec);
}

--
2.17.1

2022-01-26 21:01:54

[permalink] [raw]

Subject: [PATCH 2/5] kselftest: Fix vdso_test_time to pass on skips

When a vDSO symbol is not found, all the testcases in vdso_test_abi usually
report a SKIP, which, in turn, is reported back to Kselftest as a PASS.

Testcase vdso_test_time, instead, reporting a SKIP, causes the whole set of
tests within vdso_test_abi to be considered FAIL when symbol is not found.

Fix it reporting a PASS when vdso_test_time cannot find the vdso symbol.

Cc: Vincenzo Frascino <[email protected]>
Signed-off-by: Cristian Marussi <[email protected]>
---
Seen as a failure on both a JUNO and a Dragonboard on both recent and old
kernels/testruns:

root@deb-buster-arm64:~# /opt/ksft/vDSO/vdso_test_abi
[vDSO kselftest] VDSO_VERSION: LINUX_2.6.39
The time is 1637922136.675304
The time is 1637922136.675361000
The resolution is 0 1
clock_id: CLOCK_REALTIME [PASS]
The time is 1927.760604900
The resolution is 0 1
clock_id: CLOCK_BOOTTIME [PASS]
The time is 1637922136.675649700
The resolution is 0 1
clock_id: CLOCK_TAI [PASS]
The time is 1637922136.672000000
The resolution is 0 4000000
clock_id: CLOCK_REALTIME_COARSE [PASS]
The time is 1927.761005600
The resolution is 0 1
clock_id: CLOCK_MONOTONIC [PASS]
The time is 1927.761132780
The resolution is 0 1
clock_id: CLOCK_MONOTONIC_RAW [PASS]
The time is 1927.757093740
The resolution is 0 4000000
clock_id: CLOCK_MONOTONIC_COARSE [PASS]
Could not find __kernel_time <<< This caused a FAIL as a whole
root@deb-buster-arm64:~# echo $?
1

e.g.: https://lkft.validation.linaro.org/scheduler/job/2192570#L27778
---
tools/testing/selftests/vDSO/vdso_test_abi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/vDSO/vdso_test_abi.c b/tools/testing/selftests/vDSO/vdso_test_abi.c
index 3d603f1394af..7dcc66d1cecf 100644
--- a/tools/testing/selftests/vDSO/vdso_test_abi.c
+++ b/tools/testing/selftests/vDSO/vdso_test_abi.c
@@ -90,8 +90,9 @@ static int vdso_test_time(void)
(vdso_time_t)vdso_sym(version, name[2]);

if (!vdso_time) {
+ /* Skip if symbol not found: consider skipped tests as passed */
printf("Could not find %s\n", name[2]);
- return KSFT_SKIP;
+ return KSFT_PASS;
}

long ret = vdso_time(NULL);
--
2.17.1

2022-01-26 21:01:57

[permalink] [raw]

Subject: [PATCH 5/5] selftests: openat2: Skip testcases that fail with EOPNOTSUPP

Skip testcases that fail since the requested valid flags combination is not
supported by the underlying filesystem.

Cc: Aleksa Sarai <[email protected]>
Signed-off-by: Cristian Marussi <[email protected]>
---
tools/testing/selftests/openat2/openat2_test.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 1bddbe934204..7fb902099de4 100644
--- a/tools/testing/selftests/openat2/openat2_test.c
+++ b/tools/testing/selftests/openat2/openat2_test.c
@@ -259,6 +259,16 @@ void test_openat2_flags(void)
unlink(path);

fd = sys_openat2(AT_FDCWD, path, &test->how);
+ if (fd < 0 && fd == -EOPNOTSUPP) {
+ /*
+ * Skip the testcase if it failed because not supported
+ * by FS. (e.g. a valid O_TMPFILE combination on NFS)
+ */
+ ksft_test_result_skip("openat2 with %s fails with %d (%s)\n",
+ test->name, fd, strerror(-fd));
+ goto next;
+ }
+
if (test->err >= 0)
failed = (fd < 0);
else
@@ -303,7 +313,7 @@ void test_openat2_flags(void)
else
resultfn("openat2 with %s fails with %d (%s)\n",
test->name, test->err, strerror(-test->err));
-
+next:
free(fdpath);
fflush(stdout);
}
--
2.17.1

2022-01-26 21:02:12

[permalink] [raw]

Subject: [PATCH 4/5] selftests: openat2: Add missing dependency in Makefile

Add a dependency on header helpers.h to the main target; while at that add
to helpers.h also a missing include for bool types.

Cc: Aleksa Sarai <[email protected]>
Signed-off-by: Cristian Marussi <[email protected]>
---
tools/testing/selftests/openat2/Makefile | 2 +-
tools/testing/selftests/openat2/helpers.h | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/openat2/Makefile b/tools/testing/selftests/openat2/Makefile
index 4b93b1417b86..843ba56d8e49 100644
--- a/tools/testing/selftests/openat2/Makefile
+++ b/tools/testing/selftests/openat2/Makefile
@@ -5,4 +5,4 @@ TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test

include ../lib.mk

-$(TEST_GEN_PROGS): helpers.c
+$(TEST_GEN_PROGS): helpers.c helpers.h
diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h
index ad5d0ba5b6ce..7056340b9339 100644
--- a/tools/testing/selftests/openat2/helpers.h
+++ b/tools/testing/selftests/openat2/helpers.h
@@ -9,6 +9,7 @@

#define _GNU_SOURCE
#include <stdint.h>
+#include <stdbool.h>
#include <errno.h>
#include <linux/types.h>
#include "../kselftest.h"
--
2.17.1

2022-01-26 21:04:18

[permalink] [raw]

Subject: [PATCH 3/5] selftests: openat2: Print also errno in failure messages

In E_func() macro, on error, print also errno in order to aid debugging.

Cc: Aleksa Sarai <[email protected]>
Signed-off-by: Cristian Marussi <[email protected]>
---
tools/testing/selftests/openat2/helpers.h | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h
index a6ea27344db2..ad5d0ba5b6ce 100644
--- a/tools/testing/selftests/openat2/helpers.h
+++ b/tools/testing/selftests/openat2/helpers.h
@@ -62,11 +62,12 @@ bool needs_openat2(const struct open_how *how);
(similar to chroot(2)). */
#endif /* RESOLVE_IN_ROOT */

-#define E_func(func, ...) \
- do { \
- if (func(__VA_ARGS__) < 0) \
- ksft_exit_fail_msg("%s:%d %s failed\n", \
- __FILE__, __LINE__, #func);\
+#define E_func(func, ...) \
+ do { \
+ errno = 0; \
+ if (func(__VA_ARGS__) < 0) \
+ ksft_exit_fail_msg("%s:%d %s failed - errno:%d\n", \
+ __FILE__, __LINE__, #func, errno); \
} while (0)

#define E_asprintf(...) E_func(asprintf, __VA_ARGS__)
--
2.17.1

2022-01-26 21:14:27

[permalink] [raw]

Subject: Re: [PATCH 2/5] kselftest: Fix vdso_test_time to pass on skips

Hi Cristian,

On 1/26/22 10:27 AM, Cristian Marussi wrote:
> When a vDSO symbol is not found, all the testcases in vdso_test_abi usually
> report a SKIP, which, in turn, is reported back to Kselftest as a PASS.
>
> Testcase vdso_test_time, instead, reporting a SKIP, causes the whole set of
> tests within vdso_test_abi to be considered FAIL when symbol is not found.
>
> Fix it reporting a PASS when vdso_test_time cannot find the vdso symbol.
>
> Cc: Vincenzo Frascino <[email protected]>
> Signed-off-by: Cristian Marussi <[email protected]>
> ---
> Seen as a failure on both a JUNO and a Dragonboard on both recent and old
> kernels/testruns:
>
> root@deb-buster-arm64:~# /opt/ksft/vDSO/vdso_test_abi
> [vDSO kselftest] VDSO_VERSION: LINUX_2.6.39
> The time is 1637922136.675304
> The time is 1637922136.675361000
> The resolution is 0 1
> clock_id: CLOCK_REALTIME [PASS]
> The time is 1927.760604900
> The resolution is 0 1
> clock_id: CLOCK_BOOTTIME [PASS]
> The time is 1637922136.675649700
> The resolution is 0 1
> clock_id: CLOCK_TAI [PASS]
> The time is 1637922136.672000000
> The resolution is 0 4000000
> clock_id: CLOCK_REALTIME_COARSE [PASS]
> The time is 1927.761005600
> The resolution is 0 1
> clock_id: CLOCK_MONOTONIC [PASS]
> The time is 1927.761132780
> The resolution is 0 1
> clock_id: CLOCK_MONOTONIC_RAW [PASS]
> The time is 1927.757093740
> The resolution is 0 4000000
> clock_id: CLOCK_MONOTONIC_COARSE [PASS]
> Could not find __kernel_time <<< This caused a FAIL as a whole
> root@deb-buster-arm64:~# echo $?
> 1
>
> e.g.: https://lkft.validation.linaro.org/scheduler/job/2192570#L27778
> ---
> tools/testing/selftests/vDSO/vdso_test_abi.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/vDSO/vdso_test_abi.c b/tools/testing/selftests/vDSO/vdso_test_abi.c
> index 3d603f1394af..7dcc66d1cecf 100644
> --- a/tools/testing/selftests/vDSO/vdso_test_abi.c
> +++ b/tools/testing/selftests/vDSO/vdso_test_abi.c
> @@ -90,8 +90,9 @@ static int vdso_test_time(void)
> (vdso_time_t)vdso_sym(version, name[2]);
>
> if (!vdso_time) {
> + /* Skip if symbol not found: consider skipped tests as passed */
> printf("Could not find %s\n", name[2]);
> - return KSFT_SKIP;
> + return KSFT_PASS;

My preference would be to keep "KSFT_SKIP" here and verify separately the return
status of each test. This would maintain compliance with the kselftest API.
Could you please test the patch in-reply-to this one (will be sent shortly) and
let me know if it works for you?

If it does feel free to fold it in the next version of your series with your
"Tested-by:" otherwise let me know.

Thanks!

> }
>
> long ret = vdso_time(NULL);
>

--
Regards,
Vincenzo

2022-01-26 21:14:40

[permalink] [raw]

Subject: Re: [PATCH 2/5] kselftest: Fix vdso_test_time to pass on skips

On Wed, Jan 26, 2022 at 12:22:45PM +0000, Vincenzo Frascino wrote:
> Hi Cristian,
>

Hi Vincenzo,

thanks for the feedback.

> On 1/26/22 10:27 AM, Cristian Marussi wrote:
> > When a vDSO symbol is not found, all the testcases in vdso_test_abi usually
> > report a SKIP, which, in turn, is reported back to Kselftest as a PASS.
> >
> > Testcase vdso_test_time, instead, reporting a SKIP, causes the whole set of
> > tests within vdso_test_abi to be considered FAIL when symbol is not found.
> >
> > Fix it reporting a PASS when vdso_test_time cannot find the vdso symbol.
> >
> > Cc: Vincenzo Frascino <[email protected]>
> > Signed-off-by: Cristian Marussi <[email protected]>
> > ---
> > Seen as a failure on both a JUNO and a Dragonboard on both recent and old
> > kernels/testruns:
> >
> > root@deb-buster-arm64:~# /opt/ksft/vDSO/vdso_test_abi
> > [vDSO kselftest] VDSO_VERSION: LINUX_2.6.39
> > The time is 1637922136.675304
> > The time is 1637922136.675361000
> > The resolution is 0 1
> > clock_id: CLOCK_REALTIME [PASS]
> > The time is 1927.760604900
> > The resolution is 0 1
> > clock_id: CLOCK_BOOTTIME [PASS]
> > The time is 1637922136.675649700
> > The resolution is 0 1
> > clock_id: CLOCK_TAI [PASS]
> > The time is 1637922136.672000000
> > The resolution is 0 4000000
> > clock_id: CLOCK_REALTIME_COARSE [PASS]
> > The time is 1927.761005600
> > The resolution is 0 1
> > clock_id: CLOCK_MONOTONIC [PASS]
> > The time is 1927.761132780
> > The resolution is 0 1
> > clock_id: CLOCK_MONOTONIC_RAW [PASS]
> > The time is 1927.757093740
> > The resolution is 0 4000000
> > clock_id: CLOCK_MONOTONIC_COARSE [PASS]
> > Could not find __kernel_time <<< This caused a FAIL as a whole
> > root@deb-buster-arm64:~# echo $?
> > 1
> >
> > e.g.: https://lkft.validation.linaro.org/scheduler/job/2192570#L27778
> > ---
> > tools/testing/selftests/vDSO/vdso_test_abi.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/vDSO/vdso_test_abi.c b/tools/testing/selftests/vDSO/vdso_test_abi.c
> > index 3d603f1394af..7dcc66d1cecf 100644
> > --- a/tools/testing/selftests/vDSO/vdso_test_abi.c
> > +++ b/tools/testing/selftests/vDSO/vdso_test_abi.c
> > @@ -90,8 +90,9 @@ static int vdso_test_time(void)
> > (vdso_time_t)vdso_sym(version, name[2]);
> >
> > if (!vdso_time) {
> > + /* Skip if symbol not found: consider skipped tests as passed */
> > printf("Could not find %s\n", name[2]);
> > - return KSFT_SKIP;
> > + return KSFT_PASS;
>
> My preference would be to keep "KSFT_SKIP" here and verify separately the return
> status of each test. This would maintain compliance with the kselftest API.
> Could you please test the patch in-reply-to this one (will be sent shortly) and
> let me know if it works for you?
>
Sure, I was indeed not sure my solution was what you wanted.

> If it does feel free to fold it in the next version of your series with your
> "Tested-by:" otherwise let me know.

Sure, I'll do and keep you on CC.

Thanks,
Cristian

2022-01-26 21:14:47

[permalink] [raw]

Subject: [PATCH] kselftest: Fix vdso_test_abi return status

vdso_test_abi contains a batch of tests that verify the validity of the
vDSO ABI.

When a vDSO symbol is not found the relevant test is skipped reporting
KSFT_SKIP. All the tests return values are then added in a single
variable which is checked to verify failures. This approach can have
side effects which result in reporting the wrong kselftest exit status.

Fix vdso_test_abi verifying the return code of each test separately.

Cc: Shuah Khan <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Reported-by: Cristian Marussi <[email protected]>
Signed-off-by: Vincenzo Frascino <[email protected]>
---
tools/testing/selftests/vDSO/vdso_test_abi.c | 27 +++++++++++---------
1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/vDSO/vdso_test_abi.c b/tools/testing/selftests/vDSO/vdso_test_abi.c
index 3d603f1394af..3a4efb91b9b2 100644
--- a/tools/testing/selftests/vDSO/vdso_test_abi.c
+++ b/tools/testing/selftests/vDSO/vdso_test_abi.c
@@ -184,10 +184,12 @@ static inline int vdso_test_clock(clockid_t clock_id)
return ret0;
}

+#define VDSO_TESTS_MAX 9
+
int main(int argc, char **argv)
{
unsigned long sysinfo_ehdr = getauxval(AT_SYSINFO_EHDR);
- int ret;
+ int ret[VDSO_TESTS_MAX] = {0};

if (!sysinfo_ehdr) {
printf("AT_SYSINFO_EHDR is not present!\n");
@@ -201,44 +203,45 @@ int main(int argc, char **argv)

vdso_init_from_sysinfo_ehdr(getauxval(AT_SYSINFO_EHDR));

- ret = vdso_test_gettimeofday();
+ ret[0] = vdso_test_gettimeofday();

#if _POSIX_TIMERS > 0

#ifdef CLOCK_REALTIME
- ret += vdso_test_clock(CLOCK_REALTIME);
+ ret[1] = vdso_test_clock(CLOCK_REALTIME);
#endif

#ifdef CLOCK_BOOTTIME
- ret += vdso_test_clock(CLOCK_BOOTTIME);
+ ret[2] = vdso_test_clock(CLOCK_BOOTTIME);
#endif

#ifdef CLOCK_TAI
- ret += vdso_test_clock(CLOCK_TAI);
+ ret[3] = vdso_test_clock(CLOCK_TAI);
#endif

#ifdef CLOCK_REALTIME_COARSE
- ret += vdso_test_clock(CLOCK_REALTIME_COARSE);
+ ret[4] = vdso_test_clock(CLOCK_REALTIME_COARSE);
#endif

#ifdef CLOCK_MONOTONIC
- ret += vdso_test_clock(CLOCK_MONOTONIC);
+ ret[5] = vdso_test_clock(CLOCK_MONOTONIC);
#endif

#ifdef CLOCK_MONOTONIC_RAW
- ret += vdso_test_clock(CLOCK_MONOTONIC_RAW);
+ ret[6] = vdso_test_clock(CLOCK_MONOTONIC_RAW);
#endif

#ifdef CLOCK_MONOTONIC_COARSE
- ret += vdso_test_clock(CLOCK_MONOTONIC_COARSE);
+ ret[7] = vdso_test_clock(CLOCK_MONOTONIC_COARSE);
#endif

#endif

- ret += vdso_test_time();
+ ret[8] = vdso_test_time();

- if (ret > 0)
- return KSFT_FAIL;
+ for (int i = 0; i < VDSO_TESTS_MAX; i++)
+ if (ret[i] == KSFT_FAIL)
+ return KSFT_FAIL;

return KSFT_PASS;
}
--
2.34.1

2022-01-26 21:57:17

[permalink] [raw]

Subject: Re: [PATCH 2/5] kselftest: Fix vdso_test_time to pass on skips

Hi Cristian,

On 1/26/22 12:34 PM, Cristian Marussi wrote:
> Sure, I was indeed not sure my solution was what you wanted.
>

Not a problem and more then anything thank you for reporting the issue.

>> If it does feel free to fold it in the next version of your series with your
>> "Tested-by:" otherwise let me know.
> Sure, I'll do and keep you on CC.

Thanks!

--
Regards,
Vincenzo

2022-01-28 20:42:15

[permalink] [raw]

Subject: Re: [PATCH 0/5] Miscellaneous trivial fixes

On 1/26/22 3:27 AM, Cristian Marussi wrote:
> Hi Shuah,
>
> a small series collecting a few trivial fixes that I have already sent
> previously (~Dec2021) as distinct patches.
>
> They are mostly trivial patches addressing failures that seemed more
> sensible to be marked as skips instead. (at least to me ...).
> Original developers are in Cc. (but not heard back from anyone :D)
>
> Thanks,
> Cristian
>
> Cristian Marussi (5):
> selftests: skip mincore.check_file_mmap when fs lacks needed support
> kselftest: Fix vdso_test_time to pass on skips
> selftests: openat2: Print also errno in failure messages
> selftests: openat2: Add missing dependency in Makefile
> selftests: openat2: Skip testcases that fail with EOPNOTSUPP
>
> .../selftests/mincore/mincore_selftest.c | 20 +++++++++++++------
> tools/testing/selftests/openat2/Makefile | 2 +-
> tools/testing/selftests/openat2/helpers.h | 12 ++++++-----
> .../testing/selftests/openat2/openat2_test.c | 12 ++++++++++-
> tools/testing/selftests/vDSO/vdso_test_abi.c | 3 ++-
> 5 files changed, 35 insertions(+), 14 deletions(-)
>

Thank you Cristian. Please see responses for individual patches.

thanks,
-- Shuah

2022-01-28 20:43:07

[permalink] [raw]

Subject: Re: [PATCH 2/5] kselftest: Fix vdso_test_time to pass on skips

On 1/26/22 3:27 AM, Cristian Marussi wrote:
> When a vDSO symbol is not found, all the testcases in vdso_test_abi usually
> report a SKIP, which, in turn, is reported back to Kselftest as a PASS.
>
> Testcase vdso_test_time, instead, reporting a SKIP, causes the whole set of
> tests within vdso_test_abi to be considered FAIL when symbol is not found.
>
> Fix it reporting a PASS when vdso_test_time cannot find the vdso symbol.
>
> Cc: Vincenzo Frascino <[email protected]>
> Signed-off-by: Cristian Marussi <[email protected]>
> ---
> Seen as a failure on both a JUNO and a Dragonboard on both recent and old
> kernels/testruns:
>
> root@deb-buster-arm64:~# /opt/ksft/vDSO/vdso_test_abi
> [vDSO kselftest] VDSO_VERSION: LINUX_2.6.39
> The time is 1637922136.675304
> The time is 1637922136.675361000
> The resolution is 0 1
> clock_id: CLOCK_REALTIME [PASS]
> The time is 1927.760604900
> The resolution is 0 1
> clock_id: CLOCK_BOOTTIME [PASS]
> The time is 1637922136.675649700
> The resolution is 0 1
> clock_id: CLOCK_TAI [PASS]
> The time is 1637922136.672000000
> The resolution is 0 4000000
> clock_id: CLOCK_REALTIME_COARSE [PASS]
> The time is 1927.761005600
> The resolution is 0 1
> clock_id: CLOCK_MONOTONIC [PASS]
> The time is 1927.761132780
> The resolution is 0 1
> clock_id: CLOCK_MONOTONIC_RAW [PASS]
> The time is 1927.757093740
> The resolution is 0 4000000
> clock_id: CLOCK_MONOTONIC_COARSE [PASS]
> Could not find __kernel_time <<< This caused a FAIL as a whole
> root@deb-buster-arm64:~# echo $?
> 1
>
> e.g.: https://lkft.validation.linaro.org/scheduler/job/2192570#L27778
> ---
> tools/testing/selftests/vDSO/vdso_test_abi.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/vDSO/vdso_test_abi.c b/tools/testing/selftests/vDSO/vdso_test_abi.c
> index 3d603f1394af..7dcc66d1cecf 100644
> --- a/tools/testing/selftests/vDSO/vdso_test_abi.c
> +++ b/tools/testing/selftests/vDSO/vdso_test_abi.c
> @@ -90,8 +90,9 @@ static int vdso_test_time(void)
> (vdso_time_t)vdso_sym(version, name[2]);
>
> if (!vdso_time) {
> + /* Skip if symbol not found: consider skipped tests as passed */
> printf("Could not find %s\n", name[2]);
> - return KSFT_SKIP;
> + return KSFT_PASS;

Skip is a the right option here. Pass indicates that the functionality
has been tested and it passed. There is a clear message that says that
the symbol isn't found

thanks,
-- Shuah

2022-01-28 21:48:57

[permalink] [raw]

Subject: Re: [PATCH 3/5] selftests: openat2: Print also errno in failure messages

On 1/26/22 3:27 AM, Cristian Marussi wrote:
> In E_func() macro, on error, print also errno in order to aid debugging.
>
> Cc: Aleksa Sarai <[email protected]>
> Signed-off-by: Cristian Marussi <[email protected]>
> ---
> tools/testing/selftests/openat2/helpers.h | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h
> index a6ea27344db2..ad5d0ba5b6ce 100644
> --- a/tools/testing/selftests/openat2/helpers.h
> +++ b/tools/testing/selftests/openat2/helpers.h
> @@ -62,11 +62,12 @@ bool needs_openat2(const struct open_how *how);
> (similar to chroot(2)). */
> #endif /* RESOLVE_IN_ROOT */
>
> -#define E_func(func, ...) \
> - do { \
> - if (func(__VA_ARGS__) < 0) \
> - ksft_exit_fail_msg("%s:%d %s failed\n", \
> - __FILE__, __LINE__, #func);\
> +#define E_func(func, ...) \
> + do { \
> + errno = 0; \
> + if (func(__VA_ARGS__) < 0) \
> + ksft_exit_fail_msg("%s:%d %s failed - errno:%d\n", \
> + __FILE__, __LINE__, #func, errno); \
> } while (0)
>
> #define E_asprintf(...) E_func(asprintf, __VA_ARGS__)
>

Looks good. Will apply to linux-kselftest rc3

thanks,
-- Shuah

2022-01-28 21:49:18

[permalink] [raw]

Subject: Re: [PATCH 4/5] selftests: openat2: Add missing dependency in Makefile

On 1/26/22 3:27 AM, Cristian Marussi wrote:
> Add a dependency on header helpers.h to the main target; while at that add
> to helpers.h also a missing include for bool types.
>
> Cc: Aleksa Sarai <[email protected]>
> Signed-off-by: Cristian Marussi <[email protected]>
> ---
> tools/testing/selftests/openat2/Makefile | 2 +-
> tools/testing/selftests/openat2/helpers.h | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/openat2/Makefile b/tools/testing/selftests/openat2/Makefile
> index 4b93b1417b86..843ba56d8e49 100644
> --- a/tools/testing/selftests/openat2/Makefile
> +++ b/tools/testing/selftests/openat2/Makefile
> @@ -5,4 +5,4 @@ TEST_GEN_PROGS := openat2_test resolve_test rename_attack_test
>
> include ../lib.mk
>
> -$(TEST_GEN_PROGS): helpers.c
> +$(TEST_GEN_PROGS): helpers.c helpers.h
> diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h
> index ad5d0ba5b6ce..7056340b9339 100644
> --- a/tools/testing/selftests/openat2/helpers.h
> +++ b/tools/testing/selftests/openat2/helpers.h
> @@ -9,6 +9,7 @@
>
> #define _GNU_SOURCE
> #include <stdint.h>
> +#include <stdbool.h>
> #include <errno.h>
> #include <linux/types.h>
> #include "../kselftest.h"
>

Thanks for the patch. Will apply for linux-kselftest fixes branch for rc3

thanks,
-- Shuah

2022-01-28 21:49:21

[permalink] [raw]

Subject: Re: [PATCH 5/5] selftests: openat2: Skip testcases that fail with EOPNOTSUPP

On 1/26/22 3:27 AM, Cristian Marussi wrote:
> Skip testcases that fail since the requested valid flags combination is not
> supported by the underlying filesystem.
>
> Cc: Aleksa Sarai <[email protected]>
> Signed-off-by: Cristian Marussi <[email protected]>
> ---
> tools/testing/selftests/openat2/openat2_test.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
> index 1bddbe934204..7fb902099de4 100644
> --- a/tools/testing/selftests/openat2/openat2_test.c
> +++ b/tools/testing/selftests/openat2/openat2_test.c
> @@ -259,6 +259,16 @@ void test_openat2_flags(void)
> unlink(path);
>
> fd = sys_openat2(AT_FDCWD, path, &test->how);
> + if (fd < 0 && fd == -EOPNOTSUPP) {
> + /*
> + * Skip the testcase if it failed because not supported
> + * by FS. (e.g. a valid O_TMPFILE combination on NFS)
> + */
> + ksft_test_result_skip("openat2 with %s fails with %d (%s)\n",
> + test->name, fd, strerror(-fd));
> + goto next;
> + }
> +
> if (test->err >= 0)
> failed = (fd < 0);
> else
> @@ -303,7 +313,7 @@ void test_openat2_flags(void)
> else
> resultfn("openat2 with %s fails with %d (%s)\n",
> test->name, test->err, strerror(-test->err));
> -
> +next:
> free(fdpath);
> fflush(stdout);
> }
>

Thanks for the patch. Will apply to linux-kselftest fixes branc for rc3

thanks,
-- Shuah

2022-01-28 22:22:16

[permalink] [raw]

Subject: Re: [PATCH] kselftest: Fix vdso_test_abi return status

On 1/26/22 5:26 AM, Vincenzo Frascino wrote:
> vdso_test_abi contains a batch of tests that verify the validity of the
> vDSO ABI.
>
> When a vDSO symbol is not found the relevant test is skipped reporting
> KSFT_SKIP. All the tests return values are then added in a single
> variable which is checked to verify failures. This approach can have
> side effects which result in reporting the wrong kselftest exit status.
>
> Fix vdso_test_abi verifying the return code of each test separately.
>
> Cc: Shuah Khan <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Reported-by: Cristian Marussi <[email protected]>
> Signed-off-by: Vincenzo Frascino <[email protected]>
> ---
> tools/testing/selftests/vDSO/vdso_test_abi.c | 27 +++++++++++---------
> 1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/tools/testing/selftests/vDSO/vdso_test_abi.c b/tools/testing/selftests/vDSO/vdso_test_abi.c
> index 3d603f1394af..3a4efb91b9b2 100644
> --- a/tools/testing/selftests/vDSO/vdso_test_abi.c
> +++ b/tools/testing/selftests/vDSO/vdso_test_abi.c
> @@ -184,10 +184,12 @@ static inline int vdso_test_clock(clockid_t clock_id)
> return ret0;
> }
>
> +#define VDSO_TESTS_MAX 9
> +
> int main(int argc, char **argv)
> {
> unsigned long sysinfo_ehdr = getauxval(AT_SYSINFO_EHDR);
> - int ret;
> + int ret[VDSO_TESTS_MAX] = {0};
>
> if (!sysinfo_ehdr) {
> printf("AT_SYSINFO_EHDR is not present!\n");
> @@ -201,44 +203,45 @@ int main(int argc, char **argv)
>
> vdso_init_from_sysinfo_ehdr(getauxval(AT_SYSINFO_EHDR));
>
> - ret = vdso_test_gettimeofday();
> + ret[0] = vdso_test_gettimeofday();
>
> #if _POSIX_TIMERS > 0
>
> #ifdef CLOCK_REALTIME
> - ret += vdso_test_clock(CLOCK_REALTIME);
> + ret[1] = vdso_test_clock(CLOCK_REALTIME);
> #endif
>
> #ifdef CLOCK_BOOTTIME
> - ret += vdso_test_clock(CLOCK_BOOTTIME);
> + ret[2] = vdso_test_clock(CLOCK_BOOTTIME);
> #endif
>
> #ifdef CLOCK_TAI
> - ret += vdso_test_clock(CLOCK_TAI);
> + ret[3] = vdso_test_clock(CLOCK_TAI);
> #endif
>
> #ifdef CLOCK_REALTIME_COARSE
> - ret += vdso_test_clock(CLOCK_REALTIME_COARSE);
> + ret[4] = vdso_test_clock(CLOCK_REALTIME_COARSE);
> #endif
>
> #ifdef CLOCK_MONOTONIC
> - ret += vdso_test_clock(CLOCK_MONOTONIC);
> + ret[5] = vdso_test_clock(CLOCK_MONOTONIC);
> #endif
>
> #ifdef CLOCK_MONOTONIC_RAW
> - ret += vdso_test_clock(CLOCK_MONOTONIC_RAW);
> + ret[6] = vdso_test_clock(CLOCK_MONOTONIC_RAW);
> #endif
>
> #ifdef CLOCK_MONOTONIC_COARSE
> - ret += vdso_test_clock(CLOCK_MONOTONIC_COARSE);
> + ret[7] = vdso_test_clock(CLOCK_MONOTONIC_COARSE);
> #endif
>
> #endif
>
> - ret += vdso_test_time();
> + ret[8] = vdso_test_time();
>
> - if (ret > 0)
> - return KSFT_FAIL;
> + for (int i = 0; i < VDSO_TESTS_MAX; i++)
> + if (ret[i] == KSFT_FAIL)
> + return KSFT_FAIL;
>
> return KSFT_PASS;
> }
>

You can use the ksft_* counts interfaces for this instead of adding
counts here. ksft_test_result_*() can be used to increment the right
result counters and then print counts at the end.

Either if there is a failure in any of the tests it will be fail with
clear indication on which tests failed. vdso_test_clock() test for
example is reporting false positives by overriding the Skip return
with a pass.

thanks,
-- Shuah

2022-01-28 22:23:09

[permalink] [raw]

Subject: Re: [PATCH 1/5] selftests: skip mincore.check_file_mmap when fs lacks needed support

On 1/26/22 3:27 AM, Cristian Marussi wrote:
> Report mincore.check_file_mmap as SKIP instead of FAIL if the underlying
> filesystem lacks support of O_TMPFILE or fallocate since such failures
> are not really related to mincore functionality.
>
> Cc: Ricardo Cañuelo <[email protected]>
> Signed-off-by: Cristian Marussi <[email protected]>
> ---

Thanks. Will apply to linux-kselftest fixes for rc3

thanks,
-- Shuah

2022-01-31 07:25:33