Date:   Thu, 2 Dec 2021 13:18:35 -0800
From:   Kees Cook <keescook@chromium.org>
To:     Cristian Marussi <cristian.marussi@arm.com>
Cc:     linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
        sjpark@amazon.de, shuah@kernel.org
Subject: Re: [PATCH] selftests/kselftest/runner.sh: Add optional command
 parameters in settings
Message-ID: <202112021310.E1B3208@keescook>
References: <20211202142056.17386-1-cristian.marussi@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211202142056.17386-1-cristian.marussi@arm.com>
Precedence: bulk

On Thu, Dec 02, 2021 at 02:20:56PM +0000, Cristian Marussi wrote:
> Some testcases allow for optional commandline parameters but as of now
> there is now way to provide such arguments to the runner script.
> 
> Add support to the per-test-directory "settings" file to provide such
> optional arguments; two new optional fields can now be defined in
> "settings":
> 
>  - args="<options>": general arguments common to all testcase commands in
>    the test directory
> 
>  - <BASENAME_TEST>_args="<options>": custom arguments specific to only one
>    specific testcase command
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> Used to configure the use of a specific rtc device on CI systems with:
>  tools/testing/selftests/rtc/settings:
>    timeout=90
>    rtctest_args="/dev/rtc1"

I like this idea generally, but I have some concern that this is
muddling the test's settings ("do not expect me to finish before
timeout=90") vs the local system's settings ("here is where to find the
rtc to test"). I can't, however, think of a better way to handle this
currently. :P

Is this case common enough that a given test shouldn't, instead, just
take config from environment variables set by the CI?

(Also, will we need to worry in the future about running the same test
multiple times with different system settings? ("try each of these /dev
nodes...")

Is there a patch for the changes to the RTC test?

> ---
>  tools/testing/selftests/kselftest/runner.sh | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> index a9ba782d8ca0..f877a8571927 100644
> --- a/tools/testing/selftests/kselftest/runner.sh
> +++ b/tools/testing/selftests/kselftest/runner.sh
> @@ -49,6 +49,15 @@ run_one()
>  
>  	# Reset any "settings"-file variables.
>  	export kselftest_timeout="$kselftest_default_timeout"
> +
> +	# Optional arguments for any command, possibly defined in settings
> +	# as args="<options>"
> +	kselftest_args=""
> +
> +	# Optional arguments for this command, possibly defined in settings
> +	# as <$BASENAME_TEST>_args="<options>"
> +	kselftest_cmd_args_ref="kselftest_${BASENAME_TEST}_args"
> +
>  	# Load per-test-directory kselftest "settings" file.
>  	settings="$BASE_DIR/$DIR/settings"
>  	if [ -r "$settings" ] ; then
> @@ -69,7 +78,8 @@ run_one()
>  		echo "# Warning: file $TEST is missing!"
>  		echo "not ok $test_num $TEST_HDR_MSG"
>  	else
> -		cmd="./$BASENAME_TEST"
> +		eval kselftest_cmd_args="\$$kselftest_cmd_args_ref"

nitpit: Just to avoid tripping any future work to gracefully handle
unset variables, maybe this could specify an empty-string default:

		eval kselftest_cmd_args="\${$kselftest_cmd_args_ref:-}"

> +		cmd="./$BASENAME_TEST $kselftest_cmd_args $kselftest_args"
>  		if [ ! -x "$TEST" ]; then
>  			echo "# Warning: file $TEST is not executable"
>  
> -- 
> 2.17.1
> 

-- 
Kees Cook