[LTP] [PATCH v4 1/2] net: Add tst_net_run helper

[LTP] [PATCH v4 1/2] net: Add tst_net_run helper

[Petr Vorel]

and use it in tst_set_sysctl

+ improve doc of tst_rhost_run()

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

Maybe we could signal where error occurred:
* return 0: ok on both lhost and rhost
* return 1: failure on lhost
* return 2: failure on rhost
* return 3: failure on both lhost and rhost

But instead I'd be for tst_rhost_run return real exit code, not always 1
for any error.

Workaround for sh -c requiring binary could be to create temporary file
which would source tst_net.sh and than run the command. Maybe a bit
complicated.

Kind regards,
Petr
---
 testcases/lib/tst_net.sh | 61 +++++++++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 10 deletions(-)

diff --git a/testcases/lib/tst_net.sh b/testcases/lib/tst_net.sh
index d1206e285..f8f7be0f2 100644
--- a/testcases/lib/tst_net.sh
+++ b/testcases/lib/tst_net.sh
@@ -131,7 +131,8 @@ init_ltp_netspace()
 # -b run in background
 # -B run in background and save output to $TST_TMPDIR/bg.cmd
 # -s safe option, if something goes wrong, will exit with TBROK
-# -c specify command to run
+# -c specify command to run (this must be binary, not shell buildin/function)
+# RETURN: 0 on success, 1 on failure
 tst_rhost_run()
 {
 	local pre_cmd=
@@ -191,6 +192,52 @@ tst_rhost_run()
 	return $ret
 }
 
+# Run command on both lhost and rhost.
+# tst_net_run [-s] [-l LPARAM] [-r RPARAM] CMD [ARG [ARG2]]
+# Options:
+# LPARAM: parameter passed to CMD in lhost
+# RPARAM: parameter passed to CMD in rhost
+# CMD: command to run (this must be binary, not shell buildin/function due
+# tst_rhost_run() limitation)
+# RETURN: 0 on success, 1 on missing CMD or exit code of cmd failure of on lhost or rhost
+tst_net_run()
+{
+	local cmd
+	local lparams
+	local rparams
+	local lsafe
+	local rsafe
+	local lret
+	local rret
+
+	local OPTIND
+	while getopts l:r:s opt; do
+		case "$opt" in
+		l) lparams="$OPTARG" ;;
+		r) rparams="$OPTARG" ;;
+		s) lsafe="ROD"; rsafe="-s" ;;
+		*) tst_brk_ TBROK "tst_net_run: unknown option: $OPTARG" ;;
+		esac
+	done
+	shift $((OPTIND - 1))
+	cmd="$1"
+	shift
+
+	if [ -z "$cmd" ]; then
+		[ -n "$lsafe" ] && \
+			tst_brk_ TBROK "tst_net_run: command not defined"
+		tst_res_ TWARN "tst_net_run: command not defined"
+		return 1
+	fi
+
+	$lsafe $cmd $lparams $@
+	lret=$?
+	tst_rhost_run $rsafe -c "$cmd $rparams $@"
+	rret=$?
+	[ $lret -ne 0 ] && return $lret
+	return $rret
+}
+
 EXPECT_RHOST_PASS()
 {
 	tst_rhost_run -c "$*" > /dev/null
@@ -642,16 +689,10 @@ tst_set_sysctl()
 	local safe=
 	[ "$3" = "safe" ] && safe="-s"
 
-	local add_opt=
-	[ "$TST_USE_NETNS" = "yes" ] && add_opt="-e"
-
-	if [ "$safe" ]; then
-		ROD sysctl -q -w $name=$value
-	else
-		sysctl -q -w $name=$value
-	fi
+	local rparam=
+	[ "$TST_USE_NETNS" = "yes" ] && rparam="-e"
 
-	tst_rhost_run $safe -c "sysctl -q -w $add_opt $name=$value"
+	tst_net_run $safe -r $rparam sysctl -q -w $name=$value
 }
 
 tst_cleanup_rhost()
-- 
2.19.1

[LTP] [PATCH v4 2/2] net/ipsec: Add check for xfrm_user kernel module

[Petr Vorel]

