Re: [PATCH mptcp-next v6 5/9] selftests: net: lib: remove ns from list after clean-up

[Geliang Tang <geliang@kernel.org>]

On Tue, 2024-05-28 at 17:26 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 28/05/2024 15:18, Geliang Tang wrote:
> > On Tue, 2024-05-28 at 12:38 +0200, Matthieu Baerts wrote:
> > > Hi Geliang,
> > > 
> > > Thank you for your review!
> > > 
> > > On 28/05/2024 05:47, Geliang Tang wrote:
> > > > On Mon, 2024-05-27 at 12:58 +0200, Matthieu Baerts (NGI0)
> > > > wrote:
> > > > > Instead of only appending items to the list, remove 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.
> > > > > 
> > > > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > > > ---
> > > > >  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
> > > > 
> > > > nit: Should we also remove ns_list when "ip netns list" is
> > > > busy? Or
> > > > should we not delete ns when "ip netns list" is busy?
> > > > 
> > > > I think we should do these two commands together:
> > > > 
> > > > 	ip netns delete "${ns}" &> /dev/null || true
> > > > 	remove_ns_list "${ns}"
> > > > 
> > > > I'm not sure. WDYT?
> > > 
> > > Good point, I'm not sure either. I kept it in the list because in
> > > case
> > > of errors, the caller could still get the list of NS that have
> > > been
> > > created, but not deleted yet.
> > > 
> > > To be honest, I don't think any actions should be done on the NS
> > > after
> > > having called cleanup: it would probably better to kill all
> > > attached
> > > processes before that, and not retry later after errors and a
> > > kill.
> > > But
> > > I didn't want to change this logic as there might be other
> > > impacts.
> > > So
> > > at least here, we let the responsibility to the caller, and it
> > > can
> > > call
> > > cleanup_all_ns() again in case of errors. WDYT?
> > 
> > If so, I think this is better:
> > 
> >         for ns in "$@"; do
> >                 [ -z "${ns}" ] && continue
> >                 ip netns delete "${ns}" &> /dev/null || true
> >                 remove_ns_list "${ns}"
> >                 if ! busywait $BUSYWAIT_TIMEOUT ip netns list \|
> > grep -
> > vq "^$ns$" &> /dev/null; then
> >                         echo "Warn: Failed to remove namespace $ns"
> >                         ret=1
> >                 fi  
> >         done
> 
> Sorry, I was not clear enough: I wanted to say that I think it is
> better
> to keep "remove_ns_list" in the "else", only to remove the netns from
> the list if it has been deleted. By doing that, the caller can call
> cleanup_all_ns() again, after having stopped everything.

Yes, you're right, it's better to keep "remove_ns_list" in the "else".
My bad, I misunderstood it. Please apply this patch as is.

> 
> (That's a detail, I guess the removal should not fail, and if it
> does,
> the caller should adapt the code to make sure the clean doesn't
> fail.)
> 
> > If you agree, please update this when merging it. No need to send a
> > v7.
> 
> Thanks! I will wait for your feedback on what is above before
> applying
> this series (even if we can always have squash-to patches later).

I changed this series as "Queued" on patchwork.

Thanks,
-Geliang

> 
> Cheers,
> Matt