Hangbin Liu liuhangbin@gmail.com writes:
On Fri, Nov 24, 2023 at 03:35:51PM +0100, Petr Machata wrote:
Hangbin Liu liuhangbin@gmail.com writes:
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?
So the motivation makes sense. But in general, invoking cleanup from the same abstraction layer that acquired the resource, makes the code easier to analyze. And in particular here that we are talking about traps, which are a global resource, and one that the client might well want to use for their own management. The client should be using the trap instead of the framework.
The framework might expose APIs to allow clients to register cleanups etc., which the framework itself is then free to use of course, for resources that it itself has acquired. But even with these APIs in place I think it would be better if the client that acquires a resource also schedules its release. (Though it's not as clear-cut in that case.)
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