Instead of only appending items to the list, removing them when the
netns has been deleted.
By doing that, we can make sure 'cleanup_all_ns()' is not trying to
remove already deleted netns.
Reviewed-by: Geliang Tang <[email protected]>
Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
---
tools/testing/selftests/net/lib.sh | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
index b2572aff6286..c7a8cfb477cc 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -125,6 +125,20 @@ slowwait_for_counter()
slowwait "$timeout" until_counter_is ">= $((base + delta))" "$@"
}
+remove_ns_list()
+{
+ local item=$1
+ local ns
+ local ns_list=("${NS_LIST[@]}")
+ NS_LIST=()
+
+ for ns in "${ns_list[@]}"; do
+ if [ "${ns}" != "${item}" ]; then
+ NS_LIST+=("${ns}")
+ fi
+ done
+}
+
cleanup_ns()
{
local ns=""
@@ -136,6 +150,8 @@ cleanup_ns()
if ! busywait $BUSYWAIT_TIMEOUT ip netns list \| grep -vq "^$ns$" &> /dev/null; then
echo "Warn: Failed to remove namespace $ns"
ret=1
+ else
+ remove_ns_list "${ns}"
fi
done
@@ -154,17 +170,14 @@ setup_ns()
local ns=""
local ns_name=""
local ns_list=()
- local ns_exist=
for ns_name in "$@"; do
# Some test may setup/remove same netns multi times
if unset ${ns_name} 2> /dev/null; then
ns="${ns_name,,}-$(mktemp -u XXXXXX)"
eval readonly ${ns_name}="$ns"
- ns_exist=false
else
eval ns='$'${ns_name}
cleanup_ns "$ns"
- ns_exist=true
fi
if ! ip netns add "$ns"; then
@@ -173,7 +186,7 @@ setup_ns()
return $ksft_skip
fi
ip -n "$ns" link set lo up
- ! $ns_exist && ns_list+=("$ns")
+ ns_list+=("$ns")
done
NS_LIST+=("${ns_list[@]}")
}
--
2.43.0
On Fri, Jun 07, 2024 at 06:31:03PM +0200, Matthieu Baerts (NGI0) wrote:
> Instead of only appending items to the list, removing them when the
> netns has been deleted.
>
> By doing that, we can make sure 'cleanup_all_ns()' is not trying to
> remove already deleted netns.
>
> Reviewed-by: Geliang Tang <[email protected]>
> Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
Hi Matthieu,
I like this, and I am happy to see that it has been accepted.
I do wonder if we can go a step further and use an associative array for
ns_list (maybe renamed). I think this would reduce remove_ns_list to
something like:
unset ns_list["$item"]
OTOH, perhaps this breaks with older versions of bash that we still
care about.
...
Hi Simon,
Thank you for your reply!
On 14/06/2024 12:40, Simon Horman wrote:
> On Fri, Jun 07, 2024 at 06:31:03PM +0200, Matthieu Baerts (NGI0) wrote:
>> Instead of only appending items to the list, removing them when the
>> netns has been deleted.
>>
>> By doing that, we can make sure 'cleanup_all_ns()' is not trying to
>> remove already deleted netns.
>
> I do wonder if we can go a step further and use an associative array for
> ns_list (maybe renamed). I think this would reduce remove_ns_list to
> something like:
>
> unset ns_list["$item"]
I agree that it would ease the removal of one item -- which is not
complex to deal with the new helper :) -- but do you see any other benefits?
For the moment, there is no other value to associate with, so we would
do something like NS_MAP["$ns"]=1. We could link the name of the global
variable, but that's not needed for the tests for the moment.
Also, I don't know if it is important, but when we will iterate over the
list of netns, it will not be done following the same order items have
been added into the hashmap. So we will change the order in which items
are deleted.
> OTOH, perhaps this breaks with older versions of bash that we still
> care about.
Good point. I don't have the answer, but associative arrays are starting
to be quite old now :)
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
Hi Matthieu,
Likewise, thanks for your response.
On Fri, Jun 14, 2024 at 04:42:38PM +0200, Matthieu Baerts wrote:
> Hi Simon,
>
> Thank you for your reply!
>
> On 14/06/2024 12:40, Simon Horman wrote:
> > On Fri, Jun 07, 2024 at 06:31:03PM +0200, Matthieu Baerts (NGI0) wrote:
> >> Instead of only appending items to the list, removing them when the
> >> netns has been deleted.
> >>
> >> By doing that, we can make sure 'cleanup_all_ns()' is not trying to
> >> remove already deleted netns.
> >
> > I do wonder if we can go a step further and use an associative array for
> > ns_list (maybe renamed). I think this would reduce remove_ns_list to
> > something like:
> >
> > unset ns_list["$item"]
>
> I agree that it would ease the removal of one item -- which is not
> complex to deal with the new helper :) -- but do you see any other benefits?
>
> For the moment, there is no other value to associate with, so we would
> do something like NS_MAP["$ns"]=1. We could link the name of the global
> variable, but that's not needed for the tests for the moment.
>
> Also, I don't know if it is important, but when we will iterate over the
> list of netns, it will not be done following the same order items have
> been added into the hashmap. So we will change the order in which items
> are deleted.
I agree that it would probably end up being a NS_MAP["$ns"]=1,
i.e. a dummy value as there is no natural one to use.
I had not considered the order issue.
And yes, the benefit I see would be limited to removal.
Which as you point out is not a terrible burden with the helper you added.
While, OTOH, my idea adds complexity and unknowns elsewhere.
So overall, perhaps it's best left as an idea for later.
As the code changes for other reasons (who knows what?)
an associative array may make more sense than it does now.
> > OTOH, perhaps this breaks with older versions of bash that we still
> > care about.
>
> Good point. I don't have the answer, but associative arrays are starting
> to be quite old now :)
Yes, I think so too.
But I also thought it was worth mentioning.