Re: [PATCH net-next 01/38] selftests/net: add lib.sh

[Hangbin Liu <liuhangbin@gmail.com>]

On Fri, Nov 24, 2023 at 03:35:51PM +0100, Petr Machata wrote:
> 
> Hangbin Liu <liuhangbin@gmail.com> 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-xxxxxxx, 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