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

[Petr Vorel <pvorel@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...

Sure, I'm not against it. We would just simply revert
1878502f63 ("tst_test.sh/tst_brk(): Allow only TBROK and TCONF")

BTW we have a real problem in shell when calling tst_brk in code evaluated in
$(...) with cd on mounted device. I fixed one of these problems:
304d4178a7 ("IMA: Fix exit test on subprocess")
but it would be great to have better solution. This will not be fixed by shell
loader.

Kind regards,
Petr

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