Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 17 Jun 2022 15:52:52 +0200
From:   Cyril Hrubis <chrubis@suse.cz>
To:     Petr Vorel <pvorel@suse.cz>
Cc:     ltp@lists.linux.it, linux-nfs@vger.kernel.org
Subject: Re: [LTP] [PATCH v2 5/9] tst_test.sh: Add $TST_ALL_FILESYSTEMS
Message-ID: <YqyHNFkJdC0q2Vxs@yuki>
References: <20220609214223.4608-1-pvorel@suse.cz>
 <20220609214223.4608-6-pvorel@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220609214223.4608-6-pvorel@suse.cz>
Precedence: bulk

Hi!
> +_tst_run_iterations()
> +{
> +	local _tst_i=$TST_ITERATIONS
>  
> -	if [ "$TST_MOUNT_DEVICE" = 1 ]; then
> -		tst_mount
> -		TST_MOUNT_FLAG=1
> -	fi
> +	[ "$TST_NEEDS_TMPDIR" = 1 ] && cd "$TST_TMPDIR"
>  
> -	[ -n "$TST_NEEDS_CHECKPOINTS" ] && _tst_init_checkpoints
> +	_tst_setup_timer
>  
>  	if [ -n "$TST_SETUP" ]; then
>  		if command -v $TST_SETUP >/dev/null 2>/dev/null; then
> @@ -724,7 +741,7 @@ tst_run()
>  	fi
>  
>  	#TODO check that test reports some results for each test function call
> -	while [ $TST_ITERATIONS -gt 0 ]; do
> +	while [ $_tst_i -gt 0 ]; do
>  		if [ -n "$TST_TEST_DATA" ]; then
>  			tst_require_cmds cut tr wc
>  			_tst_max=$(( $(echo $TST_TEST_DATA | tr -cd "$TST_TEST_DATA_IFS" | wc -c) +1))
> @@ -735,9 +752,22 @@ tst_run()
>  		else
>  			_tst_run_tests
>  		fi
> -		TST_ITERATIONS=$((TST_ITERATIONS-1))
> +		_tst_i=$((_tst_i-1))
>  	done
> -	_tst_do_exit
> +
> +	if [ -n "$TST_DO_CLEANUP" -a -n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
> +		if command -v $TST_CLEANUP >/dev/null 2>/dev/null; then
> +			$TST_CLEANUP
> +		else
> +			tst_res TWARN "TST_CLEANUP=$TST_CLEANUP declared, but function not defined (or cmd not found)"
> +		fi
> +	fi
> +
> +	if [ "$TST_MOUNT_FLAG" = 1 ]; then
> +		tst_umount
> +	fi
> +
> +	_tst_cleanup_timer

Generally the code looks good, there is still a minor difference between
C API and this changes though. As we do call the _tst_cleanup_timer at
the end of this function the script runs without a timeout for a short
while (which includes tst_mkfs call).

I guess that instead of stopping the timer at the end of the
_tst_run_iterations() we can simply reset it (on the top of this patch):

diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index d96ce3448..f5af4c214 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -50,6 +50,8 @@ _tst_do_exit()
                rm $LTP_IPC_PATH
        fi

+       _tst_cleanup_timer
+
        if [ $TST_FAIL -gt 0 ]; then
                ret=$((ret|1))
        fi
@@ -552,6 +554,12 @@ _tst_setup_timer()
        done
 }

+_tst_reset_timer()
+{
+       _tst_cleanup_timer
+       _tst_setup_timer
+}
+
 tst_require_root()
 {
        if [ "$(id -ru)" != 0 ]; then
@@ -665,6 +673,8 @@ tst_run()
                tst_brk TBROK "Number of iterations (-i) must be >= 0"
        fi

+       _tst_setup_timer
+
        [ "$TST_NEEDS_ROOT" = 1 ] && tst_require_root

        [ "$TST_DISABLE_APPARMOR" = 1 ] && tst_disable_apparmor
@@ -729,8 +739,6 @@ _tst_run_iterations()

        [ "$TST_NEEDS_TMPDIR" = 1 ] && cd "$TST_TMPDIR"

-       _tst_setup_timer
-
        if [ -n "$TST_SETUP" ]; then
                if command -v $TST_SETUP >/dev/null 2>/dev/null; then
                        TST_DO_CLEANUP=1
@@ -767,7 +775,7 @@ _tst_run_iterations()
                tst_umount
        fi

-       _tst_cleanup_timer
+       _tst_reset_timer
 }


-- 
Cyril Hrubis
chrubis@suse.cz