[LTP] [PATCH 1/1] net/sendfile01.sh: Check with timeout

[LTP] [PATCH 1/1] net/sendfile01.sh: Check with timeout

[Petr Vorel]

Suggested-by: Alexey Kodanev <alexey.kodanev@oracle.com>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Hi Alexey,

I guess nothing controversial here as failure of starting server is
guarded by -s.
I was thinking about using TST_RETRY_FUNC, but passing command to it
leads to: tst_rhost_run: unknown option: l

What bothers me more, that TST_NEEDS_CMDS does not check command on
rhost. Do we want to have something like TST_NEEDS_CMDS_RHOST or we just
don't care?

Kind regards,
Petr

 testcases/network/tcp_cmds/sendfile/sendfile01.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/testcases/network/tcp_cmds/sendfile/sendfile01.sh b/testcases/network/tcp_cmds/sendfile/sendfile01.sh
index af9cadf57..94fff2c9a 100755
--- a/testcases/network/tcp_cmds/sendfile/sendfile01.sh
+++ b/testcases/network/tcp_cmds/sendfile/sendfile01.sh
@@ -11,7 +11,7 @@ TST_SETUP=do_setup
 TST_CLEANUP=do_cleanup
 TST_TESTFUNC=do_test
 TST_NEEDS_TMPDIR=1
-TST_NEEDS_CMDS="diff stat"
+TST_NEEDS_CMDS="diff ss stat"
 . tst_net.sh
 
 do_setup()
@@ -28,7 +28,10 @@ do_setup()
 	tst_rhost_run -s -b -c "$server $(tst_ipaddr rhost) $port"
 	server_started=1
 	tst_res TINFO "wait for the server to start"
-	sleep 1
+	while true; do
+		tst_rhost_run -c "ss -ltp" | grep -q "$port.*testsf" && break
+		tst_sleep 10ms
+	done
 }
 
 do_test()
-- 
2.26.0

[LTP] [PATCH 1/1] net/sendfile01.sh: Check with timeout

[Alexey Kodanev]

On 21.04.2020 21:00, Petr Vorel wrote:
> Suggested-by: Alexey Kodanev <alexey.kodanev@oracle.com>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> Hi Alexey,
> 
> I guess nothing controversial here as failure of starting server is
> guarded by -s.
> I was thinking about using TST_RETRY_FUNC, but passing command to it
> leads to: tst_rhost_run: unknown option: l
> 

Hi Petr,

eval might help in this case, take a look at tst_retry() in test.sh
old api, not sure why exactly it was removed in the new one...

index 1d8a71d9f..e34edb26a 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -207,7 +207,7 @@ TST_RETRY_FN_EXP_BACKOFF()
        fi
 
        while true; do
-               $tst_fun
+               eval "$tst_fun"
                if [ "$?" = "$tst_exp" ]; then
                        break
                fi

> What bothers me more, that TST_NEEDS_CMDS does not check command on
> rhost. Do we want to have something like TST_NEEDS_CMDS_RHOST or we just
> don't care?

In general, yes, we need to check if a command exists on the remote host.
Yet another variable, what about checking what in TST_NEEDS_CMDS on the
remote host? Though TST_NEEDS_CMDS_RHOST looks quite well.

> 
> Kind regards,
> Petr
> 
>  testcases/network/tcp_cmds/sendfile/sendfile01.sh | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/testcases/network/tcp_cmds/sendfile/sendfile01.sh b/testcases/network/tcp_cmds/sendfile/sendfile01.sh
> index af9cadf57..94fff2c9a 100755
> --- a/testcases/network/tcp_cmds/sendfile/sendfile01.sh
> +++ b/testcases/network/tcp_cmds/sendfile/sendfile01.sh
> @@ -11,7 +11,7 @@ TST_SETUP=do_setup
>  TST_CLEANUP=do_cleanup
>  TST_TESTFUNC=do_test
>  TST_NEEDS_TMPDIR=1
> -TST_NEEDS_CMDS="diff stat"
> +TST_NEEDS_CMDS="diff ss stat"
>  . tst_net.sh
>  
>  do_setup()
> @@ -28,7 +28,10 @@ do_setup()
>  	tst_rhost_run -s -b -c "$server $(tst_ipaddr rhost) $port"
>  	server_started=1
>  	tst_res TINFO "wait for the server to start"
> -	sleep 1
> +	while true; do
> +		tst_rhost_run -c "ss -ltp" | grep -q "$port.*testsf" && break
> +		tst_sleep 10ms
> +	done
>  }
>  
>  do_test()
> 

[LTP] [PATCH 1/1] net/sendfile01.sh: Check with timeout

[Petr Vorel]

Hi Alexey,

> > I guess nothing controversial here as failure of starting server is
> > guarded by -s.
> > I was thinking about using TST_RETRY_FUNC, but passing command to it
> > leads to: tst_rhost_run: unknown option: l


> Hi Petr,

> eval might help in this case, take a look at tst_retry() in test.sh
Good point.

