Re: [PATCH v6 42/43] KVM: selftests: Add script to exercise private_mem_conversions_test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Installation Path Resolution Failure in `private_mem_conversions_test.sh`
- [High] Silent failure and brittle logic due to unhandled helper utility exit status
- [Medium] Bash syntax error when checking default hugepage size if hugetlbfs is disabled
- [Medium] Redundant execution of `private_mem_conversions_test` binary
--

commit b88f984e8ecc669593e0deefcd4e6e9441fbfa8c
Author: Ackerley Tng <ackerleytng@google.com>
Subject: KVM: selftests: Add script to exercise private_mem_conversions_test

This commit adds a wrapper script to simplify running the
private_mem_conversions_test with various configurations. It automatically
detects hugepage availability to build a list of source types, running tests
across single and multiple memslots.

> diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
> index 6232881be500a..e5769268936a7 100644
> --- a/tools/testing/selftests/kvm/Makefile.kvm
> +++ b/tools/testing/selftests/kvm/Makefile.kvm
> @@ -54,6 +54,7 @@ LIBKVM_loongarch += lib/loongarch/exception.S
>  
>  # Non-compiled test targets
>  TEST_PROGS_x86 += x86/nx_huge_pages_test.sh
> +TEST_PROGS_x86 += x86/private_mem_conversions_test.sh

[Severity: Medium]
Since the script is added to TEST_PROGS_x86 here, the kselftest harness will
execute it. Will the harness also run the underlying
private_mem_conversions_test binary directly if it remains in
TEST_GEN_PROGS_x86?

Should the C test be moved to TEST_GEN_PROGS_EXTENDED_x86 to prevent it from
being executed twice?

> diff --git a/tools/testing/selftests/kvm/x86/private_mem_conversions_test.sh b/tools/testing/selftests/kvm/x86/private_mem_conversions_test.sh
> new file mode 100755
> index 0000000000000..7179a4fcdd498
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86/private_mem_conversions_test.sh
> @@ -0,0 +1,128 @@
[ ... ]
> +script_dir=$(dirname "$(realpath "$0")")
> +test_executable="${script_dir}/private_mem_conversions_test"
> +kvm_has_gmem_attributes_tool="${script_dir}/../kvm_has_gmem_attributes"

[Severity: High]
Will this path resolution work correctly when tests are executed from an
installed location (e.g., after make install)?

When installed, TEST_PROGS and TEST_GEN_PROGS_EXTENDED typically end up in
the same flat directory. Does looking for the helper tool in script_dir/../
cause the script to fail immediately with a "utility not found" error in
that environment?

[ ... ]
> +kvm_has_gmem_attributes=$("$kvm_has_gmem_attributes_tool" | tail -n1)
> +
> +if [ "$kvm_has_gmem_attributes" -eq 1 ]; then

[Severity: High]
Does this pipeline hide the exit status of the helper tool since set -o
pipefail is not used?

If /dev/kvm is missing, the helper tool might output a SKIP message instead
of a number. Will evaluating the output against 1 trigger a bash syntax
error (integer expression expected), causing the script to incorrectly fall
into the else block?

If it falls into the else block, wouldn't it attempt to run all the
hugepage tests, which will then individually skip and return KSFT_SKIP (4)?

Will the wrapper script misinterpret this return code as a hard failure?

[ ... ]
> +    default_hugepage_size_kb=$(get_default_hugepage_size_in_kb)
> +    hugepage_default_enabled=0
> +    if [ "$default_hugepage_size_kb" -eq 2048 ]; then

[Severity: Medium]
What happens here if CONFIG_HUGETLB_PAGE is disabled in the kernel?

In that case, /proc/meminfo won't contain Hugepagesize:, making the result
from get_default_hugepage_size_in_kb() an empty string. Will evaluating an
empty string against 2048 trigger a bash syntax error and pollute the test
output?

Should the variable be validated as a non-empty integer or defaulted to 0
before the numeric comparison?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260507-gmem-inplace-conversion-v6-0-91ab5a8b19a4@google.com?part=42