[LTP] [PATCH v1] tst_net.sh: Add more tst_require_cmds check

[LTP] [PATCH v1] tst_net.sh: Add more tst_require_cmds check

[Wei Gao via ltp]

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
 		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 $?")
 
-- 
2.35.3


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

Re: [LTP] [PATCH v1] tst_net.sh: Add more tst_require_cmds check

[Petr Vorel]

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

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

Re: [LTP] [PATCH v1] tst_net.sh: Add more tst_require_cmds check

[Wei Gao via ltp]

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

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

Re: [LTP] [PATCH v1] tst_net.sh: Add more tst_require_cmds check

[Petr Vorel]

> 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

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

Re: [LTP] [PATCH v1] tst_net.sh: Add more tst_require_cmds check

[Wei Gao via ltp]

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

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

Re: [LTP] [PATCH v1] tst_net.sh: Add more tst_require_cmds check

[Petr Vorel]

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

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

Re: [LTP] [PATCH v1] tst_net.sh: Add more tst_require_cmds check

[Petr Vorel]

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

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