The current test scripts contain duplicated root permission checks in multiple locations. This patch consolidates these checks into _common.sh to eliminate code redundancy.
Signed-off-by: Enze Li lienze@kylinos.cn --- tools/testing/selftests/damon/_common.sh | 14 ++++++++++++++ tools/testing/selftests/damon/lru_sort.sh | 9 ++------- tools/testing/selftests/damon/reclaim.sh | 9 ++------- tools/testing/selftests/damon/sysfs.sh | 12 +----------- .../damon/sysfs_update_removed_scheme_dir.sh | 9 ++------- 5 files changed, 21 insertions(+), 32 deletions(-) create mode 100644 tools/testing/selftests/damon/_common.sh
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 +source _common.sh
-if [ $EUID -ne 0 ] -then - echo "Run as root" - exit $ksft_skip -fi +check_dependencies
damon_lru_sort_enabled="/sys/module/damon_lru_sort/parameters/enabled" if [ ! -f "$damon_lru_sort_enabled" ] diff --git a/tools/testing/selftests/damon/reclaim.sh b/tools/testing/selftests/damon/reclaim.sh index 78dbc2334cbe..41e450a696ae 100755 --- a/tools/testing/selftests/damon/reclaim.sh +++ b/tools/testing/selftests/damon/reclaim.sh @@ -1,14 +1,9 @@ #!/bin/bash # SPDX-License-Identifier: GPL-2.0
-# Kselftest framework requirement - SKIP code is 4. -ksft_skip=4 +source _common.sh
-if [ $EUID -ne 0 ] -then - echo "Run as root" - exit $ksft_skip -fi +check_dependencies
damon_reclaim_enabled="/sys/module/damon_reclaim/parameters/enabled" if [ ! -f "$damon_reclaim_enabled" ] diff --git a/tools/testing/selftests/damon/sysfs.sh b/tools/testing/selftests/damon/sysfs.sh index e9a976d296e2..0326b9ad55ca 100755 --- a/tools/testing/selftests/damon/sysfs.sh +++ b/tools/testing/selftests/damon/sysfs.sh @@ -1,8 +1,7 @@ #!/bin/bash # SPDX-License-Identifier: GPL-2.0
-# Kselftest frmework requirement - SKIP code is 4. -ksft_skip=4 +source _common.sh
ensure_write_succ() { @@ -364,14 +363,5 @@ test_damon_sysfs() test_kdamonds "$damon_sysfs/kdamonds" }
-check_dependencies() -{ - if [ $EUID -ne 0 ] - then - echo "Run as root" - exit $ksft_skip - fi -} - check_dependencies test_damon_sysfs "/sys/kernel/mm/damon/admin" diff --git a/tools/testing/selftests/damon/sysfs_update_removed_scheme_dir.sh b/tools/testing/selftests/damon/sysfs_update_removed_scheme_dir.sh index ade35576e748..730165bd7f03 100755 --- a/tools/testing/selftests/damon/sysfs_update_removed_scheme_dir.sh +++ b/tools/testing/selftests/damon/sysfs_update_removed_scheme_dir.sh @@ -1,14 +1,9 @@ #!/bin/bash # SPDX-License-Identifier: GPL-2.0
-# Kselftest framework requirement - SKIP code is 4. -ksft_skip=4 +source _common.sh
-if [ $EUID -ne 0 ] -then - echo "Run as root" - exit $ksft_skip -fi +check_dependencies
damon_sysfs="/sys/kernel/mm/damon/admin" if [ ! -d "$damon_sysfs" ]
base-commit: e2291551827fe5d2d3758c435c191d32b6d1350e
On Thu, 17 Jul 2025 17:19:02 +0800 Enze Li lienze@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)
Hi Joshua,
On Thu, 17 Jul 2025 06:54:32 -0700 Joshua Hahn joshua.hahnjy@gmail.com wrote:
On Thu, 17 Jul 2025 17:19:02 +0800 Enze Li lienze@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.
This is my personal pattern that I sometimes use, to distinguish files that aimed to be only indirectly be used. We already have a file of the pattern, namely _damon_sysfs.py.
I don't think this pattern is particularly good, but not making something worse, so I'm ok with current file name.
[...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 ;)
I agree Joshua's point. I'd prefer keeping ksft_skip definition here.
Thank you for your patch, I hope you have a great day!
Thank you for your valuable comments, Joshua :)
Thanks, SJ
[...]
Hi SJ, Joshua,
On Thu, Jul 17 2025 at 09:14:54 AM -0700, SeongJae Park wrote:
Hi Joshua,
On Thu, 17 Jul 2025 06:54:32 -0700 Joshua Hahn joshua.hahnjy@gmail.com wrote:
On Thu, 17 Jul 2025 17:19:02 +0800 Enze Li lienze@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.
This is my personal pattern that I sometimes use, to distinguish files that aimed to be only indirectly be used. We already have a file of the pattern, namely _damon_sysfs.py.
I don't think this pattern is particularly good, but not making something worse, so I'm ok with current file name.
Yes, I've noted the naming convention from _damon_sysfs.py and have maintained consistency with the existing pattern in this patch. :)
[...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 ;)
I agree Joshua's point. I'd prefer keeping ksft_skip definition here.
Thank you for the review. I'll send v2 addressing your comments to the list soon.
BR, Enze
Thank you for your patch, I hope you have a great day!
Thank you for your valuable comments, Joshua :)
Thanks, SJ
[...]
Hi Enze,
On Thu, 17 Jul 2025 17:19:02 +0800 Enze Li lienze@kylinos.cn wrote:
The current test scripts contain duplicated root permission checks in multiple locations. This patch consolidates these checks into _common.sh to eliminate code redundancy.
Thank you for this patch!
Signed-off-by: Enze Li lienze@kylinos.cn
tools/testing/selftests/damon/_common.sh | 14 ++++++++++++++ tools/testing/selftests/damon/lru_sort.sh | 9 ++------- tools/testing/selftests/damon/reclaim.sh | 9 ++------- tools/testing/selftests/damon/sysfs.sh | 12 +----------- .../damon/sysfs_update_removed_scheme_dir.sh | 9 ++------- 5 files changed, 21 insertions(+), 32 deletions(-) create mode 100644 tools/testing/selftests/damon/_common.sh
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
As Joshua also pointed out, let's keep ksft_skip definition in each file, unless there is no more use of the variable.
+source _common.sh -if [ $EUID -ne 0 ] -then
- echo "Run as root"
- exit $ksft_skip
-fi +check_dependencies damon_lru_sort_enabled="/sys/module/damon_lru_sort/parameters/enabled" if [ ! -f "$damon_lru_sort_enabled" ]
[...]
Other than ksft_skip, looks good to me. Looking forward to your next version :)
Thanks, SJ
linux-kselftest-mirror@lists.linaro.org
-
Enze Li -
Joshua Hahn -
SeongJae Park