xfrm_user is required by ip xfrm command.
Check is performed only when needed.
Call in cleanup function could trigger infinite loop,
therefore check only once.

NOTE: we cannot use tst_test_drivers() due tst_rhost_run() limitation.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
Suggested-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---
 testcases/network/stress/ipsec/ipsec_lib.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/testcases/network/stress/ipsec/ipsec_lib.sh b/testcases/network/stress/ipsec/ipsec_lib.sh
index b099fdeaf..74d4fc0cb 100644
--- a/testcases/network/stress/ipsec/ipsec_lib.sh
+++ b/testcases/network/stress/ipsec/ipsec_lib.sh
@@ -81,6 +81,12 @@ ipsec_lib_setup()
 	cleanup_vti=
 	ALG=
 	ALGR=
+
+	if [ -n "$IPSEC_MODE" ]; then
+		tst_net_run "tst_check_drivers xfrm_user" || \
+			tst_brk TCONF "xfrm_user driver not available on lhost or rhost"
+		cleanup_xfrm=1
+	fi
 }
 
 TST_OPTS="l:m:p:s:S:k:A:e:a:c:r:"
@@ -110,6 +116,8 @@ tst_ipsec_setup()
 # tst_ipsec_cleanup: flush ipsec state and policy rules
 tst_ipsec_cleanup()
 {
+	[ -z "$cleanup_xfrm" ] && return
+
 	ip xfrm state flush
 	ip xfrm policy flush
 	tst_rhost_run -c "ip xfrm state flush && ip xfrm policy flush"
-- 
2.19.1

[LTP] [PATCH v4 1/2] net: Add tst_net_run helper

[Alexey Kodanev]

Hi Petr,
On 13.11.2018 19:30, Petr Vorel wrote:
> and use it in tst_set_sysctl
> 
> + improve doc of tst_rhost_run()
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> Reviewed-by: Alexey Kodanev <alexey.kodanev@oracle.com>
> ---
> Hi,
> 
> Maybe we could signal where error occurred:
> * return 0: ok on both lhost and rhost
> * return 1: failure on lhost
> * return 2: failure on rhost
> * return 3: failure on both lhost and rhost
> 

It is only when there is no "safe" option, right? when doing cleanup?
Could we print TWARN message inside tst_net_run() in such case?

[LTP] [PATCH v4 1/2] net: Add tst_net_run helper

[Petr Vorel]

Hi Alexey,

> > Maybe we could signal where error occurred:
> > * return 0: ok on both lhost and rhost
> > * return 1: failure on lhost
> > * return 2: failure on rhost
> > * return 3: failure on both lhost and rhost


> It is only when there is no "safe" option, right?
yes, as safe exit with TCONF and warning.

> when doing cleanup?
IMHO in any "non-safe" run.

> Could we print TWARN message inside tst_net_run() in such case?
I wanted tst_net_run() to behave similarly as tst_rhost_run(), which doesn't
warn about it. So I propose to add warning to both functions, but also add -q
flag (quiet), which suppress this warning.

The reason for quiet mode is, that (I suppose) there are cases when some
features is tested and TWARN isn't wanted. Some examples (looks -s is used often
in cleanup functions):

testcases/network/stress/multicast/grp-operation/mcast-lib.sh:

mcast_cleanup()
{
    [ "$TST_IPV6" ] && mcast_cleanup6 || mcast_cleanup4

    pkill -SIGHUP -f "$MCAST_LCMD"
    tst_sleep 10ms
    pkill -9 -f "$MCAST_LCMD"

    tst_rhost_run -c "pkill -SIGHUP -f '$MCAST_RCMD'"
}

testcases/network/virt/virt_lib.sh
virt_cleanup_rmt()
{
    cleanup_vifaces
    tst_rhost_run -c "ip link delete ltp_v0 2>/dev/null"
    if [ "$virt_tcp_syn" ]; then
        sysctl -q net.ipv4.tcp_syn_retries=$virt_tcp_syn
        virt_tcp_syn=
    fi
}

nfs_setup_server() in testcases/network/nfs/nfs_stress/nfs_lib.sh:

	if ! tst_rhost_run -c "test -d $remote_dir"; then
        tst_rhost_run -s -c "mkdir -p $remote_dir; $export_cmd"
    fi


