| Message ID | 20230117040132.5245-1-wegao@suse.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1] tst_net.sh: Add more tst_require_cmds check | expand |
Hi Wei, > More strict check for ns_xxx etc will help avoid following issue: > https://github.com/linux-test-project/ltp/issues/991 > Signed-off-by: Wei Gao <wegao@suse.com> > Suggested-by: Petr Vorel <pvorel@suse.cz> > --- > testcases/lib/tst_net.sh | 2 ++ > 1 file changed, 2 insertions(+) > diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh > index ceb45c98d..ec915ad74 100644 > --- a/testcases/lib/tst_net.sh > +++ b/testcases/lib/tst_net.sh > @@ -205,6 +205,7 @@ tst_rhost_run() > sh_cmd="$pre_cmd $cmd $post_cmd" > if [ -n "${TST_USE_NETNS:-}" ]; then > + tst_require_cmds ip ns_create ns_exec ns_ifmove Why this? none of these commands is used here. > use="NETNS" > rcmd="$LTP_NETNS sh -c" > else > @@ -1006,6 +1007,7 @@ IPV6_RHOST="${IPV6_RHOST:-fd00:1:1:1::1/64}" > # tst_net_iface_prefix -h > # tst_net_vars -h > if [ -z "$_tst_net_parse_variables" ]; then > + tst_require_cmds tst_net_ip_prefix This is correct. Kind regards, Petr
On Tue, Jan 17, 2023 at 09:33:14AM +0100, Petr Vorel wrote: > Hi Wei, > > > More strict check for ns_xxx etc will help avoid following issue: > > https://github.com/linux-test-project/ltp/issues/991 > > > Signed-off-by: Wei Gao <wegao@suse.com> > > Suggested-by: Petr Vorel <pvorel@suse.cz> > > --- > > testcases/lib/tst_net.sh | 2 ++ > > 1 file changed, 2 insertions(+) > > > diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh > > index ceb45c98d..ec915ad74 100644 > > --- a/testcases/lib/tst_net.sh > > +++ b/testcases/lib/tst_net.sh > > @@ -205,6 +205,7 @@ tst_rhost_run() > > sh_cmd="$pre_cmd $cmd $post_cmd" > > > if [ -n "${TST_USE_NETNS:-}" ]; then > > + tst_require_cmds ip ns_create ns_exec ns_ifmove > Why this? none of these commands is used here. This line is the key to avoid wrong configuration(such as add ip inteface etc..) continue even ns_xxx not exist, you can check following code, line 222 will execute ns_xxx with error but not exit test immediately, so if we add require_cmds here then we can avoid it. (line 222 $rcmd will contain command ns_xxx) 218 tst_res_ TINFO "tst_rhost_run: cmd: $cmd" 219 tst_res_ TINFO "$use: $rcmd \"$sh_cmd\" $out 2>&1" 220 fi 221 222 output=$($rcmd "$sh_cmd" $out 2>&1 || echo 'RTERR') 223 224 echo "$output" | grep -q 'RTERR$' && ret=1 225 if [ $ret -eq 1 ]; then > > > use="NETNS" > > rcmd="$LTP_NETNS sh -c" > > else > > @@ -1006,6 +1007,7 @@ IPV6_RHOST="${IPV6_RHOST:-fd00:1:1:1::1/64}" > > # tst_net_iface_prefix -h > > # tst_net_vars -h > > if [ -z "$_tst_net_parse_variables" ]; then > > + tst_require_cmds tst_net_ip_prefix > This is correct. > > Kind regards, > Petr
> On Tue, Jan 17, 2023 at 09:33:14AM +0100, Petr Vorel wrote: > > Hi Wei, > > > More strict check for ns_xxx etc will help avoid following issue: > > > https://github.com/linux-test-project/ltp/issues/991 > > > Signed-off-by: Wei Gao <wegao@suse.com> > > > Suggested-by: Petr Vorel <pvorel@suse.cz> > > > --- > > > testcases/lib/tst_net.sh | 2 ++ > > > 1 file changed, 2 insertions(+) > > > diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh > > > index ceb45c98d..ec915ad74 100644 > > > --- a/testcases/lib/tst_net.sh > > > +++ b/testcases/lib/tst_net.sh > > > @@ -205,6 +205,7 @@ tst_rhost_run() > > > sh_cmd="$pre_cmd $cmd $post_cmd" > > > if [ -n "${TST_USE_NETNS:-}" ]; then > > > + tst_require_cmds ip ns_create ns_exec ns_ifmove > > Why this? none of these commands is used here. > This line is the key to avoid wrong configuration(such as add ip inteface etc..) continue even ns_xxx not exist, > you can check following code, line 222 will execute ns_xxx with error but not exit test immediately, so if we add > require_cmds here then we can avoid it. (line 222 $rcmd will contain command ns_xxx) > 218 tst_res_ TINFO "tst_rhost_run: cmd: $cmd" > 219 tst_res_ TINFO "$use: $rcmd \"$sh_cmd\" $out 2>&1" > 220 fi > 221 > 222 output=$($rcmd "$sh_cmd" $out 2>&1 || echo 'RTERR') > 223 > 224 echo "$output" | grep -q 'RTERR$' && ret=1 > 225 if [ $ret -eq 1 ]; then Long time ago the first run code for netns was init_ltp_netspace. I added IPv6 check quite recently. (I originally submitted it as run after, but in the end it is run before it.) I need to have a deeper look how tst_net_detect_ipv6 rhost works, calling tst_rhost_run before init_ltp_netspace should not work (interfaces aren't configured yet). But even if "ip ns_create ns_exec ns_ifmove" are needed to be checked to fix tst_rhost_run (not yet convinced), why to check them each time tst_rhost_run is called? It should be checked before first tst_rhost_run call to avoid useless repeating. Kind regards, Petr > > > use="NETNS" > > > rcmd="$LTP_NETNS sh -c" > > > else > > > @@ -1006,6 +1007,7 @@ IPV6_RHOST="${IPV6_RHOST:-fd00:1:1:1::1/64}" > > > # tst_net_iface_prefix -h > > > # tst_net_vars -h > > > if [ -z "$_tst_net_parse_variables" ]; then > > > + tst_require_cmds tst_net_ip_prefix > > This is correct. > > Kind regards, > > Petr
On Fri, Jan 20, 2023 at 09:38:43AM +0100, Petr Vorel wrote: > > On Tue, Jan 17, 2023 at 09:33:14AM +0100, Petr Vorel wrote: > > > Hi Wei, > > > > > More strict check for ns_xxx etc will help avoid following issue: > > > > https://github.com/linux-test-project/ltp/issues/991 > > > > > Signed-off-by: Wei Gao <wegao@suse.com> > > > > Suggested-by: Petr Vorel <pvorel@suse.cz> > > > > --- > > > > testcases/lib/tst_net.sh | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh > > > > index ceb45c98d..ec915ad74 100644 > > > > --- a/testcases/lib/tst_net.sh > > > > +++ b/testcases/lib/tst_net.sh > > > > @@ -205,6 +205,7 @@ tst_rhost_run() > > > > sh_cmd="$pre_cmd $cmd $post_cmd" > > > > > if [ -n "${TST_USE_NETNS:-}" ]; then > > > > + tst_require_cmds ip ns_create ns_exec ns_ifmove > > > Why this? none of these commands is used here. > > This line is the key to avoid wrong configuration(such as add ip inteface etc..) continue even ns_xxx not exist, > > you can check following code, line 222 will execute ns_xxx with error but not exit test immediately, so if we add > > require_cmds here then we can avoid it. (line 222 $rcmd will contain command ns_xxx) > > > 218 tst_res_ TINFO "tst_rhost_run: cmd: $cmd" > > 219 tst_res_ TINFO "$use: $rcmd \"$sh_cmd\" $out 2>&1" > > 220 fi > > 221 > > 222 output=$($rcmd "$sh_cmd" $out 2>&1 || echo 'RTERR') > > 223 > > 224 echo "$output" | grep -q 'RTERR$' && ret=1 > > 225 if [ $ret -eq 1 ]; then > > Long time ago the first run code for netns was init_ltp_netspace. > I added IPv6 check quite recently. (I originally submitted it as run after, but > in the end it is run before it.) > I need to have a deeper look how tst_net_detect_ipv6 rhost works, calling > tst_rhost_run before init_ltp_netspace should not work (interfaces aren't > configured yet). > > But even if "ip ns_create ns_exec ns_ifmove" are needed to be checked to fix > tst_rhost_run (not yet convinced), why to check them each time tst_rhost_run is > called? It should be checked before first tst_rhost_run call to avoid useless > repeating. tst_rhost_run already be called everywhere so if you need check before call tst_rhost_run, means lot of place/cases maybe need review and update. Furthermore, check each time in tst_rhost_run can make this function more robust, the old code also do tst_require_cmds if go ssh logic in line 211 for example. 205 sh_cmd="$pre_cmd $cmd $post_cmd" 206 207 if [ -n "${TST_USE_NETNS:-}" ]; then 208 use="NETNS" 209 rcmd="$LTP_NETNS sh -c" 210 else 211 tst_require_cmds ssh 212 use="SSH" 213 rcmd="ssh -nq $user@$RHOST" 214 fi > > Kind regards, > Petr > > > > > use="NETNS" > > > > rcmd="$LTP_NETNS sh -c" > > > > else > > > > @@ -1006,6 +1007,7 @@ IPV6_RHOST="${IPV6_RHOST:-fd00:1:1:1::1/64}" > > > > # tst_net_iface_prefix -h > > > > # tst_net_vars -h > > > > if [ -z "$_tst_net_parse_variables" ]; then > > > > + tst_require_cmds tst_net_ip_prefix > > > This is correct. > > > > Kind regards, > > > Petr
Hi Wei, ... > > But even if "ip ns_create ns_exec ns_ifmove" are needed to be checked to fix > > tst_rhost_run (not yet convinced), why to check them each time tst_rhost_run is > > called? It should be checked before first tst_rhost_run call to avoid useless > > repeating. > tst_rhost_run already be called everywhere so if you need check before call > tst_rhost_run, means lot of place/cases maybe need review and update. No, the check would be on single place at tst_net.sh. > Furthermore, check each time in tst_rhost_run can make this function more robust, the > old code also do tst_require_cmds if go ssh logic in line 211 for example. Well, repeated check for ssh is not ideal either (how likely is that ssh will disappear during testing?), but it's a single command which is used here. And it's definitely less awkward than checking "ip ns_create ns_exec ns_ifmove" which aren't used there. tst_net.sh needs cleanup, not more unneeded code. We could document the reason to make this check more obvious, but the code is wrong, because for netns (the default) running "tst_net_detect_ipv6 rhost" before init_ltp_netspace does not make sense, because interfaces aren't defined (probably error code is not detected, I need to recheck that). It's actually enough for netns to test IPv6 support only on localhost (it will be the same on rhost), I'll send a patch to fix that. Kind regards, Petr > 205 sh_cmd="$pre_cmd $cmd $post_cmd" > 206 > 207 if [ -n "${TST_USE_NETNS:-}" ]; then > 208 use="NETNS" > 209 rcmd="$LTP_NETNS sh -c" > 210 else > 211 tst_require_cmds ssh > 212 use="SSH" > 213 rcmd="ssh -nq $user@$RHOST" > 214 fi
Hi Wei, > ... > > > But even if "ip ns_create ns_exec ns_ifmove" are needed to be checked to fix > > > tst_rhost_run (not yet convinced), why to check them each time tst_rhost_run is > > > called? It should be checked before first tst_rhost_run call to avoid useless > > > repeating. > > tst_rhost_run already be called everywhere so if you need check before call > > tst_rhost_run, means lot of place/cases maybe need review and update. > No, the check would be on single place at tst_net.sh. > > Furthermore, check each time in tst_rhost_run can make this function more robust, the > > old code also do tst_require_cmds if go ssh logic in line 211 for example. > Well, repeated check for ssh is not ideal either (how likely is that ssh will > disappear during testing?), but it's a single command which is used here. > And it's definitely less awkward than checking "ip ns_create ns_exec ns_ifmove" > which aren't used there. tst_net.sh needs cleanup, not more unneeded code. > We could document the reason to make this check more obvious, but the code is > wrong, because for netns (the default) running "tst_net_detect_ipv6 rhost" > before init_ltp_netspace does not make sense, because interfaces aren't defined > (probably error code is not detected, I need to recheck that). > It's actually enough for netns to test IPv6 support only on localhost (it will > be the same on rhost), I'll send a patch to fix that. I'm sorry, I was wrong. tst_net_detect_ipv6() checks [ -f /proc/net/if_inet6 ], which is ok even before interfaces are set. It's a question whether this needs to be run on netns also on rhost (IMHO the result will be always the same). We should investigate that. FYI I changed your patch to use just check in $_tst_net_parse_variables and put it into larger cleanup. I haven't addressed move tools to testcases/lib/ yet, nor the case when tools are missing. Instead I fixed few real problems with IPv6 disabled and did cleanup. https://lore.kernel.org/ltp/20230126215401.29101-1-pvorel@suse.cz/ https://patchwork.ozlabs.org/project/ltp/list/?series=338700&state=* Kind regards, Petr
diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh index ceb45c98d..ec915ad74 100644 --- a/testcases/lib/tst_net.sh +++ b/testcases/lib/tst_net.sh @@ -205,6 +205,7 @@ tst_rhost_run() sh_cmd="$pre_cmd $cmd $post_cmd" if [ -n "${TST_USE_NETNS:-}" ]; then + tst_require_cmds ip ns_create ns_exec ns_ifmove use="NETNS" rcmd="$LTP_NETNS sh -c" else @@ -1006,6 +1007,7 @@ IPV6_RHOST="${IPV6_RHOST:-fd00:1:1:1::1/64}" # tst_net_iface_prefix -h # tst_net_vars -h if [ -z "$_tst_net_parse_variables" ]; then + tst_require_cmds tst_net_ip_prefix eval $(tst_net_ip_prefix $IPV4_LHOST || echo "exit $?") eval $(tst_net_ip_prefix -r $IPV4_RHOST || echo "exit $?")
More strict check for ns_xxx etc will help avoid following issue: https://github.com/linux-test-project/ltp/issues/991 Signed-off-by: Wei Gao <wegao@suse.com> Suggested-by: Petr Vorel <pvorel@suse.cz> --- testcases/lib/tst_net.sh | 2 ++ 1 file changed, 2 insertions(+)