Re: [PATCH net-next 01/38] selftests/net: add lib.sh
Hangbin Liu writes:
> On Fri, Nov 24, 2023 at 03:35:51PM +0100, Petr Machata wrote:
>>
>> Hangbin Liu writes:
>>
>> > + fi
>> > + done
>> > +
>> > + [ $errexit -eq 1 ] && set -e
>> > + return 0
>> > +}
>> > +
>> > +# By default, remove all netns before EXIT.
>> > +cleanup_all_ns()
>> > +{
>> > + cleanup_ns $NS_LIST
>> > +}
>> > +trap cleanup_all_ns EXIT
>>
>> Hmm, OK, this is a showstopper for inclusion from forwarding/lib.sh,
>> because basically all users of forwarding/lib.sh use the EXIT trap.
>>
>> I wonder if we need something like these push_cleanup / on_exit helpers:
>>
>> https://github.com/pmachata/stuff/blob/master/ptp-test/lib.sh#L15
>
> When I added this, I just want to make sure the netns are cleaned up if the
> client script forgot. I think the client script trap function should
> cover this one, no?
So the motivation makes sense. But in general, invoking cleanup from the
same abstraction layer that acquired the resource, makes the code easier
to analyze. And in particular here that we are talking about traps,
which are a global resource, and one that the client might well want to
use for their own management. The client should be using the trap
instead of the framework.
The framework might expose APIs to allow clients to register cleanups
etc., which the framework itself is then free to use of course, for
resources that it itself has acquired. But even with these APIs in place
I think it would be better if the client that acquires a resource also
schedules its release. (Though it's not as clear-cut in that case.)
>>
>> But I don't want to force this on your already large patchset :)
>
> Yes, Paolo also told me that this is too large. I will break it to
> 2 path set or merge some small patches together for next version.
>
>> So just ignore the bit about including from forwarding/lib.sh.
>
>> Actually I take this back. The cleanup should be invoked from where the
>> init was called. I don't think the library should be auto-invoking it,
>> the client scripts should. Whether through a trap or otherwise.
>
> OK, also makes sense. I will remove this trap.
>
> Thanks for all your comments.
> Hangbin
Re: [PATCH net-next 01/38] selftests/net: add lib.sh
Hangbin Liu writes:
> On Fri, Nov 24, 2023 at 03:05:18PM +0100, Petr Machata wrote:
>>
>> Hangbin Liu writes:
>>
>> > +# Helpers
>> > +busywait()
>> > +{
>> > + local timeout=$1; shift
>> > +
>> > + local start_time="$(date -u +%s%3N)"
>> > + while true
>> > + do
>> > + local out
>> > + out=$($@)
>> > + local ret=$?
>> > + if ((!ret)); then
>> > + echo -n "$out"
>> > + return 0
>> > + fi
>> > +
>> > + local current_time="$(date -u +%s%3N)"
>> > + if ((current_time - start_time > timeout)); then
>> > + echo -n "$out"
>> > + return 1
>> > + fi
>> > + done
>> > +}
>>
>> This is lifted from forwarding/lib.sh, right? Would it make sense to
>
> Yes.
>
>> just source this new file from forwarding/lib.sh instead of copying
>
> Do you mean let net/forwarding/lib.sh source net.lib, and let other net
> tests source the net/forwarding/lib.sh?
>
> Or move the busywait() function from net/forwarding/lib.sh to net.lib.
> Then let net/forwarding/lib.sh source net.lib?
This.
>> stuff around? I imagine there will eventually be more commonality, and
>> when that pops up, we can just shuffle the forwarding code to
>> net/lib.sh.
>
> Yes, make sense.
>
> Thanks
> Hangbin
Re: [PATCH net-next 01/38] selftests/net: add lib.sh
On Fri, Nov 24, 2023 at 03:35:51PM +0100, Petr Machata wrote:
>
> Hangbin Liu writes:
>
> > +cleanup_ns()
> > +{
> > + local ns=""
> > + local errexit=0
> > +
> > + # disable errexit temporary
> > + if [[ $- =~ "e" ]]; then
> > + errexit=1
> > + set +e
> > + fi
> > +
> > + for ns in "$@"; do
> > + ip netns delete "${ns}" &> /dev/null
> > + busywait 2 "ip netns list | grep -vq $1" &> /dev/null
>
> The grep would get confused by substrings of other names.
> This should be grep -vq "^$ns$".
Thanks. I just thought the ns name would like foo-xxx, but I forgot this
is a common function, which maybe called with normal ns name.
>
> > + if ip netns list | grep -q $1; then
>
> Busywait returns != 0 when the wait condition is not reached within a
> given time. So it should be possible to roll the duplicated if-grep into
> the busywait line like so:
>
> if ! busywait 2 "ip netns etc."; then
You are right.
>
> > + echo "Failed to remove namespace $1"
> > + return $ksft_skip
>
> This does not restore the errexit.
>
> I think it might be clearest to have this function as a helper, say
> __cleanup_ns, and then have a wrapper that does the errexit management:
>
> cleanup_ns()
> {
> local errexit
> local rc
>
> # disable errexit temporarily
> if [[ $- =~ "e" ]]; then
> errexit=1
> set +e
> fi
>
> __cleanup_ns "$@"
> rc=$?
>
> [ $errexit -eq 1 ] && set -e
> return $rc
> }
>
> If this comes up more often, we can have a helper like
> with_disabled_errexit or whatever, that does this management and
> dispatches to "$@", so cleanup_ns() would become:
>
> cleanup_ns()
> {
> with_disabled_errexit __cleanup_ns "$@"
> }
Thanks for your suggestion.
>
> > + fi
> > + done
> > +
> > + [ $errexit -eq 1 ] && set -e
> > + return 0
> > +}
> > +
> > +# By default, remove all netns before EXIT.
> > +cleanup_all_ns()
> > +{
> > + cleanup_ns $NS_LIST
> > +}
> > +trap cleanup_all_ns EXIT
>
> Hmm, OK, this is a showstopper for inclusion from forwarding/lib.sh,
> because basically all users of forwarding/lib.sh use the EXIT trap.
>
> I wonder if we need something like these push_cleanup / on_exit helpers:
>
> https://github.com/pmachata/stuff/blob/master/ptp-test/lib.sh#L15
When I added this, I just want to make sure the netns are cleaned up if the
client script forgot. I think the client script trap function should
cover this one, no?
>
> But I don't want to force this on your already large patchset :)
Yes, Paolo also told me that this is too large. I will break it to
2 path set or merge some small patches together for next version.
> So just ignore the bit about including from forwarding/lib.sh.
> Actually I take this back. The cleanup should be invoked from where the
> init was called. I don't think the library should be auto-invoking it,
> the client scripts should. Whether through a trap or otherwise.
OK, also makes sense. I will remove this trap.
Thanks for all your comments.
Hangbin
Re: [PATCH net-next 01/38] selftests/net: add lib.sh
On Fri, Nov 24, 2023 at 03:05:18PM +0100, Petr Machata wrote:
>
> Hangbin Liu writes:
>
> > Add a lib.sh for net selftests. This file can be used to define commonly
> > used variables and functions.
> >
> > Add function setup_ns() for user to create unique namespaces with given
> > prefix name.
> >
> > Signed-off-by: Hangbin Liu
> > ---
> > diff --git a/tools/testing/selftests/net/lib.sh
> > b/tools/testing/selftests/net/lib.sh
> > new file mode 100644
> > index ..239ab2beb438
> > --- /dev/null
> > +++ b/tools/testing/selftests/net/lib.sh
> > @@ -0,0 +1,98 @@
> > +#!/bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +##
> > +# Defines
> > +
> > +# Kselftest framework requirement - SKIP code is 4.
> > +ksft_skip=4
> > +# namespace list created by setup_ns
> > +NS_LIST=""
> > +
> > +##
> > +# Helpers
> > +busywait()
> > +{
> > + local timeout=$1; shift
> > +
> > + local start_time="$(date -u +%s%3N)"
> > + while true
> > + do
> > + local out
> > + out=$($@)
> > + local ret=$?
> > + if ((!ret)); then
> > + echo -n "$out"
> > + return 0
> > + fi
> > +
> > + local current_time="$(date -u +%s%3N)"
> > + if ((current_time - start_time > timeout)); then
> > + echo -n "$out"
> > + return 1
> > + fi
> > + done
> > +}
>
> This is lifted from forwarding/lib.sh, right? Would it make sense to
Yes.
> just source this new file from forwarding/lib.sh instead of copying
Do you mean let net/forwarding/lib.sh source net.lib, and let other net
tests source the net/forwarding/lib.sh?
Or move the busywait() function from net/forwarding/lib.sh to net.lib.
Then let net/forwarding/lib.sh source net.lib?
> stuff around? I imagine there will eventually be more commonality, and
> when that pops up, we can just shuffle the forwarding code to
> net/lib.sh.
Yes, make sense.
Thanks
Hangbin
Re: [PATCH net-next 01/38] selftests/net: add lib.sh
Petr Machata writes:
> Hangbin Liu writes:
>
>> +# By default, remove all netns before EXIT.
>> +cleanup_all_ns()
>> +{
>> +cleanup_ns $NS_LIST
>> +}
>> +trap cleanup_all_ns EXIT
>
> Hmm, OK, this is a showstopper for inclusion from forwarding/lib.sh,
> because basically all users of forwarding/lib.sh use the EXIT trap.
[...]
> So just ignore the bit about including from forwarding/lib.sh.
Actually I take this back. The cleanup should be invoked from where the
init was called. I don't think the library should be auto-invoking it,
the client scripts should. Whether through a trap or otherwise.
Re: [PATCH net-next 01/38] selftests/net: add lib.sh
Hangbin Liu writes:
> +cleanup_ns()
> +{
> + local ns=""
> + local errexit=0
> +
> + # disable errexit temporary
> + if [[ $- =~ "e" ]]; then
> + errexit=1
> + set +e
> + fi
> +
> + for ns in "$@"; do
> + ip netns delete "${ns}" &> /dev/null
> + busywait 2 "ip netns list | grep -vq $1" &> /dev/null
The grep would get confused by substrings of other names.
This should be grep -vq "^$ns$".
> + if ip netns list | grep -q $1; then
Busywait returns != 0 when the wait condition is not reached within a
given time. So it should be possible to roll the duplicated if-grep into
the busywait line like so:
if ! busywait 2 "ip netns etc."; then
> + echo "Failed to remove namespace $1"
> + return $ksft_skip
This does not restore the errexit.
I think it might be clearest to have this function as a helper, say
__cleanup_ns, and then have a wrapper that does the errexit management:
cleanup_ns()
{
local errexit
local rc
# disable errexit temporarily
if [[ $- =~ "e" ]]; then
errexit=1
set +e
fi
__cleanup_ns "$@"
rc=$?
[ $errexit -eq 1 ] && set -e
return $rc
}
If this comes up more often, we can have a helper like
with_disabled_errexit or whatever, that does this management and
dispatches to "$@", so cleanup_ns() would become:
cleanup_ns()
{
with_disabled_errexit __cleanup_ns "$@"
}
> + fi
> + done
> +
> + [ $errexit -eq 1 ] && set -e
> + return 0
> +}
> +
> +# By default, remove all netns before EXIT.
> +cleanup_all_ns()
> +{
> + cleanup_ns $NS_LIST
> +}
> +trap cleanup_all_ns EXIT
Hmm, OK, this is a showstopper for inclusion from forwarding/lib.sh,
because basically all users of forwarding/lib.sh use the EXIT trap.
I wonder if we need something like these push_cleanup / on_exit helpers:
https://github.com/pmachata/stuff/blob/master/ptp-test/lib.sh#L15
But I don't want to force this on your already large patchset :)
So just ignore the bit about including from forwarding/lib.sh.
> +# setup netns with given names as prefix. e.g
> +# setup_ns local remote
> +setup_ns()
> +{
> + local ns=""
> + # the ns list we created in this call
> + local ns_list=""
> + while [ -n "$1" ]; do
I would find it more readable if this used the same iteration approach
as the 'for ns in "$@"' above. The $1/shift approach used here is
somewhat confusing.
> + # Some test may setup/remove same netns multi times
> + if unset $1 2> /dev/null; then
> + ns="${1,,}-$(mktemp -u XX)"
> + eval readonly $1=$ns
> + else
> + eval ns='$'$1
> + cleanup_ns $ns
> +
> + fi
> +
> + ip netns add $ns
> + if ! ip netns list | grep -q $ns; then
As above, the grep could get confused. But in fact wouldn't just
checking the exit code of ip netns add be enough?
> + echo "Failed to create namespace $1"
> + cleanup_ns $ns_list
> + return $ksft_skip
> + fi
> + ip -n $ns link set lo up
> + ns_list="$ns_list $ns"
> +
> + shift
> + done
> + NS_LIST="$NS_LIST $ns_list"
> +}
Re: [PATCH net-next 01/38] selftests/net: add lib.sh
Hangbin Liu writes:
> Add a lib.sh for net selftests. This file can be used to define commonly
> used variables and functions.
>
> Add function setup_ns() for user to create unique namespaces with given
> prefix name.
>
> Signed-off-by: Hangbin Liu
> ---
> tools/testing/selftests/net/Makefile | 2 +-
> tools/testing/selftests/net/lib.sh | 98
> 2 files changed, 99 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/net/lib.sh
>
> diff --git a/tools/testing/selftests/net/Makefile
> b/tools/testing/selftests/net/Makefile
> index 9274edfb76ff..14bd68da7466 100644
> --- a/tools/testing/selftests/net/Makefile
> +++ b/tools/testing/selftests/net/Makefile
> @@ -54,7 +54,7 @@ TEST_PROGS += ip_local_port_range.sh
> TEST_PROGS += rps_default_mask.sh
> TEST_PROGS += big_tcp.sh
> TEST_PROGS_EXTENDED := in_netns.sh setup_loopback.sh setup_veth.sh
> -TEST_PROGS_EXTENDED += toeplitz_client.sh toeplitz.sh
> +TEST_PROGS_EXTENDED += toeplitz_client.sh toeplitz.sh lib.sh
> TEST_GEN_FILES = socket nettest
> TEST_GEN_FILES += psock_fanout psock_tpacket msg_zerocopy reuseport_addr_any
> TEST_GEN_FILES += tcp_mmap tcp_inq psock_snd txring_overwrite
> diff --git a/tools/testing/selftests/net/lib.sh
> b/tools/testing/selftests/net/lib.sh
> new file mode 100644
> index ..239ab2beb438
> --- /dev/null
> +++ b/tools/testing/selftests/net/lib.sh
> @@ -0,0 +1,98 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +##
> +# Defines
> +
> +# Kselftest framework requirement - SKIP code is 4.
> +ksft_skip=4
> +# namespace list created by setup_ns
> +NS_LIST=""
> +
> +##
> +# Helpers
> +busywait()
> +{
> + local timeout=$1; shift
> +
> + local start_time="$(date -u +%s%3N)"
> + while true
> + do
> + local out
> + out=$($@)
> + local ret=$?
> + if ((!ret)); then
> + echo -n "$out"
> + return 0
> + fi
> +
> + local current_time="$(date -u +%s%3N)"
> + if ((current_time - start_time > timeout)); then
> + echo -n "$out"
> + return 1
> + fi
> + done
> +}
This is lifted from forwarding/lib.sh, right? Would it make sense to
just source this new file from forwarding/lib.sh instead of copying
stuff around? I imagine there will eventually be more commonality, and
when that pops up, we can just shuffle the forwarding code to
net/lib.sh.