Kind regards,
Petr

[LTP] [PATCH v4 1/2] net: Add tst_net_run helper

[Alexey Kodanev]

Hi Petr,
On 16.11.2018 17:24, Petr Vorel wrote:
> Hi Alexey,
> 
>>> Maybe we could signal where error occurred:
>>> * return 0: ok on both lhost and rhost
>>> * return 1: failure on lhost
>>> * return 2: failure on rhost
>>> * return 3: failure on both lhost and rhost
> 
> 
>> It is only when there is no "safe" option, right?
> yes, as safe exit with TCONF and warning.
> 
>> when doing cleanup?
> IMHO in any "non-safe" run.
> 
>> Could we print TWARN message inside tst_net_run() in such case?
> I wanted tst_net_run() to behave similarly as tst_rhost_run(), which doesn't
> warn about it. So I propose to add warning to both functions, but also add -q
> flag (quiet), which suppress this warning.

I think we don't need it for tst_rhost_run() as it is clear where the
error has happened. Unlike tst_net_run() without safe option.

> 
> The reason for quiet mode is, that (I suppose) there are cases when some
> features is tested and TWARN isn't wanted. Some examples (looks -s is used often
> in cleanup functions):

'-s' is used often in cleanup with tst_rhost_run()?

>
> testcases/network/stress/multicast/grp-operation/mcast-lib.sh:
> 
> mcast_cleanup()
> {
>     [ "$TST_IPV6" ] && mcast_cleanup6 || mcast_cleanup4
> 
>     pkill -SIGHUP -f "$MCAST_LCMD"
>     tst_sleep 10ms
>     pkill -9 -f "$MCAST_LCMD"
> 
>     tst_rhost_run -c "pkill -SIGHUP -f '$MCAST_RCMD'"
> }
> 
> testcases/network/virt/virt_lib.sh
> virt_cleanup_rmt()
> {
>     cleanup_vifaces
>     tst_rhost_run -c "ip link delete ltp_v0 2>/dev/null"
>     if [ "$virt_tcp_syn" ]; then
>         sysctl -q net.ipv4.tcp_syn_retries=$virt_tcp_syn
>         virt_tcp_syn=
>     fi
> }
> 
> nfs_setup_server() in testcases/network/nfs/nfs_stress/nfs_lib.sh:
> 
> 	if ! tst_rhost_run -c "test -d $remote_dir"; then
>         tst_rhost_run -s -c "mkdir -p $remote_dir; $export_cmd"
>     fi
> 
> 
> Kind regards,
> Petr
> 

[LTP] [PATCH v4 1/2] net: Add tst_net_run helper

[Petr Vorel]

Hi Alexey,

> >>> Maybe we could signal where error occurred:
> >>> * return 0: ok on both lhost and rhost
> >>> * return 1: failure on lhost
> >>> * return 2: failure on rhost
> >>> * return 3: failure on both lhost and rhost


> >> It is only when there is no "safe" option, right?
> > yes, as safe exit with TCONF and warning.

> >> when doing cleanup?
> > IMHO in any "non-safe" run.

> >> Could we print TWARN message inside tst_net_run() in such case?
> > I wanted tst_net_run() to behave similarly as tst_rhost_run(), which doesn't
> > warn about it. So I propose to add warning to both functions, but also add -q
> > flag (quiet), which suppress this warning.

> I think we don't need it for tst_rhost_run() as it is clear where the
> error has happened. Unlike tst_net_run() without safe option.
OK, make sense, I'll put it just into tst_net_run().


> > The reason for quiet mode is, that (I suppose) there are cases when some
> > features is tested and TWARN isn't wanted. Some examples (looks -s is used often
> > in cleanup functions):

> '-s' is used often in cleanup with tst_rhost_run()?
Sorry, I was wrong. Previously I meant tst_rhost_run *without -s* option.

tst_rhost_run with -s option in cleanup is only in 2 files:
testcases/network/mpls/mpls_lib.sh
testcases/network/tcp_cmds/sendfile/sendfile01
Maybe that's a mistake (unlike C functions where error in SAFE_* continue with
warning, in shell it exits during cleanup).

