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

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