> old api, not sure why exactly it was removed in the new one...
It was designed from scratch I guess.
But this patch makes sense to me, I'll test it tomorrow.

> index 1d8a71d9f..e34edb26a 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -207,7 +207,7 @@ TST_RETRY_FN_EXP_BACKOFF()
>         fi

>         while true; do
> -               $tst_fun
> +               eval "$tst_fun"
>                 if [ "$?" = "$tst_exp" ]; then
>                         break
>                 fi

> > What bothers me more, that TST_NEEDS_CMDS does not check command on
> > rhost. Do we want to have something like TST_NEEDS_CMDS_RHOST or we just
> > don't care?

> In general, yes, we need to check if a command exists on the remote host.
> Yet another variable, what about checking what in TST_NEEDS_CMDS on the
> remote host? Though TST_NEEDS_CMDS_RHOST looks quite well.
In this case ss was needed only on rhost.
I don't know if SUT in two hosts configurations (ssh/rsh) are identical.
If yes, it'd be enough just to check TST_NEEDS_CMDS also on rhost.
I'm for this variant as it's a simpler change. I'll send a patch tomorrow.

Kind regards,
Petr

[LTP] [PATCH 1/1] net/sendfile01.sh: Check with timeout

[Petr Vorel]

Hi Alexey,

> > > I guess nothing controversial here as failure of starting server is
> > > guarded by -s.
> > > I was thinking about using TST_RETRY_FUNC, but passing command to it
> > > leads to: tst_rhost_run: unknown option: l


> > Hi Petr,

> > eval might help in this case, take a look at tst_retry() in test.sh
> Good point.

> > old api, not sure why exactly it was removed in the new one...
> It was designed from scratch I guess.
> But this patch makes sense to me, I'll test it tomorrow.

Actually, I now consider a bit cleaner and safer solution to *not* use eval
and require test to specify function. E.g.:

+retry_fnc()
+{
+	tst_rhost_run -c 'ss -ltp' | grep -q "$port.*testsf"
+}
+
 do_setup()
 {
 
@@ -28,7 +33,7 @@ do_setup()
 	tst_rhost_run -s -b -c "$server $(tst_ipaddr rhost) $port"
 	server_started=1
 	tst_res TINFO "wait for the server to start"
-	sleep 1
+	TST_RETRY_FUNC retry_fnc 0
 }

Instead of simple:

 do_setup()
@@ -28,7 +28,7 @@ do_setup()
 	tst_rhost_run -s -b -c "$server $(tst_ipaddr rhost) $port"
 	server_started=1
 	tst_res TINFO "wait for the server to start"
-	sleep 1
+	TST_RETRY_FUNC "tst_rhost_run -c 'ss -ltp' | grep -q '$port.*testsf'" 0
 }

But I don't have strong opinion on it.
Cyril, Li, any preference?

Kind regards,
Petr

> > index 1d8a71d9f..e34edb26a 100644
> > --- a/testcases/lib/tst_test.sh
> > +++ b/testcases/lib/tst_test.sh
> > @@ -207,7 +207,7 @@ TST_RETRY_FN_EXP_BACKOFF()
> >         fi

> >         while true; do
> > -               $tst_fun
> > +               eval "$tst_fun"
> >                 if [ "$?" = "$tst_exp" ]; then
> >                         break
> >                 fi
...

[LTP] [PATCH 1/1] net/sendfile01.sh: Check with timeout

[Li Wang]

Petr Vorel <pvorel@suse.cz> wrote:

...
>
> > > old api, not sure why exactly it was removed in the new one...
> > It was designed from scratch I guess.
> > But this patch makes sense to me, I'll test it tomorrow.
>
> Actually, I now consider a bit cleaner and safer solution to *not* use eval
> and require test to specify function. E.g.:
>

Why not use eval for that? It helps us to perform more commands directly
without wrap into function.


> +retry_fnc()
> +{
> +       tst_rhost_run -c 'ss -ltp' | grep -q "$port.*testsf"
> +}
> +
>  do_setup()
>  {
>
> @@ -28,7 +33,7 @@ do_setup()
>         tst_rhost_run -s -b -c "$server $(tst_ipaddr rhost) $port"
>         server_started=1
>         tst_res TINFO "wait for the server to start"
> -       sleep 1
> +       TST_RETRY_FUNC retry_fnc 0
>  }
>
> Instead of simple:
>
>  do_setup()
> @@ -28,7 +28,7 @@ do_setup()
>         tst_rhost_run -s -b -c "$server $(tst_ipaddr rhost) $port"
>         server_started=1
>         tst_res TINFO "wait for the server to start"
> -       sleep 1
> +       TST_RETRY_FUNC "tst_rhost_run -c 'ss -ltp' | grep -q
> '$port.*testsf'" 0
>  }
>
> But I don't have strong opinion on it.
> Cyril, Li, any preference?
>

If no more strict reasons I prefer to go the simpler way. And there is no
need to wrap a retry_fun() I think.

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20200425/a8bc5114/attachment.htm>

