Re: [PATCH] selftests/damon: introduce _common.sh to host shared function

[SeongJae Park <sj@kernel.org>]

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

[...]