[LTP] [PATCH 1/2] testcases/lib: Fix tst_ns_* helpers

[LTP] [PATCH 1/2] testcases/lib: Fix tst_ns_* helpers

[Cyril Hrubis]

Replaces SAFE_CLONE() with tst_clone() in the tst_ns_* helpers.

The reason for the replacement is that SAFE_CLONE() uses
TST_RETRY_FUNC() which calls tst_multiply_timeout(). The problem with
that is that the tst_multiply_timeout() is a test library function that
started to print TINFO messages recently and that we rely on parsing the
output from the tst_ns_* helpers.

The reason SAFE_CLONE() started to call TST_RETRY_FUNC() is that in the
case that we create new namespaces with the clone call, we may end up
creating them faster than kernel can clean them up which is described in:

commit 7d882081a5613f44a12fc6b1c44267d4df0857a4
Author: Petr Vorel <pvorel@suse.cz>
Date:   Mon Mar 28 22:46:43 2022 +0200

    lib: Retry safe_clone() on ENOSPC

This combined with the newly introduced changes in the test library that
check for kernel debugging options that may need to adjust default
timeouts:

commit 893ca0abe7e82851ff0e5d93c09b1098f2eff121
Author: Li Wang <liwang@redhat.com>
Date:   Sun Dec 22 15:22:49 2024 +0800

    lib: multiply the timeout if detect slow kconfigs

which adds tst_has_slow_kconfig() into the tst_multiply_timeout() causes
the TINFO messages to be printed.

The reason why we can safely replace the SAFE_CLONE() with tst_clone()
here is that we are not creating new namspaces in the tst_ns_* helpers,
but rather than that cloning a new process to be executed inside of the
namespace, hence we do not need to retry on ENOSPC.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 testcases/lib/tst_ns_create.c | 15 ++++++---------
 testcases/lib/tst_ns_exec.c   | 15 ++++++---------
 2 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/testcases/lib/tst_ns_create.c b/testcases/lib/tst_ns_create.c
index ce3707a60..bd7633b0b 100644
--- a/testcases/lib/tst_ns_create.c
+++ b/testcases/lib/tst_ns_create.c
@@ -23,12 +23,6 @@
 #include "tst_test.h"
 #include "tst_ns_common.h"
 
-extern struct tst_test *tst_test;
-
-static struct tst_test test = {
-	.forks_child = 1, /* Needed by SAFE_CLONE */
-};
-
 static void print_help(void)
 {
 	int i;
@@ -66,8 +60,6 @@ int main(int argc, char *argv[])
 		return 1;
 	}
 
-	tst_test = &test;
-
 	while ((token = strsep(&argv[1], ","))) {
 		struct param *p = get_param(token);
 
@@ -80,7 +72,12 @@ int main(int argc, char *argv[])
 		args.flags |= p->flag;
 	}
 
