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

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

[Petr Vorel]

1878502f63 forbid using tst_brk with TFAIL but usage was not converted
to TFAIL. Use tst_res and add necessary return.

Fixes: 1878502f63 ("tst_test.sh/tst_brk(): Allow only TBROK and TCONF")
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
This fixes problem on failing netstress.c:

sctp_ipsec 1 TINFO: Running: sctp_ipsec.sh -6 -A rfc4106_192 -p esp_aead -m transport -s 100:1000:65535:R65535
sctp_ipsec 1 TINFO: Tested kernel: Linux susetest 6.4.0-150700.252.g83759b3-default #1 SMP PREEMPT_DYNAMIC Fri Dec 13 18:41:41 UTC 2024 (83759b3) aarch64 aarch64 aarch64 GNU/Linux
sctp_ipsec 1 TINFO: initialize 'lhost' 'ltp_ns_veth2' interface
sctp_ipsec 1 TINFO: add local addr 10.0.0.2/24
sctp_ipsec 1 TINFO: add local addr fd00:1:1:1::2/64
sctp_ipsec 1 TINFO: initialize 'rhost' 'ltp_ns_veth1' interface
sctp_ipsec 1 TINFO: add remote addr 10.0.0.1/24
sctp_ipsec 1 TINFO: add remote addr fd00:1:1:1::1/64
sctp_ipsec 1 TINFO: Network config (local -- remote):
sctp_ipsec 1 TINFO: ltp_ns_veth2 -- ltp_ns_veth1
sctp_ipsec 1 TINFO: 10.0.0.2/24 -- 10.0.0.1/24
sctp_ipsec 1 TINFO: fd00:1:1:1::2/64 -- fd00:1:1:1::1/64
sctp_ipsec 1 TINFO: Using /tmp/LTP_sctp_ipsec.3UdSsZtTZM as tmpdir (btrfs filesystem)
sctp_ipsec 1 TINFO: timeout per run is 0h 15m 0s
sctp_ipsec 1 TINFO: IPsec[esp_aead/transport]
sctp_ipsec 1 TINFO: run server 'netstress -T sctp -S fd00:1:1:1::1 -d ltp_ns_veth1 -R 500000 -B /tmp/LTP_sctp_ipsec.3UdSsZtTZM'
sctp_ipsec 1 TINFO: run client 'netstress -l -T sctp -H fd00:1:1:1::1 -n 100 -N 100 -S fd00:1:1:1::2 -d ltp_ns_veth2 -a 2 -r 100 -c /tmp/LTP_sctp_ipsec.3UdSsZtTZM/tst_netload.res' 5 times
sctp_ipsec 1 TPASS: netstress passed, median time 4 ms, data: 4 4 4 4 4
sctp_ipsec 2 TINFO: run server 'netstress -T sctp -S fd00:1:1:1::1 -d ltp_ns_veth1 -R 500000 -B /tmp/LTP_sctp_ipsec.3UdSsZtTZM'
sctp_ipsec 2 TINFO: run client 'netstress -l -T sctp -H fd00:1:1:1::1 -n 1000 -N 1000 -S fd00:1:1:1::2 -d ltp_ns_veth2 -a 2 -r 100 -c /tmp/LTP_sctp_ipsec.3UdSsZtTZM/tst_netload.res' 5 times
sctp_ipsec 2 TWARN: netstress failed, ret: 6
sctp_ipsec 2 TWARN: netstress has warnings
netstress.c:577: TFAIL: recv failed, sock '6'
netstress.c:644: TBROK: Server closed
tst_tmpdir.c:317: TINFO: Using /tmp/LTP_netf5tGoS as tmpdir (btrfs filesystem)
tst_test.c:1890: TINFO: LTP version: 20240930
tst_test.c:1727: TINFO: Timeout per run is 0h 06m 30s
netstress.c:892: TINFO: IP_BIND_ADDRESS_NO_PORT is used
netstress.c:895: TINFO: connection: addr 'fd00:1:1:1::1', port '44481'
netstress.c:896: TINFO: client max req: 100
netstress.c:897: TINFO: clients num: 2
netstress.c:902: TINFO: client msg size: 1000
netstress.c:903: TINFO: server msg size: 1000
netstress.c:975: TINFO: SCTP client
netstress.c:478: TINFO: Running the test over IPv6
netstress.c:437: TWARN: client exit on '65' request
netstress.c:497: TBROK: client[0] failed: Cannot send after transport endpoint shutdown

Summary:
passed   0
failed   0
broken   1
skipped  0
warnings 1
sctp_ipsec 2 TBROK: tst_brk can be called only with TBROK or TCONF (TFAIL)

Summary:
passed   1
failed   0
broken   1
skipped  0
warnings 2
### TEST sctp6_ipsec10 COMPLETE >>> 6.

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?

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.

Kind regards,
Petr

 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)
-- 
2.45.2


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

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

[Cyril Hrubis]

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

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

[Petr Vorel]

> 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