Hi Miguel,
On 2/29/24 17:44, Miguel Ojeda wrote:
> On Thu, Feb 29, 2024 at 4:53 PM Laura Nao <laura.nao@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> Reviewed-by: Sergio Gonzalez Collado <sergio.collado@xxxxxxxxx>
>> Reviewed-by: Muhammad Usama Anjum <usama.anjum@xxxxxxxxxxxxx>
>
> 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
