[LTP] [PATCH v3] Add containers/share to PATH when call init_ltp_netspace

[LTP] [PATCH v3] Add containers/share to PATH when call init_ltp_netspace

[coolgw]

When run single test case use command such as:
LTP_SHELL_API_TESTS=shell/net/tst_rhost_run.sh make test-shell
Error msg such as "ns_create: command not found" will popup, so
need update PATH before call ns_create etc..

Signed-off-by: WEI GAO <wegao@suse.com>
---
V2 -> V3: move path to test case itself 
V1 -> V2: add tst_require_cmds for init_ltp_netspace()

 lib/newlib_tests/shell/net/tst_rhost_run.sh | 2 +-
 testcases/lib/tst_net.sh                    | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/newlib_tests/shell/net/tst_rhost_run.sh b/lib/newlib_tests/shell/net/tst_rhost_run.sh
index 773b8dd33..70087aa61 100755
--- a/lib/newlib_tests/shell/net/tst_rhost_run.sh
+++ b/lib/newlib_tests/shell/net/tst_rhost_run.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2020 Petr Vorel <pvorel@suse.cz>
 
 TST_TESTFUNC=do_test
-PATH="$(dirname $0)/../../../../testcases/lib/:$PATH"
+PATH="$(dirname $0)/../../../../testcases/lib/:$(dirname $0)/../../../../testcases/kernel/containers/share/:$PATH"
 
 export TST_NET_RHOST_RUN_DEBUG=1
 
diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh
index ceb45c98d..2849f6bc6 100644
--- a/testcases/lib/tst_net.sh
+++ b/testcases/lib/tst_net.sh
@@ -128,10 +128,11 @@ tst_net_require_ipv6()
 
 init_ltp_netspace()
 {
+
 	local pid
 
 	if [ ! -f /var/run/netns/ltp_ns -a -z "$LTP_NETNS" ]; then
-		tst_require_cmds ip
+		tst_require_cmds ip ns_create ns_exec ns_ifmove
 		tst_require_root
 
 		tst_require_drivers veth
-- 
2.35.3


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

Re: [LTP] [PATCH v3] Add containers/share to PATH when call init_ltp_netspace

[Petr Vorel]

Hi Wei,

There are 2 unrelated changes, they should be separated (in 2 commits).
We are sometimes benevolent on this, if the other change is tiny,
but these are 2 functional changes, they should really be separated.

Also, the subject (first line of the patch) is too long and describe only one
change.

> When run single test case use command such as:
> LTP_SHELL_API_TESTS=shell/net/tst_rhost_run.sh make test-shell
Instead of the description how to run tst_rhost_run.sh
(which now with the PATH can be run simply as
./lib/newlib_tests/shell/net/tst_rhost_run.sh)
more important is to say:
1) LTP shell API tests depend on properly compiled LTP.
Therefore this is just a workaround to make visible that some tool is missing.

2) I wonder if there is way to properly fix this dependency in make.
I guess test-shell target should depend on (at least): ns_create ns_exec
ns_ifmove.

> Error msg such as "ns_create: command not found" will popup, so
> need update PATH before call ns_create etc..

> Signed-off-by: WEI GAO <wegao@suse.com>
> ---
> V2 -> V3: move path to test case itself 
> V1 -> V2: add tst_require_cmds for init_ltp_netspace()

>  lib/newlib_tests/shell/net/tst_rhost_run.sh | 2 +-
>  testcases/lib/tst_net.sh                    | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)

> diff --git a/lib/newlib_tests/shell/net/tst_rhost_run.sh b/lib/newlib_tests/shell/net/tst_rhost_run.sh
> index 773b8dd33..70087aa61 100755
> --- a/lib/newlib_tests/shell/net/tst_rhost_run.sh
> +++ b/lib/newlib_tests/shell/net/tst_rhost_run.sh
> @@ -3,7 +3,7 @@
>  # Copyright (c) 2020 Petr Vorel <pvorel@suse.cz>

