diff mbox series

[2/2] testcases/lib: tst_net.sh: Do not use stderr in tst_rhost_run()

Message ID 20250117122455.7989-2-chrubis@suse.cz
State Deferred
Headers show
Series [1/2] testcases/lib: Fix tst_ns_* helpers | expand

Commit Message

Cyril Hrubis Jan. 17, 2025, 12:24 p.m. UTC
This is another place that broke the network test on the unrelated
changes that caused the SAFE_CLONE() to produce TINFO messages. The test
library prints messages into the stdout, because this is something that
is not supposed to be the command output but rather diagnostic messages.
So there was no good reason to include the stderr in the data we got
from the tst_rhost_run().

If this patch does not break anything it should be pushed since this
will fix the tst_ns_* helpers even if the they start print diagnostics
TINFO messages again.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 testcases/lib/tst_net.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Cyril Hrubis Jan. 17, 2025, 12:33 p.m. UTC | #1
Hi!
> This is another place that broke the network test on the unrelated
> changes that caused the SAFE_CLONE() to produce TINFO messages. The test
> library prints messages into the stdout, because this is something that
                                      ^
				      stderr

> is not supposed to be the command output but rather diagnostic messages.
> So there was no good reason to include the stderr in the data we got
> from the tst_rhost_run().
> 
> If this patch does not break anything it should be pushed since this
> will fix the tst_ns_* helpers even if the they start print diagnostics
> TINFO messages again.
> 
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  testcases/lib/tst_net.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh
> index ee0ae1cad..60bc25b79 100644
> --- a/testcases/lib/tst_net.sh
> +++ b/testcases/lib/tst_net.sh
> @@ -257,7 +257,7 @@ tst_rhost_run()
>  		tst_res_ TINFO "$use: $rcmd \"$sh_cmd\" $out 2>&1"
>  	fi
>  
> -	output=$($rcmd "$sh_cmd" $out 2>&1 || echo 'RTERR')
> +	output=$($rcmd "$sh_cmd" $out || echo 'RTERR')
>  
>  	echo "$output" | grep -q 'RTERR$' && ret=1
>  	if [ $ret -eq 1 ]; then
> -- 
> 2.45.2
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp
Li Wang Jan. 17, 2025, 12:40 p.m. UTC | #2
On Fri, Jan 17, 2025 at 8:25 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> This is another place that broke the network test on the unrelated
> changes that caused the SAFE_CLONE() to produce TINFO messages. The test
> library prints messages into the stdout, because this is something that
> is not supposed to be the command output but rather diagnostic messages.
> So there was no good reason to include the stderr in the data we got
> from the tst_rhost_run().
>
> If this patch does not break anything it should be pushed since this
> will fix the tst_ns_* helpers even if the they start print diagnostics
> TINFO messages again.
>
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
>

Reviewed-by: Li Wang <liwang@redhat.com>


---
>  testcases/lib/tst_net.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh
> index ee0ae1cad..60bc25b79 100644
> --- a/testcases/lib/tst_net.sh
> +++ b/testcases/lib/tst_net.sh
> @@ -257,7 +257,7 @@ tst_rhost_run()
>                 tst_res_ TINFO "$use: $rcmd \"$sh_cmd\" $out 2>&1"
>         fi
>
> -       output=$($rcmd "$sh_cmd" $out 2>&1 || echo 'RTERR')
> +       output=$($rcmd "$sh_cmd" $out || echo 'RTERR')
>
>         echo "$output" | grep -q 'RTERR$' && ret=1
>         if [ $ret -eq 1 ]; then
> --
> 2.45.2
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>
Petr Vorel Jan. 17, 2025, 1:27 p.m. UTC | #3
> Hi!
> > This is another place that broke the network test on the unrelated
> > changes that caused the SAFE_CLONE() to produce TINFO messages. The test
> > library prints messages into the stdout, because this is something that
>                                       ^
> 				      stderr

+1.

> > is not supposed to be the command output but rather diagnostic messages.
> > So there was no good reason to include the stderr in the data we got
> > from the tst_rhost_run().

> > If this patch does not break anything it should be pushed since this
> > will fix the tst_ns_* helpers even if the they start print diagnostics
> > TINFO messages again.

> > Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> > ---
> >  testcases/lib/tst_net.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)

> > diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh
> > index ee0ae1cad..60bc25b79 100644
> > --- a/testcases/lib/tst_net.sh
> > +++ b/testcases/lib/tst_net.sh
> > @@ -257,7 +257,7 @@ tst_rhost_run()
> >  		tst_res_ TINFO "$use: $rcmd \"$sh_cmd\" $out 2>&1"
> >  	fi