[LTP] [PATCH 1/1] net/sendfile01.sh: Check with timeout

[Petr Vorel]

Hi Li,

> > > > old api, not sure why exactly it was removed in the new one...
> > > It was designed from scratch I guess.
> > > But this patch makes sense to me, I'll test it tomorrow.

> > Actually, I now consider a bit cleaner and safer solution to *not* use eval
> > and require test to specify function. E.g.:


> Why not use eval for that? It helps us to perform more commands directly
> without wrap into function.


> > +retry_fnc()
> > +{
> > +       tst_rhost_run -c 'ss -ltp' | grep -q "$port.*testsf"
> > +}
> > +
> >  do_setup()
> >  {

> > @@ -28,7 +33,7 @@ do_setup()
> >         tst_rhost_run -s -b -c "$server $(tst_ipaddr rhost) $port"
> >         server_started=1
> >         tst_res TINFO "wait for the server to start"
> > -       sleep 1
> > +       TST_RETRY_FUNC retry_fnc 0
> >  }

> > Instead of simple:

> >  do_setup()
> > @@ -28,7 +28,7 @@ do_setup()
> >         tst_rhost_run -s -b -c "$server $(tst_ipaddr rhost) $port"
> >         server_started=1
> >         tst_res TINFO "wait for the server to start"
> > -       sleep 1
> > +       TST_RETRY_FUNC "tst_rhost_run -c 'ss -ltp' | grep -q
> > '$port.*testsf'" 0
> >  }

> > But I don't have strong opinion on it.
> > Cyril, Li, any preference?


> If no more strict reasons I prefer to go the simpler way. And there is no
> need to wrap a retry_fun() I think.

I'm a bit careful and try to avoid eval in scripts for security reasons.
But ok, LTP code is a bit different from running shell scripts on the server,
security does not matters on SUT, so I'm not against it. I just wanted to hear
more opinions on that, thanks for your comment.

Kind regards,
Petr

[LTP] [PATCH 1/1] net/sendfile01.sh: Check with timeout

[Petr Vorel]

Hi all,

> > What bothers me more, that TST_NEEDS_CMDS does not check command on
> > rhost. Do we want to have something like TST_NEEDS_CMDS_RHOST or we just
> > don't care?

> In general, yes, we need to check if a command exists on the remote host.
> Yet another variable, what about checking what in TST_NEEDS_CMDS on the
> remote host? Though TST_NEEDS_CMDS_RHOST looks quite well.

Hm, looking on the implementation. tst_require_cmds() and tst_cmd_available()
are functions, thus not runnable with tst_rhost_run(). We can add this code to
custom shell script which checks or C code (shell has dependencies). Or any
other idea?

Kind regards,
Petr

[LTP] [PATCH 1/1] net/sendfile01.sh: Check with timeout

[Petr Vorel]

Hi all,

> > > What bothers me more, that TST_NEEDS_CMDS does not check command on
> > > rhost. Do we want to have something like TST_NEEDS_CMDS_RHOST or we just
> > > don't care?

> > In general, yes, we need to check if a command exists on the remote host.
> > Yet another variable, what about checking what in TST_NEEDS_CMDS on the
> > remote host? Though TST_NEEDS_CMDS_RHOST looks quite well.

> Hm, looking on the implementation. tst_require_cmds() and tst_cmd_available()
> are functions, thus not runnable with tst_rhost_run(). We can add this code to
> custom shell script which checks or C code (shell has dependencies). Or any
> other idea?

BTW normally it works to load script and then run function.
$ cat foo.sh
#!/bin/sh
foo() { echo "foo: $$"; }

$ sh -c ". foo.sh; foo"
foo: 32479

But this somehow doesn't work for tst_net.sh:

diff --git testcases/lib/tst_net.sh testcases/lib/tst_net.sh
index 1ec03c738..728987f0b 100644
--- testcases/lib/tst_net.sh
+++ testcases/lib/tst_net.sh
@@ -57,6 +57,7 @@ tst_net_remote_tmpdir()
 
 tst_net_setup()
 {
+	tst_rhost_run -c ". tst_net.sh; tst_require_cmds $TST_NEEDS_CMDS"
 	tst_net_remote_tmpdir
 	[ -n "$TST_SETUP_CALLER" ] && $TST_SETUP_CALLER
 

results in:
/opt/ltp/testcases/bin/tst_net.sh: line 944: tst_cmd_available: command not found

tst_cmd_available() is needed for the end of tst_net.sh:
if [ -z "$TST_USE_LEGACY_API" ] && ! tst_cmd_available ping6; then
	ping6()
	{
		ping -6 $@
	}
	if [ -z "$_tst_net_ping6_warn_printed" ]; then
		tst_res_ TINFO "ping6 binary/symlink is missing, using workaround. Please, report missing ping6 to your distribution."
		export _tst_net_ping6_warn_printed=1
	fi
fi

=> sourcing of shell scripts with "." does not work on 'sh -c'.

Kind regards,
Petr