On Thu, May 9, 2024 at 5:38 PM Simon Horman <horms@xxxxxxxxxx> wrote:
>
Hi Simon,
Thanks a lot for the review!
> On Wed, May 08, 2024 at 04:06:43AM +0000, Taehee Yoo wrote:
> > In the forwarding testcase, it opens a server and a client with the nc.
> > The server receives the correct message from NC, it prints OK.
> > The server prints FAIL if it receives the wrong message from the client.
> >
> > But If the server can't receive any message, it will not close so
> > the amt.sh waits forever.
> > There are several reasons.
> > 1. crash of smcrouted.
> > 2. Send a message from the client to the server before the server is up.
> >
> > To avoid this problem, the server waits only for 10 seconds.
> > The client sends messages for 10 seconds.
> > If the server is successfully closed, it kills the client.
> >
> > Fixes: c08e8baea78e ("selftests: add amt interface selftest script")
> > Signed-off-by: Taehee Yoo <ap420073@xxxxxxxxx>
> > ---
> > tools/testing/selftests/net/amt.sh | 63 +++++++++++++++++++-----------
> > 1 file changed, 40 insertions(+), 23 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/amt.sh b/tools/testing/selftests/net/amt.sh
> > index 75528788cb95..16641d3dccce 100755
> > --- a/tools/testing/selftests/net/amt.sh
> > +++ b/tools/testing/selftests/net/amt.sh
> > @@ -77,6 +77,7 @@ readonly LISTENER=$(mktemp -u listener-XXXXXXXX)
> > readonly GATEWAY=$(mktemp -u gateway-XXXXXXXX)
> > readonly RELAY=$(mktemp -u relay-XXXXXXXX)
> > readonly SOURCE=$(mktemp -u source-XXXXXXXX)
> > +readonly RESULT=$(mktemp -p /tmp amt-XXXXXXXX)
> > ERR=4
> > err=0
> >
> > @@ -85,6 +86,10 @@ exit_cleanup()
> > for ns in "$@"; do
> > ip netns delete "${ns}" 2>/dev/null || true
> > done
> > + rm $RESULT
> > + smcpid=$(< $SMCROUTEDIR/amt.pid)
> > + kill $smcpid
> > + rm -rf $SMCROUTEDIR
>
> Hi Taehee Yoo,
>
> I think this cleanup may be executed before SMCROUTEDIR exists.
>
> For consistency with other temp files, perhaps
> perpahps it is best to move the creation of SMCROUTEDIR up
> to where RESULT is instantiated above.
>
> And perhaps the pid handling can be made conditional on the
> existence of $SMCROUTEDIR/amt.pid
>
> if [ -f "$SMCROUTEDIR/amt.pid" ]; then
> ...
> fi
>
Thanks!
I will check a pid file before kills smcrouted.
> >
> > exit $ERR
> > }
> > @@ -167,7 +172,9 @@ setup_iptables()
> >
> > setup_mcast_routing()
> > {
> > - ip netns exec "${RELAY}" smcrouted
> > + SMCROUTEDIR="$(mktemp -d)"
> > +
> > + ip netns exec "${RELAY}" smcrouted -P $SMCROUTEDIR/amt.pid
> > ip netns exec "${RELAY}" smcroutectl a relay_src \
> > 172.17.0.2 239.0.0.1 amtr
> > ip netns exec "${RELAY}" smcroutectl a relay_src \
> > @@ -210,40 +217,52 @@ check_features()
> >
> > test_ipv4_forward()
> > {
> > - RESULT4=$(ip netns exec "${LISTENER}" nc -w 1 -l -u 239.0.0.1 4000)
> > + echo "" > $RESULT
> > + bash -c "$(ip netns exec "${LISTENER}" \
> > + timeout 10s nc -w 1 -l -u 239.0.0.1 4000 > $RESULT)"
>
> Hi,
>
> It's unclear to me what the purpose of the bash -c "$(...)" construction is
> here. Can the same be achieved using simply:
>
> ip netns exec "${LISTENER}" \
> timeout 10s nc -w 1 -l -u 239.0.0.1 4000 > $RESULT
>
The purpose of using bash -s was to avoid exiting main bash program
by timeout expiration due to 'set -e' option.
But Jakub avoided that problem by adding (|| true) in the recent patch.
> Also, not strictly related to this patch, it seems a little odd here, and
> elsewhere, to call bash in a /bin/sh script.
>
Oh Thanks,
Shebang should be bash, not sh.
I will fix it.
> > + RESULT4=$(< $RESULT)
> > if [ "$RESULT4" == "172.17.0.2" ]; then
> > printf "TEST: %-60s [ OK ]\n" "IPv4 amt multicast forwarding"
> > - exit 0
> > else
> > printf "TEST: %-60s [FAIL]\n" "IPv4 amt multicast forwarding"
> > - exit 1
> > fi
> > +
> > }
>
> ...
>
> > send_mcast4()
> > {
> > sleep 2
> > - ip netns exec "${SOURCE}" bash -c \
> > - 'echo 172.17.0.2 | nc -w 1 -u 239.0.0.1 4000' &
> > + for n in {0..10}; do
> > + ip netns exec "${SOURCE}" bash -c \
> > + 'echo 172.17.0.2 | nc -w 1 -u 239.0.0.1 4000'
> > + sleep 1
> > + done
> > +
> > }
> >
> > send_mcast6()
> > {
> > sleep 2
> > - ip netns exec "${SOURCE}" bash -c \
> > - 'echo 2001:db8:3::2 | nc -w 1 -u ff0e::5:6 6000' &
> > + for n in {0..10}; do
> > + ip netns exec "${SOURCE}" bash -c \
> > + 'echo 2001:db8:3::2 | nc -w 1 -u ff0e::5:6 6000'
> > + sleep 1
> > + done
> > +
> > }
> >
> > check_features
>
> ...
>
> --
> pw-bot: under-review
Thanks a lot!
Taehee Yoo
