| Message ID | 20230111195231.23596-1-wegao@suse.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v4] Add PATH to tst_rhost_run.sh | expand |
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
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.
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/
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.
> 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
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