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 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

If you agree, please update this when merging it. No need to send a v7.

And please add my Reviewd-by tag. I'll repay it in the cover-letter.

Thanks,
-Geliang

> 
> Cheers,
> Matt