On Thu, 17 Jul 2025 17:19:02 +0800 Enze Li <lie...@kylinos.cn> wrote:
Hi Enze,
Thank you for the patch! I just have a few comments about the patch.
> The current test scripts contain duplicated root permission checks
> in multiple locations. This patch consolidates these checks into
> _common.sh to eliminate code redundancy.
Is there a reason we named the file _common.sh? IIRC there are no other files
that begin with an underscore, so it might be confusing for users. Maybe
remaining it to damon_common.sh might fit better with the convention used
by other selftests.
[...snip...]
> diff --git a/tools/testing/selftests/damon/_common.sh
> b/tools/testing/selftests/damon/_common.sh
> new file mode 100644
> index 000000000000..3920b619c30f
> --- /dev/null
> +++ b/tools/testing/selftests/damon/_common.sh
> @@ -0,0 +1,14 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# Kselftest frmework requirement - SKIP code is 4.
> +ksft_skip=4
> +
> +check_dependencies()
> +{
> + if [ $EUID -ne 0 ]
> + then
> + echo "Run as root"
> + exit $ksft_skip
> + fi
> +}
> diff --git a/tools/testing/selftests/damon/lru_sort.sh
> b/tools/testing/selftests/damon/lru_sort.sh
> index 61b80197c896..0d128d809fd3 100755
> --- a/tools/testing/selftests/damon/lru_sort.sh
> +++ b/tools/testing/selftests/damon/lru_sort.sh
> @@ -1,14 +1,9 @@
> #!/bin/bash
> # SPDX-License-Identifier: GPL-2.0
>
> -# Kselftest framework requirement - SKIP code is 4.
> -ksft_skip=4
Hm, I think factoring out check_dependencies() is a good idea, but maybe we
should keep ksft_skip in here since other checks in the script use the value?
My 2c is that it might make it unnecessarily opaque for others.
Same comment applies for the other files as well.
But I will let SJ comment on this more ;)
Thank you for your patch, I hope you have a great day!
Joshua
Sent using hkml (https://github.com/sjp38/hackermail)
