Message ID | 20250117122455.7989-2-chrubis@suse.cz |
---|---|
State | Deferred |
Headers | show |
Series | [1/2] testcases/lib: Fix tst_ns_* helpers | expand |
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
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 > >
> 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"
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.
> 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 --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
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(-)