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

[Matthieu Baerts <matttbe@kernel.org>]

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.

(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).

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.