> > -	output=$($rcmd "$sh_cmd" $out 2>&1 || echo 'RTERR')
> > +	output=$($rcmd "$sh_cmd" $out || echo 'RTERR')

I'm not sure about this. Maybe it's safe, because stderr gets to the output.
But in case some network test need to verify something which is defined on
stderr it will fails (when tst_rhost_run is run with -s).

FYI *without* this patch:

	# route-redirect.sh
	...
	route-redirect 1 TINFO: timeout per run is 0h 5m 0s
	route-redirect 1 TBROK: 'ns-icmp_redirector -I ltp_ns_veth1 -b' failed on '': 'sh: 1: ns-icmp_redirector: not found'
	route-redirect 1 TWARN: 'killall -SIGHUP ns-icmp_redirector' failed on '': 'ns-icmp_redirector: no process found'
	ns-icmp_redirector: no process found

and *with* this patch:

	# route-redirect.sh
	...
	route-redirect 1 TINFO: timeout per run is 0h 5m 0s
	sh: 1: ns-icmp_redirector: not found
	route-redirect 1 TBROK: 'ns-icmp_redirector -I ltp_ns_veth1 -b' failed on '': ''
	ns-icmp_redirector: no process found
	route-redirect 1 TWARN: 'killall -SIGHUP ns-icmp_redirector' failed on '': ''

Info is there, but not on the same line (second '').

Also with the change below it would be more meaningful (fixing first empty ''):

route-redirect 1 TBROK: 'ns-icmp_redirector -I ltp_ns_veth1 -b' failed on NETNS: 'sh: 1: ns-icmp_redirector: not found'
route-redirect 1 TWARN: 'killall -SIGHUP ns-icmp_redirector' failed on NETNS: 'ns-icmp_redirector: no process found'
ns-icmp_redirector: no process found

I also wrote conditional quiet on stderr (not sent to ML), but actually if we
merge this and need sometimes to parse stderr, the opposite would be needed.

Kind regards,
Petr

+++ testcases/lib/tst_net.sh
@@ -263,7 +263,7 @@ tst_rhost_run()
 	if [ $ret -eq 1 ]; then
 		output=$(echo "$output" | sed 's/RTERR//')
 		[ "$safe" ] && \
-			tst_brk_ TBROK "'$cmd' failed on '$RHOST': '$output'"
+			tst_brk_ TBROK "'$cmd' failed on $use: '$output'"
 	fi
 
 	[ -z "$out" -a -n "$output" ] && echo "$output"
Cyril Hrubis Jan. 17, 2025, 1:45 p.m. UTC | #4
Hi!
> I'm not sure about this. Maybe it's safe, because stderr gets to the output.
> But in case some network test need to verify something which is defined on
> stderr it will fails (when tst_rhost_run is run with -s).

That's why I send the first patch that I know is 100% safe.

> FYI *without* this patch:
> 
> 	# route-redirect.sh
> 	...
> 	route-redirect 1 TINFO: timeout per run is 0h 5m 0s
> 	route-redirect 1 TBROK: 'ns-icmp_redirector -I ltp_ns_veth1 -b' failed on '': 'sh: 1: ns-icmp_redirector: not found'
> 	route-redirect 1 TWARN: 'killall -SIGHUP ns-icmp_redirector' failed on '': 'ns-icmp_redirector: no process found'
> 	ns-icmp_redirector: no process found
> 
> and *with* this patch:
> 
> 	# route-redirect.sh
> 	...
> 	route-redirect 1 TINFO: timeout per run is 0h 5m 0s
> 	sh: 1: ns-icmp_redirector: not found
> 	route-redirect 1 TBROK: 'ns-icmp_redirector -I ltp_ns_veth1 -b' failed on '': ''
> 	ns-icmp_redirector: no process found
> 	route-redirect 1 TWARN: 'killall -SIGHUP ns-icmp_redirector' failed on '': ''
> 
> Info is there, but not on the same line (second '').
> 
> Also with the change below it would be more meaningful (fixing first empty ''):
> 
> route-redirect 1 TBROK: 'ns-icmp_redirector -I ltp_ns_veth1 -b' failed on NETNS: 'sh: 1: ns-icmp_redirector: not found'
> route-redirect 1 TWARN: 'killall -SIGHUP ns-icmp_redirector' failed on NETNS: 'ns-icmp_redirector: no process found'
> ns-icmp_redirector: no process found
> 
> I also wrote conditional quiet on stderr (not sent to ML), but actually if we
> merge this and need sometimes to parse stderr, the opposite would be needed.

I supose that we would need to explicitly tell the tst_rhost_run() if we
want stderr included or not. That's probably something to be done after
the release.
Petr Vorel Jan. 17, 2025, 2:16 p.m. UTC | #5
> Hi!
> > I'm not sure about this. Maybe it's safe, because stderr gets to the output.
> > But in case some network test need to verify something which is defined on
> > stderr it will fails (when tst_rhost_run is run with -s).