Without -s we don't get any warning, which is often desirable.
=> I'll put warning and -q switch to suppress it only into tst_net_run().


Kind regards,
Petr

[LTP] [PATCH v4 1/2] net: Add tst_net_run helper

[Alexey Kodanev]

Hi Petr,
On 11/19/2018 06:40 PM, Petr Vorel wrote:
> Hi Alexey,
> 
>>>>> Maybe we could signal where error occurred:
>>>>> * return 0: ok on both lhost and rhost
>>>>> * return 1: failure on lhost
>>>>> * return 2: failure on rhost
>>>>> * return 3: failure on both lhost and rhost
> 
> 
>>>> It is only when there is no "safe" option, right?
>>> yes, as safe exit with TCONF and warning.
> 
>>>> when doing cleanup?
>>> IMHO in any "non-safe" run.
> 
>>>> Could we print TWARN message inside tst_net_run() in such case?
>>> I wanted tst_net_run() to behave similarly as tst_rhost_run(), which doesn't
>>> warn about it. So I propose to add warning to both functions, but also add -q
>>> flag (quiet), which suppress this warning.
> 
>> I think we don't need it for tst_rhost_run() as it is clear where the
>> error has happened. Unlike tst_net_run() without safe option.
> OK, make sense, I'll put it just into tst_net_run().
> 
> 
>>> The reason for quiet mode is, that (I suppose) there are cases when some
>>> features is tested and TWARN isn't wanted. Some examples (looks -s is used often
>>> in cleanup functions):
> 
>> '-s' is used often in cleanup with tst_rhost_run()?
> Sorry, I was wrong. Previously I meant tst_rhost_run *without -s* option.
> 
> tst_rhost_run with -s option in cleanup is only in 2 files:
> testcases/network/mpls/mpls_lib.sh
> testcases/network/tcp_cmds/sendfile/sendfile01
> Maybe that's a mistake (unlike C functions where error in SAFE_* continue with
> warning, in shell it exits during cleanup).
> 

For tst_test.sh we could add something like this to tst_brk()

iff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 16081fa..e84cf7f 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -41,6 +41,7 @@ trap "tst_brk TBROK 'test interrupted'" INT
 _tst_do_exit()
 {
        local ret=0
+       TST_DO_EXIT=1

        if [ -n "$TST_SETUP_STARTED" -a -n "$TST_CLEANUP" -a \
             -z "$TST_NO_CLEANUP" ]; then
@@ -123,6 +124,11 @@ tst_brk()
        local res=$1
        shift

+       if [ "$TST_DO_EXIT" = 1 ]; then
+               tst_res TWARN "$@"
+               return
+       fi
+
        tst_res "$res" "$@"
        _tst_do_exit
 }


> Without -s we don't get any warning, which is often desirable.
> => I'll put warning and -q switch to suppress it only into tst_net_run().
> 
> 
> Kind regards,
> Petr
> 

[LTP] [PATCH v4 1/2] net: Add tst_net_run helper

[Petr Vorel]

Hi Alexey,

> > tst_rhost_run with -s option in cleanup is only in 2 files:
> > testcases/network/mpls/mpls_lib.sh
> > testcases/network/tcp_cmds/sendfile/sendfile01
> > Maybe that's a mistake (unlike C functions where error in SAFE_* continue with
> > warning, in shell it exits during cleanup).


> For tst_test.sh we could add something like this to tst_brk()

> iff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> index 16081fa..e84cf7f 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -41,6 +41,7 @@ trap "tst_brk TBROK 'test interrupted'" INT
>  _tst_do_exit()
>  {
>         local ret=0
> +       TST_DO_EXIT=1

>         if [ -n "$TST_SETUP_STARTED" -a -n "$TST_CLEANUP" -a \
>              -z "$TST_NO_CLEANUP" ]; then
> @@ -123,6 +124,11 @@ tst_brk()
>         local res=$1
>         shift

> +       if [ "$TST_DO_EXIT" = 1 ]; then
> +               tst_res TWARN "$@"
> +               return
> +       fi
> +
>         tst_res "$res" "$@"
>         _tst_do_exit
>  }

Good idea! Could you please send a patch?


Kind regards,
Petr