>  TST_TESTFUNC=do_test
> -PATH="$(dirname $0)/../../../../testcases/lib/:$PATH"
> +PATH="$(dirname $0)/../../../../testcases/lib/:$(dirname $0)/../../../../testcases/kernel/containers/share/:$PATH"
PATH line will get quite long, I'd use variable for path to root.
It will be also more readable (similar to Makefiles, which have top_srcdir
variable).

root="$(dirname $0)/../../../../"
PATH="$root/testcases/lib/:$root/testcases/kernel/containers/share/:$PATH"

>  export TST_NET_RHOST_RUN_DEBUG=1

> diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh
> index ceb45c98d..2849f6bc6 100644
> --- a/testcases/lib/tst_net.sh
> +++ b/testcases/lib/tst_net.sh
> @@ -128,10 +128,11 @@ tst_net_require_ipv6()

>  init_ltp_netspace()
>  {
> +
Please avoid added extra blank lines like this one.
>  	local pid

Kind regards,
Petr

>  	if [ ! -f /var/run/netns/ltp_ns -a -z "$LTP_NETNS" ]; then
> -		tst_require_cmds ip
> +		tst_require_cmds ip ns_create ns_exec ns_ifmove
>  		tst_require_root

>  		tst_require_drivers veth

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

[LTP] [PATCH v4] Add PATH to tst_rhost_run.sh

[coolgw]

When run single test case use command such as:
LTP_SHELL_API_TESTS=shell/net/tst_rhost_run.sh make test-shell
Error msg such as "ns_create: command not found" will popup, so
need update PATH before call ns_create etc..

More important:
1) LTP shell API tests depend on properly compiled LTP.
Therefore this is just a workaround to make visible that some tool is missing.

2) I wonder if there is way to properly fix this dependency in make.
I guess test-shell target should depend on (at least): ns_create ns_exec
ns_ifmove.

Signed-off-by: WEI GAO <wegao@suse.com>
---
v3 -> v4: update format base latest comments, remove unrelated change
V2 -> V3: move path to test case itself 
V1 -> V2: add tst_require_cmds for init_ltp_netspace()

 lib/newlib_tests/shell/net/tst_rhost_run.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/newlib_tests/shell/net/tst_rhost_run.sh b/lib/newlib_tests/shell/net/tst_rhost_run.sh
index 773b8dd33..951551514 100755
--- a/lib/newlib_tests/shell/net/tst_rhost_run.sh
+++ b/lib/newlib_tests/shell/net/tst_rhost_run.sh
@@ -3,7 +3,8 @@
 # Copyright (c) 2020 Petr Vorel <pvorel@suse.cz>
 
 TST_TESTFUNC=do_test
-PATH="$(dirname $0)/../../../../testcases/lib/:$PATH"
+root="$(dirname $0)/../../../../"
+PATH="$root/testcases/lib/:$root/testcases/kernel/containers/share/:$PATH"
 
 export TST_NET_RHOST_RUN_DEBUG=1
 
-- 
2.35.3


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

[LTP] [PATCH v4] Add PATH to tst_rhost_run.sh

[WEI GAO via ltp]

From: coolgw <wegao@suse.com>

When run single test case use command such as:
LTP_SHELL_API_TESTS=shell/net/tst_rhost_run.sh make test-shell
Error msg such as "ns_create: command not found" will popup, so
need update PATH before call ns_create etc..

More important:
1) LTP shell API tests depend on properly compiled LTP.
Therefore this is just a workaround to make visible that some tool is missing.

2) I wonder if there is way to properly fix this dependency in make.
I guess test-shell target should depend on (at least): ns_create ns_exec
ns_ifmove.

