| Message ID | 20240229155235.263157-1-laura.nao@collabora.com |
|---|---|
| State | Accepted |
| Commit | 5d94da7ff00ef45737a64d947e7ff45aca972782 |
| Headers | show |
| Series | [v5] kselftest: Add basic test for probing the rust sample modules | expand |
Hi Miguel, On 2/29/24 17:44, Miguel Ojeda wrote: > On Thu, Feb 29, 2024 at 4:53 PM Laura Nao <laura.nao@collabora.com> wrote: >> >> Add new basic kselftest that checks if the available rust sample modules >> can be added and removed correctly. >> >> Signed-off-by: Laura Nao <laura.nao@collabora.com> >> Reviewed-by: Sergio Gonzalez Collado <sergio.collado@gmail.com> >> Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > > Thanks for this Laura! > > Replying here to what you wrote in v4: > >> At first, I hadn't planned for the kselftest to skip entirely if only >> one of the two sample modules was missing. However, considering that >> this kselftest is designed to test all available sample modules, and >> given that both are enabled with the provided configuration file, I >> believe it's more logical to verify the presence of both modules before >> running the test. If either of them is missing, then we exit the test >> with a skip code. This also covers the case where rust is not available. > > I guess it depends on what is the expected behavior in kselftests in > general and whether the user is expected to have merged the provided > `config` or not. > It's my understanding (and please correct if I'm wrong) that when a kselftest is shipped with a config file, that config file should be treated as a requirement for the test and the user is expected to use it (running make kselftest-merge). I agree the script shouldn't blow up if the user doesn't though, so it still makes sense to gracefully skip the test when the requirements are not met. > Also, what about modules being built-in / `--first-run` in `modprobe`? > `modprobe` by default may return successfully even if no module was > loaded (or even present, if it was builtin). In that case, is a > kselftest script supposed to succeed, skip or fail? I would say at the > least it should be "skip" (like it is done in the case where the > module is not found), and I wouldn't mind "fail" either (i.e. running > `modprobe` with `--first-run`). > This makes me realize that I should probably put these in the config too: CONFIG_MODULES=y CONFIG_MODULE_UNLOAD=y Adding --first-time (you meant --first-time, right?) definitely makes sense, thanks for the pointer. I think having the modules being built-in should be treated as a skip, same as when they are not there at all. So something like this: for sample in "${rust_sample_modules[@]}"; do - if ! /sbin/modprobe -n -q "$sample"; then + if ! /sbin/modprobe -n -q --first-time "$sample"; then ktap_skip_all "module $sample is not found in /lib/modules/$(uname -r)" exit "$KSFT_SKIP" fi will cover both cases. > In addition, what about module removal failures? Are they ignored on > purpose, e.g. because the kernel might not be configured with module > unloading? If it is possible to check whether `MODULE_UNLOAD` is > supported in the current config, it would be nice to check the removal > also worked. And if it is not supported, skipping the removal entirely. > I think it's safe to assume no other module will depend on the sample rust modules, so is there any other reason unloading the modules might fail apart from MODULE_UNLOAD not being enabled? If not, then I think we should just check if the removal worked and continue/skip the test accordingly. I can't just simply skip all tests like this though: for sample in "${rust_sample_modules[@]}"; do if /sbin/modprobe -q "$sample"; then - /sbin/modprobe -q -r "$sample" + if ! /sbin/modprobe -q -r "$sample"; then + ktap_skip_all "Failed to unload module $sample, please enable CONFIG_MODULE_UNLOAD" + exit "$KSFT_SKIP" + fi ktap_test_pass "$sample" else ktap_test_fail "$sample" as the test plan has already been printed by then. I'll need to rework the script a bit to skip the test upon errors on module removal. > Finally, what about the case where `RUST` isn't enabled? I think Shuah > mentioned it in a previous version. > When rust is not enabled, no sample module is enabled either so the test would still catch this in the first `if ! /sbin/modprobe -n -q --first-time "$sample"` block and exit with the skip code. If we need more granularity on the feedback provided to the user (i.e. indication on what particular options are missing), then I guess we could check the current kernel config (/proc/config.gz) and skip the entire test if any required config is missing. However, this adds an extra dependency on CONFIG_IKCONFIG=y and CONFIG_IKCONFIG_PROC=y. Any advice on the best approach here? >> +KTAP_HELPERS="${DIR}/../kselftest/ktap_helpers.sh" >> +if [ -e "$KTAP_HELPERS" ]; then >> + source "$KTAP_HELPERS" >> +else >> + echo "$KTAP_HELPERS file not found [SKIP]" >> + exit 4 >> +fi > > I am not sure I understand this. In which situation could this happen? > The helpers should always be there, no? I tested this with `make > -C...../selftests install TARGETS=rust INSTALL_PATH=...` and it seems > to work in that case too. > > To be clear, I agree with Shuah that we should test that everything is > working as expected. In fact, I would prefer to run with `-e` or, much > better, use something else than bash :) But if something should never > happen, should it be a skip? Shouldn't we just fail because the test > infrastructure is somehow missing? > Kselftest exit codes are predefined (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/kselftest.h?h=v6.8-rc6#n74), so if we use `set -e` and source a missing file we end up returning "1" as if the test was run and failed. With this check we're sure to return a value that makes sense in the event the helpers file ever gets moved. > Orthogonally, if we want the test, shouldn't this just test the > `source` command directly rather than a proxy (file existing)? > Sure, checking the return value for source also makes sense. Thanks! Best, Laura
diff --git a/MAINTAINERS b/MAINTAINERS index e1475ca38ff2..94e31dac6d2c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -19231,6 +19231,7 @@ F: Documentation/rust/ F: rust/ F: samples/rust/ F: scripts/*rust* +F: tools/testing/selftests/rust/ K: \b(?i:rust)\b RXRPC SOCKETS (AF_RXRPC) diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index f7255969b695..e1504833654d 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -80,6 +80,7 @@ TARGETS += riscv TARGETS += rlimits TARGETS += rseq TARGETS += rtc +TARGETS += rust TARGETS += seccomp TARGETS += sgx TARGETS += sigaltstack diff --git a/tools/testing/selftests/rust/Makefile b/tools/testing/selftests/rust/Makefile new file mode 100644 index 000000000000..fce1584d3bc0 --- /dev/null +++ b/tools/testing/selftests/rust/Makefile @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0 +TEST_PROGS += test_probe_samples.sh + +include ../lib.mk diff --git a/tools/testing/selftests/rust/config b/tools/testing/selftests/rust/config new file mode 100644 index 000000000000..b4002acd40bc --- /dev/null +++ b/tools/testing/selftests/rust/config @@ -0,0 +1,5 @@ +CONFIG_RUST=y +CONFIG_SAMPLES=y +CONFIG_SAMPLES_RUST=y +CONFIG_SAMPLE_RUST_MINIMAL=m +CONFIG_SAMPLE_RUST_PRINT=m \ No newline at end of file diff --git a/tools/testing/selftests/rust/test_probe_samples.sh b/tools/testing/selftests/rust/test_probe_samples.sh new file mode 100755 index 000000000000..ad0397e4986f --- /dev/null +++ b/tools/testing/selftests/rust/test_probe_samples.sh @@ -0,0 +1,41 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# +# Copyright (c) 2023 Collabora Ltd +# +# This script tests whether the rust sample modules can +# be added and removed correctly. +# +DIR="$(dirname "$(readlink -f "$0")")" + +KTAP_HELPERS="${DIR}/../kselftest/ktap_helpers.sh" +if [ -e "$KTAP_HELPERS" ]; then + source "$KTAP_HELPERS" +else + echo "$KTAP_HELPERS file not found [SKIP]" + exit 4 +fi + +rust_sample_modules=("rust_minimal" "rust_print") + +ktap_print_header + +for sample in "${rust_sample_modules[@]}"; do + if ! /sbin/modprobe -n -q "$sample"; then + ktap_skip_all "module $sample is not found in /lib/modules/$(uname -r)" + exit "$KSFT_SKIP" + fi +done + +ktap_set_plan "${#rust_sample_modules[@]}" + +for sample in "${rust_sample_modules[@]}"; do + if /sbin/modprobe -q "$sample"; then + /sbin/modprobe -q -r "$sample" + ktap_test_pass "$sample" + else + ktap_test_fail "$sample" + fi +done + +ktap_finished