[PATCH] selftests/kselftest/runner/run_one(): Allow running non-executable files

[PATCH] selftests/kselftest/runner/run_one(): Allow running non-executable files

[SeongJae Park]

From: SeongJae Park <sjpark@amazon.de>

When running a test program, 'run_one()' checks if the program has the
execution permission and fails if it doesn't.  However, it's easy to
mistakenly missing the permission, as some common tools like 'diff'
don't support the permission change well[1].  Compared to that, making
mistakes in the test program's path would only rare, as those are
explicitly listed in 'TEST_PROGS'.  Therefore, it might make more sense
to resolve the situation on our own and run the program.

For the reason, this commit makes the test program runner function to
still print the warning message but run the program after giving the
execution permission in the case.  To make nothing corrupted, it also
restores the permission after running it.

[1] https://lore.kernel.org/mm-commits/YRJisBs9AunccCD4@kroah.com/

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: SeongJae Park <sjpark@amazon.de>
---
 tools/testing/selftests/kselftest/runner.sh | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
index cc9c846585f0..2eb31e945709 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -65,15 +65,16 @@ run_one()
 
 	TEST_HDR_MSG="selftests: $DIR: $BASENAME_TEST"
 	echo "# $TEST_HDR_MSG"
-	if [ ! -x "$TEST" ]; then
-		echo -n "# Warning: file $TEST is "
-		if [ ! -e "$TEST" ]; then
-			echo "missing!"
-		else
-			echo "not executable, correct this."
-		fi
+	if [ ! -e "$TEST" ]; then
+		echo "# Warning: file $TEST is missing!"
 		echo "not ok $test_num $TEST_HDR_MSG"
 	else
+		permission_added="false"
+		if [ ! -x "$TEST" ]; then
+			echo "# Warning: file $TEST is not executable"
+			chmod u+x "$TEST"
+			permission_added="true"
+		fi
 		cd `dirname $TEST` > /dev/null
 		((((( tap_timeout ./$BASENAME_TEST 2>&1; echo $? >&3) |
 			tap_prefix >&4) 3>&1) |
@@ -88,6 +89,9 @@ run_one()
 		else
 			echo "not ok $test_num $TEST_HDR_MSG # exit=$rc"
 		fi)
+		if [ "$permission_added" = "true" ]; then
+			chmod u-x "$TEST"
+		fi
 		cd - >/dev/null
 	fi
 }
-- 
2.17.1

Re: [PATCH] selftests/kselftest/runner/run_one(): Allow running non-executable files

[Greg KH]

On Tue, Aug 10, 2021 at 02:04:59PM +0000, SeongJae Park wrote:
> From: SeongJae Park <sjpark@amazon.de>
> 
> When running a test program, 'run_one()' checks if the program has the
> execution permission and fails if it doesn't.  However, it's easy to
> mistakenly missing the permission, as some common tools like 'diff'
> don't support the permission change well[1].  Compared to that, making
> mistakes in the test program's path would only rare, as those are
> explicitly listed in 'TEST_PROGS'.  Therefore, it might make more sense
> to resolve the situation on our own and run the program.
> 
> For the reason, this commit makes the test program runner function to
> still print the warning message but run the program after giving the
> execution permission in the case.  To make nothing corrupted, it also
> restores the permission after running it.
> 
> [1] https://lore.kernel.org/mm-commits/YRJisBs9AunccCD4@kroah.com/
> 
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: SeongJae Park <sjpark@amazon.de>
> ---
>  tools/testing/selftests/kselftest/runner.sh | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> index cc9c846585f0..2eb31e945709 100644
> --- a/tools/testing/selftests/kselftest/runner.sh
> +++ b/tools/testing/selftests/kselftest/runner.sh
> @@ -65,15 +65,16 @@ run_one()
>  
>  	TEST_HDR_MSG="selftests: $DIR: $BASENAME_TEST"
>  	echo "# $TEST_HDR_MSG"
> -	if [ ! -x "$TEST" ]; then
> -		echo -n "# Warning: file $TEST is "
> -		if [ ! -e "$TEST" ]; then
> -			echo "missing!"
> -		else
> -			echo "not executable, correct this."
> -		fi
> +	if [ ! -e "$TEST" ]; then
> +		echo "# Warning: file $TEST is missing!"
>  		echo "not ok $test_num $TEST_HDR_MSG"
>  	else
> +		permission_added="false"
> +		if [ ! -x "$TEST" ]; then
> +			echo "# Warning: file $TEST is not executable"
> +			chmod u+x "$TEST"
> +			permission_added="true"

No, why would you change the permission of a test?  What happens if this
is on a read-only filesystem?  You should not be modifying it as it will
end up causing changes when not needed.

thanks,

greg k-h

Re: [PATCH] selftests/kselftest/runner/run_one(): Allow running non-executable files

[SeongJae Park]

From: SeongJae Park <sjpark@amazon.de>

On Tue, 10 Aug 2021 17:07:28 +0200 Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Aug 10, 2021 at 02:04:59PM +0000, SeongJae Park wrote:
> > From: SeongJae Park <sjpark@amazon.de>
> > 
> > When running a test program, 'run_one()' checks if the program has the
> > execution permission and fails if it doesn't.  However, it's easy to
> > mistakenly missing the permission, as some common tools like 'diff'
> > don't support the permission change well[1].  Compared to that, making
> > mistakes in the test program's path would only rare, as those are
> > explicitly listed in 'TEST_PROGS'.  Therefore, it might make more sense
> > to resolve the situation on our own and run the program.
> > 
> > For the reason, this commit makes the test program runner function to
> > still print the warning message but run the program after giving the
> > execution permission in the case.  To make nothing corrupted, it also
> > restores the permission after running it.
> > 
> > [1] https://lore.kernel.org/mm-commits/YRJisBs9AunccCD4@kroah.com/
> > 
> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: SeongJae Park <sjpark@amazon.de>
> > ---
> >  tools/testing/selftests/kselftest/runner.sh | 18 +++++++++++-------
> >  1 file changed, 11 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> > index cc9c846585f0..2eb31e945709 100644
> > --- a/tools/testing/selftests/kselftest/runner.sh
> > +++ b/tools/testing/selftests/kselftest/runner.sh
> > @@ -65,15 +65,16 @@ run_one()
> >  
> >  	TEST_HDR_MSG="selftests: $DIR: $BASENAME_TEST"
> >  	echo "# $TEST_HDR_MSG"
> > -	if [ ! -x "$TEST" ]; then
> > -		echo -n "# Warning: file $TEST is "
> > -		if [ ! -e "$TEST" ]; then
> > -			echo "missing!"
> > -		else
> > -			echo "not executable, correct this."
> > -		fi
> > +	if [ ! -e "$TEST" ]; then
> > +		echo "# Warning: file $TEST is missing!"
> >  		echo "not ok $test_num $TEST_HDR_MSG"
> >  	else
> > +		permission_added="false"
> > +		if [ ! -x "$TEST" ]; then
> > +			echo "# Warning: file $TEST is not executable"
> > +			chmod u+x "$TEST"
> > +			permission_added="true"
> 
> No, why would you change the permission of a test?  What happens if this
> is on a read-only filesystem?  You should not be modifying it as it will
> end up causing changes when not needed.

Agreed.  I will parse the shebang line and use the interpreter explicitly in
the next spin.


Thanks,
SeongJae Park

> 
> thanks,
> 
> greg k-h