Signed-off-by: WEI GAO <wegao@suse.com>
---
v3 -> v4: update base Vorel's latest comments, remove unrelated change
V2 -> V3: move path to test case itself 
V1 -> V2: add tst_require_cmds for init_ltp_netspace()

 lib/newlib_tests/shell/net/tst_rhost_run.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/newlib_tests/shell/net/tst_rhost_run.sh b/lib/newlib_tests/shell/net/tst_rhost_run.sh
index 773b8dd33..951551514 100755
--- a/lib/newlib_tests/shell/net/tst_rhost_run.sh
+++ b/lib/newlib_tests/shell/net/tst_rhost_run.sh
@@ -3,7 +3,8 @@
 # Copyright (c) 2020 Petr Vorel <pvorel@suse.cz>
 
 TST_TESTFUNC=do_test
-PATH="$(dirname $0)/../../../../testcases/lib/:$PATH"
+root="$(dirname $0)/../../../../"
+PATH="$root/testcases/lib/:$root/testcases/kernel/containers/share/:$PATH"
 
 export TST_NET_RHOST_RUN_DEBUG=1
 
-- 
2.35.3


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

[LTP] [PATCH v4] Add PATH to tst_rhost_run.sh

[WEI GAO via ltp]

From: coolgw <wegao@suse.com>

When run single test case use command such as:
LTP_SHELL_API_TESTS=shell/net/tst_rhost_run.sh make test-shell
Error msg such as "ns_create: command not found" will popup, so
need update PATH before call ns_create etc..

More important:
1) LTP shell API tests depend on properly compiled LTP.
Therefore this is just a workaround to make visible that some tool is missing.

2) I wonder if there is way to properly fix this dependency in make.
I guess test-shell target should depend on (at least): ns_create ns_exec
ns_ifmove.

Signed-off-by: WEI GAO <wegao@suse.com>
---
v3 -> v4: update base Vorel's latest comments, remove unrelated change
V2 -> V3: move path to test case itself 
V1 -> V2: add tst_require_cmds for init_ltp_netspace()

 lib/newlib_tests/shell/net/tst_rhost_run.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/newlib_tests/shell/net/tst_rhost_run.sh b/lib/newlib_tests/shell/net/tst_rhost_run.sh
index 773b8dd33..951551514 100755
--- a/lib/newlib_tests/shell/net/tst_rhost_run.sh
+++ b/lib/newlib_tests/shell/net/tst_rhost_run.sh
@@ -3,7 +3,8 @@
 # Copyright (c) 2020 Petr Vorel <pvorel@suse.cz>
 
 TST_TESTFUNC=do_test
-PATH="$(dirname $0)/../../../../testcases/lib/:$PATH"
+root="$(dirname $0)/../../../../"
+PATH="$root/testcases/lib/:$root/testcases/kernel/containers/share/:$PATH"
 
 export TST_NET_RHOST_RUN_DEBUG=1
 
-- 
2.35.3


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

[LTP] [PATCH v4] Add PATH to tst_rhost_run.sh

[WEI GAO via ltp]

From: coolgw <wegao@suse.com>

When run single test case use command such as:
LTP_SHELL_API_TESTS=shell/net/tst_rhost_run.sh make test-shell
Error msg such as "ns_create: command not found" will popup, so
need update PATH before call ns_create etc..

More important:
1) LTP shell API tests depend on properly compiled LTP.
Therefore this is just a workaround to make visible that some tool is missing.

2) I wonder if there is way to properly fix this dependency in make.
I guess test-shell target should depend on (at least): ns_create ns_exec
ns_ifmove.

Signed-off-by: WEI GAO <wegao@suse.com>
---
v3 -> v4: update base Vorel's latest comments, remove unrelated change
V2 -> V3: move path to test case itself 
V1 -> V2: add tst_require_cmds for init_ltp_netspace()

 lib/newlib_tests/shell/net/tst_rhost_run.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/newlib_tests/shell/net/tst_rhost_run.sh b/lib/newlib_tests/shell/net/tst_rhost_run.sh
