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
[...]