On Thu, Sep 24, 2026 at 11:57:58AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
>
> Low priority / nit picks for your consideration:
>
> - [Low] Mid-run re-arming of the SETUP_ERR latch converts already-
> recorded test FAILures into an overall SKIP.
OK, I will update the commit description.
> - [Low] setup_end_flv_psp()
> (tools/testing/selftests/net/srv6_end_flavors_test.sh:651-670) is…
ack, I will move the ret guard to this patch.
Thanks
Hangbin
>
> --
>
> selftests: seg6: move SRv6 End PSP setup into its own helper
>
> This moves the End PSP policy installation out of setup() into a new
> setup_end_flv_psp() helper, and calls that helper from
> host_srv6_end_flv_psp_tests() with errexit enabled around it. The intent
> is to decouple the base topology setup from the flavor-specific test case.
>
> > diff --git a/tools/testing/selftests/net/srv6_end_flavors_test.sh
> > b/tools/testing/selftests/net/srv6_end_flavors_test.sh
> > index 318487eda671b..56a1c5f865a77 100755
> > --- a/tools/testing/selftests/net/srv6_end_flavors_test.sh
> > +++ b/tools/testing/selftests/net/srv6_end_flavors_test.sh
> > @@ -645,6 +645,12 @@ setup()
> > setup_rt_local_sids 3 "1 2 4"
> > setup_rt_local_sids 4 "1 2 3"
> >
> > + # testing environment was set up successfully
> > + SETUP_ERR=0
> > +}
> > +
> > +setup_end_flv_psp()
> > +{
> > # set up SRv6 policies
> > # create a connection between hosts hs-1 and hs-2.
> > # The path between hs-1 and hs-2 traverses SRv6 aware routers.
>
> [ ... ]
>
> > # - rt-1 (SRv6 End flavor PSP with SL=1)
> > setup_rt_policy_ipv6 2 1 "3:noflv 4:psp 2:psp"
> > setup_rt_policy_ipv6 1 2 "1:psp"
> > -
> > - # testing environment was set up successfully
> > - SETUP_ERR=0
> > }
>
> [Severity: Low]
> The commit message says the split lets "each PSP test case can setup and
> later cleanup independently", but this commit only adds the setup half.
> setup_end_flv_psp() installs the localsid seg6local routes, the cafe::<dst>
> H.Insert route and the proxy NDP entries through __setup_rt_policy(), and
> nothing releases them except the global EXIT trap cleanup(), which just
> does:
>
> for i in ${ROUTERS}; do
> nsname="$(get_rtname "${i}")"
> ip netns del "${nsname}" &>/dev/null || true
> done
>
> Would it make sense to mention in the commit message that the matching
> cleanup_end_flv_psp() arrives in the following patch ("selftest: seg6: add
> explicit cleanup for SRv6 End PSP test")? As it stands the description
> promises a teardown path that does not exist yet at this commit. There is no
> functional problem here, since the two PSP policies are the only flavor
> state installed and netns deletion reclaims everything.
>
> >
> > check_rt_connectivity()
> > @@ -758,7 +761,13 @@ host2gateway_tests()
> >
> > host_srv6_end_flv_psp_tests()
> > {
> > - log_section "SRv6 connectivity test hosts (h1 <-> h2, PSP flavor)"
> > + set -e
> > + SETUP_ERR=1
> > + setup_end_flv_psp
> > + SETUP_ERR=0
> > + set +e
>
> [Severity: Low]
> Can re-arming SETUP_ERR here turn already-recorded failures into a skip?
>
> By the time host_srv6_end_flv_psp_tests() runs, router_tests() and
> host2gateway_tests() have already executed and may have set ret=1 and
> incremented nfail:
>
> setup
> set +e
>
> router_tests
> host2gateway_tests
> host_srv6_end_flv_psp_tests
>
> print_log_test_results
>
> If any ip command inside setup_end_flv_psp() fails, for example the
> "encap seg6local action End flavors psp" route add, errexit aborts the
> script while SETUP_ERR is still 1, and the EXIT trap takes this path in
> cleanup():
>
> if [ "${SETUP_ERR}" -ne 0 ]; then
> echo "SKIP: Setting up the testing environment failed"
> exit "${ksft_skip}"
> fi
>
> exit "${ret}"
>
> so the script exits 4 (skip) instead of reaching exit ${ret} with ret=1, and
> print_log_test_results is never reached, meaning the tests passed/failed
> summary is lost too.
>
> The final patch of this series ("selftests: seg6: add End.X PSP selftest")
> changes the guard to also require [ "$ret" -ne "$ksft_fail" ], which removes
> the fail-to-skip masking. Could that guard change be moved earlier in the
> series, so the intermediate commits do not carry the masking behaviour?
>
> > +
> > + log_section "SRv6 connectivity test hosts (h1 <-> h2, End flavor PSP)"
> >
> > check_and_log_hs_connectivity 1 2
> > check_and_log_hs_connectivity 2 1
>
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-srv6_flavors_endx-v1-0-4d01c05c47b7%40kylinos.cn