index 773b8dd33..951551514 100755
--- a/lib/newlib_tests/shell/net/tst_rhost_run.sh
+++ b/lib/newlib_tests/shell/net/tst_rhost_run.sh
@@ -3,7 +3,8 @@
 # Copyright (c) 2020 Petr Vorel <pvorel@suse.cz>
 
 TST_TESTFUNC=do_test
-PATH="$(dirname $0)/../../../../testcases/lib/:$PATH"
+root="$(dirname $0)/../../../../"
+PATH="$root/testcases/lib/:$root/testcases/kernel/containers/share/:$PATH"
 
 export TST_NET_RHOST_RUN_DEBUG=1
 
-- 
2.35.3


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

Re: [LTP] [PATCH v4] Add PATH to tst_rhost_run.sh

[Petr Vorel]

Hi Wei, all,

> From: coolgw <wegao@suse.com>
NOTE: could you run this command to fix your setup:
git config --global user.name "Wei Gao"

> When run single test case use command such as:
> LTP_SHELL_API_TESTS=shell/net/tst_rhost_run.sh make test-shell
> Error msg such as "ns_create: command not found" will popup, so
> need update PATH before call ns_create etc..

> More important:
> 1) LTP shell API tests depend on properly compiled LTP.
> Therefore this is just a workaround to make visible that some tool is missing.

> 2) I wonder if there is way to properly fix this dependency in make.
> I guess test-shell target should depend on (at least): ns_create ns_exec
> ns_ifmove.

@Li, @Richie, @Xu, @Cyril: anything against moving
testcases/kernel/containers/share/ content to testcases/lib? They are used
mainly for container tests, but since tst_net.sh depends on it, IMHO they should
be in library. This helps to fix make dependency for tst_rhost_run.sh.

Current LTP build system does not allow to add ns_exec as make dependency for
test-shell. Tests depends on lib-all, which does not include testcases/lib,
thus make dependency for tst_rhost_run.sh will not be solved, but at least it
will be on a common place. I wonder if testcases/lib/ could be somehow added as
a dependency to test make target.

---

@Wei I was going to improve the commit message and merged. But there is actually
much better solution for this problem (unless anybody from LTP maintainers is
against it).

Because these helpers are needed also for all LTP shell network tests
(everything which uses tst_net.sh), they should be moved to testcases/lib
directory (whole content of testcases/kernel/containers/share/), where are all
shell helpers.  Files there require to have tst_ prefix (e.g. ns_exec.c =>
tst_ns_exec.c, ...).

This will include to add these names without '.c' suffix to MAKE_TARGETS in
testcases/lib/Makefile and update paths everywhere (e.g runtest/containers,
netns_sysfs.sh, ...). This needs to be in a single commit.

Kind regards,
Petr

> Signed-off-by: WEI GAO <wegao@suse.com>
> ---
> v3 -> v4: update base Vorel's latest comments, remove unrelated change
> V2 -> V3: move path to test case itself 
> V1 -> V2: add tst_require_cmds for init_ltp_netspace()

>  lib/newlib_tests/shell/net/tst_rhost_run.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

> diff --git a/lib/newlib_tests/shell/net/tst_rhost_run.sh b/lib/newlib_tests/shell/net/tst_rhost_run.sh
> index 773b8dd33..951551514 100755
> --- a/lib/newlib_tests/shell/net/tst_rhost_run.sh
> +++ b/lib/newlib_tests/shell/net/tst_rhost_run.sh
> @@ -3,7 +3,8 @@
>  # Copyright (c) 2020 Petr Vorel <pvorel@suse.cz>

>  TST_TESTFUNC=do_test
> -PATH="$(dirname $0)/../../../../testcases/lib/:$PATH"
> +root="$(dirname $0)/../../../../"
> +PATH="$root/testcases/lib/:$root/testcases/kernel/containers/share/:$PATH"

