On Fri, Nov 24, 2023 at 03:35:51PM +0100, Petr Machata wrote:
>
> Hangbin Liu <[email protected]> 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
