Hi Geliang,
On 25/09/2023 10:42, Geliang Tang wrote:
> verify_listener_events() helper is defined both in mptcp_join.sh and
> userspace_pm.sh, export it into mptcp_lib.sh and rename it with
> mptcp_lib_ prefix. Use this new helper in both scripts.
Good idea but please see below: because there are quite a few small
differences between mptcp_join.sh and userspace_pm.sh in the way we
print stuff, mark subtests as failed, print errors, etc. I think we are
not ready for such helper and it might be easier to drop this patch for
the moment. What do you think?
(if we want to keep this patch, please don't forget to clearly mention
the reason first (copy-paste from my comment on patch 21/29) before
describing what you did)
(...)
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 8c708b92a942..e59867eed5c2 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -2803,59 +2803,6 @@ backup_tests()
> fi
> }
>
> -LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
> -LISTENER_CLOSED=16 #MPTCP_EVENT_LISTENER_CLOSED
> -
> -AF_INET=2
> -AF_INET6=10
> -
> -verify_listener_events()
> -{
> - local evt=$1
> - local e_type=$2
> - local e_family=$3
> - local e_saddr=$4
> - local e_sport=$5
> - local type
> - local family
> - local saddr
> - local sport
> - local name
> -
> - if [ $e_type = $LISTENER_CREATED ]; then
> - name="LISTENER_CREATED"
> - elif [ $e_type = $LISTENER_CLOSED ]; then
> - name="LISTENER_CLOSED "
> - else
> - name="$e_type"
> - fi
> -
> - print_check "$name $e_saddr:$e_sport"
> -
> - if ! mptcp_lib_kallsyms_has "mptcp_event_pm_listener$"; then
> - print_skip "event not supported"
> - return
> - fi
> -
> - type=$(mptcp_lib_evts_get_info type "$evt" "$e_type")
> - family=$(mptcp_lib_evts_get_info family "$evt" "$e_type")
> - sport=$(mptcp_lib_evts_get_info sport "$evt" "$e_type")
> - if [ $family ] && [ $family = $AF_INET6 ]; then
> - saddr=$(mptcp_lib_evts_get_info saddr6 "$evt" "$e_type")
> - else
> - saddr=$(mptcp_lib_evts_get_info saddr4 "$evt" "$e_type")
> - fi
> -
> - if [ $type ] && [ $type = $e_type ] &&
> - [ $family ] && [ $family = $e_family ] &&
> - [ $saddr ] && [ $saddr = $e_saddr ] &&
> - [ $sport ] && [ $sport = $e_sport ]; then
> - print_ok
> - return 0
> - fi
> - fail_test "$e_type:$type $e_family:$family $e_saddr:$saddr $e_sport:$sport"
It is important to call fail_test() because that's what is going to mark
the subtest (and the whole test) as failed.
> -}
> -
> add_addr_ports_tests()
> {
> # signal address with port
> @@ -2891,8 +2838,10 @@ add_addr_ports_tests()
> chk_add_nr 1 1 1
> chk_rm_nr 1 1 invert
>
> - verify_listener_events $server_evts $LISTENER_CREATED $AF_INET 10.0.2.1 10100
> - verify_listener_events $server_evts $LISTENER_CLOSED $AF_INET 10.0.2.1 10100
> + mptcp_lib_verify_listener_events $server_evts $LISTENER_CREATED \
> + $AF_INET 10.0.2.1 10100
> + mptcp_lib_verify_listener_events $server_evts $LISTENER_CLOSED \
> + $AF_INET 10.0.2.1 10100
> mptcp_lib_evts_kill
> fi
>
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 899c21ced5a3..1301af71ad2c 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -9,6 +9,11 @@ readonly KSFT_SKIP=4
> readonly KSFT_TEST=$(basename "${0}" | sed 's/\.sh$//g')
>
> SUB_ESTABLISHED=10 # MPTCP_EVENT_SUB_ESTABLISHED
> +LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
> +LISTENER_CLOSED=16 #MPTCP_EVENT_LISTENER_CLOSED
Same as SUB_ESTABLISHED, it would be better to prefix that with
MPTCP_LIB_ if it is declared here or keep it declared elsewhere.
> +
> +AF_INET=2
> +AF_INET6=10
For these two, I think it is fine without MPTCP_LIB_ but we should add
'readonly' keyword.
>
> MPTCP_LIB_SUBTESTS=()
>
> @@ -267,3 +272,54 @@ mptcp_lib_evts_kill() {
> mptcp_lib_evts_remove() {
> rm -rf $server_evts $client_evts
> }
> +
> +mptcp_lib_verify_listener_events() {
> + local evt=$1
> + local e_type=$2
> + local e_family=$3
> + local e_saddr=$4
> + local e_sport=$5
> + local type
> + local family
> + local saddr
> + local sport
> + local name
> +
> + if [ $e_type = $LISTENER_CREATED ]; then
> + name="LISTENER_CREATED"
> + elif [ $e_type = $LISTENER_CLOSED ]; then
> + name="LISTENER_CLOSED "
> + else
> + name="$e_type"
> + fi
> +
> + if [ "$(basename "$0")" == "mptcp_join.sh" ]; then
> + printf "%-6s%-36s" " " "$name $e_saddr:$e_sport"
> + elif [ "$(basename "$0")" == "userspace_pm.sh" ]; then
> + printf "%-63s" "$name $e_saddr:$e_sport"
> + fi
I think we should avoid such a thing in the lib. Instead, we can pass
information in a parameter.
> +
> + if ! mptcp_lib_kallsyms_has "mptcp_event_pm_listener$"; then
> + mptcp_lib_print_warn "[skip] event not supported"
> + return
> + fi
> +
> + type=$(mptcp_lib_evts_get_info type "$evt" "$e_type")
> + family=$(mptcp_lib_evts_get_info family "$evt" "$e_type")
> + sport=$(mptcp_lib_evts_get_info sport "$evt" "$e_type")
> + if [ $family ] && [ $family = $AF_INET6 ]; then
> + saddr=$(mptcp_lib_evts_get_info saddr6 "$evt" "$e_type")
> + else
> + saddr=$(mptcp_lib_evts_get_info saddr4 "$evt" "$e_type")
> + fi
> +
> + if [ $type ] && [ $type = $e_type ] &&
> + [ $family ] && [ $family = $e_family ] &&
> + [ $saddr ] && [ $saddr = $e_saddr ] &&
> + [ $sport ] && [ $sport = $e_sport ]; then
> + mptcp_lib_print_ok "[ ok ]"
> + return 0
> + fi
> + mptcp_lib_print_err "[fail] $e_type:$type $e_family:$family \
> + $e_saddr:$saddr $e_sport:$sport"
That's not enough to just print a WARN, OK or ERR, some extra actions
needs to be taken, e.g. marking the subtest as failed, same for the
whole selftest.
> +}
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index 84a77ee3b633..98189b3f73dc 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -26,11 +26,6 @@ fi
> ANNOUNCED=6 # MPTCP_EVENT_ANNOUNCED
> REMOVED=7 # MPTCP_EVENT_REMOVED
> SUB_CLOSED=11 # MPTCP_EVENT_SUB_CLOSED
> -LISTENER_CREATED=15 #MPTCP_EVENT_LISTENER_CREATED
> -LISTENER_CLOSED=16 #MPTCP_EVENT_LISTENER_CLOSED
> -
> -AF_INET=2
> -AF_INET6=10
>
> file=""
> client4_pid=0
> @@ -878,36 +873,6 @@ test_prio()
> fi
> }
>
> -verify_listener_events()
> -{
> - local evt=$1
> - local e_type=$2
> - local e_family=$3
> - local e_saddr=$4
> - local e_sport=$5
> - local type
> - local family
> - local saddr
> - local sport
> -
> - if [ $e_type = $LISTENER_CREATED ]; then
> - print_test "CREATE_LISTENER $e_saddr:$e_sport"
> - elif [ $e_type = $LISTENER_CLOSED ]; then
> - print_test "CLOSE_LISTENER $e_saddr:$e_sport"
> - fi
> -
> - type=$(mptcp_lib_evts_get_info type $evt $e_type)
> - family=$(mptcp_lib_evts_get_info family $evt $e_type)
> - sport=$(mptcp_lib_evts_get_info sport $evt $e_type)
> - if [ $family ] && [ $family = $AF_INET6 ]; then
> - saddr=$(mptcp_lib_evts_get_info saddr6 $evt $e_type)
> - else
> - saddr=$(mptcp_lib_evts_get_info saddr4 $evt $e_type)
> - fi
> -
> - check_expected "type" "family" "saddr" "sport"
This a good function to call because it will tell you which parameter is
wrong. In the new helper, we will need to do the comparison ourselves
(and the subtest will not be marked as failed).
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net