>  export TST_NET_RHOST_RUN_DEBUG=1

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

Re: [LTP] [PATCH v4] Add PATH to tst_rhost_run.sh

[Cyril Hrubis]

Hi!
> @Li, @Richie, @Xu, @Cyril: anything against moving
> testcases/kernel/containers/share/ content to testcases/lib? They are used
> mainly for container tests, but since tst_net.sh depends on it, IMHO they should
> be in library. This helps to fix make dependency for tst_rhost_run.sh.

Not at all, these tools looks quite generic. Also prefixing them with
tst_ sounds good to me as well.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v4] Add PATH to tst_rhost_run.sh

[Petr Vorel]

Hi Wei, all,

I at least once reproduced the problem:
# ./tst_rhost_run.sh
RTNETLINK answers: File exists
tst_rhost_run 1 TBROK: ip li add name ltp_ns_veth1 type veth peer name ltp_ns_veth2 failed

I can't reproduce it now, let's skip it.

Unfortunately there is another problem, which can't be solved with adjusting
PATH for the test, because also remote end would need to have PATH adjusted
With this patch and with your another patch [1]:

# ./tst_rhost_run.sh
...
tst_rhost_run 1 TINFO: tst_rhost_run: cmd: tst_net_iface_prefix -r 10.0.0.1
tst_rhost_run 1 TINFO: NETNS: ns_exec 17258 net,mnt sh -c " tst_net_iface_prefix -r 10.0.0.1 || echo RTERR" 2>&1
./../../../..//testcases/lib/tst_net.sh: line 1027: sh:: command not found
tst_rhost_run 1 TINFO: tst_rhost_run: cmd: tst_net_iface_prefix -r fd00:1:1:1::1
tst_rhost_run 1 TINFO: NETNS: ns_exec 17258 net,mnt sh -c " tst_net_iface_prefix -r fd00:1:1:1::1 || echo RTERR" 2>&1
./../../../..//testcases/lib/tst_net.sh: line 1032: sh:: command not found
...
tst_rhost_run 1 TPASS: tst_rhost_run is working

=> test claims TPASS, but it actually does not work properly (false negative).

Therefore instead of adjusting PATH I want to fix it properly, i.e. moving
to testcases/kernel/containers/share/ (described previously).

Kind regards,
Petr

[1] https://patchwork.ozlabs.org/project/ltp/patch/20230117040132.5245-1-wegao@suse.com/

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

Re: [LTP] [PATCH v4] Add PATH to tst_rhost_run.sh

[Wei Gao via ltp]

On Tue, Jan 17, 2023 at 09:26:13AM +0100, Petr Vorel wrote:
> Hi Wei, all,
> 
> I at least once reproduced the problem:
> # ./tst_rhost_run.sh
> RTNETLINK answers: File exists
> tst_rhost_run 1 TBROK: ip li add name ltp_ns_veth1 type veth peer name ltp_ns_veth2 failed
> 
> I can't reproduce it now, let's skip it.
> 
> Unfortunately there is another problem, which can't be solved with adjusting
> PATH for the test, because also remote end would need to have PATH adjusted
> With this patch and with your another patch [1]:
> 
> # ./tst_rhost_run.sh
> ...
> tst_rhost_run 1 TINFO: tst_rhost_run: cmd: tst_net_iface_prefix -r 10.0.0.1
> tst_rhost_run 1 TINFO: NETNS: ns_exec 17258 net,mnt sh -c " tst_net_iface_prefix -r 10.0.0.1 || echo RTERR" 2>&1
> ./../../../..//testcases/lib/tst_net.sh: line 1027: sh:: command not found
> tst_rhost_run 1 TINFO: tst_rhost_run: cmd: tst_net_iface_prefix -r fd00:1:1:1::1
> tst_rhost_run 1 TINFO: NETNS: ns_exec 17258 net,mnt sh -c " tst_net_iface_prefix -r fd00:1:1:1::1 || echo RTERR" 2>&1
> ./../../../..//testcases/lib/tst_net.sh: line 1032: sh:: command not found
> ...
> tst_rhost_run 1 TPASS: tst_rhost_run is working
> 
> => test claims TPASS, but it actually does not work properly (false negative).
> 
> Therefore instead of adjusting PATH I want to fix it properly, i.e. moving
> to testcases/kernel/containers/share/ (described previously).
> 
> Kind regards,
> Petr
> 
> [1] https://patchwork.ozlabs.org/project/ltp/patch/20230117040132.5245-1-wegao@suse.com/