-	pid = SAFE_CLONE(&args);
+	pid = tst_clone(&args);
+	if (pid < 0) {
+		printf("clone() failed");
+		return 1;
+	}
+
 	if (!pid) {
 		child_fn();
 		return 0;
diff --git a/testcases/lib/tst_ns_exec.c b/testcases/lib/tst_ns_exec.c
index 6a8e39339..5d34e9ad5 100644
--- a/testcases/lib/tst_ns_exec.c
+++ b/testcases/lib/tst_ns_exec.c
@@ -20,12 +20,6 @@
 #include "tst_test.h"
 #include "tst_ns_common.h"
 
-extern struct tst_test *tst_test;
-
-static struct tst_test test = {
-	.forks_child = 1, /* Needed by SAFE_CLONE */
-};
-
 static int ns_fd[NS_TOTAL];
 static int ns_fds;
 
@@ -71,8 +65,6 @@ int main(int argc, char *argv[])
 	int i, status, pid;
 	char *token;
 
-	tst_test = &test;
-
 	if (argc < 4) {
 		print_help();
 		return 1;
@@ -100,7 +92,12 @@ int main(int argc, char *argv[])
 	for (i = 0; i < ns_fds; i++)
 		SAFE_SETNS(ns_fd[i], 0);
 
-	pid = SAFE_CLONE(&args);
+	pid = tst_clone(&args);
+	if (pid < 0) {
+		printf("clone() failed");
+		return 1;
+	}
+
 	if (!pid)
 		SAFE_EXECVP(argv[3], argv+3);
 
-- 
2.45.2


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

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

[Cyril Hrubis]

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

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

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

[Cyril Hrubis]

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

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 1/2] testcases/lib: Fix tst_ns_* helpers

[Li Wang]

On Fri, Jan 17, 2025 at 8:25 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Replaces SAFE_CLONE() with tst_clone() in the tst_ns_* helpers.
>
> The reason for the replacement is that SAFE_CLONE() uses
> TST_RETRY_FUNC() which calls tst_multiply_timeout(). The problem with
> that is that the tst_multiply_timeout() is a test library function that
> started to print TINFO messages recently and that we rely on parsing the
> output from the tst_ns_* helpers.
>
> The reason SAFE_CLONE() started to call TST_RETRY_FUNC() is that in the
> case that we create new namespaces with the clone call, we may end up
> creating them faster than kernel can clean them up which is described in:
>
> commit 7d882081a5613f44a12fc6b1c44267d4df0857a4
> Author: Petr Vorel <pvorel@suse.cz>
> Date:   Mon Mar 28 22:46:43 2022 +0200
>
>     lib: Retry safe_clone() on ENOSPC
>
> This combined with the newly introduced changes in the test library that
> check for kernel debugging options that may need to adjust default
> timeouts:
>
> commit 893ca0abe7e82851ff0e5d93c09b1098f2eff121
> Author: Li Wang <liwang@redhat.com>
> Date:   Sun Dec 22 15:22:49 2024 +0800
>
>     lib: multiply the timeout if detect slow kconfigs
>
> which adds tst_has_slow_kconfig() into the tst_multiply_timeout() causes
> the TINFO messages to be printed.
>
> The reason why we can safely replace the SAFE_CLONE() with tst_clone()
> here is that we are not creating new namspaces in the tst_ns_* helpers,
> but rather than that cloning a new process to be executed inside of the
> namespace, hence we do not need to retry on ENOSPC.
>
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
>

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

Nice work!

-- 
Regards,
Li Wang

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

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

[Li Wang]

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

-- 
Regards,
Li Wang

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

Re: [LTP] [PATCH 1/2] testcases/lib: Fix tst_ns_* helpers

[Petr Vorel]

Hi Cyril, Li,

> On Fri, Jan 17, 2025 at 8:25 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> > Replaces SAFE_CLONE() with tst_clone() in the tst_ns_* helpers.

> > The reason for the replacement is that SAFE_CLONE() uses
> > TST_RETRY_FUNC() which calls tst_multiply_timeout(). The problem with
> > that is that the tst_multiply_timeout() is a test library function that
> > started to print TINFO messages recently and that we rely on parsing the
> > output from the tst_ns_* helpers.

> > The reason SAFE_CLONE() started to call TST_RETRY_FUNC() is that in the
> > case that we create new namespaces with the clone call, we may end up
> > creating them faster than kernel can clean them up which is described in:

> > commit 7d882081a5613f44a12fc6b1c44267d4df0857a4
> > Author: Petr Vorel <pvorel@suse.cz>
> > Date:   Mon Mar 28 22:46:43 2022 +0200

> >     lib: Retry safe_clone() on ENOSPC

> > This combined with the newly introduced changes in the test library that
> > check for kernel debugging options that may need to adjust default
> > timeouts:

> > commit 893ca0abe7e82851ff0e5d93c09b1098f2eff121
> > Author: Li Wang <liwang@redhat.com>
> > Date:   Sun Dec 22 15:22:49 2024 +0800

> >     lib: multiply the timeout if detect slow kconfigs

> > which adds tst_has_slow_kconfig() into the tst_multiply_timeout() causes
> > the TINFO messages to be printed.

> > The reason why we can safely replace the SAFE_CLONE() with tst_clone()
> > here is that we are not creating new namspaces in the tst_ns_* helpers,
> > but rather than that cloning a new process to be executed inside of the
> > namespace, hence we do not need to retry on ENOSPC.

> > Signed-off-by: Cyril Hrubis <chrubis@suse.cz>


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

> Nice work!

Thanks for fix and review. This one fixes the problem, thus I merged it.
I'll let you know about the other patch soon (I suspect that it does not catch
other usage, some tools needs to parse stderr ...).

Kind regards,
Petr

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

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

[Petr Vorel]

> 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"

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

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

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

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

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

[Petr Vorel]

> 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 \

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