> That's why I send the first patch that I know is 100% safe.

+1

> > FYI *without* this patch:

> > 	# route-redirect.sh
> > 	...
> > 	route-redirect 1 TINFO: timeout per run is 0h 5m 0s
> > 	route-redirect 1 TBROK: 'ns-icmp_redirector -I ltp_ns_veth1 -b' failed on '': 'sh: 1: ns-icmp_redirector: not found'
> > 	route-redirect 1 TWARN: 'killall -SIGHUP ns-icmp_redirector' failed on '': 'ns-icmp_redirector: no process found'
> > 	ns-icmp_redirector: no process found

> > and *with* this patch:

> > 	# route-redirect.sh
> > 	...
> > 	route-redirect 1 TINFO: timeout per run is 0h 5m 0s
> > 	sh: 1: ns-icmp_redirector: not found
> > 	route-redirect 1 TBROK: 'ns-icmp_redirector -I ltp_ns_veth1 -b' failed on '': ''
> > 	ns-icmp_redirector: no process found
> > 	route-redirect 1 TWARN: 'killall -SIGHUP ns-icmp_redirector' failed on '': ''

> > Info is there, but not on the same line (second '').

> > Also with the change below it would be more meaningful (fixing first empty ''):

> > route-redirect 1 TBROK: 'ns-icmp_redirector -I ltp_ns_veth1 -b' failed on NETNS: 'sh: 1: ns-icmp_redirector: not found'
> > route-redirect 1 TWARN: 'killall -SIGHUP ns-icmp_redirector' failed on NETNS: 'ns-icmp_redirector: no process found'
> > ns-icmp_redirector: no process found

> > I also wrote conditional quiet on stderr (not sent to ML), but actually if we
> > merge this and need sometimes to parse stderr, the opposite would be needed.

> I supose that we would need to explicitly tell the tst_rhost_run() if we
> want stderr included or not. That's probably something to be done after
> the release.

Generally stderr was always needed. It got broken when tst_ns_exec, which runs
rhost commands on network namespaces, e.g. not the command it runs tst_rhost_run
with -c parameter, but how exactly it is executed. ATM I'm aware only about
tst_rhost_run runs in tst_net_setup_network(), which got broken due eval.

I can send a patch, but IMHO we should concentrate (after the release) on
rewriting as much as we can from tst_net.sh to single C binary + help to
simplify the architecture.

Kind regards,
Petr

init_ltp_netspace()
{
	export LTP_NETNS="${LTP_NETNS:-tst_ns_exec $pid net,mnt}"
	...

tst_rhost_run()
{
	...
	while getopts :bc:su: opt; do
		case "$opt" in
		b) [ "${TST_USE_NETNS:-}" ] && pre_cmd= || pre_cmd="nohup"
		   post_cmd=" > /dev/null 2>&1 &"
		   out="1> /dev/null"
		;;
		c) cmd="$OPTARG" ;;
		s) safe=1 ;;
		u) user="$OPTARG" ;;
		*) tst_brk_ TBROK "tst_rhost_run: unknown option: $OPTARG" ;;
		esac
	done
	...
	sh_cmd="$pre_cmd $cmd $post_cmd"

	if [ -n "${TST_USE_NETNS:-}" ]; then
		use="NETNS"
		rcmd="$LTP_NETNS sh -c"
	else
		tst_require_cmds ssh
		use="SSH"
		rcmd="ssh -nq $user@$RHOST"
	fi
	...
	output=$($rcmd "$sh_cmd" $out 2>&1 || echo 'RTERR')

tst_net_setup_network()
{
...
	eval $(tst_rhost_run -c 'tst_net_iface_prefix -r '$IPV4_RHOST \
		|| echo "exit $?")
...
		eval $(tst_net_iface_prefix $IPV6_LHOST || echo "exit $?")
		eval $(tst_rhost_run -c 'tst_net_iface_prefix -r '$IPV6_RHOST \
diff mbox series

Patch

diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh
index ee0ae1cad..60bc25b79 100644
--- a/testcases/lib/tst_net.sh
+++ b/testcases/lib/tst_net.sh
@@ -257,7 +257,7 @@  tst_rhost_run()
 		tst_res_ TINFO "$use: $rcmd \"$sh_cmd\" $out 2>&1"
 	fi
 
-	output=$($rcmd "$sh_cmd" $out 2>&1 || echo 'RTERR')
+	output=$($rcmd "$sh_cmd" $out || echo 'RTERR')
 
 	echo "$output" | grep -q 'RTERR$' && ret=1
 	if [ $ret -eq 1 ]; then