Paolo Abeni pabeni@redhat.com writes:
On Wed, 2023-12-06 at 13:32 +0100, Petr Machata wrote:
Paolo Abeni pabeni@redhat.com writes:
Side note for a possible follow-up: if you maintain $ns_list as global variable, and remove from such list the ns deleted by cleanup_ns, you could remove the cleanup trap from the individual test with something alike:
final_cleanup_ns() { cleanup_ns $ns_list }
trap final_cleanup_ns EXIT
No respin needed for the above, could be a follow-up if agreed upon.
If you propose this for the library then I'm against it. The exit trap is a global resource that the client scripts sometimes need to use as well, to do topology teardowns or just general cleanups. So either the library would have to provide APIs for cleanup management, or the trap is for exclusive use by clients. The latter is IMHO simpler.
Even the former would not be very complex:
TRAPS="" do_at_exit() { TRAPS="${TRAPS}$@;"
trap "${TRAPS}" EXIT}
And then use "do_at_exit <whatever>" instead of "trap <whatever> EXIT"
Yep. I mentioned this during v2 review:
https://github.com/pmachata/stuff/blob/master/ptp-test/lib.sh#L13
Not much code at all, though you need to convert all EXIT trap users to this contraption. Again, a mechanical process, just needs to be done.
It also puts the cleanups at the same place where the acquisition is prompted: the client allocates the NS, the client should prompt its cleanup.
I guess I could argue that the the script is asking the library to allocate the namespaces, and the library could take care of disposing them.
It could also be said that since the script asked for NS creation, the script should ask for NS disposal :)
But what I object against is that the library uses trap without having a way for user scripts to schedule at-exit work, because that's used literally everywhere in forwarding tests. If people are willing to do the conversion, I'm OK with that.
But I'm not pushing the proposed option, if there is no agreement no need for additional work ;)
Cheers,
Paolo