Your failed case already not related my patch already since failed show "sh:: command not found", could you help check env such as
"which sh" ,  "sh" normally should work in default system PATH, no need add specific PATH.

But yes the test case claim PASS is wrong so we can further make improvement on this kind of error, such as add further check 
for "sh" command then we will exit case and make result failed. I can add more check for "sh" command.

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

Re: [LTP] [PATCH v4] Add PATH to tst_rhost_run.sh

[Petr Vorel]

> On Tue, Jan 17, 2023 at 09:26:13AM +0100, Petr Vorel wrote:
> > Hi Wei, all,

> > I at least once reproduced the problem:
> > # ./tst_rhost_run.sh
> > RTNETLINK answers: File exists
> > tst_rhost_run 1 TBROK: ip li add name ltp_ns_veth1 type veth peer name ltp_ns_veth2 failed

> > I can't reproduce it now, let's skip it.

> > Unfortunately there is another problem, which can't be solved with adjusting
> > PATH for the test, because also remote end would need to have PATH adjusted
> > With this patch and with your another patch [1]:

> > # ./tst_rhost_run.sh
> > ...
> > tst_rhost_run 1 TINFO: tst_rhost_run: cmd: tst_net_iface_prefix -r 10.0.0.1
> > tst_rhost_run 1 TINFO: NETNS: ns_exec 17258 net,mnt sh -c " tst_net_iface_prefix -r 10.0.0.1 || echo RTERR" 2>&1
> > ./../../../..//testcases/lib/tst_net.sh: line 1027: sh:: command not found
> > tst_rhost_run 1 TINFO: tst_rhost_run: cmd: tst_net_iface_prefix -r fd00:1:1:1::1
> > tst_rhost_run 1 TINFO: NETNS: ns_exec 17258 net,mnt sh -c " tst_net_iface_prefix -r fd00:1:1:1::1 || echo RTERR" 2>&1
> > ./../../../..//testcases/lib/tst_net.sh: line 1032: sh:: command not found
> > ...
> > tst_rhost_run 1 TPASS: tst_rhost_run is working

> > => test claims TPASS, but it actually does not work properly (false negative).

> > Therefore instead of adjusting PATH I want to fix it properly, i.e. moving
> > to testcases/kernel/containers/share/ (described previously).

> > Kind regards,
> > Petr

> > [1] https://patchwork.ozlabs.org/project/ltp/patch/20230117040132.5245-1-wegao@suse.com/

> Your failed case already not related my patch already since failed show "sh:: command not found", could you help check env such as
> "which sh" ,  "sh" normally should work in default system PATH, no need add specific PATH.

> But yes the test case claim PASS is wrong so we can further make improvement on this kind of error, such as add further check 
> for "sh" command then we will exit case and make result failed. I can add more check for "sh" command.

I thought "sh:: command not found" is for command missing (tst_net_iface_prefix
is actually missing), but probably not. But I also think that it's not related
to sh is missing, because the message has 'sh::' (double colon).

Anyway, I really think to concentrate on this broken test should come after
tst_net.sh cleanup is merged and netns helpers are moved to testcases/lib/.
1) These things are needed not just for shake of test 2) That will allow not to
add code which will be removed after cleanup.

Kind regards,
Petr


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