Re: [LTP] [RFC][PATCH 1/1] tst_net.sh: Fix calling tst_brk with TFAIL

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> And yes, using shell runner to run netstress as a child would probably
> help, but logs would need to be printed immediately to be visible before
> calling tst_brk() from netstress.
> NOTE: Despite macro name TST_BRK_SUPPORTS_ONLY_TCONF_TBROK() the
> original concept in 0738e3753c allowed TFAIL as well.
> 
>     This patch adds simple build-check that allows only
>     TFAIL, TBROK and TCONF as parameter for tst_brk().
> 
>     TFAIL is currently quite commonly used as a shortcut for
>     TFAIL + exit() by many tests. I kept it for now, since
>     it doesn't go against current doc description.
> 
> And indeed C API allows tst_brk(TFAIL). Should we allow this also in
> shell API?

I think that it does make sense to have a reporting function that
reports a result and exits. The type of the result is really orthogonal
to the fact that the function does not return and both tst_brk(TFAIL,
...) and even tst_brk(TPASS, ...) do make sense.

The rules that we enforced on tst_brk() were mainly because of the
limits of the implementation details that should have been fixed in the
library instead.

> Also Cyril suggested for C API different approach:
> 
> https://patchwork.ozlabs.org/project/ltp/patch/20241115164101.17983-1-chrubis@suse.cz/
> 
> Therefore we should probably agree what to do with C API and then unify shell API.

I suppose so.

>  testcases/lib/tst_net.sh | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh
> index ee0ae1cad7..d44115d758 100644
> --- a/testcases/lib/tst_net.sh
> +++ b/testcases/lib/tst_net.sh
> @@ -713,9 +713,17 @@ tst_wait_ipv6_dad()
>  
>  tst_netload_brk()
>  {
> +	local res="$1"
> +	local msg="$2"
> +
>  	tst_rhost_run -c "cat $TST_TMPDIR/netstress.log"
>  	cat tst_netload.log
> -	tst_brk_ $1 $2
> +
> +	if [ "$res" = TFAIL ]; then
> +		tst_res_ "$res" "$msg"
> +	else
> +		tst_brk_ "$res" "$msg"
> +	fi
>  }
>  
>  # Run network load test, see 'netstress -h' for option description
> @@ -825,28 +833,31 @@ tst_netload()
>  		fi
>  
>  		if [ "$ret" -ne 0 ]; then
> -			[ $((ret & 32)) -ne 0 ] && \
> -				tst_netload_brk TCONF "not supported configuration"
> +			[ $((ret & 32)) -ne 0 ] && tst_netload_brk TCONF "not supported configuration"
>  
> -			[ $((ret & 3)) -ne 0 -a $was_failure -gt 0 ] && \
> +			if [ $((ret & 3)) -ne 0 -a $was_failure -gt 0 ]; then
>  				tst_netload_brk TFAIL "expected '$expect_res' but ret: '$ret'"
> +				return
> +			fi
>  
>  			tst_res_ TWARN "netstress failed, ret: $ret"
>  			was_failure=1
>  			continue
>  		fi
>  
> -		[ ! -f $rfile ] && \
> +		if [ ! -f $rfile ]; then
>  			tst_netload_brk TFAIL "can't read $rfile"
> +			return
> +		fi
>  
>  		results="$results $(cat $rfile)"
>  		passed=$((passed + 1))
>  	done
>  
>  	if [ "$ret" -ne 0 ]; then
> -		[ $((ret & 4)) -ne 0 ] && \
> -			tst_res_ TWARN "netstress has warnings"
> +		[ $((ret & 4)) -ne 0 ] && tst_res_ TWARN "netstress has warnings"
>  		tst_netload_brk TFAIL "expected '$expect_res' but ret: '$ret'"
> +		return
>  	fi
>  
>  	local median=$(tst_get_median $results)

As for this patch, wouldn't it make more sense to allow tst_brk_ TFAIL
instead, who knows how many places in shell stil use that...

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp