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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sat, 25 Nov 2023 14:15:36 +0800
From: Hangbin Liu <liuhangbin@...il.com>
To: Petr Machata <petrm@...dia.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Shuah Khan <shuah@...nel.org>, David Ahern <dsahern@...nel.org>,
	linux-kselftest@...r.kernel.org,
	Po-Hsu Lin <po-hsu.lin@...onical.com>,
	Guillaume Nault <gnault@...hat.com>,
	Björn Töpel <bjorn@...osinc.com>,
	Ryan Roberts <ryan.roberts@....com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Mark Brown <broonie@...nel.org>,
	Luis Chamberlain <mcgrof@...nel.org>
Subject: 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@...il.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