LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] selftests/kselftest/runner/run_one(): Allow running non-executable files
@ 2021-08-10 14:04 SeongJae Park
  2021-08-10 15:07 ` Greg KH
  0 siblings, 1 reply; 3+ messages in thread
From: SeongJae Park @ 2021-08-10 14:04 UTC (permalink / raw)
  To: shuah; +Cc: linux-kselftest, linux-kernel, akpm, gregkh, 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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] selftests/kselftest/runner/run_one(): Allow running non-executable files
  2021-08-10 14:04 [PATCH] selftests/kselftest/runner/run_one(): Allow running non-executable files SeongJae Park
@ 2021-08-10 15:07 ` Greg KH
  2021-08-10 16:43   ` SeongJae Park
  0 siblings, 1 reply; 3+ messages in thread
From: Greg KH @ 2021-08-10 15:07 UTC (permalink / raw)
  To: SeongJae Park; +Cc: shuah, linux-kselftest, linux-kernel, akpm, SeongJae Park

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] selftests/kselftest/runner/run_one(): Allow running non-executable files
  2021-08-10 15:07 ` Greg KH
@ 2021-08-10 16:43   ` SeongJae Park
  0 siblings, 0 replies; 3+ messages in thread
From: SeongJae Park @ 2021-08-10 16:43 UTC (permalink / raw)
  To: Greg KH
  Cc: SeongJae Park, shuah, linux-kselftest, linux-kernel, akpm, 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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-08-10 16:44 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-10 14:04 [PATCH] selftests/kselftest/runner/run_one(): Allow running non-executable files SeongJae Park
2021-08-10 15:07 ` Greg KH
2021-08-10 16:43   ` SeongJae Park

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).